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

patch: enable existing secrets + minor ingress fixes #34

Merged
merged 3 commits into from
Mar 27, 2024
Merged

patch: enable existing secrets + minor ingress fixes #34

merged 3 commits into from
Mar 27, 2024

Conversation

drew-viles
Copy link
Contributor

This introduces the option for an existing secret to be supplied with the credentials for postgres, smtp, redis and s3. By allowing users to roll their own secrets for these values, we improve their security as they don't have to hard code any values into their supplied values.yaml file.

The presumption is that if a user is supplying some credentials via a secret then all of the listed ones should be supplied.

This also fixes a few issues with the ingress resources around invalid values references and how the secretName is defined.

This introduces the option for an existing secret to be supplied with the credentials for postgres, smtp, redis and s3.
By allowing users to roll their own secrets for these values, we improve their security as they don't have to hard code any values into their supplied `values.yaml` file.

The presumption is that if a user is supplying some credentials via a secret then all of the listed ones should be supplied.
The ingress manifests had some invalid references and a duplicate secretName for the tls which has now been corrected.
@drew-viles
Copy link
Contributor Author

Just saw this 🤦
#16

I'll close this

@drew-viles drew-viles closed this Mar 26, 2024
@jdenquin
Copy link
Contributor

@drew-viles you can keep it open, #16 have some breaking changes

@drew-viles drew-viles reopened this Mar 26, 2024
@drew-viles
Copy link
Contributor Author

aaah thanks very much. I should have checked there first and this is just the first steps into lago I've taken (I was given it today to deploy into our infra).

I'll keep trying to help out where I can though as I realise it's fairly early days (relatively speaking) for the chart!

@drew-viles drew-viles changed the title patch: enable existing secrets patch: enable existing secrets + minor ingress fixes Mar 26, 2024
Copy link
Contributor

@jdenquin jdenquin left a comment

Choose a reason for hiding this comment

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

Looks good to me, did you test it with both options or should I do a bunch of tests on it?

@drew-viles
Copy link
Contributor Author

I ran a helm template command and reviewed the results both with and without the exstingSecret value but have only deployed using an external postgres/redis so far as I was in a fix to get that up and running yesterday. Happy to test this morning without it to ensure it works that way too.

Give me a few minutes and I'll confirm for you!

@drew-viles
Copy link
Contributor Author

So it works fine but only if I provide this value:
https://github.com/getlago/lago-helm-charts/blob/main/values.yaml#L73C1-L73C19

However, that value also has a typo in it (capitalised c) so I'll update that and get it pushed along with an update to the README about how it's required - unless we're happy to presume it as we do with the migrate job.
https://github.com/getlago/lago-helm-charts/blob/main/templates/migrate-job.yaml#L20

It seems to stem from redis, for example, being enabled which in turn requires that value.
https://github.com/getlago/lago-helm-charts/blob/main/templates/worker-deployment.yaml#L24-L32

Let me know your thoughts and I'll make any changes.

@jdenquin
Copy link
Contributor

oh yes we should use kubectlVersion for postgres too

@drew-viles
Copy link
Contributor Author

ok cool. I've got my standup shortly so once finished I'll take a look at those changes.

@drew-viles
Copy link
Contributor Author

ok I've pushed those changes now. I've adjusted how the kubectlVersion is used. Essentially it will infer it from the cluster version if it's not supplied.
Let me know if you're happy with that approach.

All tested again too in terms of creating the release, creating an account and interacting with elements of the app.

As I say I picked this up for the first time yesterday so if there are other things you can think of that need testing let me know.

@jdenquin jdenquin merged commit 69db374 into getlago:main Mar 27, 2024
1 check passed
@drew-viles drew-viles deleted the existing-secrets branch March 27, 2024 09:46
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.

2 participants