appc: add flag shouldStoreRoutes and controlknob for it

When an app connector is reconfigured and domains to route are removed,
we would like to no longer advertise routes that were discovered for
those domains. In order to do this we plan to store which routes were
discovered for which domains.

Add a controlknob so that we can enable/disable the new behavior.

Updates #11008
Signed-off-by: Fran Bull <fran@tailscale.com>

appc/appconnector.go

@@ -62,6 +62,9 @@ type AppConnector struct {
 	logf            logger.Logf
 	routeAdvertiser RouteAdvertiser
 
+	// storeRoutesFunc will be called to persist routes if it is not nil.
+	storeRoutesFunc func(*RouteInfo) error
+
 	// mu guards the fields that follow
 	mu sync.Mutex
 
@@ -80,11 +83,24 @@ type AppConnector struct {
 }
 
 // NewAppConnector creates a new AppConnector.
-func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser) *AppConnector {
-	return &AppConnector{
+func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser, routeInfo *RouteInfo, storeRoutesFunc func(*RouteInfo) error) *AppConnector {
+	ac := &AppConnector{
 		logf:            logger.WithPrefix(logf, "appc: "),
 		routeAdvertiser: routeAdvertiser,
+		storeRoutesFunc: storeRoutesFunc,
+	}
+	if routeInfo != nil {
+		ac.domains = routeInfo.Domains
+		ac.wildcards = routeInfo.Wildcards
+		ac.controlRoutes = routeInfo.Control
 	}
+	return ac
+}
+
+// ShouldStoreRoutes returns true if the appconnector was created with the controlknob on
+// and is storing its discovered routes persistently.
+func (e *AppConnector) ShouldStoreRoutes() bool {
+	return e.storeRoutesFunc != nil
 }
 
 // UpdateDomainsAndRoutes starts an asynchronous update of the configuration

appc/appconnector_test.go

@@ -17,9 +17,17 @@ import (
 	"tailscale.com/util/must"
 )
 
+func fakeStoreRoutes(*RouteInfo) error { return nil }
+
 func TestUpdateDomains(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
-	a := NewAppConnector(t.Logf, nil)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, nil, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, nil, nil, nil)
+		}
 		a.UpdateDomains([]string{"example.com"})
 
 		a.Wait(ctx)
@@ -43,11 +51,18 @@ func TestUpdateDomains(t *testing.T) {
 			t.Errorf("got %v; want %v", got, want)
 		}
 	}
+}
 
 func TestUpdateRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		a.updateDomains([]string{"*.example.com"})
 
 		// This route should be collapsed into the range
@@ -80,10 +95,17 @@ func TestUpdateRoutes(t *testing.T) {
 			t.Fatalf("unexpected removed routes: %v", rc.RemovedRoutes())
 		}
 	}
+}
 
 func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		mak.Set(&a.domains, "example.com", []netip.Addr{netip.MustParseAddr("192.0.2.1")})
 		rc.SetRoutes([]netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")})
 		routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}
@@ -93,10 +115,17 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
 			t.Fatalf("got %v, want %v", rc.Routes(), routes)
 		}
 	}
+}
 
 func TestDomainRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		a.updateDomains([]string{"example.com"})
 		a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 		a.Wait(context.Background())
@@ -109,11 +138,18 @@ func TestDomainRoutes(t *testing.T) {
 			t.Fatalf("DomainRoutes: got %v, want %v", got, want)
 		}
 	}
+}
 
 func TestObserveDNSResponse(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
 
 		// a has no domains configured, so it should not advertise any routes
 		a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
@@ -177,11 +213,18 @@ func TestObserveDNSResponse(t *testing.T) {
 			t.Errorf("missing %v from %v", "192.0.2.1", a.domains["exmaple.com"])
 		}
 	}
+}
 
 func TestWildcardDomains(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		var a *AppConnector
+		if shouldStore {
+			a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = NewAppConnector(t.Logf, rc, nil, nil)
+		}
 
 		a.updateDomains([]string{"*.example.com"})
 		a.ObserveDNSResponse(dnsResponse("foo.example.com.", "192.0.0.8"))
@@ -207,6 +250,7 @@ func TestWildcardDomains(t *testing.T) {
 			t.Errorf("expected only one wildcard domain, got %v", a.wildcards)
 		}
 	}
+}
 
 // dnsResponse is a test helper that creates a DNS response buffer for the given domain and address
 func dnsResponse(domain, address string) []byte {

control/controlknobs/controlknobs.go

@@ -72,6 +72,10 @@ type Knobs struct {
 	// ProbeUDPLifetime is whether the node should probe UDP path lifetime on
 	// the tail end of an active direct connection in magicsock.
 	ProbeUDPLifetime atomic.Bool
+
+	// AppCStoreRoutes is whether the node should store RouteInfo to StateStore
+	// if it's an app connector.
+	AppCStoreRoutes atomic.Bool
 }
 
 // UpdateFromNodeAttributes updates k (if non-nil) based on the provided self
@@ -96,6 +100,7 @@ func (k *Knobs) UpdateFromNodeAttributes(capMap tailcfg.NodeCapMap) {
 		forceNfTables                 = has(tailcfg.NodeAttrLinuxMustUseNfTables)
 		seamlessKeyRenewal            = has(tailcfg.NodeAttrSeamlessKeyRenewal)
 		probeUDPLifetime              = has(tailcfg.NodeAttrProbeUDPLifetime)
+		appCStoreRoutes               = has(tailcfg.NodeAttrStoreAppCRoutes)
 	)
 
 	if has(tailcfg.NodeAttrOneCGNATEnable) {
@@ -118,6 +123,7 @@ func (k *Knobs) UpdateFromNodeAttributes(capMap tailcfg.NodeCapMap) {
 	k.LinuxForceNfTables.Store(forceNfTables)
 	k.SeamlessKeyRenewal.Store(seamlessKeyRenewal)
 	k.ProbeUDPLifetime.Store(probeUDPLifetime)
+	k.AppCStoreRoutes.Store(appCStoreRoutes)
 }
 
 // AsDebugJSON returns k as something that can be marshalled with json.Marshal
@@ -141,5 +147,6 @@ func (k *Knobs) AsDebugJSON() map[string]any {
 		"LinuxForceNfTables":            k.LinuxForceNfTables.Load(),
 		"SeamlessKeyRenewal":            k.SeamlessKeyRenewal.Load(),
 		"ProbeUDPLifetime":              k.ProbeUDPLifetime.Load(),
+		"AppCStoreRoutes":               k.AppCStoreRoutes.Load(),
 	}
 }

ipn/ipnlocal/local.go

@@ -3516,8 +3516,22 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 		return
 	}
 
-	if b.appConnector == nil {
-		b.appConnector = appc.NewAppConnector(b.logf, b)
+	shouldAppCStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load()
+	if b.appConnector == nil || b.appConnector.ShouldStoreRoutes() != shouldAppCStoreRoutes {
+		var ri *appc.RouteInfo
+		var storeFunc func(*appc.RouteInfo) error
+		if shouldAppCStoreRoutes {
+			var err error
+			ri, err = b.readRouteInfoLocked()
+			if err != nil {
+				ri = &appc.RouteInfo{}
+				if err != ipn.ErrStateNotExist {
+					b.logf("Unsuccessful Read RouteInfo: ", err)
+				}
+			}
+			storeFunc = b.storeRouteInfo
+		}
+		b.appConnector = appc.NewAppConnector(b.logf, b, ri, storeFunc)
 	}
 	if nm == nil {
 		return

ipn/ipnlocal/local_test.go

@@ -55,6 +55,8 @@ import (
 	"tailscale.com/wgengine/wgcfg"
 )
 
+func fakeStoreRoutes(*appc.RouteInfo) error { return nil }
+
 func inRemove(ip netip.Addr) bool {
 	for _, pfx := range removeFromDefaultRoute {
 		if pfx.Contains(ip) {
@@ -1291,15 +1293,21 @@ func TestDNSConfigForNetmapForExitNodeConfigs(t *testing.T) {
 }
 
 func TestOfferingAppConnector(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		b := newTestBackend(t)
 		if b.OfferingAppConnector() {
 			t.Fatal("unexpected offering app connector")
 		}
-	b.appConnector = appc.NewAppConnector(t.Logf, nil)
+		if shouldStore {
+			b.appConnector = appc.NewAppConnector(t.Logf, nil, &appc.RouteInfo{}, fakeStoreRoutes)
+		} else {
+			b.appConnector = appc.NewAppConnector(t.Logf, nil, nil, nil)
+		}
 		if !b.OfferingAppConnector() {
 			t.Fatal("unexpected not offering app connector")
 		}
 	}
+}
 
 func TestRouteAdvertiser(t *testing.T) {
 	b := newTestBackend(t)
@@ -1342,13 +1350,18 @@ func TestRouterAdvertiserIgnoresContainedRoutes(t *testing.T) {
 }
 
 func TestObserveDNSResponse(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		b := newTestBackend(t)
 
 		// ensure no error when no app connector is configured
 		b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 
 		rc := &appctest.RouteCollector{}
-	b.appConnector = appc.NewAppConnector(t.Logf, rc)
+		if shouldStore {
+			b.appConnector = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes)
+		} else {
+			b.appConnector = appc.NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		b.appConnector.UpdateDomains([]string{"example.com"})
 		b.appConnector.Wait(context.Background())
 
@@ -1359,6 +1372,7 @@ func TestObserveDNSResponse(t *testing.T) {
 			t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes)
 		}
 	}
+}
 
 func TestCoveredRouteRangeNoDefault(t *testing.T) {
 	tests := []struct {

ipn/ipnlocal/peerapi_test.go

@@ -687,18 +687,25 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) {
 }
 
 func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
 		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		var a *appc.AppConnector
+		if shouldStore {
+			a = appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}, &appc.RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}, nil, nil)
+		}
 		h.ps = &peerAPIServer{
 			b: &LocalBackend{
 				e:     eng,
 				pm:    pm,
 				store: pm.Store(),
 				// configure as an app connector just to enable the API.
-			appConnector: appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}),
+				appConnector: a,
 			},
 		}
 
@@ -747,8 +754,10 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
 			netip.MustParseAddr(addr)
 		}
 	}
+}
 
 func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
@@ -756,12 +765,18 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 		rc := &appctest.RouteCollector{}
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
 		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		var a *appc.AppConnector
+		if shouldStore {
+			a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = appc.NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		h.ps = &peerAPIServer{
 			b: &LocalBackend{
 				e:            eng,
 				pm:           pm,
 				store:        pm.Store(),
-			appConnector: appc.NewAppConnector(t.Logf, rc),
+				appConnector: a,
 			},
 		}
 		h.ps.b.appConnector.UpdateDomains([]string{"example.com"})
@@ -802,8 +817,10 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 			t.Errorf("got %v; want %v", rc.Routes(), wantRoutes)
 		}
 	}
+}
 
 func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
+	for _, shouldStore := range []bool{false, true} {
 		ctx := context.Background()
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
@@ -811,12 +828,18 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
 		rc := &appctest.RouteCollector{}
 		eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
 		pm := must.Get(newProfileManager(new(mem.Store), t.Logf))
+		var a *appc.AppConnector
+		if shouldStore {
+			a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes)
+		} else {
+			a = appc.NewAppConnector(t.Logf, rc, nil, nil)
+		}
 		h.ps = &peerAPIServer{
 			b: &LocalBackend{
 				e:            eng,
 				pm:           pm,
 				store:        pm.Store(),
-			appConnector: appc.NewAppConnector(t.Logf, rc),
+				appConnector: a,
 			},
 		}
 		h.ps.b.appConnector.UpdateDomains([]string{"www.example.com"})
@@ -868,6 +891,7 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
 			t.Errorf("got %v; want %v", rc.Routes(), wantRoutes)
 		}
 	}
+}
 
 type fakeResolver struct {
 	build func(*dnsmessage.Builder)

tailcfg/tailcfg.go

@@ -2253,6 +2253,9 @@ const (
 
 	// NodeAttrAutoExitNode permits the automatic exit nodes feature.
 	NodeAttrAutoExitNode NodeCapability = "auto-exit-node"
+
+	// NodeAttrStoreAppCRoutes configures the node to store app connector routes persistently.
+	NodeAttrStoreAppCRoutes NodeCapability = "store-appc-routes"
 )
 
 // SetDNSRequest is a request to add a DNS record.