From ce4553b988bac82e7e0aef4ee6055b522f3cbda0 Mon Sep 17 00:00:00 2001 From: Charlotte Brandhorst-Satzkorn Date: Mon, 22 Jan 2024 16:57:31 -0800 Subject: [PATCH] appc,ipn/ipnlocal: optimize preference adjustments when routes update This change allows us to perform batch modification for new route advertisements and route removals. Additionally, we now handle the case where newly added routes are covered by existing ranges. This change also introduces a new appctest package that contains some shared functions used for testing. Updates tailscale/corp#16833 Signed-off-by: Charlotte Brandhorst-Satzkorn --- appc/appconnector.go | 30 +++++++++------- appc/appconnector_test.go | 63 +++++++++++----------------------- appc/appctest/appctest.go | 41 ++++++++++++++++++++++ ipn/ipnlocal/local.go | 66 +++++++++++++++++++++++++----------- ipn/ipnlocal/local_test.go | 64 +++++++++++++++++++++------------- ipn/ipnlocal/peerapi_test.go | 7 ++-- 6 files changed, 169 insertions(+), 102 deletions(-) create mode 100644 appc/appctest/appctest.go diff --git a/appc/appconnector.go b/appc/appconnector.go index 8935b7909..4e1f983f7 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -27,12 +27,12 @@ import ( // RouteAdvertiser is an interface that allows the AppConnector to advertise // newly discovered routes that need to be served through the AppConnector. type RouteAdvertiser interface { - // AdvertiseRoute adds a new route advertisement if the route is not already - // being advertised. - AdvertiseRoute(netip.Prefix) error + // AdvertiseRoute adds one or more route advertisements skipping any that + // are already advertised. + AdvertiseRoute(...netip.Prefix) error - // UnadvertiseRoute removes a route advertisement. - UnadvertiseRoute(netip.Prefix) error + // UnadvertiseRoute removes any matching route advertisements. + UnadvertiseRoute(...netip.Prefix) error } // AppConnector is an implementation of an AppConnector that performs @@ -144,26 +144,30 @@ func (e *AppConnector) updateRoutes(routes []netip.Prefix) { return } + if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil { + e.logf("failed to advertise routes: %v: %v", routes, err) + return + } + + var toRemove []netip.Prefix + nextRoute: for _, r := range routes { - if err := e.routeAdvertiser.AdvertiseRoute(r); err != nil { - e.logf("failed to advertise route: %v: %v", r, err) - continue - } - for _, addr := range e.domains { for _, a := range addr { if r.Contains(a) { pfx := netip.PrefixFrom(a, a.BitLen()) - if err := e.routeAdvertiser.UnadvertiseRoute(pfx); err != nil { - e.logf("failed to unadvertise route: %v: %v", pfx, err) - } + toRemove = append(toRemove, pfx) continue nextRoute } } } } + if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil { + e.logf("failed to unadvertise routes: %v: %v", toRemove, err) + } + e.controlRoutes = routes } diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index 2e999a589..feabf9ad8 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -12,6 +12,7 @@ import ( xmaps "golang.org/x/exp/maps" "golang.org/x/net/dns/dnsmessage" + "tailscale.com/appc/appctest" "tailscale.com/util/mak" "tailscale.com/util/must" ) @@ -44,31 +45,31 @@ func TestUpdateDomains(t *testing.T) { } func TestUpdateRoutes(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) 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 TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) mak.Set(&a.domains, "example.com", []netip.Addr{netip.MustParseAddr("192.0.2.1")}) - rc.routes = []netip.Prefix{netip.MustParsePrefix("192.0.2.1/32")} + 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 := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) a.updateDomains([]string{"example.com"}) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) @@ -83,12 +84,12 @@ func TestDomainRoutes(t *testing.T) { } func TestObserveDNSResponse(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) // 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) { + if got, want := rc.Routes(), ([]netip.Prefix)(nil); !slices.Equal(got, want) { t.Errorf("got %v; want %v", got, want) } @@ -96,21 +97,21 @@ func TestObserveDNSResponse(t *testing.T) { a.updateDomains([]string{"example.com"}) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - if got, want := rc.routes, wantRoutes; !slices.Equal(got, want) { + 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")) a.ObserveDNSResponse(dnsResponse("example.com.", "2001:db8::1")) - if got, want := rc.routes, wantRoutes; !slices.Equal(got, want) { + 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")) - if !slices.Equal(rc.routes, wantRoutes) { - t.Errorf("rc.routes: got %v; want %v", rc.routes, wantRoutes) + 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 @@ -118,8 +119,8 @@ func TestObserveDNSResponse(t *testing.T) { a.updateRoutes([]netip.Prefix{pfx}) wantRoutes = append(wantRoutes, pfx) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.2.1")) - if !slices.Equal(rc.routes, wantRoutes) { - t.Errorf("rc.routes: got %v; want %v", rc.routes, wantRoutes) + 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"]) @@ -127,12 +128,12 @@ func TestObserveDNSResponse(t *testing.T) { } func TestWildcardDomains(t *testing.T) { - rc := &routeCollector{} + rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) a.updateDomains([]string{"*.example.com"}) a.ObserveDNSResponse(dnsResponse("foo.example.com.", "192.0.0.8")) - if got, want := rc.routes, []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")}; !slices.Equal(got, want) { + 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) { @@ -191,30 +192,6 @@ func dnsResponse(domain, address string) []byte { return must.Get(b.Finish()) } -// routeCollector is a test helper that collects the list of routes advertised -type routeCollector struct { - routes []netip.Prefix -} - -// routeCollector implements RouteAdvertiser -var _ RouteAdvertiser = (*routeCollector)(nil) - -func (rc *routeCollector) AdvertiseRoute(pfx netip.Prefix) error { - rc.routes = append(rc.routes, pfx) - return nil -} - -func (rc *routeCollector) UnadvertiseRoute(pfx netip.Prefix) error { - routes := rc.routes - rc.routes = rc.routes[:0] - for _, r := range routes { - if r != pfx { - rc.routes = append(rc.routes, r) - } - } - return nil -} - func prefixEqual(a, b netip.Prefix) bool { - return a.Addr().Compare(b.Addr()) == 0 && a.Bits() == b.Bits() + return a == b } diff --git a/appc/appctest/appctest.go b/appc/appctest/appctest.go new file mode 100644 index 000000000..dc51ba352 --- /dev/null +++ b/appc/appctest/appctest.go @@ -0,0 +1,41 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package appctest + +import ( + "net/netip" + "slices" +) + +// RouteCollector is a test helper that collects the list of routes advertised +type RouteCollector struct { + routes []netip.Prefix +} + +func (rc *RouteCollector) AdvertiseRoute(pfx ...netip.Prefix) error { + rc.routes = append(rc.routes, pfx...) + return nil +} + +func (rc *RouteCollector) UnadvertiseRoute(toRemove ...netip.Prefix) error { + routes := rc.routes + rc.routes = rc.routes[:0] + for _, r := range routes { + if !slices.Contains(toRemove, r) { + rc.routes = append(rc.routes, r) + } + } + return nil +} + +// Routes returns the ordered list of routes that were added, including +// possible duplicates. +func (rc *RouteCollector) Routes() []netip.Prefix { + return rc.routes +} + +func (rc *RouteCollector) SetRoutes(routes []netip.Prefix) error { + rc.routes = routes + return nil +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 85c0eb73f..cb4926284 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -5790,45 +5790,73 @@ var ErrDisallowedAutoRoute = errors.New("route is not allowed") // AdvertiseRoute implements the appc.RouteAdvertiser interface. It sets a new // route advertisement if one is not already present in the existing routes. // If the route is disallowed, ErrDisallowedAutoRoute is returned. -func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error { - if !allowedAutoRoute(ipp) { - return ErrDisallowedAutoRoute +func (b *LocalBackend) AdvertiseRoute(ipps ...netip.Prefix) error { + finalRoutes := b.Prefs().AdvertiseRoutes().AsSlice() + newRoutes := false + + for _, ipp := range ipps { + if !allowedAutoRoute(ipp) { + continue + } + if slices.Contains(finalRoutes, ipp) { + continue + } + + // If the new prefix is already contained by existing routes, skip it. + if coveredRouteRange(finalRoutes, ipp) { + continue + } + + finalRoutes = append(finalRoutes, ipp) + newRoutes = true } - currentRoutes := b.Prefs().AdvertiseRoutes() - if currentRoutes.ContainsFunc(func(r netip.Prefix) bool { - // TODO(raggi): add support for subset checks and avoid subset route creations. - return ipp.IsSingleIP() && r.Contains(ipp.Addr()) || r == ipp - }) { + + if !newRoutes { return nil } - routes := append(currentRoutes.AsSlice(), ipp) + _, err := b.EditPrefs(&ipn.MaskedPrefs{ Prefs: ipn.Prefs{ - AdvertiseRoutes: routes, + AdvertiseRoutes: finalRoutes, }, AdvertiseRoutesSet: true, }) return err } +// coveredRouteRange checks if a route is already included in a slice of +// prefixes. +func coveredRouteRange(finalRoutes []netip.Prefix, ipp netip.Prefix) bool { + for _, r := range finalRoutes { + if ipp.IsSingleIP() { + if r.Contains(ipp.Addr()) { + return true + } + } else { + if r.Contains(ipp.Addr()) && r.Contains(netipx.PrefixLastIP(ipp)) { + return true + } + } + } + return false +} + // UnadvertiseRoute implements the appc.RouteAdvertiser interface. It removes // a route advertisement if one is present in the existing routes. -func (b *LocalBackend) UnadvertiseRoute(ipp netip.Prefix) error { +func (b *LocalBackend) UnadvertiseRoute(toRemove ...netip.Prefix) error { currentRoutes := b.Prefs().AdvertiseRoutes().AsSlice() - if !slices.Contains(currentRoutes, ipp) { - return nil - } + finalRoutes := currentRoutes[:0] - newRoutes := currentRoutes[:0] - for _, r := range currentRoutes { - if r != ipp { - newRoutes = append(newRoutes, r) + for _, ipp := range currentRoutes { + if slices.Contains(toRemove, ipp) { + continue } + finalRoutes = append(finalRoutes, ipp) } _, err := b.EditPrefs(&ipn.MaskedPrefs{ Prefs: ipn.Prefs{ - AdvertiseRoutes: newRoutes, + AdvertiseRoutes: finalRoutes, }, AdvertiseRoutesSet: true, }) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index ef4c2f450..9ff967b2e 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -18,6 +18,7 @@ import ( "go4.org/netipx" "golang.org/x/net/dns/dnsmessage" "tailscale.com/appc" + "tailscale.com/appc/appctest" "tailscale.com/control/controlclient" "tailscale.com/ipn" "tailscale.com/ipn/store/mem" @@ -1204,7 +1205,7 @@ func TestObserveDNSResponse(t *testing.T) { // ensure no error when no app connector is configured b.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) - rc := &routeCollector{} + rc := &appctest.RouteCollector{} b.appConnector = appc.NewAppConnector(t.Logf, rc) b.appConnector.UpdateDomains([]string{"example.com"}) b.appConnector.Wait(context.Background()) @@ -1212,8 +1213,44 @@ func TestObserveDNSResponse(t *testing.T) { 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) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Fatalf("got routes %v, want %v", rc.Routes(), wantRoutes) + } +} + +func TestCoveredRouteRange(t *testing.T) { + tests := []struct { + existingRoute netip.Prefix + newRoute netip.Prefix + want bool + }{ + { + existingRoute: netip.MustParsePrefix("192.0.0.1/32"), + newRoute: netip.MustParsePrefix("192.0.0.1/32"), + want: true, + }, + { + existingRoute: netip.MustParsePrefix("192.0.0.1/32"), + newRoute: netip.MustParsePrefix("192.0.0.2/32"), + want: false, + }, + { + existingRoute: netip.MustParsePrefix("192.0.0.0/24"), + newRoute: netip.MustParsePrefix("192.0.0.1/32"), + want: true, + }, + { + existingRoute: netip.MustParsePrefix("192.0.0.0/16"), + newRoute: netip.MustParsePrefix("192.0.0.0/24"), + want: true, + }, + } + + for _, tt := range tests { + got := coveredRouteRange([]netip.Prefix{tt.existingRoute}, tt.newRoute) + if got != tt.want { + t.Errorf("coveredRouteRange(%v, %v) = %v, want %v", tt.existingRoute, tt.newRoute, got, tt.want) + } } } @@ -1352,27 +1389,6 @@ func dnsResponse(domain, address string) []byte { return must.Get(b.Finish()) } -// routeCollector is a test helper that collects the list of routes advertised -type routeCollector struct { - routes []netip.Prefix -} - -func (rc *routeCollector) AdvertiseRoute(pfx netip.Prefix) error { - rc.routes = append(rc.routes, pfx) - return nil -} - -func (rc *routeCollector) UnadvertiseRoute(pfx netip.Prefix) error { - routes := rc.routes - rc.routes = rc.routes[:0] - for _, r := range routes { - if r != pfx { - rc.routes = append(rc.routes, r) - } - } - return nil -} - type errorSyspolicyHandler struct { t *testing.T err error diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index ab182bb28..a5f057bca 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -23,6 +23,7 @@ import ( "go4.org/netipx" "golang.org/x/net/dns/dnsmessage" "tailscale.com/appc" + "tailscale.com/appc/appctest" "tailscale.com/client/tailscale/apitype" "tailscale.com/ipn" "tailscale.com/ipn/store/mem" @@ -689,7 +690,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { var h peerAPIHandler h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") - rc := &routeCollector{} + rc := &appctest.RouteCollector{} eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) pm := must.Get(newProfileManager(new(mem.Store), t.Logf)) h.ps = &peerAPIServer{ @@ -722,8 +723,8 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { 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) + if !slices.Equal(rc.Routes(), wantRoutes) { + t.Errorf("got %v; want %v", rc.Routes(), wantRoutes) } }