diff --git a/hscontrol/servertest/names_compat_test.go b/hscontrol/servertest/names_compat_test.go index 556674c4f..da3bf98b8 100644 --- a/hscontrol/servertest/names_compat_test.go +++ b/hscontrol/servertest/names_compat_test.go @@ -31,19 +31,6 @@ import ( // ///. A listed check that // matches fails, so a fix must delete its entry. var nameSkipReasons = map[string]string{ - "names/N1/n63b/label": labelSuffixOverflow, - "names/N2/n63b/label": labelSuffixOverflow, - "names/N3/n63b/label": labelSuffixOverflow, - "names/N4/n63b/label": labelSuffixOverflow, - "names/N5/n63b/label": labelSuffixOverflow, - "names/N6/n63b/label": labelSuffixOverflow, - "names/N7/n63b/label": labelSuffixOverflow, - "names/N8/n63b/label": labelSuffixOverflow, - "names/N9a/n63b/label": labelSuffixOverflow, - "names/N9b/n63b/label": labelSuffixOverflow, - "names/N10/n63b/label": labelSuffixOverflow, - "names/N11/n63b/label": labelSuffixOverflow, - "names/N1/domain": v2NameBare, "names/N2/domain": v2NameBare, "names/N3/domain": v2NameBare, @@ -81,7 +68,6 @@ var nameSkipReasons = map[string]string{ } const ( - labelSuffixOverflow = "LABEL_SUFFIX_OVERFLOW: headscale appends -N to a 63-byte label, giving a 65-byte one; SaaS cuts the label so the suffix fits in 63 bytes" adminNameFollowsHostname = "ADMIN_NAME_FOLLOWS_HOSTNAME: an admin name equal to the hostname's label counts as derived from it, so a hostname change replaces it; SaaS keeps an admin name across hostname changes" nameTakenText = "NAME_TAKEN_TEXT: headscale's v2 message for a rename refused as taken is the operation, with the reason only in data; SaaS's message says the name is already taken" v2NameBare = "V2_NAME_BARE: headscale's v2 device name is the bare label; SaaS's is the FQDN under the tailnet domain" diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 0b58ac071..3549c8056 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -13,6 +13,7 @@ import ( "time" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" "tailscale.com/net/tsaddr" @@ -665,7 +666,8 @@ func (s *NodeStore) applyBatch(batch []work) { // keeps the current label. // // On collision the label is bumped as base, base-1, base-2, …, first -// unused wins. Must be called from the [NodeStore] writer goroutine +// unused wins; the base is cut so base-N stays a valid label, as Tailscale +// does. Must be called from the [NodeStore] writer goroutine // (inside [NodeStore.applyBatch]) so the nodes map reflects all earlier // ops in the batch and no other writer can interleave. func resolveGivenName( @@ -695,10 +697,18 @@ func resolveGivenName( return candidate } - candidate = base + "-" + strconv.Itoa(i) + candidate = bumpedLabel(base, i) } } +// bumpedLabel is base with a -n suffix, base cut so the result stays a valid +// label, as Tailscale does. +func bumpedLabel(base string, n int) string { + suffix := "-" + strconv.Itoa(n) + + return base[:min(len(base), util.LabelHostnameLength-len(suffix))] + suffix +} + // snapshotFromNodes builds a Snapshot from nodes. With reusePeers the // previous peer-ID adjacency is carried over unchanged; with reuseRoutes // the previous route election is. ListPeers resolves adjacency IDs diff --git a/hscontrol/state/node_store_hostname_test.go b/hscontrol/state/node_store_hostname_test.go index dc7db02b2..827bf56a1 100644 --- a/hscontrol/state/node_store_hostname_test.go +++ b/hscontrol/state/node_store_hostname_test.go @@ -2,6 +2,7 @@ package state import ( "fmt" + "strings" "sync" "testing" @@ -61,6 +62,27 @@ func TestPutNodeGivenNameCollisionBumps(t *testing.T) { require.Equal(t, "laptop-2", got3.GivenName(), "third registration bumps to -2") } +func TestPutNodeBumpCutsLabelToFitSuffix(t *testing.T) { + store := NewNodeStore(nil, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) + + store.Start() + defer store.Stop() + + long := strings.Repeat("a", 63) + + got := make([]string, 0, 12) + for id := range types.NodeID(12) { + got = append(got, store.PutNode(createTestNode(id+1, 1, "alice", long)).GivenName()) + } + + require.Equal(t, long, got[0]) + require.Equal(t, long[:61]+"-1", got[1]) + require.Equal(t, long[:60]+"-10", got[10]) + require.Equal(t, long[:60]+"-11", got[11]) + require.True(t, isAutoDerivedGivenName(got[10], long), + "a cut label still follows its hostname") +} + // TestPutNodeEmptyGivenNameFallsBackToNode covers the SaaS rule that // an empty sanitised label becomes the literal "node". Subsequent // empty-label registrations bump as usual. diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index b13581aa5..19efcd1b5 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1653,32 +1653,6 @@ func TestBackfillNodeIPsSurvivesConcurrentPersist(t *testing.T) { // differs from SaaS, keyed ///. // A listed check that matches fails, so a fix must delete its entry. var nameSkipReasons = map[string]string{ - "names/N1/n63b/label": labelSuffixOverflow, - "names/N2/n63b/label": labelSuffixOverflow, - "names/N3/n63b/label": labelSuffixOverflow, - "names/N4/n63b/label": labelSuffixOverflow, - "names/N5/n63b/label": labelSuffixOverflow, - "names/N6/n63b/label": labelSuffixOverflow, - "names/N7/n63b/label": labelSuffixOverflow, - "names/N8/n63b/label": labelSuffixOverflow, - "names/N9a/n63b/label": labelSuffixOverflow, - "names/N9b/n63b/label": labelSuffixOverflow, - "names/N10/n63b/label": labelSuffixOverflow, - "names/N11/n63b/label": labelSuffixOverflow, - - "names/N1/n63b/reopen": labelSuffixOverflow, - "names/N2/n63b/reopen": labelSuffixOverflow, - "names/N3/n63b/reopen": labelSuffixOverflow, - "names/N4/n63b/reopen": labelSuffixOverflow, - "names/N5/n63b/reopen": labelSuffixOverflow, - "names/N6/n63b/reopen": labelSuffixOverflow, - "names/N7/n63b/reopen": labelSuffixOverflow, - "names/N8/n63b/reopen": labelSuffixOverflow, - "names/N9a/n63b/reopen": labelSuffixOverflow, - "names/N9b/n63b/reopen": labelSuffixOverflow, - "names/N10/n63b/reopen": labelSuffixOverflow, - "names/N11/n63b/reopen": labelSuffixOverflow, - "names/N11/pdb/label": adminNameFollowsHostname, "names/N11/pdb/reopen": adminNameFollowsHostname, "names/N11b/pdb/label": adminNameFollowsHostname, @@ -1692,7 +1666,6 @@ var nameSkipReasons = map[string]string{ } const ( - labelSuffixOverflow = "LABEL_SUFFIX_OVERFLOW: headscale appends -N to a 63-byte label, giving a 65-byte one; SaaS cuts the label so the suffix fits in 63 bytes" adminNameFollowsHostname = "ADMIN_NAME_FOLLOWS_HOSTNAME: an admin name equal to the hostname's label counts as derived from it, so a hostname change replaces it; SaaS keeps an admin name across hostname changes" ) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 828eee802..c0f920d9b 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -3900,23 +3900,23 @@ func (s *State) PingDB(ctx context.Context) error { // isAutoDerivedGivenName reports whether given matches what // dnsname.SanitizeHostname(hostname) would produce, optionally with a -// [NodeStore] collision-bump "-N" suffix. It is used to detect whether a -// GivenName has been admin-renamed (in which case it must not be -// overwritten by client-side hostname changes). +// [NodeStore] collision-bump "-N" suffix ([bumpedLabel]). It is used to +// detect whether a GivenName has been admin-renamed (in which case it must +// not be overwritten by client-side hostname changes). func isAutoDerivedGivenName(given, hostname string) bool { base := dnsname.SanitizeHostname(hostname) if given == base { return true } - suffix, ok := strings.CutPrefix(given, base+"-") + _, suffix, ok := strings.CutLast(given, "-") if !ok { return false } - _, err := strconv.Atoi(suffix) + n, err := strconv.Atoi(suffix) - return err == nil + return err == nil && given == bumpedLabel(base, n) } // UpdateNodeFromMapRequest is the sync point where Hostinfo changes,