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

avimoondra · 2024-02-28T21:12:50Z

Addresses #144, alternate to #151

Addresses: #155, alternate to #156

helm template -f ~/retool-helm/charts/retool/values.yaml foo ~/retool-helm/charts/retool --set config.encryptionKey="foo" --set image.tag="5.6.11" --set codeExecutor.portName=http-bloop --set workflows.backend.portName=http-bar --set workflows.worker.portName=http-bleep --set workflows.worker.metricsPortName=http-bleep2 | rg "http-bloop|http-bar|http-bleep" -C 5

  ports:
  - protocol: TCP
    port: 80
    targetPort: 3004

    name: http-bloop
---
# Source: retool/templates/deployment_workflows.yaml
---
apiVersion: v1
kind: Service
--
  ports:
  - protocol: TCP
    port: 80
    targetPort: 3000

    name: http-bar
---
# Source: retool/templates/deployment_workflows_worker.yaml
---
apiVersion: v1
kind: Service
--
  ports:
  - protocol: TCP
    port: 3005
    targetPort: 3005

    name: http-bleep

  - protocol: TCP
    port: 9090
    targetPort: http-bleep2
    name: http-bleep2
---
# Source: retool/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
--
          - name: NODE_OPTIONS
            value: --max_old_space_size=1024
        ports:
        - containerPort: 3004

          name: http-bloop

          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
--
                name: foo-retool
                key: google-client-secret
        ports:
        - containerPort: 3000

          name: http-bar

          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
--
                name: foo-retool
                key: google-client-secret
        ports:
        - containerPort: 3005

          name: http-bleep

          protocol: TCP
        - containerPort: 9090
          name: http-bleep2
          protocol: TCP
        livenessProbe:
          httpGet:
            path: /api/checkHealth
            port: 3005

(also fixes #155)

avimoondra · 2024-03-01T11:38:00Z

charts/retool/templates/deployment_workflows_worker.yaml

+    {{ else }}
    name: {{ template "retool.name" . }}


line 346 is the fallback if .Values.workflows.worker.portName is not set, so we should be good here?

I mean like you do above - the conditional L343-L347 becomes one line

name: {{ .Values.workflows.worker.portName | default (template "retool.name" .) }}

alexlo03 · 2024-02-29T13:04:01Z

LGTM

you need to update values.yaml OR document these possible values
IMO there should be a default port name all the time, I believe that is a non-breaking change

avimoondra · 2024-03-01T11:36:52Z

charts/retool/templates/deployment_code_executor.yaml

-  - protocol: TCP
-    port: 9090
-    targetPort: metrics
-    name: metrics


unused, no need to expose this.

charts/retool/templates/deployment_code_executor.yaml

+    {{ if .Values.codeExecutor.portName }}
+    name: {{ .Values.codeExecutor.portName }}
+    {{ else }}
    name: {{ template "retool.name" . }}
-  - protocol: TCP
-    port: 9090
-    targetPort: metrics
-    name: metrics
+    {{ end }}


charts/retool/templates/deployment_code_executor.yaml

+          {{ if .Values.codeExecutor.portName }}
+          name: {{ .Values.codeExecutor.portName }}
+          {{ else }}
          name: {{ template "retool.name" . }}
-          protocol: TCP
-        - containerPort: 9090
-          name: metrics
+          {{ end }}


alexlo03 · 2024-03-01T13:25:56Z

LGTM, the defaulting comments are not necessary but I think you could collapse the conditionals down

avimoondra · 2024-03-01T13:35:05Z

LGTM, the defaulting comments are not necessary but I think you could collapse the conditionals down

The if/else is giving that default - but I ran into an issue using default with "template"... e.g. this wasn't working:

{{ .Values.codeExecutor.portName | default (template "retool.name" .) }}

neither this

{{ default  .Values.codeExecutor.portName (template "retool.name" .) }}

lmk if you have a different suggestion @alexlo03 ! thanks for review.

ryanartecona

I haven't looked through the whole chart in this branch, but do we want to use these new port names for the corresponding Service resources? i.e. https://kubernetes.io/docs/concepts/services-networking/service/#field-spec-ports. I believe a Service can be associated with a port either via port number or port name, so we don't have to, but we'd have the option now with this change. Not a big deal to skip tho.

ryanartecona · 2024-03-04T18:36:49Z

charts/retool/values.yaml

@@ -329,12 +329,15 @@ workflows:
    # Scaling this number will increase the number of workflow workers, e.g. a replicaCount of 4
    # will launch 9 pods -- 1 workflow backend, 4 workflow workers, and 4 for temporal cluster
    replicaCount: 1
+    # portName: custom-port-name, e.g. http-workflows-worker; defaults to "retool.name"
+    # metricsPortName: custom-port-name, e.g. http-workflow-worker-metrics; defaults to "metrics"


For all the defaults in our values.yaml, I think it would make sense to just pick a default port name and possibly just drop the template "retool.name" . and | default "metrics" dynamic fallbacks. The defaults can be static (i.e. no point to using template "retool.name" anyway) and we can even just use the istio naming convention for them. This would remove the need for those conditionals in templates and just simplify a bit. Adding port names is a non-breaking change anyway I am pretty sure, so extremely low risk.

ryanartecona · 2024-03-05T19:22:37Z

I ran into an issue using default with "template"...

oh I didn't notice this comment before, but that's because template doesn't return the value, it just prints it out to the output. you can just change template to include, which returns instead of printing, and then it will work as expected with the default pipeline. usually include is preferred over template in helm for exactly this reason.

avimoondra · 2024-03-06T14:12:26Z

I ran into an issue using default with "template"...

oh I didn't notice this comment before, but that's because template doesn't return the value, it just prints it out to the output. you can just change template to include, which returns instead of printing, and then it will work as expected with the default pipeline. usually include is preferred over template in helm for exactly this reason.

Ah thanks @ryanartecona - will address all of it w/your feedback today! 🙏

alexlo03 · 2024-04-26T18:47:57Z

gentle ping. would like to see this in before we upgrade charts. thanks 🙏

avimoondra · 2024-05-22T15:29:21Z

Closing in favor of #170

add configurable port names for istio compatibility

e75527f

avimoondra force-pushed the avimoondra/update-port-names-for-istio branch from 3be31ab to e75527f Compare February 28, 2024 21:24

bump chart

09da4cc

avimoondra marked this pull request as ready for review February 29, 2024 00:49

alexlo03 reviewed Feb 29, 2024

View reviewed changes

avimoondra commented Mar 1, 2024

View reviewed changes

avimoondra linked an issue Mar 1, 2024 that may be closed by this pull request

Support Istio Clusters #144

Open

give examples in values.yml

e7ed494

avimoondra force-pushed the avimoondra/update-port-names-for-istio branch from 8411bf1 to e7ed494 Compare March 1, 2024 11:44

avimoondra requested review from alexlo03, ryanartecona, BenjamynBaker and roberto-retool and removed request for alexlo03 March 1, 2024 11:44

alexlo03 reviewed Mar 1, 2024

View reviewed changes

avimoondra changed the title ~~add configurable port names for istio compatibility~~ [fix] add configurable port names for istio compatibility Mar 4, 2024

ryanartecona reviewed Mar 4, 2024

View reviewed changes

ryanartecona mentioned this pull request Mar 4, 2024

Remove prom scrape annotations from code-executor, which does not have metrics #156

Closed

address ce metrics issue

7b12289

avimoondra mentioned this pull request May 20, 2024

update port names for Services for istio compliance #170

Merged

avimoondra closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

avimoondra commented Feb 28, 2024 •

edited

Loading

This comment was marked as resolved.

avimoondra Mar 1, 2024

alexlo03 Mar 1, 2024

alexlo03 commented Feb 29, 2024

avimoondra Mar 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

alexlo03 commented Mar 1, 2024

avimoondra commented Mar 1, 2024 •

edited

Loading

ryanartecona left a comment

ryanartecona Mar 4, 2024

ryanartecona commented Mar 5, 2024 •

edited

Loading

avimoondra commented Mar 6, 2024

alexlo03 commented Apr 26, 2024

avimoondra commented May 22, 2024

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

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

Conversation

avimoondra commented Feb 28, 2024 • edited Loading

This comment was marked as resolved.

avimoondra Mar 1, 2024

Choose a reason for hiding this comment

alexlo03 Mar 1, 2024

Choose a reason for hiding this comment

alexlo03 commented Feb 29, 2024

avimoondra Mar 1, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

alexlo03 commented Mar 1, 2024

avimoondra commented Mar 1, 2024 • edited Loading

ryanartecona left a comment

Choose a reason for hiding this comment

ryanartecona Mar 4, 2024

Choose a reason for hiding this comment

ryanartecona commented Mar 5, 2024 • edited Loading

avimoondra commented Mar 6, 2024

alexlo03 commented Apr 26, 2024

avimoondra commented May 22, 2024

avimoondra commented Feb 28, 2024 •

edited

Loading

avimoondra commented Mar 1, 2024 •

edited

Loading

ryanartecona commented Mar 5, 2024 •

edited

Loading