add externalTrafficPolicy toggle #145
Conversation
Feature/externaltraffic toggle
charts/docker-mailserver/values.yaml
Outdated
| ## - Local | ||
| ## - Cluster | ||
| ## Set it to "Local" when used with type "LoadBalancer", unless you have a good reason not to. | ||
| # externalTrafficPolicy: Local |
There was a problem hiding this comment.
Maybe uncomment this line out? And then in service.yaml have:
externalTrafficPolicy: {{ .Values.service.externalTrafficPolicy }}
|
good to go now I think |
|
Great effort! The way you do it now, it changes the original behaviour non-transparently. When it's used with Would be better to use an override and check if the override is given, else use original logic. |
I updated the comment in the values to reflect it a bit better, but essentially I removed the logic on purpose to also get edge cases, where the logic fails, to work. Maybe someone has a very good reason to do NodePort + Local or some other combination. |
|
@kaktus42 - Not seeing a change? Service type has a default of "LoadBalancer" which would set "externalTrafficPolicy " to local, which is now its default. Having said that, should the service default should be "ClusterIP"? @furstblumier can you update the Chart version - that will make the lint command work. |
|
@cfis The issue isn't the chart version, but I think I can fix it. I'll also set the default to "ClusterIP" I guess. |
|
Sorry, I read it wrong. Should be good now |
|
Lint test failed.... |
|
Bumped the chart version and hopefully fixed ci this time |
|
Same error as before:
|
|
Alright, now it should be fine. I wasn't aware that with service type "ClusterIP" you aren't "allowed" to set externalTrafficPolicy at all. I adjusted the values accordingly. I also implemented the old default logic again. If you don't set "externalTrafficPolicy" it will use the previous defaults. |
| @@ -26,11 +26,17 @@ metadata: | |||
| spec: | |||
| ## If a load balancer is being used, ensure that the newer type of LB that passes along IP information is used | |||
| ## rather than the legacy one. | |||
| {{- if eq .Values.service.type "LoadBalancer" }} | |||
| {{- if or (eq .Values.service.type "NodePort") (eq .Values.service.type "LoadBalancer") }} | |||
There was a problem hiding this comment.
Only do this if we are using LB or NodePort
| @@ -26,11 +26,17 @@ metadata: | |||
| spec: | |||
| ## If a load balancer is being used, ensure that the newer type of LB that passes along IP information is used | |||
| ## rather than the legacy one. | |||
| {{- if eq .Values.service.type "LoadBalancer" }} | |||
| {{- if or (eq .Values.service.type "NodePort") (eq .Values.service.type "LoadBalancer") }} | |||
| {{- if .Values.service.externalTrafficPolicy }} | |||
There was a problem hiding this comment.
if we have an explicit value set, use that
| {{- if .Values.service.externalTrafficPolicy }} | ||
| externalTrafficPolicy: {{ .Values.service.externalTrafficPolicy }} | ||
| {{- end }} | ||
| {{- if and (not .Values.service.externalTrafficPolicy) (eq .Values.service.type "LoadBalancer") }} |
There was a problem hiding this comment.
if we don't have an explicit value set AND the type is Loadbalancer, use Local
| externalTrafficPolicy: Local | ||
| {{- else if eq .Values.service.type "NodePort" }} | ||
| {{- end }} | ||
| {{- if and (not .Values.service.externalTrafficPolicy) (eq .Values.service.type "NodePort") }} |
There was a problem hiding this comment.
if we don't have an explicit value set AND the type is NodePort, use Cluster
See #144