Skip to content
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

allow defining ipFamilyPolicy for external service #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roobre
Copy link
Contributor

@roobre roobre commented Aug 5, 2024

For dual stack (IPv4 and IPv6) clusters, services need to be explicitly opted-in into having two addresses. This is done through the ipFamilyPolicy field, as documented in https://kubernetes.io/docs/concepts/services-networking/dual-stack/#services.

I've wired the templates in a way where if this field is not present, nothing is rendered. This preserves the usual behavior of not specifying this value altogether for services.

@roobre roobre force-pushed the external-service-family branch 2 times, most recently from b52e938 to bb6fbac Compare August 6, 2024 14:47
@roobre
Copy link
Contributor Author

roobre commented Aug 17, 2024

CI/CD fails for reasons unknown to me, but I think the PR itself should be okay.

@@ -19,6 +19,9 @@ spec:
{{- with .Values.front.externalService }}
type: {{ .type | default "ClusterIP" }}
externalTrafficPolicy: {{ .externalTrafficPolicy | default "Local" }}
{{- with .ipFamilyPolicy }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use with here and not if?
It would seem that it's not certain that ipFamilyPolicy exists.
with assumes your variable is present: https://helm.sh/docs/chart_template_guide/control_structures/#modifying-scope-using-with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With and if should behave the same if the variable is absent (not yielding anything), but with only makes you write the name of the field (ipFamilyPolicy) once. So the chance to make a typo is reduced by 50% 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "typo chance" is not a joke, I think the vast majority of helm chart bugs I've seen, fixed, and introduced come from copypasting snippets and forget to change part of them.

@DrPsychick
Copy link

DrPsychick commented Oct 7, 2024

@fastlorenzo this looks good to me, can we get it merged?

Copy link
Contributor

github-actions bot commented Nov 7, 2024

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Nov 7, 2024
@roobre
Copy link
Contributor Author

roobre commented Nov 10, 2024

Unstale

@github-actions github-actions bot removed the Stale label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants