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

fix postgres and redis existing secrets and use secret for environment + add service account #47

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

davidspek
Copy link
Contributor

@davidspek davidspek commented Jan 5, 2022

Solves: #46

The values.yaml indicates it should be possible to use an existing secret for the Postgres and Redis password. However, this functionality was missing in the templates. This PR add the ability to use an existing secret for the Postgres and Redis passwords.

Along with that, this PR replaces the configmap used to set the environment variables with a secret, since there were many credentials being set in the environment the configmap which isn't a good practice. Kubernetes variable replacement is used to overcome the issue of the chatwoot.redis.url template not having the redis password when an existing secret is used.

Lastly, the missing service account was added to the job, web service and worker deployments. I can confirm IRSA works for access to AWS S3 when adding the annotation to the service account with the current chart.

@vishnu-narayanan
Copy link
Member

@davidspek Thank you for the PR. Let me test and get back to you.

@davidspek davidspek force-pushed the fix-secrets branch 3 times, most recently from f9b74b6 to 7799a9c Compare January 12, 2022 13:53
@davidspek
Copy link
Contributor Author

Just rebased the PR to remove the conflict.

Copy link
Member

@vishnu-narayanan vishnu-narayanan left a comment

Choose a reason for hiding this comment

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

@davidspek Thank you for the awesome PR. Tested this as a fresh install and as an upgrade to an existing install. It's working fine.

This seems to have an issue when $(REDIS_PASSWORD) substitution comes into the picture viz when using external Redis with or without TLS.

External redis without TLS

$ helm install --debug --dry-run --generate-name . > /tmp/test.yaml
$ grep "REDIS_PASSWORD" /tmp/test.yaml | awk '{ $1=""; print}' | tr -d '"' | base64 -D
redis21212
$ grep "REDIS_URL" /tmp/test.yaml | awk '{ $1=""; print}' | tr -d '"' | base64 -D
redis://:$(REDIS_PASSWORD)@redis:6379%

External redis with TLS

$ helm install --debug --dry-run --generate-name . > /tmp/test.yaml
$ grep "REDIS_PASSWORD" /tmp/test.yaml | awk '{ $1=""; print}' | tr -d '"' | base64 -D
redis21212
$ grep "REDIS_URL" /tmp/test.yaml | awk '{ $1=""; print}' | tr -d '"' | base64 -D
rediss://:$(REDIS_PASSWORD)@redis:6379%

charts/chatwoot/templates/_helpers.tpl Show resolved Hide resolved
@vishnu-narayanan
Copy link
Member

Reminder

Copy link
Member

@vishnu-narayanan vishnu-narayanan left a comment

Choose a reason for hiding this comment

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

@davidspek Could you please pull the changes from develop and also increment the chart version to 0.8.0?

@davidspek
Copy link
Contributor Author

@vishnu-narayanan sorry for the delay here, I've been very busy. I just rebased the PR on the latest main branch.

@vishnu-narayanan
Copy link
Member

@davidspek No worries. I was not able to push to this branch since maintainer edits are disabled.

Since this is changing the chart behaviour(dropping configmaps in favour of secrets), could you please increment the chart version to 0.8.0?

@davidspek
Copy link
Contributor Author

@vishnu-narayanan I just bumped the chart version to 0.8.0.

@vishnu-narayanan vishnu-narayanan merged commit a0b12d7 into chatwoot:main Feb 16, 2022
@vishnu-narayanan
Copy link
Member

Thank you for the PR @davidspek @michaeljguarino 🙇

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.

None yet

3 participants