Fix KubeadmControlPlane secrets should always be adopted

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

api/v1beta1/common_types.go

@@ -107,6 +107,7 @@ const (
 	MachineSkipRemediationAnnotation = "cluster.x-k8s.io/skip-remediation"
 
 	// ClusterSecretType defines the type of secret created by core components.
+	// This is used by KCP to determine whether a secret is created by KCP or user supplied.
 	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,20 @@ 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 ControlPlaneReference look up and generate the certificates.
+	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 {
+		// If there is a ControlPlane ref look up the existing certificates.
+		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,38 @@ 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 {
+			// If the secret isn't found ignore the error.
+			if apierrors.IsNotFound(err) {
+				continue
+			}
+			return errors.Wrapf(err, "failed to get Secret %s", secretKey)
+		}
+		// If the Type doesn't match the CAPI-created secret type this is a no-op.
+		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.
+		controller := metav1.GetControllerOf(s)
+		if 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/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")
 	}

controlplane/kubeadm/internal/controllers/helpers_test.go

@@ -237,11 +237,22 @@ func TestReconcileKubeconfigSecretDoesNotAdoptsUserSecrets(t *testing.T) {
 		},
 	}
 
-	existingKubeconfigSecret := kubeconfig.GenerateSecretWithOwner(
-		client.ObjectKey{Name: "foo", Namespace: metav1.NamespaceDefault},
-		[]byte{},
-		metav1.OwnerReference{}, // user defined secrets are not owned by the cluster.
-	)
+	existingKubeconfigSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      secret.Name("foo", secret.Kubeconfig),
+			Namespace: metav1.NamespaceDefault,
+			Labels: map[string]string{
+				clusterv1.ClusterLabelName: "foo",
+			},
+			OwnerReferences: []metav1.OwnerReference{},
+		},
+		Data: map[string][]byte{
+			secret.KubeconfigDataName: {},
+		},
+		// KCP identifies CAPI-created Secrets using the clusterv1.ClusterSecretType. Setting any other type allows
+		// the controllers to treat it as a user-provided Secret.
+		Type: corev1.SecretTypeOpaque,
+	}
 
 	fakeClient := newFakeClient(kcp.DeepCopy(), existingKubeconfigSecret.DeepCopy())
 	r := &KubeadmControlPlaneReconciler{
@@ -261,6 +272,7 @@ func TestReconcileKubeconfigSecretDoesNotAdoptsUserSecrets(t *testing.T) {
 	g.Expect(r.Client.Get(ctx, secretName, kubeconfigSecret)).To(Succeed())
 	g.Expect(kubeconfigSecret.Labels).To(Equal(existingKubeconfigSecret.Labels))
 	g.Expect(kubeconfigSecret.Data).To(Equal(existingKubeconfigSecret.Data))
+	//TODO: (killianmuldoon) find out how to achieve this with consistent adoption.
 	g.Expect(kubeconfigSecret.OwnerReferences).ToNot(ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))))
 }