From 1bd1b387b298a09ae49d7084e644e0f4ff0cb4c2 Mon Sep 17 00:00:00 2001 From: Fran Bull Date: Thu, 11 Apr 2024 10:12:13 -0700 Subject: [PATCH] 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 --- appc/appconnector.go | 20 +- appc/appconnector_test.go | 326 +++++++++++++++----------- control/controlknobs/controlknobs.go | 7 + ipn/ipnlocal/local.go | 18 +- ipn/ipnlocal/local_test.go | 52 +++-- ipn/ipnlocal/peerapi_test.go | 338 ++++++++++++++------------- tailcfg/tailcfg.go | 3 + 7 files changed, 443 insertions(+), 321 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 4df696b0d..6d06656f9 100644 --- a/appc/appconnector.go +++ b/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 diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index 5b739b097..c95683335 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -17,194 +17,238 @@ "tailscale.com/util/must" ) +func fakeStoreRoutes(*RouteInfo) error { return nil } + func TestUpdateDomains(t *testing.T) { - ctx := context.Background() - a := NewAppConnector(t.Logf, nil) - a.UpdateDomains([]string{"example.com"}) + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + 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) - if got, want := a.Domains().AsSlice(), []string{"example.com"}; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + a.Wait(ctx) + if got, want := a.Domains().AsSlice(), []string{"example.com"}; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - addr := netip.MustParseAddr("192.0.0.8") - a.domains["example.com"] = append(a.domains["example.com"], addr) - a.UpdateDomains([]string{"example.com"}) - a.Wait(ctx) + addr := netip.MustParseAddr("192.0.0.8") + a.domains["example.com"] = append(a.domains["example.com"], addr) + a.UpdateDomains([]string{"example.com"}) + a.Wait(ctx) - if got, want := a.domains["example.com"], []netip.Addr{addr}; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + if got, want := a.domains["example.com"], []netip.Addr{addr}; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - // domains are explicitly downcased on set. - a.UpdateDomains([]string{"UP.EXAMPLE.COM"}) - a.Wait(ctx) - if got, want := xmaps.Keys(a.domains), []string{"up.example.com"}; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) + // domains are explicitly downcased on set. + a.UpdateDomains([]string{"UP.EXAMPLE.COM"}) + a.Wait(ctx) + if got, want := xmaps.Keys(a.domains), []string{"up.example.com"}; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } } } func TestUpdateRoutes(t *testing.T) { - ctx := context.Background() - rc := &appctest.RouteCollector{} - a := NewAppConnector(t.Logf, rc) - a.updateDomains([]string{"*.example.com"}) + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + 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 - a.ObserveDNSResponse(dnsResponse("a.example.com.", "192.0.2.1")) - a.Wait(ctx) + // This route should be collapsed into the range + a.ObserveDNSResponse(dnsResponse("a.example.com.", "192.0.2.1")) + a.Wait(ctx) - if !slices.Equal(rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) { - t.Fatalf("got %v, want %v", rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) - } + if !slices.Equal(rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) { + t.Fatalf("got %v, want %v", rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")}) + } - // This route should not be collapsed or removed - a.ObserveDNSResponse(dnsResponse("b.example.com.", "192.0.0.1")) - a.Wait(ctx) + // This route should not be collapsed or removed + a.ObserveDNSResponse(dnsResponse("b.example.com.", "192.0.0.1")) + a.Wait(ctx) - routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24"), netip.MustParsePrefix("192.0.0.1/32")} - a.updateRoutes(routes) + routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24"), netip.MustParsePrefix("192.0.0.1/32")} + a.updateRoutes(routes) - slices.SortFunc(rc.Routes(), prefixCompare) - rc.SetRoutes(slices.Compact(rc.Routes())) - slices.SortFunc(routes, prefixCompare) + slices.SortFunc(rc.Routes(), prefixCompare) + rc.SetRoutes(slices.Compact(rc.Routes())) + slices.SortFunc(routes, prefixCompare) - // Ensure that the non-matching /32 is preserved, even though it's in the domains table. - if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { - t.Errorf("added routes: got %v, want %v", rc.Routes(), routes) - } + // Ensure that the non-matching /32 is preserved, even though it's in the domains table. + if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { + t.Errorf("added routes: got %v, want %v", rc.Routes(), routes) + } - // Ensure that the contained /32 is removed, replaced by the /24. - wantRemoved := []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")} - if !slices.EqualFunc(rc.RemovedRoutes(), wantRemoved, prefixEqual) { - t.Fatalf("unexpected removed routes: %v", rc.RemovedRoutes()) + // Ensure that the contained /32 is removed, replaced by the /24. + wantRemoved := []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")} + if !slices.EqualFunc(rc.RemovedRoutes(), wantRemoved, prefixEqual) { + t.Fatalf("unexpected removed routes: %v", rc.RemovedRoutes()) + } } } func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { - rc := &appctest.RouteCollector{} - a := NewAppConnector(t.Logf, rc) - 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")} - a.updateRoutes(routes) + for _, shouldStore := range []bool{false, true} { + rc := &appctest.RouteCollector{} + 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")} + a.updateRoutes(routes) - if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { - t.Fatalf("got %v, want %v", rc.Routes(), routes) + if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { + t.Fatalf("got %v, want %v", rc.Routes(), routes) + } } } func TestDomainRoutes(t *testing.T) { - rc := &appctest.RouteCollector{} - a := NewAppConnector(t.Logf, rc) - a.updateDomains([]string{"example.com"}) - a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - a.Wait(context.Background()) + for _, shouldStore := range []bool{false, true} { + rc := &appctest.RouteCollector{} + 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()) - want := map[string][]netip.Addr{ - "example.com": {netip.MustParseAddr("192.0.0.8")}, - } + want := map[string][]netip.Addr{ + "example.com": {netip.MustParseAddr("192.0.0.8")}, + } - if got := a.DomainRoutes(); !reflect.DeepEqual(got, want) { - t.Fatalf("DomainRoutes: got %v, want %v", got, want) + if got := a.DomainRoutes(); !reflect.DeepEqual(got, want) { + t.Fatalf("DomainRoutes: got %v, want %v", got, want) + } } } func TestObserveDNSResponse(t *testing.T) { - ctx := context.Background() - rc := &appctest.RouteCollector{} - a := NewAppConnector(t.Logf, rc) + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + 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")) - if got, want := rc.Routes(), ([]netip.Prefix)(nil); !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + // a has no domains configured, so it should not advertise any routes + a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) + if got, want := rc.Routes(), ([]netip.Prefix)(nil); !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} + wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} - a.updateDomains([]string{"example.com"}) - a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - a.Wait(ctx) - if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + a.updateDomains([]string{"example.com"}) + a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) + a.Wait(ctx) + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - // a CNAME record chain should result in a route being added if the chain - // matches a routed domain. - a.updateDomains([]string{"www.example.com", "example.com"}) - a.ObserveDNSResponse(dnsCNAMEResponse("192.0.0.9", "www.example.com.", "chain.example.com.", "example.com.")) - a.Wait(ctx) - wantRoutes = append(wantRoutes, netip.MustParsePrefix("192.0.0.9/32")) - if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + // a CNAME record chain should result in a route being added if the chain + // matches a routed domain. + a.updateDomains([]string{"www.example.com", "example.com"}) + a.ObserveDNSResponse(dnsCNAMEResponse("192.0.0.9", "www.example.com.", "chain.example.com.", "example.com.")) + a.Wait(ctx) + wantRoutes = append(wantRoutes, netip.MustParsePrefix("192.0.0.9/32")) + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - // a CNAME record chain should result in a route being added if the chain - // even if only found in the middle of the chain - a.ObserveDNSResponse(dnsCNAMEResponse("192.0.0.10", "outside.example.org.", "www.example.com.", "example.org.")) - a.Wait(ctx) - wantRoutes = append(wantRoutes, netip.MustParsePrefix("192.0.0.10/32")) - if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + // a CNAME record chain should result in a route being added if the chain + // even if only found in the middle of the chain + a.ObserveDNSResponse(dnsCNAMEResponse("192.0.0.10", "outside.example.org.", "www.example.com.", "example.org.")) + a.Wait(ctx) + wantRoutes = append(wantRoutes, netip.MustParsePrefix("192.0.0.10/32")) + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - wantRoutes = append(wantRoutes, netip.MustParsePrefix("2001:db8::1/128")) + wantRoutes = append(wantRoutes, netip.MustParsePrefix("2001:db8::1/128")) - a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) - a.Wait(ctx) - if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { - t.Errorf("got %v; want %v", got, want) - } + a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) + a.Wait(ctx) + if got, want := rc.Routes(), wantRoutes; !slices.Equal(got, want) { + t.Errorf("got %v; want %v", got, want) + } - // don't re-advertise routes that have already been advertised - a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) - a.Wait(ctx) - if !slices.Equal(rc.Routes(), wantRoutes) { - t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) - } + // don't re-advertise routes that have already been advertised + a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) + a.Wait(ctx) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) + } - // don't advertise addresses that are already in a control provided route - pfx := netip.MustParsePrefix("192.0.2.0/24") - a.updateRoutes([]netip.Prefix{pfx}) - wantRoutes = append(wantRoutes, pfx) - a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.2.1")) - a.Wait(ctx) - if !slices.Equal(rc.Routes(), wantRoutes) { - t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) - } - if !slices.Contains(a.domains["example.com"], netip.MustParseAddr("192.0.2.1")) { - t.Errorf("missing %v from %v", "192.0.2.1", a.domains["exmaple.com"]) + // don't advertise addresses that are already in a control provided route + pfx := netip.MustParsePrefix("192.0.2.0/24") + a.updateRoutes([]netip.Prefix{pfx}) + wantRoutes = append(wantRoutes, pfx) + a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.2.1")) + a.Wait(ctx) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("rc.Routes(): got %v; want %v", rc.Routes(), wantRoutes) + } + if !slices.Contains(a.domains["example.com"], netip.MustParseAddr("192.0.2.1")) { + t.Errorf("missing %v from %v", "192.0.2.1", a.domains["exmaple.com"]) + } } } func TestWildcardDomains(t *testing.T) { - ctx := context.Background() - rc := &appctest.RouteCollector{} - a := NewAppConnector(t.Logf, rc) + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + 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")) - a.Wait(ctx) - if got, want := rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}; !slices.Equal(got, want) { - t.Errorf("routes: got %v; want %v", got, want) - } - if got, want := a.wildcards, []string{"example.com"}; !slices.Equal(got, want) { - t.Errorf("wildcards: got %v; want %v", got, want) - } + a.updateDomains([]string{"*.example.com"}) + a.ObserveDNSResponse(dnsResponse("foo.example.com.", "192.0.0.8")) + a.Wait(ctx) + if got, want := rc.Routes(), []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}; !slices.Equal(got, want) { + t.Errorf("routes: got %v; want %v", got, want) + } + if got, want := a.wildcards, []string{"example.com"}; !slices.Equal(got, want) { + t.Errorf("wildcards: got %v; want %v", got, want) + } - a.updateDomains([]string{"*.example.com", "example.com"}) - if _, ok := a.domains["foo.example.com"]; !ok { - t.Errorf("expected foo.example.com to be preserved in domains due to wildcard") - } - if got, want := a.wildcards, []string{"example.com"}; !slices.Equal(got, want) { - t.Errorf("wildcards: got %v; want %v", got, want) - } + a.updateDomains([]string{"*.example.com", "example.com"}) + if _, ok := a.domains["foo.example.com"]; !ok { + t.Errorf("expected foo.example.com to be preserved in domains due to wildcard") + } + if got, want := a.wildcards, []string{"example.com"}; !slices.Equal(got, want) { + t.Errorf("wildcards: got %v; want %v", got, want) + } - // There was an early regression where the wildcard domain was added repeatedly, this guards against that. - a.updateDomains([]string{"*.example.com", "example.com"}) - if len(a.wildcards) != 1 { - t.Errorf("expected only one wildcard domain, got %v", a.wildcards) + // There was an early regression where the wildcard domain was added repeatedly, this guards against that. + a.updateDomains([]string{"*.example.com", "example.com"}) + if len(a.wildcards) != 1 { + t.Errorf("expected only one wildcard domain, got %v", a.wildcards) + } } } diff --git a/control/controlknobs/controlknobs.go b/control/controlknobs/controlknobs.go index 6e3a62967..2f80ba38c 100644 --- a/control/controlknobs/controlknobs.go +++ b/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(), } } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 73453603d..c3e92cdad 100644 --- a/ipn/ipnlocal/local.go +++ b/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 diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index acb1c3f0a..61efc2fb0 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -55,6 +55,8 @@ "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,13 +1293,19 @@ type tc struct { } func TestOfferingAppConnector(t *testing.T) { - b := newTestBackend(t) - if b.OfferingAppConnector() { - t.Fatal("unexpected offering app connector") - } - b.appConnector = appc.NewAppConnector(t.Logf, nil) - if !b.OfferingAppConnector() { - t.Fatal("unexpected not offering app connector") + for _, shouldStore := range []bool{false, true} { + b := newTestBackend(t) + if b.OfferingAppConnector() { + t.Fatal("unexpected offering app connector") + } + 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") + } } } @@ -1342,21 +1350,27 @@ func TestRouterAdvertiserIgnoresContainedRoutes(t *testing.T) { } func TestObserveDNSResponse(t *testing.T) { - b := newTestBackend(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")) + // 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.UpdateDomains([]string{"example.com"}) - b.appConnector.Wait(context.Background()) + rc := &appctest.RouteCollector{} + 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()) - b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - b.appConnector.Wait(context.Background()) - wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} - if !slices.Equal(rc.Routes(), wantRoutes) { - t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes) + b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) + b.appConnector.Wait(context.Background()) + wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes) + } } } diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index bffbb1076..1e63a396e 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -687,185 +687,209 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) { } func TestPeerAPIPrettyReplyCNAME(t *testing.T) { - var h peerAPIHandler - h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") + 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)) - 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{}), - }, - } + 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: a, + }, + } - h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { - b.CNAMEResource( - dnsmessage.ResourceHeader{ - Name: dnsmessage.MustNewName("www.example.com."), - Type: dnsmessage.TypeCNAME, - Class: dnsmessage.ClassINET, - TTL: 0, - }, - dnsmessage.CNAMEResource{ - CNAME: dnsmessage.MustNewName("example.com."), - }, - ) - b.AResource( - dnsmessage.ResourceHeader{ - Name: dnsmessage.MustNewName("example.com."), - Type: dnsmessage.TypeA, - Class: dnsmessage.ClassINET, - TTL: 0, - }, - dnsmessage.AResource{ - A: [4]byte{192, 0, 0, 8}, - }, - ) - }} - f := filter.NewAllowAllForTest(logger.Discard) - h.ps.b.setFilter(f) + h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { + b.CNAMEResource( + dnsmessage.ResourceHeader{ + Name: dnsmessage.MustNewName("www.example.com."), + Type: dnsmessage.TypeCNAME, + Class: dnsmessage.ClassINET, + TTL: 0, + }, + dnsmessage.CNAMEResource{ + CNAME: dnsmessage.MustNewName("example.com."), + }, + ) + b.AResource( + dnsmessage.ResourceHeader{ + Name: dnsmessage.MustNewName("example.com."), + Type: dnsmessage.TypeA, + Class: dnsmessage.ClassINET, + TTL: 0, + }, + dnsmessage.AResource{ + A: [4]byte{192, 0, 0, 8}, + }, + ) + }} + f := filter.NewAllowAllForTest(logger.Discard) + h.ps.b.setFilter(f) - if !h.replyToDNSQueries() { - t.Errorf("unexpectedly deny; wanted to be a DNS server") - } + if !h.replyToDNSQueries() { + t.Errorf("unexpectedly deny; wanted to be a DNS server") + } - w := httptest.NewRecorder() - h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=www.example.com.", nil)) - if w.Code != http.StatusOK { - t.Errorf("unexpected status code: %v", w.Code) - } - var addrs []string - json.NewDecoder(w.Body).Decode(&addrs) - if len(addrs) == 0 { - t.Fatalf("no addresses returned") - } - for _, addr := range addrs { - netip.MustParseAddr(addr) + w := httptest.NewRecorder() + h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=www.example.com.", nil)) + if w.Code != http.StatusOK { + t.Errorf("unexpected status code: %v", w.Code) + } + var addrs []string + json.NewDecoder(w.Body).Decode(&addrs) + if len(addrs) == 0 { + t.Fatalf("no addresses returned") + } + for _, addr := range addrs { + netip.MustParseAddr(addr) + } } } func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { - ctx := context.Background() - var h peerAPIHandler - h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + var h peerAPIHandler + h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") - rc := &appctest.RouteCollector{} - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) - h.ps = &peerAPIServer{ - b: &LocalBackend{ - e: eng, - pm: pm, - store: pm.Store(), - appConnector: appc.NewAppConnector(t.Logf, rc), - }, - } - h.ps.b.appConnector.UpdateDomains([]string{"example.com"}) - h.ps.b.appConnector.Wait(ctx) - - h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { - b.AResource( - dnsmessage.ResourceHeader{ - Name: dnsmessage.MustNewName("example.com."), - Type: dnsmessage.TypeA, - Class: dnsmessage.ClassINET, - TTL: 0, + 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: a, }, - dnsmessage.AResource{ - A: [4]byte{192, 0, 0, 8}, - }, - ) - }} - f := filter.NewAllowAllForTest(logger.Discard) - h.ps.b.setFilter(f) + } + h.ps.b.appConnector.UpdateDomains([]string{"example.com"}) + h.ps.b.appConnector.Wait(ctx) - if !h.ps.b.OfferingAppConnector() { - t.Fatal("expecting to be offering app connector") - } - if !h.replyToDNSQueries() { - t.Errorf("unexpectedly deny; wanted to be a DNS server") - } + h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { + b.AResource( + dnsmessage.ResourceHeader{ + Name: dnsmessage.MustNewName("example.com."), + Type: dnsmessage.TypeA, + Class: dnsmessage.ClassINET, + TTL: 0, + }, + dnsmessage.AResource{ + A: [4]byte{192, 0, 0, 8}, + }, + ) + }} + f := filter.NewAllowAllForTest(logger.Discard) + h.ps.b.setFilter(f) - w := httptest.NewRecorder() - h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=example.com.", nil)) - if w.Code != http.StatusOK { - t.Errorf("unexpected status code: %v", w.Code) - } - h.ps.b.appConnector.Wait(ctx) + if !h.ps.b.OfferingAppConnector() { + t.Fatal("expecting to be offering app connector") + } + if !h.replyToDNSQueries() { + t.Errorf("unexpectedly deny; wanted to be a DNS server") + } - wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} - if !slices.Equal(rc.Routes(), wantRoutes) { - t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) + w := httptest.NewRecorder() + h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=example.com.", nil)) + if w.Code != http.StatusOK { + t.Errorf("unexpected status code: %v", w.Code) + } + h.ps.b.appConnector.Wait(ctx) + + wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) + } } } func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) { - ctx := context.Background() - var h peerAPIHandler - h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + var h peerAPIHandler + h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") - rc := &appctest.RouteCollector{} - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) - h.ps = &peerAPIServer{ - b: &LocalBackend{ - e: eng, - pm: pm, - store: pm.Store(), - appConnector: appc.NewAppConnector(t.Logf, rc), - }, - } - h.ps.b.appConnector.UpdateDomains([]string{"www.example.com"}) - h.ps.b.appConnector.Wait(ctx) - - h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { - b.CNAMEResource( - dnsmessage.ResourceHeader{ - Name: dnsmessage.MustNewName("www.example.com."), - Type: dnsmessage.TypeCNAME, - Class: dnsmessage.ClassINET, - TTL: 0, + 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: a, }, - dnsmessage.CNAMEResource{ - CNAME: dnsmessage.MustNewName("example.com."), - }, - ) - b.AResource( - dnsmessage.ResourceHeader{ - Name: dnsmessage.MustNewName("example.com."), - Type: dnsmessage.TypeA, - Class: dnsmessage.ClassINET, - TTL: 0, - }, - dnsmessage.AResource{ - A: [4]byte{192, 0, 0, 8}, - }, - ) - }} - f := filter.NewAllowAllForTest(logger.Discard) - h.ps.b.setFilter(f) + } + h.ps.b.appConnector.UpdateDomains([]string{"www.example.com"}) + h.ps.b.appConnector.Wait(ctx) - if !h.ps.b.OfferingAppConnector() { - t.Fatal("expecting to be offering app connector") - } - if !h.replyToDNSQueries() { - t.Errorf("unexpectedly deny; wanted to be a DNS server") - } + h.ps.resolver = &fakeResolver{build: func(b *dnsmessage.Builder) { + b.CNAMEResource( + dnsmessage.ResourceHeader{ + Name: dnsmessage.MustNewName("www.example.com."), + Type: dnsmessage.TypeCNAME, + Class: dnsmessage.ClassINET, + TTL: 0, + }, + dnsmessage.CNAMEResource{ + CNAME: dnsmessage.MustNewName("example.com."), + }, + ) + b.AResource( + dnsmessage.ResourceHeader{ + Name: dnsmessage.MustNewName("example.com."), + Type: dnsmessage.TypeA, + Class: dnsmessage.ClassINET, + TTL: 0, + }, + dnsmessage.AResource{ + A: [4]byte{192, 0, 0, 8}, + }, + ) + }} + f := filter.NewAllowAllForTest(logger.Discard) + h.ps.b.setFilter(f) - w := httptest.NewRecorder() - h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=www.example.com.", nil)) - if w.Code != http.StatusOK { - t.Errorf("unexpected status code: %v", w.Code) - } - h.ps.b.appConnector.Wait(ctx) + if !h.ps.b.OfferingAppConnector() { + t.Fatal("expecting to be offering app connector") + } + if !h.replyToDNSQueries() { + t.Errorf("unexpectedly deny; wanted to be a DNS server") + } - wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} - if !slices.Equal(rc.Routes(), wantRoutes) { - t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) + w := httptest.NewRecorder() + h.handleDNSQuery(w, httptest.NewRequest("GET", "/dns-query?q=www.example.com.", nil)) + if w.Code != http.StatusOK { + t.Errorf("unexpected status code: %v", w.Code) + } + h.ps.b.appConnector.Wait(ctx) + + wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) + } } } diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 39ef0d7f6..5308a04ed 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -2253,6 +2253,9 @@ type Oauth2Token struct { // 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.