-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
⚠️ Remove the Secret's OwnerReferences before pivot #1411
⚠️ Remove the Secret's OwnerReferences before pivot #1411
Conversation
/cc @vincepri |
cmd/clusterctl/phases/pivot.go
Outdated
// Remove owner reference. | ||
secret.SetOwnerReferences(nil) | ||
// Remove owner reference only if they're part of our known APIGroup | ||
for _, ownerRef := range secret.OwnerReferences { |
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.
If we remove the ownerReferences here, then the Secrets will be orphaned if the Cluster
is deleted post pivot, since the Secrets are not adopted by the Cluster.
I'm wondering if we should also keep track of which secrets had ownerReferences, and set the proper ownerReferences after the Cluster
has been migrated? @vincepri thoughts?
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.
Maybe we can do that instead, re-setting the owner references after being created, because I don't think they'll be actually reconciled after they're created. @tahsinrahman Do you think you could tackle that as part of the PR?
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.
sure, i'll try!
b91c372
to
409ba25
Compare
409ba25
to
18a5daa
Compare
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.
/approve
/assign @detiber
The last change seems to remove only UIDs, which it should be ok to create the secrets with the same owners as before given that these are pivoted last
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tahsinrahman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
This breaks pivot, which seemed to work just a few days ago:
EDIT in this case, the secret is owned by:
which, in turn, is owned by the first control plane machine of the source cluster:
|
@jdef would you mind filing an issue? |
// Remove owner reference. | ||
secret.SetOwnerReferences(nil) | ||
// remove the UID from ownerReferences as it will be different across clusters | ||
for i := 0; i < len(secret.OwnerReferences); i++ { |
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.
So I think this fails API validation here, because an ownerRef can't have an empty UID.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1404