update port names for Services for istio compliance #170

avimoondra · 2024-05-20T20:25:56Z

Addresses #144, alternate to #151

Addresses: #155, alternate to #156

Lighter weight approach than #154

https://istio.io/v1.0/docs/setup/kubernetes/spec-requirements/

Named ports: Service ports must be named. The port names must be of the form [-] with http, http2, grpc, mongo, or redis as the in order to take advantage of Istio's routing features. For example, name: http2-foo or name: http are valid port names, but name: http2foo is not. If the port name does not begin with a recognized prefix or if the port is unnamed, traffic on the port will be treated as plain TCP traffic (unless the port explicitly uses Protocol: UDP to signify a UDP port).

https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/, could also use appProtocol if prefix in names is not desirable.

Tested in internal (balloon) instances.

(also fixes #155)

avimoondra · 2024-05-20T20:37:07Z

charts/retool/templates/deployment_telemetry.yaml

@@ -175,7 +175,7 @@ metadata:
    {{- include "retool.telemetry.labels" . | nindent 4 }}
 spec:
  ports:
-    - name: statsd-udp


Not needed, but just doing for consistency

Non-TCP based protocols, such as UDP, are not proxied. These protocols will continue to function as normal, without any interception by the Istio proxy but cannot be used in proxy-only components such as ingress or egress gateways.

https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/

avimoondra · 2024-05-21T05:44:12Z

charts/retool/templates/_helpers.tpl

+*/}}
+{{- define "retool.multiplayer.name" -}}
+{{ template "retool.fullname" . }}-multiplayer-ws
+{{- end -}}


was using this originally to name ports, but port names can be max 15 characters

https://stackoverflow.com/questions/73330773/is-there-any-way-to-disable-or-increase-port-name-length-in-kubernetes

IMO it could just be multiplayer-ws as port name, since it will already be on a Service object which has the retool.fullname namespace in its own name?

i think the service portName just had to have http- prefix, but will just keep as is for consistency

and then you can have "retool.name" be longer than 15 chars i suppose 😅

OH I thought this was still being used as port name, but didn't realize you stopped for that reason. good good.

avimoondra · 2024-05-21T06:06:01Z

charts/retool/templates/deployment_backend.yaml

@@ -241,7 +241,7 @@ spec:
        {{- end }}
        ports:
        - containerPort: {{ .Values.service.internalPort }}
-          name: {{ template "retool.name" . }}
+          name: http-server
          protocol: TCP
 {{- if .Values.livenessProbe.enabled }}
        livenessProbe:


there is no service defined here - attempted to add, but wasn't able to get it to compile template w/out error 👀

roberto-retool · 2024-05-21T20:05:07Z

charts/retool/charts/retool-temporal-services-helm/templates/server-deployment.yaml

@@ -198,7 +198,7 @@ spec:
            - name: membership
              containerPort: {{ include (printf "temporal.%s.membershipPort" $service) $ }}
              protocol: TCP
-            - name: metrics
+            - name: http-metrics


don't think we need this since this is the pod's port

roberto-retool · 2024-05-21T20:05:23Z

charts/retool/charts/retool-temporal-services-helm/templates/server-service.yaml

      protocol: TCP
-      name: metrics
+      name: http-metrics


would change this if also change above comment

does changing it complicate anything? I was wondering the same, but OTOH I don't have the change if only for consistency of naming scheme, if it doesn't hurt anything.

maybe a very minor downtime if service port changes before new pod comes up?
but yeah agree if we make them consistent we should do for all (e.g. the two ports defined above)

oh right, we're using a named targetPort here. good point.

actually I think technically, if this causes minor downtime, it's downtime either way, since we have to update targetPort to match the pod's new port name which comes from the Deployment template. we could choose to or not to update name here on the Service object to match purely for consistency, but I don't think that would have any downtime since the Service port number (port here) doesn't change. I think?

it seems like the ports that only need naming are on service and pod

https://istio.io/v1.0/docs/setup/kubernetes/spec-requirements/

I think will stick with the current changes then 👍

avimoondra · 2024-05-21T20:55:30Z

@alexlo03 Re-visiting this, I think it should work istio compliance requirements?

alexlo03

LGTM, I think this is a better/cleaner/clearer approach 🙏

I do not know what the upgrade looks like for existing deployments - renaming a port "should be a no-op" but IDK for sure.

avimoondra force-pushed the avimoondra/update-port-names-for-istio--2 branch from 8b39c05 to 76e43f8 Compare May 20, 2024 20:28

avimoondra commented May 20, 2024

View reviewed changes

avimoondra force-pushed the avimoondra/update-port-names-for-istio--2 branch 3 times, most recently from afbeef3 to 289018a Compare May 20, 2024 21:43

update port names for Services for istio compliance

f52c53a

avimoondra force-pushed the avimoondra/update-port-names-for-istio--2 branch from 289018a to f52c53a Compare May 21, 2024 04:17

avimoondra added 2 commits May 21, 2024 00:50

less than 15 chars

2b7667b

remove service

6bb4313

avimoondra commented May 21, 2024

View reviewed changes

Merge branch 'main' into avimoondra/update-port-names-for-istio--2

f0798a9

avimoondra force-pushed the avimoondra/update-port-names-for-istio--2 branch from 6349293 to f0798a9 Compare May 21, 2024 05:50

avimoondra commented May 21, 2024

View reviewed changes

avimoondra marked this pull request as ready for review May 21, 2024 06:06

avimoondra requested review from ryanartecona and roberto-retool May 21, 2024 06:06

roberto-retool approved these changes May 21, 2024

View reviewed changes

roberto-retool reviewed May 21, 2024

View reviewed changes

ryanartecona approved these changes May 21, 2024

View reviewed changes

alexlo03 approved these changes May 22, 2024

View reviewed changes

avimoondra merged commit ac9499d into main May 22, 2024
12 checks passed

avimoondra deleted the avimoondra/update-port-names-for-istio--2 branch May 22, 2024 14:57

avimoondra mentioned this pull request May 22, 2024

[fix] add configurable port names for istio compatibility #154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update port names for Services for istio compliance #170

update port names for Services for istio compliance #170

avimoondra commented May 20, 2024 •

edited

Loading

avimoondra May 20, 2024

avimoondra May 21, 2024

ryanartecona May 21, 2024

avimoondra May 21, 2024

ryanartecona May 21, 2024

avimoondra May 21, 2024

roberto-retool May 21, 2024

roberto-retool May 21, 2024

ryanartecona May 21, 2024

roberto-retool May 21, 2024

ryanartecona May 21, 2024

ryanartecona May 21, 2024

avimoondra May 21, 2024

avimoondra commented May 21, 2024

alexlo03 left a comment

update port names for Services for istio compliance #170

update port names for Services for istio compliance #170

Conversation

avimoondra commented May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimoondra commented May 21, 2024

alexlo03 left a comment

Choose a reason for hiding this comment

avimoondra commented May 20, 2024 •

edited

Loading