fix issue where advertise tags causes hang (#1669)

Fixes #1665

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

.github/workflows/test-integration-v2-TestNodeAdvertiseTagNoACLCommand.yaml

@@ -0,0 +1,67 @@
+# DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go
+# To regenerate, run "go generate" in cmd/gh-action-integration-generator/
+
+name: Integration Test v2 - TestNodeAdvertiseTagNoACLCommand
+
+on: [pull_request]
+
+concurrency:
+  group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  TestNodeAdvertiseTagNoACLCommand:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 2
+
+      - uses: DeterminateSystems/nix-installer-action@main
+      - uses: DeterminateSystems/magic-nix-cache-action@main
+      - uses: satackey/action-docker-layer-caching@main
+        continue-on-error: true
+
+      - name: Get changed files
+        id: changed-files
+        uses: tj-actions/changed-files@v34
+        with:
+          files: |
+            *.nix
+            go.*
+            **/*.go
+            integration_test/
+            config-example.yaml
+
+      - name: Run TestNodeAdvertiseTagNoACLCommand
+        uses: Wandalen/wretry.action@master
+        if: steps.changed-files.outputs.any_changed == 'true'
+        with:
+          attempt_limit: 5
+          command: |
+            nix develop --command -- docker run \
+              --tty --rm \
+              --volume ~/.cache/hs-integration-go:/go \
+              --name headscale-test-suite \
+              --volume $PWD:$PWD -w $PWD/integration \
+              --volume /var/run/docker.sock:/var/run/docker.sock \
+              --volume $PWD/control_logs:/tmp/control \
+              golang:1 \
+                go run gotest.tools/gotestsum@latest -- ./... \
+                  -failfast \
+                  -timeout 120m \
+                  -parallel 1 \
+                  -run "^TestNodeAdvertiseTagNoACLCommand$"
+
+      - uses: actions/upload-artifact@v3
+        if: always() && steps.changed-files.outputs.any_changed == 'true'
+        with:
+          name: logs
+          path: "control_logs/*.log"
+
+      - uses: actions/upload-artifact@v3
+        if: always() && steps.changed-files.outputs.any_changed == 'true'
+        with:
+          name: pprof
+          path: "control_logs/*.pprof.tar"

.github/workflows/test-integration-v2-TestNodeAdvertiseTagWithACLCommand.yaml

@@ -0,0 +1,67 @@
+# DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go
+# To regenerate, run "go generate" in cmd/gh-action-integration-generator/
+
+name: Integration Test v2 - TestNodeAdvertiseTagWithACLCommand
+
+on: [pull_request]
+
+concurrency:
+  group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }}
+  cancel-in-progress: true
+
+jobs:
+  TestNodeAdvertiseTagWithACLCommand:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 2
+
+      - uses: DeterminateSystems/nix-installer-action@main
+      - uses: DeterminateSystems/magic-nix-cache-action@main
+      - uses: satackey/action-docker-layer-caching@main
+        continue-on-error: true
+
+      - name: Get changed files
+        id: changed-files
+        uses: tj-actions/changed-files@v34
+        with:
+          files: |
+            *.nix
+            go.*
+            **/*.go
+            integration_test/
+            config-example.yaml
+
+      - name: Run TestNodeAdvertiseTagWithACLCommand
+        uses: Wandalen/wretry.action@master
+        if: steps.changed-files.outputs.any_changed == 'true'
+        with:
+          attempt_limit: 5
+          command: |
+            nix develop --command -- docker run \
+              --tty --rm \
+              --volume ~/.cache/hs-integration-go:/go \
+              --name headscale-test-suite \
+              --volume $PWD:$PWD -w $PWD/integration \
+              --volume /var/run/docker.sock:/var/run/docker.sock \
+              --volume $PWD/control_logs:/tmp/control \
+              golang:1 \
+                go run gotest.tools/gotestsum@latest -- ./... \
+                  -failfast \
+                  -timeout 120m \
+                  -parallel 1 \
+                  -run "^TestNodeAdvertiseTagWithACLCommand$"
+
+      - uses: actions/upload-artifact@v3
+        if: always() && steps.changed-files.outputs.any_changed == 'true'
+        with:
+          name: logs
+          path: "control_logs/*.log"
+
+      - uses: actions/upload-artifact@v3
+        if: always() && steps.changed-files.outputs.any_changed == 'true'
+        with:
+          name: pprof
+          path: "control_logs/*.pprof.tar"

hscontrol/policy/acls.go

@@ -674,14 +674,18 @@ func expandOwnersFromTag(
 	pol *ACLPolicy,
 	tag string,
 ) ([]string, error) {
+	noTagErr := fmt.Errorf(
+		"%w. %v isn't owned by a TagOwner. Please add one first. https://tailscale.com/kb/1018/acls/#tag-owners",
+		ErrInvalidTag,
+		tag,
+	)
+	if pol == nil {
+		return []string{}, noTagErr
+	}
 	var owners []string
 	ows, ok := pol.TagOwners[tag]
 	if !ok {
-		return []string{}, fmt.Errorf(
+		return []string{}, noTagErr
-			"%w. %v isn't owned by a TagOwner. Please add one first. https://tailscale.com/kb/1018/acls/#tag-owners",
-			ErrInvalidTag,
-			tag,
-		)
 	}
 	for _, owner := range ows {
 		if isGroup(owner) {

integration/cli_test.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	v1 "github.com/juanfont/headscale/gen/go/headscale/v1"
+	"github.com/juanfont/headscale/hscontrol/policy"
 	"github.com/juanfont/headscale/integration/hsic"
 	"github.com/juanfont/headscale/integration/tsic"
 	"github.com/stretchr/testify/assert"
@@ -665,6 +666,119 @@ func TestNodeTagCommand(t *testing.T) {
 	)
 }
 
+func TestNodeAdvertiseTagNoACLCommand(t *testing.T) {
+	IntegrationSkip(t)
+	t.Parallel()
+
+	scenario, err := NewScenario()
+	assertNoErr(t, err)
+	defer scenario.Shutdown()
+
+	spec := map[string]int{
+		"user1": 1,
+	}
+
+	err = scenario.CreateHeadscaleEnv(spec, []tsic.Option{tsic.WithTags([]string{"tag:test"})}, hsic.WithTestName("cliadvtags"))
+	assertNoErr(t, err)
+
+	headscale, err := scenario.Headscale()
+	assertNoErr(t, err)
+
+	// Test list all nodes after added seconds
+	resultMachines := make([]*v1.Node, spec["user1"])
+	err = executeAndUnmarshal(
+		headscale,
+		[]string{
+			"headscale",
+			"nodes",
+			"list",
+			"--tags",
+			"--output", "json",
+		},
+		&resultMachines,
+	)
+	assert.Nil(t, err)
+	found := false
+	for _, node := range resultMachines {
+		if node.GetInvalidTags() != nil {
+			for _, tag := range node.GetInvalidTags() {
+				if tag == "tag:test" {
+					found = true
+				}
+			}
+		}
+	}
+	assert.Equal(
+		t,
+		true,
+		found,
+		"should not find a node with the tag 'tag:test' in the list of nodes",
+	)
+}
+
+func TestNodeAdvertiseTagWithACLCommand(t *testing.T) {
+	IntegrationSkip(t)
+	t.Parallel()
+
+	scenario, err := NewScenario()
+	assertNoErr(t, err)
+	defer scenario.Shutdown()
+
+	spec := map[string]int{
+		"user1": 1,
+	}
+
+	err = scenario.CreateHeadscaleEnv(spec, []tsic.Option{tsic.WithTags([]string{"tag:exists"})}, hsic.WithTestName("cliadvtags"), hsic.WithACLPolicy(
+		&policy.ACLPolicy{
+			ACLs: []policy.ACL{
+				{
+					Action:       "accept",
+					Sources:      []string{"*"},
+					Destinations: []string{"*:*"},
+				},
+			},
+			TagOwners: map[string][]string{
+				"tag:exists": {"user1"},
+			},
+		},
+	))
+	assertNoErr(t, err)
+
+	headscale, err := scenario.Headscale()
+	assertNoErr(t, err)
+
+	// Test list all nodes after added seconds
+	resultMachines := make([]*v1.Node, spec["user1"])
+	err = executeAndUnmarshal(
+		headscale,
+		[]string{
+			"headscale",
+			"nodes",
+			"list",
+			"--tags",
+			"--output", "json",
+		},
+		&resultMachines,
+	)
+	assert.Nil(t, err)
+	found := false
+	for _, node := range resultMachines {
+		if node.GetValidTags() != nil {
+			for _, tag := range node.GetValidTags() {
+				if tag == "tag:exists" {
+					found = true
+				}
+			}
+		}
+	}
+	assert.Equal(
+		t,
+		true,
+		found,
+		"should not find a node with the tag 'tag:exists' in the list of nodes",
+	)
+}
+
 func TestNodeCommand(t *testing.T) {
 	IntegrationSkip(t)
 	t.Parallel()