-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
ip6tables should be set in the noop plugin #53148
Conversation
/sig network |
/assign @freehan |
/ok-to-test |
/test pull-kubernetes-e2e-gce-etcd3 |
pkg/kubelet/network/plugins.go
Outdated
@@ -215,6 +216,9 @@ func (plugin *NoopNetworkPlugin) Init(host Host, hairpinMode kubeletconfig.Hairp | |||
if err := utilsysctl.New().SetSysctl(sysctlBridgeCallIPTables, 1); err != nil { | |||
glog.Warningf("can't set sysctl %s: %v", sysctlBridgeCallIPTables, err) | |||
} | |||
if err := utilsysctl.New().SetSysctl(sysctlBridgeCallIP6Tables, 1); err != nil { | |||
glog.Warningf("can't set sysctl %s: %v", sysctlBridgeCallIP6Tables, err) |
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.
Does this sysctl even exist if the kernel is booted with IPv6 disabled or compiled out? Not sure if we care about that case, but people still do this when they don't want IPv6, and I believe those make the sysctl simply not present and thus will always error spuriously.
Perhaps you can check if the err is os.IsNotExist() and ignore the warning?
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 can ignore the warning when the element doesn't exist. There is a GetSysctl() to see if it exists, I will look at using that.
fbf338d
to
5f75599
Compare
5f75599
to
b7f5d88
Compare
cc @leblancd |
b7f5d88
to
7422898
Compare
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.
If possible, can you please add a unit test for this? The sysctl pkg has a fake interface
pkg/kubelet/network/plugins.go
Outdated
@@ -217,6 +218,13 @@ func (plugin *NoopNetworkPlugin) Init(host Host, hairpinMode kubeletconfig.Hairp | |||
if err := utilsysctl.New().SetSysctl(sysctlBridgeCallIPTables, 1); err != nil { | |||
glog.Warningf("can't set sysctl %s: %v", sysctlBridgeCallIPTables, err) | |||
} | |||
if val, err := utilsysctl.New().GetSysctl(sysctlBridgeCallIP6Tables); err == nil { |
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.
Should we log the error returned from GetSysctl if it cannot find the module?
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.
Thanks for the review, this PR was changed to not show the error if it cannot find the module.
As dcbw pointed out in an earlier review,
"Does this sysctl even exist if the kernel is booted with IPv6 disabled or compiled out? Not sure if we care about that case, but people still do this when they don't want IPv6, and I believe those make the sysctl simply not present and thus will always error spuriously."
So in the case where it can't find it, it just continues.
@cmluciano I'll look into the unit test with the sysctl pkg. Thanks. |
7422898
to
f37abe9
Compare
/test pull-kubernetes-verify |
@rpothier Since you |
@danehans Thanks, will run that. |
f37abe9
to
1a10234
Compare
This PR needs approval by someone of @thockin @dchen1107 @matchstick @freehan |
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 with a nit.
// Verify the sysctl specified is set | ||
assert.Equal(t, 1, sysctl.Settings[tt.setting], tt.setting+" sysctl should have been set") | ||
// Verify iptables is always set | ||
assert.Equal(t, 1, sysctl.Settings["net/bridge/bridge-nf-call-iptables"], "net/bridge/bridge-nf-call-ip6tables sysctl should have been set") |
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.
net/bridge/bridge-nf-call-ip6tables
-> net/bridge/bridge-nf-call-iptables
?
@MrHohn has the review /approve |
The noop plugin currently sets the iptables for IPv4. This updates that to also set the iptables for IPv6 so IPv6 can have parity with IPv4.
1a10234
to
0fd30ad
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MrHohn, rpothier, thockin Associated issue: 53147 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@cmluciano @freehan @bowei @thockin anything else needed to get this PR merged? |
@danehans Just time, it is already in submit queue :) |
thanks @MrHohn |
/retest Review the full test history for this PR. |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 54436, 53148, 55153, 55614, 55484). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
The noop plugin currently sets the iptables for IPv4.
This updates that to also set the iptables for IPv6 so
IPv6 can have parity with IPv4.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #53147Special notes for your reviewer:
Release note: