Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions registry/txt/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -339,13 +339,28 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
Delete: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.Delete),
}

for _, r := range filteredChanges.Create {
pendingCreate := filteredChanges.Create
filteredChanges.Create = make([]*endpoint.Endpoint, 0, len(pendingCreate))
for _, r := range pendingCreate {
if r.Labels == nil {
r.Labels = make(map[string]string)
}
r.Labels[endpoint.OwnerLabelKey] = im.ownerID

filteredChanges.Create = append(filteredChanges.Create, im.generateTXTRecordWithFilter(r, im.existingTXTs.isAbsent)...)
// Skip records whose ownership TXT cannot be established; creating
// them would leak an unreclaimable record into the zone.
txts := im.generateTXTRecord(r)
if len(txts) == 0 {
log.Warnf("Skipping create of %s %s: cannot establish ownership TXT (label exceeds RFC 1035 63-char limit)", r.RecordType, r.DNSName)
continue
}
Comment on lines +352 to +356
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is my understand correct that your fix doesn't bypass the string limit, but simply output log?

Copy link
Copy Markdown
Author

@mmiller-sh mmiller-sh May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It prevents the A/AAAA/CNAME records from being created (and orphaned) in the first place when the TXT record names cross the string limit, as well as logging the event.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this change breaks backward compatibility and feels more like a workaround.
I would prefer changing the TXT registry format when the record lenght limit is reached.

What do you think?

Copy link
Copy Markdown
Author

@mmiller-sh mmiller-sh May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a workaround. I believe moving the record type into it's own label when it would overflow, ie: TXT cname.label1.label2.com should fix the immediate issue. Let me look into what might be needed to safely implement this in a non-breaking way.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@u-kai I created a new PR with the alternate approach: #6436

Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this change breaks backward compatibility and feels more like a workaround. I would prefer changing the TXT registry format when the record lenght limit is reached.

What do you think?

I have not read that. Actually my view is opposite #6436 (comment). Tool should not change the registry format dynamically. This is an operational hell.

Frankly speaking, the user should have a convention, and manage zone naming. When the DNS hits 50 characters, it's already a signal that it's a time to create a new label/zone


filteredChanges.Create = append(filteredChanges.Create, r)
for _, txt := range txts {
if im.existingTXTs.isAbsent(txt) {
filteredChanges.Create = append(filteredChanges.Create, txt)
}
}
Comment on lines +344 to +363
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change manually re-inlines the isAbsent filter after splitting from generateTXTRecordWithFilter. This is fragile if the helper ever gains new behavior. Not sure if it should be even here


if im.cacheInterval > 0 {
im.addToCache(r)
Expand Down
101 changes: 101 additions & 0 deletions registry/txt/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1138,6 +1138,107 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) {
require.NoError(t, err)
}

// TestTXTRegistryApplyChangesSkipsRecordWithoutOwnership verifies that the
// registry refuses to create a parent record whenever its ownership TXT cannot
// be produced. Such a parent would be unreclaimable by future reconciles and
// would leak in the zone.
//
// The subtests below exercise the only failure mode that exists today — the
// projected TXT name exceeding the RFC 1035 63-char label limit — across the
// record types and registry configurations whose differing TXT prefixes shift
// the overflow threshold. Any future failure path in TXT generation should
// hit the same guard.
func TestTXTRegistryApplyChangesSkipsRecordWithoutOwnership(t *testing.T) {
tests := []struct {
name string
txtPrefix string
newParent func(dnsName string) *endpoint.Endpoint
labelLen int
txtPrefx string // the prefix the mapper prepends, used to assert the TXT name is not in the change set
}{
{
name: "CNAME",
newParent: func(dn string) *endpoint.Endpoint { return endpoint.NewEndpoint(dn, endpoint.RecordTypeCNAME, "lb.example.com") },
labelLen: 60, // cname- (6) + 60 = 66 -> overflow
txtPrefx: "cname-",
},
{
name: "AAAA",
newParent: func(dn string) *endpoint.Endpoint { return endpoint.NewEndpoint(dn, endpoint.RecordTypeAAAA, "fe80::1") },
labelLen: 60, // aaaa- (5) + 60 = 65 -> overflow
txtPrefx: "aaaa-",
},
{
name: "A",
newParent: func(dn string) *endpoint.Endpoint { return endpoint.NewEndpoint(dn, endpoint.RecordTypeA, "1.2.3.4") },
labelLen: 62, // a- (2) + 62 = 64 -> overflow
txtPrefx: "a-",
},
{
name: "AliasA",
newParent: func(dn string) *endpoint.Endpoint {
return endpoint.NewEndpoint(dn, endpoint.RecordTypeA, "lb.example.com").WithProviderSpecific(endpoint.ProviderSpecificAlias, "true")
},
labelLen: 60, // alias-A is encoded as cname- (6) + 60 = 66 -> overflow
txtPrefx: "cname-",
},
{
name: "WithTxtPrefix",
txtPrefix: "ext-",
newParent: func(dn string) *endpoint.Endpoint { return endpoint.NewEndpoint(dn, endpoint.RecordTypeCNAME, "lb.example.com") },
labelLen: 56, // ext- (4) + cname- (6) + 56 = 66 -> overflow
txtPrefx: "ext-cname-",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
ctx := t.Context()
p := inmemory.NewInMemoryProvider()
require.NoError(t, p.CreateZone(testZone))

r, err := newRegistry(p, tc.txtPrefix, "", "owner", time.Hour, "", []string{}, []string{}, false, nil, "")
require.NoError(t, err)

overflowLabel := strings.Repeat("a", tc.labelLen)
overflow := tc.newParent(overflowLabel + "." + testZone)
require.NotNil(t, overflow, "test setup: parent record at %d-char label must itself pass RFC 1035", tc.labelLen)
fits := tc.newParent("ok." + testZone)

hook := logtest.LogsUnderTestWithLogLevel(log.WarnLevel, t)

var got *plan.Changes
p.OnApplyChanges = func(_ context.Context, ch *plan.Changes) {
got = ch
}
require.NoError(t, r.ApplyChanges(ctx, &plan.Changes{
Create: []*endpoint.Endpoint{overflow, fits},
}))

require.NotNil(t, got)
overflowTXT := tc.txtPrefx + overflow.DNSName
for _, ep := range got.Create {
assert.NotEqual(t, overflow.DNSName, ep.DNSName, "parent without ownership TXT must be dropped")
assert.NotEqual(t, overflowTXT, ep.DNSName, "TXT companion for dropped parent must not be created")
}

fitsParent := false
fitsTXT := false
for _, ep := range got.Create {
if ep.DNSName == fits.DNSName && ep.RecordType == fits.RecordType {
fitsParent = true
}
if ep.DNSName == tc.txtPrefx+fits.DNSName && ep.RecordType == endpoint.RecordTypeTXT {
fitsTXT = true
}
}
assert.True(t, fitsParent, "fits-fine parent must still be created")
assert.True(t, fitsTXT, "fits-fine TXT companion must still be created")

logtest.TestHelperLogContains("cannot establish ownership TXT", hook, t)
})
}
}

func testTXTRegistryMissingRecords(t *testing.T) {
t.Run("No prefix", testTXTRegistryMissingRecordsNoPrefix)
t.Run("With Prefix", testTXTRegistryMissingRecordsWithPrefix)
Expand Down
Loading