From c27dc1ca3106216869a3b251ec53c70dc7dce9cf Mon Sep 17 00:00:00 2001 From: Fran Bull Date: Mon, 15 Apr 2024 10:16:02 -0700 Subject: [PATCH] appc: unadvertise routes when reconfiguring app connector If the controlknob to persist app connector routes is enabled, when reconfiguring an app connector unadvertise routes that are no longer relevant. Updates #11008 Signed-off-by: Fran Bull --- appc/appconnector.go | 37 +++++++++ appc/appconnector_test.go | 170 +++++++++++++++++++++++++++++++++++++- 2 files changed, 205 insertions(+), 2 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 4d0c5d00b..63e1946e7 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -23,6 +23,7 @@ import ( "tailscale.com/util/dnsname" "tailscale.com/util/execqueue" "tailscale.com/util/mak" + "tailscale.com/util/slicesx" ) // RouteAdvertiser is an interface that allows the AppConnector to advertise @@ -166,10 +167,26 @@ func (e *AppConnector) updateDomains(domains []string) { for _, wc := range e.wildcards { if dnsname.HasSuffix(d, wc) { e.domains[d] = addrs + delete(oldDomains, d) break } } } + + // Everything left in oldDomains is a domain we're no longer tracking + // and if we are storing route info we can unadvertise the routes + if e.ShouldStoreRoutes() { + toRemove := []netip.Prefix{} + for _, addrs := range oldDomains { + for _, a := range addrs { + toRemove = append(toRemove, netip.PrefixFrom(a, a.BitLen())) + } + } + if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { + e.logf("failed to unadvertise routes on domain removal: %v: %v: %v", xmaps.Keys(oldDomains), toRemove, err) + } + } + e.logf("handling domains: %v and wildcards: %v", xmaps.Keys(e.domains), e.wildcards) } @@ -193,6 +210,14 @@ func (e *AppConnector) updateRoutes(routes []netip.Prefix) { var toRemove []netip.Prefix + // If we're storing routes and know e.controlRoutes is a good + // representation of what should be in AdvertisedRoutes we can stop + // advertising routes that used to be in e.controlRoutes but are not + // in routes. + if e.ShouldStoreRoutes() { + toRemove = routesWithout(e.controlRoutes, routes) + } + nextRoute: for _, r := range routes { for _, addr := range e.domains { @@ -447,3 +472,15 @@ func (e *AppConnector) addDomainAddrLocked(domain string, addr netip.Addr) { func compareAddr(l, r netip.Addr) int { return l.Compare(r) } + +// routesWithout returns a without b where a and b +// are unsorted slices of netip.Prefix +func routesWithout(a, b []netip.Prefix) []netip.Prefix { + m := make(map[netip.Prefix]bool, len(b)) + for _, p := range b { + m[p] = true + } + return slicesx.Filter(make([]netip.Prefix, 0, len(a)), a, func(p netip.Prefix) bool { + return !m[p] + }) +} diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index c95683335..c987f7388 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -24,9 +24,9 @@ func TestUpdateDomains(t *testing.T) { ctx := context.Background() var a *AppConnector if shouldStore { - a = NewAppConnector(t.Logf, nil, &RouteInfo{}, fakeStoreRoutes) + a = NewAppConnector(t.Logf, &appctest.RouteCollector{}, &RouteInfo{}, fakeStoreRoutes) } else { - a = NewAppConnector(t.Logf, nil, nil, nil) + a = NewAppConnector(t.Logf, &appctest.RouteCollector{}, nil, nil) } a.UpdateDomains([]string{"example.com"}) @@ -354,3 +354,169 @@ func prefixCompare(a, b netip.Prefix) int { } return a.Addr().Compare(b.Addr()) } + +func prefixes(in ...string) []netip.Prefix { + toRet := make([]netip.Prefix, len(in)) + for i, s := range in { + toRet[i] = netip.MustParsePrefix(s) + } + return toRet +} + +func TestUpdateRouteRouteRemoval(t *testing.T) { + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + + assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) { + if !slices.Equal(routes, rc.Routes()) { + t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes()) + } + if !slices.Equal(removedRoutes, rc.RemovedRoutes()) { + t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes()) + } + } + + var a *AppConnector + if shouldStore { + a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes) + } else { + a = NewAppConnector(t.Logf, rc, nil, nil) + } + // nothing has yet been advertised + assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{}) + + a.UpdateDomainsAndRoutes([]string{}, prefixes("1.2.3.1/32", "1.2.3.2/32")) + a.Wait(ctx) + // the routes passed to UpdateDomainsAndRoutes have been advertised + assertRoutes("simple update", prefixes("1.2.3.1/32", "1.2.3.2/32"), []netip.Prefix{}) + + // one route the same, one different + a.UpdateDomainsAndRoutes([]string{}, prefixes("1.2.3.1/32", "1.2.3.3/32")) + a.Wait(ctx) + // old behavior: routes are not removed, resulting routes are both old and new + // (we have dupe 1.2.3.1 routes because the test RouteAdvertiser doesn't have the deduplication + // the real one does) + wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.1/32", "1.2.3.3/32") + wantRemovedRoutes := []netip.Prefix{} + if shouldStore { + // new behavior: routes are removed, resulting routes are new only + wantRoutes = prefixes("1.2.3.1/32", "1.2.3.1/32", "1.2.3.3/32") + wantRemovedRoutes = prefixes("1.2.3.2/32") + } + assertRoutes("removal", wantRoutes, wantRemovedRoutes) + } +} + +func TestUpdateDomainRouteRemoval(t *testing.T) { + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + + assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) { + if !slices.Equal(routes, rc.Routes()) { + t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes()) + } + if !slices.Equal(removedRoutes, rc.RemovedRoutes()) { + t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes()) + } + } + + var a *AppConnector + if shouldStore { + a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes) + } else { + a = NewAppConnector(t.Logf, rc, nil, nil) + } + assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{}) + + a.UpdateDomainsAndRoutes([]string{"a.example.com", "b.example.com"}, []netip.Prefix{}) + a.Wait(ctx) + // adding domains doesn't immediately cause any routes to be advertised + assertRoutes("update domains", []netip.Prefix{}, []netip.Prefix{}) + + a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.1")) + a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.2")) + a.ObserveDNSResponse(dnsResponse("b.example.com.", "1.2.3.3")) + a.ObserveDNSResponse(dnsResponse("b.example.com.", "1.2.3.4")) + a.Wait(ctx) + // observing dns responses causes routes to be advertised + assertRoutes("observed dns", prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32"), []netip.Prefix{}) + + a.UpdateDomainsAndRoutes([]string{"a.example.com"}, []netip.Prefix{}) + a.Wait(ctx) + // old behavior, routes are not removed + wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32") + wantRemovedRoutes := []netip.Prefix{} + if shouldStore { + // new behavior, routes are removed for b.example.com + wantRoutes = prefixes("1.2.3.1/32", "1.2.3.2/32") + wantRemovedRoutes = prefixes("1.2.3.3/32", "1.2.3.4/32") + } + assertRoutes("removal", wantRoutes, wantRemovedRoutes) + } +} + +func TestUpdateWildcardRouteRemoval(t *testing.T) { + for _, shouldStore := range []bool{false, true} { + ctx := context.Background() + rc := &appctest.RouteCollector{} + + assertRoutes := func(prefix string, routes, removedRoutes []netip.Prefix) { + if !slices.Equal(routes, rc.Routes()) { + t.Fatalf("%s: (shouldStore=%t) routes want %v, got %v", prefix, shouldStore, routes, rc.Routes()) + } + if !slices.Equal(removedRoutes, rc.RemovedRoutes()) { + t.Fatalf("%s: (shouldStore=%t) removedRoutes want %v, got %v", prefix, shouldStore, removedRoutes, rc.RemovedRoutes()) + } + } + + var a *AppConnector + if shouldStore { + a = NewAppConnector(t.Logf, rc, &RouteInfo{}, fakeStoreRoutes) + } else { + a = NewAppConnector(t.Logf, rc, nil, nil) + } + assertRoutes("appc init", []netip.Prefix{}, []netip.Prefix{}) + + a.UpdateDomainsAndRoutes([]string{"a.example.com", "*.b.example.com"}, []netip.Prefix{}) + a.Wait(ctx) + // adding domains doesn't immediately cause any routes to be advertised + assertRoutes("update domains", []netip.Prefix{}, []netip.Prefix{}) + + a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.1")) + a.ObserveDNSResponse(dnsResponse("a.example.com.", "1.2.3.2")) + a.ObserveDNSResponse(dnsResponse("1.b.example.com.", "1.2.3.3")) + a.ObserveDNSResponse(dnsResponse("2.b.example.com.", "1.2.3.4")) + a.Wait(ctx) + // observing dns responses causes routes to be advertised + assertRoutes("observed dns", prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32"), []netip.Prefix{}) + + a.UpdateDomainsAndRoutes([]string{"a.example.com"}, []netip.Prefix{}) + a.Wait(ctx) + // old behavior, routes are not removed + wantRoutes := prefixes("1.2.3.1/32", "1.2.3.2/32", "1.2.3.3/32", "1.2.3.4/32") + wantRemovedRoutes := []netip.Prefix{} + if shouldStore { + // new behavior, routes are removed for *.b.example.com + wantRoutes = prefixes("1.2.3.1/32", "1.2.3.2/32") + wantRemovedRoutes = prefixes("1.2.3.3/32", "1.2.3.4/32") + } + assertRoutes("removal", wantRoutes, wantRemovedRoutes) + } +} + +func TestRoutesWithout(t *testing.T) { + assert := func(msg string, got, want []netip.Prefix) { + if !slices.Equal(want, got) { + t.Errorf("%s: want %v, got %v", msg, want, got) + } + } + + assert("empty routes", routesWithout([]netip.Prefix{}, []netip.Prefix{}), []netip.Prefix{}) + assert("a empty", routesWithout([]netip.Prefix{}, prefixes("1.1.1.1/32", "1.1.1.2/32")), []netip.Prefix{}) + assert("b empty", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), []netip.Prefix{}), prefixes("1.1.1.1/32", "1.1.1.2/32")) + assert("no overlap", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), prefixes("1.1.1.3/32", "1.1.1.4/32")), prefixes("1.1.1.1/32", "1.1.1.2/32")) + assert("a has fewer", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32"), prefixes("1.1.1.1/32", "1.1.1.2/32", "1.1.1.3/32", "1.1.1.4/32")), []netip.Prefix{}) + assert("a has more", routesWithout(prefixes("1.1.1.1/32", "1.1.1.2/32", "1.1.1.3/32", "1.1.1.4/32"), prefixes("1.1.1.1/32", "1.1.1.3/32")), prefixes("1.1.1.2/32", "1.1.1.4/32")) +}