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 <awly@tailscale.com>

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
 		}
 	}
 

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() {

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,10 +728,15 @@ func addPrefFlagMapping(flagName string, prefNames ...string) {
 	prefsOfFlag[flagName] = prefNames
 	prefType := reflect.TypeOf(ipn.Prefs{})
 	for _, pref := range prefNames {
+		t := prefType
+		for _, name := range strings.Split(pref, ".") {
 			// Crash at runtime if there's a typo in the prefName.
-		if _, ok := prefType.FieldByName(pref); !ok {
+			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
 	}

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
 }

ipn/prefs.go

@@ -248,9 +248,10 @@ 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
 
@@ -276,12 +277,28 @@ type MaskedPrefs struct {
 	NetfilterModeSet          bool                `json:",omitempty"`
 	OperatorUserSet           bool                `json:",omitempty"`
 	ProfileNameSet            bool                `json:",omitempty"`
-	AutoUpdateSet             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
 // Set field that's true.
 func (p *Prefs) ApplyEdits(m *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,7 +385,10 @@ func (m *MaskedPrefs) Pretty() string {
 
 	for i := 1; i < mt.NumField(); i++ {
 		name := mt.Field(i).Name
-		if mv.Field(i).Bool() {
+		mf := mv.Field(i)
+		switch mf.Kind() {
+		case reflect.Bool:
+			if mf.Bool() {
 				if !first {
 					sb.WriteString(" ")
 				}
@@ -357,6 +398,18 @@ func (m *MaskedPrefs) Pretty() string {
 					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))
+			}
+			res := prettyFn.Call([]reflect.Value{mpf})
+			fmt.Fprintf(&sb, "%s={%s}", strings.TrimSuffix(name, "Set"), res[0].String())
+		}
 	}
 	sb.WriteString("}")
 	return sb.String()

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()