Refactor cli to use clap’s derive.

[partim]

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]

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]

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]

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]

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]

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]

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

[ximon18]

I've done a partial review, will continue reviewing next week.

[ximon18]

I played with the CLI in a shell, I read most if not all of the actual changed client/server code, and I scanned the modified tests to see if they looked structurally roughly the same as the originals. I did NOT try and work out if the encode() HTTP path percent encoding is correct as I need more coffee or something before I can make sense of RFC 3986. I'll approve this PR with the assumption that the comments I made will be checked. Nice cleanup by the way!

[partim]

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]

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]

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]

There seems to be an exit code regression:

Nice find! Someone mixed up the two cases …