Skip to content

Conversation

@vflaux
Copy link
Contributor

@vflaux vflaux commented Jun 3, 2025

What does it do ?

Use EndpointSlices instead of Endpoints for Service source.

Allow to use EndpointSlices instead on Endpoints for Services source.

This add a new interface for getting the endpoints from Services with two implementations: Endpoints & EndpointSlices.

A new option --[no-]use-endpointslices allow to switch between the two implementations.
Endpoints implementation stays the default.

Motivation

#5467: Endpoints are deprecated in 1.33: https://kubernetes.io/blog/2025/04/24/endpoints-deprecation/

More

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. chart labels Jun 3, 2025
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 3, 2025
@mloiseleur
Copy link
Collaborator

@vflaux As of Kubernetes 1.21, the EndpointSlices API is a stable alternative. Users of obsolete versions of Kubernetes can use obsolete versions of external dns.

IIRC, On Traefik Proxy, they have made the switch in v3.1.0, without any known issues since then.

=> Wdyt about just making the switch, without any flag ? We'll add a notice for RBAC change in release notes.

@mloiseleur mloiseleur changed the title Allow to use EndpointSlice or Endpoints for Service source feat: allow to use EndpointSlice or Endpoints for Service source Jun 3, 2025
@mloiseleur mloiseleur changed the title feat: allow to use EndpointSlice or Endpoints for Service source feat(source): allow to use EndpointSlice or Endpoints for Service Jun 3, 2025
@vflaux
Copy link
Contributor Author

vflaux commented Jun 3, 2025

@mloiseleur I followed @ivankatliarchuk’s comment on this issue: #5467 (comment), but simply switching to EndpointSlices would be less complex.
Both options are good for me 😃

@ivankatliarchuk
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 Jun 3, 2025
@ivankatliarchuk
Copy link
Member

I thought is going to me more complicated to do the switch. Nice one

@ivankatliarchuk
Copy link
Member

If we think that support for Endpoint and EndpointSlices is not reqiured, fine with me.

@mloiseleur
Copy link
Collaborator

mloiseleur commented Jun 3, 2025

For external dns feature, we should do deprecation.
On this, it has been deprecated on k8s side since multiple years.
There is no interest in keeping Endpoint support. It's an internal mechanism between external dns and k8s. There is no usage impact. It only requires to change the rbac.
So yes, let's do it in a simpler and more straightforward way.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 3, 2025
@ivankatliarchuk
Copy link
Member

for helm chart, any chance you could add a helm unit test? This is what we are using https://github.com/helm-unittest/helm-unittest

@vflaux vflaux changed the title feat(source): allow to use EndpointSlice or Endpoints for Service feat(source): use EndpointSlice instead of Endpoints for Service Jun 6, 2025
@vflaux vflaux changed the title feat(source): use EndpointSlice instead of Endpoints for Service feat(source): use EndpointSlices instead of Endpoints for Service Jun 6, 2025
@vflaux vflaux marked this pull request as ready for review June 6, 2025 13:09
@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 Jun 6, 2025
@k8s-ci-robot k8s-ci-robot requested a review from szuecs June 6, 2025 13:09
@mloiseleur
Copy link
Collaborator

mloiseleur commented Jun 15, 2025

This PR LGTM.
@vflaux for the chart, you need to update charts/external-dns/CHANGELOG.md under UNRELEASED section. See for instance this PR for an example.

Copy link
Member

@ivankatliarchuk ivankatliarchuk left a comment

Choose a reason for hiding this comment

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

Do you think you could address error cases in unit tests? Rest is lgtm as well

Screenshot 2025-06-16 at 07 25 32 Screenshot 2025-06-16 at 07 26 05 Screenshot 2025-06-16 at 07 26 23

@vflaux
Copy link
Contributor Author

vflaux commented Jun 19, 2025

@ivankatliarchuk some of them are unreachable code at runtime; I’ve added comments for these cases.
I also added a test for the scenario where a non-EndpointSlice is added to the index. In this case, the index function returns an error, and the add function then panics.

Regarding the publishHostIP condition, I see no test with it enabled.
Maybe it’s preferable to address this in a separate PR and validate the current behavior with Endpoints before merging this one?

@ivankatliarchuk
Copy link
Member

ivankatliarchuk commented Jun 19, 2025

Yeah, let's add tests in separate pr to cover missing cases

Copy link
Member

@ivankatliarchuk ivankatliarchuk left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2025
@k8s-ci-robot
Copy link
Contributor

[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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2025
@k8s-ci-robot k8s-ci-robot merged commit ef6e0e5 into kubernetes-sigs:master Jun 19, 2025
15 checks passed
@ivankatliarchuk
Copy link
Member

This PR LGTM. @vflaux for the chart, you need to update charts/external-dns/CHANGELOG.md under UNRELEASED section. See for instance this PR for an example.

@vflaux Looks like I missed this comment. Address it when you have time pls

@stevo-f3
Copy link

stevo-f3 commented Aug 6, 2025

Would it make sense to update https://github.com/kubernetes-sigs/external-dns?tab=readme-ov-file#kubernetes-version-compatibility for k8s >= 1.33 to use external-dns >= v0.18.0 ?

@mloiseleur
Copy link
Collaborator

@stevo-f3 Yes, clearly. Do you think you can open the PR to update it ?

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. chart 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

7 participants