diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 2d02b6b7a..49d8e9c4a 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -657,6 +657,13 @@ func upArgsFromOSArgs(goos string, flagArgs ...string) (args upArgsT) { return } +func newSingleUseStringForTest(v string) singleUseStringFlag { + return singleUseStringFlag{ + set: true, + value: v, + } +} + func TestPrefsFromUpArgs(t *testing.T) { tests := []struct { name string @@ -721,14 +728,14 @@ func TestPrefsFromUpArgs(t *testing.T) { { name: "error_advertise_route_invalid_ip", args: upArgsT{ - advertiseRoutes: "foo", + advertiseRoutes: newSingleUseStringForTest("foo"), }, wantErr: `"foo" is not a valid IP address or CIDR prefix`, }, { name: "error_advertise_route_unmasked_bits", args: upArgsT{ - advertiseRoutes: "1.2.3.4/16", + advertiseRoutes: newSingleUseStringForTest("1.2.3.4/16"), }, wantErr: `1.2.3.4/16 has non-address bits set; expected 1.2.0.0/16`, }, @@ -749,7 +756,7 @@ func TestPrefsFromUpArgs(t *testing.T) { { name: "error_tag_prefix", args: upArgsT{ - advertiseTags: "foo", + advertiseTags: newSingleUseStringForTest("foo"), }, wantErr: `tag: "foo": tags must start with 'tag:'`, }, @@ -829,7 +836,7 @@ func TestPrefsFromUpArgs(t *testing.T) { name: "via_route_good", goos: "linux", args: upArgsT{ - advertiseRoutes: "fd7a:115c:a1e0:b1a::bb:10.0.0.0/112", + advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::bb:10.0.0.0/112"), netfilterMode: "off", }, want: &ipn.Prefs{ @@ -848,7 +855,7 @@ func TestPrefsFromUpArgs(t *testing.T) { name: "via_route_good_16_bit", goos: "linux", args: upArgsT{ - advertiseRoutes: "fd7a:115c:a1e0:b1a::aabb:10.0.0.0/112", + advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::aabb:10.0.0.0/112"), netfilterMode: "off", }, want: &ipn.Prefs{ @@ -867,7 +874,7 @@ func TestPrefsFromUpArgs(t *testing.T) { name: "via_route_short_prefix", goos: "linux", args: upArgsT{ - advertiseRoutes: "fd7a:115c:a1e0:b1a::/64", + advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a::/64"), netfilterMode: "off", }, wantErr: "fd7a:115c:a1e0:b1a::/64 4-in-6 prefix must be at least a /96", @@ -876,7 +883,7 @@ func TestPrefsFromUpArgs(t *testing.T) { name: "via_route_short_reserved_siteid", goos: "linux", args: upArgsT{ - advertiseRoutes: "fd7a:115c:a1e0:b1a:1234:5678::/112", + advertiseRoutes: newSingleUseStringForTest("fd7a:115c:a1e0:b1a:1234:5678::/112"), netfilterMode: "off", }, wantErr: "route fd7a:115c:a1e0:b1a:1234:5678::/112 contains invalid site ID 12345678; must be 0xffff or less", @@ -1106,6 +1113,7 @@ func TestUpdatePrefs(t *testing.T) { }, env: upCheckEnv{backendState: "Running"}, }, + { // Issue 3808: explicitly empty --operator= should clear value. name: "explicit_empty_operator", @@ -1598,3 +1606,56 @@ func TestDepsNoCapture(t *testing.T) { }.Check(t) } + +func TestSingleUseStringFlag(t *testing.T) { + tests := []struct { + name string + setValues []string + wantValue string + wantErr bool + }{ + { + name: "set once", + setValues: []string{"foo"}, + wantValue: "foo", + wantErr: false, + }, + { + name: "set twice", + setValues: []string{"foo", "bar"}, + wantValue: "foo", + wantErr: true, + }, + { + name: "set nothing", + setValues: []string{}, + wantValue: "", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var flag singleUseStringFlag + var lastErr error + + for _, val := range tt.setValues { + lastErr = flag.Set(val) + } + + if tt.wantErr { + if lastErr == nil { + t.Errorf("expected error on final Set, got nil") + } + } else { + if lastErr != nil { + t.Errorf("unexpected error on final Set: %v", lastErr) + } + } + + if got := flag.String(); got != tt.wantValue { + t.Errorf("String() = %q, want %q", got, tt.wantValue) + } + }) + } +} diff --git a/cmd/tailscale/cli/set.go b/cmd/tailscale/cli/set.go index 07b3fe9ce..ab113f6e0 100644 --- a/cmd/tailscale/cli/set.go +++ b/cmd/tailscale/cli/set.go @@ -49,7 +49,7 @@ type setArgsT struct { runSSH bool runWebClient bool hostname string - advertiseRoutes string + advertiseRoutes singleUseStringFlag advertiseDefaultRoute bool advertiseConnector bool opUser string @@ -75,7 +75,7 @@ func newSetFlagSet(goos string, setArgs *setArgsT) *flag.FlagSet { setf.BoolVar(&setArgs.shieldsUp, "shields-up", false, "don't allow incoming connections") setf.BoolVar(&setArgs.runSSH, "ssh", false, "run an SSH server, permitting access per tailnet admin's declared policy") setf.StringVar(&setArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS") - setf.StringVar(&setArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes") + setf.Var(&setArgs.advertiseRoutes, "advertise-routes", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes") setf.BoolVar(&setArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet") setf.BoolVar(&setArgs.advertiseConnector, "advertise-connector", false, "offer to be an app connector for domain specific internet traffic for the tailnet") setf.BoolVar(&setArgs.updateCheck, "update-check", true, "notify about available Tailscale updates") @@ -259,11 +259,11 @@ func runSet(ctx context.Context, args []string) (retErr error) { // setArgs is the parsed command-line arguments. func calcAdvertiseRoutesForSet(advertiseExitNodeSet, advertiseRoutesSet bool, curPrefs *ipn.Prefs, setArgs setArgsT) (routes []netip.Prefix, err error) { if advertiseExitNodeSet && advertiseRoutesSet { - return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes, setArgs.advertiseDefaultRoute) + return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes.String(), setArgs.advertiseDefaultRoute) } if advertiseRoutesSet { - return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes, curPrefs.AdvertisesExitNode()) + return netutil.CalcAdvertiseRoutes(setArgs.advertiseRoutes.String(), curPrefs.AdvertisesExitNode()) } if advertiseExitNodeSet { alreadyAdvertisesExitNode := curPrefs.AdvertisesExitNode() diff --git a/cmd/tailscale/cli/set_test.go b/cmd/tailscale/cli/set_test.go index a2f211f8c..8ef85be12 100644 --- a/cmd/tailscale/cli/set_test.go +++ b/cmd/tailscale/cli/set_test.go @@ -116,7 +116,7 @@ func TestCalcAdvertiseRoutesForSet(t *testing.T) { sa.advertiseDefaultRoute = *tc.setExit } if tc.setRoutes != nil { - sa.advertiseRoutes = *tc.setRoutes + sa.advertiseRoutes = newSingleUseStringForTest(*tc.setRoutes) } got, err := calcAdvertiseRoutesForSet(tc.setExit != nil, tc.setRoutes != nil, curPrefs, sa) if err != nil { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 26db85f13..22276cd99 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -82,6 +82,25 @@ func acceptRouteDefault(goos string) bool { return p.DefaultRouteAll(goos) } +// singleUseStringFlag will throw an error if the flag is specified more than once. +type singleUseStringFlag struct { + set bool + value string +} + +func (s singleUseStringFlag) String() string { + return s.value +} + +func (s *singleUseStringFlag) Set(v string) error { + if s.set { + return errors.New("flag can only be specified once") + } + s.set = true + s.value = v + return nil +} + var upFlagSet = newUpFlagSet(effectiveGOOS(), &upArgsGlobal, "up") // newUpFlagSet returns a new flag set for the "up" and "login" commands. @@ -104,9 +123,9 @@ func newUpFlagSet(goos string, upArgs *upArgsT, cmd string) *flag.FlagSet { upf.BoolVar(&upArgs.exitNodeAllowLANAccess, "exit-node-allow-lan-access", false, "Allow direct access to the local network when routing traffic via an exit node") upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections") upf.BoolVar(&upArgs.runSSH, "ssh", false, "run an SSH server, permitting access per tailnet admin's declared policy") - upf.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "comma-separated ACL tags to request; each must start with \"tag:\" (e.g. \"tag:eng,tag:montreal,tag:ssh\")") + upf.Var(&upArgs.advertiseTags, "advertise-tags", "comma-separated ACL tags to request; each must start with \"tag:\" (e.g. \"tag:eng,tag:montreal,tag:ssh\")") upf.StringVar(&upArgs.hostname, "hostname", "", "hostname to use instead of the one provided by the OS") - upf.StringVar(&upArgs.advertiseRoutes, "advertise-routes", "", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes") + upf.Var(&upArgs.advertiseRoutes, "advertise-routes", "routes to advertise to other nodes (comma-separated, e.g. \"10.0.0.0/8,192.168.0.0/24\") or empty string to not advertise routes") upf.BoolVar(&upArgs.advertiseConnector, "advertise-connector", false, "advertise this node as an app connector") upf.BoolVar(&upArgs.advertiseDefaultRoute, "advertise-exit-node", false, "offer to be an exit node for internet traffic for the tailnet") upf.BoolVar(&upArgs.postureChecking, "posture-checking", false, hidden+"allow management plane to gather device posture information") @@ -174,9 +193,9 @@ type upArgsT struct { runWebClient bool forceReauth bool forceDaemon bool - advertiseRoutes string + advertiseRoutes singleUseStringFlag advertiseDefaultRoute bool - advertiseTags string + advertiseTags singleUseStringFlag advertiseConnector bool snat bool statefulFiltering bool @@ -244,7 +263,7 @@ func warnf(format string, args ...any) { // function exists for testing and should have no side effects or // outside interactions (e.g. no making Tailscale LocalAPI calls). func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goos string) (*ipn.Prefs, error) { - routes, err := netutil.CalcAdvertiseRoutes(upArgs.advertiseRoutes, upArgs.advertiseDefaultRoute) + routes, err := netutil.CalcAdvertiseRoutes(upArgs.advertiseRoutes.String(), upArgs.advertiseDefaultRoute) if err != nil { return nil, err } @@ -254,8 +273,8 @@ func prefsFromUpArgs(upArgs upArgsT, warnf logger.Logf, st *ipnstate.Status, goo } var tags []string - if upArgs.advertiseTags != "" { - tags = strings.Split(upArgs.advertiseTags, ",") + if upArgs.advertiseTags.String() != "" { + tags = strings.Split(upArgs.advertiseTags.String(), ",") for _, tag := range tags { err := tailcfg.CheckTag(tag) if err != nil { @@ -555,7 +574,7 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE if err != nil { return err } - authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags) + authKey, err = resolveAuthKey(ctx, authKey, upArgs.advertiseTags.String()) if err != nil { return err }