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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add missing HTTPS configs #71

wants to merge 2 commits into from

Conversation

Darrekt
Copy link

@Darrekt Darrekt commented Mar 10, 2023

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: In deployment-coordinator.yaml, the initContainers clause was incorrectly in the middle of the volumes clause, interrupting the place where the secretMounts 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.
  • Added support for the 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.

@cla-bot
Copy link

cla-bot bot commented Mar 10, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

Darrick Lau added 2 commits March 10, 2023 16:07
Signed-off-by: Darrick Lau <jkricklau@gmail.com>
Signed-off-by: Darrick Lau <jkricklau@gmail.com>
@cla-bot
Copy link

cla-bot bot commented Mar 10, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@Darrekt
Copy link
Author

Darrekt commented Mar 10, 2023

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Mar 10, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Darrick Lau.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Mar 10, 2023

The cla-bot has been summoned, and re-checked this pull request!

@mach-kernel
Copy link

Hi! We were about to embark on making similar changes to the chart but stumbled upon the PR. Any chance someone can merge this?

@Darrekt
Copy link
Author

Darrekt commented Apr 25, 2023

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.

@Torugo
Copy link

Torugo commented Jan 19, 2024

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:
Copy link
Member

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
Copy link
Member

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 }}
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants