[mindw]

What does it do ?

--exclude-unschedulable also skips Nodes that are being removed, in the node and service sources. As well as cordoned Nodes, it now skips Nodes that:

• have deletionTimestamp set;
• are tainted ToBeDeletedByClusterAutoscaler (Cluster Autoscaler) or karpenter.sh/disrupted (Karpenter). Both add the taint before they drain and delete the Node.

The flag defaults to true, so this is on by default. --no-exclude-unschedulable turns it off.

NodePort targets skip Nodes labelled node.kubernetes.io/exclude-from-external-load-balancers, with the same rule as the cloud-provider service controller (nodeIncludedPredicate): any value that doesn't parse as false excludes the Node. There is no flag for this. The node source doesn't use the label.

With externalTrafficPolicy: Local, both checks now run before the pod readiness tiers are compared. If the best pods run on excluded Nodes, the records use the best tier on the remaining Nodes instead of ending up empty. Before this, cordoned Nodes were dropped after the tiers were compared.

The service source re-syncs on Node events whenever it has a Node informer (NodePort enabled in --service-type-filter). Adding or deleting a Node triggers a sync. An update triggers one only if it changes the addresses, labels, taints, spec.unschedulable or deletion state, so status reports don't.

On upgrade, Nodes being removed leave node and NodePort records, and Nodes that already carry the label leave NodePort records.

Docs: the NodePort part of docs/sources/service.md, docs/sources/nodes.md, and the --exclude-unschedulable help text in docs/flags.md.

Motivation

NodePort records list every Node. With an autoscaler many Nodes are short-lived, so clients resolve to Nodes that were just removed.

• The taints and deletionTimestamp are the signals that kube-proxy (NodeEligible, KEP-3836) and the cloud-provider service controller (stableNodeSetPredicates) use. kube-proxy doesn't check Karpenter's taint, which Karpenter adds before it deletes the Node.
• The taints only cover the end of a Node's life, and resolvers keep a removed address until the TTL expires. The label keeps Nodes that are short-lived by design out of the records for their whole life, for example by setting it in a Karpenter NodePool's template labels. k8s.io/api defines it as: "the node should not be considered as a target for external load-balancers which use nodes as a second hop".
• Without Node events, a Node change only reaches the records at the next --interval.

Following the review discussion, this replaces the nodeport-filter annotation that the PR first proposed. #916 and #4693 asked for a way to keep Nodes out of NodePort records too.

More

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

[kubernetes-prow]

Welcome @mindw!

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 @mindw. 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.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

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]

Not required,. Same functionality supported with --target-template=TARGET-TEMPLATE and or --fqdn-target-template=FQDN-TARGET-TEMPLATE

Neither was rejected; both were closed for inactivity:

This shouldn't be read as a signal. PRs maintainers don't find valuable are typically left to expire and get auto-closed by the bot.

[ivankatliarchuk]

/hold

[ivankatliarchuk]

I unhold for now. As need too review it, maybe not supported feature

/hold cancel

[ivankatliarchuk]

#6844 aleternative to my understanding

[mloiseleur]

I agree with @ivankatliarchuk. Keeping short-lived nodes out is a cluster-wide concern, so an annotation on every Service feels like the wrong place for it.

--exclude-unschedulable already covers this, except Karpenter doesn't cordon the nodes it disrupts. It only adds the karpenter.sh/disrupted:NoSchedule taint (docs).

Could we extend --exclude-unschedulable to also skip nodes with that taint instead? That fixes it for every Service with no per-Service config.

[mindw]

@ivankatliarchuk @mloiseleur

On the earlier PRs: fair point. I've dropped that line from the description.

Templates. On the service source, --target-template and --fqdn-target-template run against the Service object only, and the template FuncMap has nothing that reads Nodes. A template there can hard-code IPs, but it can't choose Nodes by label. --target-template is also used only together with --fqdn-template. Unless --combine-fqdn-annotation is set, it's skipped for Services that already have hostname annotations.

The closest setup I can see is the unstructured source over nodes.v1, with an --fqdn-target-template that skips karpenter.sh/nodepool Nodes and emits their InternalIPs. That works only for a hostname fixed in the deployment's flags. The record is then no longer tied to the Service, so the NodePort, the SRV record, externalTrafficPolicy: Local and --exclude-unschedulable no longer apply. Since --label-filter and the templates are per instance, it also needs its own ExternalDNS deployment. Is that what you meant?

#6844. As I read proposal 007, target-from names exactly one object, and the PR covers Ingress, Istio and Traefik. It can't select a set of Nodes by label.

karpenter.sh/disrupted in --exclude-unschedulable. I'd like that too, and can send it as a separate PR. On its own, though, it doesn't stop the records changing:

• Karpenter adds the taint only once disruption or deletion has started.
• The service source doesn't watch Nodes: AddEventHandler registers only Services and EndpointSlices. So a tainted Node leaves the record at the next sync, and clients keep it until the TTL expires.
• An empty Node being consolidated, or a Spot Node with a 2-minute notice, can be gone before then.
• Every new Karpenter Node joins the record when it launches.

A selector keeps short-lived Nodes out of the record for their whole life. It also covers cases with no taint to key on: the Cluster Autoscaler, or pinning a NodePort to an edge pool.

On where it belongs: which Nodes should back a NodePort record can differ per Service, for example ingress on an edge pool and another Service elsewhere. That's why it's an annotation.

[mloiseleur]

Agreed, templates and target-from don't cover this.

Still, node lifecycle is a cluster-wide concern, not a per-Service one. Could you instead:

• make --exclude-unschedulable also skip nodes tainted karpenter.sh/disrupted or ToBeDeletedByClusterAutoscaler (same signal kube-proxy uses, KEP-3836)
• reconcile the service source on Node events

A disrupted node then leaves the record within seconds, while it's still draining.

[mindw]

Agreed, it belongs at cluster level, so I'll drop the annotation.

What you describe is close to how the cloud-provider service controller picks Nodes for LoadBalancer Services (stableNodeSetPredicates). It skips:

• Nodes with deletionTimestamp set;
• Nodes tainted ToBeDeletedByClusterAutoscaler;
• Nodes labelled node.kubernetes.io/exclude-from-external-load-balancers.

That label is the cluster-wide way to keep a Node out of load balancing. k8s.io/api defines it as:

the node should not be considered as a target for external load-balancers which use nodes as a second hop

A NodePort record is that kind of load balancer, done in DNS. So I'd rework this PR to:

1. Make --exclude-unschedulable also skip Nodes that are being removed: those tainted ToBeDeletedByClusterAutoscaler or karpenter.sh/disrupted, or with deletionTimestamp set. kube-proxy's NodeEligible (KEP-3836) checks the CA taint and deletionTimestamp, but not Karpenter's taint, which Karpenter adds before it deletes the Node.
2. Leave Nodes labelled node.kubernetes.io/exclude-from-external-load-balancers out of NodePort targets, with the same rules as the service controller.
3. Make the service source re-sync on Node events whenever it has a Node informer. Updates that leave addresses, labels, taints, unschedulable and deletionTimestamp unchanged are ignored.

The taints only cover the end of a Node's life. Resolvers keep a removed IP until the TTL expires, which is 300s by default on AWS. So when an empty Node is consolidated, or a Spot Node is interrupted, clients can still be sent to it for that long. (2) handles Nodes that are short-lived by design: with Karpenter, setting the label in the NodePool template keeps its Nodes out of the record for their whole life.

Two questions:

1. --exclude-unschedulable defaults to true, and the node source uses it too. Should the new checks apply to the node source as well, or only to NodePort targets?
2. (2) changes behaviour for anyone who already uses the label to keep Nodes out of cloud load balancers: those Nodes would also leave NodePort records. That's what the label means, but I can put it behind a flag if you'd rather it be opt-in.

[mloiseleur]

1. yes, also for node source, that's what most users expect, and it can be disabled if needed
2. I'm unsure to see the interest of yet another flag. Let's keep it that way FTM, and see if there is a need.

[mindw]

@mloiseleur reworked as agreed. The annotation is gone, and I've updated the title and description.

• --exclude-unschedulable also skips Nodes with deletionTimestamp set, or tainted ToBeDeletedByClusterAutoscaler or karpenter.sh/disrupted, in both the node and service sources.
• NodePort targets skip Nodes labelled node.kubernetes.io/exclude-from-external-load-balancers, with no flag.
• The service source re-syncs on Node events. Updates that don't change addresses, labels, taints, unschedulable or deletion state are ignored.

One behaviour change to look at: with externalTrafficPolicy: Local, excluded Nodes, cordoned ones included, are now dropped before the readiness tiers are compared. So the records fall back to the next tier instead of ending up empty.

[mloiseleur]

/ok-to-test

[coveralls]

Coverage Report for CI Build 43442799060

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.04%) to 83.512%

Details

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

Uncovered Changes

No uncovered changes found.

Coverage Regressions

10 previously-covered lines in 2 files lost coverage.

File	Lines Losing Coverage	Coverage
node.go	6	93.1%
service.go	4	93.51%

---

Coverage Stats

Relevant Lines:	19887
Covered Lines:	16608
Line Coverage:	83.51%
Coverage Strength:	1464.0 hits per line

---

💛 - Coveralls

[mloiseleur]

Thanks, this is the shape we asked for.
Filtering before the Local tier ranking is a nice fix, and the Node update filter looks right (the transform already strips conditions, and the service source does not read Node annotations).
Only wording nits left.

[mloiseleur]

Thanks, all suggestions applied.

[mloiseleur]

/lgtm

[ivankatliarchuk]

/approve

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ivankatliarchuk]

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