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]: verify ipv6 is enabled before adding ipv6 entries to hosts file #124758
base: master
Are you sure you want to change the base?
Conversation
/sig network |
fbe3b2a
to
eeff940
Compare
eeff940
to
cb7a9da
Compare
@danwinship All problems have been fixed. |
cb7a9da
to
466d4a6
Compare
/test pull-kubernetes-e2e-gce |
466d4a6
to
b4933d2
Compare
/test pull-kubernetes-e2e-kind-ipv6 |
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.
lgtm but I can't actually approve it
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danwinship, HirazawaUi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/remove-sig api-machinery |
/triage accepted |
b4933d2
to
eb0e893
Compare
pkg/kubelet/kubelet_pods.go
Outdated
@@ -85,6 +86,8 @@ const ( | |||
kubeletUser = "kubelet" | |||
) | |||
|
|||
const sysctlAllDisableIPv6 = "/proc/sys/net/ipv6/conf/all/disable_ipv6" |
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.
I think this moved the wrong direction, but I'm not really as deep in SIG node as I once was. I feel like this SHOULD be passing a sysctl.Interface
to be used, rather than hard-coding a path and doing manual parsing. It's rather inconsequential since we don't have tests for ensureHostsFile
. We shouldm and if we did we would not be able to just read from /proc.
@mrunalp @SergeyKanzhelev how much do you care about this? It's minor in the grand scheme of things.
I suggest one of two paths:
-
Go back to
utilsysctl.New().GetSysctl()
and leave a "TODO" comment that this should be injected by the caller. -
Create a kubelet-level
sysctl.Interface
early, store it, and pass it as an argument toensureHostsFile
(or make that a method which reads the Kubelet.sysctl.
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 could be a fun starter project for someone - find all the places in kubelet that use sysctl and make them all use an injected sysctl.Interface
, which is created early in Kubelet's lifecycle.
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.
- Create a kubelet-level
sysctl.Interface
early, store it, and pass it as an argument toensureHostsFile
(or make that a method which reads the Kubelet.sysctl.
I think it would be better to add sysctl.Interface
in the kubelet struct and pass it on, although we need to span makeMounts
-> makeHostsMount
-> ensureHostsFile
-> managedHostsFileContent
......
find all the places in kubelet that use sysctl and make them all use an injected
sysctl.Interface
We currently only use it in setupKernelTunables.
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.
I think it would be better to add ...
I agree, though after writing it, I am not sure that should be in THIS PR. I'd like someone more involved in kubelet to decide if that's important enough to do right now
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.
Okay, let's keep it until sig-node's approver makes the decision.
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.
I think the previous form (utilsysctl.New) is better than this, and the change to injected sysctl.Interface will be simpler
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.
reverted.
a3c9a59
to
6f1b534
Compare
@@ -85,6 +87,8 @@ const ( | |||
kubeletUser = "kubelet" | |||
) | |||
|
|||
const sysctlAllDisableIPv6 = "net/ipv6/conf/all/disable_ipv6" |
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.
I'd suggest to move this constant to the utilsysctl module as it already has similar constants used by Kubelet: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/component-helpers/node/util/sysctl/sysctl.go#L27-L46
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.
I considered doing this, but ultimately decided to put it on the caller.
The sysctl is not quite in the same category as those in the utilsysctl module, and perhaps we should be cautious about adding constants to k8s.io/component-helpers
unless we use it in multiple components.
6f1b534
to
5bac400
Compare
5bac400
to
c41c06d
Compare
/retest |
What type of PR is this?
/kind bug
What this PR does / why we need it:
we should not add IPv6 entries to /etc/hosts on systems when IPv6 is disabled.
Which issue(s) this PR fixes:
Fixes part of #124686
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: