Merge pull request #120 from awslabs/conditions

refactor: simplify resource group controller error handling and condition management

Author: michaelhtm

api/v1alpha1/conditions.go

@@ -73,3 +73,37 @@ type Condition struct {
 	// +kubebuilder:validation:Minimum=0
 	ObservedGeneration int64 `json:"observedGeneration,omitempty"`
 }
+
+// NewCondition returns a new Condition instance.
+func NewCondition(t ConditionType, status metav1.ConditionStatus, reason, message string) Condition {
+	return Condition{
+		Type:               t,
+		Status:             status,
+		LastTransitionTime: &metav1.Time{Time: metav1.Now().Time},
+		Reason:             &reason,
+		Message:            &message,
+	}
+}
+
+func GetCondition(conditions []Condition, t ConditionType) *Condition {
+	for _, c := range conditions {
+		if c.Type == t {
+			return &c
+		}
+	}
+	return nil
+}
+
+func SetCondition(conditions []Condition, condition Condition) []Condition {
+	for i, c := range conditions {
+		if c.Type == condition.Type {
+			conditions[i] = condition
+			return conditions
+		}
+	}
+	return append(conditions, condition)
+}
+
+func HasCondition(conditions []Condition, t ConditionType) bool {
+	return GetCondition(conditions, t) != nil
+}

internal/controller/resourcegroup/condition/condition.go

@@ -81,28 +81,16 @@ func (r *ResourceGroupReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Complete(r)
 }
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-// TODO(user): Modify the Reconcile function to compare the state specified by
-// the ResourceGroup object against the actual cluster state, and then
-// perform operations to make the cluster state reflect the state specified by
-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
 func (r *ResourceGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	rlog := r.log.WithValues("resourcegroup", req.NamespacedName)
 	ctx = log.IntoContext(ctx, rlog)
 
 	var resourcegroup v1alpha1.ResourceGroup
-	err := r.Get(ctx, req.NamespacedName, &resourcegroup)
-	if err != nil {
+	if err := r.Get(ctx, req.NamespacedName, &resourcegroup); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	// reconcile resourcegroup fiesta
-	err = r.reconcile(ctx, &resourcegroup)
-	if err != nil {
+	if err := r.reconcile(ctx, &resourcegroup); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -111,39 +99,33 @@ func (r *ResourceGroupReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 func (r *ResourceGroupReconciler) reconcile(ctx context.Context, resourcegroup *v1alpha1.ResourceGroup) error {
 	log, _ := logr.FromContext(ctx)
-	// if deletion timestamp is set, call cleanupResourceGroup
+
 	if !resourcegroup.DeletionTimestamp.IsZero() {
 		log.V(1).Info("ResourceGroup is being deleted")
-		err := r.cleanupResourceGroup(ctx, resourcegroup)
-		if err != nil {
+		if err := r.cleanupResourceGroup(ctx, resourcegroup); err != nil {
 			return err
 		}
 
 		log.V(1).Info("Setting resourcegroup as unmanaged")
-		// remove finalizer
-		err = r.setUnmanaged(ctx, resourcegroup)
-		if err != nil {
+		if err := r.setUnmanaged(ctx, resourcegroup); err != nil {
 			return err
 		}
 
 		return nil
 	}
 
 	log.V(1).Info("Setting resource group as managed")
-	// set finalizer
-	err := r.setManaged(ctx, resourcegroup)
-	if err != nil {
+	if err := r.setManaged(ctx, resourcegroup); err != nil {
 		return err
 	}
 
 	log.V(1).Info("Syncing resourcegroup")
 	topologicalOrder, resourcesInformation, reconcileErr := r.reconcileResourceGroup(ctx, resourcegroup)
 
 	log.V(1).Info("Setting resourcegroup status")
-	// set status
-	err = r.setResourceGroupStatus(ctx, resourcegroup, topologicalOrder, resourcesInformation, reconcileErr)
-	if err != nil {
+	if err := r.setResourceGroupStatus(ctx, resourcegroup, topologicalOrder, resourcesInformation, reconcileErr); err != nil {
 		return err
 	}
+
 	return nil
 }

internal/controller/resourcegroup/condition/condition_instance.go

@@ -26,45 +26,57 @@ import (
 	"github.com/awslabs/kro/internal/metadata"
 )
 
+// cleanupResourceGroup handles the deletion of a ResourceGroup by shutting down its associated
+// microcontroller and cleaning up the CRD if enabled. It executes cleanup operations in order:
+// 1. Shuts down the microcontroller
+// 2. Deletes the associated CRD (if CRD deletion is enabled)
 func (r *ResourceGroupReconciler) cleanupResourceGroup(ctx context.Context, rg *v1alpha1.ResourceGroup) error {
 	log, _ := logr.FromContext(ctx)
+	log.V(1).Info("cleaning up resource group", "name", rg.Name)
 
-	log.V(1).Info("Cleaning up resource group")
+	// shutdown microcontroller
 	gvr := metadata.GetResourceGroupInstanceGVR(rg.Spec.Schema.APIVersion, rg.Spec.Schema.Kind)
-
-	log.V(1).Info("Shutting down resource group microcontroller")
-	err := r.shutdownResourceGroupMicroController(ctx, &gvr)
-	if err != nil {
-		return err
+	if err := r.shutdownResourceGroupMicroController(ctx, &gvr); err != nil {
+		return fmt.Errorf("failed to shutdown microcontroller: %w", err)
 	}
 
-	crdName := r.extractCRDName(rg.Spec.Schema.Kind)
-	log.V(1).Info("Cleaning up resource group CRD", "crd", crdName)
-	err = r.cleanupResourceGroupCRD(ctx, crdName)
-	if err != nil {
-		return err
+	// cleanup CRD
+	crdName := extractCRDName(rg.Spec.Schema.Kind)
+	if err := r.cleanupResourceGroupCRD(ctx, crdName); err != nil {
+		return fmt.Errorf("failed to cleanup CRD %s: %w", crdName, err)
 	}
 
 	return nil
 }
 
-func (r *ResourceGroupReconciler) extractCRDName(kind string) string {
-	pluralKind := flect.Pluralize(strings.ToLower(kind))
-	return fmt.Sprintf("%s.%s", pluralKind, v1alpha1.KroDomainName)
-}
-
+// shutdownResourceGroupMicroController stops the dynamic controller associated with the given GVR.
+// This ensures no new reconciliations occur for this resource type.
 func (r *ResourceGroupReconciler) shutdownResourceGroupMicroController(ctx context.Context, gvr *schema.GroupVersionResource) error {
-	return r.dynamicController.StopServiceGVK(ctx, *gvr)
+	if err := r.dynamicController.StopServiceGVK(ctx, *gvr); err != nil {
+		return fmt.Errorf("error stopping service: %w", err)
+	}
+	return nil
 }
 
+// cleanupResourceGroupCRD deletes the CRD with the given name if CRD deletion is enabled.
+// If CRD deletion is disabled, it logs the skip and returns nil.
 func (r *ResourceGroupReconciler) cleanupResourceGroupCRD(ctx context.Context, crdName string) error {
-	if r.allowCRDDeletion {
-		err := r.crdManager.Delete(ctx, crdName)
-		if err != nil {
-			return err
-		}
-	} else {
-		r.log.Info("CRD deletion is disabled, skipping CRD deletion", "crd", crdName)
+	if !r.allowCRDDeletion {
+		log, _ := logr.FromContext(ctx)
+		log.Info("skipping CRD deletion (disabled)", "crd", crdName)
+		return nil
+	}
+
+	if err := r.crdManager.Delete(ctx, crdName); err != nil {
+		return fmt.Errorf("error deleting CRD: %w", err)
 	}
 	return nil
 }
+
+// extractCRDName generates the CRD name from a given kind by converting it to plural form
+// and appending the Kro domain name.
+func extractCRDName(kind string) string {
+	return fmt.Sprintf("%s.%s",
+		flect.Pluralize(strings.ToLower(kind)),
+		v1alpha1.KroDomainName)
+}