From 36e3c418aa5342f5ca353f9dfd605ccd232a9db6 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Thu, 18 Jan 2024 10:33:20 -0800 Subject: [PATCH 1/7] control/controlclient,util/execqueue: extract execqueue into a package This is a useful primitive for asynchronous execution of ordered work I want to use in another change. Updates tailscale/corp#16833 Signed-off-by: James Tucker (cherry picked from commit 38a1cf748a9920aa2cb49101730bba06db1fd839) --- cmd/tailscaled/depaware.txt | 1 + control/controlclient/auto.go | 99 ++--------------------------- util/execqueue/execqueue.go | 104 +++++++++++++++++++++++++++++++ util/execqueue/execqueue_test.go | 22 +++++++ 4 files changed, 131 insertions(+), 95 deletions(-) create mode 100644 util/execqueue/execqueue.go create mode 100644 util/execqueue/execqueue_test.go diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index eeb57a3a8545b..e363278d556f8 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -348,6 +348,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de 💣 tailscale.com/util/deephash from tailscale.com/ipn/ipnlocal+ L 💣 tailscale.com/util/dirwalk from tailscale.com/metrics+ tailscale.com/util/dnsname from tailscale.com/hostinfo+ + tailscale.com/util/execqueue from tailscale.com/control/controlclient tailscale.com/util/goroutines from tailscale.com/ipn/ipnlocal tailscale.com/util/groupmember from tailscale.com/ipn/ipnauth+ 💣 tailscale.com/util/hashx from tailscale.com/util/deephash diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 86b03efa58a1d..d0551cdab5782 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -22,6 +22,7 @@ import ( "tailscale.com/types/netmap" "tailscale.com/types/persist" "tailscale.com/types/structs" + "tailscale.com/util/execqueue" ) type LoginGoal struct { @@ -118,7 +119,7 @@ type Auto struct { closed bool updateCh chan struct{} // readable when we should inform the server of a change observer Observer // called to update Client status; always non-nil - observerQueue execQueue + observerQueue execqueue.ExecQueue unregisterHealthWatch func() @@ -675,7 +676,7 @@ func (c *Auto) Shutdown() { direct := c.direct if !closed { c.closed = true - c.observerQueue.shutdown() + c.observerQueue.Shutdown() c.cancelAuthCtxLocked() c.cancelMapCtxLocked() for _, w := range c.unpauseWaiters { @@ -696,7 +697,7 @@ func (c *Auto) Shutdown() { } ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - c.observerQueue.wait(ctx) + c.observerQueue.Wait(ctx) c.logf("Client.Shutdown done.") } } @@ -737,95 +738,3 @@ func (c *Auto) DoNoiseRequest(req *http.Request) (*http.Response, error) { func (c *Auto) GetSingleUseNoiseRoundTripper(ctx context.Context) (http.RoundTripper, *tailcfg.EarlyNoise, error) { return c.direct.GetSingleUseNoiseRoundTripper(ctx) } - -type execQueue struct { - mu sync.Mutex - closed bool - inFlight bool // whether a goroutine is running q.run - doneWaiter chan struct{} // non-nil if waiter is waiting, then closed - queue []func() -} - -func (q *execQueue) Add(f func()) { - q.mu.Lock() - defer q.mu.Unlock() - if q.closed { - return - } - if q.inFlight { - q.queue = append(q.queue, f) - } else { - q.inFlight = true - go q.run(f) - } -} - -// RunSync waits for the queue to be drained and then synchronously runs f. -// It returns an error if the queue is closed before f is run or ctx expires. -func (q *execQueue) RunSync(ctx context.Context, f func()) error { - for { - if err := q.wait(ctx); err != nil { - return err - } - q.mu.Lock() - if q.inFlight { - q.mu.Unlock() - continue - } - defer q.mu.Unlock() - if q.closed { - return errors.New("closed") - } - f() - return nil - } -} - -func (q *execQueue) run(f func()) { - f() - - q.mu.Lock() - for len(q.queue) > 0 && !q.closed { - f := q.queue[0] - q.queue[0] = nil - q.queue = q.queue[1:] - q.mu.Unlock() - f() - q.mu.Lock() - } - q.inFlight = false - q.queue = nil - if q.doneWaiter != nil { - close(q.doneWaiter) - q.doneWaiter = nil - } - q.mu.Unlock() -} - -func (q *execQueue) shutdown() { - q.mu.Lock() - defer q.mu.Unlock() - q.closed = true -} - -// wait waits for the queue to be empty. -func (q *execQueue) wait(ctx context.Context) error { - q.mu.Lock() - waitCh := q.doneWaiter - if q.inFlight && waitCh == nil { - waitCh = make(chan struct{}) - q.doneWaiter = waitCh - } - q.mu.Unlock() - - if waitCh == nil { - return nil - } - - select { - case <-waitCh: - return nil - case <-ctx.Done(): - return ctx.Err() - } -} diff --git a/util/execqueue/execqueue.go b/util/execqueue/execqueue.go new file mode 100644 index 0000000000000..889cea2555806 --- /dev/null +++ b/util/execqueue/execqueue.go @@ -0,0 +1,104 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +// Package execqueue implements an ordered asynchronous queue for executing functions. +package execqueue + +import ( + "context" + "errors" + "sync" +) + +type ExecQueue struct { + mu sync.Mutex + closed bool + inFlight bool // whether a goroutine is running q.run + doneWaiter chan struct{} // non-nil if waiter is waiting, then closed + queue []func() +} + +func (q *ExecQueue) Add(f func()) { + q.mu.Lock() + defer q.mu.Unlock() + if q.closed { + return + } + if q.inFlight { + q.queue = append(q.queue, f) + } else { + q.inFlight = true + go q.run(f) + } +} + +// RunSync waits for the queue to be drained and then synchronously runs f. +// It returns an error if the queue is closed before f is run or ctx expires. +func (q *ExecQueue) RunSync(ctx context.Context, f func()) error { + for { + if err := q.Wait(ctx); err != nil { + return err + } + q.mu.Lock() + if q.inFlight { + q.mu.Unlock() + continue + } + defer q.mu.Unlock() + if q.closed { + return errors.New("closed") + } + f() + return nil + } +} + +func (q *ExecQueue) run(f func()) { + f() + + q.mu.Lock() + for len(q.queue) > 0 && !q.closed { + f := q.queue[0] + q.queue[0] = nil + q.queue = q.queue[1:] + q.mu.Unlock() + f() + q.mu.Lock() + } + q.inFlight = false + q.queue = nil + if q.doneWaiter != nil { + close(q.doneWaiter) + q.doneWaiter = nil + } + q.mu.Unlock() +} + +// Shutdown asynchronously signals the queue to stop. +func (q *ExecQueue) Shutdown() { + q.mu.Lock() + defer q.mu.Unlock() + q.closed = true +} + +// Wait waits for the queue to be empty. +func (q *ExecQueue) Wait(ctx context.Context) error { + q.mu.Lock() + waitCh := q.doneWaiter + if q.inFlight && waitCh == nil { + waitCh = make(chan struct{}) + q.doneWaiter = waitCh + } + q.mu.Unlock() + + if waitCh == nil { + return nil + } + + select { + case <-waitCh: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/util/execqueue/execqueue_test.go b/util/execqueue/execqueue_test.go new file mode 100644 index 0000000000000..d10b741f72f8f --- /dev/null +++ b/util/execqueue/execqueue_test.go @@ -0,0 +1,22 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package execqueue + +import ( + "context" + "sync/atomic" + "testing" +) + +func TestExecQueue(t *testing.T) { + ctx := context.Background() + var n atomic.Int32 + q := &ExecQueue{} + defer q.Shutdown() + q.Add(func() { n.Add(1) }) + q.Wait(ctx) + if got := n.Load(); got != 1 { + t.Errorf("n=%d; want 1", got) + } +} From d304bdcae8d900856913650469118ae8ed004e13 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Thu, 18 Jan 2024 10:18:25 -0800 Subject: [PATCH 2/7] ipn/ipnlocal: make app connector configuration concurrent If there are routes changes as a side effect of an app connector configuration update, the connector configuration may want to reenter a lock, so must be started asynchronously. Updates tailscale/corp#16833 Signed-off-by: James Tucker (cherry picked from commit 8250582fe6758b05018efb1c6ad74062b741271a) --- appc/appconnector.go | 39 ++++++++++++++++++++++++++++++------ appc/appconnector_test.go | 22 ++++++++++++-------- cmd/tailscaled/depaware.txt | 2 +- ipn/ipnlocal/local.go | 3 +-- ipn/ipnlocal/local_test.go | 3 +++ ipn/ipnlocal/peerapi_test.go | 3 +++ 6 files changed, 55 insertions(+), 17 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 11ca41c3d896b..8935b7909efbb 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -10,6 +10,7 @@ package appc import ( + "context" "net/netip" "slices" "strings" @@ -20,6 +21,7 @@ import ( "tailscale.com/types/logger" "tailscale.com/types/views" "tailscale.com/util/dnsname" + "tailscale.com/util/execqueue" ) // RouteAdvertiser is an interface that allows the AppConnector to advertise @@ -58,6 +60,9 @@ type AppConnector struct { // wildcards is the list of domain strings that match subdomains. wildcards []string + + // queue provides ordering for update operations + queue execqueue.ExecQueue } // NewAppConnector creates a new AppConnector. @@ -68,11 +73,33 @@ func NewAppConnector(logf logger.Logf, routeAdvertiser RouteAdvertiser) *AppConn } } -// UpdateDomains replaces the current set of configured domains with the -// supplied set of domains. Domains must not contain a trailing dot, and should -// be lower case. If the domain contains a leading '*' label it matches all -// subdomains of a domain. +// UpdateDomainsAndRoutes starts an asynchronous update of the configuration +// given the new domains and routes. +func (e *AppConnector) UpdateDomainsAndRoutes(domains []string, routes []netip.Prefix) { + e.queue.Add(func() { + // Add the new routes first. + e.updateRoutes(routes) + e.updateDomains(domains) + }) +} + +// UpdateDomains asynchronously replaces the current set of configured domains +// with the supplied set of domains. Domains must not contain a trailing dot, +// and should be lower case. If the domain contains a leading '*' label it +// matches all subdomains of a domain. func (e *AppConnector) UpdateDomains(domains []string) { + e.queue.Add(func() { + e.updateDomains(domains) + }) +} + +// Wait waits for the currently scheduled asynchronous configuration changes to +// complete. +func (e *AppConnector) Wait(ctx context.Context) { + e.queue.Wait(ctx) +} + +func (e *AppConnector) updateDomains(domains []string) { e.mu.Lock() defer e.mu.Unlock() @@ -104,11 +131,11 @@ func (e *AppConnector) UpdateDomains(domains []string) { e.logf("handling domains: %v and wildcards: %v", xmaps.Keys(e.domains), e.wildcards) } -// UpdateRoutes merges the supplied routes into the currently configured routes. The routes supplied +// updateRoutes merges the supplied routes into the currently configured routes. The routes supplied // by control for UpdateRoutes are supplemental to the routes discovered by DNS resolution, but are // also more often whole ranges. UpdateRoutes will remove any single address routes that are now // covered by new ranges. -func (e *AppConnector) UpdateRoutes(routes []netip.Prefix) { +func (e *AppConnector) updateRoutes(routes []netip.Prefix) { e.mu.Lock() defer e.mu.Unlock() diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index cb42dee6f11c0..2e999a589e8c0 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -4,6 +4,7 @@ package appc import ( + "context" "net/netip" "reflect" "slices" @@ -16,8 +17,11 @@ import ( ) func TestUpdateDomains(t *testing.T) { + ctx := context.Background() a := NewAppConnector(t.Logf, 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) } @@ -25,6 +29,7 @@ func TestUpdateDomains(t *testing.T) { 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) @@ -32,6 +37,7 @@ func TestUpdateDomains(t *testing.T) { // 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) } @@ -41,7 +47,7 @@ func TestUpdateRoutes(t *testing.T) { rc := &routeCollector{} a := NewAppConnector(t.Logf, rc) routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")} - a.UpdateRoutes(routes) + a.updateRoutes(routes) if !slices.EqualFunc(routes, rc.routes, prefixEqual) { t.Fatalf("got %v, want %v", rc.routes, routes) @@ -54,7 +60,7 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { 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")} routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")} - a.UpdateRoutes(routes) + a.updateRoutes(routes) if !slices.EqualFunc(routes, rc.routes, prefixEqual) { t.Fatalf("got %v, want %v", rc.routes, routes) @@ -64,7 +70,7 @@ func TestUpdateRoutesUnadvertisesContainedRoutes(t *testing.T) { func TestDomainRoutes(t *testing.T) { rc := &routeCollector{} a := NewAppConnector(t.Logf, rc) - a.UpdateDomains([]string{"example.com"}) + a.updateDomains([]string{"example.com"}) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) want := map[string][]netip.Addr{ @@ -88,7 +94,7 @@ func TestObserveDNSResponse(t *testing.T) { wantRoutes := []netip.Prefix{netip.MustParsePrefix("192.0.0.8/32")} - a.UpdateDomains([]string{"example.com"}) + a.updateDomains([]string{"example.com"}) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.0.8")) if got, want := rc.routes, wantRoutes; !slices.Equal(got, want) { t.Errorf("got %v; want %v", got, want) @@ -109,7 +115,7 @@ func TestObserveDNSResponse(t *testing.T) { // 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}) + a.updateRoutes([]netip.Prefix{pfx}) wantRoutes = append(wantRoutes, pfx) a.ObserveDNSResponse(dnsResponse("example.com.", "192.0.2.1")) if !slices.Equal(rc.routes, wantRoutes) { @@ -124,7 +130,7 @@ func TestWildcardDomains(t *testing.T) { rc := &routeCollector{} a := NewAppConnector(t.Logf, rc) - a.UpdateDomains([]string{"*.example.com"}) + 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) { t.Errorf("routes: got %v; want %v", got, want) @@ -133,7 +139,7 @@ func TestWildcardDomains(t *testing.T) { t.Errorf("wildcards: got %v; want %v", got, want) } - a.UpdateDomains([]string{"*.example.com", "example.com"}) + 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") } @@ -142,7 +148,7 @@ func TestWildcardDomains(t *testing.T) { } // There was an early regression where the wildcard domain was added repeatedly, this guards against that. - a.UpdateDomains([]string{"*.example.com", "example.com"}) + 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/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index e363278d556f8..454c3dc66d5ad 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -348,7 +348,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de 💣 tailscale.com/util/deephash from tailscale.com/ipn/ipnlocal+ L 💣 tailscale.com/util/dirwalk from tailscale.com/metrics+ tailscale.com/util/dnsname from tailscale.com/hostinfo+ - tailscale.com/util/execqueue from tailscale.com/control/controlclient + tailscale.com/util/execqueue from tailscale.com/control/controlclient+ tailscale.com/util/goroutines from tailscale.com/ipn/ipnlocal tailscale.com/util/groupmember from tailscale.com/ipn/ipnauth+ 💣 tailscale.com/util/hashx from tailscale.com/util/deephash diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index f5ea9e5c3d666..85c0eb73fd9cf 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3460,8 +3460,7 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i slices.SortFunc(routes, func(i, j netip.Prefix) int { return i.Addr().Compare(j.Addr()) }) domains = slices.Compact(domains) routes = slices.Compact(routes) - b.appConnector.UpdateRoutes(routes) - b.appConnector.UpdateDomains(domains) + b.appConnector.UpdateDomainsAndRoutes(domains, routes) } // authReconfig pushes a new configuration into wgengine, if engine diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index f1e8aa3b25fc2..ef4c2f450318e 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -1207,8 +1207,10 @@ func TestObserveDNSResponse(t *testing.T) { rc := &routeCollector{} b.appConnector = appc.NewAppConnector(t.Logf, rc) 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) @@ -1250,6 +1252,7 @@ func TestReconfigureAppConnector(t *testing.T) { }).View() b.reconfigAppConnectorLocked(b.netMap, b.pm.prefs) + b.appConnector.Wait(context.Background()) want := []string{"example.com"} if !slices.Equal(b.appConnector.Domains().AsSlice(), want) { diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 074d114829951..ab182bb28fafa 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -685,6 +685,7 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) { } func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { + ctx := context.Background() var h peerAPIHandler h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") @@ -700,6 +701,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { }, } h.ps.b.appConnector.UpdateDomains([]string{"example.com"}) + h.ps.b.appConnector.Wait(ctx) h.ps.resolver = &fakeResolver{} f := filter.NewAllowAllForTest(logger.Discard) @@ -717,6 +719,7 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { 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) { From 8ffcd9a11b60ea6e10b13115ca65fbea603e9e44 Mon Sep 17 00:00:00 2001 From: Charlotte Brandhorst-Satzkorn Date: Mon, 22 Jan 2024 16:57:31 -0800 Subject: [PATCH 3/7] 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 (cherry picked from commit ce4553b988bac82e7e0aef4ee6055b522f3cbda0) --- appc/appconnector.go | 30 +++++++++------- appc/appconnector_test.go | 63 +++++++++++---------------------- appc/appctest/appctest.go | 41 ++++++++++++++++++++++ ipn/ipnlocal/local.go | 68 +++++++++++++++++++++++++----------- ipn/ipnlocal/local_test.go | 64 ++++++++++++++++++++------------- ipn/ipnlocal/peerapi_test.go | 7 ++-- 6 files changed, 170 insertions(+), 103 deletions(-) create mode 100644 appc/appctest/appctest.go diff --git a/appc/appconnector.go b/appc/appconnector.go index 8935b7909efbb..4e1f983f77e55 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 2e999a589e8c0..feabf9ad8e385 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 0000000000000..dc51ba35226a0 --- /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 85c0eb73fd9cf..cb49262844e32 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 ef4c2f450318e..9ff967b2ebbb2 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 ab182bb28fafa..a5f057bcad54d 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) } } From d1810a7e105f793eb684de2c5233e7a83f3dc1e5 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Mon, 22 Jan 2024 16:18:57 -0800 Subject: [PATCH 4/7] appc: add test to ensure that individual IPs are not removed during route updates If control advised the connector to advertise a route that had already been discovered by DNS it would be incorrectly removed. Now those routes are preserved. Updates tailscale/corp#16833 Signed-off-by: James Tucker (cherry picked from commit 0e2cb76abe1867736fe2aea89d3bc5bbdb5911dd) --- appc/appconnector.go | 2 +- appc/appconnector_test.go | 37 +++++++++++++++++++++++++++++++++++-- appc/appctest/appctest.go | 10 +++++++++- 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/appc/appconnector.go b/appc/appconnector.go index 4e1f983f77e55..a08fbb0b85148 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -155,7 +155,7 @@ nextRoute: for _, r := range routes { for _, addr := range e.domains { for _, a := range addr { - if r.Contains(a) { + if r.Contains(a) && netip.PrefixFrom(a, a.BitLen()) != r { pfx := netip.PrefixFrom(a, a.BitLen()) toRemove = append(toRemove, pfx) continue nextRoute diff --git a/appc/appconnector_test.go b/appc/appconnector_test.go index feabf9ad8e385..510ee83617043 100644 --- a/appc/appconnector_test.go +++ b/appc/appconnector_test.go @@ -45,13 +45,39 @@ func TestUpdateDomains(t *testing.T) { } func TestUpdateRoutes(t *testing.T) { + ctx := context.Background() rc := &appctest.RouteCollector{} a := NewAppConnector(t.Logf, rc) - routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")} + 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) + + 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) + + 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) + + // Ensure that the non-matching /32 is preserved, even though it's in the domains table. if !slices.EqualFunc(routes, rc.Routes(), prefixEqual) { - t.Fatalf("got %v, want %v", rc.Routes(), routes) + 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()) } } @@ -195,3 +221,10 @@ func dnsResponse(domain, address string) []byte { func prefixEqual(a, b netip.Prefix) bool { return a == b } + +func prefixCompare(a, b netip.Prefix) int { + if a.Addr().Compare(b.Addr()) == 0 { + return a.Bits() - b.Bits() + } + return a.Addr().Compare(b.Addr()) +} diff --git a/appc/appctest/appctest.go b/appc/appctest/appctest.go index dc51ba35226a0..d62c0e233c03f 100644 --- a/appc/appctest/appctest.go +++ b/appc/appctest/appctest.go @@ -10,7 +10,8 @@ import ( // RouteCollector is a test helper that collects the list of routes advertised type RouteCollector struct { - routes []netip.Prefix + routes []netip.Prefix + removedRoutes []netip.Prefix } func (rc *RouteCollector) AdvertiseRoute(pfx ...netip.Prefix) error { @@ -24,11 +25,18 @@ func (rc *RouteCollector) UnadvertiseRoute(toRemove ...netip.Prefix) error { for _, r := range routes { if !slices.Contains(toRemove, r) { rc.routes = append(rc.routes, r) + } else { + rc.removedRoutes = append(rc.removedRoutes, r) } } return nil } +// RemovedRoutes returns the list of routes that were removed. +func (rc *RouteCollector) RemovedRoutes() []netip.Prefix { + return rc.removedRoutes +} + // Routes returns the ordered list of routes that were added, including // possible duplicates. func (rc *RouteCollector) Routes() []netip.Prefix { From 916de26363a0a2e3d7ead1899be5a2522b742bf6 Mon Sep 17 00:00:00 2001 From: kari-ts Date: Tue, 23 Jan 2024 10:18:58 -0800 Subject: [PATCH 5/7] VERSION.txt: this is v1.58.1 Signed-off-by: kari-ts --- VERSION.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.txt b/VERSION.txt index 79f82f6b8e0ce..69478d187bd37 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.58.0 +1.58.1 From 80b20dc60fbcdbcd9c919d00393141f4bfd0cfd0 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Sun, 21 Jan 2024 22:52:32 -0500 Subject: [PATCH 6/7] net/portmapper: handle cases where we have no supported clients This no longer results in a nil pointer exception when we get a valid UPnP response with no supported clients. Updates #10911 Signed-off-by: Andrew Dunham Change-Id: I6e3715a49a193ff5261013871ad7fff197a4d77e (cherry picked from commit b45089ad851ebc3f9affd2e06d20f40a0934ffd4) --- net/portmapper/upnp.go | 17 ++- net/portmapper/upnp_test.go | 216 ++++++++++++++++++++++++++++++++++++ 2 files changed, 232 insertions(+), 1 deletion(-) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index 67d9cbbab2296..0f44463e61d9e 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -252,7 +252,8 @@ func getUPnPRootDevice(ctx context.Context, logf logger.Logf, debug DebugKnobs, } // selectBestService picks the "best" service from the given UPnP root device -// to use to create a port mapping. +// to use to create a port mapping. It may return (nil, nil) if no supported +// service was found in the provided *goupnp.RootDevice. // // loc is the parsed location that was used to fetch the given RootDevice. // @@ -559,6 +560,20 @@ func (c *Client) tryUPnPPortmapWithDevice( return netip.AddrPort{}, nil, err } + // If we have no client, we cannot continue; this can happen if we get + // a valid UPnP response that does not contain any of the service types + // that we know how to use. + if client == nil { + // For debugging, print all available services that we aren't + // using because they're not supported; use c.vlogf so we don't + // spam the logs unless verbose debugging is turned on. + rootDev.Device.VisitServices(func(s *goupnp.Service) { + c.vlogf("unsupported UPnP service: Type=%q ID=%q ControlURL=%q", s.ServiceType, s.ServiceId, s.ControlURL.Str) + }) + + return netip.AddrPort{}, nil, fmt.Errorf("no supported UPnP clients") + } + // Start by trying to make a temporary lease with a duration. var newPort uint16 newPort, err = addAnyPortMapping( diff --git a/net/portmapper/upnp_test.go b/net/portmapper/upnp_test.go index 8748cf42719be..4ca332ff1dc75 100644 --- a/net/portmapper/upnp_test.go +++ b/net/portmapper/upnp_test.go @@ -165,6 +165,172 @@ const ( http://10.0.0.1:2828 +` + + // Huawei, https://github.com/tailscale/tailscale/issues/10911 + huaweiRootDescXML = ` + + + 1 + 0 + + + urn:dslforum-org:device:InternetGatewayDevice:1 + HG531 V1 + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2600-587f668bdd9a + 000000000001 + + + urn:www-huawei-com:service:DeviceConfig:1 + urn:www-huawei-com:serviceId:DeviceConfig1 + /desc/DevCfg.xml + /ctrlt/DeviceConfig_1 + /evt/DeviceConfig_1 + + + urn:dslforum-org:service:LANConfigSecurity:1 + urn:dslforum-org:serviceId:LANConfigSecurity1 + /desc/LANSec.xml + /ctrlt/LANConfigSecurity_1 + /evt/LANConfigSecurity_1 + + + urn:dslforum-org:service:Layer3Forwarding:1 + urn:dslforum-org:serviceId:Layer3Forwarding1 + /desc/L3Fwd.xml + /ctrlt/Layer3Forwarding_1 + /evt/Layer3Forwarding_1 + + + + + urn:dslforum-org:device:WANDevice:1 + WANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2601-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANDSLInterfaceConfig:1 + urn:dslforum-org:serviceId:WANDSLInterfaceConfig1 + /desc/WanDslIfCfg.xml + /ctrlt/WANDSLInterfaceConfig_1 + /evt/WANDSLInterfaceConfig_1 + + + urn:dslforum-org:service:WANCommonInterfaceConfig:1 + urn:dslforum-org:serviceId:WANCommonInterfaceConfig1 + /desc/WanCommonIfc1.xml + /ctrlt/WANCommonInterfaceConfig_1 + /evt/WANCommonInterfaceConfig_1 + + + + + urn:dslforum-org:device:WANConnectionDevice:1 + WANConnectionDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2603-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WANPPPConnection:1 + urn:dslforum-org:serviceId:WANPPPConnection1 + /desc/WanPppConn.xml + /ctrlt/WANPPPConnection_1 + /evt/WANPPPConnection_1 + + + urn:dslforum-org:service:WANEthernetConnectionManagement:1 + urn:dslforum-org:serviceId:WANEthernetConnectionManagement1 + /desc/WanEthConnMgt.xml + /ctrlt/WANEthernetConnectionManagement_1 + /evt/WANEthernetConnectionManagement_1 + + + urn:dslforum-org:service:WANDSLLinkConfig:1 + urn:dslforum-org:serviceId:WANDSLLinkConfig1 + /desc/WanDslLink.xml + /ctrlt/WANDSLLinkConfig_1 + /evt/WANDSLLinkConfig_1 + + + + + + + urn:dslforum-org:device:LANDevice:1 + LANDevice + Huawei Technologies Co., Ltd. + http://www.huawei.com + Huawei Home Gateway + HG531 V1 + Huawei Model + http://www.huawei.com + G6J8W15326003974 + uuid:00e0fc37-2626-2828-2602-587f668bdd9a + 000000000001 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration4 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_4 + /evt/WLANConfiguration_4 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration3 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_3 + /evt/WLANConfiguration_3 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration2 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_2 + /evt/WLANConfiguration_2 + + + urn:dslforum-org:service:WLANConfiguration:1 + urn:dslforum-org:serviceId:WLANConfiguration1 + /desc/WLANCfg.xml + /ctrlt/WLANConfiguration_1 + /evt/WLANConfiguration_1 + + + urn:dslforum-org:service:LANHostConfigManagement:1 + urn:dslforum-org:serviceId:LANHostConfigManagement1 + /desc/LanHostCfgMgmt.xml + /ctrlt/LANHostConfigManagement_1 + /evt/LANHostConfigManagement_1 + + + + + http://127.0.0.1 + + ` ) @@ -233,6 +399,14 @@ func TestGetUPnPClient(t *testing.T) { "*internetgateway2.WANIPConnection1", "saw UPnP type WANIPConnection1 at http://127.0.0.1:NNN/rootDesc.xml; MikroTik Router (MikroTik), method=none\n", }, + { + "huawei", + huaweiRootDescXML, + // services not supported and thus returns nil, but shouldn't crash + "", + "", + }, + // TODO(bradfitz): find a PPP one in the wild } for _, tt := range tests { @@ -375,6 +549,48 @@ func TestGetUPnPPortMapping(t *testing.T) { } } +// TestGetUPnPPortMapping_NoValidServices tests that getUPnPPortMapping doesn't +// crash when a valid UPnP response with no supported services is discovered +// and parsed. +// +// See https://github.com/tailscale/tailscale/issues/10911 +func TestGetUPnPPortMapping_NoValidServices(t *testing.T) { + igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) + if err != nil { + t.Fatal(err) + } + defer igd.Close() + + igd.SetUPnPHandler(&upnpServer{ + t: t, + Desc: huaweiRootDescXML, + }) + + c := newTestClient(t, igd) + defer c.Close() + c.debug.VerboseLogs = true + + ctx := context.Background() + res, err := c.Probe(ctx) + if err != nil { + t.Fatalf("Probe: %v", err) + } + if !res.UPnP { + t.Errorf("didn't detect UPnP") + } + + gw, myIP, ok := c.gatewayAndSelfIP() + if !ok { + t.Fatalf("could not get gateway and self IP") + } + + // This shouldn't panic + _, ok = c.getUPnPPortMapping(ctx, gw, netip.AddrPortFrom(myIP, 12345), 0) + if ok { + t.Fatal("did not expect to get UPnP port mapping") + } +} + func TestGetUPnPPortMappingNoResponses(t *testing.T) { igd, err := NewTestIGD(t.Logf, TestIGDOptions{UPnP: true}) if err != nil { From b0e1bbb62ef3834e99f5212b44043cec1866b07e Mon Sep 17 00:00:00 2001 From: kari-ts Date: Tue, 23 Jan 2024 13:54:48 -0800 Subject: [PATCH 7/7] VERSION.txt: this is v1.58.2 Signed-off-by: kari-ts --- VERSION.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.txt b/VERSION.txt index 69478d187bd37..7af7cf49339fb 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.58.1 +1.58.2