feat(ns1): optionally identify NS1 zone by handle instead of FQDN #5750

cbertinato · 2025-08-18T19:58:22Z

What does it do ?

This PR introduces the ability to explicitly override a zone FQDN with a "zone handle" for the NS1 provider, which is an organization-unique identifier for a zone in NS1.

Motivation

When a zone is created in NS1 and a zone handle is not specified, then the zone handle is the same as the zone FQDN. However, if a zone has been created with a zone handle that is different from its FQDN, then external-dns will fail to find that zone in NS1.

The functionality allows one to explicitly map an FQDN to a zone handle via flags:

--ns1-zone-handle-map=dev.example.com=dev-view

and by environment variable:

EXTERNAL_DNS_NS1_ZONE_HANDLE_MAP="example.com=corp-prod\ndev.example.com=dev-view"

If a zone handle override is not found for a given FQDN, then external-dns will default to using the FQDN to identify the zone.

More

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

linux-foundation-easycla · 2025-08-18T19:58:28Z

The committers listed above are authorized under a signed CLA.

✅ login: cbertinato / name: Chris Bertinato (4382139, 3625a8e, eae5cc3, b2242b3)

k8s-ci-robot · 2025-08-18T19:58:31Z

Welcome @cbertinato!

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

k8s-ci-robot · 2025-08-18T19:58:32Z

Hi @cbertinato. 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.

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.

mloiseleur

@cbertinato Thanks for this PR.

Would you please update the ns1 tutorial accordingly ?

/ok-to-test

cbertinato · 2025-08-21T02:17:26Z

@mloiseleur you got it!

docs/tutorials/ns1.md

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

mloiseleur · 2025-08-22T06:43:30Z

/lgtm

ivankatliarchuk · 2025-08-24T08:45:43Z

Could you share evidences that NS1 provider still works with and without this new flag?

szuecs

Thanks for the PR, mostly cosmetic stuff but the error handling is a blocker for me.

szuecs · 2025-08-24T10:38:23Z

pkg/apis/externaldns/types.go

-	EmitEvents                                    []string
-	ForceDefaultTargets                           bool
-	sourceWrappers                                map[string]bool // map of source wrappers, e.g. "targetfilter", "nat64"
+	// Accepts repeatable --ns1-zone-handle-map flags or a comma-separated


Either godoc style or none as the others.

szuecs · 2025-08-24T10:39:47Z

provider/ns1/ns1.go

@@ -91,6 +91,11 @@ type NS1Config struct {
 	NS1IgnoreSSL  bool
 	DryRun        bool
 	MinTTLSeconds int
+	// Optional: map a zone FQDN (or suffix) to the NS1 zone handle/ID to use


Please godoc style or no comment

szuecs · 2025-08-24T10:41:08Z

provider/ns1/ns1.go

 		if err != nil {
-			return nil, err
+			if lookup != strings.TrimSuffix(zone.Zone, ".") {
+				// fallback to FQDN lookup if override missed


I think we should better fail by an error.

Do you mean return an error instead of falling back?

szuecs · 2025-08-24T10:41:26Z

provider/ns1/ns1.go

 		if err != nil {
-			return nil, err
+			if lookup != strings.TrimSuffix(zone.Zone, ".") {
+				// fallback to FQDN lookup if override missed


I think we should better fail by an error.

szuecs · 2025-08-24T10:44:19Z

provider/ns1/ns1.go

+	if len(m) == 0 {
+		return map[string]string{}
+	}
+	out := make(map[string]string, len(m))


I think you can drop the len hint for maps. You won't have any advantage in doing this and it is uncommon for maps to have it.

szuecs · 2025-08-24T10:45:26Z

provider/ns1/ns1.go

+	for k, v := range m {
+		kk := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(k)), ".")
+		vv := strings.ToLower(strings.TrimSpace(v))
+		if kk != "" && vv != "" {


Don't you want to log anything because it looks like an error if you end up ignoring empty string.

szuecs · 2025-08-24T10:47:29Z

provider/ns1/ns1.go

+// otherwise return the normalized FQDN.
+func (p *NS1Provider) zoneLookupKeyFor(fqdn string) string {
+	name := strings.TrimSuffix(strings.ToLower(strings.TrimSpace(fqdn)), ".")
+	bestKey := ""


Maybe change the name to longestMatch and you can delete the comment below.

szuecs · 2025-08-24T10:47:43Z

provider/ns1/ns1.go

+	for k := range p.zoneHandleOverrides {
+		if name == k || strings.HasSuffix(name, "."+k) {
+			if len(k) > len(bestKey) {
+				bestKey = k // longest (most specific) match wins


This comment

szuecs · 2025-08-24T10:48:17Z

provider/ns1/ns1_test.go

 	assert.Len(t, changes["bar.com"], 1)
 	assert.Len(t, changes["foo.com"], 3)
 }
+
+// helper: build a change


Drop the comment because it ha no value

k8s-ci-robot · 2025-08-24T10:49:42Z

[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 mloiseleur. For more information see the Code Review Process.

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

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

cbertinato · 2025-08-28T14:41:31Z

@ivankatliarchuk yes, I will do. Also, I will address all feedback @szuecs.

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2025

k8s-ci-robot requested review from ivankatliarchuk and szuecs August 18, 2025 19:58

mloiseleur reviewed Aug 20, 2025

View reviewed changes

cbertinato added 2 commits August 20, 2025 17:15

Allow for override of NS1 zone FQDN

b2242b3

Update tutorial; fix lint issues

3625a8e

cbertinato force-pushed the map-ns1-zone-handle branch from 949f0e7 to 3625a8e Compare August 20, 2025 21:16

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2025

mloiseleur reviewed Aug 21, 2025

View reviewed changes

docs/tutorials/ns1.md Outdated Show resolved Hide resolved

Update docs/tutorials/ns1.md

eae5cc3

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

k8s-ci-robot assigned mloiseleur Aug 22, 2025

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

szuecs requested changes Aug 24, 2025

View reviewed changes

k8s-ci-robot assigned szuecs Aug 24, 2025

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

Address feedback

4382139

feat(ns1): optionally identify NS1 zone by handle instead of FQDN #5750

Are you sure you want to change the base?

feat(ns1): optionally identify NS1 zone by handle instead of FQDN #5750

Conversation

cbertinato commented Aug 18, 2025

What does it do ?

Motivation

More

Uh oh!

linux-foundation-easycla bot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 18, 2025

Uh oh!

k8s-ci-robot commented Aug 18, 2025

Uh oh!

mloiseleur left a comment

Choose a reason for hiding this comment

Uh oh!

cbertinato commented Aug 21, 2025

Uh oh!

Uh oh!

mloiseleur commented Aug 22, 2025

Uh oh!

ivankatliarchuk commented Aug 24, 2025

Uh oh!

szuecs left a comment

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!

k8s-ci-robot commented Aug 24, 2025

Uh oh!

cbertinato commented Aug 28, 2025

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Aug 18, 2025 •

edited

Loading