diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 4d33397c..679b4397 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1456,27 +1456,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 615b044b..92521940 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 b2e746fa..379d6ba0 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",