fix(ip): fail tnet ip -6 on a node with no IPv6 instead of exiting 0 - #394
Conversation
`tnet ip -4` and `tnet ip -6` asked for an address family the target does
not have, printed `(no matching tailnet address)` on stdout, and exited 0.
Upstream does the opposite: `runIP`'s match loop sets a `match` flag, and
`if !match` returns `errors.New("no Tailscale IPv4 address")` for `-4` and
`"no Tailscale IPv6 address"` for `-6`. Those are error returns, not
`outln` calls, and that is the point of them — the CLI turns an error into
a line on stderr and a non-zero exit, so a script can write
`if tailscale ip -6 host; then ...`. Ours told such a script that an
IPv4-only node has an IPv6 address, on stdout, ready to be captured into a
variable.
Ported from `cmd/tailscale/cli/ip.go` at upstream v1.102.3,
`53a0d659afa51835dd7a9283873cca44261454f8` (the same file and ref the
surrounding `tnet ip` code cites).
WHAT CHANGED
`ip_no_match_error(sel)` is Go's `if !match` tail on its own: `-4` and `-6`
each name the family that came up empty, and neither flag yields `None`
because with both families wanted an empty answer means the target had no
address at all. `format_ip_filtered` and `format_service_ips` now return
`Result<String, &'static str>` and consult it before falling back to the
placeholder; `run_ip` prints an `Ok` and `bail!`s an `Err`, which reaches
the user as stderr text and exit 1.
One helper rather than two copies because Go has one: `runIP` resolves this
node, a peer, or a Service into a single `ips` slice and runs one match loop
over it. `tnet ip -4 <IPv6-only-service-VIP>` is the same question with the
same answer, so it takes the same path. Splitting it would have left two
arms of one Go loop disagreeing about what an unsatisfiable `-4` means.
The placeholder is not gone. It still stands in for the unflagged empty
answer, which is a node holding no address at all.
WHAT IS STILL DIFFERENT (not fixed here, and not part of this change)
Go checks `len(ips) == 0` BEFORE the match loop and returns
`no current Tailscale IPs; state: %v`. That message is built from
`BackendState`, which the `ip` wire response does not carry, so `tnet ip`
with no family flag on a node with no address still prints the placeholder
and exits 0 where Go exits 1. Fixing it means either putting the backend
state on that response or making `tnet ip` ask for `status` instead — a
protocol change, and a separate piece of work. A consequence worth naming:
with `-4` or `-6` set, that same no-address state now takes the new error
path, so it exits non-zero with the family message rather than Go's
state-bearing one. Right stream and right exit status, different words.
VERIFICATION
Four unit tests in `src/bin/tnet.rs` drive the production functions:
`ip_family_filter_that_selects_nothing_is_an_error_not_a_line` (the field
case, `-6` on an IPv4-only node, and its `-4` mirror), and the family
assertions inside `format_ip_filtered_selects_family_and_first`,
`ip_prints_the_whole_matched_node_whichever_address_named_it` (an
IPv6-only peer asked for `-4`) and
`service_ips_honor_the_family_and_first_filters` (the Service arm).
Stubbing `ip_no_match_error` back to `None` — the old behaviour — fails 5
of them; restored, `cargo test --bin tnet` is 302 passed, 0 failed.
Full gate, clean: `cargo fmt --all --check`, `cargo clippy --all-targets --
-D warnings`, `cargo test --all-targets` (all suites pass),
`cargo build --release --bins`.
STALE ELSEWHERE
Nothing in the parity ledger is edited here. What this makes stale: any
note describing `tnet ip`'s family filter as answering with a placeholder,
and any claim that the `#319` re-derivation of `format_ip_filtered` leaves
the `!match` branch diverging — it no longer does, except for the
`len(ips) == 0` case described above.
READ FIRST
`ip_no_match_error` and the two `out.is_empty()` branches that call it.
DECISIONS
- The error text is Go's, verbatim and unprefixed (`no Tailscale IPv6
address`), not re-spelled with `tnet`. The `tnet ip` refusal that already
exists is re-spelled (`tnet ip -1, -4, and -6 are mutually exclusive`)
because it names flags of this binary; this one names a fact about the
tailnet and reads identically either way, so it stays as upstream wrote
it and greps the same.
- `bail!` rather than `eprintln!` + `exit(1)`, which the peer-lookup miss a
few lines up uses. Both give stderr and exit 1; `bail!` is what the `ip`
usage refusal in the same function already does, and it keeps the
formatter's error a value the caller decides about rather than a process
exit buried in a formatter.
- No end-to-end exit-status test. Reaching the branch through the built
binary needs a daemon reporting one family and not the other, which the
in-tree harness cannot produce without a real tailnet. The unit tests
call the production functions and the caller is one `match`.
QUESTIONS
- Should `no current Tailscale IPs; state: <state>` 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. I would do it, but as
its own change: it touches the wire DTO, which this one deliberately does
not.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe IP formatting functions now return errors for unsatisfied ChangesIP family selection errors
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The command now reports an unavailable requested IP family on stderr with a non-zero exit while preserving the unfiltered empty-result placeholder. Covered node, peer, and Service cases support merge readiness. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…y-filter-exit-status
tnet ip -4andtnet ip -6asked for an address family the target doesnot have, printed
(no matching tailnet address)on stdout, and exited 0.Upstream does the opposite:
runIP's match loop sets amatchflag, andif !matchreturnserrors.New("no Tailscale IPv4 address")for-4and"no Tailscale IPv6 address"for-6. Those are error returns, notoutlncalls, and that is the point of them — the CLI turns an error intoa line on stderr and a non-zero exit, so a script can write
if tailscale ip -6 host; then .... Ours told such a script that anIPv4-only node has an IPv6 address, on stdout, ready to be captured into a
variable.
Ported from
cmd/tailscale/cli/ip.goat upstream v1.102.3,53a0d659afa51835dd7a9283873cca44261454f8(the same file and ref thesurrounding
tnet ipcode cites).WHAT CHANGED
ip_no_match_error(sel)is Go'sif !matchtail on its own:-4and-6each name the family that came up empty, and neither flag yields
Nonebecause with both families wanted an empty answer means the target had no
address at all.
format_ip_filteredandformat_service_ipsnow returnResult<String, &'static str>and consult it before falling back to theplaceholder;
run_ipprints anOkandbail!s anErr, which reachesthe user as stderr text and exit 1.
One helper rather than two copies because Go has one:
runIPresolves thisnode, a peer, or a Service into a single
ipsslice and runs one match loopover it.
tnet ip -4 <IPv6-only-service-VIP>is the same question with thesame answer, so it takes the same path. Splitting it would have left two
arms of one Go loop disagreeing about what an unsatisfiable
-4means.The placeholder is not gone. It still stands in for the unflagged empty
answer, which is a node holding no address at all.
WHAT IS STILL DIFFERENT (not fixed here, and not part of this change)
Go checks
len(ips) == 0BEFORE the match loop and returnsno current Tailscale IPs; state: %v. That message is built fromBackendState, which theipwire response does not carry, sotnet ipwith no family flag on a node with no address still prints the placeholder
and exits 0 where Go exits 1. Fixing it means either putting the backend
state on that response or making
tnet ipask forstatusinstead — aprotocol change, and a separate piece of work. A consequence worth naming:
with
-4or-6set, that same no-address state now takes the new errorpath, so it exits non-zero with the family message rather than Go's
state-bearing one. Right stream and right exit status, different words.
VERIFICATION
Four unit tests in
src/bin/tnet.rsdrive the production functions:ip_family_filter_that_selects_nothing_is_an_error_not_a_line(the fieldcase,
-6on an IPv4-only node, and its-4mirror), and the familyassertions inside
format_ip_filtered_selects_family_and_first,ip_prints_the_whole_matched_node_whichever_address_named_it(anIPv6-only peer asked for
-4) andservice_ips_honor_the_family_and_first_filters(the Service arm).Stubbing
ip_no_match_errorback toNone— the old behaviour — fails 5of them; restored,
cargo test --bin tnetis 302 passed, 0 failed.Full gate, clean:
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo test --all-targets(all suites pass),cargo build --release --bins.STALE ELSEWHERE
Nothing in the parity ledger is edited here. What this makes stale: any
note describing
tnet ip's family filter as answering with a placeholder,and any claim that the
#319re-derivation offormat_ip_filteredleavesthe
!matchbranch diverging — it no longer does, except for thelen(ips) == 0case described above.READ FIRST
ip_no_match_errorand the twoout.is_empty()branches that call it.DECISIONS
no Tailscale IPv6 address), not re-spelled withtnet. Thetnet iprefusal that alreadyexists is re-spelled (
tnet ip -1, -4, and -6 are mutually exclusive)because it names flags of this binary; this one names a fact about the
tailnet and reads identically either way, so it stays as upstream wrote
it and greps the same.
bail!rather thaneprintln!+exit(1), which the peer-lookup miss afew lines up uses. Both give stderr and exit 1;
bail!is what theipusage refusal in the same function already does, and it keeps the
formatter's error a value the caller decides about rather than a process
exit buried in a formatter.
binary needs a daemon reporting one family and not the other, which the
in-tree harness cannot produce without a real tailnet. The unit tests
call the production functions and the caller is one
match.QUESTIONS
no current Tailscale IPs; state: <state>be ported too, byputting the backend state on the
ipresponse? It is the last piece ofrunIP's tail that still exits 0 where Go exits 1. I would do it, but asits own change: it touches the wire DTO, which this one deliberately does
not.
bead
tsd-aud319· sessiontsd-aud319-c581-r1-s1n0· baserefs/heads/mainSummary by CodeRabbit
tnet ipcommand now exits with an error when an explicitly requested IPv4 or IPv6 address is unavailable.