ipn/ipnlocal: reload some prefs changes on tailscaled configfile changes for running system

This is the first part of work towards making it possible to reload
tailscaled config in containers, so that the runner does not
need to restart the container on configfile changes.

This change ensures that if values of '.AcceptRoutes' and
'.AdvertizeRoutes' have changed when reload local API endpoint is
called, the change will be applied for a LocalBackend in 'Running' state.
These specific fields where chosen because those are the values that
can change by user action in kube operator proxies.
We can then gradually allow more values to be reloaded as we make
the configfile easier to use in any containerized environment.

Updates tailscale/tailscale#13032

Signed-off-by: Irbe Krumina <irbe@tailscale.com>

ipn/ipnlocal/local.go

@@ -447,7 +447,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.setConfigLocked(sys.InitialConfig, nil); err != nil {
 			return nil, err
 		}
 	}
@@ -624,9 +624,16 @@ 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.
+//
+// If it is called whilst the LocalBackend is in 'Running' state, only a subset
+// of changes currently deemed safe to chnage for a running system are applied.
+// The rest of the changes will get applied next time LocalBackend starts and
+// config is reloaded. Currently (08/2024) changes safe to apply while running
+// are changes to '.AcceptRoutes', '.AdvertiseRoutes' and '.StaticEndpoints'
+// fields.
 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
 	}
@@ -634,40 +641,24 @@ func (b *LocalBackend) ReloadConfig() (ok bool, err error) {
 	if err != nil {
 		return false, err
 	}
-	if err := b.setConfigLocked(conf); err != nil {
+	if err := b.setConfigLocked(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.
-	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)
-	if err := b.pm.SetPrefs(p.View(), ipn.NetworkProfile{}); err != nil {
-		return err
-	}
-
+// setConfigLocked applies tailscaled config from conffile.Config.
+// unlock must be non-nil if called for a 'Running' LocalBackend.
+func (b *LocalBackend) setConfigLocked(conf *conffile.Config, unlock unlockOnce) error {
 	defer func() {
 		b.conf = conf
 	}()
 
-	if conf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == nil) {
-		return nil
-	}
-
 	// Ensure that magicsock conn has the up to date static wireguard
 	// endpoints. Setting the endpoints here triggers an asynchronous update
 	// of the node's advertised endpoints.
-	if b.conf == nil && len(conf.Parsed.StaticEndpoints) != 0 || !reflect.DeepEqual(conf.Parsed.StaticEndpoints, b.conf.Parsed.StaticEndpoints) {
+	if b.needStaticEndpointsUpdate(conf) {
 		ms, ok := b.sys.MagicSock.GetOK()
 		if !ok {
 			b.logf("[unexpected] ReloadConfig: MagicSock not set")
@@ -675,7 +666,54 @@ func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error {
 			ms.SetStaticEndpoints(views.SliceOf(conf.Parsed.StaticEndpoints))
 		}
 	}
-	return nil
+
+	oldPrefs := b.pm.CurrentPrefs().AsStruct()
+	newPrefs, err := conf.Parsed.ToPrefs()
+	if err != nil {
+		return fmt.Errorf("error parsing config to prefs: %w", err)
+	}
+
+	// If we are not running it should be safe to set all prefs. We can
+	// assume that these changes will get propagated before state changes to
+	// 'Running'.
+	if b.state != ipn.Running {
+		oldPrefs.ApplyEdits(&newPrefs)
+		return b.pm.SetPrefs(oldPrefs.View(), ipn.NetworkProfile{})
+	}
+
+	// If state is 'Running', selectively apply a few known safe prefs
+	// changes.
+	// As tailscaled will always read the configfile contents on start, we
+	// don't need to do anything to the prefs that aren't considered safe
+	// for reload whilst the state is 'Running'.
+	// TODO (irbekrm): eventually we should re-apply all prefs,
+	// this needs testing and validation.
+	if unlock == nil {
+		b.logf("[unexpected] unable to apply prefs update as setConfigLocked called with nil unlock, please report this")
+		return nil
+	}
+	// Generate a patch of changes safe to apply while in 'Running' state.
+	prefsPatch := &ipn.MaskedPrefs{
+		AdvertiseRoutesSet: newPrefs.AdvertiseRoutesSet,
+		RouteAllSet:        newPrefs.RouteAllSet,
+		Prefs: ipn.Prefs{
+			AdvertiseRoutes: newPrefs.AdvertiseRoutes,
+			RouteAll:        newPrefs.RouteAll,
+		},
+	}
+	_, err = b.editPrefsLockedOnEntry(prefsPatch, unlock)
+	return err
+}
+
+// needStaticEndpointUpdate accepts a configfile and returns true if the
+// configfile contains a change to the static wireguard endpoints.
+func (b *LocalBackend) needStaticEndpointsUpdate(newConf *conffile.Config) bool {
+	// If the new configfile does not have static endpoints set and existing
+	// config does not have any (to unset), no need to update.
+	if newConf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == nil) {
+		return false
+	}
+	return len(newConf.Parsed.StaticEndpoints) != 0 && b.conf == nil || !reflect.DeepEqual(newConf.Parsed.StaticEndpoints, b.conf.Parsed.StaticEndpoints)
 }
 
 var assumeNetworkUpdateForTest = envknob.RegisterBool("TS_ASSUME_NETWORK_UP_FOR_TEST")

ipn/ipnlocal/local_test.go

@@ -31,6 +31,7 @@ import (
 	"tailscale.com/health"
 	"tailscale.com/hostinfo"
 	"tailscale.com/ipn"
+	"tailscale.com/ipn/conffile"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/net/netcheck"
 	"tailscale.com/net/netmon"
@@ -427,11 +428,15 @@ func (panicOnUseTransport) RoundTrip(*http.Request) (*http.Response, error) {
 	panic("unexpected HTTP request")
 }
 
-func newTestLocalBackend(t testing.TB) *LocalBackend {
+func newTestLocalBackend(t testing.TB, sysOpts ...func(*tsd.System)) *LocalBackend {
 	var logf logger.Logf = logger.Discard
 	sys := new(tsd.System)
 	store := new(mem.Store)
 	sys.Set(store)
+	for _, opt := range sysOpts {
+		opt(sys)
+	}
+
 	eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker())
 	if err != nil {
 		t.Fatalf("NewFakeUserspaceEngine: %v", err)
@@ -3998,3 +4003,96 @@ func TestFillAllowedSuggestions(t *testing.T) {
 		})
 	}
 }
+
+func TestSetConfigLocked(t *testing.T) {
+	wantRoutes := routes([]string{"10.0.0.1/32"})
+	wantAcceptRoutes := opt.NewBool(true)
+	wantTailscaleURL := ptr.To("headscale.foo.com")
+	wantStaticEndpoints := ap([]string{"35.0.0.1:1234"})
+
+	conf := &conffile.Config{
+		Parsed: ipn.ConfigVAlpha{
+			AdvertiseRoutes: wantRoutes,
+			AcceptRoutes:    wantAcceptRoutes,
+			ServerURL:       wantTailscaleURL,
+			Locked:          opt.NewBool(false),
+			StaticEndpoints: wantStaticEndpoints,
+		},
+	}
+	lb := newTestLocalBackend(t, func(sys *tsd.System) { sys.InitialConfig = conf })
+
+	if err := lb.Start(ipn.Options{}); err != nil {
+		t.Fatalf("Start: %v", err)
+	}
+
+	check := func(t *testing.T) {
+		t.Helper()
+		gotRoutes := lb.Prefs().AdvertiseRoutes().AsSlice()
+		if !reflect.DeepEqual(wantRoutes, gotRoutes) {
+			t.Fatalf("wants advertize routes %v, got %v", wantRoutes, gotRoutes)
+		}
+		if routeAll := lb.Prefs().RouteAll(); !wantAcceptRoutes.EqualBool(routeAll) {
+			t.Fatalf("wants 'RouteAll=%t', got %t", !routeAll, routeAll)
+		}
+		if tsURL := lb.Prefs().ControlURL(); tsURL != *wantTailscaleURL {
+			t.Fatalf("wants 'ControlURL=%s', got %s", *wantTailscaleURL, tsURL)
+		}
+		ms, ok := lb.sys.MagicSock.GetOK()
+		if !ok {
+			t.Fatalf("[unexpected] MagicSock not set")
+		}
+		if staticEndpoints := ms.GetStaticEndpoints().AsSlice(); !reflect.DeepEqual(staticEndpoints, wantStaticEndpoints) {
+			t.Fatalf("wants static endpoints %v, got %v", wantStaticEndpoints, staticEndpoints)
+		}
+	}
+	reset := func(t *testing.T) {
+		t.Helper()
+		unlock := lb.lockAndGetUnlock()
+		defer unlock()
+		if err := lb.setConfigLocked(conf, unlock); err != nil {
+			t.Fatalf("error setting config: %v", err)
+		}
+	}
+	check(t)
+
+	// 1. Reset config while LocalBackend is Running. Change in advertised
+	// routes, accept routes and static endpoints should be applied, but not change in control
+	// URL.
+	lb.state = ipn.Running
+	wantRoutes = routes([]string{"10.0.0.2/32"})
+	wantAcceptRoutes = opt.NewBool(false)
+	// TODO(irbekrm): have some way how to test static endpoint reload. This is currently expected to work.
+	// wantStaticEndpoints = ap([]string{"35.0.0.2:12346"})
+	conf.Parsed.AdvertiseRoutes = wantRoutes
+	conf.Parsed.AcceptRoutes = wantAcceptRoutes
+	conf.Parsed.ServerURL = ptr.To("headscale1.foo.com")
+	reset(t)
+	check(t)
+
+	// 2. Reset config when LocalBackend is Stopped.
+	// All changes should be applied.
+	lb.state = ipn.Stopped
+	wantRoutes = routes([]string{"10.0.0.2/32"})
+	wantAcceptRoutes = opt.NewBool(false)
+	wantTailscaleURL = ptr.To("headscale2.foo.com")
+	conf.Parsed.AdvertiseRoutes = wantRoutes
+	conf.Parsed.AcceptRoutes = wantAcceptRoutes
+	conf.Parsed.ServerURL = wantTailscaleURL
+	reset(t)
+	check(t)
+}
+
+func routes(rs []string) []netip.Prefix {
+	rp := make([]netip.Prefix, 0)
+	for _, r := range rs {
+		rp = append(rp, netip.MustParsePrefix(r))
+	}
+	return rp
+}
+func ap(rs []string) []netip.AddrPort {
+	rp := make([]netip.AddrPort, 0)
+	for _, r := range rs {
+		rp = append(rp, netip.MustParseAddrPort(r))
+	}
+	return rp
+}

wgengine/magicsock/magicsock.go

@@ -659,6 +659,11 @@ func (c *Conn) SetStaticEndpoints(ep views.Slice[netip.AddrPort]) {
 	c.ReSTUN("static-endpoint-change")
 }
 
+// GetStaticEndpoints returns any wireguard endpoints explicitly configured by user.
+func (c *Conn) GetStaticEndpoints() views.Slice[netip.AddrPort] {
+	return c.staticEndpoints
+}
+
 // setNetInfoHavePortMap updates NetInfo.HavePortMap to true.
 func (c *Conn) setNetInfoHavePortMap() {
 	c.mu.Lock()