From aea64b34dea9be922cb5c11bbf5c398381ea7401 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 13 May 2026 09:57:54 +0000 Subject: [PATCH] policy/v2: tolerate tag-owner cycles by resolving to empty SaaS accepts tagOwners with circular references (tag:a -> tag:b -> tag:a, tag:a -> tag:a) by dropping the cycle edge instead of failing the policy. Each tag whose only path back is via the cycle resolves to the empty owner set; a sibling non-tag owner survives. headscale previously rejected the policy with ErrCircularReference at parse time. Drop the sentinel, change flattenTags to return an empty owner list on revisit, and update flattenTagOwners tests to capture the new behaviour including the sibling-survives edge case. --- hscontrol/policy/v2/policy.go | 29 ++++------ .../v2/tailscale_ssh_data_compat_test.go | 12 ----- hscontrol/policy/v2/types.go | 2 - hscontrol/policy/v2/types_test.go | 53 +++++++++++++++++-- 4 files changed, 58 insertions(+), 38 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 92b4d789..6b9c1d62 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1478,27 +1478,18 @@ func (pm *PolicyManager) invalidateGlobalPolicyCache(newNodes views.Slice[types. } } -// flattenTags flattens the TagOwners by resolving nested tags and detecting cycles. -// It will return a Owners list where all the Tag types have been resolved to their underlying Owners. +// flattenTags flattens the TagOwners by resolving nested tags. Cycles +// in the ownership graph (tag:a -> tag:b -> tag:a, or tag:a -> tag:a) +// are tolerated to match SaaS: the cycle-causing edge is dropped, the +// remaining owners propagate, and the cycle itself contributes no +// addresses. Non-cycle owners on the cycled tags still resolve. +// Undefined-tag references remain a hard error. func flattenTags(tagOwners TagOwners, tag Tag, visiting map[Tag]bool, chain []Tag) (Owners, error) { if visiting[tag] { - cycleStart := 0 - - for i, t := range chain { - if t == tag { - cycleStart = i - break - } - } - - cycleTags := make([]string, len(chain[cycleStart:])) - for i, t := range chain[cycleStart:] { - cycleTags[i] = string(t) - } - - slices.Sort(cycleTags) - - return nil, fmt.Errorf("%w: %s", ErrCircularReference, strings.Join(cycleTags, " -> ")) + // Cycle: this tag is already on the resolution stack. SaaS + // drops the edge instead of failing, so we return an empty + // owner set and let the caller continue with any siblings. + return nil, nil } visiting[tag] = true diff --git a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go index ce596d6a..a0bdc171 100644 --- a/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go +++ b/hscontrol/policy/v2/tailscale_ssh_data_compat_test.go @@ -87,18 +87,6 @@ var sshSkipReasons = map[string]string{ // equivalent for the user:*@passkey wildcard pattern. "ssh-b5": "user:*@passkey wildcard not supported in headscale", "ssh-d10": "user:*@passkey wildcard not supported in headscale", - - // TAG_OWNER_CYCLE_TOLERATED (2 tests) - // - // SaaS accepts tagOwners with circular references (tag:a -> tag:b, - // tag:b -> tag:a or tag:a -> tag:a) and resolves them as the empty - // owner set. headscale's TagOwnerSet resolver eagerly detects - // circular tag references and rejects the policy at parse time. - // Pick a side and adjust: either match SaaS by treating cycles as - // "no owners" instead of an error, or document that headscale is - // intentionally stricter and remove these scenarios. - "ssh-tag-owner-cycle": "headscale rejects tagOwner cycles that SaaS accepts as empty owner set", - "ssh-tag-owner-self-reference": "headscale rejects self-referencing tagOwner that SaaS accepts as empty owner set", } // sshRejectSkipReasons documents APIResponseCode != 200 captures where diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 4cb30438..2c882224 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -34,8 +34,6 @@ const Wildcard = Asterix(0) var ErrAutogroupSelfRequiresPerNodeResolution = errors.New("autogroup:self requires per-node resolution and cannot be resolved in this context") -var ErrCircularReference = errors.New("circular reference detected") - var ErrUndefinedTagReference = errors.New("references undefined tag") // SSH validation errors. diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 28161bac..24a14ec3 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -4153,13 +4153,45 @@ func TestFlattenTagOwners(t *testing.T) { wantErr: "", }, { - name: "circular-reference", + // SaaS tolerates tag:a <-> tag:b cycles by dropping the + // cycle edge; both tags resolve to an empty owner set + // because neither chain reaches a non-tag owner. + name: "circular-reference-resolves-to-empty", input: TagOwners{ Tag("tag:a"): Owners{new(Tag("tag:b"))}, Tag("tag:b"): Owners{new(Tag("tag:a"))}, }, - want: nil, - wantErr: "circular reference detected: tag:a -> tag:b", + want: TagOwners{ + Tag("tag:a"): nil, + Tag("tag:b"): nil, + }, + wantErr: "", + }, + { + // tag:a -> tag:a self-reference: the only owner is the + // cycle edge itself; result is empty. + name: "self-reference-resolves-to-empty", + input: TagOwners{ + Tag("tag:a"): Owners{new(Tag("tag:a"))}, + }, + want: TagOwners{ + Tag("tag:a"): nil, + }, + wantErr: "", + }, + { + // Cycle plus a sibling non-tag owner: the cycle edge + // drops out, the sibling owner survives. + name: "cycle-plus-sibling-keeps-sibling", + input: TagOwners{ + Tag("tag:a"): Owners{new(Tag("tag:b")), new(Username("alice@example.com"))}, + Tag("tag:b"): Owners{new(Tag("tag:a"))}, + }, + want: TagOwners{ + Tag("tag:a"): Owners{new(Username("alice@example.com"))}, + Tag("tag:b"): Owners{new(Username("alice@example.com"))}, + }, + wantErr: "", }, { name: "mixed-owners", @@ -4218,7 +4250,9 @@ func TestFlattenTagOwners(t *testing.T) { wantErr: "", }, { - name: "tag-long-circular-chain", + // Long cycle: every tag eventually points back to itself. + // Each tag resolves to the empty owner set. + name: "tag-long-circular-chain-resolves-to-empty", input: TagOwners{ Tag("tag:a"): Owners{new(Tag("tag:g"))}, Tag("tag:b"): Owners{new(Tag("tag:a"))}, @@ -4228,7 +4262,16 @@ func TestFlattenTagOwners(t *testing.T) { Tag("tag:f"): Owners{new(Tag("tag:e"))}, Tag("tag:g"): Owners{new(Tag("tag:f"))}, }, - wantErr: "circular reference detected: tag:a -> tag:b -> tag:c -> tag:d -> tag:e -> tag:f -> tag:g", + want: TagOwners{ + Tag("tag:a"): nil, + Tag("tag:b"): nil, + Tag("tag:c"): nil, + Tag("tag:d"): nil, + Tag("tag:e"): nil, + Tag("tag:f"): nil, + Tag("tag:g"): nil, + }, + wantErr: "", }, { name: "undefined-tag-reference",