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

DNS Controller Optional #3822

Merged

Conversation

gambol99
Copy link
Contributor

@gambol99 gambol99 commented Nov 10, 2017

The current implementation enforces a dns-controller is running; given the user can switch the make the kube-apiserver server Internal and then reuse the dns for the masterInternalName; this effectlively removes the need to run the service (assuming your not using it for pods, node and service dns)

  • adding a disableDnsController to the ExternalDNS spec provides a toggle on the addon (name is definitely up for debate)
  • the default behaviour remains, the dns-controller is always pushed as an addon

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 10, 2017
@justinsb
Copy link
Member

So I agree, and I'm very happy that we're gradually breaking kops into its component parts as we build up our capabilities. As you say - kops no longer requires a DNS controller, we could just make it a addon, and we already have alternative implementations for it.

A lot of it was necessarily tightly bound in the early days, but I would love to see e.g. etcd-management, addon-management, infrastruture/machine-management, node-management and version-management that kops does today all separable, with the current kops implementations as just one choice of implementation. (cc @bgrant0607 and @roberthbailey for the direction)

Quibbles about naming (because it's the API):

  • We're going to end up with externalDNS: disableDNSController: true. How about externalDNS: disable: true.

  • Maybe we should do controller: none so that we can plug in the external-dns project here as well with controller: external-dns?

  • Alternatively we could just allow a list of add-ins (whatever they are). This is made more complicated because we configure some DNS zone mappings on the controller, and we also configure IAM policies for that DNS zone. So it can't be totally opaque to kops. We have similar issues with networking, and there we've solved it by having a typed configuration object for each provider.

I think given the third concerns, the KISS option here of externalDNS: disable: true (or externalDNS: enable: false?) is probably best. When we support external-dns, we can have a typed configuration object like we do with networking. What do you think @gambol99 ?

@justinsb justinsb self-assigned this Nov 10, 2017
@bgrant0607
Copy link
Member

This is for a DNS name for the master, apps in the cluster, or both?

The current implementation requires enforces a dns-controller is running; given the user can switch the make the kube-apiserver server Internal and then reuse the dns for the masterInternalName; this effectlively removes the need to run the service (assuming your not using it for pods, node and service dns)

- adding a disableDnsController to the ExternalDNS spec provides a toggle on the addon (name is definitely up for debate)
- the default behaviour remains, the dns-controller is always pushed as an addon
@gambol99
Copy link
Contributor Author

hi @justinsb ... sorry for the delay on this one ... Yes, I agree on the disable flag, the controller: none is interesting, i kinda like the idea of moving all the addons to a central API location rather than having them scattered though... I know @chrislovecnm was speaking about looking at the addon-manager ..

  • updated the flag to 'disable'
  • the dns-controller is as before i.e. on by default and only disabled explicitly via the flag

@justinsb justinsb added this to the 1.8.0 milestone Nov 22, 2017
@justinsb
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your 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 Nov 26, 2017
@chrislovecnm
Copy link
Contributor

@bgrant0607 this controller handles dns for many different components. It’s primary use case is creating a dns record for the api server. kops can utilize route53, google dns or coredns to register dns records. It is using the code in k/k from federation, if I remember. It will also handle cloud dns for nodes, services, and ingress https://github.com/kubernetes/kops/blob/master/dns-controller/README.md

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 0ade1dd into kubernetes:master Nov 26, 2017
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants