Support ipFamilyPolicy on services #375
Conversation
UXabre
requested review from
a team,
TheAlgo and
DandyDeveloper
as code owners
UXabre
force-pushed
the
feature/374
branch
5 times, most recently
from
9510cd3 to
6233bd6
Compare
|
Thanks for the PR, we will review it and get back to you. Thanks!! |
| @@ -13,6 +13,9 @@ metadata: | |||
| {{- end }} | |||
| spec: | |||
| type: {{ .Values.service.type }} | |||
| {{- if (semverCompare ">= 1.23-0" .Capabilities.KubeVersion.Version) }} | |||
| ipFamilyPolicy: {{ .Values.service.ipFamilyPolicy }} | |||
There was a problem hiding this comment.
Hey @UXabre thanks for PR, this is good change to have, can you please let us know what would be the default value, is this a breaking change if a chart is updated with helm upgrade ?
Thank you
There was a problem hiding this comment.
Hi, I actually set the default value to SingleStack to be more compatible (as many people don't use dual stack yet). However, now I'm thinking about it, I think it's safer to just check if the variable is set and thus fallback to the default that the CNI provides (at least, to my understanding the CNI would define the implicit default).
Because if the singlestack default and the assumption not many people are running dual stack clusters, I don't expect it to be a breaking change but it would be safer to not alter it if not set explicitely
Also, another change, to be more versatile would be to also provide a config for the IPFamilies as well, this way, one could set it to singlestack and ipv6 (if they are brave :p).
Curious to hear what you think of this proposal.
There was a problem hiding this comment.
Agree with @prudhvigodithi, lets check on backward compatibility
|
Hi, I've added a check to see if ipFamilyPolicy is set, not setting it reverts to following the CNI defaults instead (so is the most compatible way imaginable). I've also included a commit with the ipFamilies field, this gives the most versatility to this PR. If you want, I can squash this into one commit to not pollute the git log. |
|
Hi @TheAlgo , there is (some) documentation in the values file, do you want me to further elaborate in the values files or where would you like some more documentation? :-) |
Signed-off-by: Arend Lapere <[email protected]>
Signed-off-by: Arend Lapere <[email protected]>
UXabre
requested review from
bbarani,
peterzhuamazon and
gaiksaya
as code owners
|
Hi @prudhvigodithi seems like @UXabre has updated the PR. Thanks. |
|
Hi, I've added the options to the readme as well as a link to the official k8s documentation regarding this. |
|
@prudhvigodithi Lets get this to closure whenever you are free. Thanks |
|
Thanks @UXabre merged this PR, can you please backport this change to 1.x branch? |
prudhvigodithi
added
the
backport 1.x
|
OK, will make a PR soon for the backport |
Hi @UXabre any progress on the backport? |
* Support ipFamilyPolicy on services Signed-off-by: Arend Lapere <[email protected]> * Support ipFamilies on services Signed-off-by: Arend Lapere <[email protected]> --------- Signed-off-by: Arend Lapere <[email protected]> Signed-off-by: Peter Zhu <[email protected]>
* Support ipFamilyPolicy on services * Support ipFamilies on services --------- Signed-off-by: Arend Lapere <[email protected]> Signed-off-by: Peter Zhu <[email protected]> Co-authored-by: Arend Lapere <[email protected]>
Signed-off-by: Arend Lapere [email protected]
Description
Add the ability to set a service as being a dual stack service for both dashboard as cluster.
Issues Resolved
Check List
For any changes to files within Helm chart directories:
CHANGELOG.mdupdated to reflect changeBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.