NodeLocal DNSCache #8780

mazzy89 · 2020-03-21T20:30:05Z

Signed-off-by: Salvatore Mazzarino dev@mazzarino.cz

From k8s 1.18 NodeLocal DNSCache is GA.

This PR aims to implement NodeLocal DNSCache as an addon and enable it only when CoreDNS is enabled too.

k8s-ci-robot · 2020-03-21T20:30:12Z

Hi @mazzy89. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

pkg/model/components/kubedns.go

pkg/model/components/kubelet.go

rifelpet · 2020-03-22T14:47:49Z

/ok-to-test

My only thought on the new api fields is that it might be more intuitive to have an Enabled field rather than a Disabled field

upup/models/cloudup/resources/addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml.template

mazzy89 · 2020-03-22T15:19:19Z

@rifelpet going to change the condition to Enabled because indeed more intutive.

mazzy89 · 2020-03-23T09:29:39Z

/retest

pkg/model/components/kubedns.go

pkg/model/components/kubelet.go

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

pkg/apis/kops/validation/legacy.go

docs/cluster_spec.md

pkg/apis/kops/validation/legacy.go

upup/models/cloudup/resources/addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml.template

olemarkus · 2020-03-27T12:17:44Z

The nodes start now, but ends up in this error:
plugin/forward: /etc/Corefile:10 - Error during parsing: Wrong argument count or unexpected line ending after '.'

Is there any way of testing if this works? Like prometheus counters or something?

mazzy89 · 2020-03-27T12:21:14Z

I guess you are using it in iptables mode. in that case then ClusterIP is an empty string and the template then is probably parsed with an empty space which cause the parsing failing.

I guess then we need to check if the ClusterIP is set.

mazzy89 · 2020-03-27T12:23:29Z

Let me wire here a change and see if it works.

olemarkus · 2020-03-27T12:48:08Z

Now I get

error building tasks: error reading manifest addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml: error opening resource: error executing resource template "addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml": error parsing template "addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml": template: addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml:75: unexpected {{else}}

Could you maybe try to deploy a cluster with this before submitting the next commits?

mazzy89 · 2020-03-27T12:55:31Z

Now I get


error building tasks: error reading manifest addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml: error opening resource: error executing resource template "addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml": error parsing template "addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml": template: addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml:75: unexpected {{else}}

Could you maybe try to deploy a cluster with this before submitting the next commits?

Unfortunately dont have anywhere where to test this change quickly

pkg/apis/kops/validation/legacy.go

mazzy89 · 2020-03-30T15:36:30Z

Added unit tests here. It should now be easier to tests and make sure that the different scenarios are now covered.

upup/pkg/fi/cloudup/template_functions.go

mazzy89 · 2020-04-02T16:45:43Z

Ready here to be picked up for a review.

Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

olemarkus · 2020-04-10T10:27:40Z

/test pull-kops-e2e-kubernetes-aws

123BLiN · 2020-04-10T11:01:31Z

Sorry to bother, but will it be possible to use this with Kops 1.15/1.16 and kubernetes v1.15 or this change is only for furutre 1.18 release?

mazzy89 · 2020-04-10T11:14:28Z

At the moment the change has been planned for kops 1.18

justinsb · 2020-04-10T15:37:52Z

pkg/apis/kops/validation/legacy.go

+
+			// @ check that NodeLocalDNS addon is configured correctly
+			if c.Spec.KubeDNS.NodeLocalDNS != nil && c.Spec.KubeDNS.NodeLocalDNS.Enabled {
+				if c.Spec.KubeDNS.Provider != "CoreDNS" {


Do we actually need the "raw" DNS provider to be CoreDNS? I didn't think it was required?

justinsb · 2020-04-10T15:38:42Z

pkg/apis/kops/validation/legacy.go

@@ -705,3 +714,9 @@ func validateKubelet(k *kops.KubeletConfigSpec, c *kops.Cluster, kubeletPath *fi
 	}
 	return allErrs
 }
+
+func isExperimentalClusterDNS(k *kops.KubeletConfigSpec, dns *kops.KubeDNSConfig) bool {


Nit: I'm not clear why this function is called "isExperimentalClusterDNS". Maybe isValidClusterDNS ?

It is just because when this one returns true, one needs the ExperimentalClusterDNS featureflag on. I am not really sure this feature flag makes sense. As in it doesn't really guard a feature.

One option would certainly be to remove the feature flag and only consider "valid" configurations.

justinsb · 2020-04-10T15:40:55Z

pkg/apis/kops/cluster.go

+// NodeLocalDNSConfig are options of the node-local-dns
+type NodeLocalDNSConfig struct {
+	// Disable indicates we do not wish to run the node-local-dns addon
+	Enabled bool `json:"enabled,omitempty"`


I think we actually should make this Enabled *bool, because we want to differentiate between:

nodeLocalDNSConfig: enabled: false localIP: 10.0.0.1

and

nodeLocalDNSConfig: localIP: 10.0.0.1

I suppose this could be a change we do if we consider enabling this by default. If not, we don't really need to distinguish between the two now.

justinsb · 2020-04-10T15:41:43Z

Thanks @mazzy89

A few small nits, and the enabled one is actually pretty important I think. But we can also do them in follow on PRs, so...

/approve
/lgtm

k8s-ci-robot · 2020-04-10T15:42:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, mazzy89

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:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

As discussed in kubernetes#8780 so we differentiate between false and not-set. Also tweak the comment.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 21, 2020

k8s-ci-robot requested review from joshbranham and robinpercy March 21, 2020 20:30

k8s-ci-robot added the area/api label Mar 21, 2020

mazzy89 commented Mar 21, 2020

View reviewed changes

pkg/model/components/kubedns.go Show resolved Hide resolved

mazzy89 mentioned this pull request Mar 21, 2020

NodeLocal DNSCache #7459

Closed

olemarkus reviewed Mar 22, 2020

View reviewed changes

pkg/model/components/kubedns.go Show resolved Hide resolved

pkg/model/components/kubelet.go Outdated Show resolved Hide resolved

rifelpet reviewed Mar 22, 2020

View reviewed changes

pkg/model/components/kubelet.go Outdated Show resolved Hide resolved

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 Mar 22, 2020

rifelpet reviewed Mar 22, 2020

View reviewed changes

upup/models/cloudup/resources/addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml.template Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/documentation and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2020

olemarkus reviewed Mar 23, 2020

View reviewed changes

pkg/model/components/kubedns.go Show resolved Hide resolved

pkg/model/components/kubedns.go Outdated Show resolved Hide resolved

pkg/model/components/kubelet.go Outdated Show resolved Hide resolved

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated Show resolved Hide resolved

olemarkus reviewed Mar 27, 2020

View reviewed changes

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go Outdated Show resolved Hide resolved

mazzy89 commented Mar 27, 2020

View reviewed changes

pkg/apis/kops/validation/legacy.go Show resolved Hide resolved

olemarkus reviewed Mar 27, 2020

View reviewed changes

docs/cluster_spec.md Show resolved Hide resolved

pkg/apis/kops/validation/legacy.go Show resolved Hide resolved

olemarkus reviewed Mar 27, 2020

View reviewed changes

upup/models/cloudup/resources/addons/nodelocaldns.addons.k8s.io/k8s-1.12.yaml.template Outdated Show resolved Hide resolved

mazzy89 force-pushed the nodelocaldnscache branch from 37f80a7 to 9fa2ba6 Compare March 30, 2020 13:23

mazzy89 commented Mar 30, 2020

View reviewed changes

pkg/apis/kops/validation/legacy.go Outdated Show resolved Hide resolved

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 Mar 30, 2020

mazzy89 commented Mar 31, 2020

View reviewed changes

upup/pkg/fi/cloudup/template_functions.go Show resolved Hide resolved

olemarkus force-pushed the nodelocaldnscache branch from 58fa846 to 36f8ab5 Compare April 1, 2020 08:10

olemarkus force-pushed the nodelocaldnscache branch from 36f8ab5 to dfb725c Compare April 6, 2020 15:15

mazzy89 and others added 4 commits April 10, 2020 10:44

NodeLocal DNSCache

f754cbd

Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

Improved validation logic

cf57350

Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

Add unit tests

d18c88a

Signed-off-by: Salvatore Mazzarino <dev@mazzarino.cz>

Simplify the spec and templates a bit

d5019a6

olemarkus force-pushed the nodelocaldnscache branch from dfb725c to d5019a6 Compare April 10, 2020 08:49

Verify bindata

f5a5db4

justinsb reviewed Apr 10, 2020

View reviewed changes

k8s-ci-robot assigned justinsb Apr 10, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2020

k8s-ci-robot merged commit d0dea75 into kubernetes:master Apr 10, 2020

k8s-ci-robot added this to the v1.18 milestone Apr 10, 2020

justinsb added a commit to justinsb/kops that referenced this pull request Apr 17, 2020

Change NodeLocalDNS Enabled to *bool

f3a40cf

As discussed in kubernetes#8780 so we differentiate between false and not-set. Also tweak the comment.

justinsb mentioned this pull request Apr 17, 2020

Change NodeLocalDNS Enabled to *bool #8930

Merged

fred-vogt mentioned this pull request Jun 2, 2020

node-local-dns crash looping on masters '169.254.20.10:8080: bind: address already in use' #9245

Closed

stone-z mentioned this pull request Dec 4, 2020

private-distributors-list: add Giant Swarm kubernetes/committee-security-response#118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NodeLocal DNSCache #8780

NodeLocal DNSCache #8780

mazzy89 commented Mar 21, 2020

k8s-ci-robot commented Mar 21, 2020

rifelpet commented Mar 22, 2020

mazzy89 commented Mar 22, 2020

mazzy89 commented Mar 23, 2020

olemarkus commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

olemarkus commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

mazzy89 commented Mar 30, 2020

mazzy89 commented Apr 2, 2020

olemarkus commented Apr 10, 2020

123BLiN commented Apr 10, 2020

mazzy89 commented Apr 10, 2020

justinsb Apr 10, 2020

justinsb Apr 10, 2020

olemarkus Apr 10, 2020

justinsb Apr 10, 2020

olemarkus Apr 10, 2020

justinsb commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020

NodeLocal DNSCache #8780

NodeLocal DNSCache #8780

Conversation

mazzy89 commented Mar 21, 2020

k8s-ci-robot commented Mar 21, 2020

rifelpet commented Mar 22, 2020

mazzy89 commented Mar 22, 2020

mazzy89 commented Mar 23, 2020

olemarkus commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

olemarkus commented Mar 27, 2020

mazzy89 commented Mar 27, 2020

mazzy89 commented Mar 30, 2020

mazzy89 commented Apr 2, 2020

olemarkus commented Apr 10, 2020

123BLiN commented Apr 10, 2020

mazzy89 commented Apr 10, 2020

justinsb Apr 10, 2020

Choose a reason for hiding this comment

justinsb Apr 10, 2020

Choose a reason for hiding this comment

olemarkus Apr 10, 2020

Choose a reason for hiding this comment

justinsb Apr 10, 2020

Choose a reason for hiding this comment

olemarkus Apr 10, 2020

Choose a reason for hiding this comment

justinsb commented Apr 10, 2020

k8s-ci-robot commented Apr 10, 2020