Explicitly enable IPv6 sysctl #113
Conversation
pkg/ipam/ipam_linux.go
Outdated
| // an IPv6 address to the interface | ||
| if !has_enabled_ipv6 && ipc.Version == "6" { | ||
| // Enable IPv6 for the interface being configured as well as | ||
| // loopback "lo", "all" and "default" |
There was a problem hiding this comment.
Any particular reason to do this for lo, all, and default? It may not be wanted/needed on other interfaces created by other CNI chains executing for the same container. Also if we want 'lo' to have IPv6 enabled, then that should probably be decided by the loopback plugin, not here. eg, loopback should call ConfigureIface() with an IPv6 LL address.
There was a problem hiding this comment.
You are right, for all and default I do not have any particularly strong reason. My reasoning was that it would be better to have the default to be "IPv6-enabled" but this is not something CNI need to enforce.
Regarding the loopback interface lo I could not get the kube-dns pod to run without IPv6 enabled on lo. If I understood correctly, kube-dns uses 3 containers sharing the same network NS, and the containers use the loopback device for internal communication. When running kube-dns with IPv6 it expects ::1 (IPv6 on loopback) to be available.
I think it is fairly reasonable to assume that applications running IPv6 would expect IPv6 to be available on loopback.
I have updated the pull-request to only enable IPv6 for lo and the interface being configured by CNI. Thanks for the feedback!
|
Man, it's frustrating that we have to fix this in CNI... Bah. Thanks for taking care of this. Anyways, could you add some code to only touch the sysctl if needed? There are some environments where the |
|
Looking at our |
|
Yes, totally agree, really frustrating. The code should only hit the sysctl if an IPv6 address is about to be configured, no unit tests unfortunately but I tested it manually. The implementation also make sure to only write the sysctl once per interface. Regarding read-only /proc/sys the code should then just silently fail (I intentionally do not check the return value of the sysctl function) with the reasoning that we will fail later anyway. A way to log a warning would be nice though. I am quite the n00b when it comes to go programming so if there is anything that needs to be fixed in the sysctl library I would gladly hand it over to you :) The patch seems to work at least, I have a 20+ nodes cluster which happily runs IPv6-only kubernetes with CNI bridge on Docker 17.12 now. |
|
Ah, you mean reading back the sysctl first and only writing if necessary? Sure, I can fix that. Do you think it is safe to error out if the write fails then? |
If CNI is about to configure an IPv6 address, make sure IPv6 is not disabled through the "disable_ipv6" sysctl setting. This is a workaround for Docker 17.06 and later which sets disable_ipv6=1 for all interfaces even if ipv6 is enabled in Docker.
|
Pull request updated to only write the I had a look at the Sysctl() implementation and the variadic looks OK to me. If you want to read a value you run Btw, there is no logging framework available in CNI at the moment, right? |
|
Hey Nyren, I am still not following even if the sysctl is present how can you write to the /proc/sys file-system of a POD as its only read-only.?? It will always fail. right? |
|
@abhijitherekar let us continue the discussion on why things works as they do in containernetworking/cni#531. Best to keep the pull request comments focused on the implementation details. However, if you did test the patch and it failed, then of course let me know here. |
| // Read current sysctl value | ||
| value, err := sysctl.Sysctl(ipv6SysctlValueName) | ||
| if err != nil || value == "0" { | ||
| // FIXME: log warning if unable to read sysctl value |
There was a problem hiding this comment.
Indeed, we don't currently have a logging strategy. It's not a big deal right now; if this fails, then the address will either apply or fail.
|
Need a second maintainer to approve. |
|
Gonna try and pull this in to the v0.7.0 release. |
|
Hello, I was wondering whether this has been included into k8s 1.13 or whether/how to get this change? It sems I am affected by this docker "bug", too. |
If CNI is about to configure an IPv6 address, make sure IPv6 is not disabled through the "disable_ipv6" sysctl setting.
This is a workaround for Docker 17.x which sets disable_ipv6=1 for all interfaces even if ipv6 is enabled in Docker.
Please see containernetworking/cni#531 for a more detail discussion of the issue.