From f368ed01ed18b1d9388879f17f4e78d29218fbd9 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 5 Sep 2024 16:46:20 +0200 Subject: [PATCH] 2068 AutoApprovers tests (#2105) * replace old suite approved routes test with table driven Signed-off-by: Kristoffer Dalby * add test to reproduce issue Signed-off-by: Kristoffer Dalby * add integration test for 2068 Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- .github/workflows/test-integration.yaml | 1 + hscontrol/db/node_test.go | 152 ++++++++++++++++-------- hscontrol/poll.go | 23 +--- hscontrol/util/net.go | 19 +++ integration/route_test.go | 90 ++++++++++++++ 5 files changed, 215 insertions(+), 70 deletions(-) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index d5b362b7..ed194da1 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -55,6 +55,7 @@ jobs: - TestEnablingRoutes - TestHASubnetRouterFailover - TestEnableDisableAutoApprovedRoute + - TestAutoApprovedSubRoute2068 - TestSubnetRouteACL - TestHeadscale - TestCreateTailscale diff --git a/hscontrol/db/node_test.go b/hscontrol/db/node_test.go index c83da120..94cce13b 100644 --- a/hscontrol/db/node_test.go +++ b/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) { - acl := []byte(` +func TestAutoApproveRoutes(t *testing.T) { + 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,61 +570,83 @@ func (s *Suite) TestAutoApproveRoutes(c *check.C) { "10.11.0.0/16": ["test"], } } -} - `) - - pol, err := policy.LoadACLPolicyFromBytes(acl) - c.Assert(err, check.IsNil) - c.Assert(pol, check.NotNil) - - user, err := db.CreateUser("test") - c.Assert(err, check.IsNil) - - pak, err := db.CreatePreAuthKey(user.Name, false, false, nil, nil) - c.Assert(err, check.IsNil) - - 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, - MachineKey: machineKey.Public(), - NodeKey: nodeKey.Public(), - Hostname: "test", - UserID: user.ID, - RegisterMethod: util.RegisterMethodAuthKey, - AuthKeyID: ptr.To(pak.ID), - Hostinfo: &tailcfg.Hostinfo{ - RequestTags: []string{"tag:exit"}, - RoutableIPs: []netip.Prefix{defaultRouteV4, defaultRouteV6, route1, route2}, +}`, + 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"), + }, }, - IPv4: &v4, } - trx := db.DB.Save(&node) - c.Assert(trx.Error, check.IsNil) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + adb, err := newTestDB() + assert.NoError(t, err) + pol, err := policy.LoadACLPolicyFromBytes([]byte(tt.acl)) - sendUpdate, err := db.SaveNodeRoutes(&node) - c.Assert(err, check.IsNil) - c.Assert(sendUpdate, check.Equals, false) + assert.NoError(t, err) + assert.NotNil(t, pol) - node0ByID, err := db.GetNodeByID(0) - c.Assert(err, check.IsNil) + user, err := adb.CreateUser("test") + assert.NoError(t, err) - // TODO(kradalby): Check state update - err = db.EnableAutoApprovedRoutes(pol, node0ByID) - c.Assert(err, check.IsNil) + pak, err := adb.CreatePreAuthKey(user.Name, false, false, nil, nil) + assert.NoError(t, err) - enabledRoutes, err := db.GetEnabledRoutes(node0ByID) - c.Assert(err, check.IsNil) - c.Assert(enabledRoutes, check.HasLen, 4) + nodeKey := key.NewNode() + machineKey := key.NewMachine() + + v4 := netip.MustParseAddr("100.64.0.1") + node := types.Node{ + ID: 0, + MachineKey: machineKey.Public(), + NodeKey: nodeKey.Public(), + Hostname: "test", + UserID: user.ID, + RegisterMethod: util.RegisterMethodAuthKey, + AuthKeyID: ptr.To(pak.ID), + Hostinfo: &tailcfg.Hostinfo{ + RequestTags: []string{"tag:exit"}, + RoutableIPs: tt.routes, + }, + IPv4: &v4, + } + + trx := adb.DB.Save(&node) + assert.NoError(t, trx.Error) + + sendUpdate, err := adb.SaveNodeRoutes(&node) + assert.NoError(t, err) + assert.False(t, sendUpdate) + + node0ByID, err := adb.GetNodeByID(0) + assert.NoError(t, err) + + // TODO(kradalby): Check state update + err = adb.EnableAutoApprovedRoutes(pol, node0ByID) + assert.NoError(t, err) + + enabledRoutes, err := adb.GetEnabledRoutes(node0ByID) + assert.NoError(t, err) + 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) { diff --git a/hscontrol/poll.go b/hscontrol/poll.go index b9bf65a2..d7ba682e 100644 --- a/hscontrol/poll.go +++ b/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()) -} diff --git a/hscontrol/util/net.go b/hscontrol/util/net.go index b704c936..c44b7287 100644 --- a/hscontrol/util/net.go +++ b/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()) +} diff --git a/integration/route_test.go b/integration/route_test.go index a92258af..0252e702 100644 --- a/integration/route_test.go +++ b/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