fix(source): skip invalid endpoint hostnames

[SetagGnaw]

What does it do ?

NewEndpoint and NewEndpointWithTTL return nil for a DNS name they cannot represent, so correctness depends on every caller remembering a nil check that nothing enforces. This replaces that contract rather than patching the call sites one at a time.

• New constructors: NewValidatedEndpoint and NewValidatedEndpointWithTTL return the rejection as an error, in the shape NewPTREndpoint already used. NewPTREndpoint now delegates to them, closing a gap where it could return a nil endpoint with a nil error.
• Old constructors deprecated, not changed: both are exported and called by out-of-tree webhook providers, so they stay as wrappers with their exact contract preserved, log line byte for byte. The Deprecated: marker puts staticcheck's SA1019 behind the migration, so a call site a follow-up misses fails the lint run instead of relying on convention. Test files are unaffected, since .golangci.yml excludes staticcheck for _test.go.
• Every non-test call site migrated, handling the rejection where it happens: sources and the template engine warn with the offending hostname and skip that one record; the TXT registry skips an ownership record it cannot build; internal/testutils panics, since an unrepresentable fixture name is a bug in the test itself.

That covers the two crash paths this PR set out to fix:

• The template paths: EndpointsForHostsAndTargets (endpoint/utils.go) and endpointsFromFQDNTargetTemplate (source/template/engine.go) appended the constructor's return value unchecked. Both are reached from --fqdn-template and --fqdn-target-template.
• Sites that dereferenced or collected the nil: node endpoints, pod annotation and FQDN-template endpoints, and legacy compatibility-mode service endpoints (source/node.go, source/pod.go, source/compatibility.go).

It also fixes a latent crash of the same shape in the providers: 23 provider packages built endpoints from zone listings without a nil check, so a rejected name in a listing leaked a nil out of Records() and panicked downstream.

Adds regression coverage for invalid names arriving from annotations, fqdn templates, fqdn-target templates, and legacy compatibility annotations. Each new source test fails with a nil pointer dereference when its fix is reverted.

Motivation

NewEndpointWithTTL returns nil when any dot-separated label of the DNS name is longer than 63 characters (endpoint/endpoint.go). That 63 is the DNS label limit from RFC 1035 section 2.3.4, not a Kubernetes limit, and it applies per label rather than to the name as a whole (which the same section caps separately at 255).

The affected paths either dereferenced that nil result or appended it for later processing, causing source reconciliation to panic in WithLabel, AttachRefObject, or MergeEndpoints. A single invalid name therefore took down the whole sync, when it should be logged and skipped so the remaining records still reconcile.

How an over-long label reaches these paths

The DNS per-label limit is stricter than what Kubernetes itself enforces on the names these sources read, so the API server can hand external-dns a name that NewEndpointWithTTL rejects:

• Node: node names are validated as a DNS-1123 subdomain (NameIsDNSSubdomain -> IsDNS1123Subdomain), which caps the total name at 253 characters and applies a regex that places no bound on individual labels. A single-label node name longer than 63 characters is therefore accepted.
• Pod: the external-dns.alpha.kubernetes.io/internal-hostname and external-dns.alpha.kubernetes.io/hostname annotations are free-form and fully user-controlled.
• Any source with a template: --fqdn-template and --fqdn-target-template render object fields into a hostname, so the result is only as valid as the template and the values it interpolates.

The annotation path was already guarded inside EndpointsForHostname, which is why the template paths went unnoticed for so long: when a resource declares its own hostname the template is never executed at all, so only resources relying on a template could reach the crash.

Why not a shared nil-skipping helper

An earlier revision of this branch collected the skip into one endpoint.AppendIfNotNil helper beside the constructors. Surfacing the rejection as an error made that helper unnecessary and it is gone from the final diff: a call site that gets an error handles it and moves on, so nothing needs to filter nils out of a slice afterwards.

That also undid a restructuring the helper had forced. Sites setting ProviderSpecific or a resource label between constructing and collecting an endpoint could not call a helper inline while the nil was still in the way, so they had collected first and applied the shared metadata in a second pass. With the error handled at the call site the nil is gone before those field accesses, and they set metadata directly again. The endpoints produced are unchanged.

Fixes #6589.

Testing

• make build
• go test -race ./...
• golangci-lint run ./endpoint/... ./source/...

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

[kubernetes-prow]

Welcome @SetagGnaw!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[kubernetes-prow]

Hi @SetagGnaw. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

[ivankatliarchuk]

/ok-to-test

[ivankatliarchuk]

I can't properly review at the moment. My understanding is - the label length is valid, but this annotation could only contain valid DNS.

We need to check endpoint + annotations packages, as where the validation shoild leave.

[coveralls]

Coverage Report for CI Build 31843984577

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.1%) to 81.806%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 708 coverage regressions across 28 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

708 previously-covered lines in 28 files lost coverage.

Top 10 Files by Coverage Loss	Lines Losing Coverage	Coverage
alibabacloud/alibaba_cloud.go	106	63.2%
rfc2136/rfc2136.go	105	69.82%
linode/linode.go	56	80.83%
godaddy/godaddy.go	39	70.98%
aws/aws.go	37	88.61%
civo/civo.go	33	84.68%
azure/azure.go	33	75.47%
testutils/endpoint.go	32	56.11%
scaleway/scaleway.go	28	68.91%
pdns/pdns.go	27	67.34%

---

Coverage Stats

Relevant Lines:	21513
Covered Lines:	17599
Line Coverage:	81.81%
Coverage Strength:	1450.59 hits per line

---

💛 - Coveralls

[SetagGnaw]

Thanks for taking a look @ivankatliarchuk, no rush at all!

Just to clarify the label length part: the 63 character limit involved here is the DNS per-label cap from RFC 1035 section 2.3.4, not a Kubernetes one. Kubernetes validates these names as a DNS-1123 subdomain (253 characters total, with no bound on an individual label) and the hostname annotations are free-form, so a name whose first label is 64 characters passes the API server while not being valid DNS.

The validation for that already lives in the endpoint package: the constructor checks every dot-separated label, and when one is longer than 63 characters the name is invalid DNS, so it logs an error naming the offending label and returns nil:

external-dns/endpoint/endpoint.go

Lines 303 to 308 in 05e84d6

	for label := range strings.SplitSeq(dnsName, ".") {
	if len(label) > 63 {
	log.Errorf("label %s in %s is longer than 63 characters. Cannot create endpoint", label, dnsName)
	return nil
	}
	}

The gap this PR closes is on the caller side: source/node.go, source/pod.go and source/compatibility.go used that nil result without checking it, so a single invalid name would panic the sync later in WithLabel / AttachRefObject / MergeEndpoints. With this change any nil endpoint is skipped instead, mirroring the nil guards source/service.go and EndpointsForHostname already have. The skip is also logged with the reason, following the logging pattern already used in the repo: the constructor's error above carries the label and name, and the skip itself is logged the same way source/crd.go logs nil endpoints in a DNSEndpoint spec.

Happy to also look at tightening validation in the annotations package in a follow-up if you think that is the right place for it.

[ivankatliarchuk]

I think guarding the two append sites themselves in endpoint/utils.go and engine.go with the same if ep == nil { continue } pattern fixes both gaps at once and keeps the length rule defined in exactly one place.

  endpoint/utils.go — EndpointsForHostsAndTargets (around line 124-129):

        endpoints := make([]*Endpoint, 0, len(sortedHosts)*len(sortedTypes))
        for _, hostname := range sortedHosts {
                for _, recordType := range sortedTypes {
                        ep := NewEndpoint(hostname, recordType, sortedTargets[recordType]...)
                        if ep == nil {
                                continue
                        }
                        endpoints = append(endpoints, ep)
                }
        }
        return endpoints

and

  source/template/engine.go — endpointsFromFQDNTargetTemplate (around line 173-175):

                host := strings.TrimSpace(parts[0])
                target := strings.TrimSpace(parts[1])
                if host == "" || target == "" {
                        log.Debugf("Skipping incomplete host:target pair %q from %s %s/%s: field may not yet be populated",
                                pair, kind, obj.GetNamespace(), obj.GetName())
                        continue
                }
                ep := endpoint.NewEndpoint(host, endpoint.SuitableType(target), target)
                if ep == nil {
                        continue
                }
                eps = append(eps, ep)

[ivankatliarchuk]

And I'd lean toward putting one shared helper in the endpoint package itself rather than source/endpoints.go - it operates purely on endpoint.Endpoint, and then both source/ and endpoint/utils.go/engine.go could reuse the same function instead of the fix being split across two packages with two different idioms (inline check vs. named helper).

[SetagGnaw]

For now, I created a helper in endpoint/utils.go and renamed it to AppendIfNotNil(). Not sure if using purely inline check would be better though. I am fine with either way!

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ivankatliarchuk. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[u-kai]

@SetagGnaw

Thanks for the fix — this closes the immediate panic in #6589.

One thought on the approach, though (just my take, curious what you think): I'm a bit hesitant about leaning on
AppendIfNotNil as the long-term fix, since it's a convention rather than something the compiler enforces — nothing would
stop a future call site from appending the result unchecked again, which is basically how we got here in the first place.

Since NewEndpoint/NewEndpointWithTTL are exported and used by third-party webhook providers, I don't think we can change
their signature directly. But what if we added a new constructor returning (*Endpoint, error) instead (there's already a
similar shape in this file: NewPTREndpoint, endpoint/endpoint.go:522), routed call sites through that, and marked the old
ones // Deprecated:? That way staticcheck/SA1019 would catch any spot we missed, instead of relying on convention.

[SetagGnaw]

@SetagGnaw

Thanks for the fix — this closes the immediate panic in #6589.

One thought on the approach, though (just my take, curious what you think): I'm a bit hesitant about leaning on AppendIfNotNil as the long-term fix, since it's a convention rather than something the compiler enforces — nothing would stop a future call site from appending the result unchecked again, which is basically how we got here in the first place.

Since NewEndpoint/NewEndpointWithTTL are exported and used by third-party webhook providers, I don't think we can change their signature directly. But what if we added a new constructor returning (*Endpoint, error) instead (there's already a similar shape in this file: NewPTREndpoint, endpoint/endpoint.go:522), routed call sites through that, and marked the old ones // Deprecated:? That way staticcheck/SA1019 would catch any spot we missed, instead of relying on convention.

Should we change the tests as well?

[SetagGnaw]

My latest change is doing what you both were suggesting, but the change is very large.

/cc @ivankatliarchuk @u-kai