ngclient: review preorder dfs code #1463

sechkova · 2021-06-23T08:34:22Z

Fixes #1398
Description of the changes being introduced by the pull request:

Reviews and simplifies Updater._preorder_depth_first_walk code.
makes _visit_child_role a member of DelegatedRole

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

sechkova · 2021-06-23T08:35:46Z

Opened as a draft not only because it is dependent on #1408 but also it is not well tested yet.

jku · 2021-06-30T09:15:11Z

I've had a first look and the ideas look fine. Some smaller comments:

some issues in commit splitting (like visit_child_role() being annotated as returning bool and then returning something else)
Likewise terminating child roles are added to child_roles_to_visit twice, then duplication is removed in a "simplification" commit
IMO we should avoid doing e.g. pathpattern.lstrip(os.sep) -- these are the sort of workarounds that make life hard at some point... either the path pattern is valid or it isn't valid and we should just refuse to load the metadata (we don't have to support every weird case spec defines as valid). That said, leaving it as is and filing an issue is fine as well
more type annotations would be nice: Dict without contained types is not great, role_metadata in _preorder_depth_first_walk should be manually annotated as Targets: without this the whole function is a black box as far as static typing is concerned.

I'm suspicious about how this operates when target is not found:

old code does not seem to raise if delegation limit is not reached? I agree the old code return value seems nonsense but is raising really the right thing?
new code does not refer to delegation limit in the error message if it is reached

Also, is_in_trusted_paths() returns False if both paths and path_hash_prefixes are unset. Is this correct? I can see how that's the "safe" assumption but it also makes the whole idea of unset paths and unset path_hash_prefixes meaningless: the delegation has no reason to exist...

trishankatdatadog · 2021-06-30T16:36:58Z

I really think it would be a lot simpler as a recursive function...

jku · 2021-07-01T13:23:43Z

tuf/ngclient/updater.py

+            self._load_targets(role_name, parent_role)
+
+            role_metadata = self._bundle[role_name].signed


this implies we should make bundle (or trusted_set as it maybe now) return the loaded metadata to avoid a lookup immediately afterwards. Can you file an issue please?

sechkova · 2021-07-07T15:00:04Z

I really think it would be a lot simpler as a recursive function...

I agree, it does make the code look much simpler, let me test implementing those terminating delegations correctly with it :)

sechkova · 2021-07-07T16:23:38Z

Also, is_in_trusted_paths() returns False if both paths and path_hash_prefixes are unset. Is this correct? I can see how that's the "safe" assumption but it also makes the whole idea of unset paths and unset path_hash_prefixes meaningless: the delegation has no reason to exist...

Both paths and path_hash_prefixes being None should be impossible since there is a check in the constructor which raises an exception but if you think it will be better/safer to duplicate it again here, I can add it.
https://github.com/theupdateframework/tuf/blob/24fa1128116429dc18e185047fbffb009ca509b0/tuf/api/metadata.py#L914

Forget what I wrote, the check is for both being set ...
Which leads me to the question, should the input validation include the check whether both are not set?

sechkova · 2021-07-09T14:02:54Z

Summarizing the changes made to this PR:

rebased on develop
improved the commits
added more type annotations
changed the function to return None when a targetfile is not found (instead of raising an exception)
added one possible recursive implementation of the same function e23e64b

Still there are issues pending and I am keeping this a draft to have a chance of reviewing it again myself.

File an issue so that self._load_targets(role_name, parent_role) -> TrustedMetadataSet returns the loaded metadata to avoid lookup
Avoid doing e.g. pathpattern.lstrip(os.sep) or file an issue about it
Revise return values and maybe throw exceptions in corner cases
The correct behaviour when both paths and path_hash_prefixes are unset is still unclear.
Using fnmatch does not do exactly what is promised in the documentation in terms of UNIX filename pattern matching

MVrachev

Forget what I wrote, the check is for both being set ...
Which leads me to the question, should the input validation include the check whether both are not set?

I think we should check for that.
I created an issue where we can discuss that #1497.

I like that it's much simpler and easier to read now and agree that the recursive version simplifies the code.

I think that before making this pr non-draft it's good to have tests for the ngclient updater.
I am a little worried about merging this before confirming we are not missing use-cases with this simplification.

MVrachev · 2021-07-15T16:16:20Z

tuf/ngclient/updater.py

+        self,
+        target_filepath: str,
+        visited_role_names: Set[str],
+        current_role_pair: List[Tuple[str, ...]],


What does ... mean in List[Tuple[str, ...]]?

annotations for both visited_role_names and current_role_pair are wrong

MVrachev · 2021-07-15T16:18:57Z

tuf/ngclient/updater.py

+            return targetinfo, terminated
+
+        # Pop the role name from the top of the stack.
+        role_name, parent_role = current_role_pair


From the type annotation of current_role_pair I see it's a List[Tuple[str, ...].
Don't you need to get an exact element from current_role_pair which will contain role_name and parent_role pair?

jku · 2021-07-16T11:26:37Z

added one possible recursive implementation of the same function e23e64b

I have some reservations against recursion: when you use it, you then have to think about the call stack depth limitations and how that affects your code... In our case maximum number of delegations now depends on call stack depth.

Many hundreds of delegations is not a use case we should explicitly design for, I accept that. Still, I don't like setting an artificial limit when we don't event know what that limit is: stack depth is implementation specific, cpython uses 1000, but we also don't know how much of that we can use. The application developer might love recursion and may have used half the stack already, and maybe the network stack is special as well and needs a few hundred stack levels...

Basically I think expected call stack depth is larger than maximum reasonable number of delegations... but it's only maybe 5-10 times as large and that sounds uncomfortably close to me.

trishankatdatadog · 2021-07-16T19:39:09Z

I have some reservations against recursion: when you use it, you then have to think about the call stack depth limitations and how that affects your code... In our case maximum number of delegations now depends on call stack depth.

As you said, I don't think any reasonable use case has that many delegations to worry about. Furthermore, it makes for more readable code, which is more important, I think.

sechkova · 2021-07-21T12:24:59Z

Rebased on develop + some fixes of stupid errors in last commit.

sechkova · 2021-07-23T13:34:15Z

Thanks for the reviews and suggestions. I think it is time for this PR to stop being a draft and get a full review. I've intentionally kept the diffs in multiple smaller commits hoping that it may ease the review but you tell me how valuable it is for the history. I do mean to squash whatever feels redundant.

Some issues that were raised meanwhile:

Recursive vs stack implementation:
- I reverted the recursive implementation commit (intentionally reverted and not deleted to keep the history). I agree with @trishankatdatadog that the code looks visually better but at the same time function parameters increase and when we start propagating terminating flags up the stack, it doesn't feel any simpler. @jku gave reasonable arguments against recursion too.
I opened follow up issues for the items from the previous list:
- ngclient: return loaded metadata from Updater._load_targets #1507
- ngclient: avoid lstrip(os.sep) on target paths #1506
- Revise return values and maybe throw exceptions in corner cases: the function now returns a targetinfo dictionary or None if a match is not found.
- Metadata API: validate that "paths" or "path_hash_prefixes" is set #1497 and 2159b88
- Using fnmatch does not do exactly what is promised in the documentation in terms of UNIX filename pattern matching ngclient: implement glob-like pattern matching #1505
Not an issue but DelegatedRole.is_in_trusted_paths feels a bit clumsy for a method name. Suggestions are welcome.
Update if needed when Clarify client traversal of role delegation graph specification#177 is resolved.

MVrachev

All of the changes seem reasonable and the history indeed is easier to follow when there are multiple smaller commits. Thanks for that @sechkova!

The only missing part in this pr is validation through more tests.
The _preorder_depth_first_walk is not an easy to understand function and we should
make sure we aren't introducing bugs.

We have a couple of tests using get_one_valid_targetinfo() in test_updater_ng.py, but
there are a couple of code paths not tested yet (according to coverage all):

delegation role with terminating option turn on
test when a target is not found
reaching the max limit of visiting delegations, but there are more delegations left

Additionally, we never test DelegatedRole.is_in_trusted_paths() when path_hash_prefixes is set and thus never actually test calculating the hashes.

Make _visit_child_role a public method of DelegatedRole class. Reduce debug logging. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-07-28T15:04:49Z

Rebased on latest changes in develop.

sechkova · 2021-07-28T15:08:54Z

All of the changes seem reasonable and the history indeed is easier to follow when there are multiple smaller commits. Thanks for that @sechkova!

The only missing part in this pr is validation through more tests.

No two ways about that but I don't know whether this PR is the right way given that this is a common issue for the whole Updater: #1499

MVrachev · 2021-08-03T10:50:46Z

tuf/ngclient/updater.py


-            role_metadata = self._trusted_set[role_name].signed
+            role_metadata: Targets = self._trusted_set[role_name].signed
            target = role_metadata.targets.get(target_filepath)


Suggested change

target = role_metadata.targets.get(target_filepath)

target: Union[TargetFile, None] = role_metadata.targets.get(target_filepath)

According to the metadata API if role_metadata is an instance of Targets, then
role_metadata.targets.get(target_filepath) should be a TargetFile if it exists.
I think it make sense to add annotation here.

(as reply to Martins comment:) since role_metadata is now a Targets, this does not need an annotation for target: static type check already figures out that target is a TargetFile | None

jku

I think this is a major improvement, much easier to follow now. _preorder_depth_first_walk() could maybe still be simplified a bit but this is so much better that I say let's get it in and do smaller tweaks afterwards (so the testing work can start).

A good set of test cases would be nice but I think we can leave that to #1499 -- the testing needs some serious thinking as well to prevent a mess of testing spagetti so makes sense to do in its own PR

DelegatedRole.is_in_trusted_paths() seems like a good idea. I tried to think of a better name but ... can't come up with anything, I guess this is fine.

the paths/path_hash_prefixes resolution seems reasonable.

I would remove both the recursive implementation commit and the revert if it's not a direction that was chosen.

I Also left some comments in code but overall this LGTM: I'm approving, but please have a look at whether you want to act on the comments.

jku · 2021-08-05T10:10:56Z

tuf/ngclient/updater.py

-    def get_one_valid_targetinfo(self, target_path: str) -> Dict:
+    def get_one_valid_targetinfo(
+        self, target_path: str
+    ) -> Union[Dict[str, Any], None]:


Suggested change

) -> Union[Dict[str, Any], None]:

) -> Optional[Dict[str, Any]]:

jku · 2021-08-05T10:11:34Z

tuf/ngclient/updater.py

-    def _preorder_depth_first_walk(self, target_filepath) -> Dict:
+    def _preorder_depth_first_walk(
+        self, target_filepath: str
+    ) -> Union[Dict[str, Any], None]:


Suggested change

) -> Union[Dict[str, Any], None]:

) -> Optional[Dict[str, Any]]:

these mean union is then likely unused import

jku · 2021-08-05T10:18:01Z

tuf/ngclient/updater.py


-            role_metadata = self._trusted_set[role_name].signed
+            role_metadata: Targets = self._trusted_set[role_name].signed
            target = role_metadata.targets.get(target_filepath)


(as reply to Martins comment:) since role_metadata is now a Targets, this does not need an annotation for target: static type check already figures out that target is a TargetFile | None

jku · 2021-08-05T10:31:12Z

tuf/api/metadata.py

+            # Calculate the hash of the filepath to determine which bin
+            # to find the target. The client currently assumes the repository
+            # uses sslib's default algorithm to generate hashes.
+            digest_object = sslib_hash.digest()


I think you can remove the comment about assumptions and specify sha256 as algorithm here (as per spec) , no?

Right ... it does say exactly sha256

jku · 2021-08-05T10:43:56Z

tuf/api/metadata.py

+            raise ValueError(
+                "At least one of the attributes 'paths' and"
+                "'path_hash_prefixes' must be set!"
+            )


I'd really appreciate if we could squeeze the exceptions and comments to the minimum number of lines that are required to convey the message... in the long term this has a major effect on readability. The content in this one seems a bit misleading as well so maybe:

Suggested change

raise ValueError(

"At least one of the attributes 'paths' and"

"'path_hash_prefixes' must be set!"

)

raise ValueError("One of paths or path_hash_prefixes must be set")

I tried to shorten the exception messages and also make them look consistently. It may have slightly changed the meaning though: 34f59c9

jku · 2021-08-05T10:58:17Z

tuf/ngclient/updater.py

+        Returns:
+            A targetinfo dictionary or None


I think this is fine as long as we aim to get rid of the whole dictionary soon... the dict is just bad API.

The goal for this method would be to just return Optional[TargetFile] but I guess we can't do that yet since

updated_targets() for some reason takes multiple TargetFiles and needs targetpaths for each -- this will not look good if caller needs to provide the targetpaths ... but some API redesign might help

TargetFile does not have targetpath in it (yet at least)

Hopefully we can change that with the progress of #1514

jku · 2021-08-05T12:46:03Z

tests/test_metadata_serialization.py

-        "no hash or path prefix":
-            '{"keyids": ["keyid"], "name": "a", "terminating": true, "threshold": 3}',


shouldn't there now be a test that checks that deserialization fails with this input?

Added one with invalid DelegatedRoles: 34f59c9

sechkova · 2021-08-06T12:31:04Z

I Also left some comments in code but overall this LGTM: I'm approving, but please have a look at whether you want to act on the comments.

Thanks for the comments, let me try to address them before merging.

sechkova · 2021-08-06T15:43:41Z

I think I addressed everything and removed the commits about the recursive implementation. Should be ready now.

joshuagl

Lovely set of clean ups, nice work @sechkova.

I'd prefer a different name for is_in_trusted_paths(), added a comment in-line. Let me know what you think.

tuf/api/metadata.py

Rename to DelegatedRole.is_delegated_path and return a boolean flag instead of the role's name. Minor comments and code style improvements. Some code simplification. Signed-off-by: Teodora Sechkova <[email protected]>

Remove _get_filepath_hash and call sslib_hash.digest directly instead. Signed-off-by: Teodora Sechkova <[email protected]>

Simplify the code in _preorder_depth_first_walk. Signed-off-by: Teodora Sechkova <[email protected]>

Reduce redundant logging and simplify code further. Signed-off-by: Teodora Sechkova <[email protected]>

Improve type annotations. Signed-off-by: Teodora Sechkova <[email protected]>

The specification does not state clearly what is the behaviour when none of delegation's "paths" and "path_hash_prefixes" is set. See theupdateframework#1497. Until this issue is clarified, copy current Updater which raises an error in such case. Signed-off-by: Teodora Sechkova <[email protected]>

Give 'role_names' the more verbose description 'delegations_to_visit'. Add some comments and docstrings. Signed-off-by: Teodora Sechkova <[email protected]>

jku reviewed Jul 1, 2021

View reviewed changes

joshuagl mentioned this pull request Jul 2, 2021

Merge ngclient: a new client library implementation #1408

Merged

sechkova force-pushed the review-preorder-dfs branch 2 times, most recently from ead4b19 to 8bd34de Compare July 8, 2021 15:27

trishankatdatadog mentioned this pull request Jul 12, 2021

Support delegations in client theupdateframework/go-tuf#137

Merged

MVrachev mentioned this pull request Jul 15, 2021

Metadata API: validate that "paths" or "path_hash_prefixes" is set #1497

Closed

MVrachev suggested changes Jul 15, 2021

View reviewed changes

sechkova force-pushed the review-preorder-dfs branch from e23e64b to c79f063 Compare July 21, 2021 12:19

sechkova force-pushed the review-preorder-dfs branch 3 times, most recently from a3ff2b8 to b502434 Compare July 23, 2021 12:24

sechkova marked this pull request as ready for review July 23, 2021 13:34

sechkova changed the title ~~WIP: ngclient: review preorder dfs code~~ ngclient: review preorder dfs code Jul 23, 2021

MVrachev suggested changes Jul 27, 2021

View reviewed changes

Move _visit_child_role to DelegatedRole

167441c

Make _visit_child_role a public method of DelegatedRole class. Reduce debug logging. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the review-preorder-dfs branch from b502434 to a182ca9 Compare July 28, 2021 15:02

joshuagl mentioned this pull request Jul 28, 2021

ngclient Updater: write tests to handling coverage report #1499

Closed

MVrachev reviewed Aug 3, 2021

View reviewed changes

jku approved these changes Aug 5, 2021

View reviewed changes

sechkova force-pushed the review-preorder-dfs branch 2 times, most recently from d49fab1 to fbca198 Compare August 6, 2021 15:35

joshuagl reviewed Aug 9, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

sechkova added 7 commits August 10, 2021 09:00

Rename and simplify visit_child_role

f5b81be

Rename to DelegatedRole.is_delegated_path and return a boolean flag instead of the role's name. Minor comments and code style improvements. Some code simplification. Signed-off-by: Teodora Sechkova <[email protected]>

Remove _get_filepath_hash

790aceb

Remove _get_filepath_hash and call sslib_hash.digest directly instead. Signed-off-by: Teodora Sechkova <[email protected]>

Simplify child roles loop

fd96225

Simplify the code in _preorder_depth_first_walk. Signed-off-by: Teodora Sechkova <[email protected]>

Simplify _preorder_depth_first_walk

91cfb9e

Reduce redundant logging and simplify code further. Signed-off-by: Teodora Sechkova <[email protected]>

Type annotate _preorder_depth_first_walk

917bc8d

Improve type annotations. Signed-off-by: Teodora Sechkova <[email protected]>

Improve naming and comments

bc073b8

Give 'role_names' the more verbose description 'delegations_to_visit'. Add some comments and docstrings. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the review-preorder-dfs branch from fbca198 to bc073b8 Compare August 10, 2021 06:04

jku merged commit 55c8a78 into theupdateframework:develop Aug 10, 2021

		self._load_targets(role_name, parent_role)

		role_metadata = self._bundle[role_name].signed

	target = role_metadata.targets.get(target_filepath)
	target: Union[TargetFile, None] = role_metadata.targets.get(target_filepath)

	) -> Union[Dict[str, Any], None]:
	) -> Optional[Dict[str, Any]]:

		"no hash or path prefix":
		'{"keyids": ["keyid"], "name": "a", "terminating": true, "threshold": 3}',

ngclient: review preorder dfs code #1463

ngclient: review preorder dfs code #1463

Conversation

sechkova commented Jun 23, 2021

sechkova commented Jun 23, 2021

jku commented Jun 30, 2021 • edited Loading

trishankatdatadog commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sechkova commented Jul 7, 2021

sechkova commented Jul 7, 2021 • edited Loading

sechkova commented Jul 9, 2021 • edited Loading

MVrachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Jul 16, 2021

trishankatdatadog commented Jul 16, 2021

sechkova commented Jul 21, 2021

sechkova commented Jul 23, 2021 • edited Loading

MVrachev left a comment • edited Loading

Choose a reason for hiding this comment

sechkova commented Jul 28, 2021

sechkova commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sechkova commented Aug 6, 2021

sechkova commented Aug 6, 2021

joshuagl left a comment

Choose a reason for hiding this comment

jku commented Jun 30, 2021 •

edited

Loading

sechkova commented Jul 7, 2021 •

edited

Loading

sechkova commented Jul 9, 2021 •

edited

Loading

sechkova commented Jul 23, 2021 •

edited

Loading

MVrachev left a comment •

edited

Loading