ipn/ipnlocal: set default NoStatefulFiltering in ipn.NewPrefs (#12031)

This way the default gets populated on first start, when no existing
state exists to migrate. Also fix `ipn.PrefsFromBytes` to preserve empty
fields, rather than layering `NewPrefs` values on top.

Updates https://github.com/tailscale/corp/issues/19623

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

cmd/tailscale/cli/cli_test.go

@@ -672,12 +672,14 @@ func TestPrefsFromUpArgs(t *testing.T) {
 			goos: "windows",
 			args: upArgsFromOSArgs("windows"),
 			want: &ipn.Prefs{
-				ControlURL:       ipn.DefaultControlURL,
+				ControlURL:          ipn.DefaultControlURL,
-				WantRunning:      true,
+				WantRunning:         true,
-				CorpDNS:          true,
+				CorpDNS:             true,
-				AllowSingleHosts: true,
+				AllowSingleHosts:    true,
-				RouteAll:         true,
+				RouteAll:            true,
-				NetfilterMode:    preftype.NetfilterOn,
+				NoSNAT:              false,
+				NoStatefulFiltering: "false",
+				NetfilterMode:       preftype.NetfilterOn,
 				AutoUpdate: ipn.AutoUpdatePrefs{
 					Check: true,
 				},

ipn/ipnlocal/profiles.go

@@ -353,9 +353,13 @@ func (pm *profileManager) loadSavedPrefs(key ipn.StateKey) (ipn.PrefsView, error
 	if err != nil {
 		return ipn.PrefsView{}, err
 	}
-	savedPrefs, err := ipn.PrefsFromBytes(bs)
+	savedPrefs := ipn.NewPrefs()
-	if err != nil {
+	// NewPrefs sets a default NoStatefulFiltering, but we want to actually see
-		return ipn.PrefsView{}, fmt.Errorf("PrefsFromBytes: %v", err)
+	// if the saved state had an empty value. The empty value gets migrated
+	// based on NoSNAT, while a default "false" does not.
+	savedPrefs.NoStatefulFiltering = ""
+	if err := ipn.PrefsFromBytes(bs, savedPrefs); err != nil {
+		return ipn.PrefsView{}, fmt.Errorf("parsing saved prefs: %v", err)
 	}
 	pm.logf("using backend prefs for %q: %v", key, savedPrefs.Pretty())
 
@@ -495,7 +499,6 @@ var defaultPrefs = func() ipn.PrefsView {
 	prefs := ipn.NewPrefs()
 	prefs.LoggedOut = true
 	prefs.WantRunning = false
-	prefs.NoStatefulFiltering = "false"
 
 	return prefs.View()
 }()

ipn/ipnlocal/profiles_test.go

@@ -681,8 +681,21 @@ func TestProfileBackfillStatefulFiltering(t *testing.T) {
 			// StatefulFiltering boolean.
 			pf := pm.CurrentPrefs()
 			if !pf.NoStatefulFiltering().EqualBool(tt.want) {
-				t.Fatalf("got NoStatefulFiltering=%v, want %v", pf.NoStatefulFiltering(), tt.want)
+				t.Fatalf("got NoStatefulFiltering=%q, want %v", pf.NoStatefulFiltering(), tt.want)
 			}
 		})
 	}
 }
+
+// TestDefaultPrefs tests that defaultPrefs is just NewPrefs with
+// LoggedOut=true (the Prefs we use before connecting to control). We shouldn't
+// be putting any defaulting there, and instead put all defaults in NewPrefs.
+func TestDefaultPrefs(t *testing.T) {
+	p1 := ipn.NewPrefs()
+	p1.LoggedOut = true
+	p1.WantRunning = false
+	p2 := defaultPrefs
+	if !p1.View().Equals(p2) {
+		t.Errorf("defaultPrefs is %s, want %s; defaultPrefs should only modify WantRunning and LoggedOut, all other defaults should be in ipn.NewPrefs.", p2.Pretty(), p1.Pretty())
+	}
+}

ipn/ipnlocal/profiles_windows.go

@@ -57,7 +57,7 @@ func (pm *profileManager) loadLegacyPrefs() (string, ipn.PrefsView, error) {
 	}
 
 	prefsPath := filepath.Join(userLegacyPrefsDir, legacyPrefsFile+legacyPrefsExt)
-	prefs, err := ipn.LoadPrefs(prefsPath)
+	prefs, err := ipn.LoadPrefsWindows(prefsPath)
 	pm.dlogf("ipn.LoadPrefs(%q) = %v, %v", prefsPath, prefs, err)
 	if errors.Is(err, fs.ErrNotExist) {
 		return "", ipn.PrefsView{}, errAlreadyMigrated

ipn/prefs.go

@@ -662,11 +662,12 @@ func NewPrefs() *Prefs {
 		// later anyway.
 		ControlURL: "",
 
-		RouteAll:         true,
+		RouteAll:            true,
-		AllowSingleHosts: true,
+		AllowSingleHosts:    true,
-		CorpDNS:          true,
+		CorpDNS:             true,
-		WantRunning:      false,
+		WantRunning:         false,
-		NetfilterMode:    preftype.NetfilterOn,
+		NetfilterMode:       preftype.NetfilterOn,
+		NoStatefulFiltering: opt.NewBool(false),
 		AutoUpdate: AutoUpdatePrefs{
 			Check: true,
 			Apply: opt.Bool("unset"),
@@ -875,24 +876,21 @@ func (p *Prefs) ShouldWebClientBeRunning() bool {
 	return p.WantRunning && p.RunWebClient
 }
 
-// PrefsFromBytes deserializes Prefs from a JSON blob.
+// PrefsFromBytes deserializes Prefs from a JSON blob b into base. Values in
-func PrefsFromBytes(b []byte) (*Prefs, error) {
+// base are preserved, unless they are populated in the JSON blob.
-	p := NewPrefs()
+func PrefsFromBytes(b []byte, base *Prefs) error {
 	if len(b) == 0 {
-		return p, nil
+		return nil
 	}
 
-	if err := json.Unmarshal(b, p); err != nil {
+	return json.Unmarshal(b, base)
-		return nil, err
-	}
-	return p, nil
 }
 
 var jsonEscapedZero = []byte(`\u0000`)
 
-// LoadPrefs loads a legacy relaynode config file into Prefs
+// LoadPrefsWindows loads a legacy relaynode config file into Prefs with
-// with sensible migration defaults set.
+// sensible migration defaults set. Windows-only.
-func LoadPrefs(filename string) (*Prefs, error) {
+func LoadPrefsWindows(filename string) (*Prefs, error) {
 	data, err := os.ReadFile(filename)
 	if err != nil {
 		return nil, fmt.Errorf("LoadPrefs open: %w", err) // err includes path
@@ -905,8 +903,8 @@ func LoadPrefs(filename string) (*Prefs, error) {
 		// to log in again. (better than crashing)
 		return nil, os.ErrNotExist
 	}
-	p, err := PrefsFromBytes(data)
+	p := NewPrefs()
-	if err != nil {
+	if err := PrefsFromBytes(data, p); err != nil {
 		return nil, fmt.Errorf("LoadPrefs(%q) decode: %w", filename, err)
 	}
 	return p, nil

ipn/prefs_test.go

@@ -373,7 +373,8 @@ func checkPrefs(t *testing.T, p Prefs) {
 	if p.Equals(p2) {
 		t.Fatalf("p == p2\n")
 	}
-	p2b, err = PrefsFromBytes(p2.ToBytes())
+	p2b = new(Prefs)
+	err = PrefsFromBytes(p2.ToBytes(), p2b)
 	if err != nil {
 		t.Fatalf("PrefsFromBytes(p2) failed\n")
 	}
@@ -586,7 +587,7 @@ func TestPrefsPretty(t *testing.T) {
 func TestLoadPrefsNotExist(t *testing.T) {
 	bogusFile := fmt.Sprintf("/tmp/not-exist-%d", time.Now().UnixNano())
 
-	p, err := LoadPrefs(bogusFile)
+	p, err := LoadPrefsWindows(bogusFile)
 	if errors.Is(err, os.ErrNotExist) {
 		// expected.
 		return
@@ -608,7 +609,7 @@ func TestLoadPrefsFileWithZeroInIt(t *testing.T) {
 	f.Close()
 	defer os.Remove(path)
 
-	p, err := LoadPrefs(path)
+	p, err := LoadPrefsWindows(path)
 	if errors.Is(err, os.ErrNotExist) {
 		// expected.
 		return