From 4933bb83d252eaad4613c195f0c18bf9e5db3ab5 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 22 Oct 2026 00:17:42 +0000 Subject: [PATCH] policyutil: match DstPorts as ranges when reducing filters Every node re-parsed every entry into an IPSet on every policy change; under churn that was most of the batcher's CPU. --- hscontrol/policy/policyutil/reduce.go | 56 ++++++++-------- hscontrol/policy/policyutil/reduce_test.go | 75 ++++++++++++++++++++++ hscontrol/util/addr.go | 40 ++++++++++++ hscontrol/util/addr_test.go | 39 +++++++++++ 4 files changed, 184 insertions(+), 26 deletions(-) diff --git a/hscontrol/policy/policyutil/reduce.go b/hscontrol/policy/policyutil/reduce.go index 61d6b6949..1123d5d76 100644 --- a/hscontrol/policy/policyutil/reduce.go +++ b/hscontrol/policy/policyutil/reduce.go @@ -6,6 +6,7 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "go4.org/netipx" "tailscale.com/tailcfg" ) @@ -18,6 +19,7 @@ import ( // both cases. func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcfg.FilterRule { ret := []tailcfg.FilterRule{} + nodeIPs := node.IPs() subnetRoutes := node.SubnetRoutes() exitRoutes := node.ExitRoutes() @@ -37,32 +39,7 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf var dests []tailcfg.NetPortRange for _, dest := range rule.DstPorts { - expanded, err := util.ParseIPSet(dest.IP, nil) - // Fail closed: unparseable dests are dropped. - if err != nil { - continue - } - - if node.InIPSet(expanded) { - dests = append(dests, dest) - continue - } - - // If the node has approved subnet routes, preserve - // filter rules targeting those routes. - // [types.NodeView.SubnetRoutes] returns only approved, - // non-exit routes — matching Tailscale SaaS behavior, - // which does not generate filter rules for - // advertised-but-unapproved routes. - if slices.ContainsFunc(subnetRoutes, expanded.OverlapsPrefix) { - dests = append(dests, dest) - continue - } - - // Approved exit routes count like subnet routes. They - // contain every destination, so Tailscale SaaS sends an - // exit node every rule, not only internet ones. - if slices.ContainsFunc(exitRoutes, expanded.OverlapsPrefix) { + if destReachesNode(dest.IP, nodeIPs, subnetRoutes, exitRoutes) { dests = append(dests, dest) } } @@ -79,6 +56,33 @@ func ReduceFilterRules(node types.NodeView, rules []tailcfg.FilterRule) []tailcf return ret } +// destReachesNode reports whether a DstPorts entry targets the node: one of +// its addresses, or a route it serves. Every node reduces every entry on +// every policy change, so the entry is parsed as one range rather than an +// IPSet. Unparseable entries fail closed. +func destReachesNode(dst string, nodeIPs []netip.Addr, subnetRoutes, exitRoutes []netip.Prefix) bool { + // Spans both families, so it is the one form that is not a range. + if dst == "*" { + return len(nodeIPs) > 0 || len(subnetRoutes) > 0 || len(exitRoutes) > 0 + } + + r, err := util.ParseIPRange(dst) + if err != nil { + return false + } + + overlaps := func(p netip.Prefix) bool { return r.Overlaps(netipx.RangeOfPrefix(p)) } + + // [types.NodeView.SubnetRoutes] holds only approved, non-exit routes: + // Tailscale SaaS generates no rules for advertised-but-unapproved + // routes. Approved exit routes count like subnet routes. They contain + // every destination, so SaaS sends an exit node every rule, not only + // internet ones. + return slices.ContainsFunc(nodeIPs, r.Contains) || + slices.ContainsFunc(subnetRoutes, overlaps) || + slices.ContainsFunc(exitRoutes, overlaps) +} + // reduceCapGrantRule filters a [tailcfg.CapGrant] rule to only include // [tailcfg.CapGrant] entries whose Dsts match the given node's IPs. When a // broad prefix (e.g. 100.64.0.0/10 from dst:*) contains a node's IP, it is diff --git a/hscontrol/policy/policyutil/reduce_test.go b/hscontrol/policy/policyutil/reduce_test.go index 4a51bb5e6..9039d75f1 100644 --- a/hscontrol/policy/policyutil/reduce_test.go +++ b/hscontrol/policy/policyutil/reduce_test.go @@ -1339,3 +1339,78 @@ func TestReduceFilterRulesCapGrant(t *testing.T) { }) } } + +// TestReduceFilterRulesDestForms reduces one rule holding every DstPorts form +// for a node at 100.64.0.1 routing 10.0.0.0/24, and pins which entries reach +// it. Malformed entries fail closed. +func TestReduceFilterRulesDestForms(t *testing.T) { + node := &types.Node{ + IPv4: ap("100.64.0.1"), + IPv6: ap("fd7a:115c:a1e0::1"), + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{p("10.0.0.0/24")}, + }, + ApprovedRoutes: []netip.Prefix{p("10.0.0.0/24")}, + } + + dsts := map[string]bool{ + "*": true, + "100.64.0.1": true, + "fd7a:115c:a1e0::1": true, + "100.64.0.2": false, + "100.64.0.0/10": true, + "100.128.0.0/10": false, + "10.0.0.128/25": true, + "10.0.1.0/24": false, + "100.64.0.0-100.64.0.9": true, + "100.64.0.2-100.64.0.9": false, + "10.0.0.200-10.0.1.5": true, + "100.64.0.1/10": false, // host bits set + "100.64.0.9-100.64.0.1": false, // reversed range + "100.64.0.1-fd7a:115c::1": false, // mixed families + "not-an-ip": false, + "100.64.0.1-100.64.0.2-100": false, + } + + rule := tailcfg.FilterRule{SrcIPs: []string{"*"}} + for dst := range dsts { + rule.DstPorts = append(rule.DstPorts, tailcfg.NetPortRange{IP: dst, Ports: tailcfg.PortRangeAny}) + } + + got := policyutil.ReduceFilterRules(node.View(), []tailcfg.FilterRule{rule}) + require.Len(t, got, 1) + + kept := make(map[string]bool) + for _, dp := range got[0].DstPorts { + kept[dp.IP] = true + } + + for dst, want := range dsts { + require.Equal(t, want, kept[dst], "DstPorts %q", dst) + } +} + +// TestReduceFilterRulesAllocsIndependentOfDests reduces the global filter as +// every node does on every policy change. Its DstPorts are plain strings, so +// anything allocated per entry, such as building an IPSet to test one +// address, is paid nodes × entries times per change. +func TestReduceFilterRulesAllocsIndependentOfDests(t *testing.T) { + node := (&types.Node{IPv4: ap("100.64.0.1"), IPv6: ap("fd7a:115c:a1e0::1")}).View() + + allocs := func(n int) int { + rule := tailcfg.FilterRule{SrcIPs: []string{"*"}} + for i := range n { + rule.DstPorts = append(rule.DstPorts, tailcfg.NetPortRange{ + IP: fmt.Sprintf("100.64.%d.%d", i/250, i%250+2), + Ports: tailcfg.PortRangeAny, + }) + } + + rules := []tailcfg.FilterRule{rule} + + return int(testing.AllocsPerRun(10, func() { policyutil.ReduceFilterRules(node, rules) })) + } + + require.Equal(t, allocs(10), allocs(1000), + "reducing a filter must not allocate per DstPorts entry") +} diff --git a/hscontrol/util/addr.go b/hscontrol/util/addr.go index b44343255..456e569cf 100644 --- a/hscontrol/util/addr.go +++ b/hscontrol/util/addr.go @@ -1,6 +1,7 @@ package util import ( + "errors" "fmt" "iter" "net/netip" @@ -85,6 +86,45 @@ func ParseIPSet(arg string, bits *int) (*netipx.IPSet, error) { return ipSet.IPSet() } +// ParseIPRange errors. +var ( + ErrPrefixHostBits = errors.New("prefix contains non-network bits set") + ErrInvalidIPAddress = errors.New("invalid IP address") +) + +// ParseIPRange parses what [ParseIPSet] accepts, apart from "*", as the single +// range it denotes, without building a set. +func ParseIPRange(arg string) (netipx.IPRange, error) { + if strings.Contains(arg, "/") { + pfx, err := netip.ParsePrefix(arg) + if err != nil { + return netipx.IPRange{}, err + } + + if pfx != pfx.Masked() { + return netipx.IPRange{}, fmt.Errorf("%w: %v", ErrPrefixHostBits, pfx) + } + + return netipx.RangeOfPrefix(pfx), nil + } + + if strings.Count(arg, "-") == 1 { + return netipx.ParseIPRange(arg) + } + + ip, err := netip.ParseAddr(arg) + if err != nil { + return netipx.IPRange{}, fmt.Errorf("%w: %q", ErrInvalidIPAddress, arg) + } + + r := netipx.IPRangeFrom(ip, ip) + if !r.IsValid() { + return netipx.IPRange{}, fmt.Errorf("%w: %q", ErrInvalidIPAddress, arg) + } + + return r, nil +} + func GetIPPrefixEndpoints(na netip.Prefix) (netip.Addr, netip.Addr) { ipRange := netipx.RangeOfPrefix(na) diff --git a/hscontrol/util/addr_test.go b/hscontrol/util/addr_test.go index 7c6ae31ae..80caeb549 100644 --- a/hscontrol/util/addr_test.go +++ b/hscontrol/util/addr_test.go @@ -119,3 +119,42 @@ func Test_parseIPSet(t *testing.T) { }) } } + +// TestParseIPRangeMatchesParseIPSet checks ParseIPRange accepts exactly what +// ParseIPSet does, "*" aside, and denotes the same addresses. +func TestParseIPRangeMatchesParseIPSet(t *testing.T) { + for _, arg := range []string{ + "10.0.0.1", + "2001:db8::2", + "192.168.0.0/16", + "2001:db8:abcd:1234::/64", + "192.168.0.0-192.168.255.255", + "10.0.0.5-10.0.0.9", + "fd7a::1-fd7a::ff", + "10.0.0.1/8", + "10.0.0.9-10.0.0.1", + "10.0.0.1-fd7a::1", + "10.0.0.1-10.0.0.2-10.0.0.3", + "fe80::1%eth0", + "10.0.0.0/33", + "not-an-ip", + "", + } { + set, setErr := ParseIPSet(arg, nil) + r, rangeErr := ParseIPRange(arg) + + if (setErr != nil) != (rangeErr != nil) { + t.Errorf("%q: ParseIPSet error %v, ParseIPRange error %v", arg, setErr, rangeErr) + + continue + } + + if setErr != nil { + continue + } + + if diff := cmp.Diff(set.Ranges(), []netipx.IPRange{r}, cmp.Comparer(func(a, b netipx.IPRange) bool { return a == b })); diff != "" { + t.Errorf("%q: (-ParseIPSet +ParseIPRange):\n%s", arg, diff) + } + } +}