perf(endpoint): optimize ProviderSpecific to use map for O(1) access by u-kai · Pull Request #5814 · kubernetes-sigs/external-dns

u-kai · 2025-09-06T14:58:51Z

What does it do ?

This PR optimizes the ProviderSpecific field in the Endpoint struct by changing it from a slice-based implementation ([]ProviderSpecificProperty) to a map-based implementation (map[string]string).
This change improves property access performance from O(n) linear search to O(1) constant-time lookups while maintaining full backward compatibility for CRD users.

Motivation

The current slice-based ProviderSpecific implementation creates significant performance bottlenecks in large-scale environments.
This optimization was identified as a prerequisite for addressing the performance concerns raised in PR #5799.
Before implementing provider-specific property warnings, the underlying data structure needed to be optimized to prevent the warnings themselves from becoming a performance bottleneck.

More

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

k8s-ci-robot · 2025-09-06T14:59:01Z

Hi @u-kai. 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.

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 · 2025-09-06T15:23:29Z

apis/v1alpha1/dnsendpoint.go

 // DNSEndpointSpec defines the desired state of DNSEndpoint
 type DNSEndpointSpec struct {
-	Endpoints []*endpoint.Endpoint `json:"endpoints,omitempty"`
+	Endpoints []*Endpoint `json:"endpoints"`


As you can see, the path is apis/v1alpha1. Moving Endpoints under this folder, means we are doing versioning for Endpoint object. Not against, but not sure if this is the right approach. As now we most likely going to have v1alpha.Endpoint....

I have no solution, not an easy one though

Thanks for raising this — you’re right that moving Endpoint under apis/v1alpha1 makes it part of the public API surface and therefore versioned.

A few clarifications on intent and impact:

Intentional separation:
We’re explicitly decoupling the CRD type (apis/v1alpha1.Endpoint) from the internal type (endpoint.Endpoint).
This lets us keep the CRD schema stable while giving us freedom to optimize the internal representation (e.g., map-based ProviderSpecific) without leaking those changes into the API.

No user-facing change:
The CRD schema shape for Endpoints remains the same from a user point of view; we only replaced the reference to the internal type with the versioned API type.

On future versions (v1, etc.):
If v1’s Endpoint is identical to v1alpha1, we can avoid extra helpers by using a type alias or a shared converter.
If it diverges, we’ll add a thin conversion layer and still keep the internals map cleanly.
Typically the controller consumes one served version; the apiserver handles storage-version normalization.

Maintenance cost:
We’re aware this assigns versioning responsibility to Endpoint, but it also prevents tight coupling to internals and reduces the risk for future internal refactors — or at least, we believe it can reduce such risks.

Have you generated CRDs, are they still the same or there is diff?

Sorry, I didn't handle this properly initially.
I found that the Endpoints field should have the omitempty tag.
After adding omitempty to the endpoints field and regenerating the CRDs, there are no diffs.
Everything is now in the proper state.

ivankatliarchuk · 2025-09-06T15:28:31Z

/ok-to-test

vflaux · 2025-09-09T13:27:04Z

endpoint/endpoint.go

+func (ps ProviderSpecific) String() string {
+	if len(ps) == 0 {
+		return "[]"
+	}
+	// Collect and sort keys for stable output.
+	keys := make([]string, 0, len(ps))
+	for k := range ps {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	// Build entries like "{key value}" preserving stable order.
+	b := strings.Builder{}
+	b.WriteByte('[')
+	for i, k := range keys {
+		if i > 0 {
+			b.WriteByte(' ')
+		}
+		b.WriteByte('{')
+		b.WriteString(k)
+		b.WriteByte(' ')
+		b.WriteString(ps[k])
+		b.WriteByte('}')
+	}
+	b.WriteByte(']')
+	return b.String()
+}


I'm not sure if we want to maintain code that mimic the default output from fmt?
I would prefer to keep the ProviderSpecificProperty struct (but package-private), build a slice of it from ProviderSpecific and use fmt.Sprintf().
Performance wise your code is faster though.
@ivankatliarchuk wdyt ?

edit:
Something like that:

func (ps ProviderSpecific) String() string { data := make([]providerSpecificProperty, 0, len(ps)) for k, v := range ps { data = append(data, providerSpecificProperty{Name: k, Value: v}) } slices.SortFunc(data, func(a, b providerSpecificProperty) int { return strings.Compare(a.Name, b.Name) }) return fmt.Sprint(data) }

Not too shure what this method is for.

It's to keep the log output identical. But that's mostly for debug logs.

external-dns/endpoint/endpoint.go

Line 383 in 7792e78

log.Debugf(`Skipping endpoint %v because of missing owner label (required: "%s")`, ep, ownerID)

But there is some info logs too:

external-dns/registry/dynamodb.go

Line 305 in abdf8bb

log.Infof("Skipping endpoint %v because owner does not match", ep)

external-dns/source/wrappers/targetfiltersource.go

Line 65 in b31afb4

log.WithField("endpoint", ep).Debugf("Skipping endpoint because all targets were filtered out")

@ivankatliarchuk
As @vflaux mentioned, this logic is to maintain compatibility of the log output.

it's a map, so the output should be consistent. I think I'm questioning the actual need for this method. Is formatting with %q or %v is not enough?

Hi @mloiseleur. Wdyt, do we want to keep same output in logging or Map : ProviderSpecific map[key1:value1 key2:value2 key3:value3] is just enough?

🤔 Changing logs output is not the goal of this PR.
Nonetheless, it changes the struct, so it can be somehow expected that it impacts log output.

The default output provided by Go on map is fine and well known in go software.
My recommendation is to use it as a start, for this PR.

We'll see with time if it's not enough and if we need to update it with specific code and/or struct.

@vflaux @ivankatliarchuk @u-kai Wdyt ? Does it make sense to you ?

Thanks for the feedback.
My original motivation for adding the custom String() was to keep backward compatibility in log output — since the struct changed, I wanted to avoid surprising changes in log messages.

That said, I’m also fine with simplifying the code and just returning the default map output. It makes the code cleaner, though it will slightly change the log format. If we go this route, I think it’s worth mentioning in release notes so users are aware of the change.

If you prefer that approach, I can drop the custom String() method and just let it print the map directly.

Yes, I prefer the simpler approach and drop String() method.
No problem to add a line on this in the next release notes.

I've removed the String() method and related tests as suggested.
@mloiseleur @ivankatliarchuk
Please review when you have a chance 🙇

internal/testutils/endpoint.go

plan/plan.go

provider/inmemory/inmemory.go

vflaux · 2025-09-09T14:07:56Z

provider/inmemory/inmemory.go

 			newEp.Labels[k] = v
 		}
-		newEp.ProviderSpecific = append(endpoint.ProviderSpecific(nil), ep.ProviderSpecific...)
+		if ep.ProviderSpecific != nil {


Maybe add a test case for this as this is not covered?

I'll address the test coverage for the if ep.ProviderSpecific !=nil branch in a separate PR.
The current tests use makeZone helper function which doesn't set ProviderSpecific, and modifying it would require updating all existing tests that depend on it.
A dedicated PR focused on test infrastructure improvements would be more appropriate to maintain consistency across the codebase.

source/wrappers/nat64source.go

ivankatliarchuk

Overall lgtm,. few questions left to resolve

ivankatliarchuk · 2025-09-25T09:12:18Z

source/wrappers/nat64source.go

-		v4EP := ep.DeepCopy()
-		v4EP.Targets = v4Targets
-		v4EP.RecordType = endpoint.RecordTypeA
+		v4EP := &endpoint.Endpoint{


Is deepcopy no longer works here? Not sure if we should be using endpoint.Endpoint in refactorings, dedicated methods more preferred, so we could incapsulate things in the future.

Why this code is required, with null checks and map copy?

if ep.Labels != nil { v4EP.Labels = make(endpoint.Labels, len(ep.Labels)) maps.Copy(v4EP.Labels, ep.Labels) } if ep.ProviderSpecific != nil { v4EP.ProviderSpecific = make(endpoint.ProviderSpecific, len(ep.ProviderSpecific)) maps.Copy(v4EP.ProviderSpecific, ep.ProviderSpecific) }

My understanding v4EP created without labels or provider specific, is this not just?

v4ep.Labels = ep.Labels

On DeepCopy:
This code now uses an internal Endpoint type instead of the CRD code-generated one, so we don’t have generated DeepCopy methods anymore.
I don’t think it’s worth re-implementing DeepCopy just for this use case—being explicit in the adapter keeps the conversion type-safe and makes future encapsulation easier.

On the nil checks and map copy:
A direct assignment would share the same map (Go maps are reference types), so changes to the converted value could leak back to the source.
Copying avoids aliasing and preserves nil vs empty.

ivankatliarchuk · 2025-09-25T09:16:35Z

endpoint/endpoint.go

-		if providerSpecific.Name == key {
-			return providerSpecific.Value, true
-		}
+	if e.ProviderSpecific == nil {


ivankatliarchuk · 2025-09-25T09:17:11Z

endpoint/endpoint.go

 }

 // DeleteProviderSpecificProperty deletes any ProviderSpecificProperty of the specified name.
 func (e *Endpoint) DeleteProviderSpecificProperty(key string) {


ivankatliarchuk · 2025-09-25T09:18:20Z

pkg/adapter/endpoint.go

+	"sigs.k8s.io/external-dns/endpoint"
+)
+
+func ToInternalEndpoint(crdEp *apiv1alpha1.Endpoint) *endpoint.Endpoint {


name crdEp to specific and method description is missing for exported method

ivankatliarchuk · 2025-09-25T09:18:28Z

pkg/adapter/endpoint.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2017 The Kubernetes Authors.


Suggested change

Copyright 2017 The Kubernetes Authors.

Copyright 2025 The Kubernetes Authors.

ivankatliarchuk · 2025-09-25T09:19:29Z

pkg/adapter/endpoint.go

+	return ep
+}
+
+func ToInternalEndpoints(crdEps []*apiv1alpha1.Endpoint) []*endpoint.Endpoint {


same as other method

ivankatliarchuk · 2025-09-25T09:27:02Z

provider/inmemory/inmemory.go

-		newEp.ProviderSpecific = append(endpoint.ProviderSpecific(nil), ep.ProviderSpecific...)
+		if ep.ProviderSpecific != nil {
+			newEp.ProviderSpecific = make(endpoint.ProviderSpecific)
+			maps.Copy(newEp.ProviderSpecific, ep.ProviderSpecific)


is the map copy even required, why not a direct assignment?

On the map copy: direct assignment would just alias the underlying map, since maps in Go are reference types.
That would mean v4EP and ep share the same backing map, and any mutation on one would affect the other.
To avoid this aliasing we explicitly copy into a new map, so the converted object is fully independent.

This struct is used in a variety of ways in tests, and we wanted to minimize any chance of shared state leaking between instances.
Making explicit copies ensures data isolation and makes test behavior more predictable.

ivankatliarchuk · 2025-09-25T09:33:58Z

provider/webhook/api/httpapi.go

 		w.WriteHeader(http.StatusOK)
-		if err := json.NewEncoder(w).Encode(records); err != nil {
+		apiRecords := adapter.ToAPIEndpoints(records)
+		if err := json.NewEncoder(w).Encode(apiRecords); err != nil {


Since we are only encoding valid types into an in-memory buffer, this call cannot realistically fail.
Because the failure path is not reproducible in this context, we are intentionally not covering it with tests.

ivankatliarchuk · 2025-09-25T09:34:51Z

provider/webhook/api/httpapi.go

 		log.Errorf("Failed to call adjust endpoints: %v", err)
 		w.WriteHeader(http.StatusInternalServerError)
 	}
+	pve = adapter.ToAPIEndpoints(endpoints)


It was not covered before, which probably explains why tests were green when there was an issue with webhook

ivankatliarchuk · 2025-09-25T09:49:24Z

pkg/adapter/endpoint.go

+	"sigs.k8s.io/external-dns/endpoint"
+)
+
+func ToInternalEndpoint(crdEp *apiv1alpha1.Endpoint) *endpoint.Endpoint {


The name ToInternalEndpoint not necessary is clear engough. external-dns Endpoint is not internal, as is exported. I do get the meaning probably slighly different here.

Maybe ToAPI and FromApi or ToVersionedApi and FromVersionedApi

k8s-ci-robot · 2025-09-25T09:52:21Z

[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 ivankatliarchuk. 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:

OWNERS

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

Signed-off-by: u-kai <76635578+u-kai@users.noreply.github.com>

u-kai · 2025-09-25T11:57:05Z

@ivankatliarchuk
Thank you very much for all the reviews!
I’ve made the necessary fixes and replied to the comments where a response was needed.
I’d appreciate it if you could take another look.

ivankatliarchuk · 2025-09-26T10:38:46Z

Overall lgtm. Need to find some time to execute few smoke tests on my side.

ivankatliarchuk · 2025-09-30T09:12:14Z

Seems like CRD is broken

into refactor/provider-specific-map

u-kai · 2025-09-30T09:33:17Z

@ivankatliarchuk
It doesn’t seem broken on my side when I tried it out.

Looking at your screenshot, it feels like you might be on a different branch — was that intentional?

coveralls · 2025-09-30T09:38:19Z

Pull Request Test Coverage Report for Build 18125384109

Details

0 of 0 changed or added relevant lines in 0 files are covered.
154 unchanged lines in 10 files lost coverage.
Overall coverage decreased (-0.09%) to 78.511%

Files with Coverage Reduction	New Missed Lines	%
cloudflare/cloudflare.go	2	90.86%
plan.go	3	95.21%
wrappers/nat64source.go	10	81.69%
webhook/api/httpapi.go	12	87.76%
crd.go	13	69.32%
testutils/endpoint.go	13	78.75%
inmemory/inmemory.go	14	80.4%
webhook/webhook.go	14	92.31%
endpoint.go	15	90.91%
v1alpha1/zz_generated.deepcopy.go	58	0.0%

Totals
Change from base Build 18123647456:	-0.09%
Covered Lines:	15845
Relevant Lines:	20182

💛 - Coveralls

ivankatliarchuk · 2025-09-30T09:57:30Z

Yeap. Sry incorrect branch

szuecs

This PR tries to speedup things, but there was nothing shown that:

it's an actual problem at all
how much faster or slower we will be by applying this gigantic risky change.

u-kai · 2025-09-30T23:59:42Z

@szuecs

Let me clarify the motivation:

In the current implementation, we loop through multiple ProviderSpecific keys for each endpoint, and each check internally loops again over the slice. This effectively results in a nested loop.
That means key lookups are O(n) (linear search in a slice). By moving to a map, lookups become O(1) on average, so the overall complexity improves from O(E × K) to O(E) where E = number of endpoints and K = number of keys.
In very large environments (e.g. hundreds of thousands of endpoints), this can become problematic.
Moreover, this nested-loop pattern can be seen in multiple providers. Fixing the underlying data structure here can serve as a precedent and simplify improvements in those other cases as well.
Based on previous discussions with maintainers in feat(aws): add warning for provider-specific properties without setIdentifier #5799 , we agreed to start by improving the data structure itself before addressing further ProviderSpecific logic.

szuecs · 2025-10-01T06:00:27Z

/close

Please no ai for cv driven work.
I asked not for an explanation but a benchmark and numbers.
You seem not to understand what it means.
Make sure you create change requests that are reviewable and show data.

k8s-ci-robot · 2025-10-01T06:00:35Z

@szuecs: Closed this PR.

Details

In response to this:

/close

Please no ai for cv driven work.
I asked not for an explanation but a benchmark and numbers.
You seem not to understand what it means.
Make sure you create change requests that are reviewable and show data.

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.

u-kai · 2025-10-02T12:49:55Z

@szuecs
I created a benchmark under the assumption that ProviderSpecific usually has ~30 Get/Delete ops per endpoint (based on AWS Provider’s AdjustEndpoint/ApplyChanges).
Results show slice is consistently faster for <=5 keys, and map only starts to outperform when >10 keys.
Since ProviderSpecific rarely exceeds 3–5 keys in practice, it seems slice is the better choice.
Full code + results: https://gist.github.com/u-kai/d29f97409c9f5174cb868d534f949a30

@ivankatliarchuk @mloiseleur @vflaux
That said, I’d appreciate a sanity check in case my benchmark approach or assumptions are flawed.
If the approach is correct, I think we should stick with slice and close this PR.
If that’s the case, I apologize for the time maintainers already spent reviewing — and I’m very grateful for the feedback and guidance 🙇

ivankatliarchuk · 2025-10-07T08:04:32Z

I'll work on benchmark as well. May take few month

szuecs · 2025-10-09T20:28:49Z

Thanks @u-kai for your work!

I think you could create a pr with the benchmark and then we can use that for testing optimizations that someone will come up with.

k8s-ci-robot added the apis Issues or PRs related to API change label Sep 6, 2025

k8s-ci-robot requested review from mloiseleur and szuecs September 6, 2025 14:58

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 6, 2025

ivankatliarchuk reviewed Sep 6, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2025

u-kai requested a review from ivankatliarchuk September 6, 2025 21:35

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

u-kai force-pushed the refactor/provider-specific-map branch from a2c62d3 to a2c0d29 Compare September 8, 2025 00:10

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 8, 2025

u-kai force-pushed the refactor/provider-specific-map branch from a2c0d29 to 921fb70 Compare September 9, 2025 12:08

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