Add a control knob to toggle writing RouteInfo to StateStore

To control the toggle in dev you can a) add a go workspace so that
control is using the new tailcfg from this commit and b) add the feature
flag to control.

appc/appconnector.go

@@ -69,13 +69,21 @@ type AppConnector struct {
 
 	// queue provides ordering for update operations
 	queue execqueue.ExecQueue
+
+	// whether this tailscaled should persist routes. Storing RouteInfo enables the app connector
+	// to forget routes when appropriate and should make routes smaller. While we are verifying that
+	// writing the RouteInfo to StateStore is a good solution (and doesn't for example cause writes
+	// that are too frequent or too large) use a controlknob to manage this flag.
+	ShouldStoreRoutes bool
 }
 
 // NewAppConnector creates a new AppConnector.
-func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser) *AppConnector {
+func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser, shouldStoreRoutes bool) *AppConnector {
+	// TODO(fran) if !shouldStoreRoutes we probably want to try and clean up any stored routes
 	return &AppConnector{
 		logf:              logger.WithPrefix(logf, "appc: "),
 		routeAdvertiser:   routeAdvertiser,
+		ShouldStoreRoutes: shouldStoreRoutes,
 	}
 }
 

appc/appconnector_test.go

@@ -18,8 +18,9 @@ import (
 )
 
 func TestUpdateDomains(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		ctx := context.Background()
-	a := NewAppConnector(t.Logf, nil)
+		a := NewAppConnector(t.Logf, nil, shouldStore)
 		a.UpdateDomains([]string{"example.com"})
 
 		a.Wait(ctx)
@@ -43,11 +44,13 @@ func TestUpdateDomains(t *testing.T) {
 			t.Errorf("got %v; want %v", got, want)
 		}
 	}
+}
 
 func TestUpdateRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		a := NewAppConnector(t.Logf, rc, shouldStore)
 		a.updateDomains([]string{"*.example.com"})
 
 		// This route should be collapsed into the range
@@ -80,10 +83,12 @@ func TestUpdateRoutes(t *testing.T) {
 			t.Fatalf("unexpected removed routes: %v", rc.RemovedRoutes())
 		}
 	}
+}
 
 func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		a := NewAppConnector(t.Logf, rc, shouldStore)
 		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 +98,12 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) {
 			t.Fatalf("got %v, want %v", rc.Routes(), routes)
 		}
 	}
+}
 
 func TestDomainRoutes(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		a := NewAppConnector(t.Logf, rc, shouldStore)
 		a.updateDomains([]string{"example.com"})
 		a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
 		a.Wait(context.Background())
@@ -109,11 +116,13 @@ func TestDomainRoutes(t *testing.T) {
 			t.Fatalf("DomainRoutes: got %v, want %v", got, want)
 		}
 	}
+}
 
 func TestObserveDNSResponse(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		a := NewAppConnector(t.Logf, rc, shouldStore)
 
 		// a has no domains configured, so it should not advertise any routes
 		a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8"))
@@ -177,11 +186,13 @@ 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{true, false} {
 		ctx := context.Background()
 		rc := &appctest.RouteCollector{}
-	a := NewAppConnector(t.Logf, rc)
+		a := NewAppConnector(t.Logf, rc, shouldStore)
 
 		a.updateDomains([]string{"*.example.com"})
 		a.ObserveDNSResponse(dnsResponse("foo.example.com.", "192.0.0.8"))
@@ -207,6 +218,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

@@ -3561,8 +3561,14 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 		return
 	}
 
-	if b.appConnector == nil {
-		b.appConnector = appc.NewAppConnector(b.logf, b)
+	shouldAppCStoreRoutesHasChanged := false
+	shouldAppCStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load()
+	if b.appConnector != nil {
+		shouldAppCStoreRoutesHasChanged = b.appConnector.ShouldStoreRoutes != shouldAppCStoreRoutes
+	}
+
+	if b.appConnector == nil || shouldAppCStoreRoutesHasChanged {
+		b.appConnector = appc.NewAppConnector(b.logf, b, shouldAppCStoreRoutes)
 	}
 	if nm == nil {
 		return

ipn/ipnlocal/local_test.go

@@ -1253,15 +1253,17 @@ func TestDNSConfigForNetmapForExitNodeConfigs(t *testing.T) {
 }
 
 func TestOfferingAppConnector(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		b := newTestBackend(t)
 		if b.OfferingAppConnector() {
 			t.Fatal("unexpected offering app connector")
 		}
-	b.appConnector = appc.NewAppConnector(t.Logf, nil)
+		b.appConnector = appc.NewAppConnector(t.Logf, nil, shouldStore)
 		if !b.OfferingAppConnector() {
 			t.Fatal("unexpected not offering app connector")
 		}
 	}
+}
 
 func TestRouteAdvertiser(t *testing.T) {
 	b := newTestBackend(t)
@@ -1304,13 +1306,14 @@ func TestRouterAdvertiserIgnoresContainedRoutes(t *testing.T) {
 }
 
 func TestObserveDNSResponse(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		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)
+		b.appConnector = appc.NewAppConnector(t.Logf, rc, shouldStore)
 		b.appConnector.UpdateDomains([]string{"example.com"})
 		b.appConnector.Wait(context.Background())
 
@@ -1321,6 +1324,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,6 +687,7 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) {
 }
 
 func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
 
@@ -698,7 +699,7 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
 				pm:    pm,
 				store: pm.Store(),
 				// configure as an app connector just to enable the API.
-			appConnector: appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}),
+				appConnector: appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}, shouldStore),
 			},
 		}
 
@@ -747,8 +748,10 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) {
 			netip.MustParseAddr(addr)
 		}
 	}
+}
 
 func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		ctx := context.Background()
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
@@ -761,7 +764,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 				e:            eng,
 				pm:           pm,
 				store:        pm.Store(),
-			appConnector: appc.NewAppConnector(t.Logf, rc),
+				appConnector: appc.NewAppConnector(t.Logf, rc, shouldStore),
 			},
 		}
 		h.ps.b.appConnector.UpdateDomains([]string{"example.com"})
@@ -802,8 +805,10 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) {
 			t.Errorf("got %v; want %v", rc.Routes(), wantRoutes)
 		}
 	}
+}
 
 func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
+	for _, shouldStore := range []bool{true, false} {
 		ctx := context.Background()
 		var h peerAPIHandler
 		h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345")
@@ -816,7 +821,7 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) {
 				e:            eng,
 				pm:           pm,
 				store:        pm.Store(),
-			appConnector: appc.NewAppConnector(t.Logf, rc),
+				appConnector: appc.NewAppConnector(t.Logf, rc, shouldStore),
 			},
 		}
 		h.ps.b.appConnector.UpdateDomains([]string{"www.example.com"})
@@ -868,6 +873,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

@@ -2232,8 +2232,13 @@ const (
 	// NodeAttrDisableWebClient disables using the web client.
 	NodeAttrDisableWebClient NodeCapability = "disable-web-client"
 
+<<<<<<< HEAD
 	// NodeAttrExitDstNetworkFlowLog enables exit node destinations in network flow logs.
 	NodeAttrExitDstNetworkFlowLog NodeCapability = "exit-dst-network-flow-log"
+=======
+	// NodeAttrStoreAppCRoutes enables storing app connector routes persistently.
+	NodeAttrStoreAppCRoutes NodeCapability = "store-appc-routes"
+>>>>>>> 61f7b83bd (Add a control knob to toggle writing RouteInfo to StateStore)
 )
 
 // SetDNSRequest is a request to add a DNS record.