ipn: replace SetWantRunning(bool) with EditPrefs(MaskedPrefs)

This adds a new ipn.MaskedPrefs embedding a ipn.Prefs, along with a
bunch of "has bits", kept in sync with tests & reflect.

Then it adds a Prefs.ApplyEdits(MaskedPrefs) method.

Then the ipn.Backend interface loses its weirdo SetWantRunning(bool)
method (that I added in 483141094c for "tailscale down")
and replaces it with EditPrefs (alongside the existing SetPrefs for now).

Then updates 'tailscale down' to use EditPrefs instead of SetWantRunning.

In the future, we can use this to do more interesting things with the
CLI, reconfiguring only certain properties without the reset-the-world
"tailscale up".

Updates #1436

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

cmd/tailscale/cli/down.go

@@ -59,7 +59,12 @@ func runDown(ctx context.Context, args []string) error {
 		}
 	})
 
-	bc.SetWantRunning(false)
+	bc.EditPrefs(&ipn.MaskedPrefs{
+		Prefs: ipn.Prefs{
+			WantRunning: false,
+		},
+		WantRunningSet: true,
+	})
 	pump(ctx, bc, c)
 
 	return nil

ipn/backend.go

@@ -151,9 +151,8 @@ type Backend interface {
 	// WantRunning. This may cause the wireguard engine to
 	// reconfigure or stop.
 	SetPrefs(*Prefs)
-	// SetWantRunning is like SetPrefs but sets only the
+	// EditPrefs is like SetPrefs but only sets the specified fields.
-	// WantRunning field.
+	EditPrefs(*MaskedPrefs)
-	SetWantRunning(wantRunning bool)
 	// RequestEngineStatus polls for an update from the wireguard
 	// engine. Only needed if you want to display byte
 	// counts. Connection events are emitted automatically without

ipn/fake_test.go

@@ -79,8 +79,11 @@ func (b *FakeBackend) SetPrefs(new *Prefs) {
 	}
 }
 
-func (b *FakeBackend) SetWantRunning(v bool) {
+func (b *FakeBackend) EditPrefs(mp *MaskedPrefs) {
-	b.SetPrefs(&Prefs{WantRunning: v})
+	// This fake implementation only cares about this one pref.
+	if mp.WantRunningSet {
+		b.SetPrefs(&mp.Prefs)
+	}
 }
 
 func (b *FakeBackend) RequestEngineStatus() {

ipn/ipnlocal/local.go

@@ -1260,16 +1260,17 @@ func (b *LocalBackend) SetCurrentUserID(uid string) {
 	b.mu.Unlock()
 }
 
-func (b *LocalBackend) SetWantRunning(wantRunning bool) {
+func (b *LocalBackend) EditPrefs(mp *ipn.MaskedPrefs) {
 	b.mu.Lock()
-	new := b.prefs.Clone()
+	p0 := b.prefs.Clone()
-	b.mu.Unlock()
+	p1 := b.prefs.Clone()
-	if new.WantRunning == wantRunning {
+	p1.ApplyEdits(mp)
+	if p1.Equals(p0) {
+		b.mu.Unlock()
 		return
 	}
-	new.WantRunning = wantRunning
+	b.logf("EditPrefs: %v", mp.Pretty())
-	b.logf("SetWantRunning: %v", wantRunning)
+	b.setPrefsLockedOnEntry("EditPrefs", p1)
-	b.SetPrefs(new)
 }
 
 // SetPrefs saves new user preferences and propagates them throughout
@@ -1278,9 +1279,13 @@ func (b *LocalBackend) SetPrefs(newp *ipn.Prefs) {
 	if newp == nil {
 		panic("SetPrefs got nil prefs")
 	}
-
 	b.mu.Lock()
+	b.setPrefsLockedOnEntry("SetPrefs", newp)
+}
 
+// setPrefsLockedOnEntry requires b.mu be held to call it, but it
+// unlocks b.mu when done.
+func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) {
 	netMap := b.netMap
 	stateKey := b.stateKey
 
@@ -1303,13 +1308,15 @@ func (b *LocalBackend) SetPrefs(newp *ipn.Prefs) {
 
 	if stateKey != "" {
 		if err := b.store.WriteState(stateKey, newp.ToBytes()); err != nil {
-			b.logf("Failed to save new controlclient state: %v", err)
+			b.logf("failed to save new controlclient state: %v", err)
 		}
 	}
 	b.writeServerModeStartState(userID, newp)
 
 	// [GRINDER STATS LINE] - please don't remove (used for log parsing)
-	b.logf("SetPrefs: %v", newp.Pretty())
+	if caller == "SetPrefs" {
+		b.logf("SetPrefs: %v", newp.Pretty())
+	}
 	if netMap != nil {
 		if login := netMap.UserProfiles[netMap.User].LoginName; login != "" {
 			if newp.Persist == nil {

ipn/message.go

@@ -80,7 +80,7 @@ type Command struct {
 	Login                 *tailcfg.Oauth2Token
 	Logout                *NoArgs
 	SetPrefs              *SetPrefsArgs
-	SetWantRunning        *bool
+	EditPrefs             *MaskedPrefs
 	RequestEngineStatus   *NoArgs
 	RequestStatus         *NoArgs
 	FakeExpireAfter       *FakeExpireAfterArgs
@@ -204,8 +204,8 @@ func (bs *BackendServer) GotCommand(ctx context.Context, cmd *Command) error {
 	} else if c := cmd.SetPrefs; c != nil {
 		bs.b.SetPrefs(c.New)
 		return nil
-	} else if c := cmd.SetWantRunning; c != nil {
+	} else if c := cmd.EditPrefs; c != nil {
-		bs.b.SetWantRunning(*c)
+		bs.b.EditPrefs(c)
 		return nil
 	} else if c := cmd.FakeExpireAfter; c != nil {
 		bs.b.FakeExpireAfter(c.Duration)
@@ -309,6 +309,10 @@ func (bc *BackendClient) SetPrefs(new *Prefs) {
 	bc.send(Command{SetPrefs: &SetPrefsArgs{New: new}})
 }
 
+func (bc *BackendClient) EditPrefs(mp *MaskedPrefs) {
+	bc.send(Command{EditPrefs: mp})
+}
+
 func (bc *BackendClient) RequestEngineStatus() {
 	bc.send(Command{RequestEngineStatus: &NoArgs{}})
 }
@@ -328,10 +332,6 @@ func (bc *BackendClient) Ping(ip string, useTSMP bool) {
 	}})
 }
 
-func (bc *BackendClient) SetWantRunning(v bool) {
-	bc.send(Command{SetWantRunning: &v})
-}
-
 // MaxMessageSize is the maximum message size, in bytes.
 const MaxMessageSize = 10 << 20
 

ipn/prefs.go

@@ -12,6 +12,7 @@ import (
 	"log"
 	"os"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strings"
 
@@ -147,6 +148,73 @@ type Prefs struct {
 	Persist *persist.Persist `json:"Config"`
 }
 
+// MaskedPrefs is a Prefs with an associated bitmask of which fields are set.
+type MaskedPrefs struct {
+	Prefs
+
+	ControlURLSet       bool `json:",omitempty"`
+	RouteAllSet         bool `json:",omitempty"`
+	AllowSingleHostsSet bool `json:",omitempty"`
+	ExitNodeIDSet       bool `json:",omitempty"`
+	ExitNodeIPSet       bool `json:",omitempty"`
+	CorpDNSSet          bool `json:",omitempty"`
+	WantRunningSet      bool `json:",omitempty"`
+	ShieldsUpSet        bool `json:",omitempty"`
+	AdvertiseTagsSet    bool `json:",omitempty"`
+	HostnameSet         bool `json:",omitempty"`
+	OSVersionSet        bool `json:",omitempty"`
+	DeviceModelSet      bool `json:",omitempty"`
+	NotepadURLsSet      bool `json:",omitempty"`
+	ForceDaemonSet      bool `json:",omitempty"`
+	AdvertiseRoutesSet  bool `json:",omitempty"`
+	NoSNATSet           bool `json:",omitempty"`
+	NetfilterModeSet    bool `json:",omitempty"`
+}
+
+// ApplyEdits mutates p, assigning fields from m.Prefs for each MaskedPrefs
+// Set field that's true.
+func (p *Prefs) ApplyEdits(m *MaskedPrefs) {
+	if p == nil {
+		panic("can't edit nil Prefs")
+	}
+	pv := reflect.ValueOf(p).Elem()
+	mv := reflect.ValueOf(m).Elem()
+	mpv := reflect.ValueOf(&m.Prefs).Elem()
+	fields := mv.NumField()
+	for i := 1; i < fields; i++ {
+		if mv.Field(i).Bool() {
+			newFieldValue := mpv.Field(i - 1)
+			pv.Field(i - 1).Set(newFieldValue)
+		}
+	}
+}
+
+func (m *MaskedPrefs) Pretty() string {
+	if m == nil {
+		return "MaskedPrefs{<nil>}"
+	}
+	var sb strings.Builder
+	sb.WriteString("MaskedPrefs{")
+	mv := reflect.ValueOf(m).Elem()
+	mt := mv.Type()
+	mpv := reflect.ValueOf(&m.Prefs).Elem()
+	first := true
+	for i := 1; i < mt.NumField(); i++ {
+		name := mt.Field(i).Name
+		if mv.Field(i).Bool() {
+			if !first {
+				sb.WriteString(" ")
+			}
+			first = false
+			fmt.Fprintf(&sb, "%s=%#v",
+				strings.TrimSuffix(name, "Set"),
+				mpv.Field(i-1).Interface())
+		}
+	}
+	sb.WriteString("}")
+	return sb.String()
+}
+
 // IsEmpty reports whether p is nil or pointing to a Prefs zero value.
 func (p *Prefs) IsEmpty() bool { return p == nil || p.Equals(&Prefs{}) }
 

ipn/prefs_test.go

@@ -5,11 +5,13 @@
 package ipn
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -432,3 +434,146 @@ func TestLoadPrefsFileWithZeroInIt(t *testing.T) {
 	}
 	t.Fatalf("unexpected prefs=%#v, err=%v", p, err)
 }
+
+func TestMaskedPrefsFields(t *testing.T) {
+	have := map[string]bool{}
+	for _, f := range fieldsOf(reflect.TypeOf(Prefs{})) {
+		if f == "Persist" {
+			// This one can't be edited.
+			continue
+		}
+		have[f] = true
+	}
+	for _, f := range fieldsOf(reflect.TypeOf(MaskedPrefs{})) {
+		if f == "Prefs" {
+			continue
+		}
+		if !strings.HasSuffix(f, "Set") {
+			t.Errorf("unexpected non-/Set$/ field %q", f)
+			continue
+		}
+		bare := strings.TrimSuffix(f, "Set")
+		_, ok := have[bare]
+		if !ok {
+			t.Errorf("no corresponding Prefs.%s field for MaskedPrefs.%s", bare, f)
+			continue
+		}
+		delete(have, bare)
+	}
+	for f := range have {
+		t.Errorf("missing MaskedPrefs.%sSet for Prefs.%s", f, f)
+	}
+
+	// And also make sure they line up in the right order, which
+	// ApplyEdits assumes.
+	pt := reflect.TypeOf(Prefs{})
+	mt := reflect.TypeOf(MaskedPrefs{})
+	for i := 0; i < mt.NumField(); i++ {
+		name := mt.Field(i).Name
+		if i == 0 {
+			if name != "Prefs" {
+				t.Errorf("first field of MaskedPrefs should be Prefs")
+			}
+			continue
+		}
+		prefName := pt.Field(i - 1).Name
+		if prefName+"Set" != name {
+			t.Errorf("MaskedField[%d] = %s; want %sSet", i-1, name, prefName)
+		}
+	}
+}
+
+func TestPrefsApplyEdits(t *testing.T) {
+	tests := []struct {
+		name  string
+		prefs *Prefs
+		edit  *MaskedPrefs
+		want  *Prefs
+	}{
+		{
+			name: "no_change",
+			prefs: &Prefs{
+				Hostname: "foo",
+			},
+			edit: &MaskedPrefs{},
+			want: &Prefs{
+				Hostname: "foo",
+			},
+		},
+		{
+			name: "set1_decoy1",
+			prefs: &Prefs{
+				Hostname: "foo",
+			},
+			edit: &MaskedPrefs{
+				Prefs: Prefs{
+					Hostname:    "bar",
+					DeviceModel: "ignore-this", // not set
+				},
+				HostnameSet: true,
+			},
+			want: &Prefs{
+				Hostname: "bar",
+			},
+		},
+		{
+			name:  "set_several",
+			prefs: &Prefs{},
+			edit: &MaskedPrefs{
+				Prefs: Prefs{
+					Hostname:    "bar",
+					DeviceModel: "galaxybrain",
+				},
+				HostnameSet:    true,
+				DeviceModelSet: true,
+			},
+			want: &Prefs{
+				Hostname:    "bar",
+				DeviceModel: "galaxybrain",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.prefs.Clone()
+			got.ApplyEdits(tt.edit)
+			if !got.Equals(tt.want) {
+				gotj, _ := json.Marshal(got)
+				wantj, _ := json.Marshal(tt.want)
+				t.Errorf("fail.\n got: %s\nwant: %s\n", gotj, wantj)
+			}
+		})
+	}
+}
+
+func TestMaskedPrefsPretty(t *testing.T) {
+	tests := []struct {
+		m    *MaskedPrefs
+		want string
+	}{
+		{
+			m:    &MaskedPrefs{},
+			want: "MaskedPrefs{}",
+		},
+		{
+			m: &MaskedPrefs{
+				Prefs: Prefs{
+					Hostname:         "bar",
+					DeviceModel:      "galaxybrain",
+					AllowSingleHosts: true,
+					RouteAll:         false,
+				},
+				RouteAllSet:    true,
+				HostnameSet:    true,
+				DeviceModelSet: true,
+			},
+			want: `MaskedPrefs{RouteAll=false Hostname="bar" DeviceModel="galaxybrain"}`,
+		},
+	}
+	for i, tt := range tests {
+		got := tt.m.Pretty()
+		if got != tt.want {
+			t.Errorf("%d.\n got: %#q\nwant: %#q\n", i, got, tt.want)
+		}
+	}
+}