-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: improve PACT_BROKER_DATABASE_PASSWORD env var configuration #43
Conversation
@caiohasouza Thanks for the PR. Would you be able to at a description in the PR about what the problem was and what the fix does? |
@ChrisJBurns done, could you check please ? |
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
@ChrisJBurns done, could you check please? |
@ChrisJBurns i think that action erros is because the Postgres password is not setting: I need change something else? Regards |
@caiohasouza I believe it is because Bitnami SubChart will create a Secret relating to the Postgres information that would have been used in the reference to a secret, but now, we have added logic around it, so it doesn't get sourced. |
@ChrisJBurns Ok, no problem, thank you! |
@caiohasouza No problems, let me know when you've pushed up a fix and I can trigger the pipeline again 👍 |
@ChrisJBurns done (e15c0d5), could you run test again? |
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.
lgtm
@@ -93,10 +93,16 @@ spec: | |||
- name: PACT_BROKER_DATABASE_USERNAME | |||
value: {{ include "broker.databaseUser" . }} | |||
- name: PACT_BROKER_DATABASE_PASSWORD | |||
{{- if and .Values.postgresql.enabled .Values.postgresql.auth.password }} | |||
value: {{ .Values.postgresql.auth.password | quote }} | |||
{{- else if and .Values.externalDatabase.enabled .Values.externalDatabase.config.auth.password }} |
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.
the and
operator in this if statement prevents cloudsql postgres iam passwordless authentication from working -- as opposed to something like
{{- ternary .Values.postgresql.auth.password .Values.externalDatabase.config.auth.password .Values.postgresql.enabled | quote -}}
ex: using these values
externalDatabase:
enabled: true
config:
host: "127.0.0.1"
port: "5432"
adapter: postgres
databaseName: pact
auth:
username: "pact-broker-dev@dev-gcp-project.iam"
password: "" # passwordless cloudsql iam auth
results in an invalid secret object. using a random placeholder string results in this error
pact-broker-67966bbd86-5b4cj pact-broker ! Unable to load application: Sequel::DatabaseConnectionError: PG::ConnectionBad: connection to server at "127.0.0.1", port 5432 failed: Connection refused
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.
@joshuasimon-taulia Stepping outside of the chart for a moment, how does passwordless authentication work with the Pact Broker? It has to have a username and password to be able to connect to the Postgres database as far as I know
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.
if you use these values or something similar to run a cloudsql-proxy sidecar
extraContainers:
- name: cloudsql-proxy
image: gcr.io/cloudsql-docker/gce-proxy:1.22.0
command:
- /cloud_sql_proxy
- '-instances=my-project:my-zone:my-db-name=tcp:5432'
- '-enable_iam_login'
cloudsql-proxy authenticates to the db using the GCP IAM credentials bound to the pact broker pod's service account and exposes postgres on port 5432 or optionally as a socket on a shared volume mount. connecting with PACT_BROKER_DATABASE_PASSWORD=""
was then working with the changeset in https://github.com/pact-foundation/pact-broker-chart/pull/44/files#diff-9b40c5793a6365436c02520da0ecf44ee7712bc048ee54a4d117831dc0a855d8R96-R98. to be fair, i figured out a workaround where i set password
to an empty space to satisfy the if condition in the helm logic
externalDatabase:
config:
auth:
password: " " # passwordless cloudsql iam auth
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.
So I've tried your changes in #44 locally, and when I enable postgres
and also provide no password. I expect the Chart to generate a password and then use that in the deployment. But the password field is blank. This is the tricky situation where the subchart for postgres will generate the password for you and then use it in the deployment.
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.
---
# Source: pact-broker/charts/postgresql/templates/secrets.yaml
apiVersion: v1
kind: Secret
metadata:
name: release-name-postgresql
namespace: "default"
labels:
app.kubernetes.io/name: postgresql
helm.sh/chart: postgresql-11.9.13
app.kubernetes.io/instance: release-name
app.kubernetes.io/managed-by: Helm
type: Opaque
data:
postgres-password: "SUJqWWpzVHp0Uw=="
password: "Rk9xd1VjYkpzVQ=="
# We don't auto-generate LDAP password when it's not provided as we do for other passwords
- name: PACT_BROKER_DATABASE_PASSWORD
value: ""
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.
ah ok. ill just continue using my workaround
This PR solve 2 problems: