fix: Prevent orphaning records due to label lengths#6431
Conversation
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
|
|
Welcome @mmiller-sh! |
|
Hi @mmiller-sh. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
mmiller-sh
force-pushed
the
fix/orphaned-records
branch
from
755bdef to
c9b1efb
Compare
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
u-kai
left a comment
u-kai
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
Could you take a look at my comment?
| 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 | ||
| } |
There was a problem hiding this comment.
Is my understand correct that your fix doesn't bypass the string limit, but simply output log?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Reopening based on further feedback on the alternate approach. #6436 (comment) |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Coverage Report for CI Build 27476544493Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.05%) to 80.576%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions763 previously-covered lines in 32 files lost coverage.
Coverage Stats
💛 - Coveralls |
ivankatliarchuk
left a comment
ivankatliarchuk
left a comment
There was a problem hiding this comment.
This change may take a while, as it changes behaviour regardless of how we are going to implement it.
Most likely we should consider a new flag, something like.
--strict-label-compliance
--strict-dns-compliance
Depends what other think about that. I know internally we had a discussion while back, to find ways to reduce number of flags. So No flag, no config, always enforced.o flag, no config, always enforced is an option too.
A new flag implies this is optional or user-configurable behavior. But if ExternalDNS can't establish ownership it most likely should never silently create the record - that's a correctness guarantee, not a preference.
| 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 | ||
| } | ||
|
|
||
| filteredChanges.Create = append(filteredChanges.Create, r) | ||
| for _, txt := range txts { | ||
| if im.existingTXTs.isAbsent(txt) { | ||
| filteredChanges.Create = append(filteredChanges.Create, txt) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Something like
Create: endpoint.FilterEndpointsByDNSCompliance(
func(ep *endpoint.Endpoint) string { return im.mapper.ToTXTName(ep.DNSName, ep.RecordType) },
changes.Create,
),
and
// FilterEndpointsByDNSCompliance filters out endpoints for which the name derived by
// nameFunc violates RFC 1035 label length constraints (labels must be ≤ 63 characters).
func FilterEndpointsByDNSCompliance(nameFunc func(*Endpoint) string, eps []*Endpoint) []*Endpoint {
filtered := make([]*Endpoint, 0, len(eps))
for _, ep := range eps {
valid := true
for label := range strings.SplitSeq(nameFunc(ep), ".") {
if len(label) > 63 {
log.Warnf("Skipping endpoint %s %s: label %q exceeds RFC 1035 63-char limit", ep.RecordType, ep.DNSName, label)
valid = false
break
}
}
if valid {
filtered = append(filtered, ep)
}
}
return filtered
}
To be consistent with the rest
🤔 As you said, @ivankatliarchuk
So if we were to introduce a new flag, that would be to support this corner case between 57 & 63 ? It's a common and known pattern to fail gracefully when the user is asking the software to execute unreachable tasks. => So to me, this should be resolved with no new flags and a breaking chance with always enforced behavior where it does not create the record and log a clear message when txt registry is enabled and the domain is 57+ chars. @u-kai Wdyt ? Is there anything missed with this approach? |
|
"It's a common and known pattern to fail gracefully when the user is asking the software to execute unreachable tasks" This is a tricky one. Not creating an orphaned record is most likely better than creating one. That's the right call - a record with no ownership TXT is worse than no record at all. The task isn't "unreachable." The A/AAAA/CNAME record itself is perfectly valid — the label is under 63 chars, and created without any problem. What's unreachable is establishing ownership via the TXT registry mechanism. The constraint is an internal implementation detail of external-dns. "fail gracefully" usually implies the failure is clearly communicated. A log.Warnf buried in controller output is not graceful from a user perspective - the Ingress has no DNS record and use must to grep logs to find out why. A truly graceful failure in a Kubernetes controller context would surface a condition or event on the object with metric, a kubernetes event, or at minimum a log.Errorf. Silent skips with warn-level logs have caused production incidents in controllers before and will cause it most likely in the future. I'm not against changing the behaviour, If we all think that orphaned records are worse than missing ones, we could have no flag for that. I've composed some pros/cons for behavior Orphaned DNS record (current behavior) Pros:
Cons:
No record at all (this PR) Pros:
Cons:
Orphaned record is the worse long-term outcome - it's hidden, permanent, and requires manual intervention (not great for controller). Missing record is painful immediately but at least it's honest and recoverable (fix the name, DNS appears) The catch is that "no record" is only acceptable if the failure is loud enough to act on. Right now the log level and lack of a Kubernetes event and metric make it too quiet for the severity of the consequence (broken service). That's the gap worth be clear about. |
|
Noting that the controller currently logs the naming violation when it attempts to reconcile the invalid TXT records, it does not log anything about orphaned records though. What would you think about increasing log level to error when a record would have been orphaned and adding a Prometheus counter that increments on skips? I'm happy to make both of these changes to make it easier to surface failures involving this DNS spec nuance. I have mild hesitations around adding a metric specific to this corner case/bug fix, but can rationalize it to myself given it would be a single counter. Let me know if that seems reasonable, thank you! |
|
I'm just sharing thoughts and ideas at the moment |
I agree with this approach for simplicity. @mmiller-sh I can see the concern about dynamically changing the registry format now. Keeping the behavior simple and narrowing the supported scope in ExternalDNS instead of introducing more operational complexity sounds like the better approach to me as well. |
Address review feedback on kubernetes-sigs#6431: - Move the label-length check into endpoint.FilterEndpointsByDNSCompliance so the registry no longer manually re-inlines the isAbsent filter. - Log skips at error level — Warnf was easy to miss in controller output. - Track skips with the external_dns_registry_skipped_records_label_too_long_total counter, labeled by record_type and apex domain. - Document the limit and its observability in docs/registry/txt.md. Signed-off-by: Matt Miller <matt@mattmiller.io>
|
Apologies for the delay getting back to this. I attempted to address feedback with the following:
Please let me know if I missed anything! |
u-kai
left a comment
u-kai
left a comment
There was a problem hiding this comment.
These are just my personal opinions, so please feel free to take them as you see fit. Sorry again for the late feedback!
| extends that label. | ||
|
|
||
| If any label in the projected TXT name exceeds 63 characters, external-dns skips both | ||
| the parent record and its ownership TXT. Creating the parent without a TXT would leave |
There was a problem hiding this comment.
The term parent is used here, but owned record might be a better fit — it maps directly to the ownedRecord label key in endpoint/labels.go and reflects how the registry already models this relationship.
| // Such records cannot be owned by the registry and would orphan in the zone. | ||
| // onSkip, if non-nil, is invoked per drop with the offending label so callers | ||
| // can attach metrics or events without importing this package. | ||
| func FilterEndpointsByDNSCompliance(toRegistryName func(*Endpoint) string, eps []*Endpoint, onSkip func(skipped *Endpoint, badLabel string)) []*Endpoint { |
There was a problem hiding this comment.
Sorry for the late comment — sharing my personal take on the current design. 🙇
FilterEndpointsByDNSCompliance takes a toRegistryName func(*Endpoint) string parameter, which is essentially TXT registry naming logic injected into the endpoint package. I find this
harder to follow than necessary, and toRegistryName doesn't feel like a natural responsibility of the endpoint package.
Also, since NewEndpoint already validates label length and returns nil for violations, filtering endpoints before calling it feels redundant to me — the check ends up happening in two
places conceptually.
There was a problem hiding this comment.
What do you think about an alternative like this instead?
endpoint/endpoint.go — expose the check as a public utility:
// OverflowingLabel returns the first label in name exceeding RFC 1035's 63-char limit.
func OverflowingLabel(name string) (string, bool) {
for label := range strings.SplitSeq(name, ".") {
if len(label) > 63 {
return label, true
}
}
return "", false
}registry/txt/registry.go — validate in generateTXTRecord explicitly, removing generateTXTRecordWithFilter:
func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
recordType := r.RecordType
if shouldUseCNAMEForTxtRecord(r) {
recordType = endpoint.RecordTypeCNAME
}
if im.oldOwnerID != "" && r.Labels[endpoint.OwnerLabelKey] == im.oldOwnerID {
r.Labels[endpoint.OwnerLabelKey] = im.ownerID
}
txtName := im.mapper.ToTXTName(r.DNSName, recordType)
if label, ok := endpoint.OverflowingLabel(txtName); ok {
return nil, fmt.Errorf("%s %s: projected TXT name %q has label %q exceeding RFC 1035's 63-char limit", r.RecordType, r.DNSName, txtName, label)
}
txtNew := endpoint.NewEndpoint(txtName, endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey))
txtNew.WithSetIdentifier(r.SetIdentifier)
txtNew.Labels[endpoint.OwnedRecordLabelKey] = r.DNSName
txtNew.ProviderSpecific = r.ProviderSpecific
return []*endpoint.Endpoint{txtNew}, nil
}registry/txt/registry.go — Create loop in ApplyChanges, with isAbsent made explicit at the call site:
filteredChanges := &plan.Changes{
Create: make([]*endpoint.Endpoint, 0, len(changes.Create)),
UpdateNew: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateNew),
UpdateOld: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateOld),
Delete: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.Delete),
}
for _, r := range changes.Create {
if r.Labels == nil {
r.Labels = make(map[string]string)
}
r.Labels[endpoint.OwnerLabelKey] = im.ownerID
txts, err := im.generateTXTRecord(r)
if err != nil {
log.Errorf("Skipping owned record: %v; record would become unmanageable", err)
recordSkippedLabelTooLong(r)
continue
}
filteredChanges.Create = append(filteredChanges.Create, r)
for _, txt := range txts {
if im.existingTXTs.isAbsent(txt) {
filteredChanges.Create = append(filteredChanges.Create, txt)
}
}
if im.cacheInterval > 0 {
im.addToCache(r)
}
}This keeps the label length check and TXT name generation in one place (generateTXTRecord), avoids leaking registry-specific logic into the endpoint package, and makes isAbsent explicit at the call site.
There was a problem hiding this comment.
Ideally, if NewEndpoint returned an error instead of nil when endpoint label length overflow, things could be even simpler — but given the broad impact of that change, I'd suggest the approach above as a practical middle ground
There was a problem hiding this comment.
Thanks @u-kai, I believe I've addressed your feedback with the most recent changes. Can you let me know what you think?
Signed-off-by: Matt Miller <matt@mattmiller.io>
k8s-ci-robot
added
the
needs-rebase
# Conflicts: # registry/txt/registry.go
k8s-ci-robot
removed
the
needs-rebase
| if err != nil { | ||
| log.Errorf("Skipping TXT companion for updateNew: %v", err) | ||
| } |
There was a problem hiding this comment.
I think generateTXTRecord can never return an error in the Update and Delete paths.
Records in both paths were already recognized as owned Record() method, which means their TXT names were valid at creation time.
Since updates never change the DNS name or record type, only change targets or labels, the same TXT name is recognized and cannot overflow.
IMO, we should drop the error catches.
That's said, I might missing something, please collect me if I'm wrong.
Wdyt??
There was a problem hiding this comment.
My perception is that this error checking is pretty harmless and could potentially be helpful if the registry behavior changes in the future. That said, I'll remove the error check to keep the code cleaner as it pertains to current behavior.
u-kai
left a comment
u-kai
left a comment
There was a problem hiding this comment.
Thank you for updating this.
I left a small comment.
| if err != nil { | ||
| log.Errorf("Skipping TXT companion for updateOld: %v", err) | ||
| } | ||
| txts, _ := im.generateTXTRecord(r) |
There was a problem hiding this comment.
IMO
Other comments in this file are already duplicated across similar paths, so it would be consistent to add the same "unreachable" explanation to UpdateOld and UpdateNew, not just Delete.
| if err != nil { | ||
| log.Warnf("Skipping migration check: %v", err) | ||
| } |
There was a problem hiding this comment.
@mmiller-sh
IMO, this error is unreachable for the same reason as the _ discards in the Delete/UpdateOld/UpdateNew paths — any ep reaching here already had its TXT name successfully created, so the label-length check can never fire.
| 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 |
There was a problem hiding this comment.
I'll rename the var to be more clear of it's usage.
|
|
||
| txtNew := endpoint.NewEndpoint(txtName, endpoint.RecordTypeTXT, r.Labels.Serialize(true, im.txtEncryptEnabled, im.txtEncryptAESKey)) | ||
| if txtNew == nil { | ||
| return nil, nil |
| prometheus.GaugeOpts{ | ||
| Subsystem: "registry", | ||
| Name: "skipped_records_label_too_long_per_sync", | ||
| Help: "Number of records skipped because the projected TXT registry name has a DNS label exceeding RFC 1035's 63-char limit, for each record type and domain (vector).", |
There was a problem hiding this comment.
And the metric name describes the wrong thing. label_too_long describes a property of the original record's label - but that's not what's overflowing. It's the projected TXT name's label that overflows, after the cname-/aaaa-/--txt-prefix is prepended. A record with a 60-char label is perfectly valid and will be skipped only because cname- + 60 = 66 > 63. An operator seeing label_too_long might think their source record is invalid, not that the TXT prefix is the problem.
Maybe something like
registry_skipped_records_label_overflow_per_sync{record_type, domain, overflow_in}
with overflow_in taking two values:
- "source" — the original DNS name's label is > 63 chars (caught in NewEndpoint)
- "txt_prefix" — the projected TXT name's label overflows after the record-type prefix and/or --txt-prefix/--txt-suffix is applied
label_overflow rather than label_too_long — "overflow" is the RFC 1035 term and is directional (it implies something caused it to exceed the limit, not that the label was inherently wrong)
Help text: "Number of records skipped per sync because a DNS label exceeds RFC 1035's 63-char limit, by record type, apex domain, and whether the overflow is in the source name or the projected TXT registry name."
There was a problem hiding this comment.
Not covered
I believe this check is no longer necessary due to the suggested OverflowingLabel function that was added to check for correctness before calling NewEndpoint per review. I'm going to drop this check.
And the metric name describes the wrong thing.
I agree, skipped_records_label_overflow_per_sync is more descriptive of the actual problem.
One implementation detail I want to call out here while wiring the "source" overflow_in metric label, it initially seemed more natural to me to move the metric registration to the endpoint package, but there are cyclical dependencies that result from doing so. The smallest change set I could come up with to avoid refactoring the metrics package was to keep the metric registration in registry, and wire up a callback function to this from endpoint. Please let me know if this approach seems appropriate, or if you have other suggestions (I couldn't easily find precedent for this type of plumbing in the code base). I also considered having NewEndpoint return error, or overflowing labels so we could detect this in the registry package, but changing the interface seemed like a deal breaker to me.
There was a problem hiding this comment.
Yeap, I thought about moving metric, but not found a way yet. Most likely we could do it in follow-up at some point.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Seems like adding metrics create some design tensions. Most likely worth to drop metrics from this pull request. Add initial fix, and if you have interest, in follo-up pull request you could try to add a metric.
With current design, there is a bit of struggling with data flow, global callbacks and etc
|
|
||
| // parentDomain returns the parent domain of dnsName (without the first label), | ||
| // or dnsName unchanged when it is an apex/two-label name. | ||
| func parentDomain(dnsName string) string { |
There was a problem hiding this comment.
duplicate
external-dns/endpoint/endpoint.go
Line 501 in e4e6037
| } | ||
|
|
||
| // OverflowingLabel returns the first label in name longer than 63 chars. | ||
| func OverflowingLabel(name string) (string, bool) { |
There was a problem hiding this comment.
The name is grammatically awkward though; Go convention favors either HasOverflowingLabel(name) bool for a predicate, or FindOverflowingLabel if you need the value.
Not sure if the file location is correct
6 participants
What does it do ?
This prevents the controller from creating A/AAAA/CNAME records when a corresponding ownership TXT record is unable to be created for the domain.
In cases where the A/AAAA/CNAME record is under the 63 character DNS label length limit, but the generated TXT record label (with
aaaa-orcname-prefixes) exceeds the limit, these records will become orphaned by the controller given lack of established ownership.Motivation
Fixes: #6430
More