Merge pull request #2355 from killianmuldoon/pr-1.7-ownerref

[release-1.7] 🌱 Add ownerReference resilience test
kubernetes-sigs · Sep 14, 2023 · 0c3f34f · 0c3f34f
2 parents 25eb1b8 + 28051ee
commit 0c3f34f
Show file tree

Hide file tree

Showing 5 changed files with 84 additions and 73 deletions.
diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go
@@ -285,38 +285,46 @@ func (r clusterReconciler) reconcileNormal(ctx *context.ClusterContext) (reconci
 
 func (r clusterReconciler) reconcileIdentitySecret(ctx *context.ClusterContext) error {
 	vsphereCluster := ctx.VSphereCluster
-	if identity.IsSecretIdentity(vsphereCluster) {
-		secret := &apiv1.Secret{}
-		secretKey := client.ObjectKey{
-			Namespace: vsphereCluster.Namespace,
-			Name:      vsphereCluster.Spec.IdentityRef.Name,
-		}
-		err := ctx.Client.Get(ctx, secretKey, secret)
-		if err != nil {
-			return err
-		}
+	if !identity.IsSecretIdentity(vsphereCluster) {
+		return nil
+	}
+	secret := &apiv1.Secret{}
+	secretKey := client.ObjectKey{
+		Namespace: vsphereCluster.Namespace,
+		Name:      vsphereCluster.Spec.IdentityRef.Name,
+	}
+	err := ctx.Client.Get(ctx, secretKey, secret)
+	if err != nil {
+		return err
+	}
 
-		// check if cluster is already an owner
-		if !clusterutilv1.IsOwnedByObject(secret, vsphereCluster) {
-			ownerReferences := secret.GetOwnerReferences()
-			if identity.IsOwnedByIdentityOrCluster(ownerReferences) {
-				return fmt.Errorf("another cluster has set the OwnerRef for secret: %s/%s", secret.Namespace, secret.Name)
-			}
-			ownerReferences = append(ownerReferences, metav1.OwnerReference{
-				APIVersion: infrav1.GroupVersion.String(),
-				Kind:       vsphereCluster.Kind,
-				Name:       vsphereCluster.Name,
-				UID:        vsphereCluster.UID,
-			})
-			secret.SetOwnerReferences(ownerReferences)
-		}
-		if !ctrlutil.ContainsFinalizer(secret, infrav1.SecretIdentitySetFinalizer) {
-			ctrlutil.AddFinalizer(secret, infrav1.SecretIdentitySetFinalizer)
-		}
-		err = r.Client.Update(ctx, secret)
-		if err != nil {
-			return err
-		}
+	// If a different VSphereCluster is an owner return an error.
+	if !clusterutilv1.IsOwnedByObject(secret, vsphereCluster) && identity.IsOwnedByIdentityOrCluster(secret.GetOwnerReferences()) {
+		return fmt.Errorf("another cluster has set the OwnerRef for secret: %s/%s", secret.Namespace, secret.Name)
+	}
+
+	helper, err := patch.NewHelper(secret, ctx.Client)
+	if err != nil {
+		return err
+	}
+
+	// Ensure the VSphereCluster is an owner and that the APIVersion is up to date.
+	secret.SetOwnerReferences(clusterutilv1.EnsureOwnerRef(secret.GetOwnerReferences(),
+		metav1.OwnerReference{
+			APIVersion: infrav1.GroupVersion.String(),
+			Kind:       vsphereCluster.Kind,
+			Name:       vsphereCluster.Name,
+			UID:        vsphereCluster.UID,
+		},
+	))
+
+	// Ensure the finalizer is added.
+	if !ctrlutil.ContainsFinalizer(secret, infrav1.SecretIdentitySetFinalizer) {
+		ctrlutil.AddFinalizer(secret, infrav1.SecretIdentitySetFinalizer)
+	}
+	err = helper.Patch(ctx, secret)
+	if err != nil {
+		return err
 	}
 
 	return nil

diff --git a/controllers/vsphereclusteridentity_controller.go b/controllers/vsphereclusteridentity_controller.go
@@ -134,30 +134,30 @@ func (r clusterIdentityReconciler) Reconcile(ctx _context.Context, req reconcile
 		return reconcile.Result{}, errors.Errorf("secret: %s not found in namespace: %s", secretKey.Name, secretKey.Namespace)
 	}
 
-	if !clusterutilv1.IsOwnedByObject(secret, identity) {
-		ownerReferences := secret.GetOwnerReferences()
-		if pkgidentity.IsOwnedByIdentityOrCluster(ownerReferences) {
-			conditions.MarkFalse(identity, infrav1.CredentialsAvailableCondidtion, infrav1.SecretAlreadyInUseReason, clusterv1.ConditionSeverityError, "secret being used by another Cluster/VSphereIdentity")
-			identity.Status.Ready = false
-			return reconcile.Result{}, errors.New("secret being used by another Cluster/VSphereIdentity")
-		}
-
-		ownerReferences = append(ownerReferences, metav1.OwnerReference{
-			APIVersion: infrav1.GroupVersion.String(),
-			Kind:       identity.Kind,
-			Name:       identity.Name,
-			UID:        identity.UID,
-		})
-		secret.SetOwnerReferences(ownerReferences)
+	// If this secret is owned by a different VSphereClusterIdentity or a VSphereCluster, mark the identity as not ready and return an error.
+	if !clusterutilv1.IsOwnedByObject(secret, identity) && pkgidentity.IsOwnedByIdentityOrCluster(secret.GetOwnerReferences()) {
+		conditions.MarkFalse(identity, infrav1.CredentialsAvailableCondidtion, infrav1.SecretAlreadyInUseReason, clusterv1.ConditionSeverityError, "secret being used by another Cluster/VSphereIdentity")
+		identity.Status.Ready = false
+		return reconcile.Result{}, errors.New("secret being used by another Cluster/VSphereIdentity")
+	}
 
-		if !ctrlutil.ContainsFinalizer(secret, infrav1.SecretIdentitySetFinalizer) {
-			ctrlutil.AddFinalizer(secret, infrav1.SecretIdentitySetFinalizer)
-		}
-		err = r.Client.Update(ctx, secret)
-		if err != nil {
-			conditions.MarkFalse(identity, infrav1.CredentialsAvailableCondidtion, infrav1.SecretOwnerReferenceFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
-			return reconcile.Result{}, err
-		}
+	// Ensure the VSphereClusterIdentity is set as the owner of the secret, and that the reference has an up to date APIVersion.
+	secret.SetOwnerReferences(
+		clusterutilv1.EnsureOwnerRef(secret.GetOwnerReferences(),
+			metav1.OwnerReference{
+				APIVersion: infrav1.GroupVersion.String(),
+				Kind:       identity.Kind,
+				Name:       identity.Name,
+				UID:        identity.UID,
+			}))
+
+	if !ctrlutil.ContainsFinalizer(secret, infrav1.SecretIdentitySetFinalizer) {
+		ctrlutil.AddFinalizer(secret, infrav1.SecretIdentitySetFinalizer)
+	}
+	err = r.Client.Update(ctx, secret)
+	if err != nil {
+		conditions.MarkFalse(identity, infrav1.CredentialsAvailableCondidtion, infrav1.SecretOwnerReferenceFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
+		return reconcile.Result{}, err
 	}
 
 	conditions.MarkTrue(identity, infrav1.CredentialsAvailableCondidtion)

diff --git a/controllers/vspheredeploymentzone_controller.go b/controllers/vspheredeploymentzone_controller.go
@@ -182,24 +182,7 @@ func (r vsphereDeploymentZoneReconciler) reconcileNormal(ctx *context.VSphereDep
 	}
 	conditions.MarkTrue(ctx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition)
 
-	// Ensure the VSphereDeploymentZone is marked as an owner of the VSphereFailureDomain.
-	if !clusterutilv1.HasOwnerRef(ctx.VSphereFailureDomain.GetOwnerReferences(), metav1.OwnerReference{
-		APIVersion: infrav1.GroupVersion.String(),
-		Kind:       "VSphereDeploymentZone",
-		Name:       ctx.VSphereDeploymentZone.Name,
-	}) {
-		if err := updateOwnerReferences(ctx, ctx.VSphereFailureDomain, r.Client, func() []metav1.OwnerReference {
-			return append(ctx.VSphereFailureDomain.OwnerReferences, metav1.OwnerReference{
-				APIVersion: infrav1.GroupVersion.String(),
-				Kind:       ctx.VSphereDeploymentZone.Kind,
-				Name:       ctx.VSphereDeploymentZone.Name,
-				UID:        ctx.VSphereDeploymentZone.UID,
-			})
-		}); err != nil {
-			return err
-		}
-	}
-
+	// Mark the deployment zone as ready.
 	ctx.VSphereDeploymentZone.Status.Ready = pointer.Bool(true)
 	return nil
 }

diff --git a/controllers/vspheredeploymentzone_controller_domain.go b/controllers/vspheredeploymentzone_controller_domain.go
@@ -18,8 +18,10 @@ package controllers
 
 import (
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	apierrors "k8s.io/apimachinery/pkg/util/errors"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	clusterutilv1 "sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/conditions"
 	ctrl "sigs.k8s.io/controller-runtime"
 
@@ -58,6 +60,24 @@ func (r vsphereDeploymentZoneReconciler) reconcileFailureDomain(ctx *context.VSp
 		logger.Error(err, "topology is not configured correctly")
 		return errors.Wrap(err, "topology is not configured correctly")
 	}
+
+	// Ensure the VSphereDeploymentZone is marked as an owner of the VSphereFailureDomain.
+	if err := updateOwnerReferences(ctx, ctx.VSphereFailureDomain, r.Client,
+		func() []metav1.OwnerReference {
+			return clusterutilv1.EnsureOwnerRef(
+				ctx.VSphereFailureDomain.OwnerReferences,
+				metav1.OwnerReference{
+					APIVersion: infrav1.GroupVersion.String(),
+					Kind:       ctx.VSphereDeploymentZone.Kind,
+					Name:       ctx.VSphereDeploymentZone.Name,
+					UID:        ctx.VSphereDeploymentZone.UID,
+				})
+		}); err != nil {
+		return err
+	}
+
+	// Mark the VSphereDeploymentZone as having a valid VSphereFailureDomain.
+	conditions.MarkTrue(ctx.VSphereDeploymentZone, infrav1.VSphereFailureDomainValidatedCondition)
 	return nil
 }
 

diff --git a/pkg/identity/identity.go b/pkg/identity/identity.go
@@ -122,11 +122,11 @@ func validateInputs(c client.Client, cluster *infrav1.VSphereCluster) error {
 	return nil
 }
 
+// IsSecretIdentity returns true if the VSphereCluster identity is a Secret.
 func IsSecretIdentity(cluster *infrav1.VSphereCluster) bool {
 	if cluster == nil || cluster.Spec.IdentityRef == nil {
 		return false
 	}
-
 	return cluster.Spec.IdentityRef.Kind == infrav1.SecretKind
 }