From 28051ee94c832aa477239e95279129a76743c400 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Thu, 10 Aug 2023 19:04:59 +0100 Subject: [PATCH] Add ownerReference resilience test Signed-off-by: killianmuldoon (cherry picked from commit d63e2bcdebe6498f185ee11939c01a4aebd93c00) --- controllers/vspherecluster_reconciler.go | 70 +++++++++++-------- .../vsphereclusteridentity_controller.go | 46 ++++++------ .../vspheredeploymentzone_controller.go | 19 +---- ...vspheredeploymentzone_controller_domain.go | 20 ++++++ pkg/identity/identity.go | 2 +- 5 files changed, 84 insertions(+), 73 deletions(-) diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go index 7c2bb9cdf8..a3b608e17a 100644 --- 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 index 409e5626bb..e581fc2b57 100644 --- 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 index ce555162b0..1c856fa5fa 100644 --- 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 index 5437825b63..7258ab2ef2 100644 --- 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 index 420cb4676c..92cc68efaf 100644 --- 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 }