-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow for some coredns customization #5839
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Globegitter If they are not already assigned, you can assign the PR to them by writing 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 |
/ok-to-test |
Or as another idea, could we possibly just have |
@@ -62,10 +62,13 @@ data: | |||
upstream | |||
fallthrough in-addr.arpa ip6.arpa | |||
} | |||
{{ if KubeDNS.Corefile.EnableAutopath -}} | |||
autpath @kubernetes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a typo? autpath
-> autopath
?
@@ -84,7 +87,8 @@ spec: | |||
strategy: | |||
type: RollingUpdate | |||
rollingUpdate: | |||
maxUnavailable: 1 | |||
maxSurge: 10% | |||
maxUnavailable: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Globegitter I like your idea of being able to replace the entire Corefile, so any customization is possible. I'm currently interested in disabling DNS resolution logs. In my understanding the new spec key in the cluster spec yaml should replace the entire content of the Corefile Configmap and the use would be like this:
The current implementation have keys to autopath (specific feature) and version (this is nice and I've incorporated in the example above, a bit differently). Could you provide some examples on how you think the cluster spec should be? What are your thoughts on the approach described above? Thank you for contributing this! |
@rochacon Sorry I have been away on holiday and not sure I left it in a completely working state beforehand. I'll get back to it in the coming day. But yes your example is how it should be working. |
@Globegitter: PR needs rebase. 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/test-infra repository. |
Looks like a rebase is needed. The other challenge is that some of these are CoreDNS specific, but we're in the generic kubedns configuration file. It's not terrible, but it makes me a little uneasy. So I'm hoping that we're going to have a better way to manage addons in general in kops 1.12 / 1.13 - based around channels and kustomize, so we might be able to get this functionality without having to plumb everything through the API :-) Moving to the next milestone anyway, as we're in the final burndown for 1.11 /milestone 1.12 |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Thanks again for this @Globegitter We discussed in office-hours today the need to prioritize bug fixes until we can get 1.12 out (which will hopefully mean we get to everything faster overall), and I believe this is a feature, so I'm going to move this to the next release. /kind feature |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle rotten |
FYI the current CoreDNS version (1.3.1) used by Kops has a bug causing the CoreDNS failure when the kubernetes api is down. The option to change the CoreDNS version would be really helpful. There's a workaround - mount an EmptyDir to /tmp. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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/test-infra repository. I understand the commands that are listed here. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Sorry, for not responding, I will not have time for this anymore. |
I wanted to be able to enable autopath config and thought I would also try and fix #5773 along the way and add a few other minor changes. Imo the update strategy of kube-dns made more sense to have maxUnavailable set to 0. What do you think?
I also haven't tested the cache setting, but I think by having it a string, the full control should be possible including the extended config
by just using a yaml multi-line string.
This is the first PR of this nature that I have done btw, so hope modified everything at the right place.