Admin commands options modifications #612

MVrachev · 2024-06-06T16:20:13Z

Note: Based on top of pr #607 and umbrella pr repository-service-tuf/repository-service-tuf#742.
Merge those before merging this one.

Description

Introduce admin commands options modifications:

add --api-url as a top-level admin command.
change ceremony OUTPUT arg and sign --save into one common option --out.
change sign SIGNING_JSON_INPUT_FILE argument to --in
The last two changes aim to provide options with simpler names, remove the need of multiple arguments and overall achieve consistency between admin commands.

With the introduction of --api-url as an admin option I have also added
RSTUF API communication for ceremony.

TODO in future prs:

introduce --dry-run option (see Introduce --offline and --api-server for admin & ceremony add support for API #602 (comment))
save file automatically in DEFAULT_PATH for ceremony and sign if --dry-run is set, but --out is not.
after --dry-run is added, then add better docs for ceremony and sign

Related to #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-06-06T16:24:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.94%. Comparing base (47d0a69) to head (b57b1d8).
Report is 97 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #612      +/-   ##
==========================================
+ Coverage   98.90%   98.94%   +0.03%     
==========================================
  Files          20       26       +6     
  Lines        1367     1887     +520     
==========================================
+ Hits         1352     1867     +515     
- Misses         15       20       +5

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

lukpueh · 2024-06-07T08:43:07Z

add --timeout ceremony option

I suggest to add this to the admin base command too. It seems relevant whenever we interact with the api.

lukpueh · 2024-06-07T08:51:30Z

save file automatically in DEFAULT_PATH for ceremony and sign if --dry-run is set, but --out is not.

We can discuss this when we get there, but I prefer to not save it automatically. Dry run usually means no side-effect. Instead we can mention in the help string that --out should be used to persist the result. Then both options remain independent.

MVrachev · 2024-06-07T08:55:20Z

add --timeout ceremony option

I suggest to add this to the admin base command too. It seems relevant whenever we interact with the API.

A good suggestion, will move it there and update the commits while there is no real reviews yet.

MVrachev · 2024-06-07T17:50:46Z

I realized the way we use --timeout for bootstrap is a little strange.
Currently, we accept --timeout as an option for POST /api/v1/bootstrap https://github.com/repository-service-tuf/repository-service-tuf-api/blob/cda8e5d3366feb424c54fc4562391040f79160ae/repository_service_tuf_api/bootstrap.py#L111 and then we use it to make sure that a thread posting to the worker https://github.com/repository-service-tuf/repository-service-tuf-api/blob/cda8e5d3366feb424c54fc4562391040f79160ae/repository_service_tuf_api/bootstrap.py#L230 will finish correctly if bootstrap has succeded or if timeout has been exceeded: https://github.com/repository-service-tuf/repository-service-tuf-api/blob/cda8e5d3366feb424c54fc4562391040f79160ae/repository_service_tuf_api/bootstrap.py#L154

I think we need to discuss how we want to support --timeout:

will it be bootstrap specific for this scenario? Maybe we can rename it to something else?
timeout as an requests.(get|post|delete) option

I think both can work, but we need to rename the bootstrap CLI option to something else which we can then pass in the payload.

Regarding this pr I decided to remove --timeout as a whole.
It can be added in a future pr.

MVrachev · 2024-06-10T12:44:45Z

Rebased on top of main and ready for review @lukpueh and @kairoaraujo.

lukpueh

Looks good. I left a few comments on ceremony, they mostly also apply to the sign command.

repository_service_tuf/cli/admin/ceremony.py

lukpueh · 2024-06-10T15:11:19Z

repository_service_tuf/cli/admin/ceremony.py

+    if settings.get("SERVER"):
+        bs_status = bootstrap_status(settings)
+        if bs_status.get("data", {}).get("bootstrap") is True:
+            raise click.ClickException(f"{bs_status.get('message')}")


Do we need to check the bootstrap status here? Wouldn't it be enough, if the server was ready for bootstrap at the time of sending the bootstrap message below? We need to handle the error case, where the server wasn't ready, anyway, because things can change between here and line 124.

IMO adding this LBYL check here is premature ux optimisation and I suggest to remove it.

I think it's better with the check than without it.
It's better to receive a message that bootstrap has been done already:

as compared to doing all of the work for the ceremony, adding at least two keys, and signing with one of them just to get:

As I said, we should handle the case (or rather the many possible cases), where the server fails, anyway. This includes giving proper user feedback and likely dumping the result to a local file so that the work is not lost.

Adding this preliminary check but not handling errors is the wrong order of priorities.

We already handle other server problems in bootstrap_status:

I am talking about send_payload. It could fail for all sorts of reasons. If that happens, the user's work will be lost, unless you handle it.

Yes, you re-raise them and work will be lost.

Ahaa, I see now what you mean.
I agree saving the payload in the default path if not saved before sending to the payload sounds smart.
Will add this here as it's a small addition.

Not sure, if we're really on the same page. I was trying to say that the preliminary check (bootstrap_status) only gives you a limited certainty that posting the data (send_payload) will succeed. So I was actually suggesting to reduce functionality and not to add more. :)

I don't agree that it's an unnecessary check.
Calling bootstrap_status early can help the user when:

the given settings.SERVER is not reachable

RSTUF deployment has been bootstrapped already

For 1 he will be able to resolve his --api-url/settings.SERVER before thinking about the ceremony as a whole.
For 2 if we don't check that in the beginning and he does a full ceremony even if we save payload locally it's likely that the ceremony was a waste of time. As probably it doesn't make sense to restart a bootstrap RSTUF deployment.
Also, if 2 happens at the beginning of the ceremony then the user will be able to assess whether restarting RSTUF is required or not without spending the time on ceremony.

If you want to discuss it further maybe it's good to do it on a daily when it's three of us.

Some notes from a conversation with @lukpueh and @kairoaraujo:

if we want to have a pre-check before running the ceremony we need to take into account other failures besides those checked now. For example, the server could become unreachable when we finish the ceremony

one option to consider is to start a server session at the beginning of the ceremony which will be used in the end when sending the payload.

We have decided we would remove this check for now and an issue about this will be created.
Here is the issue: #618

repository_service_tuf/cli/admin/ceremony.py

lukpueh · 2024-06-11T06:11:00Z

repository_service_tuf/cli/admin/ceremony.py

+    if settings.get("SERVER"):
+        bs_status = bootstrap_status(settings)
+        if bs_status.get("data", {}).get("bootstrap") is True:
+            raise click.ClickException(f"{bs_status.get('message')}")


As I said, we should handle the case (or rather the many possible cases), where the server fails, anyway. This includes giving proper user feedback and likely dumping the result to a local file so that the work is not lost.

Adding this preliminary check but not handling errors is the wrong order of priorities.

repository_service_tuf/cli/admin/ceremony.py

MVrachev · 2024-06-13T16:38:19Z

I had to rebase as there where other prs merged, but the only new commit is the last one.
No other changes have been made.
Ready for a review @lukpueh, @kairoaraujo.

lukpueh

LGTM.

lukpueh · 2024-06-17T09:11:07Z

tests/conftest.py

+
+    context = test_context
+    if test_context is None:
+        context = _create_test_context(api_url)


Programming tip: I'd do all of above outside of the helper, and pass only the args and context you want here.

All of the above is done inside invoke_command as even if you don't test --out/--save we want to use it as we save the result of the command in out_file.json inside the isolated filesystem and read the result of the command from this file.
I want the tests to provide only args that are connected to what they are testing.
Also, we are reusing this code in many tests now.

Okay, makes sense. And how is this useful for import_artifacts? And why are you moving the --api-server argument into the context?

Something I am investigating now is how to pass --api-server as it's an admin option.
Meaning it's easy for me to pass options related to the command I am currently testing for example --in and --out for sign command, but I need to pass --api-server to admin -
rstuf admin --api-server <URL> send ... which I have not found yet how to do and that's why I am setting settings.SERVER as a solution for now.

I am currently testing for example --in and --out for sign command,

How can you test passing --out, if your helper always adds it?

By the way, changing the context of the command seems to be the way to go as mentioned by a click core-developer in two issues: pallets/click#831 (comment), pallets/click#832 (comment).
I will add a simple commit mentioning these discussions for reference as to why we do it that way.

I am currently testing for example --in and --out for sign command,

How can you test passing --out, if your helper always adds it?

By not calling my helper. See:

repository-service-tuf-cli/tests/unit/cli/admin/test_ceremony.py

Line 10 in 0d73c3d

def test_ceremony_with_custom_out(

Sure, but we were discussing the helper.

Anyway, I have already approved the PR. So feel free to merge.

tests/conftest.py

MVrachev · 2024-06-17T12:34:33Z

I addressed the latest comment by @lukpueh.
There needs to be a change in our ft tests after that we can approve and check if ft tests will pass and if they do merge.

lukpueh · 2024-06-17T12:43:16Z

tests/conftest.py

-        api_url = args.pop(api_server_flag_index + 1)
-        args.remove("--api-server")
+    if not test_context:
+        test_context = _create_test_context()


lukpueh · 2024-06-17T12:49:06Z

tests/unit/cli/admin/test_import_artifacts.py

-        result = invoke_command(import_artifacts.import_artifacts, [], args)
+        test_context["settings"].SERVER = "http://127.0.0.1"
+
+        result = invoke_command(


You didn't answer my question earlier. Why do you use the helper when testing import_artifacts? The helper's main purpose seems to run invoke in an isolated_filesystem, save the result to a file, and return the contents.

This does not work for import_artifacts, and you even have to explicitly disable the behavior in the helper, when called for import_artifacts.

I want to use a single place where we call client.invoke() as much as I can, so if we need to change how we invoke the commands to have a single place where to do it.
The only place where I don't do this is when we need to manually set custom output.

Maybe I can do that? What do you think?
I think I will try and see how it looks.

I added a commit allowing the use custom output tests with invoke_command.
How does it look for you?

lukpueh

I'm okay with merging this as is and improve things as we move forward.

Let me once more summarise the things I pointed out in previous comments which seem improvement-worthy to me:

Consistent use of --api-server, --in, --out (and --dry-run)

I still think this behaviour is the way to go eventually (note that I just fixed a mistake there)

I also think that, until we add --dry-run, --api-server should be just mandatory period. Right now it depends on --out in ceremony (as discussed here) and on --in in sign. Besides making the future --dry-run implementation a bigger behaviour change than necsessary, this temporary solution also feels inconsistent.

Testing

The helper function design is a matter of taste. In the case of invoke_command, I think you are jumping through quite a few hoops so that you can call it from everywhere (more than half of the code is case handling calling contexts). But I also think it's not worth our time to further discuss this.

MVrachev · 2024-06-18T12:28:28Z

I had to submit one more commit as in my last commit it seems I have not correctly changed the unit test and they were failing. I created a fix.

MVrachev · 2024-06-19T12:47:07Z

The changes from this pr are part of #620 pr.
Commits from Make "--api-server" an admim option up to Fix unit tests were all part of this pr, but due to mistake we merged #620 first.

MVrachev mentioned this pull request Jun 6, 2024

Introduce --offline and --api-server for admin & ceremony add support for API #602

Closed

6 tasks

MVrachev self-assigned this Jun 6, 2024

MVrachev force-pushed the admin-options-modifications branch from 4a90134 to 288561c Compare June 7, 2024 17:38

MVrachev force-pushed the admin-options-modifications branch 3 times, most recently from 46a3916 to 4651bea Compare June 10, 2024 12:40

lukpueh reviewed Jun 10, 2024

View reviewed changes

lukpueh suggested changes Jun 11, 2024

View reviewed changes

MVrachev mentioned this pull request Jun 13, 2024

Task: new admin ceremony: consider adding a pre-ceremony server check #618

Open

1 task

MVrachev force-pushed the admin-options-modifications branch from 549dac5 to 06bf664 Compare June 13, 2024 16:37

MVrachev mentioned this pull request Jun 14, 2024

Add send payload for ceremony, metadata update and sign #620

Merged

6 tasks

kairoaraujo approved these changes Jun 15, 2024

View reviewed changes

lukpueh approved these changes Jun 17, 2024

View reviewed changes

kairoaraujo self-requested a review June 17, 2024 10:40

kairoaraujo approved these changes Jun 17, 2024

View reviewed changes

lukpueh reviewed Jun 17, 2024

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

MVrachev force-pushed the admin-options-modifications branch from e537039 to 7c832d4 Compare June 17, 2024 12:32

lukpueh reviewed Jun 17, 2024

View reviewed changes

lukpueh approved these changes Jun 18, 2024

View reviewed changes

MVrachev closed this Jun 19, 2024

MVrachev force-pushed the admin-options-modifications branch from b57b1d8 to f8b9752 Compare June 19, 2024 12:42

Admin commands options modifications #612

Admin commands options modifications #612

Conversation

MVrachev commented Jun 6, 2024 • edited Loading

Description

Types of changes

Additional requirements

Code of Conduct

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

lukpueh commented Jun 7, 2024

lukpueh commented Jun 7, 2024 • edited Loading

MVrachev commented Jun 7, 2024 • edited Loading

MVrachev commented Jun 7, 2024 • edited Loading

MVrachev commented Jun 10, 2024

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 10, 2024 • edited Loading

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 Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Jun 13, 2024

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

MVrachev commented Jun 18, 2024

MVrachev commented Jun 19, 2024

MVrachev commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading

lukpueh commented Jun 7, 2024 •

edited

Loading

MVrachev commented Jun 7, 2024 •

edited

Loading

MVrachev commented Jun 7, 2024 •

edited

Loading

MVrachev Jun 10, 2024 •

edited

Loading

MVrachev Jun 11, 2024 •

edited

Loading

MVrachev Jun 17, 2024 •

edited

Loading

MVrachev Jun 17, 2024 •

edited

Loading

MVrachev Jun 17, 2024 •

edited

Loading

MVrachev Jun 17, 2024 •

edited

Loading