re-apply secret OwnerRef on pivot with correct UID #1435
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
the
cncf-cla: yes
|
/hold (need to do some validation from CAPV first) |
k8s-ci-robot
added
do-not-merge/hold
andrewsykim
changed the title
vincepri
approved these changes
Sep 24, 2019
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, 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 |
k8s-ci-robot
added
the
approved
ncdc
reviewed
Sep 24, 2019
cmd/clusterctl/phases/pivot.go
Outdated
| for i := 0; i < len(secret.OwnerReferences); i++ { | ||
| secret.OwnerReferences[i].UID = "" | ||
| } | ||
| // Clear the owner reference since it's UID is different per cluster |
There was a problem hiding this comment.
Suggested change
| // Clear the owner reference since it's UID is different per cluster | |
| // Clear the owner reference since its UID is different per cluster |
andrewsykim
force-pushed
the
fix-secret-pivot
branch
from
f9fb83a
to
23840d6
Compare
|
lgtm based on a quick review |
|
Did a quick test on CAPV, seeing the owner ref applied correctly kind: Secret
metadata:
creationTimestamp: "2019-09-24T15:20:21Z"
name: management-cluster-kubeconfig
namespace: default
ownerReferences:
- apiVersion: cluster.x-k8s.io/v1alpha2
kind: Cluster
name: management-cluster
uid: d2a5e541-dede-11e9-8a41-005056b01c90
resourceVersion: "1122"
selfLink: /api/v1/namespaces/default/secrets/management-cluster-kubeconfig
uid: d2b4d231-dede-11e9-8a41-005056b01c90
type: Opaque |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
@voor FYI, GitHub will only automatically close issues on merge if they're listed in the issue's description. It doesn't evaluate |
ncdc
reviewed
Sep 24, 2019
Signed-off-by: Andrew Sy Kim <kiman@vmware.com>
andrewsykim
force-pushed
the
fix-secret-pivot
branch
from
23840d6
to
60d8231
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Andrew Sy Kim kiman@vmware.com
What this PR does / why we need it:
In #1411, we started to only reset the secret owner ref's UID for clusters, which fails API validation (see here). In order to pass validation, we should create the secret without the owner ref and re-apply it afterwards using the target cluster's UID.
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 #1428