Review download code #1448

sechkova · 2021-06-15T13:33:10Z

Fixes #1399

Description of the changes being introduced by the pull request:

This PR builds on top of #1408 and will remain a draft until its merge.

simplify download code, remove average download speed check
wrap the download functionality in FileDownload class
simplify RequestsFetcher.fetch()
add type annotations

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

jku · 2021-06-16T08:45:22Z

Looks great at first glance.

This maybe a dumb question but is there something that prevents us from putting the new FileDownloader functionality directly into FetcherInterface?

Are the RequestsFetcher improvements dependent on the other changes or just included in same PR for convenience?

sechkova · 2021-06-16T09:49:19Z

This maybe a dumb question but is there something that prevents us from putting the new FileDownloader functionality directly into FetcherInterface?

Only me not thinking enough about it ... 🙄

Are the RequestsFetcher improvements dependent on the other changes or just included in same PR for convenience?

No, they are not dependent, I just consider fetcher as part of the download code.

I do like the idea of simplifying further and keeping only a single download/fetcher module instead of the two. How should we do it? Build on top of this PR, a separate PR or I can rewrite these commits here and directly move download.py functions into FetcherInterface?

jku · 2021-06-16T10:10:14Z

I do like the idea of simplifying further and keeping only a single download/fetcher module instead of the two. How should we do it? Build on top of this PR, a separate PR or I can rewrite these commits here and directly move download.py functions into FetcherInterface?

I think merging a PR that adds a new object would be weird if we intend to remove it soon in a followup PR... otherwise whatever works for you I think

sechkova · 2021-06-17T12:41:41Z

Rewrote part of the commits here. Now the simplified download functions are made part of FetcherInterface and the download.py module is removed.

sechkova · 2021-06-17T12:44:58Z

I don't know what is blocking GitHub actions here?

jku · 2021-06-22T08:58:06Z

I don't know what is blocking GitHub actions here?

quite strange. It never actually started the action (https://github.com/theupdateframework/tuf/actions) so I can't even restart it.

Looks really nice btw... Net result of removing 200 lines and ending up with a cleaner implementation is not too bad.

jku

I like it a lot: this makes download code much easier to understand (and I agree with the decisions made)! Left several comments but the only one that I think needs to be fixed here is the argument name: required_length does not seem to represent what it is.

Feel free to file issues for the other things

tuf/ngclient/fetcher.py

tuf/ngclient/_internal/requests_fetcher.py

sechkova · 2021-07-06T17:10:34Z

I think all suggestions are added with 959ab5a.

The average download speed check does not achieve its purpose of protecting against slow retrieval attacks. Such protection is highly dependent on the networking stack which could be user-provided and not under tuf's control. The slow retrieval attack protection has been removed from the specification for similar reasons. Signed-off-by: Teodora Sechkova <[email protected]>

Remove the strict length check in download.py and check only if an upper bowndary is exceeded. Exact length is not known for some metadata files during download. The check makes the code unnecessarily complicated and is better suited for the later metadata verification step. Signed-off-by: Teodora Sechkova <[email protected]>

Remove format checking of download_file() parameters. The function is internal and receives trusted input from updater.py. Signed-off-by: Teodora Sechkova <[email protected]>

Remove logging when an exception is raised. Reduce logging level to debug. Signed-off-by: Teodora Sechkova <[email protected]>

It allows us to remove the disable of pylint consider-using-with warning and does not require any manual file closing. Signed-off-by: Teodora Sechkova <[email protected]>

Make the internal function chunks() a separate private method of Reqests fetcher. Signed-off-by: Teodora Sechkova <[email protected]>

Remove repetitive debug logging. Signed-off-by: Teodora Sechkova <[email protected]>

Add missing type annotations in download.py, fetcher.py and requests_fetcher.py Update docstrings to match the new style. Signed-off-by: Teodora Sechkova <[email protected]>

Make download_file and download_bytes functions part of the FetcherInterface class. Remove download.py module. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-07-07T14:49:13Z

Rebased on latest changes in develop.

jku

Left comments/question about naming of the argument to Fetcher.fetch() as well... but I'm approving as is, this is a very good improvement on download code IMO

jku · 2021-07-09T06:20:30Z

tuf/ngclient/_internal/requests_fetcher.py

@@ -53,7 +53,7 @@ def __init__(self):
        self.chunk_size: int = 400000  # bytes
        self.sleep_before_round: Optional[int] = None

-    def fetch(self, url, required_length):
+    def fetch(self, url: str, required_length: int) -> Iterator[bytes]:


I guess this is more max_length than required_length as well?

the docstring could be better as well

jku · 2021-07-09T06:22:56Z

tuf/ngclient/fetcher.py

@@ -20,7 +29,7 @@ class FetcherInterface:
    __metaclass__ = abc.ABCMeta

    @abc.abstractmethod
-    def fetch(self, url, required_length):
+    def fetch(self, url: str, required_length: int) -> Iterator[bytes]:


maybe should be max_length (also docstring could be better)?

This is sort of API already but the change maybe worth it as required_length really is misleading...

Rename required_length argument in fetcher to max_length to better match its purpose. Docstring improvements. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-07-09T12:21:48Z

Amended the last commit 0eff004 with the suggestion.

MVrachev

Great job!
I liked all of the simplifications.
It's easier to read now.
It looks good to me as well.

sechkova force-pushed the review-download-code branch from 7e288a0 to b87fb5f Compare June 17, 2021 12:39

jku mentioned this pull request Jun 21, 2021

Improve testing using generated metadata #1444

Closed

joshuagl mentioned this pull request Jul 2, 2021

Merge ngclient: a new client library implementation #1408

Merged

sechkova force-pushed the review-download-code branch from b87fb5f to 5b3a4de Compare July 5, 2021 14:52

sechkova marked this pull request as ready for review July 5, 2021 14:55

jku reviewed Jul 6, 2021

View reviewed changes

joshuagl requested a review from MVrachev July 7, 2021 08:44

sechkova added 9 commits July 7, 2021 17:31

Remove format checks

8c22294

Remove format checking of download_file() parameters. The function is internal and receives trusted input from updater.py. Signed-off-by: Teodora Sechkova <[email protected]>

Reduce download logging

8692663

Remove logging when an exception is raised. Reduce logging level to debug. Signed-off-by: Teodora Sechkova <[email protected]>

Implement download_file as contextmanager

0b77eb9

It allows us to remove the disable of pylint consider-using-with warning and does not require any manual file closing. Signed-off-by: Teodora Sechkova <[email protected]>

Simplify RequestsFetcher.fetch()

35fb22c

Make the internal function chunks() a separate private method of Reqests fetcher. Signed-off-by: Teodora Sechkova <[email protected]>

Reduce logging in requests_fetcher

9daccac

Remove repetitive debug logging. Signed-off-by: Teodora Sechkova <[email protected]>

Add type annotations

16f42fc

Add missing type annotations in download.py, fetcher.py and requests_fetcher.py Update docstrings to match the new style. Signed-off-by: Teodora Sechkova <[email protected]>

Move download.py functions to FetcherInterface

1596de2

Make download_file and download_bytes functions part of the FetcherInterface class. Remove download.py module. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the review-download-code branch from 959ab5a to f91182a Compare July 7, 2021 14:48

jku approved these changes Jul 9, 2021

View reviewed changes

Replace required_length with max_length

0eff004

Rename required_length argument in fetcher to max_length to better match its purpose. Docstring improvements. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the review-download-code branch from f91182a to 0eff004 Compare July 9, 2021 12:20

jku mentioned this pull request Jul 12, 2021

Enable mypy for ngclient #1489

Merged

3 tasks

MVrachev approved these changes Jul 12, 2021

View reviewed changes

jku merged commit f6df086 into theupdateframework:develop Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Review download code #1448

Review download code #1448

sechkova commented Jun 15, 2021

jku commented Jun 16, 2021 •

edited

Loading

sechkova commented Jun 16, 2021

jku commented Jun 16, 2021

sechkova commented Jun 17, 2021

sechkova commented Jun 17, 2021

jku commented Jun 22, 2021 •

edited

Loading

jku left a comment •

edited

Loading

sechkova commented Jul 6, 2021

sechkova commented Jul 7, 2021

jku left a comment

jku Jul 9, 2021

jku Jul 9, 2021

sechkova commented Jul 9, 2021

MVrachev left a comment •

edited

Loading

Review download code #1448

Review download code #1448

Conversation

sechkova commented Jun 15, 2021

jku commented Jun 16, 2021 • edited Loading

sechkova commented Jun 16, 2021

jku commented Jun 16, 2021

sechkova commented Jun 17, 2021

sechkova commented Jun 17, 2021

jku commented Jun 22, 2021 • edited Loading

jku left a comment • edited Loading

Choose a reason for hiding this comment

sechkova commented Jul 6, 2021

sechkova commented Jul 7, 2021

jku left a comment

Choose a reason for hiding this comment

jku Jul 9, 2021

Choose a reason for hiding this comment

jku Jul 9, 2021

Choose a reason for hiding this comment

sechkova commented Jul 9, 2021

MVrachev left a comment • edited Loading

Choose a reason for hiding this comment

jku commented Jun 16, 2021 •

edited

Loading

jku commented Jun 22, 2021 •

edited

Loading

jku left a comment •

edited

Loading

MVrachev left a comment •

edited

Loading