2068 AutoApprovers tests (#2105)

* replace old suite approved routes test with table driven

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* add test to reproduce issue

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

* add integration test for 2068

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

---------

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

.github/workflows/test-integration.yaml

@@ -55,6 +55,7 @@ jobs:
           - TestEnablingRoutes
           - TestHASubnetRouterFailover
           - TestEnableDisableAutoApprovedRoute
+          - TestAutoApprovedSubRoute2068
           - TestSubnetRouteACL
           - TestHeadscale
           - TestCreateTailscale

hscontrol/db/node_test.go

@@ -6,6 +6,7 @@ import (
 	"math/big"
 	"net/netip"
 	"regexp"
+	"sort"
 	"strconv"
 	"sync"
 	"testing"
@@ -518,8 +519,37 @@ func TestHeadscale_generateGivenName(t *testing.T) {
 	}
 }
 
-func (s *Suite) TestAutoApproveRoutes(c *check.C) {
+func TestAutoApproveRoutes(t *testing.T) {
-	acl := []byte(`
+	tests := []struct {
+		name   string
+		acl    string
+		routes []netip.Prefix
+		want   []netip.Prefix
+	}{
+		{
+			name: "2068-approve-issue-sub",
+			acl: `
+{
+	"groups": {
+		"group:k8s": ["test"]
+	},
+
+	"acls": [
+		{"action": "accept", "users": ["*"], "ports": ["*:*"]},
+	],
+
+	"autoApprovers": {
+		"routes": {
+			"10.42.0.0/16": ["test"],
+		}
+	}
+}`,
+			routes: []netip.Prefix{netip.MustParsePrefix("10.42.7.0/24")},
+			want:   []netip.Prefix{netip.MustParsePrefix("10.42.7.0/24")},
+		},
+		{
+			name: "2068-approve-issue-sub",
+			acl: `
 {
 	"tagOwners": {
 		"tag:exit": ["test"],
@@ -540,28 +570,40 @@ func (s *Suite) TestAutoApproveRoutes(c *check.C) {
 			"10.11.0.0/16": ["test"],
 		}
 	}
+}`,
+			routes: []netip.Prefix{
+				netip.MustParsePrefix("0.0.0.0/0"),
+				netip.MustParsePrefix("::/0"),
+				netip.MustParsePrefix("10.10.0.0/16"),
+				netip.MustParsePrefix("10.11.0.0/24"),
+			},
+			want: []netip.Prefix{
+				netip.MustParsePrefix("::/0"),
+				netip.MustParsePrefix("10.11.0.0/24"),
+				netip.MustParsePrefix("10.10.0.0/16"),
+				netip.MustParsePrefix("0.0.0.0/0"),
+			},
+		},
 	}
-	`)
 
-	pol, err := policy.LoadACLPolicyFromBytes(acl)
+	for _, tt := range tests {
-	c.Assert(err, check.IsNil)
+		t.Run(tt.name, func(t *testing.T) {
-	c.Assert(pol, check.NotNil)
+			adb, err := newTestDB()
+			assert.NoError(t, err)
+			pol, err := policy.LoadACLPolicyFromBytes([]byte(tt.acl))
 
-	user, err := db.CreateUser("test")
+			assert.NoError(t, err)
-	c.Assert(err, check.IsNil)
+			assert.NotNil(t, pol)
 
-	pak, err := db.CreatePreAuthKey(user.Name, false, false, nil, nil)
+			user, err := adb.CreateUser("test")
-	c.Assert(err, check.IsNil)
+			assert.NoError(t, err)
+
+			pak, err := adb.CreatePreAuthKey(user.Name, false, false, nil, nil)
+			assert.NoError(t, err)
 
 			nodeKey := key.NewNode()
 			machineKey := key.NewMachine()
 
-	defaultRouteV4 := netip.MustParsePrefix("0.0.0.0/0")
-	defaultRouteV6 := netip.MustParsePrefix("::/0")
-	route1 := netip.MustParsePrefix("10.10.0.0/16")
-	// Check if a subprefix of an autoapproved route is approved
-	route2 := netip.MustParsePrefix("10.11.0.0/24")
-
 			v4 := netip.MustParseAddr("100.64.0.1")
 			node := types.Node{
 				ID:             0,
@@ -573,28 +615,38 @@ func (s *Suite) TestAutoApproveRoutes(c *check.C) {
 				AuthKeyID:      ptr.To(pak.ID),
 				Hostinfo: &tailcfg.Hostinfo{
 					RequestTags: []string{"tag:exit"},
-			RoutableIPs: []netip.Prefix{defaultRouteV4, defaultRouteV6, route1, route2},
+					RoutableIPs: tt.routes,
 				},
 				IPv4: &v4,
 			}
 
-	trx := db.DB.Save(&node)
+			trx := adb.DB.Save(&node)
-	c.Assert(trx.Error, check.IsNil)
+			assert.NoError(t, trx.Error)
 
-	sendUpdate, err := db.SaveNodeRoutes(&node)
+			sendUpdate, err := adb.SaveNodeRoutes(&node)
-	c.Assert(err, check.IsNil)
+			assert.NoError(t, err)
-	c.Assert(sendUpdate, check.Equals, false)
+			assert.False(t, sendUpdate)
 
-	node0ByID, err := db.GetNodeByID(0)
+			node0ByID, err := adb.GetNodeByID(0)
-	c.Assert(err, check.IsNil)
+			assert.NoError(t, err)
 
 			// TODO(kradalby): Check state update
-	err = db.EnableAutoApprovedRoutes(pol, node0ByID)
+			err = adb.EnableAutoApprovedRoutes(pol, node0ByID)
-	c.Assert(err, check.IsNil)
+			assert.NoError(t, err)
 
-	enabledRoutes, err := db.GetEnabledRoutes(node0ByID)
+			enabledRoutes, err := adb.GetEnabledRoutes(node0ByID)
-	c.Assert(err, check.IsNil)
+			assert.NoError(t, err)
-	c.Assert(enabledRoutes, check.HasLen, 4)
+			assert.Len(t, enabledRoutes, len(tt.want))
+
+			sort.Slice(enabledRoutes, func(i, j int) bool {
+				return util.ComparePrefix(enabledRoutes[i], enabledRoutes[j]) > 0
+			})
+
+			if diff := cmp.Diff(tt.want, enabledRoutes, util.Comparers...); diff != "" {
+				t.Errorf("unexpected enabled routes (-want +got):\n%s", diff)
+			}
+		})
+	}
 }
 
 func TestEphemeralGarbageCollectorOrder(t *testing.T) {

hscontrol/poll.go

@@ -1,12 +1,10 @@
 package hscontrol
 
 import (
-	"cmp"
 	"context"
 	"fmt"
 	"math/rand/v2"
 	"net/http"
-	"net/netip"
 	"sort"
 	"strings"
 	"time"
@@ -14,6 +12,7 @@ import (
 	"github.com/juanfont/headscale/hscontrol/db"
 	"github.com/juanfont/headscale/hscontrol/mapper"
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/util"
 	"github.com/rs/zerolog/log"
 	"github.com/sasha-s/go-deadlock"
 	xslices "golang.org/x/exp/slices"
@@ -742,10 +741,10 @@ func hostInfoChanged(old, new *tailcfg.Hostinfo) (bool, bool) {
 	newRoutes := new.RoutableIPs
 
 	sort.Slice(oldRoutes, func(i, j int) bool {
-		return comparePrefix(oldRoutes[i], oldRoutes[j]) > 0
+		return util.ComparePrefix(oldRoutes[i], oldRoutes[j]) > 0
 	})
 	sort.Slice(newRoutes, func(i, j int) bool {
-		return comparePrefix(newRoutes[i], newRoutes[j]) > 0
+		return util.ComparePrefix(newRoutes[i], newRoutes[j]) > 0
 	})
 
 	if !xslices.Equal(oldRoutes, newRoutes) {
@@ -764,19 +763,3 @@ func hostInfoChanged(old, new *tailcfg.Hostinfo) (bool, bool) {
 
 	return false, false
 }
-
-// TODO(kradalby): Remove after go 1.23, will be in stdlib.
-// Compare returns an integer comparing two prefixes.
-// The result will be 0 if p == p2, -1 if p < p2, and +1 if p > p2.
-// Prefixes sort first by validity (invalid before valid), then
-// address family (IPv4 before IPv6), then prefix length, then
-// address.
-func comparePrefix(p, p2 netip.Prefix) int {
-	if c := cmp.Compare(p.Addr().BitLen(), p2.Addr().BitLen()); c != 0 {
-		return c
-	}
-	if c := cmp.Compare(p.Bits(), p2.Bits()); c != 0 {
-		return c
-	}
-	return p.Addr().Compare(p2.Addr())
-}

hscontrol/util/net.go

@@ -1,8 +1,10 @@
 package util
 
 import (
+	"cmp"
 	"context"
 	"net"
+	"net/netip"
 )
 
 func GrpcSocketDialer(ctx context.Context, addr string) (net.Conn, error) {
@@ -10,3 +12,20 @@ func GrpcSocketDialer(ctx context.Context, addr string) (net.Conn, error) {
 
 	return d.DialContext(ctx, "unix", addr)
 }
+
+
+// TODO(kradalby): Remove after go 1.24, will be in stdlib.
+// Compare returns an integer comparing two prefixes.
+// The result will be 0 if p == p2, -1 if p < p2, and +1 if p > p2.
+// Prefixes sort first by validity (invalid before valid), then
+// address family (IPv4 before IPv6), then prefix length, then
+// address.
+func ComparePrefix(p, p2 netip.Prefix) int {
+	if c := cmp.Compare(p.Addr().BitLen(), p2.Addr().BitLen()); c != 0 {
+		return c
+	}
+	if c := cmp.Compare(p.Bits(), p2.Bits()); c != 0 {
+		return c
+	}
+	return p.Addr().Compare(p2.Addr())
+}

integration/route_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	v1 "github.com/juanfont/headscale/gen/go/headscale/v1"
 	"github.com/juanfont/headscale/hscontrol/policy"
 	"github.com/juanfont/headscale/hscontrol/util"
@@ -957,6 +958,95 @@ func TestEnableDisableAutoApprovedRoute(t *testing.T) {
 	assert.Equal(t, true, reAdvertisedRoutes[0].GetIsPrimary())
 }
 
+func TestAutoApprovedSubRoute2068(t *testing.T) {
+	IntegrationSkip(t)
+	t.Parallel()
+
+	expectedRoutes := "10.42.7.0/24"
+
+	user := "subroute"
+
+	scenario, err := NewScenario(dockertestMaxWait())
+	assertNoErrf(t, "failed to create scenario: %s", err)
+	defer scenario.Shutdown()
+
+	spec := map[string]int{
+		user: 1,
+	}
+
+	err = scenario.CreateHeadscaleEnv(spec, []tsic.Option{tsic.WithTags([]string{"tag:approve"})}, hsic.WithTestName("clienableroute"), hsic.WithACLPolicy(
+		&policy.ACLPolicy{
+			ACLs: []policy.ACL{
+				{
+					Action:       "accept",
+					Sources:      []string{"*"},
+					Destinations: []string{"*:*"},
+				},
+			},
+			TagOwners: map[string][]string{
+				"tag:approve": {user},
+			},
+			AutoApprovers: policy.AutoApprovers{
+				Routes: map[string][]string{
+					"10.42.0.0/16": {"tag:approve"},
+				},
+			},
+		},
+	))
+	assertNoErrHeadscaleEnv(t, err)
+
+	allClients, err := scenario.ListTailscaleClients()
+	assertNoErrListClients(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	assertNoErrSync(t, err)
+
+	headscale, err := scenario.Headscale()
+	assertNoErrGetHeadscale(t, err)
+
+	subRouter1 := allClients[0]
+
+	// Initially advertise route
+	command := []string{
+		"tailscale",
+		"set",
+		"--advertise-routes=" + expectedRoutes,
+	}
+	_, _, err = subRouter1.Execute(command)
+	assertNoErrf(t, "failed to advertise route: %s", err)
+
+	time.Sleep(10 * time.Second)
+
+	var routes []*v1.Route
+	err = executeAndUnmarshal(
+		headscale,
+		[]string{
+			"headscale",
+			"routes",
+			"list",
+			"--output",
+			"json",
+		},
+		&routes,
+	)
+	assertNoErr(t, err)
+	assert.Len(t, routes, 1)
+
+	want := []*v1.Route{
+		{
+			Id:         1,
+			Prefix:     expectedRoutes,
+			Advertised: true,
+			Enabled:    true,
+			IsPrimary:  true,
+		},
+	}
+
+	if diff := cmp.Diff(want, routes, cmpopts.IgnoreUnexported(v1.Route{}), cmpopts.IgnoreFields(v1.Route{}, "Node", "CreatedAt", "UpdatedAt", "DeletedAt")); diff != "" {
+		t.Errorf("unexpected routes (-want +got):\n%s", diff)
+	}
+}
+
 // TestSubnetRouteACL verifies that Subnet routes are distributed
 // as expected when ACLs are activated.
 // It implements the issue from