-
Notifications
You must be signed in to change notification settings - Fork 962
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: enable owner references #2688
Conversation
…o/postgres-operator into mbegenau-feat-498-ownerReferences
great |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need OwnerReferences on logical backup jobs?
@@ -957,9 +970,15 @@ func (c *Cluster) updateSecret( | |||
userMap[userKey] = pwdUser | |||
} | |||
|
|||
if !reflect.DeepEqual(secret.ObjectMeta.OwnerReferences, generatedSecret.ObjectMeta.OwnerReferences) { | |||
updateSecret = true | |||
updateSecretMsg = fmt.Sprintf("secret %s owner references do not match the current ones", secretName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am wondering, shouldn't we concatenate all the update msgs instead of re-assigning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other cases are mutual exclusive to one another. This line here is the only case that might happen additionally. But given that enabling owner references is probably a one time operation, I don't think it's worth to add more logic.
👍 |
1 similar comment
👍 |
Thank you for this great work! |
This a follow up to #2199 with some greater changes. The most important part of owner references is the cascaded removal of child resources which bypasses the delete protection offered by our operator.
Quick reminder about delete protection: this checks pre-defined annotations before calling delete code but cannot block the actual kubectl delete call itself. This you could only achieve with a K8s admission controller which, for example, does the same annotation checks.
We use the latter at Zalando, but some folks out there might rely on the protection by the operator. Therefore, we should not enable owner references by default but make it configurable.
#2199 also lacked code to update existing resources once you start using owner references or vice versa remove them. Like with the recently added annotation inheritance, this produced many code extensions. I’ve also found some oversights from #2657 which I’ve fixed along the way. All owner references syncs are done with Update API calls, which requires extending the RBAC.
A new e2e test is added which tests syncing owner references in both directions and deleting the acid-test-cluster cascadingly which was previously done in the multi namespace test. Some unit test were extended, too.
closes #2199
fixes #498