fix(endpoint): compare every target in Targets.Same

[PixiBixi]

What does it do ?

Targets.Same returned on the first differing pair when both sides parsed as IP
addresses, so that one pair decided the whole comparison. It now keeps going
through the rest of the targets.

While in there, Same and IsLess no longer sort their receiver and their
argument in place.

Motivation

Found this reading through the plan and registry path. If the first target is an
IPv6 that the provider returns in a different form than the source has it
(shortened vs expanded), the two strings differ, both parse as IPs, and the
function returns "same" right there. Anything after it is never looked at.

a := Targets{"::1", "foo.example.com"}
b := Targets{"::0001", "bar.example.com"}
a.Same(b) // true

plan.targetChanged uses this, so no update gets planned and the record stays
stale. No error, nothing in the logs.

The in-place sort is a separate thing I noticed while writing the test. plan
calls Same on endpoints the TXT registry keeps in its cache, so the reordering
survives into the next reconciliation. It sorts a copy now, and skips it when the
targets are already in order.

More

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

[kubernetes-prow]

[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 assign mloiseleur 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:

• OWNERS

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

[kubernetes-prow]

Hi @PixiBixi. 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]

Could you provide real kubernetes setup, where the problem could be reproduced? I'm not fully understand what this actually fixing, I couldn't construct a failing scenario against current setup in my head. Looks more like a correctness of sorting algorithm at the moment.

I see here a behavior change bundled into what's framed as a pure bug fix aka "alphabetically first" to "first in source order". Most likely low risk.

+ On top of that, the new code no longer mutates the caller's slice, which the old code did as an undocumented side effect. Under the old code, the first comparison in a scan physically sorted a candidate's Targets in place; every later comparison touching that same candidate then hit an already-sorted slice, which sort.Stable handles cheaply. Under the new sortedCopy, the original slice is never touched, so sort.IsSorted fails on every call and each comparison clones and re-sorts from scratch - there's no amortization across a multi-candidate scan anymore..

[ivankatliarchuk]

Same and IsLess still each hand-roll their own IP-parse-and-compare loop, and the Len/Less/Swap sort.Interface trio only exists to support the old in-place sort.Sort/sort.Stable calls this PR is replacing - nothing else in the repo calls sort on a Targets value or uses those methods directly.

[ivankatliarchuk]

/ok-to-test

[coveralls]

Coverage Report for CI Build 33436331167

Coverage decreased (-0.006%) to 82.075%

Details

• Coverage decreased (-0.006%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• 1 coverage regression across 1 file.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
openshift_route.go	1	86.07%

---

Coverage Stats

Relevant Lines:	21339
Covered Lines:	17514
Line Coverage:	82.08%
Coverage Strength:	1466.12 hits per line

---

💛 - Coveralls

[PixiBixi]

You're right on both counts, I dropped the sortedCopy change. The PR is now just the early return plus a repro.

Repro: controller/reconcile_ipv6_test.go. Fake client, service source, TXT registry, and a provider that canonicalizes IPv6 targets the way Route53, Cloudflare and Azure do. A Service annotated target: 2001:0db8:0000:0000:0000:0000:0000:0001,2001:db8::2 syncs fine. Change only the second target to 2001:db8::3, and on master the provider keeps 2001:db8::2:

--- FAIL: TestReconcileIPv6SecondTargetChangeReachesProvider
    Messages: the change to the second target never reached the provider

The provider hands back 2001:db8::1 while the annotation still holds the expanded form. Not string-equal, so they reach the IP fallback, and that fallback returns instead of continuing. The second target is never looked at. No error, nothing logged.

Order: confirmed. The source produced 2001:0db8:...:0002 ; 2001:db8::1, master sends the reverse. That ordering is a side effect of the in-place sort in Same. Out of the PR, order matches master again.

Amortization: also confirmed, and the cost was worse than the lost amortization. sort.IsSorted boxes Targets into sort.Interface, so it allocated even when it skipped the clone: ResolveUpdate with 8 candidates was +54% time and +59% allocs. slices.IsSortedFunc didn't help either, its comparator calls netip.Addr.String() on every pair. Gone, benchstat against master is flat.

The in-place mutation is still real, plan calls Same on endpoints the TXT registry caches so the reordering survives into the next reconciliation. I'd rather open a separate issue for it, it needs its own call on what order providers should get.

[ivankatliarchuk]

not too sure about this tests, what are they for?

[kubernetes-prow]

PR needs rebase.

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.