fix(ip): make an empty ip -4/-6 fail like Go instead of exiting 0 - #396
fix(ip): make an empty ip -4/-6 fail like Go instead of exiting 0#396GeiserX wants to merge 1 commit into
ip -4/-6 fail like Go instead of exiting 0#396Conversation
`tnet ip -6` on a node whose tailnet holds no IPv6 printed
`(no matching tailnet address)` on stdout and exited 0. A script that
tests the exit status — `if tnet ip -6 "$host"; then ...` — read that as
"the host has an IPv6 address". It does not.
Upstream is `cmd/tailscale/cli/ip.go` (`runIP`) @
53a0d659afa51835dd7a9283873cca44261454f8. Its match loop sets `match`
only when it printed an address, and the function ends:
if !match {
if ipArgs.want4 { return errors.New("no Tailscale IPv4 address") }
if ipArgs.want6 { return errors.New("no Tailscale IPv6 address") }
}
return nil
Those are `errors.New` returns, not `outln` calls. That is the whole
reason upstream wrote them as errors: the CLI turns a returned error into
a stderr line and a non-zero exit, so the command is usable as a
predicate. This port had re-derived the branch as a printed placeholder.
WHAT CHANGED
`format_ip_filtered` and `format_service_ips` now return
`Result<String, &'static str>`. The empty-selection branch consults the
new `ip_no_match_error`, which is Go's `!match` tail verbatim: `-4` gives
`no Tailscale IPv4 address`, `-6` gives `no Tailscale IPv6 address`, and
with neither flag it gives `None`. `run_ip` prints an `Ok` and
`anyhow::bail!`s an `Err`, which reaches the shell as stderr plus exit 1.
Three shape decisions worth naming:
- The branch is on the RAW `-4`/`-6` flags, not on a derived "both
families wanted" pair. Go branches on `ipArgs.want4`/`want6`, not on
the local `v4` that defaults to true, so an unflagged empty answer
correctly stays a non-error and keeps the placeholder.
- `format_service_ips` moves too. Both formatters feed the same `let out`
expression in `run_ip`, so they share a return type; and Go really does
run one match loop over one `ips` slice however the target resolved, so
the Service arm owes the same error. Not scope creep — forced.
- The unflagged empty answer keeps the placeholder rather than borrowing
Go's other message. Go covers that state EARLIER, with
`no current Tailscale IPs; state: %v`, built from `BackendState` — see
QUESTIONS.
HOW IT WAS VERIFIED
New `tests/tnet_ip_family_filter_exit_status.rs` runs the built `tnet`
against a stub daemon on a Unix socket, the way the lock/cert/kubeconfig
tests do, and pins the surface a script actually sees: exit status,
stdout, stderr. Four cases — `-6` against an IPv4-only reply, `-4`
against an IPv6-only reply, a positive control (`-4` against a node that
has one, so exit 0 with the address on stdout), and the unflagged empty
answer (still exit 0). The control is there so a broken socket path
cannot make the two negative cases pass vacuously.
Checked against the pre-fix binary: the two negative cases fail with
`stdout: (no matching tailnet address)` and a zero exit, while the
control and the unflagged case pass either way. The unit tests beside
`format_ip_filtered` pin which state is an error; this file pins the
wiring, so a future edit that turned the `bail!` back into an
`eprintln!` + `Ok(())`, or that printed the message on stdout, fails
here.
Gate, all four green:
`cargo fmt --all --check`; `cargo clippy --all-targets -- -D warnings`;
`cargo test --all-targets` (437 + 302 unit tests plus every integration
suite, 0 failed); `cargo build --release --bins`.
WHAT TO READ FIRST
`ip_no_match_error` in `src/bin/tnet.rs` — it is the ported tail, and its
`else { None }` arm is the part an over-eager fix gets wrong. Then the
`match out` at the end of `run_ip`, then the new test file.
STALE ELSEWHERE
`docs/PARITY_GAP_ANALYSIS.md` is not touched here, but its `tailscale ip`
row no longer holds: the `-4`/`-6` family filter now matches upstream's
error return and exit status, so only the address-less unflagged case
(below) still diverges.
DECISIONS
- The error text is a `&'static str` carried through the formatters,
rather than an error type. `anyhow::bail!` on a `&'static str` is
already the idiom in this same function (`ip_usage_refusal` does it),
and Go's strings are fixed, so nothing needs formatting.
- The `-1` evaluation-order comments in both formatters are unchanged.
Upstream's `-1` reads as a `break` inside the match branch rather than
a slice truncation ahead of it; the two are indistinguishable in
behaviour here and `ip_usage_refusal` makes the difference unreachable,
so rewording them belongs in its own change.
- `ip_refusal_covers_the_service_arm_that_would_answer_emptily` is
renamed to `..._that_would_select_nothing`, and its rationale rewritten.
The old name and the sentence under it said the command "would answer
that empty set" — after this change it would not; it would report the
family error. The refusal is still worth testing, only its
justification had to move.
QUESTIONS
Q: Should `no current Tailscale IPs; state: %v` be ported too, by putting
the backend state on the `ip` response? It is the last piece of `runIP`'s
tail that still exits 0 where Go exits 1.
A: Yes, but as its own change — it needs a new field on `Response::Ip` in
`src/localapi.rs` plus the daemon side, which is a wire-DTO change, and
this one is a CLI-side fix. Two notes for whoever takes it. First, the
remaining gap is narrower than it sounds: with `-4` or `-6` set, a node
holding no address at all now exits non-zero here, with the family text
rather than the state text. What still exits 0 is only the unflagged (or
`-1`-only) call against an address-less node. Second, Go checks
`len(ips) == 0` BEFORE the match loop, so `no current Tailscale IPs` must
win over `no Tailscale IPv6 address` when both would apply.
|
Warning Review limit reachedNext included review available in 29 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superseded by a sibling pull request from the same task, which has already merged — this one would land the same change twice, so it can never be merged. Closing it to free the factory's open-pull-request budget. The branch is kept: reopen this if you want this variant instead. |
tnet ip -6on a node whose tailnet holds no IPv6 printed(no matching tailnet address)on stdout and exited 0. A script thattests the exit status —
if tnet ip -6 "$host"; then ...— read that as"the host has an IPv6 address". It does not.
Upstream is
cmd/tailscale/cli/ip.go(runIP) @53a0d659afa51835dd7a9283873cca44261454f8. Its match loop sets
matchonly when it printed an address, and the function ends:
Those are
errors.Newreturns, notoutlncalls. That is the wholereason upstream wrote them as errors: the CLI turns a returned error into
a stderr line and a non-zero exit, so the command is usable as a
predicate. This port had re-derived the branch as a printed placeholder.
WHAT CHANGED
format_ip_filteredandformat_service_ipsnow returnResult<String, &'static str>. The empty-selection branch consults thenew
ip_no_match_error, which is Go's!matchtail verbatim:-4givesno Tailscale IPv4 address,-6givesno Tailscale IPv6 address, andwith neither flag it gives
None.run_ipprints anOkandanyhow::bail!s anErr, which reaches the shell as stderr plus exit 1.Three shape decisions worth naming:
-4/-6flags, not on a derived "bothfamilies wanted" pair. Go branches on
ipArgs.want4/want6, not onthe local
v4that defaults to true, so an unflagged empty answercorrectly stays a non-error and keeps the placeholder.
format_service_ipsmoves too. Both formatters feed the samelet outexpression in
run_ip, so they share a return type; and Go really doesrun one match loop over one
ipsslice however the target resolved, sothe Service arm owes the same error. Not scope creep — forced.
Go's other message. Go covers that state EARLIER, with
no current Tailscale IPs; state: %v, built fromBackendState— seeQUESTIONS.
HOW IT WAS VERIFIED
New
tests/tnet_ip_family_filter_exit_status.rsruns the builttnetagainst a stub daemon on a Unix socket, the way the lock/cert/kubeconfig
tests do, and pins the surface a script actually sees: exit status,
stdout, stderr. Four cases —
-6against an IPv4-only reply,-4against an IPv6-only reply, a positive control (
-4against a node thathas one, so exit 0 with the address on stdout), and the unflagged empty
answer (still exit 0). The control is there so a broken socket path
cannot make the two negative cases pass vacuously.
Checked against the pre-fix binary: the two negative cases fail with
stdout: (no matching tailnet address)and a zero exit, while thecontrol and the unflagged case pass either way. The unit tests beside
format_ip_filteredpin which state is an error; this file pins thewiring, so a future edit that turned the
bail!back into aneprintln!+Ok(()), or that printed the message on stdout, failshere.
Gate, all four green:
cargo fmt --all --check;cargo clippy --all-targets -- -D warnings;cargo test --all-targets(437 + 302 unit tests plus every integrationsuite, 0 failed);
cargo build --release --bins.WHAT TO READ FIRST
ip_no_match_errorinsrc/bin/tnet.rs— it is the ported tail, and itselse { None }arm is the part an over-eager fix gets wrong. Then thematch outat the end ofrun_ip, then the new test file.STALE ELSEWHERE
docs/PARITY_GAP_ANALYSIS.mdis not touched here, but itstailscale iprow no longer holds: the
-4/-6family filter now matches upstream'serror return and exit status, so only the address-less unflagged case
(below) still diverges.
DECISIONS
&'static strcarried through the formatters,rather than an error type.
anyhow::bail!on a&'static strisalready the idiom in this same function (
ip_usage_refusaldoes it),and Go's strings are fixed, so nothing needs formatting.
-1evaluation-order comments in both formatters are unchanged.Upstream's
-1reads as abreakinside the match branch rather thana slice truncation ahead of it; the two are indistinguishable in
behaviour here and
ip_usage_refusalmakes the difference unreachable,so rewording them belongs in its own change.
ip_refusal_covers_the_service_arm_that_would_answer_emptilyisrenamed to
..._that_would_select_nothing, and its rationale rewritten.The old name and the sentence under it said the command "would answer
that empty set" — after this change it would not; it would report the
family error. The refusal is still worth testing, only its
justification had to move.
QUESTIONS
Q: Should
no current Tailscale IPs; state: %vbe ported too, by puttingthe backend state on the
ipresponse? It is the last piece ofrunIP'stail that still exits 0 where Go exits 1.
A: Yes, but as its own change — it needs a new field on
Response::Ipinsrc/localapi.rsplus the daemon side, which is a wire-DTO change, andthis one is a CLI-side fix. Two notes for whoever takes it. First, the
remaining gap is narrower than it sounds: with
-4or-6set, a nodeholding no address at all now exits non-zero here, with the family text
rather than the state text. What still exits 0 is only the unflagged (or
-1-only) call against an address-less node. Second, Go checkslen(ips) == 0BEFORE the match loop, sono current Tailscale IPsmustwin over
no Tailscale IPv6 addresswhen both would apply.bead
tsd-aud319· sessiontsd-aud319-c581-r2-s1n0· baserefs/heads/main