Parse `ip%scope` in CLI arguments #443

hawkw · 2024-01-31T21:26:26Z

Currently, the Humility CLI accepts IP addresses as a String, and then
attempts to parse them as an Ipv6Addr and scope. This is a bit
unfortunate, as when argument parsing is performed by clap, the CLI
will emit a somewhat nicer error message when parsing fails (indicating
which argument the error occurred while parsing, including the invalid
value, and suggesting the help text). On the other hand, when we accept
the argument as a String, clap doesn't add that useful context to
the error message.

This branch adds a ScopedV6Addr newtype, consisting of an Ipv6Addr
and a scope identifier, and adds a FromStr implementation for that
type. Now, we can use ScopedV6Addr as the type of the CLI argument,
and clap performs the parsing for us, getting us somewhat nicer error
messages. Compare the error output from master:

$ cargo run -- --ip ::%fakeiface0 probe
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/humility --ip '::%fakeiface0' probe`
humility probe failed: Could not find interface for fakeiface0

$ cargo run -- --ip :: probe
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/humility --ip '::' probe`
humility probe failed: Missing scope id in IP (e.g. '%en0')

$ cargo run -- --ip barf%eth0 probe
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/humility --ip 'barf%eth0' probe`
humility probe failed: invalid Zip archive: Invalid zip header

with the error output on this branch:

$ cargo run -- --ip :: probe
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/humility --ip '::' probe`
error: Invalid value "::" for '--ip <IP>': missing scope ID (e.g. "%en0") in IPv6 address

For more information try --help

$ cargo run -- --ip ::%fakeiface0 probe
    Finished dev [unoptimized + debuginfo] target(s) in 8.45s
     Running `target/debug/humility --ip '::%fakeiface0' probe`
error: Invalid value "::%fakeiface0" for '--ip <IP>': Could not find interface for fakeiface0

For more information try --help

$ cargo run -- --ip barf%eth0 probe
    Finished dev [unoptimized + debuginfo] target(s) in 12.87s
     Running `target/debug/humility --ip 'barf%eth0' probe`
error: Invalid value "barf%eth0" for '--ip <IP>': "barf" is not a valid IPv6 address

For more information try --help

Additionally, using the ScopedIpv6Addr type allows us to provide a
fmt::Display implementation that combines the IP address and scope ID
when formatting. This lets us get rid of a little bit of repeated logic
for formatting an address with the delimiter, which seems kinda nice to
me.

Currently, the Humility CLI accepts IP addresses as a `String`, and then attempts to parse them as an `Ipv6Addr` and scope. This is a bit unfortunate, as when argument parsing is performed by `clap`, the CLI will emit a somewhat nicer error message when parsing fails (indicating which argument the error occurred while parsing, including the invalid value, and suggesting the help text). On the other hand, when we accept the argument as a `String`, `clap` doesn't add that useful context to the error message. This branch adds a `ScopedV6Addr` newtype, consisting of an `Ipv6Addr` and a scope identifier, and adds a `FromStr` implementation for that type. Now, we can use `ScopedV6Addr` as the type of the CLI argument, and `clap` performs the parsing for us, getting us somewhat nicer error messages. Compare the error output from master: ```console $ cargo run -- --ip ::%fakeiface0 probe Finished dev [unoptimized + debuginfo] target(s) in 0.13s Running `target/debug/humility --ip '::%fakeiface0' probe` humility probe failed: Could not find interface for fakeiface0 $ cargo run -- --ip :: probe Finished dev [unoptimized + debuginfo] target(s) in 0.13s Running `target/debug/humility --ip '::' probe` humility probe failed: Missing scope id in IP (e.g. '%en0') $ cargo run -- --ip barf%eth0 probe Finished dev [unoptimized + debuginfo] target(s) in 0.13s Running `target/debug/humility --ip 'barf%eth0' probe` humility probe failed: invalid Zip archive: Invalid zip header ``` with the error output on this branch: ```console $ cargo run -- --ip :: probe Finished dev [unoptimized + debuginfo] target(s) in 0.14s Running `target/debug/humility --ip '::' probe` error: Invalid value "::" for '--ip <IP>': missing scope ID (e.g. "%en0") in IPv6 address For more information try --help $ cargo run -- --ip ::%fakeiface0 probe Finished dev [unoptimized + debuginfo] target(s) in 8.45s Running `target/debug/humility --ip '::%fakeiface0' probe` error: Invalid value "::%fakeiface0" for '--ip <IP>': Could not find interface for fakeiface0 For more information try --help $ cargo run -- --ip barf%eth0 probe Finished dev [unoptimized + debuginfo] target(s) in 12.87s Running `target/debug/humility --ip 'barf%eth0' probe` error: Invalid value "barf%eth0" for '--ip <IP>': "barf" is not a valid IPv6 address For more information try --help ``` Additionally, using the `ScopedIpv6Addr` type allows us to provide a `fmt::Display` implementation that combines the IP address and scope ID when formatting. This lets us get rid of a little bit of repeated logic for formatting an address with the delimiter, which seems kinda nice to me.

cbiffle · 2024-02-01T00:20:58Z

humility probe failed: invalid Zip archive: Invalid zip header

Hoo boy. I can't even explain to myself why this would happen, particularly for probe which doesn't use the archive.

hawkw · 2024-02-01T00:24:52Z

humility probe failed: invalid Zip archive: Invalid zip header

Hoo boy. I can't even explain to myself why this would happen, particularly for probe which doesn't use the archive.

Yeah, I, uh...have no idea what's going on with that...

cbiffle

This is an improvement! The two remaining sites of manually copy-pasted scope ID parsing that I'm aware of are in cmd/rpc and cmd/gimlet -- if you've got the bandwidth to fix those in this PR that'd rock, though IMO it's an improvement either way. One suggestion below.

cbiffle · 2024-02-01T00:22:02Z

humility-cli/src/lib.rs

@@ -158,7 +158,7 @@ impl ExecutionContext {
            if let Ok(e) = env::var("HUMILITY_PROBE") {
                cli.probe = Some(e);
            } else if let Ok(e) = env::var("HUMILITY_IP") {


Huh, this is news to me -- some commands use a different env var for this, like HUMILITY_RPC_IP. Wonder how that happened.

humility-core/src/net.rs

cbiffle · 2024-02-01T00:25:40Z

Yeah, I, uh...have no idea what's going on with that...

Humility does some manual wrangling of Clap, e.g. not using its built-in subcommand facility, so it's possible we confused it somehow or messed up an error handling path somewhere.

cbiffle · 2024-02-01T00:30:41Z

Ah, I understand the zip archive failure, I think.

When no archive is given, Humility still creates a HubrisArchive and passes it around, only a bunch of its methods now fail. Some of them don't, however, and in particular .archive() appears to return &[] rather than, like, being an Option. This angers the Zip gods.

So the error is, deceptively, not related to IP address parsing -- Humility has actually accepted barf%eth0 and is about to try connecting (at which point I bet it gets a name resolution failure).

I had to change it to barf%enps2s0f0 to repro this on my machine because of Linux stable interface names.

cbiffle · 2024-02-01T00:32:19Z

...more generally, it looks like --ip is accepted as a global flag, which is news to the rpc command that reimplements it, and causes all commands to start requiring an archive whether they need it or not -- failing to provide an archive gets you this error message, currently, because of assumptions in NetCore.

humility-core/src/net.rs

hawkw · 2024-02-01T00:55:03Z

When no archive is given, Humility still creates a HubrisArchive and passes it around, only a bunch of its methods now fail. Some of them don't, however, and in particular .archive() appears to return &[] rather than, like, being an Option. This angers the Zip gods.

Ah, yeah, that makes sense...it's a bit unfortunate that the failure thus ends up being "the zip file is missing a header" rather than "there is no zip file at all". I could look into fixing that separately, if you like?

hawkw · 2024-02-01T00:56:47Z

The two remaining sites of manually copy-pasted scope ID parsing that I'm aware of are in cmd/rpc and cmd/gimlet -- if you've got the bandwidth to fix those in this PR that'd rock,

Now that your gimlet-inspector PR has merged, I'll go ahead and pick up those changes so we can get that code using this as well, and I'll see about getting the rpc command on board too!

cbiffle · 2024-02-01T01:21:43Z

Ah, yeah, that makes sense...it's a bit unfortunate that the failure thus ends up being "the zip file is missing a header" rather than "there is no zip file at all". I could look into fixing that separately, if you like?

I'm prepping a fix, unfortunately the place where the failure happens is outside Humility's normal "I have no archive but I must scream" failure path, so the reporting will be a tad shoddy, but I can get it to say

humility probe failed: archive is required but was not provided

(The reason it's outside the failure path: probe literally does not require an archive, this requirement is coming from the internals of NetCore.)

Because `humility rpc` accepts a list of addresses or a `-i <INTERFACE>` (with `--listen`), we still need to use `decode_iface` directly when called with `-i`.

hawkw · 2024-02-01T17:53:59Z

@cbiffle I've updated the humility gimlet and humility rpc commands to also use the new parsing logic. Let me know if you want to take another look at the latest changes? Otherwise, I'm happy to merge when CI passes.

hawkw requested review from bcantrill and cbiffle January 31, 2024 21:27

hawkw mentioned this pull request Jan 31, 2024

gimlet inspector command: first version #442

Merged

cbiffle approved these changes Feb 1, 2024

View reviewed changes

add a doc comment

46fe68e

cbiffle reviewed Feb 1, 2024

View reviewed changes

humility-core/src/net.rs Show resolved Hide resolved

Merge branch 'master' into eliza/out-of-scope

24c0d7f

hawkw added 2 commits February 1, 2024 09:41

Use ScopedV6Addr in humility gimlet

093e628

Use ScopedV6Addr in humility rpc

aa598f1

Because `humility rpc` accepts a list of addresses or a `-i <INTERFACE>` (with `--listen`), we still need to use `decode_iface` directly when called with `-i`.

cbiffle approved these changes Feb 1, 2024

View reviewed changes

Merge branch 'master' into eliza/out-of-scope

f728bf8

hawkw enabled auto-merge (squash) February 1, 2024 22:13

hawkw merged commit 94ed14a into master Feb 1, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse `ip%scope` in CLI arguments #443

Parse `ip%scope` in CLI arguments #443

hawkw commented Jan 31, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

cbiffle left a comment •

edited

Loading

cbiffle Feb 1, 2024

cbiffle commented Feb 1, 2024

cbiffle commented Feb 1, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

hawkw commented Feb 1, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

Parse ip%scope in CLI arguments #443

Parse ip%scope in CLI arguments #443

Conversation

hawkw commented Jan 31, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

cbiffle left a comment • edited Loading

Choose a reason for hiding this comment

cbiffle Feb 1, 2024

Choose a reason for hiding this comment

cbiffle commented Feb 1, 2024

cbiffle commented Feb 1, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

hawkw commented Feb 1, 2024

cbiffle commented Feb 1, 2024

hawkw commented Feb 1, 2024

Parse `ip%scope` in CLI arguments #443

Parse `ip%scope` in CLI arguments #443

cbiffle left a comment •

edited

Loading