From dd6c76ea24d14157f46908ecc8a35afc7d9efef0 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 15 Apr 2024 21:40:21 -0700 Subject: [PATCH] ipn: remove unused Options.LegacyMigrationPrefs I'm on a mission to simplify LocalBackend.Start and its locking and deflake some tests. I noticed this hasn't been used since March 2023 when it was removed from the Windows client in corp 66be796d33c. So, delete. Updates #11649 Change-Id: I40f2cb75fb3f43baf23558007655f65a8ec5e1b2 Signed-off-by: Brad Fitzpatrick --- ipn/backend.go | 13 ++------ ipn/ipnlocal/local.go | 53 ++++-------------------------- wgengine/netstack/netstack_test.go | 14 ++++---- 3 files changed, 16 insertions(+), 64 deletions(-) diff --git a/ipn/backend.go b/ipn/backend.go index 764579ef7..6408d7e98 100644 --- a/ipn/backend.go +++ b/ipn/backend.go @@ -232,18 +232,11 @@ var DebuggableComponents = []string{ type Options struct { // FrontendLogID is the public logtail id used by the frontend. FrontendLogID string - // LegacyMigrationPrefs are used to migrate preferences from the - // frontend to the backend. - // If non-nil, they are imported as a new profile. - LegacyMigrationPrefs *Prefs `json:"Prefs"` - // UpdatePrefs, if provided, overrides Options.LegacyMigrationPrefs - // *and* the Prefs already stored in the backend state, *except* for - // the Persist member. If you just want to provide prefs, this is - // probably what you want. + // UpdatePrefs, if provided, overrides the Prefs already stored in the + // backend state, *except* for the Persist member. // // TODO(apenwarr): Rename this to Prefs, and possibly move Prefs.Persist - // elsewhere entirely (as it always should have been). Or, move the - // fancy state migration stuff out of Start(). + // elsewhere entirely (as it always should have been). UpdatePrefs *Prefs // AuthKey is an optional node auth key used to authorize a // new node key without user interaction. diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 6c575d2d4..f88e88009 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1623,7 +1623,6 @@ func (b *LocalBackend) startIsNoopLocked(opts ipn.Options) bool { return b.state == ipn.Running && b.hostinfo != nil && b.hostinfo.FrontendLogID == opts.FrontendLogID && - opts.LegacyMigrationPrefs == nil && opts.UpdatePrefs == nil && opts.AuthKey == "" } @@ -1639,28 +1638,15 @@ func (b *LocalBackend) startIsNoopLocked(opts ipn.Options) bool { // actually a supported operation (it should be, but it's very unclear // from the following whether or not that is a safe transition). func (b *LocalBackend) Start(opts ipn.Options) error { - if opts.LegacyMigrationPrefs != nil { - b.logf("Start: %v", opts.LegacyMigrationPrefs.Pretty()) - } else { - b.logf("Start") - } + b.logf("Start") b.mu.Lock() - if opts.LegacyMigrationPrefs == nil && !b.pm.CurrentPrefs().Valid() { - b.mu.Unlock() - return errors.New("no prefs provided") - } if opts.UpdatePrefs != nil { if err := b.checkPrefsLocked(opts.UpdatePrefs); err != nil { b.mu.Unlock() return err } - } else if opts.LegacyMigrationPrefs != nil { - if err := b.checkPrefsLocked(opts.LegacyMigrationPrefs); err != nil { - b.mu.Unlock() - return err - } } profileID := b.pm.CurrentProfile().ID if b.state != ipn.Running && b.conf != nil && b.conf.Parsed.AuthKey != nil && opts.AuthKey == "" { @@ -1720,11 +1706,6 @@ func (b *LocalBackend) Start(opts ipn.Options) error { b.hostinfo = hostinfo b.state = ipn.NoState - if err := b.migrateStateLocked(opts.LegacyMigrationPrefs); err != nil { - b.mu.Unlock() - return fmt.Errorf("loading requested state: %v", err) - } - if opts.UpdatePrefs != nil { oldPrefs := b.pm.CurrentPrefs() newPrefs := opts.UpdatePrefs.Clone() @@ -1737,6 +1718,8 @@ func (b *LocalBackend) Start(opts ipn.Options) error { b.logf("failed to save UpdatePrefs state: %v", err) } b.setAtomicValuesFromPrefsLocked(pv) + } else { + b.setAtomicValuesFromPrefsLocked(b.pm.CurrentPrefs()) } prefs := b.pm.CurrentPrefs() @@ -1799,9 +1782,9 @@ func (b *LocalBackend) Start(opts ipn.Options) error { } // TODO(apenwarr): The only way to change the ServerURL is to - // re-run b.Start(), because this is the only place we create a - // new controlclient. SetPrefs() allows you to overwrite ServerURL, - // but it won't take effect until the next Start(). + // re-run b.Start, because this is the only place we create a + // new controlclient. EditPrefs allows you to overwrite ServerURL, + // but it won't take effect until the next Start. cc, err := b.getNewControlClientFunc()(controlclient.Options{ GetMachinePrivateKey: b.createGetMachinePrivateKeyFunc(), Logf: logger.WithPrefix(b.logf, "control: "), @@ -2681,30 +2664,6 @@ func (b *LocalBackend) clearMachineKeyLocked() error { return nil } -// migrateStateLocked migrates state from the frontend to the backend. -// It is a no-op if prefs is nil -// b.mu must be held. -func (b *LocalBackend) migrateStateLocked(prefs *ipn.Prefs) (err error) { - if prefs == nil && !b.pm.CurrentPrefs().Valid() { - return fmt.Errorf("no prefs provided and no current profile") - } - if prefs != nil { - // Backend owns the state, but frontend is trying to migrate - // state into the backend. - b.logf("importing frontend prefs into backend store; frontend prefs: %s", prefs.Pretty()) - if err := b.pm.SetPrefs(prefs.View(), ipn.NetworkProfile{ - MagicDNSName: b.netMap.MagicDNSSuffix(), - DomainName: b.netMap.DomainName(), - }); err != nil { - return fmt.Errorf("store.WriteState: %v", err) - } - } - - b.setAtomicValuesFromPrefsLocked(b.pm.CurrentPrefs()) - - return nil -} - // setTCPPortsIntercepted populates b.shouldInterceptTCPPortAtomic with an // efficient func for ShouldInterceptTCPPort to use, which is called on every // incoming packet. diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 73e1d1297..a8a4edcd9 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -292,7 +292,7 @@ func TestShouldProcessInbound(t *testing.T) { netip.MustParsePrefix("fd7a:115c:a1e0:b1a:0:7:a01:100/120"), } i.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) i.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress) }, @@ -325,7 +325,7 @@ func TestShouldProcessInbound(t *testing.T) { netip.MustParsePrefix("fd7a:115c:a1e0:b1a:0:7:a01:200/120"), } i.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) }, want: false, @@ -343,7 +343,7 @@ func TestShouldProcessInbound(t *testing.T) { prefs := ipn.NewPrefs() prefs.RunSSH = true i.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) i.atomicIsLocalIPFunc.Store(func(addr netip.Addr) bool { return addr.String() == "100.101.102.104" // Dst, above @@ -365,7 +365,7 @@ func TestShouldProcessInbound(t *testing.T) { prefs := ipn.NewPrefs() prefs.RunSSH = false // default, but to be explicit i.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) i.atomicIsLocalIPFunc.Store(func(addr netip.Addr) bool { return addr.String() == "100.101.102.104" // Dst, above @@ -430,7 +430,7 @@ func TestShouldProcessInbound(t *testing.T) { netip.MustParsePrefix("10.0.0.1/24"), } i.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) // Set the PeerAPI port to the Dst port above. @@ -549,7 +549,7 @@ func TestTCPForwardLimits(t *testing.T) { netip.MustParsePrefix("192.0.2.0/24"), } impl.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) impl.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress) @@ -629,7 +629,7 @@ func TestTCPForwardLimits_PerClient(t *testing.T) { netip.MustParsePrefix("192.0.2.0/24"), } impl.lb.Start(ipn.Options{ - LegacyMigrationPrefs: prefs, + UpdatePrefs: prefs, }) impl.atomicIsLocalIPFunc.Store(looksLikeATailscaleSelfAddress)