fix: allow ipv6 on kernel/host level#1298
Conversation
Allowing ipv6 on kernel level persisted over reboots so on subsequent PR we can create NetworkManager configuration which can configure dual-stack which allows assignment of ipv6 addresses. The current change layers dynamic config on top of the baseline ipv6.conf in the repo along with sysctl config allowing ipv6. Change is safe to merge with no impact because Network Manager still disabled ipv6 on network interface level. Added unit tests with check as part of the table for cos file. Check is part of the table as the assertion steps were different based on directory/file/content. Signed-off-by: Martin Dekov <martin.dekov@suse.com>
martindekov
requested review from
ibrokethecloud,
rrajendran17,
starbops,
tserong and
w13915984028
There was a problem hiding this comment.
Pull request overview
This PR enables IPv6 at the kernel/sysctl level (persisted across reboots) while leaving interface-level IPv6 handling to NetworkManager configuration, preparing the groundwork for future dual-stack support.
Changes:
- Enable IPv6 sysctls at runtime during installer network application (
sysctl -w ...). - Generate a persistent
/etc/sysctl.d/zz-harvester-enable-ipv6.confdrop-in and set equivalent sysctl values in the generated cOS (yip) initramfs stage. - Add unit tests asserting the sysctl directory/file presence/content and sysctl map keys in the generated initramfs stage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/console/network.go | Enables IPv6 sysctls at runtime before applying NetworkManager profiles during installation. |
| pkg/config/cos.go | Adds persistent sysctl drop-in + initramfs-stage sysctl settings to keep IPv6 enabled across reboots. |
| pkg/config/cos_test.go | Adds unit tests validating the new sysctl directory/file and initramfs sysctl map entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // disable multipath for longhorn | ||
| disableLonghornMultipathing(&initramfs) | ||
|
|
||
| // write a persistent sysctl drop-in and apply at runtime; persists after reboot |
There was a problem hiding this comment.
Is there an installer option to indicate if ipv6 is required (dual mode or ipv6 only mode) ? thanks.
There was a problem hiding this comment.
This is the groundwork PR, on subsequent PR (the sub task 1.2 of the installer) we can add that feature along with conditional dual stack cidr and conditional network interface configuration based on installer option / panel
Extracting config in constants and construct the config using fmt.Sprintf along with those Signed-off-by: Martin Dekov <martin.dekov@suse.com>
| config.SysctlDisableIPv6Lo, | ||
| } { | ||
| if out, execErr := exec.Command("sysctl", "-w", fmt.Sprintf("%s=0", param)).CombinedOutput(); execErr != nil { | ||
| logrus.Warnf("Failed to enable IPv6 sysctl %s: %v (%s)", param, execErr, string(out)) |
There was a problem hiding this comment.
Why is this only a warning and why is an error silently ignored?
There was a problem hiding this comment.
Thanks for the review Volker, good question!
As of today we can't say whether we are in dual stack or ipv4 only mode. This is warning as in case it fails it's not critical for harvester - we don't expect ipv6 to work in IPv4 scenario and we don't expect our installations to fail with ipv6 related errors as of today.
Once we have way to say - ok we are in dual stack mode, then the error here becomes critical for the work of the current configuration and it becomes distinguishable. We can either assign err = execErr or skip evaluation altogether. This is close to what Jian said above.
But merging the change as is today won't break anything in case we decide to release tomorrow with this change in.
This is good catch also it is not clear that's why I added as first point in the feature itself to make this a toggle:
Make sure IPv6 is enabled at Kernel level: When we are in dual stack scenario make sure we error out in case we are in dual stack scenario. Currently in applyNetworks we only log warning as we support IPv4 before this feature. Concerns were raised #1298 (comment) and #1298 (comment). So stating it clearly as first point.
There was a problem hiding this comment.
Again thanks for the question, will leave the discussion open for others to take a look in case they wonder as well
Allowing ipv6 on kernel level persisted over reboots so on subsequent PR we can create NetworkManager
configuration which can configure dual-stack which allows assignment of ipv6 addresses.
The current change layers dynamic config on top of the baseline ipv6.conf in the repo along with sysctl config allowing ipv6.
Change is safe to merge with no impact because Network Manager still disabled ipv6 on network interface level.
Added unit tests with check as part of the table for cos file. Check is part of the table as the assertion steps were different based on directory/file/content.
Problem:
Currently we disable ipv6 on a kernel level which also prevents networkmanager from configuring network interface which supports dual stack.
Solution:
Allow ipv6 on kernel level, while still disabling it on network interface level. That way we can then add change which configures the network interface with dual stack along with installer UX.
Related Issue(s):
harvester/harvester#10795
harvester/harvester#10755
Test plan:
Added unit tests.
End to end testing:
After first boot IPv6 is allowed on kernel level
NetworkManager has IPv6 disabled
After reboot kernel settings persisted and NetworkManager still has IPv6 disabled
Additional documentation or context
HEP
The change is added so it won't break the existing functionality or features e.g. - log warning in case we can't enable the system ipv6 settings as of today when we only support ipv4.