fix(2582): added cluster level delete secrets config #2750
Conversation
Yingrjimsch
requested review from
sdudoladov,
Jan-M,
FxKu,
jopadi,
idanovinda,
hughcapet and
macedigital
as code owners
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I guess this file has been committed by accident :)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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:
I suggest you to simply do that for now, and then let's wait for other reviews 😉
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@Yingrjimsch ok, got it. In this case yeah maybe you're right and that's not needed. Thank you for the explanation 👍
Yingrjimsch
force-pushed
the
2582-secrets-deletion-per-cluster
branch
from
05fc045 to
44337ad
Compare
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
force-pushed
the
2582-secrets-deletion-per-cluster
branch
from
6d57db7 to
6fdf2c9
Compare
|
@FxKu could you please review this "small" fix? I would really appreciate it 😄 |
| @@ -66,6 +66,7 @@ type Resources struct { | |||
| MaxInstances int32 `name:"max_instances" default:"-1"` | |||
| MinInstances int32 `name:"min_instances" default:"-1"` | |||
| IgnoreInstanceLimitsAnnotationKey string `name:"ignore_instance_limits_annotation_key"` | |||
| EnableSecretsDeletionKey string `name:"enable_secrets_deletion_key"` | |||
There was a problem hiding this comment.
Can you rename the to IgnoreSecretDeletionAnnotationKey?
|
I haven't checked yet, if the option works vice versa, but this would be my preferred implementation. So that you can have: Therefore, I think naming this option "ignore..." is more suitable and easier to understand. |
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-clusternameand as a safety configenable_secrets_deletion: falsewhich 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_keyin a similar way asdelete_annotation_name_keyworks. This allows me to override the operator wideenable_secrets_deletionconfiguratin per postgresql cluster. If it is not set, the operator decides whether the secrets should be deleted, if it is set totruethe secrets are deleted nevertheless and if it is set tofalsethe secrets will not be deleted, allowing for a more fine granular configuration.I have checked several possible scenarios with the following results:
enable_secrets_deletiondelete_annotation_name_keydelete-clusternameenable-secrets-deletion@dmotte can you review this please?