From e25f114916b4d8ced60ae2fd77a5982a826f2e63 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Fri, 8 Dec 2023 12:19:25 -0600 Subject: [PATCH] ipn,cmd/tailscale/cli: support hierarchical MaskedPrefs (#10507) Some fields if `ipn.Prefs` are structs. `ipn.MaskedPrefs` has a single level of boolean `*Set` flags, which doesn't map well to nested structs within `ipn.Prefs`. Change `MaskedPrefs` and `ApplyEdits` to support `FooSet` struct fields that map to a nested struct of `ipn.Prefs` like `AutoUpdates`. Each struct field in `MaskedPrefs` is just a bundle of more `Set` bool fields or other structs. This allows you to have a `Set` flag for any arbitrarily-nested field of `ipn.Prefs`. Also, make `ApplyEdits` match fields between `Prefs` and `MaskedPrefs` by name instead of order, to make it a bit less finicky. It's probably slower but `ipn.ApplyEdits` should not be in any hot path. As a result, `AutoUpdate.Check` and `AutoUpdate.Apply` fields don't clobber each other when set individually. Updates #16247 Signed-off-by: Andrew Lytvynov --- cmd/tailscale/cli/cli_test.go | 2 +- cmd/tailscale/cli/set.go | 2 +- cmd/tailscale/cli/up.go | 21 +++-- ipn/conf.go | 2 +- ipn/prefs.go | 139 +++++++++++++++++++++++----------- ipn/prefs_test.go | 36 +++++++++ 6 files changed, 150 insertions(+), 52 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index c1fe9b70a..c2e092aa5 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/cmd/tailscale/cli/cli_test.go @@ -786,7 +786,7 @@ func TestPrefFlagMapping(t *testing.T) { prefHasFlag := map[string]bool{} for _, pv := range prefsOfFlag { for _, pref := range pv { - prefHasFlag[pref] = true + prefHasFlag[strings.Split(pref, ".")[0]] = true } } diff --git a/cmd/tailscale/cli/set.go b/cmd/tailscale/cli/set.go index b436b4a9a..ffa0d6e6f 100644 --- a/cmd/tailscale/cli/set.go +++ b/cmd/tailscale/cli/set.go @@ -167,7 +167,7 @@ func runSet(ctx context.Context, args []string) (retErr error) { return err } } - if maskedPrefs.AutoUpdateSet { + if maskedPrefs.AutoUpdateSet.ApplySet { // On macsys, tailscaled will set the Sparkle auto-update setting. It // does not use clientupdate. if version.IsMacSysExt() { diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index e418c0504..67d6a3483 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -718,8 +718,8 @@ func init() { addPrefFlagMapping("ssh", "RunSSH") addPrefFlagMapping("webclient", "RunWebClient") addPrefFlagMapping("nickname", "ProfileName") - addPrefFlagMapping("update-check", "AutoUpdate") - addPrefFlagMapping("auto-update", "AutoUpdate") + addPrefFlagMapping("update-check", "AutoUpdate.Check") + addPrefFlagMapping("auto-update", "AutoUpdate.Apply") addPrefFlagMapping("advertise-connector", "AppConnector") addPrefFlagMapping("posture-checking", "PostureChecking") } @@ -728,9 +728,14 @@ func addPrefFlagMapping(flagName string, prefNames ...string) { prefsOfFlag[flagName] = prefNames prefType := reflect.TypeOf(ipn.Prefs{}) for _, pref := range prefNames { - // Crash at runtime if there's a typo in the prefName. - if _, ok := prefType.FieldByName(pref); !ok { - panic(fmt.Sprintf("invalid ipn.Prefs field %q", pref)) + t := prefType + for _, name := range strings.Split(pref, ".") { + // Crash at runtime if there's a typo in the prefName. + f, ok := t.FieldByName(name) + if !ok { + panic(fmt.Sprintf("invalid ipn.Prefs field %q", pref)) + } + t = f.Type } } } @@ -751,7 +756,11 @@ func updateMaskedPrefsFromUpOrSetFlag(mp *ipn.MaskedPrefs, flagName string) { } if prefs, ok := prefsOfFlag[flagName]; ok { for _, pref := range prefs { - reflect.ValueOf(mp).Elem().FieldByName(pref + "Set").SetBool(true) + f := reflect.ValueOf(mp).Elem() + for _, name := range strings.Split(pref, ".") { + f = f.FieldByName(name + "Set") + } + f.SetBool(true) } return } diff --git a/ipn/conf.go b/ipn/conf.go index d55146d58..f795e5205 100644 --- a/ipn/conf.go +++ b/ipn/conf.go @@ -124,7 +124,7 @@ func (c *ConfigVAlpha) ToPrefs() (MaskedPrefs, error) { } if c.AutoUpdate != nil { mp.AutoUpdate = *c.AutoUpdate - mp.AutoUpdateSet = true + mp.AutoUpdateSet = AutoUpdatePrefsMask{ApplySet: true, CheckSet: true} } return mp, nil } diff --git a/ipn/prefs.go b/ipn/prefs.go index bb9b193fa..3cf0dd3f9 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -248,38 +248,55 @@ type AppConnectorPrefs struct { } // MaskedPrefs is a Prefs with an associated bitmask of which fields are set. -// Make sure that the bool you add here maintains the same ordering of fields -// as the Prefs struct, because the ApplyEdits() function below relies on this -// ordering to be the same. +// +// Each FooSet field maps to a corresponding Foo field in Prefs. FooSet can be +// a struct, in which case inner fields of FooSet map to inner fields of Foo in +// Prefs (see AutoUpdateSet for example). type MaskedPrefs struct { Prefs - ControlURLSet bool `json:",omitempty"` - RouteAllSet bool `json:",omitempty"` - AllowSingleHostsSet bool `json:",omitempty"` - ExitNodeIDSet bool `json:",omitempty"` - ExitNodeIPSet bool `json:",omitempty"` - ExitNodeAllowLANAccessSet bool `json:",omitempty"` - CorpDNSSet bool `json:",omitempty"` - RunSSHSet bool `json:",omitempty"` - RunWebClientSet bool `json:",omitempty"` - WantRunningSet bool `json:",omitempty"` - LoggedOutSet bool `json:",omitempty"` - ShieldsUpSet bool `json:",omitempty"` - AdvertiseTagsSet bool `json:",omitempty"` - HostnameSet bool `json:",omitempty"` - NotepadURLsSet bool `json:",omitempty"` - ForceDaemonSet bool `json:",omitempty"` - EggSet bool `json:",omitempty"` - AdvertiseRoutesSet bool `json:",omitempty"` - NoSNATSet bool `json:",omitempty"` - NetfilterModeSet bool `json:",omitempty"` - OperatorUserSet bool `json:",omitempty"` - ProfileNameSet bool `json:",omitempty"` - AutoUpdateSet bool `json:",omitempty"` - AppConnectorSet bool `json:",omitempty"` - PostureCheckingSet bool `json:",omitempty"` - NetfilterKindSet bool `json:",omitempty"` + ControlURLSet bool `json:",omitempty"` + RouteAllSet bool `json:",omitempty"` + AllowSingleHostsSet bool `json:",omitempty"` + ExitNodeIDSet bool `json:",omitempty"` + ExitNodeIPSet bool `json:",omitempty"` + ExitNodeAllowLANAccessSet bool `json:",omitempty"` + CorpDNSSet bool `json:",omitempty"` + RunSSHSet bool `json:",omitempty"` + RunWebClientSet bool `json:",omitempty"` + WantRunningSet bool `json:",omitempty"` + LoggedOutSet bool `json:",omitempty"` + ShieldsUpSet bool `json:",omitempty"` + AdvertiseTagsSet bool `json:",omitempty"` + HostnameSet bool `json:",omitempty"` + NotepadURLsSet bool `json:",omitempty"` + ForceDaemonSet bool `json:",omitempty"` + EggSet bool `json:",omitempty"` + AdvertiseRoutesSet bool `json:",omitempty"` + NoSNATSet bool `json:",omitempty"` + NetfilterModeSet bool `json:",omitempty"` + OperatorUserSet bool `json:",omitempty"` + ProfileNameSet bool `json:",omitempty"` + AutoUpdateSet AutoUpdatePrefsMask `json:",omitempty"` + AppConnectorSet bool `json:",omitempty"` + PostureCheckingSet bool `json:",omitempty"` + NetfilterKindSet bool `json:",omitempty"` +} + +type AutoUpdatePrefsMask struct { + CheckSet bool `json:",omitempty"` + ApplySet bool `json:",omitempty"` +} + +func (m AutoUpdatePrefsMask) Pretty(au AutoUpdatePrefs) string { + var fields []string + if m.CheckSet { + fields = append(fields, fmt.Sprintf("Check=%v", au.Check)) + } + if m.ApplySet { + fields = append(fields, fmt.Sprintf("Apply=%v", au.Apply)) + } + return strings.Join(fields, " ") } // ApplyEdits mutates p, assigning fields from m.Prefs for each MaskedPrefs @@ -291,15 +308,36 @@ func (p *Prefs) ApplyEdits(m *MaskedPrefs) { 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) + applyPrefsEdits(mpv, pv, maskFields(mv)) +} + +func applyPrefsEdits(src, dst reflect.Value, mask map[string]reflect.Value) { + for n, m := range mask { + switch m.Kind() { + case reflect.Bool: + if m.Bool() { + dst.FieldByName(n).Set(src.FieldByName(n)) + } + case reflect.Struct: + applyPrefsEdits(src.FieldByName(n), dst.FieldByName(n), maskFields(m)) + default: + panic(fmt.Sprintf("unsupported mask field kind %v", m.Kind())) } } } +func maskFields(v reflect.Value) map[string]reflect.Value { + mask := make(map[string]reflect.Value) + for i := 0; i < v.NumField(); i++ { + f := v.Type().Field(i).Name + if !strings.HasSuffix(f, "Set") { + continue + } + mask[strings.TrimSuffix(f, "Set")] = v.Field(i) + } + return mask +} + // IsEmpty reports whether there are no masks set or if m is nil. func (m *MaskedPrefs) IsEmpty() bool { if m == nil { @@ -308,7 +346,7 @@ func (m *MaskedPrefs) IsEmpty() bool { mv := reflect.ValueOf(m).Elem() fields := mv.NumField() for i := 1; i < fields; i++ { - if mv.Field(i).Bool() { + if !mv.Field(i).IsZero() { return false } } @@ -347,15 +385,30 @@ func (m *MaskedPrefs) Pretty() string { for i := 1; i < mt.NumField(); i++ { name := mt.Field(i).Name - if mv.Field(i).Bool() { - if !first { - sb.WriteString(" ") + mf := mv.Field(i) + switch mf.Kind() { + case reflect.Bool: + if mf.Bool() { + if !first { + sb.WriteString(" ") + } + first = false + f := mpv.Field(i - 1) + fmt.Fprintf(&sb, format(f), + strings.TrimSuffix(name, "Set"), + f.Interface()) + } + case reflect.Struct: + if mf.IsZero() { + continue + } + mpf := mpv.Field(i - 1) + prettyFn := mf.MethodByName("Pretty") + if !prettyFn.IsValid() { + panic(fmt.Sprintf("MaskedPrefs field %q is missing the Pretty method", name)) } - first = false - f := mpv.Field(i - 1) - fmt.Fprintf(&sb, format(f), - strings.TrimSuffix(name, "Set"), - f.Interface()) + res := prettyFn.Call([]reflect.Value{mpf}) + fmt.Fprintf(&sb, "%s={%s}", strings.TrimSuffix(name, "Set"), res[0].String()) } } sb.WriteString("}") diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index f3987d2bf..1a09a94a9 100644 --- a/ipn/prefs_test.go +++ b/ipn/prefs_test.go @@ -761,6 +761,42 @@ func TestMaskedPrefsPretty(t *testing.T) { }, want: `MaskedPrefs{ExitNodeIP=100.102.104.105}`, }, + { + m: &MaskedPrefs{ + Prefs: Prefs{ + AutoUpdate: AutoUpdatePrefs{Check: true, Apply: false}, + }, + AutoUpdateSet: AutoUpdatePrefsMask{CheckSet: true, ApplySet: false}, + }, + want: `MaskedPrefs{AutoUpdate={Check=true}}`, + }, + { + m: &MaskedPrefs{ + Prefs: Prefs{ + AutoUpdate: AutoUpdatePrefs{Check: true, Apply: true}, + }, + AutoUpdateSet: AutoUpdatePrefsMask{CheckSet: true, ApplySet: true}, + }, + want: `MaskedPrefs{AutoUpdate={Check=true Apply=true}}`, + }, + { + m: &MaskedPrefs{ + Prefs: Prefs{ + AutoUpdate: AutoUpdatePrefs{Check: true, Apply: false}, + }, + AutoUpdateSet: AutoUpdatePrefsMask{CheckSet: false, ApplySet: true}, + }, + want: `MaskedPrefs{AutoUpdate={Apply=false}}`, + }, + { + m: &MaskedPrefs{ + Prefs: Prefs{ + AutoUpdate: AutoUpdatePrefs{Check: true, Apply: true}, + }, + AutoUpdateSet: AutoUpdatePrefsMask{CheckSet: false, ApplySet: false}, + }, + want: `MaskedPrefs{}`, + }, } for i, tt := range tests { got := tt.m.Pretty()