code review feedback

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 change 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,53 @@ 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 reports whether newConf modifies 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

@@ -645,7 +645,7 @@ func (c *Conn) setEndpoints(endpoints []tailcfg.Endpoint) (changed bool) {
 
 // SetStaticEndpoints sets static endpoints to the provided value and triggers
 // an asynchronous update of the endpoints that this node advertises.
-// Static endpoints are endpoints explicitly configured by user.
+// Static endpoints are endpoints explicitly configured by the user.
 func (c *Conn) SetStaticEndpoints(ep views.Slice[netip.AddrPort]) {
 	c.mu.Lock()
 	if reflect.DeepEqual(c.staticEndpoints.AsSlice(), ep.AsSlice()) {
@@ -659,6 +659,13 @@ func (c *Conn) SetStaticEndpoints(ep views.Slice[netip.AddrPort]) {
 	c.ReSTUN("static-endpoint-change")
 }
 
+// GetStaticEndpoints returns any wireguard endpoints explicitly configured by the user.
+func (c *Conn) GetStaticEndpoints() views.Slice[netip.AddrPort] {
+	c.mu.Lock()
+	c.mu.Unlock()
+	return c.staticEndpoints
+}
+
 // setNetInfoHavePortMap updates NetInfo.HavePortMap to true.
 func (c *Conn) setNetInfoHavePortMap() {
 	c.mu.Lock()