cmd/tailscale/cli: don't let up change prefs based on implicit flag values

This changes the behavior of "tailscale up".

Previously "tailscale up" always did a new Start and reset all the settings.

Now "tailscale up" with no flags just brings the world [back] up.
(The opposite of "tailscale down").

But with flags, "tailscale up" now only is allowed to change
preferences if they're explicitly named in the flags. Otherwise it's
an error. Or you need to use --reset to explicitly nuke everything.

RELNOTE=tailscale up change

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -0,0 +1,117 @@
+// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cli
+
+import (
+	"flag"
+	"testing"
+
+	"tailscale.com/ipn"
+)
+
+// Test that checkForAccidentalSettingReverts's updateMaskedPrefsFromUpFlag can handle
+// all flags. This will panic if a new flag creeps in that's unhandled.
+func TestUpdateMaskedPrefsFromUpFlag(t *testing.T) {
+	mp := new(ipn.MaskedPrefs)
+	upFlagSet.VisitAll(func(f *flag.Flag) {
+		updateMaskedPrefsFromUpFlag(mp, f.Name)
+	})
+}
+
+func TestCheckForAccidentalSettingReverts(t *testing.T) {
+	f := func(flags ...string) map[string]bool {
+		m := make(map[string]bool)
+		for _, f := range flags {
+			m[f] = true
+		}
+		return m
+	}
+	tests := []struct {
+		name     string
+		flagSet  map[string]bool
+		curPrefs *ipn.Prefs
+		mp       *ipn.MaskedPrefs
+		want     string
+	}{
+		{
+			name:    "bare_up_means_up",
+			flagSet: f(),
+			curPrefs: &ipn.Prefs{
+				WantRunning: false,
+				Hostname:    "foo",
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					WantRunning: true,
+				},
+				WantRunningSet: true,
+			},
+			want: "",
+		},
+		{
+			name:    "losing_hostname",
+			flagSet: f("accept-dns"),
+			curPrefs: &ipn.Prefs{
+				WantRunning: false,
+				Hostname:    "foo",
+				CorpDNS:     true,
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					WantRunning: true,
+					CorpDNS:     true,
+				},
+				WantRunningSet: true,
+				CorpDNSSet:     true,
+			},
+			want: `'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --hostname is not specified but its default value of "" differs from current value "foo"`,
+		},
+		{
+			name:    "hostname_changing_explicitly",
+			flagSet: f("hostname"),
+			curPrefs: &ipn.Prefs{
+				WantRunning: false,
+				Hostname:    "foo",
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					WantRunning: true,
+					Hostname:    "bar",
+				},
+				WantRunningSet: true,
+				HostnameSet:    true,
+			},
+			want: "",
+		},
+		{
+			name:    "hostname_changing_empty_explicitly",
+			flagSet: f("hostname"),
+			curPrefs: &ipn.Prefs{
+				WantRunning: false,
+				Hostname:    "foo",
+			},
+			mp: &ipn.MaskedPrefs{
+				Prefs: ipn.Prefs{
+					WantRunning: true,
+					Hostname:    "",
+				},
+				WantRunningSet: true,
+				HostnameSet:    true,
+			},
+			want: "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var got string
+			if err := checkForAccidentalSettingReverts(tt.flagSet, tt.curPrefs, tt.mp); err != nil {
+				got = err.Error()
+			}
+			if got != tt.want {
+				t.Errorf("unexpected result\n got: %s\nwant: %s\n", got, tt.want)
+			}
+		})
+	}
+}

cmd/tailscale/cli/up.go

@@ -9,13 +9,15 @@
 	"errors"
 	"flag"
 	"fmt"
-	"log"
 	"os"
+	"reflect"
 	"runtime"
 	"sort"
+	"strconv"
 	"strings"
 	"sync"
 
+	"github.com/go-multierror/multierror"
 	"github.com/peterbourgon/ff/v2/ffcli"
 	"inet.af/netaddr"
 	"tailscale.com/client/tailscale"
@@ -34,19 +36,32 @@
 "tailscale up" connects this machine to your Tailscale network,
 triggering authentication if necessary.
 
-The flags passed to this command are specific to this machine. If you don't
+With no flags, "tailscale up" brings the network online without
-specify any flags, options are reset to their default.
+changing any settings. (That is, it's the opposite of "tailscale
+down").
+
+If flags are specified, the flags must be the complete set of desired
+settings. An error is returned if any setting would be changed as a
+result of an unspecified flag's default value, unless the --reset
+flag is also used.
 `),
-	FlagSet: (func() *flag.FlagSet {
+	FlagSet: upFlagSet,
+	Exec:    runUp,
+}
+
+var upFlagSet = (func() *flag.FlagSet {
 	upf := flag.NewFlagSet("up", flag.ExitOnError)
+
+	upf.BoolVar(&upArgs.forceReauth, "force-reauth", false, "force reauthentication")
+	upf.BoolVar(&upArgs.reset, "reset", false, "reset unspecified settings to their default values")
+
 	upf.StringVar(&upArgs.server, "login-server", "https://login.tailscale.com", "base URL of control server")
 	upf.BoolVar(&upArgs.acceptRoutes, "accept-routes", false, "accept routes advertised by other Tailscale nodes")
 	upf.BoolVar(&upArgs.acceptDNS, "accept-dns", true, "accept DNS configuration from the admin panel")
 	upf.BoolVar(&upArgs.singleRoutes, "host-routes", true, "install host routes to other Tailscale nodes")
 	upf.StringVar(&upArgs.exitNodeIP, "exit-node", "", "Tailscale IP of the exit node for internet traffic")
 	upf.BoolVar(&upArgs.shieldsUp, "shields-up", false, "don't allow incoming connections")
-		upf.BoolVar(&upArgs.forceReauth, "force-reauth", false, "force reauthentication")
+	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.StringVar(&upArgs.advertiseTags, "advertise-tags", "", "ACL tags to request (comma-separated, e.g. \"tag:eng,tag:montreal,tag:ssh\")")
 	upf.StringVar(&upArgs.authKey, "authkey", "", "node authorization key")
 	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\")")
@@ -56,9 +71,7 @@
 		upf.StringVar(&upArgs.netfilterMode, "netfilter-mode", defaultNetfilterMode(), "netfilter mode (one of on, nodivert, off)")
 	}
 	return upf
-	})(),
+})()
-	Exec: runUp,
-}
 
 func defaultNetfilterMode() string {
 	if distro.Get() == distro.Synology {
@@ -68,6 +81,7 @@ func defaultNetfilterMode() string {
 }
 
 var upArgs struct {
+	reset                 bool
 	server                string
 	acceptRoutes          bool
 	acceptDNS             bool
@@ -95,7 +109,12 @@ func warnf(format string, args ...interface{}) {
 
 func runUp(ctx context.Context, args []string) error {
 	if len(args) > 0 {
-		log.Fatalf("too many non-flag arguments: %q", args)
+		fatalf("too many non-flag arguments: %q", args)
+	}
+
+	st, err := tailscale.Status(ctx)
+	if err != nil {
+		fatalf("can't fetch status from tailscaled: %v", err)
 	}
 
 	if distro.Get() == distro.Synology {
@@ -165,6 +184,14 @@ func runUp(ctx context.Context, args []string) error {
 		}
 	}
 
+	if !exitNodeIP.IsZero() {
+		for _, ip := range st.TailscaleIPs {
+			if exitNodeIP == ip {
+				fatalf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", exitNodeIP)
+			}
+		}
+	}
+
 	var tags []string
 	if upArgs.advertiseTags != "" {
 		tags = strings.Split(upArgs.advertiseTags, ",")
@@ -209,27 +236,70 @@ func runUp(ctx context.Context, args []string) error {
 		}
 	}
 
-	c, bc, ctx, cancel := connect(ctx)
+	curPrefs, err := tailscale.GetPrefs(ctx)
+	if err != nil {
+		return err
+	}
+
+	flagSet := map[string]bool{}
+	mp := new(ipn.MaskedPrefs)
+	mp.WantRunningSet = true
+	mp.Prefs = *prefs
+	upFlagSet.Visit(func(f *flag.Flag) {
+		updateMaskedPrefsFromUpFlag(mp, f.Name)
+		flagSet[f.Name] = true
+	})
+
+	if !upArgs.reset {
+		if err := checkForAccidentalSettingReverts(flagSet, curPrefs, mp); err != nil {
+			fatalf("%s", err)
+		}
+	}
+
+	controlURLChanged := curPrefs.ControlURL != prefs.ControlURL
+	if controlURLChanged && st.BackendState == ipn.Running.String() && !upArgs.forceReauth {
+		fatalf("can't change --login-server without --force-reauth")
+	}
+
+	// If we're already running and none of the flags require a
+	// restart, we can just do an EditPrefs call and change the
+	// prefs at runtime (e.g. changing hostname, changinged
+	// advertised tags, routes, etc)
+	justEdit := st.BackendState == ipn.Running.String() &&
+		!upArgs.forceReauth &&
+		!upArgs.reset &&
+		upArgs.authKey == "" &&
+		!controlURLChanged
+	if justEdit {
+		_, err := tailscale.EditPrefs(ctx, mp)
+		return err
+	}
+
+	// simpleUp is whether we're running a simple "tailscale up"
+	// to transition to running from a previously-logged-in but
+	// down state, without changing any settings.
+	simpleUp := len(flagSet) == 0 && curPrefs.Persist != nil && curPrefs.Persist.LoginName != ""
+
+	// At this point we need to subscribe to the IPN bus to watch
+	// for state transitions and possible need to authenticate.
+	c, bc, pumpCtx, cancel := connect(ctx)
 	defer cancel()
 
-	if !prefs.ExitNodeIP.IsZero() {
+	startingOrRunning := make(chan bool, 1) // gets value once starting or running
-		st, err := tailscale.Status(ctx)
+	gotEngineUpdate := make(chan bool, 1)   // gets value upon an engine update
-		if err != nil {
+	go pump(pumpCtx, bc, c)
-			fatalf("can't fetch status from tailscaled: %v", err)
-		}
-		for _, ip := range st.TailscaleIPs {
-			if prefs.ExitNodeIP == ip {
-				fatalf("cannot use %s as the exit node as it is a local IP address to this machine, did you mean --advertise-exit-node?", ip)
-			}
-		}
-	}
 
-	var printed bool
+	printed := !simpleUp
 	var loginOnce sync.Once
 	startLoginInteractive := func() { loginOnce.Do(func() { bc.StartLoginInteractive() }) }
 
-	bc.SetPrefs(prefs)
 	bc.SetNotifyCallback(func(n ipn.Notify) {
+		if n.Engine != nil {
+			select {
+			case gotEngineUpdate <- true:
+			default:
+			}
+		}
 		if n.ErrMessage != nil {
 			msg := *n.ErrMessage
 			if msg == ipn.ErrMsgPermissionDenied {
@@ -256,19 +326,44 @@ func runUp(ctx context.Context, args []string) error {
 					// Only need to print an update if we printed the "please click" message earlier.
 					fmt.Fprintf(os.Stderr, "Success.\n")
 				}
+				select {
+				case startingOrRunning <- true:
+				default:
+				}
 				cancel()
 			}
 		}
 		if url := n.BrowseToURL; url != nil {
+			printed = true
 			fmt.Fprintf(os.Stderr, "\nTo authenticate, visit:\n\n\t%s\n\n", *url)
 		}
 	})
+	// Wait for backend client to be connected so we know
+	// we're subscribed to updates. Otherwise we can miss
+	// an update upon its transition to running. Do so by causing some traffic
+	// back to the bus that we then wait on.
+	bc.RequestEngineStatus()
+	<-gotEngineUpdate
+
+	// Special case: bare "tailscale up" means to just start
+	// running, if there's ever been a login.
+	if simpleUp {
+		_, err := tailscale.EditPrefs(ctx, &ipn.MaskedPrefs{
+			Prefs: ipn.Prefs{
+				WantRunning: true,
+			},
+			WantRunningSet: true,
+		})
+		if err != nil {
+			return err
+		}
+	} else {
+		bc.SetPrefs(prefs)
 
 		opts := ipn.Options{
 			StateKey: ipn.GlobalDaemonStateKey,
 			AuthKey:  upArgs.authKey,
 		}
-
 		// On Windows, we still run in mostly the "legacy" way that
 		// predated the server's StateStore. That is, we send an empty
 		// StateKey and send the prefs directly. Although the Windows
@@ -284,18 +379,135 @@ func runUp(ctx context.Context, args []string) error {
 			opts.Prefs = prefs
 		}
 
-	// We still have to Start for now. This causes a bunch of churn
-	// every time the CLI touches anything.
-	//
-	// TODO(danderson): redo the frontend/backend API to assume
-	// ephemeral frontends that read/modify/write state, once
-	// Windows/Mac state is moved into backend.
 		bc.Start(opts)
-	if upArgs.forceReauth {
-		printed = true
 		startLoginInteractive()
 	}
-	pump(ctx, bc, c)
 
+	select {
+	case <-startingOrRunning:
 		return nil
+	case <-ctx.Done():
+		select {
+		case <-startingOrRunning:
+			return nil
+		default:
+		}
+		return ctx.Err()
+	}
+}
+
+var (
+	flagForPref = map[string]string{} // "ExitNodeIP" => "exit-node"
+	prefsOfFlag = map[string][]string{}
+)
+
+func init() {
+	addPrefFlagMapping("accept-dns", "CorpDNS")
+	addPrefFlagMapping("accept-routes", "RouteAll")
+	addPrefFlagMapping("advertise-routes", "AdvertiseRoutes")
+	addPrefFlagMapping("advertise-tags", "AdvertiseTags")
+	addPrefFlagMapping("host-routes", "AllowSingleHosts")
+	addPrefFlagMapping("hostname", "Hostname")
+	addPrefFlagMapping("login-server", "ControlURL")
+	addPrefFlagMapping("netfilter-mode", "NetfilterMode")
+	addPrefFlagMapping("shields-up", "ShieldsUp")
+	addPrefFlagMapping("snat-subnet-routes", "NoSNAT")
+	addPrefFlagMapping("exit-node", "ExitNodeIP", "ExitNodeIP")
+}
+
+func addPrefFlagMapping(flagName string, prefNames ...string) {
+	prefsOfFlag[flagName] = prefNames
+	for _, pref := range prefNames {
+		flagForPref[pref] = flagName
+	}
+}
+
+func updateMaskedPrefsFromUpFlag(mp *ipn.MaskedPrefs, flagName string) {
+	if prefs, ok := prefsOfFlag[flagName]; ok {
+		for _, pref := range prefs {
+			reflect.ValueOf(mp).Elem().FieldByName(pref + "Set").SetBool(true)
+		}
+		return
+	}
+	switch flagName {
+	case "authkey", "force-reauth", "reset":
+		// Not pref-related flags.
+	case "advertise-exit-node":
+		// This pref is a shorthand for advertise-routes.
+	default:
+		panic("internal error: unhandled flag " + flagName)
+	}
+}
+
+// checkForAccidentalSettingReverts checks for people running
+// "tailscale up" with a subset of the flags they originally ran it
+// with.
+//
+// For example, in Tailscale 1.6 and prior, a user might've advertised
+// a tag, but later tried to change just one other setting and forgot
+// to mention the tag later and silently wiped it out. We now
+// require --reset to change preferences to flag default values when
+// the flag is not mentioned on the command line.
+//
+// curPrefs is what's currently active on the server.
+//
+// mp is the mask of settings actually set, where mp.Prefs is the new
+// preferences to set, including any values set from implicit flags.
+func checkForAccidentalSettingReverts(flagSet map[string]bool, curPrefs *ipn.Prefs, mp *ipn.MaskedPrefs) error {
+	if len(flagSet) == 0 {
+		// A bare "tailscale up" is a special case to just
+		// mean bringing the network up without any changes.
+		return nil
+	}
+	curWithExplicitEdits := curPrefs.Clone()
+	curWithExplicitEdits.ApplyEdits(mp)
+
+	prefType := reflect.TypeOf(ipn.Prefs{})
+
+	// Explicit values (current + explicit edit):
+	ev := reflect.ValueOf(curWithExplicitEdits).Elem()
+	// Implicit values (what we'd get if we replaced everything with flag defaults):
+	iv := reflect.ValueOf(&mp.Prefs).Elem()
+	var errs []error
+	var didExitNodeErr bool
+	for i := 0; i < prefType.NumField(); i++ {
+		prefName := prefType.Field(i).Name
+		if prefName == "Persist" {
+			continue
+		}
+		flagName, hasFlag := flagForPref[prefName]
+		if hasFlag && flagSet[flagName] {
+			continue
+		}
+		// Get explicit value and implicit value
+		evi, ivi := ev.Field(i).Interface(), iv.Field(i).Interface()
+		if reflect.DeepEqual(evi, ivi) {
+			continue
+		}
+		switch flagName {
+		case "":
+			errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; this command would change the value of flagless pref %q", prefName))
+		case "exit-node":
+			if !didExitNodeErr {
+				didExitNodeErr = true
+				errs = append(errs, errors.New("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --exit-node is not specified but an exit node is currently configured"))
+			}
+		default:
+			errs = append(errs, fmt.Errorf("'tailscale up' without --reset requires all preferences with changing values to be explicitly mentioned; --%s is not specified but its default value of %v differs from current value %v",
+				flagName, fmtSettingVal(ivi), fmtSettingVal(evi)))
+		}
+	}
+	return multierror.New(errs)
+}
+
+func fmtSettingVal(v interface{}) string {
+	switch v := v.(type) {
+	case bool:
+		return strconv.FormatBool(v)
+	case string, preftype.NetfilterMode:
+		return fmt.Sprintf("%q", v)
+	case []string:
+		return strings.Join(v, ",")
+	}
+	return fmt.Sprint(v)
 }

cmd/tailscale/depaware.txt

@@ -2,6 +2,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
 
    W 💣 github.com/alexbrainman/sspi                                 from github.com/alexbrainman/sspi/negotiate
    W 💣 github.com/alexbrainman/sspi/negotiate                       from tailscale.com/net/tshttpproxy
+        github.com/go-multierror/multierror                          from tailscale.com/cmd/tailscale/cli
         github.com/peterbourgon/ff/v2                                from github.com/peterbourgon/ff/v2/ffcli
         github.com/peterbourgon/ff/v2/ffcli                          from tailscale.com/cmd/tailscale/cli
         github.com/tcnksm/go-httpstat                                from tailscale.com/net/netcheck

ipn/ipnlocal/local.go

@@ -1331,8 +1331,14 @@ func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) (*ipn.Prefs, error) {
 		b.mu.Unlock()
 		return p1, nil
 	}
+	cc := b.cc
 	b.logf("EditPrefs: %v", mp.Pretty())
-	b.setPrefsLockedOnEntry("EditPrefs", p1)
+	b.setPrefsLockedOnEntry("EditPrefs", p1) // does a b.mu.Unlock
+
+	if !p0.WantRunning && p1.WantRunning {
+		b.logf("EditPrefs: transitioning to running; doing Login...")
+		cc.Login(nil, controlclient.LoginDefault)
+	}
 	return p1, nil
 }