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

[installer]: deprecate static message bus password and replace with secret #15905

Merged
merged 3 commits into from
Jan 23, 2023

Conversation

mrsimonemms
Copy link
Contributor

@mrsimonemms mrsimonemms commented Jan 19, 2023

Description

The experimental config had a staticMessagebusPassword key which violates our rules on configuration, namely "sensitive data must be stored in a secret".

To that end, this deprecates the old behaviour and replaces with a new messageBus object in the config. I took the decision to put it in a new object because, as a dependency, it's plausible that we'll need to have additional configuration options in the future (similar to registry/db/storage).

The complexity with this ticket is that RabbitMQ load definitions secret which requires the password. As there's no documentation on how to achieve that, the load definition secret stores the password as %PASSWORD%, I've created an init container which reads the load definition and password secrets and replaces %PASSWORD% with the actual password and then saves it to the emptyDir which is what's actually used by the RabbitMQ resource.

If no password is given or the old experimental config is used, this stores the given password in a secret so RabbitMQ can read it.

There is no MapValue for the experimental config that can be done because it's too complex to create a secret for a time-limited deprecated function. The deprecated message doesn't display the password because I don't want to write credentials into logs, even if the password is easily readable.

Related Issue(s)

Fixes #15899

How to test

Install a cluster with the following config. To check that the new secret has been promulgated, run kubectl exec -it -n gitpod deployments/server -- env | grep MESSAGEBUS_PASSWORD

1. No password specified (default)

No special config required

2. Deprecated experimental config

experimental:
  common:
    staticMessagebusPassword: some-password

3. New password secret

kubectl create secret generic -n gitpod --from-literal=rabbitmq-password=secret-password message-bus-password
messageBus:
  credentials:
    kind: secret
    name: message-bus-password

Release Notes

[installer]: deprecate static message bus password and replace with secret 

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch from 8318571 to 5b4275f Compare January 20, 2023 09:55
@roboquat roboquat added size/XL and removed size/L labels Jan 20, 2023
@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch 2 times, most recently from 2845cd9 to 94492fa Compare January 20, 2023 14:38
@roboquat roboquat added size/XXL and removed size/XL labels Jan 20, 2023
@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch 4 times, most recently from 0e6f3b4 to 1b5dc78 Compare January 23, 2023 10:19
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sje-deprecate-messagebus-pass.8 because the annotations in the pull request description changed
(with .werft/ from main)

@mrsimonemms mrsimonemms changed the title Sje/deprecate messagebus pass [installer]: deprecate static message bus password and replace with secret Jan 23, 2023
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sje-deprecate-messagebus-pass.9 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sje-deprecate-messagebus-pass.10 because the annotations in the pull request description changed
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sje-deprecate-messagebus-pass.11 because the annotations in the pull request description changed
(with .werft/ from main)

@mrsimonemms mrsimonemms marked this pull request as ready for review January 23, 2023 11:37
@mrsimonemms mrsimonemms requested review from a team January 23, 2023 11:37
@github-actions github-actions bot added team: SID team: webapp Issue belongs to the WebApp team labels Jan 23, 2023
@mrsimonemms
Copy link
Contributor Author

/hold so I can rebase the golden files before final merge

@mrsimonemms
Copy link
Contributor Author

mrsimonemms commented Jan 23, 2023

/werft run

👍 started the job as gitpod-build-sje-deprecate-messagebus-pass.13
(with .werft/ from main)

@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch from a707f57 to f1b106d Compare January 23, 2023 12:09
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM apart from the above comment! Init container does seem to be a bit complicated but seems to be the only way without changing too much logic. Hoping we can move to using extraConfiguration option in the future so that we don't have to pass the password at two different places 🤔

/hold Just in case

@geropl
Copy link
Member

geropl commented Jan 23, 2023

I tried to review the code, but it was a bit too large for me to judge with confidence.
Now looking into comparing the rendered diff... 👀

@mrsimonemms
Copy link
Contributor Author

@geropl yes, it's a bigger than I'd have hoped. Happy to sync with you if you need it

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Reviewed the diff, and chatted with Simon.
LGTM!

Simon Emms added 2 commits January 23, 2023 13:34
Previously, this was defined as a Helm secret inline. Now, this uses the
way that the Installer supports by default.
@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch from f1b106d to bd70425 Compare January 23, 2023 13:35
@mrsimonemms
Copy link
Contributor Author

/unhold

Rebased from main and updated golden files

@mrsimonemms
Copy link
Contributor Author

/hold

@mrsimonemms mrsimonemms force-pushed the sje/deprecate-messagebus-pass branch from bd70425 to a71f403 Compare January 23, 2023 13:40
@mrsimonemms
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 30a33f7 into main Jan 23, 2023
@roboquat roboquat deleted the sje/deprecate-messagebus-pass branch January 23, 2023 13:45
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production release-note size/XXL team: SID team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move StaticMessagebusPassword out of experimental
4 participants