diff --git a/cmd/tailscale/cli/down.go b/cmd/tailscale/cli/down.go index dd1e8491f..90a0a8e71 100644 --- a/cmd/tailscale/cli/down.go +++ b/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 diff --git a/ipn/backend.go b/ipn/backend.go index 60c5d4395..e900ffe5e 100644 --- a/ipn/backend.go +++ b/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 - // WantRunning field. - SetWantRunning(wantRunning bool) + // EditPrefs is like SetPrefs but only sets the specified fields. + EditPrefs(*MaskedPrefs) // RequestEngineStatus polls for an update from the wireguard // engine. Only needed if you want to display byte // counts. Connection events are emitted automatically without diff --git a/ipn/fake_test.go b/ipn/fake_test.go index eef580f57..6e7e2809f 100644 --- a/ipn/fake_test.go +++ b/ipn/fake_test.go @@ -79,8 +79,11 @@ func (b *FakeBackend) SetPrefs(new *Prefs) { } } -func (b *FakeBackend) SetWantRunning(v bool) { - b.SetPrefs(&Prefs{WantRunning: v}) +func (b *FakeBackend) EditPrefs(mp *MaskedPrefs) { + // This fake implementation only cares about this one pref. + if mp.WantRunningSet { + b.SetPrefs(&mp.Prefs) + } } func (b *FakeBackend) RequestEngineStatus() { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 1dde07eaa..0ba0b067c 100644 --- a/ipn/ipnlocal/local.go +++ b/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() - b.mu.Unlock() - if new.WantRunning == wantRunning { + p0 := b.prefs.Clone() + p1 := b.prefs.Clone() + p1.ApplyEdits(mp) + if p1.Equals(p0) { + b.mu.Unlock() return } - new.WantRunning = wantRunning - b.logf("SetWantRunning: %v", wantRunning) - b.SetPrefs(new) + b.logf("EditPrefs: %v", mp.Pretty()) + b.setPrefsLockedOnEntry("EditPrefs", p1) } // 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 { diff --git a/ipn/message.go b/ipn/message.go index 905664c93..29c709c12 100644 --- a/ipn/message.go +++ b/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 { - bs.b.SetWantRunning(*c) + } else if c := cmd.EditPrefs; c != nil { + 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 diff --git a/ipn/prefs.go b/ipn/prefs.go index ced33f23f..69ba3bcab 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -12,6 +12,7 @@ "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{}" + } + 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{}) } diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index 1cb5d052f..211a8a7e7 100644 --- a/ipn/prefs_test.go +++ b/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) + } + } +}