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
When resolvConf is "" in kubelet configuration, pod will be created with wrong dns policy #104624
Conversation
Skipping CI for Draft Pull Request. |
This fixes the mismatch on the behavior between flags and config file. |
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.
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.
/sig node |
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. |
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.
Agree that setting with config file and setting with command argument should be more consistent.
/area kubelet |
/retest |
/priority important-longterm |
/lgtm @kubernetes/sig-node-api-reviews |
/label api-review |
@@ -601,7 +601,7 @@ type KubeletConfiguration struct { | |||
// the node is recommended before changing this field. | |||
// Default: "/etc/resolv.conf" |
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.
the behavior of providing an explicit empty string needs to be described in the field doc
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.
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{"."} | |
} | |
} |
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 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.
if obj.ResolverConfig == "" { | ||
obj.ResolverConfig = kubetypes.ResolvConfDefault | ||
if obj.ResolverConfig == nil { | ||
obj.ResolverConfig = utilpointer.String(kubetypes.ResolvConfDefault) |
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.
Note internal type is remaining string
and we will always default here if unset so the addition of a pointer should not cause NPEs.
@@ -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"` |
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.
This was omitempty before so old clients should behave the right way.
aef8b8b
to
46454ea
Compare
/lgtm |
[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 |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: