Make secret fields not immutable anymore #1355
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: StopMotionCuber The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @StopMotionCuber. Thanks for your PR. I'm waiting for a pulp member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
To allow for migrations, leveraging different secrets for the same pulp instance, fields have been made mutable Closes pulp#1343
StopMotionCuber
force-pushed
the
immutable-fields-squashed
branch
from
350b3e7
to
aa893a3
Compare
|
Just as a side-note: here a non-squashed version of the PR. I'm not sure about the CI failures, whether these are just flaky or whether there is an actual error. Logs are not that great for the failures |
| @@ -183,13 +183,6 @@ func checkImmutableFields(ctx context.Context, r *RepoManagerReconciler, pulp *r | |||
| // Checking immutable fields update | |||
| immutableFields := []immutableField{ | |||
| {FieldName: "DeploymentType", FieldPath: repomanagerpulpprojectorgv1beta2.PulpSpec{}}, | |||
| {FieldName: "ObjectStorageAzureSecret", FieldPath: repomanagerpulpprojectorgv1beta2.PulpSpec{}}, | |||
| {FieldName: "ObjectStorageS3Secret", FieldPath: repomanagerpulpprojectorgv1beta2.PulpSpec{}}, | |||
| {FieldName: "DBFieldsEncryptionSecret", FieldPath: repomanagerpulpprojectorgv1beta2.PulpSpec{}}, | |||
There was a problem hiding this comment.
Can we keep DBFieldsEncryptionSecret immutable for now? If a user changes it without properly following the key rotation (https://pulpproject.org/pulpcore/docs/admin/reference/settings/#db_encryption_key) then Pulp will become unusable and you will have to drop your database and start over. Until we can add this fancy rotation logic to the operator I think it is best to leave it immutable.
There was a problem hiding this comment.
That would be possible. However admins can already shoot themselves in the foot when changing the value within the secret. And if someone wants to change to another secret, referencing the same secret value, they could not do so.
So IMO keeping it immutable would just be snake oil
There was a problem hiding this comment.
We want there to be something to tell the user "PLEASE DON'T CHANGE THIS VALUE".
There was a problem hiding this comment.
That makes sense. I would think of adding it here with a warning box as well as changing the docs on the parameter for kubebuilder generation (and work with some caps in the latter case).
I would go with something along the lines of "DON'T CHANGE THIS VALUE UNLESS YOU KNOW WHAT YOU ARE DOING".
Is that reasonable/sufficient or would you still like to keep immutability on that?
This lifts the restriction of fields being immutable in many places without a good reason for that behavior.
The
DeploymentTypeis still mutable, as I didn't figure out completely what that was about and assume that changing that betweenpulpandgalaxyhas deeper implications, rendering the instance useless (in that case newly creating an instance is indeed the way to go). As suggested by @git-hyagi here, I've added a transition hook which should return a failure instead of silently rolling back the value on clusters where supported.If that assumption is false and changing between
pulpandgalaxyis possible without bad implications, I could remove the whole rollback machinery.I've tested the changes on a local k3s cluster with following scenarios:
settings.pyafterwards had the values of the second S3 secret included and the deployments were restarted.admin_password_secretwith a new one (where no secret existed). A new secret with new credentials was created and I was able to login with these credentials, while not being able to login with the former credentials. Former credentials were not deleted, but I guess that's fine.I didn't test all of the secrets, as I do not have any automation for that, but I think all of them are in the same code path, updating the
pulp-server:settings.py, which should all trigger proper reconciliation and restart of the services.Closes #1343