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
kubelet: append options to pod if there are multi options in /etc/resolv.conf #112414
Conversation
@pacoxu: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
The KEP about it is https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/504-configurable-pod-dns#proposal
|
Thanks @pacoxu . Let's paste how golang parse the config for your references. https://github.com/golang/go/blob/master/src/net/dnsconfig_unix.go#L39-L140 |
I think we don't need to validate the option or do some aliases merging or further meaning parsing.
to an array like This is enough in this case. |
8c6e8ec
to
468b2a2
Compare
/lgtm It can be argued that is not a bug, just a known limitation of current implementation, since it seems that is working as documented. |
{"options ndots:1\noptions ndots:5 attempts:3", []string{}, []string{}, []string{"ndots:5", "attempts:3"}, false}, | ||
{"options ndots:1 timeout:3 timeout:1 attempts:3\noptions ndots:5", []string{}, []string{}, []string{"ndots:5", "timeout:1", "attempts:3"}, false}, | ||
{"options ndots:1 attempts:3\noptions ndots:1 attempts:3 ndots:5", []string{}, []string{}, []string{"ndots:5", "attempts:3"}, false}, |
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.
can we add a test for badly formatted 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.
can we add a test for badly formatted resolv.conf?
Sorry, I can not get the point. Would you provide a badly formatted options example?
I still can't find any spec that says how this is SUPPOSED to work. I commented more on the issue #112323 |
Absent a real user who is really impacted by this, my inclination is to (again) do nothing. The risk of breakage is non-zero. The reward is zero (nobody is complaining, IIUC). |
/approve Since seems to be valid to have multiple lines with options in a resolv.conf but is not documented how to interpret them, we now merge those options giving preference to the latest entries, making the options field consistent inside the pods, avoiding issues if muslc/libc/... do not agree on the interpretation of the multiline configuration. I do not know if this should be backported, the references to the kubernetes implementation mentions that only the last option line will be used, so we can argue that it was working as expected on the initial design. @pacoxu I suggest to add a more detailed release note |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, pacoxu 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 |
I changed the release note like below.
|
/lgtm |
What type of PR is this?
/kind bug
What this PR does / why we need it:
kubelet will be loading above reslov.conf as below.
Which issue(s) this PR fixes:
Fixes #112323
Special notes for your reviewer:
The feature was added by #54773 long time ago by @phsiao following the design kubernetes/community#1276.
However, per #91052 (comment) as Tim pointed out that
we may keep it as.
I tried centos/ubuntu, both will append extra options to the options list and overwrite them if it already exists by the later settings.
If we want to keep the same behavior, we can accept this pr as a bug fix.
Does this PR introduce a user-facing change?
Design details