From ecd69c9cf2e749d83894147f8fa83147b6764731 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Mon, 6 May 2024 18:28:00 -0700 Subject: [PATCH] ipn/ipnlocal: set default NoStatefulFiltering in ipn.NewPrefs 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 --- cmd/tailscale/cli/cli_test.go | 14 +++++++------ ipn/ipnlocal/profiles.go | 11 +++++++---- ipn/ipnlocal/profiles_test.go | 15 +++++++++++++- ipn/ipnlocal/profiles_windows.go | 2 +- ipn/prefs.go | 34 +++++++++++++++----------------- ipn/prefs_test.go | 7 ++++--- 6 files changed, 50 insertions(+), 33 deletions(-) diff --git a/cmd/tailscale/cli/cli_test.go b/cmd/tailscale/cli/cli_test.go index 1e516f040..c49707612 100644 --- a/cmd/tailscale/cli/cli_test.go +++ b/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, - WantRunning: true, - CorpDNS: true, - AllowSingleHosts: true, - RouteAll: true, - NetfilterMode: preftype.NetfilterOn, + ControlURL: ipn.DefaultControlURL, + WantRunning: true, + CorpDNS: true, + AllowSingleHosts: true, + RouteAll: true, + NoSNAT: false, + NoStatefulFiltering: "false", + NetfilterMode: preftype.NetfilterOn, AutoUpdate: ipn.AutoUpdatePrefs{ Check: true, }, diff --git a/ipn/ipnlocal/profiles.go b/ipn/ipnlocal/profiles.go index b0a002947..d50a141ad 100644 --- a/ipn/ipnlocal/profiles.go +++ b/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) - if err != nil { - return ipn.PrefsView{}, fmt.Errorf("PrefsFromBytes: %v", err) + savedPrefs := ipn.NewPrefs() + // NewPrefs sets a default NoStatefulFiltering, but we want to actually see + // 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() }() diff --git a/ipn/ipnlocal/profiles_test.go b/ipn/ipnlocal/profiles_test.go index 0acd0dcab..113aaec65 100644 --- a/ipn/ipnlocal/profiles_test.go +++ b/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()) + } +} diff --git a/ipn/ipnlocal/profiles_windows.go b/ipn/ipnlocal/profiles_windows.go index 276b20ba9..d98f4b526 100644 --- a/ipn/ipnlocal/profiles_windows.go +++ b/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 diff --git a/ipn/prefs.go b/ipn/prefs.go index 2a353a531..3a603ecf2 100644 --- a/ipn/prefs.go +++ b/ipn/prefs.go @@ -662,11 +662,12 @@ func NewPrefs() *Prefs { // later anyway. ControlURL: "", - RouteAll: true, - AllowSingleHosts: true, - CorpDNS: true, - WantRunning: false, - NetfilterMode: preftype.NetfilterOn, + RouteAll: true, + AllowSingleHosts: true, + CorpDNS: true, + WantRunning: false, + 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. -func PrefsFromBytes(b []byte) (*Prefs, error) { - p := NewPrefs() +// PrefsFromBytes deserializes Prefs from a JSON blob b into base. Values in +// base are preserved, unless they are populated in the JSON blob. +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 nil, err - } - return p, nil + return json.Unmarshal(b, base) } var jsonEscapedZero = []byte(`\u0000`) -// LoadPrefs loads a legacy relaynode config file into Prefs -// with sensible migration defaults set. -func LoadPrefs(filename string) (*Prefs, error) { +// LoadPrefsWindows loads a legacy relaynode config file into Prefs with +// sensible migration defaults set. Windows-only. +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) - if err != nil { + p := NewPrefs() + if err := PrefsFromBytes(data, p); err != nil { return nil, fmt.Errorf("LoadPrefs(%q) decode: %w", filename, err) } return p, nil diff --git a/ipn/prefs_test.go b/ipn/prefs_test.go index ad6758cd7..45f8829a6 100644 --- a/ipn/prefs_test.go +++ b/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