fix(login): take the flags Go's shared up/login flag set gives login - #397
Conversation
`cmd/tailscale/cli/up.go` builds ONE flag set for two commands:
`newUpFlagSet(goos, upArgs, cmd)` registers `--auth-key` and the hidden
`notFalseVar` `--host-routes` unconditionally, and `--nickname` inside
`if cmd == "login"`. `login.go` hands `newUpFlagSet(..., "login")` to the
`login` command and its `Exec` calls the same `runUp`, so upstream
`tailscale login` takes all three.
`tnet login` took none of them. It carried only `--authkey`,
`--authkey-file` and `--login-server`, so a command line copied from
upstream — `tnet login --auth-key=tskey-… `, `tnet login --host-routes`,
`tnet login --nickname=work` — died at the parser with exit 2 before it
reached the daemon. The `up` side had already gained the same spellings,
which is what made the asymmetry easy to miss: the flags were "ported",
just onto one of the two commands that share them.
Read against cmd/tailscale/cli/up.go and cmd/tailscale/cli/login.go at
53a0d659afa51835dd7a9283873cca44261454f8 (v1.102.3): up.go:90-140 for the
shared flag set, cli.go:65-90 for `CleanUpArgs`, up.go:924 and
ipn/ipnlocal/profiles.go for what `--nickname` does.
What changed
* `login` takes `--auth-key` as a visible alias of `--authkey`, exactly
as `up` does. Upstream `CleanUpArgs` rewrites `--authkey` to
`--auth-key` for every subcommand, so both spellings work on both
commands there. The `file:<path>` value form already reached `login`
(it is resolved inside the shared `resolve_authkey`) and is unchanged.
* `login` takes the hidden `--host-routes` with Go's `notFalseVar`
shape, and refuses every value but `true` with the message `up`
already used. The check moved into `check_host_routes`, shared by both
commands the way the upstream registration is, and on `login` it runs
before every other check — Go decides it in the flag parser, before
`Exec` runs.
* `login` takes `--nickname` and applies it, rather than accepting it
inertly. It is sent as a one-pref `set` request just before the login
round trip. That shape, rather than a new field on the login's `up`
request, because the daemon's `set` path is the only door to both
halves Go's `profileManager.SetPrefs` does — persist `ProfileName` and
rename the current login profile, so `tnet switch <NAME>` resolves it
— and because the login's `up` request has to keep mentioning no pref:
that is what exempts `login` from the accidental-revert guard. It is
ordered after the auth key resolves (an unreadable `file:` fails with
nothing renamed) and before the re-auth, matching upstream, where the
name is in the prefs handed to `Start` and so outlives an interactive
auth the operator never completes.
* `up --nickname` is still refused — upstream's `up` has no such flag
either — but the refusal now names `tnet login --nickname` next to
`tnet set --nickname` instead of recording that `login`'s is missing.
`login --nickname=` (empty) clears, the same clear-by-empty-value form
`tnet set --nickname=` takes and the one Go's own `fmtFlagValueArg`
renders. It therefore inherits the already-recorded divergence in
`rename_current_profile`: Go's `setProfilePrefs` puts the account's login
name back where this fork stores an empty name. That is one finding's job
and not this one's; nothing here makes it worse.
Tests
* `tests/tnet_up_go_flag_spellings.rs` gained the `login` counterparts
of its three `up` tests, driving the built `tnet` binary: every flag
the shared set gives `login` gets past the parser, `login
--host-routes=false` carries Go's refusal, and `--nickname` is a
declared `login` flag while `--auth-key` stays an alias rather than
becoming a second flag. With `src/bin/tnet.rs` stashed back to the
parent commit, all three fail and the other three still pass:
`unexpected argument '--auth-key' found` (exit 2), `unexpected
argument '--host-routes' found` where the refusal should be, and
`login --help` listing no `--nickname`.
* `src/bin/tnet.rs` unit tests: `login_takes_the_flags_gos_shared_flag_
set_gives_it`, `login_host_routes_accepts_only_the_value_go_allows`,
and `login_nickname_names_the_profile_and_mentions_no_other_pref`,
which calls `login_nickname_request` and asserts the built request
names the nickname and leaves all sixteen other prefs at the
unchanged sentinel.
Verified with, from the repo root: `cargo fmt --all --check`, `cargo
clippy --all-targets -- -D warnings`, `cargo test --all-targets`, `cargo
build --release --bins` — all four clean.
Read first: `run_login` and `login_nickname_request` in `src/bin/tnet.rs`,
then the three new tests in `tests/tnet_up_go_flag_spellings.rs`.
Stale elsewhere: any parity note that says `login --nickname` is not
implemented here, or that Go's `--auth-key`/`--host-routes` spellings are
carried on `up` alone, no longer holds. The parity ledger is not edited
here on purpose.
DECISIONS
* The profile name rides the existing `set` request instead of a new
field on the `up` wire request. Reversible either way; this one was
chosen because a pref on the login's `up` request would make that
request "mention a pref" and expose `login` to the accidental-revert
guard, and because `node_nickname` is deliberately not an up-managed
pref (it is excluded from `--reset`, since Go registers `--reset` on
`up` only, where `--nickname` does not exist).
* The rename step prints nothing of its own. Upstream prints nothing for
it either, and a "preferences updated" line ahead of the login's `ok:`
would only read as a second command having run.
QUESTIONS
* Upstream's `login` shares the WHOLE pref surface, not just these three
flags: `--hostname`, `--advertise-routes`, `--ssh`, `--accept-dns`,
`--operator` and the rest are all on `newUpFlagSet`, and `login.go`
calls `SwitchToEmptyProfile` before `runUp`. `tnet login` takes no
pref flags and re-authenticates the current profile. Should `login`
grow the full shared surface and the empty-profile switch, or stay the
auth-only verb it is documented to be? My answer: separate work, out
of scope here, and it should be one change rather than a flag at a
time — whether `login --hostname` means "set it on this profile" or
"set it on the fresh profile this login creates" depends on the
empty-profile question, so porting flags before settling that would
bake in an answer nobody chose.
|
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 (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesLogin flag compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The new login flags, validation, and nickname update behavior are covered without an actionable merge-blocking issue. 🚥 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 |
cmd/tailscale/cli/up.gobuilds ONE flag set for two commands:newUpFlagSet(goos, upArgs, cmd)registers--auth-keyand the hiddennotFalseVar--host-routesunconditionally, and--nicknameinsideif cmd == "login".login.gohandsnewUpFlagSet(..., "login")to thelogincommand and itsExeccalls the samerunUp, so upstreamtailscale logintakes all three.tnet logintook none of them. It carried only--authkey,--authkey-fileand--login-server, so a command line copied fromupstream —
tnet login --auth-key=tskey-…,tnet login --host-routes,tnet login --nickname=work— died at the parser with exit 2 before itreached the daemon. The
upside had already gained the same spellings,which is what made the asymmetry easy to miss: the flags were "ported",
just onto one of the two commands that share them.
Read against cmd/tailscale/cli/up.go and cmd/tailscale/cli/login.go at
53a0d659afa51835dd7a9283873cca44261454f8 (v1.102.3): up.go:90-140 for the
shared flag set, cli.go:65-90 for
CleanUpArgs, up.go:924 andipn/ipnlocal/profiles.go for what
--nicknamedoes.What changed
logintakes--auth-keyas a visible alias of--authkey, exactlyas
updoes. UpstreamCleanUpArgsrewrites--authkeyto--auth-keyfor every subcommand, so both spellings work on bothcommands there. The
file:<path>value form already reachedlogin(it is resolved inside the shared
resolve_authkey) and is unchanged.logintakes the hidden--host-routeswith Go'snotFalseVarshape, and refuses every value but
truewith the messageupalready used. The check moved into
check_host_routes, shared by bothcommands the way the upstream registration is, and on
loginit runsbefore every other check — Go decides it in the flag parser, before
Execruns.logintakes--nicknameand applies it, rather than accepting itinertly. It is sent as a one-pref
setrequest just before the loginround trip. That shape, rather than a new field on the login's
uprequest, because the daemon's
setpath is the only door to bothhalves Go's
profileManager.SetPrefsdoes — persistProfileNameandrename the current login profile, so
tnet switch <NAME>resolves it— and because the login's
uprequest has to keep mentioning no pref:that is what exempts
loginfrom the accidental-revert guard. It isordered after the auth key resolves (an unreadable
file:fails withnothing renamed) and before the re-auth, matching upstream, where the
name is in the prefs handed to
Startand so outlives an interactiveauth the operator never completes.
up --nicknameis still refused — upstream'suphas no such flageither — but the refusal now names
tnet login --nicknamenext totnet set --nicknameinstead of recording thatlogin's is missing.login --nickname=(empty) clears, the same clear-by-empty-value formtnet set --nickname=takes and the one Go's ownfmtFlagValueArgrenders. It therefore inherits the already-recorded divergence in
rename_current_profile: Go'ssetProfilePrefsputs the account's loginname back where this fork stores an empty name. That is one finding's job
and not this one's; nothing here makes it worse.
Tests
tests/tnet_up_go_flag_spellings.rsgained thelogincounterpartsof its three
uptests, driving the builttnetbinary: every flagthe shared set gives
logingets past the parser,login --host-routes=falsecarries Go's refusal, and--nicknameis adeclared
loginflag while--auth-keystays an alias rather thanbecoming a second flag. With
src/bin/tnet.rsstashed back to theparent commit, all three fail and the other three still pass:
unexpected argument '--auth-key' found(exit 2),unexpected argument '--host-routes' foundwhere the refusal should be, andlogin --helplisting no--nickname.src/bin/tnet.rsunit tests:login_takes_the_flags_gos_shared_flag_ set_gives_it,login_host_routes_accepts_only_the_value_go_allows,and
login_nickname_names_the_profile_and_mentions_no_other_pref,which calls
login_nickname_requestand asserts the built requestnames the nickname and leaves all sixteen other prefs at the
unchanged sentinel.
Verified with, from the repo root:
cargo fmt --all --check,cargo clippy --all-targets -- -D warnings,cargo test --all-targets,cargo build --release --bins— all four clean.Read first:
run_loginandlogin_nickname_requestinsrc/bin/tnet.rs,then the three new tests in
tests/tnet_up_go_flag_spellings.rs.Stale elsewhere: any parity note that says
login --nicknameis notimplemented here, or that Go's
--auth-key/--host-routesspellings arecarried on
upalone, no longer holds. The parity ledger is not editedhere on purpose.
DECISIONS
The profile name rides the existing
setrequest instead of a newfield on the
upwire request. Reversible either way; this one waschosen because a pref on the login's
uprequest would make thatrequest "mention a pref" and expose
loginto the accidental-revertguard, and because
node_nicknameis deliberately not an up-managedpref (it is excluded from
--reset, since Go registers--resetonuponly, where--nicknamedoes not exist).The rename step prints nothing of its own. Upstream prints nothing for
it either, and a "preferences updated" line ahead of the login's
ok:would only read as a second command having run.
QUESTIONS
loginshares the WHOLE pref surface, not just these threeflags:
--hostname,--advertise-routes,--ssh,--accept-dns,--operatorand the rest are all onnewUpFlagSet, andlogin.gocalls
SwitchToEmptyProfilebeforerunUp.tnet logintakes nopref flags and re-authenticates the current profile. Should
logingrow the full shared surface and the empty-profile switch, or stay the
auth-only verb it is documented to be? My answer: separate work, out
of scope here, and it should be one change rather than a flag at a
time — whether
login --hostnamemeans "set it on this profile" or"set it on the fresh profile this login creates" depends on the
empty-profile question, so porting flags before settling that would
bake in an answer nobody chose.
bead
tsd-aud313· sessiontsd-aud313-c582-r1-s1n0· baserefs/heads/mainSummary by CodeRabbit
New Features
--nicknamesupport totnet login, including the--auth-keyspelling alias.--host-routessupport totnet login, matching the existingtnet upbehavior.Bug Fixes
--host-routesvalues.