From d09e9d967f1fd6349a2bddefffe2e9e9f4b33044 Mon Sep 17 00:00:00 2001 From: Maisem Ali Date: Thu, 31 Oct 2024 08:30:11 -0700 Subject: [PATCH] ipn/ipnlocal: reload prefs correctly on ReloadConfig We were only updating the ProfileManager and not going down the EditPrefs path which meant the prefs weren't applied till either the process restarted or some other pref changed. This makes it so that we reconfigure everything correctly when ReloadConfig is called. Updates #13032 Signed-off-by: Maisem Ali --- ipn/ipnlocal/local.go | 48 ++++++++++++++++++++++--------- ipn/ipnlocal/local_test.go | 59 ++++++++++++++++++++++++++++++++------ 2 files changed, 85 insertions(+), 22 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index b91f1337a..edd56f7c4 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -479,7 +479,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo mConn.SetNetInfoCallback(b.setNetInfo) if sys.InitialConfig != nil { - if err := b.setConfigLocked(sys.InitialConfig); err != nil { + if err := b.initPrefsFromConfig(sys.InitialConfig); err != nil { return nil, err } } @@ -712,8 +712,8 @@ func (b *LocalBackend) SetDirectFileRoot(dir string) { // It returns (false, nil) if not running in declarative mode, (true, nil) on // success, or (false, error) on failure. func (b *LocalBackend) ReloadConfig() (ok bool, err error) { - b.mu.Lock() - defer b.mu.Unlock() + unlock := b.lockAndGetUnlock() + defer unlock() if b.conf == nil { return false, nil } @@ -721,18 +721,21 @@ func (b *LocalBackend) ReloadConfig() (ok bool, err error) { if err != nil { return false, err } - if err := b.setConfigLocked(conf); err != nil { + if err := b.setConfigLockedOnEntry(conf, unlock); err != nil { return false, fmt.Errorf("error setting config: %w", err) } return true, nil } -func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error { - - // TODO(irbekrm): notify the relevant components to consume any prefs - // updates. Currently only initial configfile settings are applied - // immediately. +// initPrefsFromConfig initializes the backend's prefs from the provided config. +// This should only be called once, at startup. For updates at runtime, use +// [LocalBackend.setConfigLocked]. +func (b *LocalBackend) initPrefsFromConfig(conf *conffile.Config) error { + // TODO(maisem,bradfitz): combine this with setConfigLocked. This is called + // before anything is running, so there's no need to lock and we don't + // update any subsystems. At runtime, we both need to lock and update + // subsystems with the new prefs. p := b.pm.CurrentPrefs().AsStruct() mp, err := conf.Parsed.ToPrefs() if err != nil { @@ -742,13 +745,14 @@ func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error { if err := b.pm.SetPrefs(p.View(), ipn.NetworkProfile{}); err != nil { return err } + b.setStaticEndpointsFromConfigLocked(conf) + b.conf = conf + return nil +} - defer func() { - b.conf = conf - }() - +func (b *LocalBackend) setStaticEndpointsFromConfigLocked(conf *conffile.Config) { if conf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == nil) { - return nil + return } // Ensure that magicsock conn has the up to date static wireguard @@ -762,6 +766,22 @@ func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error { ms.SetStaticEndpoints(views.SliceOf(conf.Parsed.StaticEndpoints)) } } +} + +// setConfigLockedOnEntry uses the provided config to update the backend's prefs +// and other state. +func (b *LocalBackend) setConfigLockedOnEntry(conf *conffile.Config, unlock unlockOnce) error { + defer unlock() + p := b.pm.CurrentPrefs().AsStruct() + mp, err := conf.Parsed.ToPrefs() + if err != nil { + return fmt.Errorf("error parsing config to prefs: %w", err) + } + p.ApplyEdits(&mp) + b.setStaticEndpointsFromConfigLocked(conf) + b.setPrefsLockedOnEntry(p, unlock) + + b.conf = conf return nil } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 5fee5d00e..433679dda 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -13,6 +13,7 @@ import ( "net/http" "net/netip" "os" + "path/filepath" "reflect" "slices" "strings" @@ -32,6 +33,7 @@ import ( "tailscale.com/health" "tailscale.com/hostinfo" "tailscale.com/ipn" + "tailscale.com/ipn/conffile" "tailscale.com/ipn/ipnauth" "tailscale.com/ipn/store/mem" "tailscale.com/net/netcheck" @@ -432,16 +434,25 @@ func (panicOnUseTransport) RoundTrip(*http.Request) (*http.Response, error) { } func newTestLocalBackend(t testing.TB) *LocalBackend { + return newTestLocalBackendWithSys(t, new(tsd.System)) +} + +// newTestLocalBackendWithSys creates a new LocalBackend with the given tsd.System. +// If the state store or engine are not set in sys, they will be set to a new +// in-memory store and fake userspace engine, respectively. +func newTestLocalBackendWithSys(t testing.TB, sys *tsd.System) *LocalBackend { var logf logger.Logf = logger.Discard - sys := new(tsd.System) - store := new(mem.Store) - sys.Set(store) - eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker(), sys.UserMetricsRegistry()) - if err != nil { - t.Fatalf("NewFakeUserspaceEngine: %v", err) + if _, ok := sys.StateStore.GetOK(); !ok { + sys.Set(new(mem.Store)) + } + if _, ok := sys.Engine.GetOK(); !ok { + eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker(), sys.UserMetricsRegistry()) + if err != nil { + t.Fatalf("NewFakeUserspaceEngine: %v", err) + } + t.Cleanup(eng.Close) + sys.Set(eng) } - t.Cleanup(eng.Close) - sys.Set(eng) lb, err := NewLocalBackend(logf, logid.PublicID{}, sys, 0) if err != nil { t.Fatalf("NewLocalBackend: %v", err) @@ -4423,3 +4434,35 @@ func TestLoginNotifications(t *testing.T) { }) } } + +// TestConfigFileReload tests that the LocalBackend reloads its configuration +// when the configuration file changes. +func TestConfigFileReload(t *testing.T) { + cfg1 := `{"Hostname": "foo", "Version": "alpha0"}` + f := filepath.Join(t.TempDir(), "cfg") + must.Do(os.WriteFile(f, []byte(cfg1), 0600)) + sys := new(tsd.System) + sys.InitialConfig = must.Get(conffile.Load(f)) + lb := newTestLocalBackendWithSys(t, sys) + must.Do(lb.Start(ipn.Options{})) + + lb.mu.Lock() + hn := lb.hostinfo.Hostname + lb.mu.Unlock() + if hn != "foo" { + t.Fatalf("got %q; want %q", hn, "foo") + } + + cfg2 := `{"Hostname": "bar", "Version": "alpha0"}` + must.Do(os.WriteFile(f, []byte(cfg2), 0600)) + if !must.Get(lb.ReloadConfig()) { + t.Fatal("reload failed") + } + + lb.mu.Lock() + hn = lb.hostinfo.Hostname + lb.mu.Unlock() + if hn != "bar" { + t.Fatalf("got %q; want %q", hn, "bar") + } +}