Refactor Cluster controller

Author: fabriziopandini

internal/controllers/cluster/cluster_controller.go

@@ -19,7 +19,6 @@ package cluster
 import (
 	"context"
 	"fmt"
-	"path"
 	"strings"
 	"time"
 
@@ -28,6 +27,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
@@ -141,6 +141,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return ctrl.Result{}, err
 	}
 
+	s := &scope{
+		cluster: cluster,
+	}
+
 	// Initialize the patch helper.
 	patchHelper, err := patch.NewHelper(cluster, r.Client)
 	if err != nil {
@@ -185,13 +189,28 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		})
 	}
 
+	alwaysReconcile := []clusterReconcileFunc{
+		r.reconcileInfrastructure,
+		r.reconcileControlPlane,
+	}
+
 	// Handle deletion reconciliation loop.
 	if !cluster.ObjectMeta.DeletionTimestamp.IsZero() {
-		return r.reconcileDelete(ctx, cluster)
+		reconcileDelete := append(
+			alwaysReconcile,
+			r.reconcileDelete,
+		)
+
+		return doReconcile(ctx, reconcileDelete, s)
 	}
 
 	// Handle normal reconciliation loop.
-	return r.reconcile(ctx, cluster)
+	reconcileNormal := append(
+		alwaysReconcile,
+		r.reconcileKubeconfig,
+		r.reconcileControlPlaneInitialized,
+	)
+	return doReconcile(ctx, reconcileNormal, s)
 }
 
 func patchCluster(ctx context.Context, patchHelper *patch.Helper, cluster *clusterv1.Cluster, options ...patch.Option) error {
@@ -216,30 +235,14 @@ func patchCluster(ctx context.Context, patchHelper *patch.Helper, cluster *clust
 	return patchHelper.Patch(ctx, cluster, options...)
 }
 
-// reconcile handles cluster reconciliation.
-func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster) (ctrl.Result, error) {
-	log := ctrl.LoggerFrom(ctx)
-
-	if cluster.Spec.Topology != nil {
-		if cluster.Spec.ControlPlaneRef == nil || cluster.Spec.InfrastructureRef == nil {
-			// TODO: add a condition to surface this scenario
-			log.Info("Waiting for the topology to be generated")
-			return ctrl.Result{}, nil
-		}
-	}
-
-	phases := []func(context.Context, *clusterv1.Cluster) (ctrl.Result, error){
-		r.reconcileInfrastructure,
-		r.reconcileControlPlane,
-		r.reconcileKubeconfig,
-		r.reconcileControlPlaneInitialized,
-	}
+type clusterReconcileFunc func(context.Context, *scope) (ctrl.Result, error)
 
+func doReconcile(ctx context.Context, phases []clusterReconcileFunc, s *scope) (ctrl.Result, error) {
 	res := ctrl.Result{}
 	errs := []error{}
 	for _, phase := range phases {
 		// Call the inner reconciliation methods.
-		phaseResult, err := phase(ctx, cluster)
+		phaseResult, err := phase(ctx, s)
 		if err != nil {
 			errs = append(errs, err)
 		}
@@ -248,12 +251,42 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster)
 		}
 		res = util.LowestNonZeroResult(res, phaseResult)
 	}
-	return res, kerrors.NewAggregate(errs)
+
+	if len(errs) > 0 {
+		return ctrl.Result{}, kerrors.NewAggregate(errs)
+	}
+
+	return res, nil
+}
+
+// scope holds the different objects that are read and used during the reconcile.
+type scope struct {
+	// cluster is the Cluster object the Machine belongs to.
+	// It is set at the beginning of the reconcile function.
+	cluster *clusterv1.Cluster
+
+	// infraCluster is the Infrastructure Cluster object that is referenced by the
+	// Cluster. It is set after reconcileInfrastructure is called.
+	infraCluster *unstructured.Unstructured
+
+	// infraClusterNotFound is true if getting the infra cluster object failed with an NotFound err
+	infraClusterIsNotFound bool
+
+	// controlPlane is the ControlPlane object that is referenced by the
+	// Cluster. It is set after reconcileBootstrap is called.
+	controlPlane *unstructured.Unstructured
+
+	// controlPlaneNotFound is true if getting the ControlPlane object failed with an NotFound err
+	controlPlaneIsNotFound bool
+
+	// descendants is the list of objects related to this Cluster
+	// descendants clusterDescendants
 }
 
 // reconcileDelete handles cluster deletion.
-func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) {
+func (r *Reconciler) reconcileDelete(ctx context.Context, s *scope) (reconcile.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
+	cluster := s.cluster
 
 	// If the RuntimeSDK and ClusterTopology flags are enabled, for clusters with managed topologies
 	// only proceed with delete if the cluster is marked as `ok-to-delete`
@@ -314,28 +347,18 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu
 	}
 
 	if cluster.Spec.ControlPlaneRef != nil {
-		obj, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Namespace)
-		switch {
-		case apierrors.IsNotFound(errors.Cause(err)):
+		if s.controlPlaneIsNotFound {
 			// All good - the control plane resource has been deleted
 			conditions.MarkFalse(cluster, clusterv1.ControlPlaneReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
-		case err != nil:
-			return reconcile.Result{}, errors.Wrapf(err, "failed to get %s %q for Cluster %s/%s",
-				path.Join(cluster.Spec.ControlPlaneRef.APIVersion, cluster.Spec.ControlPlaneRef.Kind),
-				cluster.Spec.ControlPlaneRef.Name, cluster.Namespace, cluster.Name)
-		default:
-			// Report a summary of current status of the control plane object defined for this cluster.
-			conditions.SetMirror(cluster, clusterv1.ControlPlaneReadyCondition,
-				conditions.UnstructuredGetter(obj),
-				conditions.WithFallbackValue(false, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, ""),
-			)
+		}
 
+		if s.controlPlane != nil {
 			// Issue a deletion request for the control plane object.
 			// Once it's been deleted, the cluster will get processed again.
-			if err := r.Client.Delete(ctx, obj); err != nil {
+			if err := r.Client.Delete(ctx, s.controlPlane); err != nil {
 				return ctrl.Result{}, errors.Wrapf(err,
 					"failed to delete %v %q for Cluster %q in namespace %q",
-					obj.GroupVersionKind(), obj.GetName(), cluster.Name, cluster.Namespace)
+					s.controlPlane.GroupVersionKind(), s.controlPlane.GetName(), cluster.Name, cluster.Namespace)
 			}
 
 			// Return here so we don't remove the finalizer yet.
@@ -345,28 +368,18 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu
 	}
 
 	if cluster.Spec.InfrastructureRef != nil {
-		obj, err := external.Get(ctx, r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
-		switch {
-		case apierrors.IsNotFound(errors.Cause(err)):
+		if s.infraClusterIsNotFound {
 			// All good - the infra resource has been deleted
 			conditions.MarkFalse(cluster, clusterv1.InfrastructureReadyCondition, clusterv1.DeletedReason, clusterv1.ConditionSeverityInfo, "")
-		case err != nil:
-			return ctrl.Result{}, errors.Wrapf(err, "failed to get %s %q for Cluster %s/%s",
-				path.Join(cluster.Spec.InfrastructureRef.APIVersion, cluster.Spec.InfrastructureRef.Kind),
-				cluster.Spec.InfrastructureRef.Name, cluster.Namespace, cluster.Name)
-		default:
-			// Report a summary of current status of the infrastructure object defined for this cluster.
-			conditions.SetMirror(cluster, clusterv1.InfrastructureReadyCondition,
-				conditions.UnstructuredGetter(obj),
-				conditions.WithFallbackValue(false, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, ""),
-			)
+		}
 
+		if s.infraCluster != nil {
 			// Issue a deletion request for the infrastructure object.
 			// Once it's been deleted, the cluster will get processed again.
-			if err := r.Client.Delete(ctx, obj); err != nil {
+			if err := r.Client.Delete(ctx, s.infraCluster); err != nil {
 				return ctrl.Result{}, errors.Wrapf(err,
 					"failed to delete %v %q for Cluster %q in namespace %q",
-					obj.GroupVersionKind(), obj.GetName(), cluster.Name, cluster.Namespace)
+					s.infraCluster.GroupVersionKind(), s.infraCluster.GetName(), cluster.Name, cluster.Namespace)
 			}
 
 			// Return here so we don't remove the finalizer yet.
@@ -516,8 +529,9 @@ func (c clusterDescendants) filterOwnedDescendants(cluster *clusterv1.Cluster) (
 	return ownedDescendants, nil
 }
 
-func (r *Reconciler) reconcileControlPlaneInitialized(ctx context.Context, cluster *clusterv1.Cluster) (ctrl.Result, error) {
+func (r *Reconciler) reconcileControlPlaneInitialized(ctx context.Context, s *scope) (ctrl.Result, error) {
 	log := ctrl.LoggerFrom(ctx)
+	cluster := s.cluster
 
 	// Skip checking if the control plane is initialized when using a Control Plane Provider (this is reconciled in
 	// reconcileControlPlane instead).