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

Configure SQL TLS environment variables in server-job #411

Merged
merged 3 commits into from
Jun 14, 2024

Conversation

grzegorz8
Copy link
Contributor

@grzegorz8 grzegorz8 commented Jul 20, 2023

What was changed

Added support for SQL_TLS* properties in schema jobs (server-job.yaml).

Why?

Currently, when PostgreSQL database is secured with TLS, schema jobs are not properly configured.

In the Pull Request we reuse server.config.persistence.*.sql.tls, server.additionalVolumes and server.additionalVolumeMounts properties..

Checklist

  1. Closes
    No related issues.

  2. How was this tested:

  1. Any docs updates needed?

No updates needed.

@grzegorz8 grzegorz8 requested review from a team as code owners July 20, 2023 12:01
@CLAassistant
Copy link

CLAassistant commented Jul 20, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@LukaGiorgadze
Copy link

@grzegorz8
May I ask you - what does it mean "schema jobs are not properly configured"?

@grzegorz8
Copy link
Contributor Author

@grzegorz8 May I ask you - what does it mean "schema jobs are not properly configured"?

I meant that TLS related env variables are not set (SQL_TLS_KEY_FILE, SQL_TLS_CERT_FILE, etc).

@arkoc
Copy link

arkoc commented Oct 3, 2023

SQL_TLS=true is required option for connecting to Azure Postgres Flexible Servers. Probably it is required for other 'serverless' managed databases, but we only tested on Azure.

This is the error we recieved (without SQL_TLS)

2023-10-03T12:34:11.898Z	ERROR	Unable to create SQL database.	{"error": "unable to connect to DB, tried default DB names: postgres,defaultdb, errors: [pq: no pg_hba.conf entry for host \"XX.XX.XX.XX\", user \"admin\", database \"postgres\", no encryption pq: no pg_hba.conf entry for host \"XX.XX.XX.XX\", user \"admin\", database \"defaultdb\", no encryption]", "logging-call-at": "handler.go:94"}

@debugger24
Copy link
Contributor

Hi @grzegorz8 any update on this PR ?

Copy link
Contributor

@debugger24 debugger24 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@MasonLegere MasonLegere left a comment

Choose a reason for hiding this comment

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

This looks good to me - tested with Postgres 15 on RDS

@syugoman-armenotech
Copy link

Hi everyone! Any updates for this PR?

@syugoman-armenotech
Copy link

@grzegorz8
Copy link
Contributor Author

@grzegorz8 Hi! Can you please update an example? It will be helpful https://github.com/temporalio/helm-charts/blob/master/charts/temporal/values/values.postgresql.yaml

Example updated. Please check if it looks fine.

@ChristianGottinger
Copy link

Tested the Branch with Azure MySql with TLS enabled. Works.

@devinargenta
Copy link

Hello! Wanted to bump this as it would be useful for us. Thank you!

@moss2k13
Copy link

can someone clarify the status of this PR?

we are using these changes without issues with postgresql versions 15.x

# certFile: /path/to/certs/<client-cert-file>
# keyFile: /path/to/certs/<client-key-file>

# additionalVolumes:
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be indented so that they are under server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robholland Thanks, fixed.

@robholland
Copy link
Contributor

Looks good, one tiny tweak.

@robholland robholland requested review from a team and removed request for a team June 14, 2024 07:56
@robholland robholland added the needs revision Team has requested some changes label Jun 14, 2024
@grzegorz8 grzegorz8 requested a review from tsurdilo as a code owner June 14, 2024 11:02
@robholland robholland merged commit b9df837 into temporalio:master Jun 14, 2024
2 checks passed
@robholland
Copy link
Contributor

Thanks for your contribution :) For future reference, when you've attended to feedback in PR, if you can re-request review from the reviewer, it helps us spot when something is ready for us to look at again.

@robholland robholland removed the needs revision Team has requested some changes label Jun 14, 2024
mihaelabalas84 added a commit to fairmoney/temporal-helm-charts that referenced this pull request Aug 16, 2024
* update ui image to 2.25.0 (temporalio#478)

Signed-off-by: Tihomir Surdilovic <tihomir@temporal.io>

* Allow forcing a specific chart version. (temporalio#479)

This is useful for patch releases on older release lines.

* Update Chart to 0.37.0, Temporal v1.23.1

* Ensure appVersion is used by default as the server image tag. (temporalio#488)

* Bumps server version to the specified appVersion
* Use chart.appversion instead of image.tag from values in deployment spec
* Allow overriding deployment spec with image.tag

---------

Co-authored-by: Kshitij <kshitij.tulsyan@observe.ai>

* Update Chart to 0.38.1, Temporal v1.23.1

* [Bug] Allow and document configuring Web UI via values.yaml (temporalio#394)

1. remove web ui config in values.yaml
2. remove web config volume in web-deployment.yaml
3. remove web-config config map
4. remove line 280 since the install bash didn't configure web ui auth
5. update the document for web ui configuration with env variable

Co-authored-by: Rob Holland <rob.holland@gmail.com>

* fix(imageTag): Fix default type in values for imageTag (temporalio#489)

* Update Chart to 0.39.0, Temporal v1.24.0

* Update README.md

* Update Chart to 0.39.1, Temporal v1.24.1

* Switch to devrel.

* Support new admintools image tag format (temporalio#493)

* Require tags for server, admintools and ui.

Don't use server tag for admintools, it's versioned separately now.

* Update Chart to 0.40.0.

* fix: add support for pre upgrade. (temporalio#476)

* adding missing ImagePullSecrets section to web deployment

* Whitespace.

* fix: Use visibility server when defining visibility config (temporalio#436)

Co-authored-by: Rob Holland <rob@temporal.io>

* fix: sidecarContainers should be an array, not a dict

* Use tplvalues.render for templating inside values (temporalio#492)

Allow templated values inside the values.yaml for web annotations

* Apply security context regardless of persistence engine. (temporalio#494)

Replaces temporalio#308.

* Update Chart to 0.40.1.

* Allow to skip database creation (temporalio#480)

Signed-off-by: Valentin Zayash <valioozz@gmail.com>

* Update Cassandra host URLs to remove the ".cluster.local" suffix (temporalio#485)

* Update Cassandra host URLs to remove the ".cluster.local" suffix

* Configure SQL TLS environment variables in server-job (temporalio#411)

* Configure SQL_TLS environment variables in server-job

* Update Chart to 0.41.0.

* Fix weird helm lint false alarm. Fixes temporalio#284.

* Helm 2 compat. Fixes temporalio#187.

* Update codeowners.

* Revert "Update Cassandra host URLs to remove the ".cluster.local" suffix (temporalio#485)" (temporalio#500)

This reverts commit b25c4fc.

* Update Chart to 0.41.1.

* Adds a PodDisruptionBudget and topologySpreadConstraints to web (temporalio#409)

* Adds a PodDisruptionBudget and topologySpreadConstraints to web

* fixing type on topologySpreadConstraints
topologySpreadConstraints should be a list instead of map

* Update Chart to 0.42.0.

* Update README to mention you cannot do Cassandra only anymore. (temporalio#499)

Related: temporalio#470.

* Remove invalid line. (temporalio#502)

Fixes temporalio#426

* Service account should be set when present, even if not creating. (temporalio#498)

Fixes temporalio#403.

* Correct outdated port config. (temporalio#497)

Fixes temporalio#333 and temporalio#149.

Context: temporalio/temporal#650

* Update Chart to 0.43.0.

* ElasticSearch -> Elasticsearch

* fix: Escape ES credentials (temporalio#505)

* feat: updated grafana and prometheus helm dependencies (temporalio#424)

* feat: bump grafana and prometheus charts versions

Signed-off-by: David Calvert <david@0xdc.me>

* Remove hard coded cluster.local references. (temporalio#501)

Switch check-cassandra-service init container to use nc. nslookup in recent
busybox images is broken and doesn't obey resolv.conf, meaning it won't check
k8s search domains.

* Enable HTTP API for Nexus (temporalio#511)

* Enable HTTP API for Nexus

* Update charts/temporal/templates/server-service.yaml

* Add FE Ingress (temporalio#435)

* Add ingress for frontend

* Add CONTRIBUTING.

* Note contributing in README.

* Note slack channel.

* Use a shared config map for all services. (temporalio#514)

Remove helpers in favour of defaults via values file.
Remove unused Elasticsearch environment variables.
Stop switching between es-visibility and visibility for store names.

* Pass `ct lint`

* Lint chart on PR.

* Setup for `ci install`. (temporalio#522)

* Setup for `ci install`.

Adds a test so that `helm test` now checks that the cluster is healthy after
the system is deployed.

Refactors server-job to remove the use of helm hooks which cause lot of users
pain and don't work with --wait, or the mechanism that helm test uses.

Improve the handling of elasticsearch by treating it just like the other drivers.

Correctly handle the schema.createDatabase setting which previously had edge cases.

* Secret key fixes.

* Fix missed ES quoting issue.

* Add postgres-es test.

* Fix branch reference.

* Update Chart to 0.44.0.

* Update README to use repo. (temporalio#531)

* Update README to use repo.

Fixes temporalio#458

---------

Co-authored-by: Alex Garnett <axfelix@gmail.com>

* Check for pgx driver as well as plain postgres. (temporalio#546)

Fixes temporalio#532

* Templatize resourceLabels for standardization (temporalio#539)

* Templatize resourceLabels for standardization

---------

Signed-off-by: Tihomir Surdilovic <tihomir@temporal.io>
Signed-off-by: Valentin Zayash <valioozz@gmail.com>
Signed-off-by: David Calvert <david@0xdc.me>
Co-authored-by: Tihomir Surdilovic <tihomir@temporal.io>
Co-authored-by: Rob Holland <rob.holland@gmail.com>
Co-authored-by: Temporal Data <commander-data@temporal.io>
Co-authored-by: Kshitij Tulsyan <ktulsyan1990@gmail.com>
Co-authored-by: Kshitij <kshitij.tulsyan@observe.ai>
Co-authored-by: Jingyu <56581242+washanhanzi@users.noreply.github.com>
Co-authored-by: Theo REY <account@reyth.dev>
Co-authored-by: Alex Shtin <alex@shtin.com>
Co-authored-by: Rob Holland <rob@temporal.io>
Co-authored-by: Punit Kulal <punitkulal1996@gmail.com>
Co-authored-by: Gerardo Enrique Mora Salazar <gerardo@ibm.com>
Co-authored-by: Giovanny Gutiérrez <giovanny.gutierrez@commure.com>
Co-authored-by: vogre <334187+vogre@users.noreply.github.com>
Co-authored-by: Valentin Zayash <VLZZZ@users.noreply.github.com>
Co-authored-by: Chris Taylor <taylor.cj@gmail.com>
Co-authored-by: Grzegorz Kołakowski <grzegorz8@gmail.com>
Co-authored-by: Prathyush PV <prathyushpv@gmail.com>
Co-authored-by: sringel <903498+sringel@users.noreply.github.com>
Co-authored-by: Alex Shtin <alex@temporal.io>
Co-authored-by: David Calvert <david@0xdc.me>
Co-authored-by: Roey Berman <roey@temporal.io>
Co-authored-by: Rahul Kumar <rahulcomp24@gmail.com>
Co-authored-by: Alex Garnett <axfelix@gmail.com>
Co-authored-by: Sahil Vazirani <sahilvv@gmail.com>
@esn89
Copy link

esn89 commented Oct 2, 2024

Does this additionalVolumeMount nest in:
server.config.additionalVolumes

Like what the original post says?

Or nested under server.additionalVolumes as per:
https://github.com/temporalio/helm-charts/pull/411/files#diff-5f8eb04d49dd7caffd33da659dd7bae84435c114a6255319b2965dcf6a2536abR254

mihaelabalas84 added a commit to fairmoney/temporal-helm-charts that referenced this pull request Oct 2, 2024
* update ui image to 2.25.0 (temporalio#478)

Signed-off-by: Tihomir Surdilovic <tihomir@temporal.io>

* Allow forcing a specific chart version. (temporalio#479)

This is useful for patch releases on older release lines.

* Update Chart to 0.37.0, Temporal v1.23.1

* Ensure appVersion is used by default as the server image tag. (temporalio#488)

* Bumps server version to the specified appVersion
* Use chart.appversion instead of image.tag from values in deployment spec
* Allow overriding deployment spec with image.tag

---------

Co-authored-by: Kshitij <kshitij.tulsyan@observe.ai>

* Update Chart to 0.38.1, Temporal v1.23.1

* [Bug] Allow and document configuring Web UI via values.yaml (temporalio#394)

1. remove web ui config in values.yaml
2. remove web config volume in web-deployment.yaml
3. remove web-config config map
4. remove line 280 since the install bash didn't configure web ui auth
5. update the document for web ui configuration with env variable

Co-authored-by: Rob Holland <rob.holland@gmail.com>

* fix(imageTag): Fix default type in values for imageTag (temporalio#489)

* Update Chart to 0.39.0, Temporal v1.24.0

* Update README.md

* Update Chart to 0.39.1, Temporal v1.24.1

* Switch to devrel.

* Support new admintools image tag format (temporalio#493)

* Require tags for server, admintools and ui.

Don't use server tag for admintools, it's versioned separately now.

* Update Chart to 0.40.0.

* fix: add support for pre upgrade. (temporalio#476)

* adding missing ImagePullSecrets section to web deployment

* Whitespace.

* fix: Use visibility server when defining visibility config (temporalio#436)

Co-authored-by: Rob Holland <rob@temporal.io>

* fix: sidecarContainers should be an array, not a dict

* Use tplvalues.render for templating inside values (temporalio#492)

Allow templated values inside the values.yaml for web annotations

* Apply security context regardless of persistence engine. (temporalio#494)

Replaces temporalio#308.

* Update Chart to 0.40.1.

* Allow to skip database creation (temporalio#480)

Signed-off-by: Valentin Zayash <valioozz@gmail.com>

* Update Cassandra host URLs to remove the ".cluster.local" suffix (temporalio#485)

* Update Cassandra host URLs to remove the ".cluster.local" suffix

* Configure SQL TLS environment variables in server-job (temporalio#411)

* Configure SQL_TLS environment variables in server-job

* Update Chart to 0.41.0.

* Fix weird helm lint false alarm. Fixes temporalio#284.

* Helm 2 compat. Fixes temporalio#187.

* Update codeowners.

* Revert "Update Cassandra host URLs to remove the ".cluster.local" suffix (temporalio#485)" (temporalio#500)

This reverts commit b25c4fc.

* Update Chart to 0.41.1.

* Adds a PodDisruptionBudget and topologySpreadConstraints to web (temporalio#409)

* Adds a PodDisruptionBudget and topologySpreadConstraints to web

* fixing type on topologySpreadConstraints
topologySpreadConstraints should be a list instead of map

* Update Chart to 0.42.0.

* Update README to mention you cannot do Cassandra only anymore. (temporalio#499)

Related: temporalio#470.

* Remove invalid line. (temporalio#502)

Fixes temporalio#426

* Service account should be set when present, even if not creating. (temporalio#498)

Fixes temporalio#403.

* Correct outdated port config. (temporalio#497)

Fixes temporalio#333 and temporalio#149.

Context: temporalio/temporal#650

* Update Chart to 0.43.0.

* ElasticSearch -> Elasticsearch

* fix: Escape ES credentials (temporalio#505)

* feat: updated grafana and prometheus helm dependencies (temporalio#424)

* feat: bump grafana and prometheus charts versions

Signed-off-by: David Calvert <david@0xdc.me>

* Remove hard coded cluster.local references. (temporalio#501)

Switch check-cassandra-service init container to use nc. nslookup in recent
busybox images is broken and doesn't obey resolv.conf, meaning it won't check
k8s search domains.

* Enable HTTP API for Nexus (temporalio#511)

* Enable HTTP API for Nexus

* Update charts/temporal/templates/server-service.yaml

* Add FE Ingress (temporalio#435)

* Add ingress for frontend

* Add CONTRIBUTING.

* Note contributing in README.

* Note slack channel.

* Use a shared config map for all services. (temporalio#514)

Remove helpers in favour of defaults via values file.
Remove unused Elasticsearch environment variables.
Stop switching between es-visibility and visibility for store names.

* Pass `ct lint`

* Lint chart on PR.

* Setup for `ci install`. (temporalio#522)

* Setup for `ci install`.

Adds a test so that `helm test` now checks that the cluster is healthy after
the system is deployed.

Refactors server-job to remove the use of helm hooks which cause lot of users
pain and don't work with --wait, or the mechanism that helm test uses.

Improve the handling of elasticsearch by treating it just like the other drivers.

Correctly handle the schema.createDatabase setting which previously had edge cases.

* Secret key fixes.

* Fix missed ES quoting issue.

* Add postgres-es test.

* Fix branch reference.

* Update Chart to 0.44.0.

* Update README to use repo. (temporalio#531)

* Update README to use repo.

Fixes temporalio#458

---------

Co-authored-by: Alex Garnett <axfelix@gmail.com>

* Check for pgx driver as well as plain postgres. (temporalio#546)

Fixes temporalio#532

* Templatize resourceLabels for standardization (temporalio#539)

* Templatize resourceLabels for standardization

* Feat add authorization options (temporalio#542)

Server authorization config.

* Quote sql password for default store in the same way as the visibility store (temporalio#551)

* Update Chart to 0.45.0.

* Update Chart to 0.45.1.

* Update UI version description (temporalio#556)

* Use admintools-env and secret for ES password consistent with e.g. jobs (temporalio#530)

* Provide option to create default namespace (temporalio#550)

* Provide option to create default namespace
* Create multiple namespaces with optional retention
---------

Co-authored-by: Manan Mangal <mmangal@paloaltonetworks.com>

* add Job annotations and labels (temporalio#536)

* add job labels and annotations

Signed-off-by: André Bauer <andre.bauer@staffbase.com>
Co-authored-by: Rob Holland <rob.holland@gmail.com>

---------

Signed-off-by: André Bauer <andre.bauer@staffbase.com>
Co-authored-by: Rob Holland <rob.holland@gmail.com>

* Config to specify tags to be excluded in prometheus metrics (temporalio#566)

* Config to specify tags to be excluded in prometheus metrics

---------

Co-authored-by: Rob Holland <rob@temporal.io>

* Update Chart to 0.46.0.

* Ensure we use global for includes. (temporalio#568)

Some call sites used . which was sometimes not the global context.

* Update Chart to 0.46.1.

* Update _helpers.tpl to avoid nested custom resource label (temporalio#576)

* Update Chart to 0.46.2.

* fix identation

---------

Signed-off-by: Tihomir Surdilovic <tihomir@temporal.io>
Signed-off-by: Valentin Zayash <valioozz@gmail.com>
Signed-off-by: David Calvert <david@0xdc.me>
Signed-off-by: André Bauer <andre.bauer@staffbase.com>
Co-authored-by: Tihomir Surdilovic <tihomir@temporal.io>
Co-authored-by: Rob Holland <rob.holland@gmail.com>
Co-authored-by: Temporal Data <commander-data@temporal.io>
Co-authored-by: Kshitij Tulsyan <ktulsyan1990@gmail.com>
Co-authored-by: Kshitij <kshitij.tulsyan@observe.ai>
Co-authored-by: Jingyu <56581242+washanhanzi@users.noreply.github.com>
Co-authored-by: Theo REY <account@reyth.dev>
Co-authored-by: Alex Shtin <alex@shtin.com>
Co-authored-by: Rob Holland <rob@temporal.io>
Co-authored-by: Punit Kulal <punitkulal1996@gmail.com>
Co-authored-by: Gerardo Enrique Mora Salazar <gerardo@ibm.com>
Co-authored-by: Giovanny Gutiérrez <giovanny.gutierrez@commure.com>
Co-authored-by: vogre <334187+vogre@users.noreply.github.com>
Co-authored-by: Valentin Zayash <VLZZZ@users.noreply.github.com>
Co-authored-by: Chris Taylor <taylor.cj@gmail.com>
Co-authored-by: Grzegorz Kołakowski <grzegorz8@gmail.com>
Co-authored-by: Prathyush PV <prathyushpv@gmail.com>
Co-authored-by: sringel <903498+sringel@users.noreply.github.com>
Co-authored-by: Alex Shtin <alex@temporal.io>
Co-authored-by: David Calvert <david@0xdc.me>
Co-authored-by: Roey Berman <roey@temporal.io>
Co-authored-by: Rahul Kumar <rahulcomp24@gmail.com>
Co-authored-by: Alex Garnett <axfelix@gmail.com>
Co-authored-by: Sahil Vazirani <sahilvv@gmail.com>
Co-authored-by: Quinn <116631861+qs-synth@users.noreply.github.com>
Co-authored-by: Kristian Nordman <kristian@limber.no>
Co-authored-by: Alex Tideman <alex.tideman@gmail.com>
Co-authored-by: Csaba Tűz <124735422+csabatuz-chess@users.noreply.github.com>
Co-authored-by: Manan Mangal <mananmangal@gmail.com>
Co-authored-by: Manan Mangal <mmangal@paloaltonetworks.com>
Co-authored-by: André Bauer <monotek@users.noreply.github.com>
@grzegorz8
Copy link
Contributor Author

Does this additionalVolumeMount nest in: server.config.additionalVolumes

Like what the original post says?

Or nested under server.additionalVolumes as per: https://github.com/temporalio/helm-charts/pull/411/files#diff-5f8eb04d49dd7caffd33da659dd7bae84435c114a6255319b2965dcf6a2536abR254

The description is wrong, the code is right. Sorry for confusion.

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

Successfully merging this pull request may close these issues.