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 - } - 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 - }) { +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 + } + + 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) } }