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.

Sign up for GitHub

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

Add https port to the service #187

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Open

Add https port to the service #187

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions charts/trino/templates/service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ spec:
targetPort: http
protocol: TCP
name: http
{{- if .Values.server.config.https.enabled }}
Copy link
Member

Choose a reason for hiding this comment

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

We have to think about existing deployments, where the service.port is already set to the same port as https - won't it cause issues to configure two same ports? This change also brings inconsistency - why one port is set using service.port, and another one using server.config.https.port?

Additionally, since there's no way to disable http, changing service.port is currently the only way of limiting access to it.

Copy link
Author

Choose a reason for hiding this comment

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

I think there is already some deployment using a port for https, the administrator should choose another port for http, in the same case for http. I think this option is less problematic than having to manually release the port for https (source from several questions on the forum)

For example, I use a CD tool and I don't have access to kubectl because I'm part of the data team and not the devops team and having to patch the service in kubectl wasn't a very suitable solution for my scenario, which I imagine to be a common scenario and where we set up a hook so we can use it without manual intervention for each deployment.

As for the configuration, the place where I got the https port accessible was without values, if there is another place, we can improve this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand how this responds to the concerns I raised.

Can you add a test for the scenario I described?

- port: {{ .Values.server.config.https.port }}
targetPort: {{ .Values.server.config.https.port }}
protocol: TCP
name: https
{{- end }}
{{- if .Values.jmx.exporter.enabled }}
- port: {{ .Values.jmx.exporter.port }}
targetPort: jmx-exporter
Expand Down