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.

Sign up for GitHub

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

Jump to bottom

Use ExternalDNS as drop-in for dns-controller #11859

Merged
merged 5 commits into from
Aug 27, 2021
Merged

Use ExternalDNS as drop-in for dns-controller #11859

merged 5 commits into from
Aug 27, 2021

Conversation

olemarkus
Copy link
Member

All the changes needed should now be in externalDNS. The remaining work is mainly being able to define DNS provider in the cluster spec.

This will probably never work for gossip clusters though.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2021
@k8s-ci-robot k8s-ci-robot added area/addons area/provider/aws Issues or PRs related to aws provider labels Jun 24, 2021
@olemarkus olemarkus changed the title Use ExternalDNS as drop-in for dns-controller WIP: Use ExternalDNS as drop-in for dns-controller Jun 24, 2021
@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. area/api size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 24, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2021
@johngmyers
Copy link
Member

johngmyers commented Jun 26, 2021 •
edited
Loading

The external-dns docs/faq.md should be updated to fix "Compatibility mode with Kops DNS Controller is planned to be added in the future."

Currently dns-controller has to watch all the pods in the cluster in order to find the annotations. It's unfortunate we're making external-dns do the same. Could we migrate to using labels instead?

I think we should deprecate dns-controller's functionality for services and ingresses, in favor of having people use external-dns's API directly.

@olemarkus
Copy link
Member Author

I think it would be good to have a discussion on how we can use external-dns without compatibility mode, and what the future role of dns-controller may be (e.g it could become a gossip-only thing).

But alone, this PR will make it easier for us to support other DNS providers, and for other cloud providers to not have to implement DNS provider logic in dns-controller.

@johngmyers
Copy link
Member

kOps relies on DNS to NodePort pods, which external-dns doesn't support without compatibility mode. I think it would be better for external-dns to implement a native mechanism for NodePort pods using labels than for it to implement a compatibility mode for the dns-controller API.

@olemarkus
Copy link
Member Author

olemarkus commented Jun 26, 2021 •
edited
Loading

I am not sure replacing annotations with labels is the right thing to do. But one could add a label selector to the pod source to limit what it watches. That way we could feed it a common label we set on the CP pods (by default) for example.

@olemarkus olemarkus force-pushed the external-dns-provider branch 3 times, most recently from 36fa560 to cb6bd17 Compare June 26, 2021 13:31
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2021
@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 17, 2021
@olemarkus
Copy link
Member Author

/retest

4 similar comments
@olemarkus
Copy link
Member Author

/retest

@olemarkus
Copy link
Member Author

/retest

@olemarkus
Copy link
Member Author

/retest

@olemarkus
Copy link
Member Author

/retest

@rifelpet
Copy link
Member

/test pull-kops-e2e-cni-amazonvpc

@olemarkus olemarkus added this to the v1.22 milestone Aug 22, 2021
@olemarkus
Copy link
Member Author

/cc @rifelpet

rifelpet
rifelpet requested changes Aug 26, 2021
View reviewed changes
pkg/apis/kops/validation/validation.go Outdated Show resolved Hide resolved
pkg/model/components/addonmanifests/remap.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/dns.go Show resolved Hide resolved
@olemarkus @rifelpet
Apply suggestions from code review
b3982e1 
Co-authored-by: Peter Rifel <rifelpet@users.noreply.github.com>
@olemarkus
Copy link
Member Author

/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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 Aug 27, 2021
@rifelpet
Copy link
Member

Can you add a prow job for this? It should be pretty simple

@k8s-ci-robot k8s-ci-robot merged commit ea89f70 into kubernetes:master Aug 27, 2021
@olemarkus
Copy link
Member Author

That's absolutely on my agenda 😄

@olemarkus olemarkus deleted the external-dns-provider branch August 27, 2021 12:31
@rifelpet rifelpet mentioned this pull request Sep 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rifelpet rifelpet rifelpet approved these changes

@KashifSaadat KashifSaadat Awaiting requested review from KashifSaadat

@rdrgmnzs rdrgmnzs Awaiting requested review from rdrgmnzs

Assignees

@rifelpet rifelpet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api area/provider/aws Issues or PRs related to aws provider 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@olemarkus @johngmyers @rifelpet @k8s-ci-robot