-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: add support for dual stack clusters #84
Conversation
Signed-off-by: Ludovic Ortega <[email protected]>
Thanks for this contribution! Can you tell us a bit more about a use case when this is useful? Even though it's just an extra feature, I'm a bit concerned by the ever growing number of tuning nobs on the chart! Also, this conflicts with #83 |
# -- Set the ip family policy to configure dual-stack see [Configure dual-stack](https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services) | ||
ipFamilyPolicy: "" | ||
# -- Sets the families that should be supported and the order in which they should be applied to ClusterIP as well. Can be IPv4 and/or IPv6. | ||
ipFamilies: [] | ||
|
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.
Not sure what the best practice is here.
# -- Set the ip family policy to configure dual-stack see [Configure dual-stack](https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services) | |
ipFamilyPolicy: "" | |
# -- Sets the families that should be supported and the order in which they should be applied to ClusterIP as well. Can be IPv4 and/or IPv6. | |
ipFamilies: [] | |
# -- Set the ip family policy to configure dual-stack. See [Configure dual-stack](https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services) | |
# ipFamilyPolicy: "PreferDualStack" | |
# -- Sets the families that should be supported and the order in which they should be applied to ClusterIP as well. Can be IPv4 and/or IPv6. | |
# ipFamilies: ["IPv4","IPv6"] |
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.
We could comment it out to indicate that it's not a required configuration if you prefer but if you use one day helm-docs to comment helm chart configuration commented field will not appears it's why I did that. However if you want to comment it, I believe it's better to retain the current value to demonstrate the default behavior.
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.
the main benefit of commenting out I think is that when we test it, we test that it is compatible with old configuration that didn't have the value!
I believe it's better to retain the current value to demonstrate the default behavior
What is the default behavior actually? ""
and []
are pretty confusing to me.
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.
the main benefit of commenting out I think is that when we test it, we test that it is compatible with old configuration that didn't have the value!
As you want :)
What is the default behavior actually? "" and [] are pretty confusing to me.
They are not set as they are treated as empty value for gotemplate and use default kubernetes values.
The default kubernetes values are :
ipFamilyPolicy: "SingleStack"
ipFamilies: "" # no value, it depends on cluster configuration and ipFamilyPolicy
https://kubernetes.io/docs/reference/kubernetes-api/service-resources/service-v1/
Yes, indeed. Some users, like myself, operate dual-stack clusters with both IPv4 and IPv6 deployed. Quickwit for now only supports IPv4 services in dual stack configuration, as IPv4 takes precedence over IPv6 and this cannot be altered. The introduction of the Additionally, it's worth noting that Quickwit pods already possess IPv4, and only IPv6 services are affected, as detailed in the Kubernetes documentation here: link. Importantly, the current implementation does not constitute a breaking change. Rather, it allow concerned users to opt into IPv6 support if they desire. |
Ok to ship this feature as long as it remains invisible to users who don't care about IPv6. |
Sorry I didn't have the time to test this PR yet, I'll do it early next week. Looking good overall! |
I tested that it works without specifying the new parameters. I don't have a cluster with IPv6 configured so I can't test dual stack, but given that the current functionalities are not impact, I'm merging this anyway. |
Support dual stack clusters :)