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

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()
+	unlock := b.lockAndGetUnlock()
-	defer b.mu.Unlock()
+	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 {
+// initPrefsFromConfig initializes the backend's prefs from the provided config.
-
+// This should only be called once, at startup. For updates at runtime, use
-	// TODO(irbekrm): notify the relevant components to consume any prefs
+// [LocalBackend.setConfigLocked].
-	// updates. Currently only initial configfile settings are applied
+func (b *LocalBackend) initPrefsFromConfig(conf *conffile.Config) error {
-	// immediately.
+	// 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)
-	defer func() {
 	b.conf = conf
-	}()
-
-	if conf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == nil) {
 	return nil
+}
+
+func (b *LocalBackend) setStaticEndpointsFromConfigLocked(conf *conffile.Config) {
+	if conf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == 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
 }
 

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)
+	if _, ok := sys.StateStore.GetOK(); !ok {
-	store := new(mem.Store)
+		sys.Set(new(mem.Store))
-	sys.Set(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)
+	}
 	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")
+	}
+}