From a592ae56b45e2bbe032999afc323ef60c4feed3c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 4 Jan 2024 21:26:49 +0100 Subject: [PATCH] fix issue where advertise tags causes hang (#1669) Fixes #1665 Signed-off-by: Kristoffer Dalby --- ...n-v2-TestNodeAdvertiseTagNoACLCommand.yaml | 67 ++++++++++ ...v2-TestNodeAdvertiseTagWithACLCommand.yaml | 67 ++++++++++ hscontrol/policy/acls.go | 14 ++- integration/cli_test.go | 114 ++++++++++++++++++ 4 files changed, 257 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/test-integration-v2-TestNodeAdvertiseTagNoACLCommand.yaml create mode 100644 .github/workflows/test-integration-v2-TestNodeAdvertiseTagWithACLCommand.yaml diff --git a/.github/workflows/test-integration-v2-TestNodeAdvertiseTagNoACLCommand.yaml b/.github/workflows/test-integration-v2-TestNodeAdvertiseTagNoACLCommand.yaml new file mode 100644 index 00000000..f51fa612 --- /dev/null +++ b/.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" diff --git a/.github/workflows/test-integration-v2-TestNodeAdvertiseTagWithACLCommand.yaml b/.github/workflows/test-integration-v2-TestNodeAdvertiseTagWithACLCommand.yaml new file mode 100644 index 00000000..9e0fcd28 --- /dev/null +++ b/.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" diff --git a/hscontrol/policy/acls.go b/hscontrol/policy/acls.go index 11f280ac..4798d818 100644 --- a/hscontrol/policy/acls.go +++ b/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( - "%w. %v isn't owned by a TagOwner. Please add one first. https://tailscale.com/kb/1018/acls/#tag-owners", - ErrInvalidTag, - tag, - ) + return []string{}, noTagErr } for _, owner := range ows { if isGroup(owner) { diff --git a/integration/cli_test.go b/integration/cli_test.go index 0ff0ffca..d2d741e0 100644 --- a/integration/cli_test.go +++ b/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()