Experimental client: use bundle #1396

jku · 2021-05-18T08:44:04Z

Use the MetadataBundle in the new Updater:

Remove MetadataWrapper
Update and expand tests
Stop using tuf.settings, store the constants in updater_rework.py
Use MetadataBundle to verify current set of metadata, remove all verification code from Updater
load local metadata from disk
persist new verified metadata to disk

jku · 2021-05-18T08:52:28Z

The constants could use some thought: We definitely don't want to keep using tuf.settings, but I guess the question is, should these be instance variables in Updater instead of module level ones:

MAX_ROOT_ROTATIONS = 32
MAX_DELEGATIONS = 32
DEFAULT_ROOT_MAX_LENGTH = 512000  # bytes
DEFAULT_TIMESTAMP_MAX_LENGTH = 16384  # bytes
DEFAULT_SNAPSHOT_MAX_LENGTH = 2000000  # bytes
DEFAULT_TARGETS_MAX_LENGTH = 5000000  # bytes

Maybe it does not hurt if they were instance variables instead. They don't even need to be optional constructor arguments (as initialization does not use any of them: caller can initialize the Updater and then modify the values).
I guess the fanciest option is that constructor takes an optional argument dataclass UpdaterOptions with those attributes ...

Use the MetadataBundle to verify metadata validity. * Updater now handles reading metadata files (from filesystem as well as network * Updater feeds bytes to MetadataBundle for verification * Updater persists data on disk after it had been verified Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

Add test for a refresh with just a local root.json. Remove unused code. Add docstrings for raised exceptions, add TODOs for the missing exception handling. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

tuf/client_rework/updater_rework.py

MVrachev · 2021-05-19T14:26:45Z

After I made consistent_snapshot optional for root here, I realized that the MetadataBundle was created before that.
Can we check that if consistent_snapshot is not provided everything will work with the update process?

sechkova

The constants could use some thought: We definitely don't want to keep using tuf.settings, but I guess the question is, should these be instance variables in Updater instead of module level ones:
MAX_ROOT_ROTATIONS = 32
MAX_DELEGATIONS = 32
DEFAULT_ROOT_MAX_LENGTH = 512000  # bytes
DEFAULT_TIMESTAMP_MAX_LENGTH = 16384  # bytes
DEFAULT_SNAPSHOT_MAX_LENGTH = 2000000  # bytes
DEFAULT_TARGETS_MAX_LENGTH = 5000000  # bytes
Maybe it does not hurt if they were instance variables instead. They don't even need to be optional constructor arguments (as initialization does not use any of them: caller can initialize the Updater and then modify the values).
I guess the fanciest option is that constructor takes an optional argument dataclass UpdaterOptions with those attributes ...

Given the proposed structure in #1397 (comment) where the idea is to expose only the Updater class in the client __init__ , probably the constants need to be inside the class (or the fancier options). However this can happen when solving #1397.

I left some minor comments but I in general I like how Updater looks now.

tests/test_updater_rework.py

tuf/client_rework/updater_rework.py

sechkova · 2021-05-19T14:12:11Z

tuf/client_rework/updater_rework.py

-                f"{settings.MAX_NUMBER_OF_DELEGATIONS}",
-                " delegations.",
+                f"{len(role_names)} roles left to visit, but allowed to ",
+                f"visit at most {MAX_DELEGATIONS} delegations.",
            )
            logger.debug(msg)


This trick of mine with logging needs to be reverted now that we agreed on %s-strings in the logging calls.
But not in this PR. I'll note it for myself.

Captured in #1400

sechkova · 2021-05-19T14:17:11Z

tuf/client_rework/updater_rework.py

-
-        return intermediate_targets
+            data = self._load_local_metadata(role)
+            self._bundle.update_delegated_targets(data, role, parent_role)


Using self._bundle.update_delegated_targets here leaves MetadataBundle.update_targets() unused.
Not a big deal but mentioning it.

jku · 2021-05-19T16:21:37Z

Addressed the resolved discussions. Also checked that consistent_snapshots=None seems to work fine (using a falsy test)

joshuagl

This looks very concise, nice. It took me a couple of reads to figure out why the fetching of metadata only happened in the exception handler of a load. That might want some attention in terms of comments and/or function names? 🤔

I also noticed there are some unhandled exceptions which I am not sure whether it is reasonable to leave those to a caller to handle or not. Would welcome thoughts on that.

Overall, very solid. LGTM.

tuf/client_rework/updater_rework.py

joshuagl · 2021-05-20T10:32:28Z

tuf/client_rework/updater_rework.py

+            self._bundle.update_snapshot(data)
+            self._persist_metadata("snapshot", data)


Here, and other _load_*() functions, the exceptions raised by MetadataBundle.update_snapshot and the open and write calls in _persist_metadata become errors the user must handle. The former seems reasonable enough, I'm less certain about the latter. Especially when get_one_valid_targetinfo() -> _preorder_depth_first_walk() could result in a myriad of errors (download, file open/write, ???).

Probably a topic for #1312 ?

yes I vote for handling in 1312. I think the three classes of exceptions (repository / download / file IO) are logical and also the granularity that I could see a serious client wanting to handle (or not handle as might be reasonable with e.g. file IO ).

Mostly remove comments that provide little value after all the changes. Also remove a unused variable from a test. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

sechkova added the experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch) label May 18, 2021

jku force-pushed the experimental-client-use-bundle branch from 228a01c to 05421e0 Compare May 18, 2021 08:57

This was referenced May 18, 2021

Remove MetadataWrapper class #1304

Closed

client refactor: review _get_*_meta_name() usage #1319

Closed

Jussi Kukkonen added 2 commits May 18, 2021 20:56

tests: Cleanup in new Updater tests

2bbf5bc

Add test for a refresh with just a local root.json. Remove unused code. Add docstrings for raised exceptions, add TODOs for the missing exception handling. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

jku force-pushed the experimental-client-use-bundle branch from 05421e0 to 2bbf5bc Compare May 18, 2021 18:05

jku commented May 19, 2021

View reviewed changes

tuf/client_rework/updater_rework.py Outdated Show resolved Hide resolved

jku marked this pull request as ready for review May 19, 2021 13:23

jku requested review from sechkova and joshuagl May 19, 2021 13:24

sechkova reviewed May 19, 2021

View reviewed changes

jku mentioned this pull request May 19, 2021

Remove temporary commits #1320

Closed

sechkova approved these changes May 20, 2021

View reviewed changes

joshuagl approved these changes May 20, 2021

View reviewed changes

This was referenced May 20, 2021

Experimental client: renaming #1401

Merged

ngclient: client-level constants #1402

Closed

New updater: Update comments

d35fc27

Mostly remove comments that provide little value after all the changes. Also remove a unused variable from a test. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

jku force-pushed the experimental-client-use-bundle branch from ad4e671 to d35fc27 Compare May 20, 2021 12:42

jku merged commit db02fa6 into theupdateframework:experimental-client May 20, 2021

jku deleted the experimental-client-use-bundle branch December 30, 2024 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental client: use bundle #1396

Experimental client: use bundle #1396

jku commented May 18, 2021 •

edited

Loading

jku commented May 18, 2021 •

edited

Loading

MVrachev commented May 19, 2021

sechkova left a comment

sechkova May 19, 2021

sechkova May 20, 2021

sechkova May 19, 2021

jku commented May 19, 2021

joshuagl left a comment

joshuagl May 20, 2021

jku May 20, 2021

		self._bundle.update_snapshot(data)
		self._persist_metadata("snapshot", data)

Experimental client: use bundle #1396

Experimental client: use bundle #1396

Conversation

jku commented May 18, 2021 • edited Loading

jku commented May 18, 2021 • edited Loading

MVrachev commented May 19, 2021

sechkova left a comment

Choose a reason for hiding this comment

sechkova May 19, 2021

Choose a reason for hiding this comment

sechkova May 20, 2021

Choose a reason for hiding this comment

sechkova May 19, 2021

Choose a reason for hiding this comment

jku commented May 19, 2021

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl May 20, 2021

Choose a reason for hiding this comment

jku May 20, 2021

Choose a reason for hiding this comment

jku commented May 18, 2021 •

edited

Loading

jku commented May 18, 2021 •

edited

Loading