Skip to content

feat(template): support repeatable --fqdn-template, --target-template, --fqdn-target-template flags#6429

Open
ivankatliarchuk wants to merge 8 commits into
kubernetes-sigs:masterfrom
gofogo:feat/multi-fqdn-target-template-flags
Open

feat(template): support repeatable --fqdn-template, --target-template, --fqdn-target-template flags#6429
ivankatliarchuk wants to merge 8 commits into
kubernetes-sigs:masterfrom
gofogo:feat/multi-fqdn-target-template-flags

Conversation

@ivankatliarchuk
Copy link
Copy Markdown
Member

@ivankatliarchuk ivankatliarchuk commented May 10, 2026

What does it do ?

  • FQDNTemplate, TargetTemplate, and FQDNTargetTemplate on Config changed from string to []string; CLI flags switched from StringVar to StringsVar so each can be specified multiple times
  • fqdn-templating.md updated to show repeated-flag syntax as the canonical multi-template form, with FAQ and tips refreshed
  • unstructured.md configuration table marks all three flags as repeatable; EndpointSlice example comment fixed and the double-flag pattern explained as intentional and now correct

Motivation

  • --fqdn-template, --target-template, and --fqdn-target-template each accepted only a single string; multiple templates had to be crammed into one value as comma-separated text
  • The unstructured source docs already showed --fqdn-target-template specified twice (EndpointSlice example), but with StringVar only the last value was used - the first was silently discarded

More

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

…arget-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 10, 2026
@k8s-ci-robot k8s-ci-robot requested review from mloiseleur and szuecs May 10, 2026 13:32
@k8s-ci-robot k8s-ci-robot added apis Issues or PRs related to API change docs source size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2026
…arget-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@coveralls
Copy link
Copy Markdown

coveralls commented May 10, 2026

Coverage Report for CI Build 25665899121

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 decreased (-0.01%) to 80.572%

Details

  • Coverage decreased (-0.01%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • 49 coverage regressions across 4 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

49 previously-covered lines in 4 files lost coverage.

File Lines Losing Coverage Coverage
istio_virtualservice.go 26 87.83%
istio_gateway.go 12 90.3%
template/engine.go 8 92.79%
pod.go 3 97.19%

Coverage Stats

Coverage Status
Relevant Lines: 21397
Covered Lines: 17240
Line Coverage: 80.57%
Coverage Strength: 1451.99 hits per line

💛 - Coveralls

Copy link
Copy Markdown
Contributor

@vflaux vflaux left a comment

Choose a reason for hiding this comment

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

One caveat concerns the use of variables or define blocks in templates. Because flags are concatenated into a single, unified template, these variables are shared across all flags. This can lead to unexpected behavior for users.
This should at least be clearly documented.

@ivankatliarchuk
Copy link
Copy Markdown
Member Author

Could you clarify those unexpected behaviours? I might missing them

Copy link
Copy Markdown
Contributor

@vflaux vflaux left a comment

Choose a reason for hiding this comment

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

Variable should actually not cause issues as you can redefine them, but define block will :

  • {{ define "foo" }}bar{{ end }}{{ template "foo" }}
  • {{ define "foo" }}foobar{{ end }}{{ template "foo" }}

Should results in multiple definition of template "foo".
Also any invalid template will make other templates invalid too.

@ivankatliarchuk
Copy link
Copy Markdown
Member Author

Variable should actually not cause issues as you can redefine them, but define block will :

* `{{ define "foo" }}bar{{ end }}{{ template "foo" }}`

* `{{ define "foo" }}foobar{{ end }}{{ template "foo" }}`

Should results in multiple definition of template "foo". Also any invalid template will make other templates invalid too.

I'm missing the point here, why we care about edge cases, there are way too many things that could go wrong, user input is responsibility of the user, unless we could automate checks? How supporting multiple templates flags relates to all of this? Currently user could define multiple templates in single line, this is just to allow to define them in multiple flags. Template engine is supported for a while, we are using golang template engine. If the user define incorrect template, or missplace variable (could not be compiled or whatever) high chances the external-dns template engine will catch that and it will fail fast

tmpls, err := template.NewEngine(cfg.FQDNTemplate, cfg.TargetTemplate, cfg.FQDNTargetTemplate, cfg.CombineFQDNAndAnnotation)
.

There are two states resulting template either parsed successfully or not.

Could you specify what need to be added and where, I could copy paste it then.

…arget-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
…arget-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk ivankatliarchuk changed the title feat: support repeatable --fqdn-template, --target-template, --fqdn-target-template flags feat(template): support repeatable --fqdn-template, --target-template, --fqdn-target-template flags May 11, 2026
@ivankatliarchuk
Copy link
Copy Markdown
Member Author

I've removed sort for flags, to make order clear and added logging for resulting templates. The right location for logging is execute.go but it is hard to test it there

@ivankatliarchuk ivankatliarchuk marked this pull request as draft May 11, 2026 09:18
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2026
…, --fqdn-target-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk
Copy link
Copy Markdown
Member Author

I've added validation and debug logging to make things clear in case of config misconfiguration.

…, --fqdn-target-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk ivankatliarchuk marked this pull request as ready for review May 11, 2026 10:18
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2026
@k8s-ci-robot k8s-ci-robot requested a review from u-kai May 11, 2026 10:18
@ivankatliarchuk ivankatliarchuk marked this pull request as draft May 11, 2026 10:27
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2026
…, --fqdn-target-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
@ivankatliarchuk ivankatliarchuk marked this pull request as ready for review May 11, 2026 10:54
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2026
@ivankatliarchuk ivankatliarchuk requested a review from vflaux May 11, 2026 10:54
…, --fqdn-target-template flags

Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Copy link
Copy Markdown
Member

@u-kai u-kai left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2026
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: u-kai
Once this PR has been reviewed and has the lgtm label, please assign szuecs for approval. 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:

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

@u-kai
Copy link
Copy Markdown
Member

u-kai commented May 17, 2026

Since logs are now emitted, I can't claim there are zero edge cases, but the pre-fix behavior was already broken (specifying --fqdn-target-template multiple times silently discarded all but the last value). IMO, given that, this fix is the right call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apis Issues or PRs related to API change cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. docs lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. source

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants