Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ngclient: top-level targets is unnecessarily updated twice #1578

Closed
jku opened this issue Sep 10, 2021 · 3 comments · Fixed by #1593
Closed

ngclient: top-level targets is unnecessarily updated twice #1578

jku opened this issue Sep 10, 2021 · 3 comments · Fixed by #1593
Assignees
Labels
backlog Issues to address with priority for current development goals bug ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Sep 10, 2021

ngclient updates top-level targets during refresh(). Then on get_one_valid_targetinfo() the top-level targets is updated again:

  • this is not dangerous and does not break any TrustedMetadataSet constraints
  • nothing is downloaded but the local metadata file is re-read and re-parsed

Mostly this is just annoying in the logs because it looks like a bug, with two updates usually one right after the other.

This could be fixed as a special case for top-level targets in the delegation loop but I think a similar issue comes up if other targets are already loaded, but then are needed in _preorder_depth_first_walk(). Maybe _load_targets() should check if a targets metadata is loaded and avoid re-loading in that case (if a targets is loaded it should be completely final and valid)?

@MVrachev MVrachev added the backlog Issues to address with priority for current development goals label Sep 15, 2021
@sechkova sechkova self-assigned this Sep 15, 2021
@sechkova sechkova added this to the Sprint 8 milestone Sep 15, 2021
@sechkova
Copy link
Contributor

Maybe _load_targets() should check if a targets metadata is loaded and avoid re-loading in that case (if a targets is loaded it should be completely final and valid)?

The only scenario where we might be skipping a newer delegated role metadata file is the unlikely case where the file is updated between two get_one_valid_targetinfo calls but this should be a very short period of time:

updater = Updater()
updater.refresh()
updater.get_one_valid_targetinfo("file1") # "some_role" is loaded and checked for file1
...
updater.refresh() # newer snapshot is downloaded with newer version of "some_role"
updater.get_one_valid_targetinfo("file2") # "some_role" is not reloaded and the older version is used to check for file2

But is this the correct sequence? I don't think we should be calling refresh() before each get_one_valid_targetinfo?

@jku
Copy link
Member Author

jku commented Sep 27, 2021

But is this the correct sequence? I don't think we should be calling refresh() before each get_one_valid_targetinfo?

No we should not. It's also currently impossible: calling refresh() twice is an error as it tries to break TrustedMetadataSet rules (of course we could change how refresh works but in practice it would mean calling refresh() again is the same thing as creating a new Updater and then calling refresh() -- the only optimization would be that we don't have to load the local root from disk so not really worth it)

@sechkova
Copy link
Contributor

Sync with #1507

@sechkova sechkova modified the milestones: Sprint 8, Sprint 12 Nov 10, 2021
@sechkova sechkova modified the milestones: Sprint 12, Sprint 13 Nov 24, 2021
@jku jku closed this as completed in #1593 Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals bug ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants