RepositorySimulator: add non-consistent snapshot support #1666

sechkova · 2021-11-08T14:38:15Z

Extend filename partitioning to support serving non-versioned
metadata and non-prefixed target files.

Fixes #1637, fixes #1667, fixes #1573

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-11-08T14:48:21Z

I didn't add tests in this PR, I opened this one instead: #1667

coveralls · 2021-11-08T15:07:42Z

Pull Request Test Coverage Report for Build 1472626893

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 98.471%

Totals
Change from base Build 1449719184:	-0.1%
Covered Lines:	3282
Relevant Lines:	3302

💛 - Coveralls

jku

Did you somehow test this privately?

The core difficulty with this issue is the corner we painted ourselves in by telling warehouse they can be consistent_snapshot and offer non-consistent targets for download at the same time: so we can't 100% tell if a targetname is hash prefixed or not when a client requests it ...

This code seems to assume that if target name contains a "." then it contains a prefix. I realize there probably aren't perfect solutions here but this seems non-optimal: it should be possible to request non-prefixed targets with dots in their names

jku · 2021-11-09T09:27:25Z

tests/repository_simulator.py

-                role = "timestamp"
-            else:
-                version, _, role = ver_and_name.partition(".")
+            role, version = self._partition_filename(ver_and_name)


hmm, it seems to do the right thing, but your function is called "partition filename" and then you need to feed it something that is explicitly just a portion of a filename ...

sechkova · 2021-11-09T10:14:27Z

This code seems to assume that if target name contains a "." then it contains a prefix. I realize there probably aren't perfect solutions here but this seems non-optimal: it should be possible to request non-prefixed targets with dots in their names

Oh yes, you are right. I guess I will rework it in parallel with tests to cover all the weird cases.

jku

marking "request changes" so it's visible in issue list

sechkova · 2021-11-15T17:29:09Z

I reworked the implementation and now RepositorySimulator makes the decision whether the client requests versioned/prefixed files or not based on: self.root.consistent_snapshot and self.prefix_targets_with_hash. While the specification describes how repository and client sync on consistent_snapshot, I presumed that they both need to know in advance whether they will work with hash-prefixed targets and I added the same prefix_targets_with_hash option to the repository.

I also added one new test file for consistent snapshot. I couldn't decide if the class TestConsistentSnapshot needs to be in a separate test file or integrated in another. Let me know your opinion on this as well as the general idea/readability of the tests.
Since we want to test the client and not that much the RepositorySimulator and it is not possible to verify that internally Updater builds the correct prefix+name, I "wrapped" RepositorySimulator._fetch_* to check the calls made by Updater.

sechkova · 2021-11-15T17:31:01Z

tests/repository_simulator.py

-                role = "timestamp"
-            else:
+            # root is always version-prefixed while timestamp is always NOT
+            if ver_and_name.endswith("root") or (


This will fail if a delegated role name ends with "root" (and consistent_snapshot is not enabled). But otherwise it is hard to generalize and I guess we don't want to support that weird cases in RepositorySimulator?

yeah no need to spend a lot of time polishing a test utility to perfection...

that said you could move version, _, role = ver_and_name.partition(".") to before this line so you could compare role == "root" (but would keep rest of the code as is -- I admit the else clause is unintuitive if this is done)?

up to you

I tried to do it in a readable way ... at least now it is correct for the cases I can think of.

jku

I think your reasoning about prefix_targets_with_hash option makes sense.

I also think counting the fetches at repository side is fine and actually better than poking into updater internals.

The patching in the tests is not pretty but it is much improved in this version. I think the asserts you use do not check whether other fetches were also made though, which I think we want (so we know it doesn't download things twice or something)... I think we should be comparing our expected calls list directly to the actually made calls list. WDYT?

...which leads me to: Making the call counters a RepositorySimulator feature might make the tests more readable and probably wouldn't be a lot of code: sim.call_stats.metadata could contain basically the same list of tuples that your testdata contains -- but this could also be a followup idea if you'd rather get these tests in ASAP.

jku · 2021-11-16T12:48:18Z

tests/repository_simulator.py

-                role = "timestamp"
-            else:
+            # root is always version-prefixed while timestamp is always NOT
+            if ver_and_name.endswith("root") or (


yeah no need to spend a lot of time polishing a test utility to perfection...

that said you could move version, _, role = ver_and_name.partition(".") to before this line so you could compare role == "root" (but would keep rest of the code as is -- I admit the else clause is unintuitive if this is done)?

up to you

jku · 2021-11-16T13:01:55Z

tests/test_updater_consistent_snapshot.py

+        for targetpath in targetpaths:
+            sim.targets.version += 1
+            sim.add_target("targets", b"content", targetpath)
+            sim.update_snapshot()


It's not important but there should be no need to update snapshot more than once

jku · 2021-11-16T13:19:52Z

tests/test_updater_consistent_snapshot.py

+# Copyright 2021, New York University and the TUF contributors
+# SPDX-License-Identifier: MIT OR Apache-2.0
+
+"""Test ngclient Updater consistent snapshot"""


this could be better (we're testing consistent snapshot in all client tests, hopefully). This file seems to test

that the client works with all combinations of consistent_snapshot and prefix_targets_with_hash

that expected URLs are being fetched with all of these combinations

sechkova · 2021-11-17T16:30:09Z

...which leads me to: Making the call counters a RepositorySimulator feature might make the tests more readable and probably wouldn't be a lot of code: sim.call_stats.metadata could contain basically the same list of tuples that your testdata contains -- but this could also be a followup idea if you'd rather get these tests in ASAP.

I think I will open a separate issue. It may double some of the work but it would be easier at least for me to do it as a follow up.
There it is: #1682

sechkova · 2021-11-17T16:31:04Z

Just noting that I'd like to squash the last two commits before merging :)

jku

👍

Extend URL partitioning to support serving non-versioned metadata and non-prefixed target files. Signed-off-by: Teodora Sechkova <[email protected]>

Add tests for ngclient.Updater toggling 'consitent_snapshot' and 'prefix_targets_with_hash'. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-11-18T09:19:55Z

Squashed the last two commits, should be ready now.

sechkova · 2021-11-18T10:02:49Z

I now saw this issue: #1573
I think test_refresh_on_consistent_targets() can be replaced by the new test file (test_updater_consistent_snapshot.py) and we can close it. I can do it in this PR if you agree?

jku · 2021-11-18T10:22:29Z

Sounds good to me!

Consistent snapshot and consistent targets are now extensively tested in test_updater_consistent_snapshot.py. Signed-off-by: Teodora Sechkova <[email protected]>

jku · 2021-11-18T12:03:28Z

Thanks

This change is to implement a feature to track the fetch calls to the metadata and targets. This feature was mentioned in PR theupdateframework#1666 that generated issue theupdateframework#1682. It is the ``RepositorySimulator.fetch_tracker``. It also changes the ``test_updater_consistent_snapshot`` to use the fetch_tracker instead of mocking it. It implements a ``dataclass`` that stores the calls to fetch metadata (``_fetch_metadata``) and targets (``_fetch_targets``). This dataclass has a method to clean (``clear``) the current metadata and targets tracker. The fetch calls for metadata are stored as a list of tuples that contains metadata role name and version, and fetch calls for targets are stored as a list of target paths. Signed-off-by: Kairo de Araujo <[email protected]>

This commit implements a feature in Repository Simulator to track the fetch calls to the metadata and targets. This feature was mentioned in PR theupdateframework#1666 that generated issue theupdateframework#1682. This commit adds ``RepositorySimulator.fetch_tracker``. It also changes the ``tests/test_updater_consistent_snapshot.py`` to use the ``fetch_tracker`` instead of using mock. It implements a ``dataclass`` that stores the calls to fetch metadata (``_fetch_metadata``) in ``fetch_tracker.metadata`` and targets (``_fetch_targets``) in ``fetch_tracker.targets``. The fetch calls for metadata, and targets are stored as lists. Signed-off-by: Kairo de Araujo <[email protected]>

This commit implements a feature in Repository Simulator to track the fetch calls to the metadata and targets. This feature was mentioned in PR theupdateframework#1666 that generated issue theupdateframework#1682. This commit adds RepositorySimulator.fetch_tracker. It also changes the tests/test_updater_consistent_snapshot.py to use the fetch_tracker instead of using mock. It implements a dataclass that stores the calls to fetch metadata (_fetch_metadata) in fetch_tracker.metadata and targets (_fetch_targets) in fetch_tracker.targets. The fetch calls for metadata, and targets are stored as lists. Signed-off-by: Kairo de Araujo <[email protected]>

jku reviewed Nov 9, 2021

View reviewed changes

jku requested changes Nov 10, 2021

View reviewed changes

sechkova force-pushed the reposim-consistent-snapshot branch from 089cfea to 15e458c Compare November 15, 2021 17:15

sechkova requested a review from jku November 15, 2021 17:29

sechkova commented Nov 15, 2021

View reviewed changes

jku reviewed Nov 16, 2021

View reviewed changes

sechkova requested a review from jku November 17, 2021 16:31

sechkova mentioned this pull request Nov 17, 2021

RepositorySimulator: add trackers/counters for downloaded files #1682

Closed

jku approved these changes Nov 18, 2021

View reviewed changes

sechkova added 2 commits November 18, 2021 11:13

RepositorySimulator: add non-consistent snapshot support

54969e8

Extend URL partitioning to support serving non-versioned metadata and non-prefixed target files. Signed-off-by: Teodora Sechkova <[email protected]>

ngtests: Add consistent_snapshot tests

a72fa62

Add tests for ngclient.Updater toggling 'consitent_snapshot' and 'prefix_targets_with_hash'. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the reposim-consistent-snapshot branch from f2ce6de to a72fa62 Compare November 18, 2021 09:17

Remove test_refresh_on_consistent_targets

da1d975

Consistent snapshot and consistent targets are now extensively tested in test_updater_consistent_snapshot.py. Signed-off-by: Teodora Sechkova <[email protected]>

jku merged commit 1b5df4c into theupdateframework:develop Nov 18, 2021

kairoaraujo mentioned this pull request Dec 2, 2021

Issue#1682/repository simulator fetch tracker #1704

Closed

3 tasks

kairoaraujo mentioned this pull request Dec 3, 2021

Implemented fetch_tracker to RepositorySimulator #1705

Merged

3 tasks

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

RepositorySimulator: add non-consistent snapshot support #1666

RepositorySimulator: add non-consistent snapshot support #1666

Uh oh!

Conversation

sechkova commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova commented Nov 8, 2021

Uh oh!

coveralls commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1472626893

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

jku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

sechkova commented Nov 9, 2021

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

sechkova commented Nov 15, 2021

Uh oh!

sechkova Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sechkova Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

jku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jku Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

jku Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

sechkova commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sechkova commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

sechkova commented Nov 18, 2021

Uh oh!

sechkova commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Nov 18, 2021

Uh oh!

jku commented Nov 18, 2021

Uh oh!

Uh oh!

sechkova commented Nov 8, 2021 •

edited

Loading

coveralls commented Nov 8, 2021 •

edited

Loading

jku left a comment •

edited

Loading

sechkova Nov 15, 2021 •

edited

Loading

jku Nov 16, 2021 •

edited

Loading

jku left a comment •

edited

Loading

jku Nov 16, 2021 •

edited

Loading

sechkova commented Nov 17, 2021 •

edited

Loading

sechkova commented Nov 17, 2021 •

edited

Loading

sechkova commented Nov 18, 2021 •

edited

Loading