Skip to content

Commit f47c3d3

Browse files
killianmuldoonkillianmuldoon
committed
Fix KubeadmControlPlane secrets should always be adopted
Signed-off-by: killianmuldoon <[email protected]>
1 parent 844eff2 commit f47c3d3

File tree

6 files changed

+402
-30
lines changed

6 files changed

+402
-30
lines changed

‎api/v1beta1/common_types.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ const (
107107
MachineSkipRemediationAnnotation = "cluster.x-k8s.io/skip-remediation"
108108

109109
// ClusterSecretType defines the type of secret created by core components.
110+
// Note: This is used by core CAPI, CAPBK, and KCP to determine whether a secret is created by the controllers
111+
// themselves or supplied by the user (e.g. bring your own certificates).
110112
ClusterSecretType corev1.SecretType = "cluster.x-k8s.io/secret" //nolint:gosec
111113

112114
// InterruptibleLabel is the label used to mark the nodes that run on interruptible instances.

‎bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -437,12 +437,21 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex
437437
}
438438

439439
certificates := secret.NewCertificatesForInitialControlPlane(scope.Config.Spec.ClusterConfiguration)
440-
err = certificates.LookupOrGenerate(
441-
ctx,
442-
r.Client,
443-
util.ObjectKey(scope.Cluster),
444-
*metav1.NewControllerRef(scope.Config, bootstrapv1.GroupVersion.WithKind("KubeadmConfig")),
445-
)
440+
441+
// If the Cluster does not have a ControlPlane reference look up and generate the certificates.
442+
// Otherwise rely on certificates generated by the ControlPlane controller.
443+
// Note: A cluster does not have a ControlPlane reference when using standalone CP machines.
444+
if scope.Cluster.Spec.ControlPlaneRef == nil {
445+
err = certificates.LookupOrGenerate(
446+
ctx,
447+
r.Client,
448+
util.ObjectKey(scope.Cluster),
449+
*metav1.NewControllerRef(scope.Config, bootstrapv1.GroupVersion.WithKind("KubeadmConfig")))
450+
} else {
451+
err = certificates.Lookup(ctx,
452+
r.Client,
453+
util.ObjectKey(scope.Cluster))
454+
}
446455
if err != nil {
447456
conditions.MarkFalse(scope.Config, bootstrapv1.CertificatesAvailableCondition, bootstrapv1.CertificatesGenerationFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
448457
return ctrl.Result{}, err

‎controlplane/kubeadm/internal/controllers/controller.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,9 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
312312
err = r.adoptMachines(ctx, kcp, adoptableMachines, cluster)
313313
return ctrl.Result{}, err
314314
}
315+
if err := ensureCertificatesOwnerRef(ctx, r.Client, util.ObjectKey(cluster), certificates, *controllerRef); err != nil {
316+
return ctrl.Result{}, err
317+
}
315318

316319
ownedMachines := controlPlaneMachines.Filter(collections.OwnedMachines(kcp))
317320
if len(ownedMachines) != len(controlPlaneMachines) {
@@ -773,3 +776,33 @@ func (r *KubeadmControlPlaneReconciler) adoptOwnedSecrets(ctx context.Context, k
773776

774777
return nil
775778
}
779+
780+
// ensureCertificatesOwnerRef ensures an ownerReference to the owner is added on the Secrets holding certificates.
781+
func ensureCertificatesOwnerRef(ctx context.Context, ctrlclient client.Client, clusterKey client.ObjectKey, certificates secret.Certificates, owner metav1.OwnerReference) error {
782+
for _, c := range certificates {
783+
s := &corev1.Secret{}
784+
secretKey := client.ObjectKey{Namespace: clusterKey.Namespace, Name: secret.Name(clusterKey.Name, c.Purpose)}
785+
if err := ctrlclient.Get(ctx, secretKey, s); err != nil {
786+
return errors.Wrapf(err, "failed to get Secret %s", secretKey)
787+
}
788+
// If the Type doesn't match the type used for secrets created by core components, KCP included
789+
if s.Type != clusterv1.ClusterSecretType {
790+
continue
791+
}
792+
patchHelper, err := patch.NewHelper(s, ctrlclient)
793+
if err != nil {
794+
return errors.Wrapf(err, "failed to create patchHelper for Secret %s", secretKey)
795+
}
796+
797+
// Remove the current controller if one exists.
798+
if controller := metav1.GetControllerOf(s); controller != nil {
799+
s.SetOwnerReferences(util.RemoveOwnerRef(s.OwnerReferences, *controller))
800+
}
801+
802+
s.OwnerReferences = util.EnsureOwnerRef(s.OwnerReferences, owner)
803+
if err := patchHelper.Patch(ctx, s); err != nil {
804+
return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", secretKey, owner.String())
805+
}
806+
}
807+
return nil
808+
}

‎controlplane/kubeadm/internal/controllers/controller_test.go

Lines changed: 198 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,12 @@ package controllers
1818

1919
import (
2020
"context"
21+
"crypto/rand"
22+
"crypto/rsa"
23+
"crypto/x509"
24+
"crypto/x509/pkix"
2125
"fmt"
26+
"math/big"
2227
"sync"
2328
"testing"
2429
"time"
@@ -49,6 +54,7 @@ import (
4954
"sigs.k8s.io/cluster-api/feature"
5055
"sigs.k8s.io/cluster-api/internal/test/builder"
5156
"sigs.k8s.io/cluster-api/util"
57+
"sigs.k8s.io/cluster-api/util/certs"
5258
"sigs.k8s.io/cluster-api/util/collections"
5359
"sigs.k8s.io/cluster-api/util/conditions"
5460
"sigs.k8s.io/cluster-api/util/kubeconfig"
@@ -520,11 +526,12 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
520526
g.Expect(machine.GetAnnotations()).NotTo(HaveKey(clusterv1.TemplateClonedFromNameAnnotation))
521527
}
522528
})
529+
523530
t.Run("adopts v1alpha2 cluster secrets", func(t *testing.T) {
524531
g := NewWithT(t)
525532

526533
cluster, kcp, tmpl := createClusterWithControlPlane(metav1.NamespaceDefault)
527-
cluster.Spec.ControlPlaneEndpoint.Host = "bar"
534+
cluster.Spec.ControlPlaneEndpoint.Host = "validhost"
528535
cluster.Spec.ControlPlaneEndpoint.Port = 6443
529536
cluster.Status.InfrastructureReady = true
530537
kcp.Spec.Version = version
@@ -565,7 +572,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
565572
},
566573
}
567574

568-
// A simulcrum of the various Certificate and kubeconfig secrets
575+
// A simulacrum of the various Certificate and kubeconfig secrets
569576
// it's a little weird that this is one per KubeadmConfig rather than just whichever config was "first,"
570577
// but the intent is to ensure that the owner is changed regardless of which Machine we start with
571578
clusterSecret := &corev1.Secret{
@@ -749,6 +756,164 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
749756
})
750757
}
751758

759+
func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
760+
g := NewWithT(t)
761+
762+
cluster, kcp, tmpl := createClusterWithControlPlane(metav1.NamespaceDefault)
763+
cluster.Spec.ControlPlaneEndpoint.Host = "bar"
764+
cluster.Spec.ControlPlaneEndpoint.Port = 6443
765+
cluster.Status.InfrastructureReady = true
766+
kcp.Spec.Version = "v1.21.0"
767+
key, err := certs.NewPrivateKey()
768+
g.Expect(err).To(BeNil())
769+
crt, err := getTestCACert(key)
770+
g.Expect(err).To(BeNil())
771+
772+
fmc := &fakeManagementCluster{
773+
Machines: collections.Machines{},
774+
Workload: fakeWorkloadCluster{},
775+
}
776+
777+
clusterSecret := &corev1.Secret{
778+
// The Secret's Type is used by KCP to determine whether it is user-provided.
779+
// clusterv1.ClusterSecretType signals that the Secret is CAPI-provided.
780+
ObjectMeta: metav1.ObjectMeta{
781+
Namespace: cluster.Namespace,
782+
Name: "",
783+
Labels: map[string]string{
784+
"cluster.x-k8s.io/cluster-name": cluster.Name,
785+
"testing": "yes",
786+
},
787+
},
788+
Data: map[string][]byte{
789+
secret.TLSCrtDataName: certs.EncodeCertPEM(crt),
790+
secret.TLSKeyDataName: certs.EncodePrivateKeyPEM(key),
791+
},
792+
}
793+
794+
t.Run("add KCP owner for secrets with no controller reference", func(t *testing.T) {
795+
objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
796+
for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
797+
s := clusterSecret.DeepCopy()
798+
// Set the secret name to the purpose
799+
s.Name = secret.Name(cluster.Name, purpose)
800+
// Set the Secret Type to clusterv1.ClusterSecretType which signals this Secret was generated by CAPI.
801+
s.Type = clusterv1.ClusterSecretType
802+
803+
objs = append(objs, s)
804+
}
805+
806+
fakeClient := newFakeClient(objs...)
807+
fmc.Reader = fakeClient
808+
r := &KubeadmControlPlaneReconciler{
809+
Client: fakeClient,
810+
APIReader: fakeClient,
811+
managementCluster: fmc,
812+
managementClusterUncached: fmc,
813+
}
814+
815+
_, err := r.reconcile(ctx, cluster, kcp)
816+
g.Expect(err).To(BeNil())
817+
818+
secrets := &corev1.SecretList{}
819+
g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
820+
for _, secret := range secrets.Items {
821+
g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))))
822+
}
823+
})
824+
825+
t.Run("replace non-KCP controller with KCP controller reference", func(t *testing.T) {
826+
objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
827+
// A simulacrum of the various Certificate and kubeconfig secrets
828+
// it's a little weird that this is one per KubeadmConfig rather than just whichever config was "first,"
829+
// but the intent is to ensure that the owner is changed regardless of which Machine we start with
830+
for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
831+
s := clusterSecret.DeepCopy()
832+
// Set the secret name to the purpose
833+
s.Name = secret.Name(cluster.Name, purpose)
834+
// Set the Secret Type to clusterv1.ClusterSecretType which signals this Secret was generated by CAPI.
835+
s.Type = clusterv1.ClusterSecretType
836+
837+
// Set the a controller owner reference of an unknown type on the secret.
838+
s.SetOwnerReferences([]metav1.OwnerReference{
839+
{
840+
APIVersion: bootstrapv1.GroupVersion.String(),
841+
// KCP should take ownership of any Secret of the correct type linked to the Cluster.
842+
Kind: "OtherController",
843+
Name: "name",
844+
UID: "uid",
845+
Controller: pointer.Bool(true),
846+
},
847+
})
848+
objs = append(objs, s)
849+
}
850+
851+
fakeClient := newFakeClient(objs...)
852+
fmc.Reader = fakeClient
853+
r := &KubeadmControlPlaneReconciler{
854+
Client: fakeClient,
855+
APIReader: fakeClient,
856+
managementCluster: fmc,
857+
managementClusterUncached: fmc,
858+
}
859+
860+
_, err := r.reconcile(ctx, cluster, kcp)
861+
g.Expect(err).To(BeNil())
862+
863+
secrets := &corev1.SecretList{}
864+
g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
865+
for _, secret := range secrets.Items {
866+
g.Expect(secret.OwnerReferences).To(HaveLen(1))
867+
g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))))
868+
}
869+
})
870+
871+
t.Run("does not add owner reference to user-provided secrets", func(t *testing.T) {
872+
g := NewWithT(t)
873+
objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
874+
for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
875+
s := clusterSecret.DeepCopy()
876+
// Set the secret name to the purpose
877+
s.Name = secret.Name(cluster.Name, purpose)
878+
// Set the Secret Type to any type which signals this Secret was is user-provided.
879+
s.Type = corev1.SecretTypeOpaque
880+
// Set the a controller owner reference of an unknown type on the secret.
881+
s.SetOwnerReferences([]metav1.OwnerReference{
882+
{
883+
APIVersion: bootstrapv1.GroupVersion.String(),
884+
// This owner reference to a different controller should be preserved.
885+
Kind: "OtherController",
886+
Name: kcp.Name,
887+
UID: kcp.UID,
888+
Controller: pointer.Bool(true),
889+
BlockOwnerDeletion: pointer.Bool(true),
890+
},
891+
})
892+
893+
objs = append(objs, s)
894+
}
895+
896+
fakeClient := newFakeClient(objs...)
897+
fmc.Reader = fakeClient
898+
r := &KubeadmControlPlaneReconciler{
899+
Client: fakeClient,
900+
APIReader: fakeClient,
901+
managementCluster: fmc,
902+
managementClusterUncached: fmc,
903+
}
904+
905+
_, err := r.reconcile(ctx, cluster, kcp)
906+
g.Expect(err).To(BeNil())
907+
908+
secrets := &corev1.SecretList{}
909+
g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
910+
for _, secret := range secrets.Items {
911+
g.Expect(secret.OwnerReferences).To(HaveLen(1))
912+
g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, bootstrapv1.GroupVersion.WithKind("OtherController"))))
913+
}
914+
})
915+
}
916+
752917
func TestReconcileCertificateExpiries(t *testing.T) {
753918
g := NewWithT(t)
754919

@@ -1769,3 +1934,34 @@ func newCluster(namespacedName *types.NamespacedName) *clusterv1.Cluster {
17691934
},
17701935
}
17711936
}
1937+
1938+
func getTestCACert(key *rsa.PrivateKey) (*x509.Certificate, error) {
1939+
cfg := certs.Config{
1940+
CommonName: "kubernetes",
1941+
}
1942+
1943+
now := time.Now().UTC()
1944+
1945+
tmpl := x509.Certificate{
1946+
SerialNumber: new(big.Int).SetInt64(0),
1947+
Subject: pkix.Name{
1948+
CommonName: cfg.CommonName,
1949+
Organization: cfg.Organization,
1950+
},
1951+
NotBefore: now.Add(time.Minute * -5),
1952+
NotAfter: now.Add(time.Hour * 24), // 1 day
1953+
KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
1954+
MaxPathLenZero: true,
1955+
BasicConstraintsValid: true,
1956+
MaxPathLen: 0,
1957+
IsCA: true,
1958+
}
1959+
1960+
b, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, key.Public(), key)
1961+
if err != nil {
1962+
return nil, err
1963+
}
1964+
1965+
c, err := x509.ParseCertificate(b)
1966+
return c, err
1967+
}

‎controlplane/kubeadm/internal/controllers/helpers.go

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
7474
return ctrl.Result{}, errors.Wrap(err, "failed to retrieve kubeconfig Secret")
7575
}
7676

77-
// check if the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP;
78-
// if yes, adopt it.
79-
if util.IsOwnedByObject(configSecret, cluster) && !util.IsControlledBy(configSecret, kcp) {
80-
if err := r.adoptKubeconfigSecret(ctx, cluster, configSecret, controllerOwnerRef); err != nil {
81-
return ctrl.Result{}, err
82-
}
77+
if err := r.adoptKubeconfigSecret(ctx, cluster, configSecret, kcp); err != nil {
78+
return ctrl.Result{}, err
8379
}
8480

8581
// only do rotation on owned secrets
@@ -102,21 +98,45 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
10298
return ctrl.Result{}, nil
10399
}
104100

105-
func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, controllerOwnerRef metav1.OwnerReference) error {
101+
// Ensure the KubeadmConfigSecret has an owner reference to the control plane if it is not a user-provided secret.
102+
func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) error {
106103
log := ctrl.LoggerFrom(ctx)
107-
log.Info("Adopting KubeConfig secret created by v1alpha2 controllers", "Secret", klog.KObj(configSecret))
104+
controller := metav1.GetControllerOf(configSecret)
108105

106+
// If the Type doesn't match the CAPI-created secret type this is a no-op.
107+
if configSecret.Type != clusterv1.ClusterSecretType {
108+
return nil
109+
}
110+
// If the secret is already controlled by KCP this is a no-op.
111+
if controller != nil && controller.Kind == "KubeadmControlPlane" {
112+
return nil
113+
}
114+
log.Info("Adopting KubeConfig secret", "Secret", klog.KObj(configSecret))
109115
patch, err := patch.NewHelper(configSecret, r.Client)
110116
if err != nil {
111117
return errors.Wrap(err, "failed to create patch helper for the kubeconfig secret")
112118
}
113-
configSecret.OwnerReferences = util.RemoveOwnerRef(configSecret.OwnerReferences, metav1.OwnerReference{
114-
APIVersion: clusterv1.GroupVersion.String(),
115-
Kind: "Cluster",
116-
Name: cluster.Name,
117-
UID: cluster.UID,
118-
})
119-
configSecret.OwnerReferences = util.EnsureOwnerRef(configSecret.OwnerReferences, controllerOwnerRef)
119+
120+
// If the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP.
121+
// In this case remove the ownerReference to the Cluster.
122+
if util.IsOwnedByObject(configSecret, cluster) {
123+
configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, metav1.OwnerReference{
124+
APIVersion: clusterv1.GroupVersion.String(),
125+
Kind: "Cluster",
126+
Name: cluster.Name,
127+
UID: cluster.UID,
128+
}))
129+
}
130+
131+
// Remove the current controller if one exists.
132+
if controller != nil {
133+
configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, *controller))
134+
}
135+
136+
// Add the KubeadmControlPlane as the controller for this secret.
137+
configSecret.OwnerReferences = util.EnsureOwnerRef(configSecret.OwnerReferences,
138+
*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")))
139+
120140
if err := patch.Patch(ctx, configSecret); err != nil {
121141
return errors.Wrap(err, "failed to patch the kubeconfig secret")
122142
}

0 commit comments

Comments
 (0)