From dd54240f421fb0bcbdad748ec6e35e11f659490e Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Fri, 21 Aug 2026 11:02:40 +0200 Subject: [PATCH] Release the routing reference when the route manager shuts down --- .../firewall/iptables/manager_linux_test.go | 8 +++ client/internal/routemanager/server/server.go | 8 +++ .../routemanager/server/server_test.go | 66 +++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 client/internal/routemanager/server/server_test.go diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index 3f45bf398..3f9a0e7b9 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -420,6 +420,14 @@ func TestIptablesCloseRemovesAllState(t *testing.T) { require.NoError(t, err) require.NoError(t, manager.Init(nil)) + // A failed assertion below returns before the Close under test, which would + // leave this test's chains and sets in the kernel for the next one. + t.Cleanup(func() { + if err := manager.Close(nil); err != nil { + t.Logf("close after failure: %v", err) + } + }) + sources := []netip.Prefix{ netip.MustParsePrefix("10.20.0.42/32"), netip.MustParsePrefix("10.20.0.43/32"), diff --git a/client/internal/routemanager/server/server.go b/client/internal/routemanager/server/server.go index f569c0cac..38d7f0db4 100644 --- a/client/internal/routemanager/server/server.go +++ b/client/internal/routemanager/server/server.go @@ -135,6 +135,14 @@ func (r *Router) CleanUp() { } } + // Give back the routing reference taken in UpdateRoutes, after the routes + // are gone as above. Without this the sysctls enabling it changed (IPv6 + // forwarding and the accept_ra values that keep RA handling alive next to + // it) stay applied once the client stops. + if err := r.firewall.DisableRouting(); err != nil { + log.Errorf("Failed to disable routing: %v", err) + } + r.statusRecorder.CleanLocalPeerStateRoutes() } diff --git a/client/internal/routemanager/server/server_test.go b/client/internal/routemanager/server/server_test.go new file mode 100644 index 000000000..1b42115e2 --- /dev/null +++ b/client/internal/routemanager/server/server_test.go @@ -0,0 +1,66 @@ +package server + +import ( + "context" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + firewall "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/internal/peer" + "github.com/netbirdio/netbird/route" +) + +// routingFirewall records the routing lifecycle calls the router makes. The +// embedded interface covers the methods this test never reaches. +type routingFirewall struct { + firewall.Manager + + removed []firewall.RouterPair + enabled int + disabled int +} + +func (f *routingFirewall) RemoveNatRule(pair firewall.RouterPair) error { + f.removed = append(f.removed, pair) + return nil +} + +func (f *routingFirewall) EnableRouting() error { + f.enabled++ + return nil +} + +func (f *routingFirewall) DisableRouting() error { + f.disabled++ + return nil +} + +// TestRouterCleanUpReleasesRouting covers the shutdown path: the router holds a +// routing reference for as long as it serves routes, and CleanUp has to give it +// back. Without that the sysctls the reference enabled (IPv6 forwarding and the +// accept_ra values that keep RA handling working alongside it) stay applied +// after the client stops, leaving the host configured as a router. +func TestRouterCleanUpReleasesRouting(t *testing.T) { + fw := &routingFirewall{} + r := &Router{ + ctx: context.Background(), + firewall: fw, + statusRecorder: peer.NewRecorder("https://mgm"), + routes: map[route.ID]*route.Route{ + "route-1": { + ID: "route-1", + Network: netip.MustParsePrefix("192.168.55.0/24"), + NetworkType: route.IPv4Network, + Masquerade: true, + }, + }, + } + + r.CleanUp() + + require.Len(t, fw.removed, 1, "the route's NAT rule must be removed") + assert.Equal(t, 1, fw.disabled, "CleanUp must release the routing reference") +}