-
Notifications
You must be signed in to change notification settings - Fork 104
fix: allow ipv6 on kernel/host level #1298
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,17 @@ func applyNetworks(network config.Network, hostname string) error { | |
| } | ||
| } | ||
|
|
||
| // enable IPv6 before applying NetworkManager profiles | ||
| for _, param := range []string{ | ||
| config.SysctlDisableIPv6All, | ||
| config.SysctlDisableIPv6Default, | ||
| 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)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this only a warning and why is an error silently ignored?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again thanks for the question, will leave the discussion open for others to take a look in case they wonder as well |
||
| } | ||
| } | ||
|
|
||
| err = config.UpdateManagementInterfaceConfig(network, []string{}, config.NMConnectionPath, true) | ||
| if 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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