From 53cfff109b01baa3d219697a1a9e2ea16c4b0d3d Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 31 Mar 2021 21:35:21 -0700 Subject: [PATCH] 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 --- cmd/tailscale/cli/down.go | 7 +- ipn/backend.go | 5 +- ipn/fake_test.go | 7 +- ipn/ipnlocal/local.go | 27 ++++--- ipn/message.go | 14 ++-- ipn/prefs.go | 68 ++++++++++++++++++ ipn/prefs_test.go | 145 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 250 insertions(+), 23 deletions(-) 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 @@ 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{}" + } + 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) + } + } +}