Add https port to the service #187
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
1 similar comment
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
| @@ -14,6 +14,12 @@ spec: | |||
| targetPort: http | |||
| protocol: TCP | |||
| name: http | |||
| {{- if .Values.server.config.https.enabled }} | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry, I don't understand how this responds to the concerns I raised.
Can you add a test for the scenario I described?
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
When we configure https in helm, it is not configured in the service, requiring a patch manually. This proposal comes so that when we configure HTTPS, it will be configured in the service.