-
Notifications
You must be signed in to change notification settings - Fork 250
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
Enable necessary sysctls in the container and host net namespace for IPv6 #380
Conversation
Fixes #345 |
e44b568
to
11db86d
Compare
@caseydavenport updated the PR as per our discussion. PTAL |
utils/network.go
Outdated
// Without these sysctls enabled, interfaces will come up but they won't get a link local IPv6 address | ||
// which is required to add the default IPv6 route. | ||
if err = writeProcSys("/proc/sys/net/ipv6/conf/all/disable_ipv6", "0"); err != nil { | ||
return 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.
Will this error be descriptive enough?
I think we should wrap all of these so there's more context.
return fmt.Errorf("failed to set conf/all/disable_ipv6=0: %s", 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.
I've updated all the sysctl errors. I used the syntax used for working on sysctl commands so it's more user-friendly.
utils/network.go
Outdated
@@ -258,6 +273,12 @@ func configureSysctls(hostVethName string, hasIPv4, hasIPv6 bool) error { | |||
} | |||
|
|||
if hasIPv6 { | |||
// Make sure ipv6 is enabled on the hostVeth interface in the host network namespace. | |||
// Interfaces won't get a linc local address without this sysctl set to 0. |
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.
typo: linc -> link
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.
Once you wrap the errors with some better descriptions and squash commits this LGTM.
a159f95
to
1576232
Compare
a91f868
to
5aefc3e
Compare
…IPv6 to work remove invalid old test
7680422
to
239c1b9
Compare
|
||
By("Manually programming the route that CNI plugin is going to add.") | ||
netlink.RouteAdd( | ||
&netlink.Route{ |
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.
@caseydavenport I'm removing this whole test (unrelated to this PR) because you can't add a route without the interface being present first which is why this test was failing. There's no easy way to reproduce the interface and route being present before we call CNI ADD, so I'm removing this test for now
Description
Add support for IPv6. This PR should make Calico work with kubernetes in IPv6
Todos
Release Note