Fix KubeadmControlPlane secrets should always be adopted

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

api/v1beta1/common_types.go

@@ -107,6 +107,8 @@ const (
 	MachineSkipRemediationAnnotation = "cluster.x-k8s.io/skip-remediation"
 
 	// ClusterSecretType defines the type of secret created by core components.
+	// Note: This is used by core CAPI, CAPBK, and KCP to determine whether a secret is created by the controllers
+	// themselves or supplied by the user (e.g. bring your own certificates).
 	ClusterSecretType corev1.SecretType = "cluster.x-k8s.io/secret" //nolint:gosec
 
 	// InterruptibleLabel is the label used to mark the nodes that run on interruptible instances.

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -437,12 +437,21 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex
 	}
 
 	certificates := secret.NewCertificatesForInitialControlPlane(scope.Config.Spec.ClusterConfiguration)
-	err = certificates.LookupOrGenerate(
-		ctx,
-		r.Client,
-		util.ObjectKey(scope.Cluster),
-		*metav1.NewControllerRef(scope.Config, bootstrapv1.GroupVersion.WithKind("KubeadmConfig")),
-	)
+
+	// If the Cluster does not have a ControlPlane reference look up and generate the certificates.
+	// Otherwise rely on certificates generated by the ControlPlane controller.
+	// Note: A cluster does not have a ControlPlane reference when using standalone CP machines.
+	if scope.Cluster.Spec.ControlPlaneRef == nil {
+		err = certificates.LookupOrGenerate(
+			ctx,
+			r.Client,
+			util.ObjectKey(scope.Cluster),
+			*metav1.NewControllerRef(scope.Config, bootstrapv1.GroupVersion.WithKind("KubeadmConfig")))
+	} else {
+		err = certificates.Lookup(ctx,
+			r.Client,
+			util.ObjectKey(scope.Cluster))
+	}
 	if err != nil {
 		conditions.MarkFalse(scope.Config, bootstrapv1.CertificatesAvailableCondition, bootstrapv1.CertificatesGenerationFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
 		return ctrl.Result{}, err

controlplane/kubeadm/internal/controllers/controller.go

@@ -312,6 +312,9 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
 		err = r.adoptMachines(ctx, kcp, adoptableMachines, cluster)
 		return ctrl.Result{}, err
 	}
+	if err := ensureCertificatesOwnerRef(ctx, r.Client, util.ObjectKey(cluster), certificates, *controllerRef); err != nil {
+		return ctrl.Result{}, err
+	}
 
 	ownedMachines := controlPlaneMachines.Filter(collections.OwnedMachines(kcp))
 	if len(ownedMachines) != len(controlPlaneMachines) {
@@ -773,3 +776,33 @@ func (r *KubeadmControlPlaneReconciler) adoptOwnedSecrets(ctx context.Context, k
 
 	return nil
 }
+
+// ensureCertificatesOwnerRef ensures an ownerReference to the owner is added on the Secrets holding certificates.
+func ensureCertificatesOwnerRef(ctx context.Context, ctrlclient client.Client, clusterKey client.ObjectKey, certificates secret.Certificates, owner metav1.OwnerReference) error {
+	for _, c := range certificates {
+		s := &corev1.Secret{}
+		secretKey := client.ObjectKey{Namespace: clusterKey.Namespace, Name: secret.Name(clusterKey.Name, c.Purpose)}
+		if err := ctrlclient.Get(ctx, secretKey, s); err != nil {
+			return errors.Wrapf(err, "failed to get Secret %s", secretKey)
+		}
+		// If the Type doesn't match the type used for secrets created by core components, KCP included
+		if s.Type != clusterv1.ClusterSecretType {
+			continue
+		}
+		patchHelper, err := patch.NewHelper(s, ctrlclient)
+		if err != nil {
+			return errors.Wrapf(err, "failed to create patchHelper for Secret %s", secretKey)
+		}
+
+		// Remove the current controller if one exists.
+		if controller := metav1.GetControllerOf(s); controller != nil {
+			s.SetOwnerReferences(util.RemoveOwnerRef(s.OwnerReferences, *controller))
+		}
+
+		s.OwnerReferences = util.EnsureOwnerRef(s.OwnerReferences, owner)
+		if err := patchHelper.Patch(ctx, s); err != nil {
+			return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", secretKey, owner.String())
+		}
+	}
+	return nil
+}

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -18,7 +18,12 @@ package controllers
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
 	"fmt"
+	"math/big"
 	"sync"
 	"testing"
 	"time"
@@ -49,6 +54,7 @@ import (
 	"sigs.k8s.io/cluster-api/feature"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
 	"sigs.k8s.io/cluster-api/util"
+	"sigs.k8s.io/cluster-api/util/certs"
 	"sigs.k8s.io/cluster-api/util/collections"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/kubeconfig"
@@ -520,11 +526,12 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 			g.Expect(machine.GetAnnotations()).NotTo(HaveKey(clusterv1.TemplateClonedFromNameAnnotation))
 		}
 	})
+
 	t.Run("adopts v1alpha2 cluster secrets", func(t *testing.T) {
 		g := NewWithT(t)
 
 		cluster, kcp, tmpl := createClusterWithControlPlane(metav1.NamespaceDefault)
-		cluster.Spec.ControlPlaneEndpoint.Host = "bar"
+		cluster.Spec.ControlPlaneEndpoint.Host = "validhost"
 		cluster.Spec.ControlPlaneEndpoint.Port = 6443
 		cluster.Status.InfrastructureReady = true
 		kcp.Spec.Version = version
@@ -565,7 +572,7 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 				},
 			}
 
-			// A simulcrum of the various Certificate and kubeconfig secrets
+			// A simulacrum of the various Certificate and kubeconfig secrets
 			// it's a little weird that this is one per KubeadmConfig rather than just whichever config was "first,"
 			// but the intent is to ensure that the owner is changed regardless of which Machine we start with
 			clusterSecret := &corev1.Secret{
@@ -749,6 +756,164 @@ func TestKubeadmControlPlaneReconciler_adoption(t *testing.T) {
 	})
 }
 
+func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
+	g := NewWithT(t)
+
+	cluster, kcp, tmpl := createClusterWithControlPlane(metav1.NamespaceDefault)
+	cluster.Spec.ControlPlaneEndpoint.Host = "bar"
+	cluster.Spec.ControlPlaneEndpoint.Port = 6443
+	cluster.Status.InfrastructureReady = true
+	kcp.Spec.Version = "v1.21.0"
+	key, err := certs.NewPrivateKey()
+	g.Expect(err).To(BeNil())
+	crt, err := getTestCACert(key)
+	g.Expect(err).To(BeNil())
+
+	fmc := &fakeManagementCluster{
+		Machines: collections.Machines{},
+		Workload: fakeWorkloadCluster{},
+	}
+
+	clusterSecret := &corev1.Secret{
+		// The Secret's Type is used by KCP to determine whether it is user-provided.
+		// clusterv1.ClusterSecretType signals that the Secret is CAPI-provided.
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: cluster.Namespace,
+			Name:      "",
+			Labels: map[string]string{
+				"cluster.x-k8s.io/cluster-name": cluster.Name,
+				"testing":                       "yes",
+			},
+		},
+		Data: map[string][]byte{
+			secret.TLSCrtDataName: certs.EncodeCertPEM(crt),
+			secret.TLSKeyDataName: certs.EncodePrivateKeyPEM(key),
+		},
+	}
+
+	t.Run("add KCP owner for secrets with no controller reference", func(t *testing.T) {
+		objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
+		for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
+			s := clusterSecret.DeepCopy()
+			// Set the secret name to the purpose
+			s.Name = secret.Name(cluster.Name, purpose)
+			// Set the Secret Type to clusterv1.ClusterSecretType which signals this Secret was generated by CAPI.
+			s.Type = clusterv1.ClusterSecretType
+
+			objs = append(objs, s)
+		}
+
+		fakeClient := newFakeClient(objs...)
+		fmc.Reader = fakeClient
+		r := &KubeadmControlPlaneReconciler{
+			Client:                    fakeClient,
+			APIReader:                 fakeClient,
+			managementCluster:         fmc,
+			managementClusterUncached: fmc,
+		}
+
+		_, err := r.reconcile(ctx, cluster, kcp)
+		g.Expect(err).To(BeNil())
+
+		secrets := &corev1.SecretList{}
+		g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
+		for _, secret := range secrets.Items {
+			g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))))
+		}
+	})
+
+	t.Run("replace non-KCP controller with KCP controller reference", func(t *testing.T) {
+		objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
+		// A simulacrum of the various Certificate and kubeconfig secrets
+		// it's a little weird that this is one per KubeadmConfig rather than just whichever config was "first,"
+		// but the intent is to ensure that the owner is changed regardless of which Machine we start with
+		for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
+			s := clusterSecret.DeepCopy()
+			// Set the secret name to the purpose
+			s.Name = secret.Name(cluster.Name, purpose)
+			// Set the Secret Type to clusterv1.ClusterSecretType which signals this Secret was generated by CAPI.
+			s.Type = clusterv1.ClusterSecretType
+
+			// Set the a controller owner reference of an unknown type on the secret.
+			s.SetOwnerReferences([]metav1.OwnerReference{
+				{
+					APIVersion: bootstrapv1.GroupVersion.String(),
+					// KCP should take ownership of any Secret of the correct type linked to the Cluster.
+					Kind:       "OtherController",
+					Name:       "name",
+					UID:        "uid",
+					Controller: pointer.Bool(true),
+				},
+			})
+			objs = append(objs, s)
+		}
+
+		fakeClient := newFakeClient(objs...)
+		fmc.Reader = fakeClient
+		r := &KubeadmControlPlaneReconciler{
+			Client:                    fakeClient,
+			APIReader:                 fakeClient,
+			managementCluster:         fmc,
+			managementClusterUncached: fmc,
+		}
+
+		_, err := r.reconcile(ctx, cluster, kcp)
+		g.Expect(err).To(BeNil())
+
+		secrets := &corev1.SecretList{}
+		g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
+		for _, secret := range secrets.Items {
+			g.Expect(secret.OwnerReferences).To(HaveLen(1))
+			g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))))
+		}
+	})
+
+	t.Run("does not add owner reference to user-provided secrets", func(t *testing.T) {
+		g := NewWithT(t)
+		objs := []client.Object{fakeGenericMachineTemplateCRD, cluster.DeepCopy(), kcp.DeepCopy(), tmpl.DeepCopy()}
+		for _, purpose := range []secret.Purpose{secret.ClusterCA, secret.FrontProxyCA, secret.ServiceAccount, secret.EtcdCA} {
+			s := clusterSecret.DeepCopy()
+			// Set the secret name to the purpose
+			s.Name = secret.Name(cluster.Name, purpose)
+			// Set the Secret Type to any type which signals this Secret was is user-provided.
+			s.Type = corev1.SecretTypeOpaque
+			// Set the a controller owner reference of an unknown type on the secret.
+			s.SetOwnerReferences([]metav1.OwnerReference{
+				{
+					APIVersion: bootstrapv1.GroupVersion.String(),
+					// This owner reference to a different controller should be preserved.
+					Kind:               "OtherController",
+					Name:               kcp.Name,
+					UID:                kcp.UID,
+					Controller:         pointer.Bool(true),
+					BlockOwnerDeletion: pointer.Bool(true),
+				},
+			})
+
+			objs = append(objs, s)
+		}
+
+		fakeClient := newFakeClient(objs...)
+		fmc.Reader = fakeClient
+		r := &KubeadmControlPlaneReconciler{
+			Client:                    fakeClient,
+			APIReader:                 fakeClient,
+			managementCluster:         fmc,
+			managementClusterUncached: fmc,
+		}
+
+		_, err := r.reconcile(ctx, cluster, kcp)
+		g.Expect(err).To(BeNil())
+
+		secrets := &corev1.SecretList{}
+		g.Expect(fakeClient.List(ctx, secrets, client.InNamespace(cluster.Namespace), client.MatchingLabels{"testing": "yes"})).To(Succeed())
+		for _, secret := range secrets.Items {
+			g.Expect(secret.OwnerReferences).To(HaveLen(1))
+			g.Expect(secret.OwnerReferences).To(ContainElement(*metav1.NewControllerRef(kcp, bootstrapv1.GroupVersion.WithKind("OtherController"))))
+		}
+	})
+}
+
 func TestReconcileCertificateExpiries(t *testing.T) {
 	g := NewWithT(t)
 
@@ -1769,3 +1934,34 @@ func newCluster(namespacedName *types.NamespacedName) *clusterv1.Cluster {
 		},
 	}
 }
+
+func getTestCACert(key *rsa.PrivateKey) (*x509.Certificate, error) {
+	cfg := certs.Config{
+		CommonName: "kubernetes",
+	}
+
+	now := time.Now().UTC()
+
+	tmpl := x509.Certificate{
+		SerialNumber: new(big.Int).SetInt64(0),
+		Subject: pkix.Name{
+			CommonName:   cfg.CommonName,
+			Organization: cfg.Organization,
+		},
+		NotBefore:             now.Add(time.Minute * -5),
+		NotAfter:              now.Add(time.Hour * 24), // 1 day
+		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
+		MaxPathLenZero:        true,
+		BasicConstraintsValid: true,
+		MaxPathLen:            0,
+		IsCA:                  true,
+	}
+
+	b, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, key.Public(), key)
+	if err != nil {
+		return nil, err
+	}
+
+	c, err := x509.ParseCertificate(b)
+	return c, err
+}

controlplane/kubeadm/internal/controllers/helpers.go

@@ -74,12 +74,8 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
 		return ctrl.Result{}, errors.Wrap(err, "failed to retrieve kubeconfig Secret")
 	}
 
-	// check if the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP;
-	// if yes, adopt it.
-	if util.IsOwnedByObject(configSecret, cluster) && !util.IsControlledBy(configSecret, kcp) {
-		if err := r.adoptKubeconfigSecret(ctx, cluster, configSecret, controllerOwnerRef); err != nil {
-			return ctrl.Result{}, err
-		}
+	if err := r.adoptKubeconfigSecret(ctx, cluster, configSecret, kcp); err != nil {
+		return ctrl.Result{}, err
 	}
 
 	// only do rotation on owned secrets
@@ -102,21 +98,45 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
 	return ctrl.Result{}, nil
 }
 
-func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, controllerOwnerRef metav1.OwnerReference) error {
+// Ensure the KubeadmConfigSecret has an owner reference to the control plane if it is not a user-provided secret.
+func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) error {
 	log := ctrl.LoggerFrom(ctx)
-	log.Info("Adopting KubeConfig secret created by v1alpha2 controllers", "Secret", klog.KObj(configSecret))
+	controller := metav1.GetControllerOf(configSecret)
 
+	// If the Type doesn't match the CAPI-created secret type this is a no-op.
+	if configSecret.Type != clusterv1.ClusterSecretType {
+		return nil
+	}
+	// If the secret is already controlled by KCP this is a no-op.
+	if controller != nil && controller.Kind == "KubeadmControlPlane" {
+		return nil
+	}
+	log.Info("Adopting KubeConfig secret", "Secret", klog.KObj(configSecret))
 	patch, err := patch.NewHelper(configSecret, r.Client)
 	if err != nil {
 		return errors.Wrap(err, "failed to create patch helper for the kubeconfig secret")
 	}
-	configSecret.OwnerReferences = util.RemoveOwnerRef(configSecret.OwnerReferences, metav1.OwnerReference{
-		APIVersion: clusterv1.GroupVersion.String(),
-		Kind:       "Cluster",
-		Name:       cluster.Name,
-		UID:        cluster.UID,
-	})
-	configSecret.OwnerReferences = util.EnsureOwnerRef(configSecret.OwnerReferences, controllerOwnerRef)
+
+	// If the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP.
+	// In this case remove the ownerReference to the Cluster.
+	if util.IsOwnedByObject(configSecret, cluster) {
+		configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, metav1.OwnerReference{
+			APIVersion: clusterv1.GroupVersion.String(),
+			Kind:       "Cluster",
+			Name:       cluster.Name,
+			UID:        cluster.UID,
+		}))
+	}
+
+	// Remove the current controller if one exists.
+	if controller != nil {
+		configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, *controller))
+	}
+
+	// Add the KubeadmControlPlane as the controller for this secret.
+	configSecret.OwnerReferences = util.EnsureOwnerRef(configSecret.OwnerReferences,
+		*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")))
+
 	if err := patch.Patch(ctx, configSecret); err != nil {
 		return errors.Wrap(err, "failed to patch the kubeconfig secret")
 	}