-
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
🐛 Fix adoption KubeadmControlPlane owned secrets #7592
🐛 Fix adoption KubeadmControlPlane owned secrets #7592
Conversation
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
Outdated
Show resolved
Hide resolved
168da4c
to
80ef10c
Compare
/cherry-pick release-1.2 |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-1.3 |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-cluster-api-e2e-full-main |
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
Outdated
Show resolved
Hide resolved
80ef10c
to
0e81a0d
Compare
/test pull-cluster-api-e2e-full-main |
A couple of questions on this one to close out:
|
As far as I can tell To be honest, not sure what we want to do here. When they are generated by us it definitely makes sense to have a controller ownerRef. If some other controller is generating them we shouldn't set a controller ownerRef (should we even set an ownerRef at all / do we own a customer generated secret / do we want to delete it on KCP deletion?). I wonder how we can distinguish the cases though. After ownerRefs are reset we can't do it by ownerRef. // ClusterSecretType defines the type of secret created by core components.
ClusterSecretType corev1.SecretType = "cluster.x-k8s.io/secret" //nolint:gosec We set it when generating the certs, it's not part of the doc (https://cluster-api.sigs.k8s.io/tasks/certs/using-custom-certificates.html) and the godoc comment explicitly states "secret created by core components". As far as I can tell "user-supplied" kubeconfig is not a supported scenario and it also doesn't make a lot of sense. We documented how to provide "custom certificates" (https://cluster-api.sigs.k8s.io/tasks/certs/using-custom-certificates.html) but there is no mention of how to use a custom kubeconfig. |
0e81a0d
to
352d663
Compare
2d5db42
to
da77e2e
Compare
/retest |
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.
change lgtm to me pending last round of nits + the question about code coverage.
WRT to code coverage, it will be enough for me to have a unit test around adoptKubeconfigSecret (assuming this will be faster than testing all the use cases in TestReconcileKubeconfigSecretDoesNotAdoptsUserSecrets, but I'll defer this to you)
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
Outdated
Show resolved
Hide resolved
bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
Outdated
Show resolved
Hide resolved
d413a95
to
22b7185
Compare
Changes so far lgtm from my side. Feel free to merge independent of another review from my side. |
396250d
to
1bcd318
Compare
Thank you! /lgtm /assign @fabriziopandini |
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
1bcd318
to
f47c3d3
Compare
Last change added comments and one more case to the kubeadmconfig secret tests. |
Looks good as well /lgtm |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
@sbueringer: #7592 failed to apply on top of branch "release-1.2":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@sbueringer: new pull request created: #7659 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Signed-off-by: killianmuldoon kmuldoon@vmware.com
Ensure that KubeadmControlPlane attempts to adopt secrets under its ownerReference on every reconcile. Part of this change ensures that KubeadmConfigController never creates secrets as long as a ControlPlaneReference is set in the Cluster.Spec.
Part of #6979 , #7575