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

feat(helm): generate random secrets when possible #3327

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aledegano
Copy link
Contributor

@aledegano aledegano commented Oct 27, 2023

Generates random secrets on deploy whenever possible.

@aledegano aledegano temporarily deployed to ci-renku-3327 October 27, 2023 08:36 — with GitHub Actions Inactive
@rokroskar
Copy link
Member

Doesn't this approach lead to things breaking when you redeploy? For example, the Jena password, redis password etc. Or can those be changed on-the-fly?

@rokroskar rokroskar changed the title feat(helm): Generate all secrets that can be random when deploying th… feat(helm): generate random secrets when possible Oct 31, 2023
@rokroskar rokroskar temporarily deployed to ci-renku-3327 October 31, 2023 17:04 — with GitHub Actions Inactive
@aledegano
Copy link
Contributor Author

Doesn't this approach lead to things breaking when you redeploy? For example, the Jena password, redis password etc. Or can those be changed on-the-fly?

There might be a problem if you keep the PVs BUT not the secrets, then if you also have not kept track of the secrets generated you are screwed indeed. But this is already the case for many secrets in Renku.

On the other hand if you don't delete the secrets you'll be fine as the chart reads them.

This feature is meant for CI and dev deployments, admins are still supposed to define all the secrets in the values.

@rokroskar
Copy link
Member

There might be a problem if you keep the PVs BUT not the secrets, then if you also have not kept track of the secrets generated you are screwed indeed. But this is already the case for many secrets in Renku.

On the other hand if you don't delete the secrets you'll be fine as the chart reads them.

This feature is meant for CI and dev deployments, admins are still supposed to define all the secrets in the values.

We had some mechanism with hooks that allowed you to do this and not overwrite the secrets on redeploy. I'm not sure if that is still being used. I believe @Panaetius (and or @olevski ?) implemented that at some point.

The problem of overwriting secrets for dev/CI deployments makes it worse imho because that's exactly where you are constantly redeploying. So the strategy would need to be to delete the deployment between iterations which is not great.

@olevski
Copy link
Member

olevski commented Nov 2, 2023

This is on my to-do list to review. I just wont be able to get it today. There is a way to simply never delete the auto-generated secrets. That is what we do already in most cases when we autogenerate stuff.

@olevski
Copy link
Member

olevski commented Nov 2, 2023

@aledegano this is still a draft though? Are you thinking of pushing more changes? Should I wait for more code/changes before I review?

@aledegano
Copy link
Contributor Author

@aledegano this is still a draft though? Are you thinking of pushing more changes? Should I wait for more code/changes before I review?

Tasko, yes this is a draft as I need to test it more and I would prefer SwissDataScienceCenter/renku-graph#1768 to be done and integrated.
That said, if you could quickly peek at how the secrets are read through the Helm chart to avoid changing them (which is what I believe we do now for some of the secrets), I would be grateful.

@aledegano aledegano deployed to ci-renku-3327 November 2, 2023 13:28 — with GitHub Actions Active
@rokroskar
Copy link
Member

@aledegano do you plan on picking this up again?

@aledegano
Copy link
Contributor Author

@aledegano do you plan on picking this up again?

Yes, I still want to finish it and get it merged...

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.

3 participants