feat: add autogroup:member, autogroup:tagged #2572

vdovhanych · 2025-05-07T18:31:32Z

This implements two autogroups autogroup:member and autogroup:tagged. I talked with @kradalby about leaving out autogroup:self for now, as that is a bit trickier in the end.

The logic of the autogroups and test is taken from the draft pr #2230, but it has been implemented for the new policy structure.

have read the CONTRIBUTING.md file
raised a GitHub issue or discussed it on the projects chat beforehand
added unit tests
added integration tests
updated documentation if needed
updated CHANGELOG.md

review-ai-agent · 2025-05-07T18:31:35Z

Pull Request Revisions

Revision	Description
r3	Added support for autogroup member, tagged Implemented `autogroup:member` and `autogroup:tagged` in policy system, updating ACL resolving logic and test coverage
r2	Enhance autogroup member and tagged logic Improved `autogroup:member` and `autogroup:tagged` resolution with more nuanced tag handling and ownership checks
r1	New autogroups support added now Implemented `autogroup:member` and `autogroup:tagged` in policy resolution, updating ACL handling and test coverage

✅ AI review completed for r3

Help

React with emojis to give feedback on AI-generated reviews:

👍 means the feedback was helpful and actionable
👎 means the feedback was incorrect or unhelpful

💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

review-ai-agent · 2025-05-07T18:32:25Z

hscontrol/policy/v2/types.go

+func allIPs() *netipx.IPSet {
+	if allIPSet != nil {
+		return allIPSet
+	}
+
+	var build netipx.IPSetBuilder
+	build.AddPrefix(netip.MustParsePrefix("::/0"))
+	build.AddPrefix(netip.MustParsePrefix("0.0.0.0/0"))
+
+	allTheIps, _ := build.IPSet()
+
+	return allTheIps
+}


In allIPs() function, the IPSet is being built but not stored in the global allIPSet variable. This means the caching mechanism isn't working - the function always builds a new set even if it's already been created.

hscontrol/policy/v2/types.go

review-ai-agent · 2025-05-07T18:32:25Z

integration/acl_test.go

+				},
+			},
+		},
+		2,
+	)
+	defer scenario.ShutdownAssertNoPanics(t)
+
+	allClients, err := scenario.ListTailscaleClients()
+	require.NoError(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	require.NoError(t, err)
+
+	// Test that untagged nodes can access each other
+	for _, client := range allClients {
+		status, err := client.Status()
+		require.NoError(t, err)
+		if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+			continue
+		}
+
+		for _, peer := range allClients {
+			if client.Hostname() == peer.Hostname() {
+				continue
+			}
+
+			status, err := peer.Status()
+			require.NoError(t, err)
+			if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+				continue
+			}
+
+			fqdn, err := peer.FQDN()
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/etc/hostname", fqdn)
+			t.Logf("url from %s to %s", client.Hostname(), url)
+
+			result, err := client.Curl(url)
+			assert.Len(t, result, 13)
+			require.NoError(t, err)
+		}
+	}
+}
+
+func TestACLAutogroupTagged(t *testing.T) {
+	IntegrationSkip(t)
+	t.Parallel()
+
+	scenario := aclScenario(t,
+		&policyv1.ACLPolicy{
+			ACLs: []policyv1.ACL{
+				{
+					Action:       "accept",
+					Sources:      []string{"autogroup:tagged"},
+					Destinations: []string{"autogroup:tagged:*"},
+				},
+			},
+		},
+		2,
+	)
+	defer scenario.ShutdownAssertNoPanics(t)
+
+	allClients, err := scenario.ListTailscaleClients()
+	require.NoError(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	require.NoError(t, err)
+
+	// Test that tagged nodes can access each other
+	for _, client := range allClients {
+		status, err := client.Status()
+		require.NoError(t, err)
+		if status.Self.Tags == nil || status.Self.Tags.Len() == 0 {
+			continue
+		}
+
+		for _, peer := range allClients {
+			if client.Hostname() == peer.Hostname() {
+				continue
+			}
+
+			status, err := peer.Status()
+			require.NoError(t, err)
+			if status.Self.Tags == nil || status.Self.Tags.Len() == 0 {
+				continue
+			}
+
+			fqdn, err := peer.FQDN()
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/etc/hostname", fqdn)
+			t.Logf("url from %s to %s", client.Hostname(), url)
+
+			result, err := client.Curl(url)
+			assert.Len(t, result, 13)
+			require.NoError(t, err)
+		}
+	}
+}


The integration tests for autogroup:member and autogroup:tagged have duplicated code structures. Consider extracting a common test helper function that takes the autogroup type and condition check as parameters to reduce duplication and increase maintainability.

kradalby · 2025-05-09T05:09:54Z

CHANGELOG.md

@@ -122,6 +122,7 @@ working in v1 and not tested might be broken in v2 (and vice versa).
  [#2438](https://github.com/juanfont/headscale/pull/2438)
 - Add documentation for routes
  [#2496](https://github.com/juanfont/headscale/pull/2496)
+- Add support for `autogroup:member`, `autogroup:tagged`


Lets add the PR to this

done in 2b3ee90

hscontrol/policy/v2/types.go

hscontrol/policy/v2/types_test.go

kradalby · 2025-05-09T05:32:08Z

Great start!

review-ai-agent · 2025-05-09T12:52:02Z

integration/acl_test.go

+	require.NoError(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	require.NoError(t, err)
+
+	// Test that untagged nodes can access each other
+	for _, client := range allClients {
+		status, err := client.Status()
+		require.NoError(t, err)
+		if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+			continue
+		}
+
+		for _, peer := range allClients {
+			if client.Hostname() == peer.Hostname() {
+				continue
+			}
+
+			status, err := peer.Status()
+			require.NoError(t, err)
+			if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+				continue
+			}
+
+			fqdn, err := peer.FQDN()
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/etc/hostname", fqdn)
+			t.Logf("url from %s to %s", client.Hostname(), url)
+
+			result, err := client.Curl(url)
+			assert.Len(t, result, 13)
+			require.NoError(t, err)


Both TestACLAutogroupMember and TestACLAutogroupTagged contain similar error handling and test setup logic. Consider extracting a helper function to reduce duplication and improve maintainability.

review-ai-agent · 2025-05-09T12:52:03Z

hscontrol/policy/v2/types.go

The error message in validateAutogroupForSSHDst incorrectly states 'for SSH sources' but should say 'for SSH destinations' to match the function's purpose.

vdovhanych

Most of the comments are for me to better understand the logic, i was getting lost in it a bit.

vdovhanych · 2025-05-09T12:51:38Z

CHANGELOG.md

@@ -122,6 +122,7 @@ working in v1 and not tested might be broken in v2 (and vice versa).
  [#2438](https://github.com/juanfont/headscale/pull/2438)
 - Add documentation for routes
  [#2496](https://github.com/juanfont/headscale/pull/2496)
+- Add support for `autogroup:member`, `autogroup:tagged`


done in 2b3ee90

hscontrol/policy/v2/types.go

hscontrol/policy/v2/types_test.go

kradalby

I think this looks great, I am happy to merge this and add more tests as we continue testing.

vdovhanych · 2025-05-16T09:04:57Z

I think this looks great, I am happy to merge this and add more tests as we continue testing.

Okay, cool. I will squash the fixup commit.
Also, @kradalby do you think I should remove some comments from the code? They were more for me than something essential to have there.

kradalby · 2025-05-16T09:09:48Z

None of them seem to out of place, let's leave them and then we can update them as needed

kradalby · 2025-05-16T09:10:12Z

I'll run the tests when you have squashed and if all passes I'll merge it

vdovhanych · 2025-05-16T10:34:48Z

Squashed

review-ai-agent · 2025-05-16T10:35:42Z

integration/acl_test.go

+				},
+			},
+		},
+		2,
+	)
+	defer scenario.ShutdownAssertNoPanics(t)
+
+	allClients, err := scenario.ListTailscaleClients()
+	require.NoError(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	require.NoError(t, err)
+
+	// Test that untagged nodes can access each other
+	for _, client := range allClients {
+		status, err := client.Status()
+		require.NoError(t, err)
+		if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+			continue
+		}
+
+		for _, peer := range allClients {
+			if client.Hostname() == peer.Hostname() {
+				continue
+			}
+
+			status, err := peer.Status()
+			require.NoError(t, err)
+			if status.Self.Tags != nil && status.Self.Tags.Len() > 0 {
+				continue
+			}
+
+			fqdn, err := peer.FQDN()
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/etc/hostname", fqdn)
+			t.Logf("url from %s to %s", client.Hostname(), url)
+
+			result, err := client.Curl(url)
+			assert.Len(t, result, 13)
+			require.NoError(t, err)
+		}
+	}
+}
+
+func TestACLAutogroupTagged(t *testing.T) {
+	IntegrationSkip(t)
+	t.Parallel()
+
+	scenario := aclScenario(t,
+		&policyv1.ACLPolicy{
+			ACLs: []policyv1.ACL{
+				{
+					Action:       "accept",
+					Sources:      []string{"autogroup:tagged"},
+					Destinations: []string{"autogroup:tagged:*"},
+				},
+			},
+		},
+		2,
+	)
+	defer scenario.ShutdownAssertNoPanics(t)
+
+	allClients, err := scenario.ListTailscaleClients()
+	require.NoError(t, err)
+
+	err = scenario.WaitForTailscaleSync()
+	require.NoError(t, err)
+
+	// Test that tagged nodes can access each other
+	for _, client := range allClients {
+		status, err := client.Status()
+		require.NoError(t, err)
+		if status.Self.Tags == nil || status.Self.Tags.Len() == 0 {
+			continue
+		}
+
+		for _, peer := range allClients {
+			if client.Hostname() == peer.Hostname() {
+				continue
+			}
+
+			status, err := peer.Status()
+			require.NoError(t, err)
+			if status.Self.Tags == nil || status.Self.Tags.Len() == 0 {
+				continue
+			}
+
+			fqdn, err := peer.FQDN()
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/etc/hostname", fqdn)
+			t.Logf("url from %s to %s", client.Hostname(), url)
+
+			result, err := client.Curl(url)
+			assert.Len(t, result, 13)
+			require.NoError(t, err)
+		}
+	}
+}


The integration tests for autogroup:member and autogroup:tagged have nearly identical structure and logic. Consider extracting a common test helper function that takes the autogroup type and filter conditions as parameters to reduce duplication.

review-ai-agent · 2025-05-16T10:35:43Z

hscontrol/policy/v2/types.go

In the validateAutogroupForSSHDst function, the error message incorrectly refers to 'SSH sources' instead of 'SSH destinations', which could be confusing during troubleshooting.

vdovhanych requested review from ohdearaugustin, nblock, juanfont and kradalby as code owners May 7, 2025 18:31

review-ai-agent bot approved these changes May 7, 2025

View reviewed changes

kradalby requested changes May 9, 2025

View reviewed changes

review-ai-agent bot approved these changes May 9, 2025

View reviewed changes

review-ai-agent bot reviewed May 9, 2025

View reviewed changes

vdovhanych commented May 9, 2025

View reviewed changes

vdovhanych requested a review from kradalby May 13, 2025 12:09

kradalby approved these changes May 16, 2025

View reviewed changes

feat: add autogroup:member, autogroup:tagged

54d7e61

vdovhanych force-pushed the add-autogroup-member-and-tagged branch from 2b3ee90 to 54d7e61 Compare May 16, 2025 10:34

review-ai-agent bot approved these changes May 16, 2025

View reviewed changes

review-ai-agent bot reviewed May 16, 2025

View reviewed changes

kradalby merged commit 6750414 into juanfont:main May 17, 2025
277 of 284 checks passed

kradalby mentioned this pull request May 21, 2025

Policy: autogroup support #2618

Open

5 tasks

Uh oh!

feat: add autogroup:member, autogroup:tagged #2572

feat: add autogroup:member, autogroup:tagged #2572

Conversation

vdovhanych commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

review-ai-agent bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Revisions

Uh oh!

review-ai-agent bot May 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

review-ai-agent bot May 7, 2025

Choose a reason for hiding this comment

Uh oh!

kradalby May 9, 2025

Choose a reason for hiding this comment

Uh oh!

vdovhanych May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kradalby commented May 9, 2025

Uh oh!

review-ai-agent bot May 9, 2025

Choose a reason for hiding this comment

Uh oh!

review-ai-agent bot May 9, 2025

Choose a reason for hiding this comment

Uh oh!

vdovhanych left a comment

Choose a reason for hiding this comment

Uh oh!

vdovhanych May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kradalby left a comment

Choose a reason for hiding this comment

Uh oh!

vdovhanych commented May 16, 2025

Uh oh!

kradalby commented May 16, 2025

Uh oh!

kradalby commented May 16, 2025

Uh oh!

vdovhanych commented May 16, 2025

Uh oh!

review-ai-agent bot May 16, 2025

Choose a reason for hiding this comment

Uh oh!

review-ai-agent bot May 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vdovhanych commented May 7, 2025 •

edited

Loading

review-ai-agent bot commented May 7, 2025 •

edited

Loading