New admin sign: add support for API communication & add tests #579

MVrachev · 2024-05-13T21:07:37Z

Description

New admin sign: add support for API communication

Add support to fetch pending roles for signing and send the result
back to the API.
Added tests for all cases.

Related to issue #533

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality not to work as expected)

Additional requirements

Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

Code of Conduct

By submitting this PR, you agree to follow our Code of Conduct.

I agree to follow this project's Code of Conduct

codecov · 2024-05-13T21:12:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.61%. Comparing base (47d0a69) to head (46b0a7e).
Report is 73 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   98.90%   98.61%   -0.29%     
==========================================
  Files          20       26       +6     
  Lines        1367     1881     +514     
==========================================
+ Hits         1352     1855     +503     
- Misses         15       26      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

repository_service_tuf/cli/admin/sign.py

lukpueh · 2024-05-14T12:27:29Z

repository_service_tuf/cli/admin/sign.py

+
+    if settings.get("SERVER") is None:
+        api_server = Prompt.ask("\n[cyan]API[/] URL address")
+        settings.SERVER = api_server


Do we really need to support 3 ways - settings, arg, prompt - of specifying the server address? It seems like a lot of effort for no usability gain. I suggest to at least remove the prompt.

I am okay to drop the settings check, but there is something I am missing as a usage for settings.SERVER.
I see we set it in other commands as well such as add artifacts, delete artifacts and import-artifacts.
@kairoaraujo if we set it in one of the commands will it be setup for my next command?

What is missing in settings.SERVER is the documentation for the user.
As a practical example, I utilize my ~/.rstuf.yml with SERVER defined, which empowers me to effectively not fill the server or address by the prompt every time.
(BTW, @MVrachev, your code works with it, I had to comment to test it)

❯ rstuf admin metadata sign ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ ┃ Metadata Signing Tool ┃ ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ ╭─ Error ─────────────────────────────────────────────────────────────────────────────╮ │ {"detail":{"message":"No metadata pending signing available","error":"Requires │ │ bootstrap started. State: None"}} │ ╰─────────────────────────────────────────────────────────────────────────────────────╯

The problem here is that we focus on the code and we forgot to document it.
For example, if you setup in the rstuf.yaml

Regarding arg and prompt, prompt is better as it is interactive.

Regarding arg and prompt, prompt is better as it is interactive

IMO interactivity isn't better per se, and we did have the discussion before, about which inputs should be provided via cli arg, and which via prompt. From what I remember ,you can argue both ways.

pro all prompt: consistent "wizard" ux for all inputs
pro cli args and prompt: inputs like server address, input file path and output file path differ are different types of inputs compared to the rest (keys, threshold, signers etc.) The former can be seen as config parameters to even use the tool, and the latter are what the tool is all about.

I lean towards using cli args for the config inputs, because they are generally easier to develop/maintain and to use. Independently, it's a nice additional feature to support providing these args via config file.

I think I missed/forgot this discussion.
We should document it in our docs/devel/design.rst

I think I support @kairoaraujo more on this one as for regular users of rstuf CLI it makes sense to have it configured.
If we want to continue this discussion I don't mind, but I don't feel this pr is the place to do it.
I will add settings.SERVER as an option input for now as that's how we have proceeded until now.
If we agree on something else we need to backtrack, not only for this function but everywhere to be consistent.

repository_service_tuf/cli/admin/sign.py

MVrachev · 2024-05-15T17:23:43Z

I addressed all comments and added support for local signing as suggested by @kairoaraujo.
Also, added documentation on what will happen when local signing is used.

MVrachev · 2024-05-15T19:02:53Z

Code coverage fails because of the #581 pr.
Not sure why it fails here as the files I have touched on have full coverage.

kairoaraujo · 2024-05-16T06:53:02Z

Code coverage fails because of the #581 pr. Not sure why it fails here as the files I have touched on have full coverage.

I think it disappears if you rebase on top of the main.

MVrachev · 2024-05-17T12:20:59Z

Rebased on top of main and fixed UT tests.
Ready for another review.
The code coverage issue remains.
PS: We will merge this pr after we release: #589.

Now, we got an exception that "file out_file_name cannot be found" as we try to open a file that doesn't exist, but it doesn't exist because the execution failed for some reason. Because of that I make sure to check the stderr before opening the file and actually receive a meaningfull error. Additionally, because click merges stdout and stderr by default (see "mix_stderr" argument https://click.palletsprojects.com/en/8.1.x/api/#testing) we receive a error "ValueError: stderr not separately captured" seamingly comming from the subprocess module or another library. To solve this issue I made "mix_stderr" to True. Signed-off-by: Martin Vrachev <[email protected]>

Add support to fetch pending roles for signing and send the result back to the API. Added tests for all cases. Signed-off-by: Martin Vrachev <[email protected]>

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev · 2024-05-20T10:04:51Z

Rebased on top of main.
The only real commit is Update docs where I update docs for the new rstuf admin sign -h.

kairoaraujo

LGTM

lukpueh

Sorry for being late to the party. This looks pretty good. I pointed out some docs mistakes, which must be fixed, and programming recommendations, which could be fixed, and I raised question about behavior. Curious what others think.

lukpueh · 2024-05-23T08:07:59Z

docs/source/guide/index.rst

@@ -179,11 +179,19 @@ sign (``sign``)
    Usage: rstuf admin metadata sign [OPTIONS] ROOT_IN [PREV_ROOT_IN]


This now is:

Usage: rstuf admin metadata sign [OPTIONS] [SIGNING_JSON_INPUT_FILE]

Good catch!

lukpueh · 2024-05-23T08:10:32Z

docs/source/guide/index.rst

+    2) provide a local file using the SIGNING_JSON_INPUT_FILE argument
+    When using method 2:
+     - 'SIGNING_JSON_INPUT_FILE' must be a file containing the JSON response from the 'GET /api/v1/metadata/sign' API endpoint.
+     - '--api_server' will be ignored.


The option has a hyphen not an underscore: --api-server

lukpueh · 2024-05-23T08:18:34Z

repository_service_tuf/cli/admin/sign.py


+    - '--api_server' will be ignored.


See comment about underscore above.

lukpueh · 2024-05-23T08:43:59Z

repository_service_tuf/cli/admin/sign.py

+    settings = context.obj["settings"]
+    signing_input: Optional[Dict[str, Any]] = None
+    if signing_json_input_file:
+        signing_input = json.load(signing_json_input_file)  # type: ignore


minor programming advice: Since you already case handle the presence of an input file, you could add another line to extract the contents of metadata into pending_roles here. Then _get_pending roles wouldn't need the large if/else block and could be responsible for fetching pending_roles from the api only.

lukpueh · 2024-05-23T09:13:45Z

repository_service_tuf/cli/admin/sign.py

+
+        console.print("Saved result to 'sign-payload.json'")
+
+    if not signing_json_input_file:


This is not documented, and I'm not sure if it's intuitive. Shouldn't it be possible to also post a locally read and then signed payload to the API?

MVrachev · 2024-05-23T11:11:11Z

TODO: remove prompt for server if settings.SERVER and api-server miss.

Think if we can use default value of setting.SERVER for api-server for admin top-level command

MVrachev · 2024-05-25T14:01:03Z

@lukpueh I addressed your comments in #600.
I would appreciate it if you have a look there. :)

MVrachev requested a review from kairoaraujo May 13, 2024 21:08

MVrachev self-assigned this May 13, 2024

kairoaraujo reviewed May 14, 2024

View reviewed changes

repository_service_tuf/cli/admin/sign.py Show resolved Hide resolved

lukpueh suggested changes May 14, 2024

View reviewed changes

MVrachev force-pushed the admin2-add-signing-api-support branch 2 times, most recently from e00fb4d to 3ac2581 Compare May 17, 2024 10:30

MVrachev added 7 commits May 20, 2024 12:55

New admin sign: add support for API communication

006b60e

Add support to fetch pending roles for signing and send the result back to the API. Added tests for all cases. Signed-off-by: Martin Vrachev <[email protected]>

Small optimizations

ad5ea1e

Signed-off-by: Martin Vrachev <[email protected]>

Remove choice from pending_roles

d4c36e3

Signed-off-by: Martin Vrachev <[email protected]>

Admin sign: add support for local signing

06d2b1c

Signed-off-by: Martin Vrachev <[email protected]>

Fix unit tests

c7df63d

Signed-off-by: Martin Vrachev <[email protected]>

Update docs

1bd4a43

Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the admin2-add-signing-api-support branch from 46795fc to 1bd4a43 Compare May 20, 2024 10:04

MVrachev mentioned this pull request May 20, 2024

Prepare ft tests for cli signing API communication repository-service-tuf/repository-service-tuf#713

Merged

MVrachev added 2 commits May 21, 2024 23:40

Merge branch 'main' into admin2-add-signing-api-support

8a01e42

Merge branch 'main' into admin2-add-signing-api-support

46b0a7e

kairoaraujo approved these changes May 22, 2024

View reviewed changes

kairoaraujo merged commit dc7a52b into repository-service-tuf:main May 22, 2024
13 of 14 checks passed

lukpueh reviewed May 23, 2024

View reviewed changes

MVrachev deleted the admin2-add-signing-api-support branch May 25, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New admin sign: add support for API communication & add tests #579

New admin sign: add support for API communication & add tests #579

MVrachev commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

lukpueh May 14, 2024

MVrachev May 14, 2024

kairoaraujo May 14, 2024

lukpueh May 15, 2024

kairoaraujo May 15, 2024

MVrachev May 17, 2024

MVrachev commented May 15, 2024

MVrachev commented May 15, 2024 •

edited

Loading

kairoaraujo commented May 16, 2024

MVrachev commented May 17, 2024

MVrachev commented May 20, 2024

kairoaraujo left a comment

lukpueh left a comment

lukpueh May 23, 2024

MVrachev May 23, 2024

lukpueh May 23, 2024

lukpueh May 23, 2024

lukpueh May 23, 2024

lukpueh May 23, 2024

MVrachev commented May 23, 2024

MVrachev commented May 25, 2024

		@@ -179,11 +179,19 @@ sign (``sign``)
		Usage: rstuf admin metadata sign [OPTIONS] ROOT_IN [PREV_ROOT_IN]


		console.print("Saved result to 'sign-payload.json'")

		if not signing_json_input_file:

New admin sign: add support for API communication & add tests #579

New admin sign: add support for API communication & add tests #579

Conversation

MVrachev commented May 13, 2024 • edited Loading

Description

Types of changes

Additional requirements

Code of Conduct

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

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

MVrachev commented May 15, 2024

MVrachev commented May 15, 2024 • edited Loading

kairoaraujo commented May 16, 2024

MVrachev commented May 17, 2024

MVrachev commented May 20, 2024

kairoaraujo left a comment

Choose a reason for hiding this comment

lukpueh 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

MVrachev commented May 23, 2024

MVrachev commented May 25, 2024

MVrachev commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

MVrachev commented May 15, 2024 •

edited

Loading