fix(annotations): initialize annotation keys at declaration time

[ivankatliarchuk]

What does it do ?

Rollback changes that was added here #5889

Without this fix, every test file that touches annotations needs workarounds, and test order becomes fragile. And now tests need to know about runtime behaviour......

Curretnly, annotation keys are uninitialized strings that did required init() or explicit SetAnnotationPrefix() calls to populate. This caused issues where tests needed boilerplate like TestMain just to ensure defaults were set.

This is still a band-aid though. The root issue is global mutable state. A cleaner long-term solution would be passing prefix configuration through context or making annotation keys instance-based rather than package-level globals.

• Initialize all annotation key variables with default values at declaration time
• Remove need for init() function
• SetAnnotationPrefix now only needed when customizing prefix (runtime config)
• Specific Tests use DefaultAnnotationPrefix constant for maintainability
• Added whitespace handling to SetAnnotationPrefix. As empty strings creates undocumented issues.

Some other PR, curerrently in review

Motivation

SetAnnotationPrefix is called at runtime from controller/execute.go to configure custom prefixes. However, the current implementation skips updates when the prefix equals

the default (DefaultAnnotationPrefix == prefix), which prevents:

• Tests from resetting to a known state (e.g., TestMain in cloudflare_test.go)
• Any code that needs to explicitly ensure the default prefix is active

It was added here #5889

More

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

[coveralls]

Pull Request Test Coverage Report for Build 21551962450

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 298 unchanged lines in 10 files lost coverage.
• Overall coverage decreased (-0.02%) to 79.158%

---

Files with Coverage Reduction	New Missed Lines	%
txt/registry.go	1	93.72%
service.go	12	93.12%
f5_transportserver.go	14	76.92%
f5_virtualserver.go	16	78.79%
contour_httpproxy.go	18	83.33%
openshift_route.go	18	84.73%
ambassador_host.go	20	82.99%
ovh/ovh.go	33	91.75%
kong_tcpingress.go	59	47.95%
traefik_proxy.go	107	66.67%

Totals
Change from base Build 21510752750:	-0.02%
Covered Lines:	16039
Relevant Lines:	20262

---

💛 - Coveralls

[vflaux]

Shoudn't this return an error instead of silently ignoring the value.
Maybe add a proper validation? validation.IsDNS1123Subdomain() is what validate annotation prefixes I think.

[ivankatliarchuk]

This is not a DNS subdomain, this is a kubernetes annotation prefix current default external-dns.alpha.kubernetes.io/. Basically any annotation prefix could be used

[ivankatliarchuk]

The method could return an error, but I'd rather to keep it as is for now. I'm not actually a big fan of this functionality. In past we had always all annotations defaulted and prefixed with "external-dns.alpha.kubernetes.io/". This is a runtime hack for split brain functionality.

Why I think is better to keep as is(aka without error return)

• default behaviour is here
• this method require a proper validation for annotations prefixes(could be added in the future)

I'd rather remove this checks `prefix == "" || !strings.HasSuffix(prefix, "/"), wdyt?

Returnign error could be added in follow up. I'm still looking for options to completely remove this method

[ivankatliarchuk]

This checks a mainly for tests. Runtime validation is here pkg/apis/externaldns/validation/validation.go

[vflaux]

Basically any annotation prefix could be used

There is a validation on the prefix. With a_/b the api-server returns:

Invalid value: "a_/b": prefix part a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

However, external-dns only reads the annotations, so validation isn’t required anyway. If an invalid prefix is set, it simply won’t match any annotation.

I'd rather remove this checks `prefix == "" || !strings.HasSuffix(prefix, "/"), wdyt?

This can probably be removed.

[ivankatliarchuk]

Removed

[ivankatliarchuk]

Basically any annotation prefix could be used

There is a validation on the prefix. With a_/b the api-server returns:

Invalid value: "a_/b": prefix part a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

However, external-dns only reads the annotations, so validation isn’t required anyway. If an invalid prefix is set, it simply won’t match any annotation.

I'd rather remove this checks `prefix == "" || !strings.HasSuffix(prefix, "/"), wdyt?

This can probably be removed.

The behavour is sligthly different. If incorrect annotation is set for split brain functionality -> one could end up wihtout records. But not a big deal at the moment. This split brain feature is complex and risky

[ivankatliarchuk]

But this is an issue of brain split feature itself - too complex

[vflaux]

We may use a function for this:

func setAnnotationKeyPrefix(prefix string, key *string) {
    suffix,_ := strings.CutPrefix(DefaultAnnotationPrefix, *key)
	*key = prefix + suffix
}

setAnnotationKeyPrefix(prefix, &CloudflareProxiedKey)

This would avoid potential typos.

[ivankatliarchuk]

This will create more issues with initial problem. CloudflareProxiedKey is not a suffix, CloudflareProxiedKey must be defaulted always to external-dns.alpha.kubernetes.io/cloudflare-key. This is what I'm trying to fix.

At the moment, in all tests annotation.CloudflareProxiedKey is just cloudlare-key without annotation prefix, when it was beforeexternal-dns.alpha.kubernetes.io/cloudflare-key. So now in pretty much every test we need

annotations.SetAnnotationPrefix("external-dns.alpha.kubernetes.io/")

I may missing something in your suggestion, or there is misunderstanding what the issue curretly is.

Currently without this fix, every test file that touches annotations needs workarounds, and test order becomes fragile. And now tests need to know about runtime behaviour......

[vflaux]

What I understand is that you define default values for the annotation keys and when SetAnnotationPrefix() is called it changes the values with a new prefix.
My suggestion is to use a function that replace the prefix on all annotations like this:

// SetAnnotationPrefix sets a custom annotation prefix and rebuilds all annotation keys.
// This must be called before any sources are initialized.
// The prefix must end with '/'.
func SetAnnotationPrefix(prefix string) {
	prefix = strings.TrimSpace(prefix)
	if prefix == "" || !strings.HasSuffix(prefix, "/") {
		return
	}

	replaceAnnotationPrefix(
		AnnotationKeyPrefix,
		prefix,
		// Cloudflare annotations
		&CloudflareProxiedKey,
		&CloudflareCustomHostnameKey,
		&CloudflareRegionKey,
		&CloudflareRecordCommentKey,
		&CloudflareTagsKey,
		// Provider prefixes
		&AWSPrefix,
		&CoreDNSPrefix,
		&SCWPrefix,
		&WebhookPrefix,
		&CloudflarePrefix,
		// Core annotations
		&TtlKey,
		&SetIdentifierKey,
		&AliasKey,
		&TargetKey,
		&ControllerKey,
		&HostnameKey,
		&AccessKey,
		&EndpointsTypeKey,
		&Ingress,
		&IngressHostnameSourceKey,
		&InternalHostnameKey,
		&GatewayHostnameSourceKey,
	)

	AnnotationKeyPrefix = prefix
}

func replaceAnnotationPrefix(currentPrefix, newPrefix string, keys ...*string) {
	for _, key := range keys {
		suffix, found := strings.CutPrefix(*key, currentPrefix)
		if !found {
			panic(fmt.Sprintf("annotation key %q does not have default prefix %s ", *key, currentPrefix))
		}
		*key = newPrefix + suffix
	}
}

There’s no need to duplicate the annotation suffixes, which eliminates any risk of typos.
wdyt?

[ivankatliarchuk]

I understand but do not agree with this proposal. Please have a look, I'm rolling back a change that does break how things should be.

If you think we should add this function in function, open a pull request lets discuss.

This public function is a hack. It changes the behaviour of annotations at runtime that cause and brake normal flow for tests. It should not exist. This is my view. I'm just trying to get the code to work as before.

[ivankatliarchuk]

I don't understand why there is a spelling problem, when I'm clearly solving different problem.

Could you explain why to extend the scope of the initial pull request?

[ivankatliarchuk]

My view I described in title

• anything that could remove that public function -- make sense to folloup
• anything to extend the current function is just building more tech debt on top of the current hack

[vflaux]

I'm rolling back a change that does break how things should be.

I missed that it was a rollback of a previous PR, my mistake.

[ivankatliarchuk]

Thanks. Feel free to submit a new PR with your proposal. On my side, I'm trying to undersant how to remove this fucntions ;-0

[vflaux]

/lgtm

[ivankatliarchuk]

Hi @vflaux. I removed #6159 (comment)

[vflaux]

/lgtm

[ivankatliarchuk]

/approve

[k8s-ci-robot]

[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