feat(source/pod): add support for fqdn templating #5512

ivankatliarchuk · 2025-06-09T08:38:48Z

What does it do ?

Motivation

Worth to add support for FQDN templating for pods. Similar to nodes

fix(source/node): template expansion #5498

Quite useful for debugging and create on the fly DNS records based on different pod spec or annotations without the need to restart/reboot them or apply any other sources.

What else I found, pod source does not support

target annotation
host annotaions
resource label is not set
ttl is not supported Proposal: Support for Configurable TTL via Annotations for Pod-Based ExternalDNS Entries #5069
provider specific annotations not supported
We currently have clusters with 1500+ nodes, and miriads of pods, this line seems like not cached

external-dns/source/pod.go

Line 167 in 3e0c509

node, err := ps.nodeInformer.Lister().Get(pod.Spec.NodeName)

. Just leaving it here, for someone to create a fix or as a reminder

More

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

Signed-off-by: ivan katliarchuk <[email protected]>

ivankatliarchuk · 2025-06-09T08:46:09Z

Hi @vflaux wdyt to review this feature?

Probably worth to have a look, there is one test, and it seems bit flakey #5470 (comment)

Never seen it before the refactoring.

ivankatliarchuk · 2025-06-09T14:35:59Z

Hi @vflaux wdyt to review this feature?

Probably worth to have a look, there is one test, and it seems bit flakey #5470 (comment)

Never seen it before the refactoring.

possibly flaky test is fixed #5514, we will see it

source/pod.go

source/pod_test.go

vflaux · 2025-06-10T11:46:10Z

source/pod_test.go

+			// if tc.expectedDebugMsgs != nil {
+			// 	for _, expectedMsg := range tc.expectedDebugMsgs {
+			// 		testutils.TestHelperLogContains(expectedMsg, hook, t)
+			// 	}
+			// } else {
+			// 	require.Empty(t, hook.AllEntries(), "Expected no debug messages")
+			// }


Should this still be commented?

Nice one. Need to uncomment this

Yeah. There were a bug, fixed in this pr #5514. Merged with master

vflaux · 2025-06-10T11:47:27Z

source/pod_test.go

+
+			// for _, ep := range endpoints {
+			// 	// TODO: source should always set the resource label key. currently not supported by the pod source.
+			// 	assert.Empty(t, ep.Labels, "Labels should not be empty for endpoint %s", ep.DNSName)
+			// 	assert.NotContains(t, ep.Labels, endpoint.ResourceLabelKey)
+			// }


Should this still be commented?

Maybe use require instead of assert?

changed to require

vflaux · 2025-06-10T11:51:17Z

source/pod.go

 }

-func (*podSource) AddEventHandler(ctx context.Context, handler func()) {
+func (*podSource) AddEventHandler(_ context.Context, _ func()) {


Out of curiosity, do you know why this function does nothing? (Other sources often add the handler to informers)

Yeah. We need to fix that as well. Related PR #5274

We need to review and fix all the handlers. It's a useful feature. I'm not too sure how it differ from adding handlers in constructur, that the only gotcha

vflaux · 2025-06-10T12:39:48Z

source/pod.go

 	}

 	endpointMap := make(map[endpoint.EndpointKey][]string)
+	fqdnEndpointMap := make(map[endpoint.EndpointKey][]string)


Is this map there to ensure the fqdn endpoints get priority over pod endpoints by adding them to endpointMap at the end? Or we could copy directly into endpointMap without it?

I can try to copy. The code could be slightly more complex. As currently we could merge them at the end. Do you want me try to re-use single map?

The main challange I experience, is that ps.addPodEndpointsToEndpointMap(endpointMap, pod) actually modify same map. But still possible

done. removed fqdn map, and we are using single map

vflaux · 2025-06-10T12:42:07Z

source/pod.go

+		if ps.fqdnTemplate != nil {
+			fqdnHosts, err := ps.hostsFromTemplate(pod)
+			if err != nil {
+				log.Debug(err)


Shouldn't this be a warning at least? Or this is expected to "silently" ignore errors?

Someone may decide to use FQDN with {{ .PodSpec.PodDNSConfig }}, one pod could have it, the other pod not necessary. We may have too many warnings. No prference, shell I change it to warn?

I've looked at other Sources and they return the error directly.
I'm not sure what is the best thing to do here.

Make sense. I'll do the same then

Changed to return error directly as in others.

source/pod_fqdn_test.go

k8s-ci-robot · 2025-06-10T13:38:54Z

@vflaux: changing LGTM is restricted to collaborators

In response to this:

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.

* master: test(source): fix flaky test (kubernetes-sigs#5514) chore(source/node): added resouce labels for nodes (kubernetes-sigs#5509) chore(deps): bump the dev-dependencies group across 1 directory with 14 updates (kubernetes-sigs#5511) fix(ovh): correct handling of records deletion (kubernetes-sigs#5450)

Signed-off-by: ivan katliarchuk <[email protected]>

Co-authored-by: vflaux <[email protected]>

Signed-off-by: ivan katliarchuk <[email protected]>

k8s-ci-robot · 2025-06-11T09:51:48Z

@vflaux: changing LGTM is restricted to collaborators

In response to this:

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 · 2025-06-12T06:06:56Z

/assign @mloiseleur

docs/advanced/fqdn-templating.md

mloiseleur

/lgtm
/approve

k8s-ci-robot · 2025-06-13T06:29:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur, vflaux

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mloiseleur]

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

Co-authored-by: Michel Loiseleur <[email protected]>

ivankatliarchuk · 2025-06-13T06:57:17Z

/lgtm

k8s-ci-robot · 2025-06-13T06:57:18Z

@ivankatliarchuk: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

Signed-off-by: ivan katliarchuk <[email protected]>

…t-for-fqdn' into feat-pod-add-support-for-fqdn * refs/remotes/origin/feat-pod-add-support-for-fqdn: feat(source/pod): add support for fqdn templating

mloiseleur · 2025-06-13T12:32:35Z

/lgtm

feat(source/pod): add support for fqdn templating

fc790f3

Signed-off-by: ivan katliarchuk <[email protected]>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2025

k8s-ci-robot requested a review from mloiseleur June 9, 2025 08:39

k8s-ci-robot added the docs label Jun 9, 2025

k8s-ci-robot requested a review from szuecs June 9, 2025 08:39

k8s-ci-robot added source size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 9, 2025

vflaux suggested changes Jun 10, 2025

View reviewed changes

ivankatliarchuk and others added 7 commits June 10, 2025 16:13

feat(source/pod): add support for fqdn templating

c4a7d10

Signed-off-by: ivan katliarchuk <[email protected]>

feat(source/pod): add support for fqdn templating

77f1127

Signed-off-by: ivan katliarchuk <[email protected]>

feat(source/pod): add support for fqdn templating

f036698

Co-authored-by: vflaux <[email protected]>

feat(source/pod): add support for fqdn templating

66c92f9

Signed-off-by: ivan katliarchuk <[email protected]>

feat(source/pod): add support for fqdn templating

343f1aa

Signed-off-by: ivan katliarchuk <[email protected]>

feat(source/pod): add support for fqdn templating

500db5a

Signed-off-by: ivan katliarchuk <[email protected]>

ivankatliarchuk requested a review from vflaux June 11, 2025 07:46

vflaux approved these changes Jun 11, 2025

View reviewed changes

k8s-ci-robot assigned mloiseleur Jun 12, 2025

mloiseleur reviewed Jun 13, 2025

View reviewed changes

docs/advanced/fqdn-templating.md Outdated Show resolved Hide resolved

mloiseleur approved these changes Jun 13, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2025

feat(source/pod): add support for fqdn templating

c8a549c

Co-authored-by: Michel Loiseleur <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2025

ivankatliarchuk added 2 commits June 13, 2025 10:33

feat(source/pod): add support for fqdn templating

626933e

Signed-off-by: ivan katliarchuk <[email protected]>

Merge remote-tracking branch 'refs/remotes/origin/feat-pod-add-suppor…

f4e5410

…t-for-fqdn' into feat-pod-add-support-for-fqdn * refs/remotes/origin/feat-pod-add-support-for-fqdn: feat(source/pod): add support for fqdn templating

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2025

k8s-ci-robot merged commit 7792986 into kubernetes-sigs:master Jun 13, 2025
13 checks passed

ivankatliarchuk deleted the feat-pod-add-support-for-fqdn branch June 14, 2025 09:15

feat(source/pod): add support for fqdn templating #5512

feat(source/pod): add support for fqdn templating #5512

Uh oh!

Conversation

ivankatliarchuk commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does it do ?

Motivation

More

Uh oh!

ivankatliarchuk commented Jun 9, 2025

Uh oh!

ivankatliarchuk commented Jun 9, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivankatliarchuk Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 10, 2025

Uh oh!

k8s-ci-robot commented Jun 11, 2025

Uh oh!

ivankatliarchuk commented Jun 12, 2025

Uh oh!

Uh oh!

mloiseleur left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 13, 2025

Uh oh!

ivankatliarchuk commented Jun 13, 2025

Uh oh!

k8s-ci-robot commented Jun 13, 2025

Uh oh!

mloiseleur commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

ivankatliarchuk commented Jun 9, 2025 •

edited

Loading

ivankatliarchuk Jun 10, 2025 •

edited

Loading