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(2582): added cluster level delete secrets config #2750

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

Conversation

Yingrjimsch
Copy link

Here #2582 it has been discussed to add a config for deleting secrets globally on operator level. This is very nice and helps greatly.

In my case I have a setup where my operator contains the config delete_annotation_name_key: delete-clustername and as a safety config enable_secrets_deletion: false which is needed if I want to clear a postgres cluster completely but still need the secrets. This works like a charm but I have temporary walg cluster clones which can be applied to get some backups. This temporary walg clones should still delete their secrets post deletion therefore a config is needed on cluster level.
I implemented the config enable_secrets_deletion_key in a similar way as delete_annotation_name_key works. This allows me to override the operator wide enable_secrets_deletion configuratin per postgresql cluster. If it is not set, the operator decides whether the secrets should be deleted, if it is set to true the secrets are deleted nevertheless and if it is set to false the secrets will not be deleted, allowing for a more fine granular configuration.

I have checked several possible scenarios with the following results:

enable_secrets_deletion delete_annotation_name_key delete-clustername enable-secrets-deletion Keep Secrets?
- - - - NO
true - - - NO
false - - - YES
- delete-clustername acid-minimal-cluster - NO
- delete-clustername - - YES
true delete-clustername - - YES
false delete-clustername - - YES
true delete-clustername - true YES
false delete-clustername - false YES
true delete-clustername acid-minimal-cluster - NO
false delete-clustername acid-minimal-cluster - YES
true delete-clustername acid-minimal-cluster true NO
false delete-clustername acid-minimal-cluster false YES

@dmotte can you review this please?

Copy link
Contributor

@dmotte dmotte left a comment

Choose a reason for hiding this comment

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

In general it looks good to me @Yingrjimsch thank you so much for taking the time to do this!

Maybe it's also worth to document this new config option, WDYT? I think it should go into the docs/reference/cluster_manifest.md file.

Last thing: I think the pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go file needs to be regenerated after your changes. Could you please regenerate and commit it? Thanks!

2 Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this file has been committed by accident :)

Copy link
Author

@Yingrjimsch Yingrjimsch Sep 6, 2024

Choose a reason for hiding this comment

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

@dmotte thanks for reviewing. I added the documentation in docs/reference/operator_parameters.md because there all the other properties delete_annotation_name_key and your enable_secret_deletion are placed.
Can you tell me how to regenerate the pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go? I tried by running hack/update-codegen.sh but this did not update anything.

@FxKu I saw you have approved the request from @dmotte is it possible for you to review this as well? (Shouldn't take too much time ;)

Edit: The file 2 really was commited by accident I removed it and squashed my commits so it has a minimal footprint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you tell me how to regenerate the pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go ?

@Yingrjimsch honestly I'm not sure, but I think you just have to put this piece of code

	if in.EnableSecretsDeletionKey != nil {
		in, out := &in.EnableSecretsDeletionKey, &out.EnableSecretsDeletionKey
		*out = new(string)
		**out = **in
	}

right after the if in.EnableSecretsDeletion != nil { ... } block, i.e. right here:

https://github.com/Yingrjimsch/postgres-operator/blob/6fdf2c92fa71e1bf3d99deff35401fd93d736086/pkg/apis/acid.zalan.do/v1/zz_generated.deepcopy.go#L289-L290

I suggest you to simply do that for now, and then let's wait for other reviews 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you to simply do that for now

@Yingrjimsch I see that you reacted with a "thumbs up" but I cannot see the fix yet :) let me know if you need further explanation

Copy link
Author

Choose a reason for hiding this comment

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

I suggest you to simply do that for now

@Yingrjimsch I see that you reacted with a "thumbs up" but I cannot see the fix yet :) let me know if you need further explanation

@dmotte I've tried it with your suggested codeblock. The result is, that the build fails. I saw that the DeleteAnnotationNameKey is also missing in the zz_generated.deepcopy.go and due to the fact that the hack/update-codegen.sh has not done anything I thought I will leave it for now. If you know why it is not able to build I'm gladly adding it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Yingrjimsch ok, got it. In this case yeah maybe you're right and that's not needed. Thank you for the explanation 👍

@Yingrjimsch Yingrjimsch force-pushed the 2582-secrets-deletion-per-cluster branch from 05fc045 to 44337ad Compare September 6, 2024 06:27
chore: removed log messages

chore: removed test yaml

Revert "chore: removed test yaml"

This reverts commit f19110c.

chore: removed test yaml

chore: added docs

chore: remove accident file
@Yingrjimsch
Copy link
Author

@FxKu could you please review this "small" fix? I would really appreciate it 😄

@FxKu FxKu added the minor label Oct 10, 2024
@FxKu FxKu added this to the 1.14.0 milestone Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants