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

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

Merged

Conversation

@Haleygo
Copy link
Contributor

@Haleygo Haleygo commented Aug 27, 2021

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
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Aug 27, 2021

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

Loading

@aojea
Copy link
Member

@aojea aojea commented Aug 27, 2021

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

Loading

Copy link
Member

@neolit123 neolit123 left a comment

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.

Loading

@pacoxu
Copy link
Member

@pacoxu pacoxu commented Aug 30, 2021

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

Loading

@k8s-triage-robot
Copy link

@k8s-triage-robot k8s-triage-robot commented Aug 30, 2021

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.

Loading

Copy link
Member

@gjkim42 gjkim42 left a comment

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

Loading

@neolit123
Copy link
Member

@neolit123 neolit123 commented Aug 30, 2021

/area kubelet

Loading

@Haleygo
Copy link
Contributor Author

@Haleygo Haleygo commented Aug 30, 2021

/retest

Loading

@Haleygo
Copy link
Contributor Author

@Haleygo Haleygo commented Sep 2, 2021

/retest

Loading

@pacoxu
Copy link
Member

@pacoxu pacoxu commented Sep 4, 2021

/priority important-longterm
/triage accepted

Loading

@pacoxu
Copy link
Member

@pacoxu pacoxu commented Sep 10, 2021

/lgtm

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

Loading

@liggitt
Copy link
Member

@liggitt liggitt commented Sep 11, 2021

/label api-review
/assign
/assign @ehashman

Loading

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

@liggitt liggitt Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor Author

@Haleygo Haleygo Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!
Added description according to this

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{"."}
}
}

Loading

Copy link
Member

@ehashman ehashman left a comment

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.

Loading

if obj.ResolverConfig == "" {
obj.ResolverConfig = kubetypes.ResolvConfDefault
if obj.ResolverConfig == nil {
obj.ResolverConfig = utilpointer.String(kubetypes.ResolvConfDefault)
Copy link
Member

@ehashman ehashman Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@@ -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"`
Copy link
Member

@ehashman ehashman Sep 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

@Haleygo Haleygo force-pushed the support-null-resolvConf-in-configFile branch from aef8b8b to 46454ea Sep 14, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 14, 2021
@liggitt liggitt moved this from Assigned to API review completed, 1.23 in API Reviews Sep 14, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Sep 14, 2021

/lgtm
/approve

Loading

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Sep 14, 2021

[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:

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

Loading

@k8s-ci-robot k8s-ci-robot merged commit fa2657b into kubernetes:master Sep 14, 2021
14 checks passed
Loading
SIG Node PR Triage automation moved this from Needs Reviewer to Done Sep 14, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment