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

[charts][trino] add additionalSecrets #297

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

Conversation

luismacosta
Copy link

@luismacosta luismacosta commented Jan 27, 2025

[charts][trino] add additionalSecrets:

  • move catalogs from confimap to secrets
  • use ldap config as a secret

Purpose: allow catalogs configuration using secrets. Don't expose passwords in a configMap

@nineinchnick @mosabua can you please review? Thanks
cla sent today via email

Copy link

cla-bot bot commented Jan 27, 2025

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

cla-bot bot commented Jan 27, 2025

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

@nineinchnick
Copy link
Member

Check the CI workflow runs for errors

Copy link

cla-bot bot commented Jan 27, 2025

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

cla-bot bot commented Jan 27, 2025

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

@mosabua
Copy link
Member

mosabua commented Jan 27, 2025

What is the motivation and need for this change? Also .. is this a breaking change (which is bad in my opinion) or does it duplicate and allow both setups (which is also bad)

@luismacosta
Copy link
Author

luismacosta commented Jan 27, 2025

I would like to avoid having passwords in the configmap and use secrets instead

@mosabua
Copy link
Member

mosabua commented Jan 27, 2025

You can do that already .. there is no need to have the whole catalog as secret

Copy link

cla-bot bot commented Jan 28, 2025

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

5 similar comments
Copy link

cla-bot bot commented Jan 28, 2025

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

Copy link

cla-bot bot commented Jan 28, 2025

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

Copy link

cla-bot bot commented Jan 28, 2025

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

Copy link

cla-bot bot commented Jan 28, 2025

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

Copy link

cla-bot bot commented Jan 28, 2025

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

Copy link

cla-bot bot commented Jan 28, 2025

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

3 similar comments
Copy link

cla-bot bot commented Feb 3, 2025

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

Copy link

cla-bot bot commented Feb 3, 2025

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

Copy link

cla-bot bot commented Feb 3, 2025

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

@luismacosta
Copy link
Author

luismacosta commented Feb 3, 2025

Hello @nineinchnick

I've applied the changes requested
In order to allow both ConfigMap or Secret, for catalogs creation, this valuation had to be changed:

{{- if or .Values.catalogs .Values.additionalCatalogs}}

Copy link

cla-bot bot commented Feb 3, 2025

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

6 similar comments
Copy link

cla-bot bot commented Feb 3, 2025

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

Copy link

cla-bot bot commented Feb 4, 2025

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

Copy link

cla-bot bot commented Feb 4, 2025

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

Copy link

cla-bot bot commented Feb 4, 2025

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

Copy link

cla-bot bot commented Feb 4, 2025

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

Copy link

cla-bot bot commented Feb 4, 2025

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

Copy link

cla-bot bot commented Feb 12, 2025

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

Copy link

cla-bot bot commented Feb 12, 2025

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

Copy link

cla-bot bot commented Feb 12, 2025

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

Copy link

cla-bot bot commented Feb 12, 2025

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

Copy link

cla-bot bot commented Feb 13, 2025

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

Copy link

cla-bot bot commented Feb 13, 2025

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

Copy link

cla-bot bot commented Feb 13, 2025

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

@@ -0,0 +1,19 @@
catalogs: {}
catalogsSecrets:
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of using this over catalogs? Please update the PR description with the rationale for the change.

@nineinchnick
Copy link
Member

Purpose: allow catalogs configuration using secrets. Don't expose passwords in a configMap

Secrets are not a lot more secure. They obfuscate the values, but if anyone has access to configmaps, they can read secrets too. There's also no audit logs who read secrets. It's better to reference sensitive properties from outside, see https://trino.io/docs/current/security/secrets.html. You can already mount additional secrets as env vars using .Values.envFrom.

@luismacosta
Copy link
Author

luismacosta commented Feb 13, 2025

Purpose: allow catalogs configuration using secrets. Don't expose passwords in a configMap

Secrets are not a lot more secure. They obfuscate the values, but if anyone has access to configmaps, they can read secrets too. There's also no audit logs who read secrets. It's better to reference sensitive properties from outside, see https://trino.io/docs/current/security/secrets.html. You can already mount additional secrets as env vars using .Values.envFrom.

I've many catalogs configured, mount additional secrets as env vars would required different envs vars per each catalog, mount additional secrets as env vars using .Values.envFrom. is an approach that we do want to follow. My goal is to have simple config. Declare all catalogs config in the same secrets.yaml, as I'm using mozilla sops to encrypt it as a values.yaml, in order to have the config committed in github.

@nineinchnick
Copy link
Member

mount additional secrets as env vars would required different envs vars per each catalog, mount additional secrets as env vars using .Values.envFrom.

Can you give an example?

@luismacosta
Copy link
Author

luismacosta commented Feb 13, 2025

mount additional secrets as env vars would required different envs vars per each catalog, mount additional secrets as env vars using .Values.envFrom.

Can you give an example?

I've:

  • Chart.yaml: trino helm chart is defined as a dependency
  • values.yaml: overrides defaults values.yaml from helm chart
trino:
  catalogs: {}
  secretMounts:
    - name: catalogs
      secretName: catalogs
      path: /etc/trino/catalog
  • secrets.yaml: all catalogs configured using catalogsSecrets and the file is encrypted with mozilla sops, using vault key
trino:
  - name: catalogs
    value:
      #hive
      aaaaa.properties: |
        connector.name=hive
        hive.metastore.uri=
        fs.native-s3.enabled=true
      #iceberg
      bbbbb.properties: |
          connector.name=iceberg
          hive.metastore.uri=
      #mariadb
      cccc.properties: |
        connector.name=mariadb
        connection-url=
        connection-user=
        connection-password=
      #postgresql
      dddd_data.properties: |
        connector.name=postgresql
        connection-url=
        connection-user=
        connection-password=
  • ArgoCD appSet reads values.yaml + secrets.yaml as values, uses vault key to decrypt secrets.yaml and deploy trino in eks cluster

@nineinchnick
Copy link
Member

Sorry, I don't understand this use case. All the catalogs you provided are different. I don't see why you couldn't reference env vars in them and mount the env vars from a secret.

@luismacosta
Copy link
Author

luismacosta commented Feb 13, 2025

Sorry, I don't understand this use case. All the catalogs you provided are different. I don't see why you couldn't reference env vars in them and mount the env vars from a secret.

Those catalogs are just an example of configuration. I've multiple catalogs for several database engines. I can refer env vars and mount the env vars from a secret. In that case I would need multiple env vars for each catalog password and declare them with .Values.envFrom. It would be a complex configuration in my values.yaml. With secrets.yaml I can have the entire catalogs config in one place, without having the need to declare .Values.envFrom with many env vars.

@nineinchnick
Copy link
Member

You reference a single secret in .Values.envFrom and all its keys will be available as different environmental variables. You manage this secret outside the chart. Can you give a complete example how this requires a complex configuration in values.yaml?

@luismacosta
Copy link
Author

luismacosta commented Feb 13, 2025

You reference a single secret in .Values.envFrom and all its keys will be available as different environmental variables. You manage this secret outside the chart. Can you give a complete example how this requires a complex configuration in values.yaml?

Let's say that I've 50 catalogs (postgresql, mariadb, mongodb, scylladb, hive, iceberg)

values.yaml

trino: 
  envFrom:
     - secretRef:
       name: catalogs
  catalogs:
    aaaaa.properties: |
      connector.name=hive
      hive.metastore.uri=
      fs.native-s3.enabled=true
    #iceberg
    bbbbb.properties: |
        connector.name=iceberg
        hive.metastore.uri=
    #mariadb
    cccc.properties: |
      connector.name=mariadb
      connection-url=
      connection-user=
      connection-password=${ENV:DB_PASSWORD_CCCC}
    #postgresql
    dddd_data.properties: |
      connector.name=postgresql
      connection-url=
      connection-user=
      connection-password=${ENV:DB_PASSWORD_DDDD}

The secret.yaml from helm chart (https://github.com/trinodb/charts/blob/main/charts/trino/templates/secret.yaml) does not allow to create more secrets, but that's not a problem, I could create my own template, which would need a config to store all passwords as env vars. I would like to avoid the creation of this secret. With the change that I'm proposing in this PR, is possible to create a single secret, where I have catalogs configured with the passwords under connection-password=passordx and I don't need to declare .Values.envFrom. Also, I would like to pull your chart without having the need to add extra templates.

Copy link

cla-bot bot commented Feb 18, 2025

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

@nineinchnick
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Feb 19, 2025
Copy link

cla-bot bot commented Feb 19, 2025

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

@luismacosta
Copy link
Author

@nineinchnick Can you please review this PR? Thanks.

@nineinchnick
Copy link
Member

So the main change here is to store catalogs in secrets, instead of a configmap? I don't think adding a second property is a good way to solve this. Using a secret is not more secure than using a configmap, and makes debugging harder, since you have to decode its contents. Think about how users will need to pick which property to use.

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

Successfully merging this pull request may close these issues.

3 participants