When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624

Haleygo · 2021-08-27T06:43:51Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Now we can launch kubelet with "--resolv-conf= " and use the default DNS Policy like this.
But if set resolvConf in a config file to "", resolv.conf path was set to "/etc/resolv.conf" and it's not what we wanted.

Which issue(s) this PR fixes:

Fixes ##103110

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED: kubelet: turn the KubeletConfiguration v1beta1 `ResolverConfig` field from a `string` to `*string`.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-08-27T06:43:52Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

aojea · 2021-08-27T08:20:54Z

This fixes the mismatch on the behavior between flags and config file.
Please check with sig-node how they use to handle the changes on the config fields, since you are moving one field form string to pointer, I don't know if you need an api bump or since this is a bug it can be done directly

neolit123

this is a breaking change to KubeletConfiguration users that import the Go struct to set the ResolverConfig field - e.g. the kubeadm change is an example of how they need to adapt.
normally things like this should be done in a version increment.

deferring to SIG Node if they are OK with this.

the change also needs a release note; proposing:

kubelet: turn the KubeletConfiguration v1beta1 `ResolverConfig` field from a `string` to `*string`.

perhaps prefixed with ACTION REQUIRED too.

pacoxu · 2021-08-30T06:08:33Z

/sig node
/cc gjkim42 ehashman
since you have reviewed #103183

k8s-triage-robot · 2021-08-30T08:02:35Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

gjkim42

Agree that setting with config file and setting with command argument should be more consistent.

neolit123 · 2021-08-30T12:20:48Z

/area kubelet

Haleygo · 2021-08-30T13:34:06Z

/retest

pacoxu · 2021-09-04T12:25:22Z

/priority important-longterm
/triage accepted

pacoxu · 2021-09-10T02:11:03Z

/lgtm

@kubernetes/sig-node-api-reviews
as it is a small api-change(no behavior change IMO).

liggitt · 2021-09-11T04:32:44Z

/label api-review
/assign
/assign @ehashman

liggitt · 2021-09-13T18:47:41Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

@@ -601,7 +601,7 @@ type KubeletConfiguration struct {
 	// the node is recommended before changing this field.
 	// Default: "/etc/resolv.conf"


the behavior of providing an explicit empty string needs to be described in the field doc

Sure!
Added description according to this

kubernetes/pkg/kubelet/network/dns/dns.go

Lines 401 to 422 in 9c7426e

case podDNSHost:

// When the kubelet --resolv-conf flag is set to the empty string, use

// DNS settings that override the docker default (which is to use

// /etc/resolv.conf) and effectively disable DNS lookups. According to

// the bind documentation, the behavior of the DNS client library when

// "nameservers" are not specified is to "use the nameserver on the

// local machine". A nameserver setting of localhost is equivalent to

// this documented behavior.

if c.ResolverConfig == "" {

for _, nodeIP := range c.nodeIPs {

if utilnet.IsIPv6(nodeIP) {

dnsConfig.Servers = append(dnsConfig.Servers, "::1")

} else {

dnsConfig.Servers = append(dnsConfig.Servers, "127.0.0.1")

}

}

if len(dnsConfig.Servers) == 0 {

dnsConfig.Servers = append(dnsConfig.Servers, "127.0.0.1")

}

dnsConfig.Searches = []string{"."}

}

}

ehashman

Is there anything that should be added to the kubelet config validation tests for this change? (note that there doesn't appear to be a defaults_test.go defined and validation in https://github.com/kubernetes/kubernetes/blob/release-1.22/pkg/kubelet/apis/config/validation/validation_test.go is not very targeted)

Generally LGTM, fixes a drift in behaviour between CLI flag and the kubeletConfig.

ehashman · 2021-09-13T18:59:16Z

pkg/kubelet/apis/config/v1beta1/defaults.go

-	if obj.ResolverConfig == "" {
-		obj.ResolverConfig = kubetypes.ResolvConfDefault
+	if obj.ResolverConfig == nil {
+		obj.ResolverConfig = utilpointer.String(kubetypes.ResolvConfDefault)


Note internal type is remaining string and we will always default here if unset so the addition of a pointer should not cause NPEs.

ehashman · 2021-09-13T19:38:26Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

@@ -601,7 +601,7 @@ type KubeletConfiguration struct {
 	// the node is recommended before changing this field.
 	// Default: "/etc/resolv.conf"
 	// +optional
-	ResolverConfig string `json:"resolvConf,omitempty"`
+	ResolverConfig *string `json:"resolvConf,omitempty"`


This was omitempty before so old clients should behave the right way.

liggitt · 2021-09-14T20:10:58Z

/lgtm
/approve

k8s-ci-robot · 2021-09-14T20:11:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Haleygo, liggitt

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:

~~cmd/kubeadm/OWNERS~~ [liggitt]
~~pkg/kubelet/apis/config/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kubelet/config/OWNERS~~ [liggitt]

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 requested review from dchen1107 and pacoxu August 27, 2021 06:44

neolit123 reviewed Aug 29, 2021

View reviewed changes

k8s-ci-robot requested review from ehashman and gjkim42 August 30, 2021 06:08

Haleygo marked this pull request as ready for review August 30, 2021 06:08

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2021

gjkim42 reviewed Aug 30, 2021

View reviewed changes

k8s-ci-robot added the area/kubelet label Aug 30, 2021

pacoxu moved this from Triage to Needs Reviewer in SIG Node PR Triage Sep 4, 2021

k8s-ci-robot assigned pacoxu Sep 10, 2021

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

k8s-ci-robot assigned ehashman and liggitt Sep 11, 2021

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Sep 11, 2021

liggitt added this to Assigned in API Reviews Sep 11, 2021

liggitt reviewed Sep 13, 2021

View reviewed changes

ehashman reviewed Sep 13, 2021

View reviewed changes

support null resolvConf in Kubelet Configuration

46454ea

Haleygo force-pushed the support-null-resolvConf-in-configFile branch from aef8b8b to 46454ea Compare September 14, 2021 08:13

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2021

liggitt moved this from Assigned to API review completed, 1.23 in API Reviews Sep 14, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021

k8s-ci-robot merged commit fa2657b into kubernetes:master Sep 14, 2021

SIG Node PR Triage automation moved this from Needs Reviewer to Done Sep 14, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 14, 2021

gjkim42 mentioned this pull request Nov 11, 2021

Failed to generate pod sandbox config if missing /etc/resolv.conf #103110

Closed

aojea mentioned this pull request Mar 21, 2022

kube-proxy: We should be able to override bind addresses for healthz and metrics #108737

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624

When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624

Haleygo commented Aug 27, 2021 •

edited

k8s-ci-robot commented Aug 27, 2021

aojea commented Aug 27, 2021

neolit123 left a comment •

edited

pacoxu commented Aug 30, 2021

k8s-triage-robot commented Aug 30, 2021

gjkim42 left a comment

neolit123 commented Aug 30, 2021

Haleygo commented Aug 30, 2021

pacoxu commented Sep 4, 2021

pacoxu commented Sep 10, 2021 •

edited

liggitt commented Sep 11, 2021

liggitt Sep 13, 2021

Haleygo Sep 14, 2021

ehashman left a comment

ehashman Sep 13, 2021

ehashman Sep 13, 2021

liggitt commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

		@@ -601,7 +601,7 @@ type KubeletConfiguration struct {
		// the node is recommended before changing this field.
		// Default: "/etc/resolv.conf"

	case podDNSHost:
	// When the kubelet --resolv-conf flag is set to the empty string, use
	// DNS settings that override the docker default (which is to use
	// /etc/resolv.conf) and effectively disable DNS lookups. According to
	// the bind documentation, the behavior of the DNS client library when
	// "nameservers" are not specified is to "use the nameserver on the
	// local machine". A nameserver setting of localhost is equivalent to
	// this documented behavior.
	if c.ResolverConfig == "" {
	for _, nodeIP := range c.nodeIPs {
	if utilnet.IsIPv6(nodeIP) {
	dnsConfig.Servers = append(dnsConfig.Servers, "::1")
	} else {
	dnsConfig.Servers = append(dnsConfig.Servers, "127.0.0.1")
	}
	}
	if len(dnsConfig.Servers) == 0 {
	dnsConfig.Servers = append(dnsConfig.Servers, "127.0.0.1")
	}
	dnsConfig.Searches = []string{"."}
	}
	}

When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624

When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624

Conversation

Haleygo commented Aug 27, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 27, 2021

aojea commented Aug 27, 2021

neolit123 left a comment • edited

Choose a reason for hiding this comment

pacoxu commented Aug 30, 2021

k8s-triage-robot commented Aug 30, 2021

gjkim42 left a comment

Choose a reason for hiding this comment

neolit123 commented Aug 30, 2021

Haleygo commented Aug 30, 2021

pacoxu commented Sep 4, 2021

pacoxu commented Sep 10, 2021 • edited

liggitt commented Sep 11, 2021

liggitt Sep 13, 2021

Choose a reason for hiding this comment

Haleygo Sep 14, 2021

Choose a reason for hiding this comment

ehashman left a comment

Choose a reason for hiding this comment

ehashman Sep 13, 2021

Choose a reason for hiding this comment

ehashman Sep 13, 2021

Choose a reason for hiding this comment

liggitt commented Sep 14, 2021

k8s-ci-robot commented Sep 14, 2021

Haleygo commented Aug 27, 2021 •

edited

neolit123 left a comment •

edited

pacoxu commented Sep 10, 2021 •

edited