NLnetLabs / krill

RPKI Certificate Authority and Publication Server written in Rust
https://nlnetlabs.nl/projects/routing/krill/
Mozilla Public License 2.0
289 stars 40 forks source link

Refactor cli to use clap’s derive. #1228

Closed partim closed 2 weeks ago

partim commented 1 month ago

This PR changes how the clients – krillc, krillta, as well as the integration tests – work to better fit the derive model provided by clap. This results in basically everything in the cli module and all the integration tests being different now.

The PR slightly changes the options for both krillc and krillta. For krillc, the --server, --token, --format, and --api options are now before the first subcommand (since they affect all commands). For krillta, those options are now after krillta proxy but before the next subcommand, while --format is now after krillta signer.

This PR also removes client support and integration tests for RTA.

This is a breaking change.

partim commented 1 month ago

The new client code percent-encodes handles that contain slashes. However, the server will reject those in API paths as it doesn’t do percent-decoding. However, it can’t properly deal with such handles, anyway, which will need fixing in a separate PR.

ximon18 commented 3 weeks ago

I see that the new krillc --help behaviour no longer shows the version in the first line but instead a description string.

Also I see that the old code sorts command line subcommands alphabetically, whereas now they are grouped and ordered deliberately.

Are these deliberate changes?

ximon18 commented 3 weeks ago

I note that error messages no longer contain tips, e.g.

Old:

Missing argument: --token, alternatively you may use env var: KRILL_CLI_TOKEN

New:

error: the following required arguments were not provided:
  --token <TOKEN>

Is this a deliberate change?

ximon18 commented 3 weeks ago

The text printed for krillc info --help no longer shows relevant command line arguments:

Old:

krillc-info
Show server info

USAGE:
    krillc info [FLAGS] [OPTIONS]

FLAGS:
        --api        Only show the API call and exit. Or set env: KRILL_CLI_API=1
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -f, --format <type>     Report format: none|json|text (default). Or set env: KRILL_CLI_FORMAT
    -s, --server <URI>      The full URI to the Krill server. Or set env: KRILL_CLI_SERVER
    -t, --token <string>    The secret token for the Krill server. Or set env: KRILL_CLI_TOKEN

New:

Show server info

Usage: krillc --token <TOKEN> info

Options:
  -h, --help  Print help

Various command line arguments are no longer shown, with --server and --api being very relevant and now missing.


Update:

This is made worse by the error message when a user puts the argument in the old position by accident:

New:

$ read -r KRILL_CLI_TOKEN
$ export KRILL_CLI_TOKEN
$ target/release/krillc info -s https://krill.nlnetlabs.nl/
error: unexpected argument '-s' found

Usage: krillc --token <TOKEN> info

For more information, try '--help'.

The issue here is NOT the lack of --token as that is defined as an env var, it's that -s is valid but no longer in that position, so changing the position resolves the issue:

New:

$ target/release/krillc -s https://krill.nlnetlabs.nl/ info
Version: 0.14.4
Started: 2024-07-24T11:13:19+00:00
ximon18 commented 3 weeks ago

One other thing I don't get, though I'm not sure that this is caused by this PR, is that the /health endpoint is an unprotected endpoint, so that load balancers for example can query it without needing credentials, and this works localhost, e.g.:

root@krill:~# wget --no-check-certificate -qSO- https://127.0.0.1:3000/health
  HTTP/1.1 200 OK
  content-type: text/plain
  content-length: 0
  date: Thu, 15 Aug 2024 13:19:17 GMT

But it doesn't work via the krillc command, either with an earlier version or this version:

Old:

root@krill:~# krillc -V
Krill Client 0.14.4
root@krill:~# krillc health
Missing argument: --token, alternatively you may use env var: KRILL_CLI_TOKEN

New:

$ target/release/krillc health -s https://krill.nlnetlabs.nl/
Missing argument: --token, alternatively you may use env var: KRILL_CLI_TOKEN

Update:

Ah, I see why, but this is confusing:

$ target/release/krillc health -s https://krill.nlnetlabs.nl/ --api
GET:
  https://krill.nlnetlabs.nl/api/v1/authorized
Headers:
  Authorization: Bearer *********...

Why does health contact the /api/v1/authorized endpoint rather than the /health endpoint?

ximon18 commented 3 weeks ago

There seems to be an exit code regression:

Old:

$ krillc roas list --ca nlnetlabs
2a04:b907::/48-48 => 0 # Invalid more specific at Coloclue, valid less specific at Vultr
...
$ echo $?
0

New:

$ target/release/krillc roas list --ca nlnetlabs
2a04:b907::/48-48 => 0 # Invalid more specific at Coloclue, valid less specific at Vultr
...
$ echo $?
1
partim commented 2 weeks ago

Regarding the changes to the help output: I suppose they are the price for using the derive model. We could probably try and work around some of them, but I’m not sure whether the krillc is used widely enough to make this worthwhile.

partim commented 2 weeks ago

Why does health contact the /api/v1/authorized endpoint rather than the /health endpoint?

Not sure. I will leave it like it is for now, though.

ximon18 commented 2 weeks ago

Why does health contact the /api/v1/authorized endpoint rather than the /health endpoint?

Not sure. I will leave it like it is for now, though.

This was my bad, this behaviour wasn't introduced by this PR.

partim commented 2 weeks ago

There seems to be an exit code regression:

Nice find! Someone mixed up the two cases …