-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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 IPv6 support for Cilium #11754
Conversation
@@ -223,6 +223,7 @@ func (tf *TemplateFunctions) AddTo(dest template.FuncMap, secretStore fi.SecretS | |||
return strings.Join(labels, ",") | |||
} | |||
|
|||
dest["IsIPv6Only"] = tf.IsIPv6Only |
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.
Same as for Calico, would you be ok with something like this? It would make the template more clear.
At this stage, it is irrelevant if cluster is IPv4 or IPv6, but only if they should be enabled or not.
Later, if we decide to add DualStack, we can keep the template and just modify the logic in the function(s).
dest["IsIPv6Only"] = tf.IsIPv6Only | |
dest["EnableIPv4"] = !tf.IsIPv6Only | |
dest["EnableIPv6"] = tf.IsIPv6Only |
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.
Dual stack is out of scope. If and when it is implemented it will require extensive changes, not just to this.
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 never said it would not require changes or that we will ever do it, just that using EnableIPv4
and EnableIPv6
feels more natural in templates.
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 disagree. Which would be more natural for selecting the health probe host?
There are only two states we support; that should be evident when reading the templates.
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 am not quite sure the health probe host needs changing.
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.
Cilium requires the health check to explicitly use localhost. If this isn't set, the health check will use the node IP, which will fail.
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.
When I put Cilium into IPv6-only mode, it stopped listening to 127.0.0.1
.
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.
Fine by me.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olemarkus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @olemarkus @hakman
Needs
--override=spec.kubeControllerManager.nodeCIDRMaskSize=80
. Will send that as a separate PR as it isn't specific to Cilium.