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.
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 missing HTTPS configs #71
base: main
Are you sure you want to change the base?
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
|
Signed-off-by: Darrick Lau <jkricklau@gmail.com>
Signed-off-by: Darrick Lau <jkricklau@gmail.com>
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
|
@cla-bot check |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
|
The cla-bot has been summoned, and re-checked this pull request! |
Hi! We were about to embark on making similar changes to the chart but stumbled upon the PR. Any chance someone can merge this? |
Yep. I'm happy to integrate changes if I get some feedback. Also, some help on how to appease the cla-bot would be appreciated. I've signed my commits, but it's still complaining. |
I'm having some problems when trying have an https connection to my cluster as well, how did you managed to do it, can you give me some guidance in it please? |
@@ -45,7 +48,8 @@ server: | |||
maxReplicas: 5 | |||
targetCPUUtilizationPercentage: 50 | |||
|
|||
accessControl: {} | |||
accessControl: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert these changes, they're not related to HTTPS improvements.
@@ -24,11 +24,14 @@ server: | |||
path: /etc/trino | |||
http: | |||
port: 8080 | |||
allowInsecureOverHttp: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be added in server.coordinatorExtraConfig
or additionalConfigProperties
. We should not encourage using this, as it reduces the security, so I'm not sure if we want a dedicated value for this. If you disagree, can you open a separate PR with this change?
{{- range .Values.secretMounts }} | ||
- name: {{ .name }} | ||
secret: | ||
secretName: {{ .secretName }} | ||
{{- end }} | ||
{{- if .Values.initContainers.coordinator }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase, I think this should have been fixed already.
While trying to follow the official instructions for TLS/HTTPS, I realised that a lot of the required flags in
config.properties
for the coordinator were not yet implemented.This PR attempts to implement them to the best of my knowledge. This is my very first open-source contribution, so if I did something wrong I am happy to receive feedback and fix my mistakes / convention breaks accordingly.
The following changes were made:
secretMounts
was incorrectly implemented: Indeployment-coordinator.yaml
, theinitContainers
clause was incorrectly in the middle of thevolumes
clause, interrupting the place where thesecretMounts
would be added. I've fixed this to permit correct volume addition for certificates which I assume would be mounted using this mechanism.keystore.keystorePassword
was previously not implemented to support keystores with passwords.keystore.keymanagerPassword
was similarly missing.allowInsecureOverHttp
flag.Edit: Would also like some help on getting around the CLA checks. I've submitted my signed CLA a few days ago, and signed my commits with my email using
git rebase --signoff HEAD~2
after setting my email in my git client, but still getting the error.