Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Headless service: retrieve endpoints via Endpoints resource; evaluate spec.publishNotReadyAddresses #1005

Conversation

alfredkrohmer
Copy link
Contributor

@alfredkrohmer alfredkrohmer commented Apr 28, 2019

Currently, the endpoints of headless services are retrieved
by querying pods using the pod selector of the service.
Instead, we now query for an Endpoints resource with the
same name as the Service object to get the endpoints for the
service. This is needed in order to support the
spec.publishNotReadyPods attribute of a service.

  • needs documentation and manifest/helm chart update to include permissions to retrieve endpoints resources

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 28, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 28, 2019
@alfredkrohmer
Copy link
Contributor Author

/assign @Raffo

@alfredkrohmer
Copy link
Contributor Author

Note: this will change the default behaviour. Before, the IPs of all Running pods of a headless services were published; now this is only the case when the .spec.publishNotReadyAddresses is set to true.

@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 2b14f6f to 94fa80f Compare April 28, 2019 16:33
@alfredkrohmer
Copy link
Contributor Author

@Raffo @njuettner any chance to get this reviewed?

Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

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

@devkid I added one minor comment and I have two requests:

  1. Add a comment to the function extractHeadlessEndpoints to specify what it does. I know there was no comment before but this was IMO bad and we now have a chance to fix it, especially with the changed behaviour.
  2. Write a little sentence that we can add to future release notes on how the behaviour has changed.

Additionally, I'm thinking wether we should add a flag to enable/disable the changed behaviour. I think we could add it and keep it around for a few releases before getting rid of it. WDYT? /cc @linki @njuettner

source/service.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 23, 2019
@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 623b15c to 9e43d65 Compare August 23, 2019 11:13
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2019
@alfredkrohmer
Copy link
Contributor Author

@Raffo added an option to restore the old behavior via CLI flag and added a comment for extractHeadlessEndpoints.

@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 9e43d65 to 6546c9e Compare September 17, 2019 07:23
@alfredkrohmer
Copy link
Contributor Author

Failing test is fixed now.

Description for release:

Breaking change: Endpoints (pod IPs) for the service source are now retrieved using the Endpoints kubernetes resource instead of using the selector of the service to query for pods and their IPs. The implication is that unready pods will not be picked up anymore by external-dns by default. In order to restore the old behavior and also register IPs of unready pods with DNS:

  • for a single service, set .spec.publishNotReadyAddresses to true
  • to restore the old behavior globally, start external-dns with --always-publish-not-ready-addresses

@judofyr
Copy link

judofyr commented Oct 30, 2019

Any update on this?

@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 1535588 to 36f7984 Compare November 24, 2019 08:17
@alfredkrohmer
Copy link
Contributor Author

Conflict resolved, documentation with inline RBAC permissions updated. I will create a pull request in https://github.com/helm/charts to up the version and add the endpoints resources permissions when there is a new release.

@vivekgarg20
Copy link

When is this targeted?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2019
@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 23dfd7b to 8882900 Compare December 13, 2019 16:18
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 13, 2019
@alfredkrohmer alfredkrohmer changed the title [RFC] Headless service: retrieve endpoints via Endpoints resource; evaluate spec.publishNotReadyAddresses Headless service: retrieve endpoints via Endpoints resource; evaluate spec.publishNotReadyAddresses Dec 23, 2019
@ashley
Copy link

ashley commented Jan 17, 2020

Any update on this?

Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

added some comments, PTAL.

source/service.go Show resolved Hide resolved
source/service_test.go Outdated Show resolved Hide resolved
docs/tutorials/alibabacloud.md Outdated Show resolved Hide resolved
alfredkrohmer and others added 4 commits February 18, 2020 19:41
… spec.publishNotReadyAddresses

Currently, the endpoints of headless services are retrieved
by querying pods using the pod selector of the service.
Instead, we now query for an Endpoints resource with the
same name as the Service object to get the endpoints for the
service. This is needed in order to support the
spec.publishNotReadyPods attribute of a service.
@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 0860c23 to 85bf10a Compare February 18, 2020 18:59
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 18, 2020
@alfredkrohmer alfredkrohmer force-pushed the feature/headless-services-publishnotreadyaddresses branch from 85bf10a to 65208db Compare February 18, 2020 19:19
Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

/lgtm
/cc @Raffo @linki

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020
@njuettner
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devkid, njuettner

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 Mar 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit a563022 into kubernetes-sigs:master Mar 4, 2020
julian3xl added a commit to julian3xl/charts that referenced this pull request Mar 11, 2020
as described in kubernetes-sigs/external-dns#1005 now external-dns uses endpoints resource so it needs permissions to do it
julian3xl added a commit to julian3xl/charts that referenced this pull request Mar 11, 2020
as described in kubernetes-sigs/external-dns#1005 now external-dns uses endpoints resource so it needs permissions to do it

Signed-off-by: Julián Martínez <julian.martinez@piksel.com>
julian3xl pushed a commit to julian3xl/bitnami-charts that referenced this pull request Mar 11, 2020
as described in kubernetes-sigs/external-dns#1005 now external-dns uses endpoints resource so it needs permissions to do it

Version bumped to 2.20.4

Signed-off-by: Julián Martínez <julian.martinez@piksel.com>
miguelaeh pushed a commit to bitnami/charts that referenced this pull request Mar 12, 2020
as described in kubernetes-sigs/external-dns#1005 now external-dns uses endpoints resource so it needs permissions to do it

Version bumped to 2.20.4

Signed-off-by: Julián Martínez <julian.martinez@piksel.com>
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants