Rework Authentication status writes (#1028)

Originating issue: [IBMPrivateCloud/roadmap#66484](https://github.ibm.com/IBMPrivateCloud/roadmap/issues/66484)

* Simplify to one deferred function that writes out status changes to
  the Authentication CR

Signed-off-by: Rob Hundley <[email protected]>

Author: rwhundley

controllers/operator/authentication_controller.go

@@ -18,9 +18,9 @@ package operator
 
 import (
 	"context"
+	"reflect"
 
 	"fmt"
-	"reflect"
 	"sync"
 	"time"
 
@@ -218,14 +218,6 @@ func (r *AuthenticationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	needToRequeue := false
 
 	reconcileCtx := logf.IntoContext(ctx, reqLogger)
-	// Set default result
-	result = ctrl.Result{}
-	// Set Requeue to true if requeue is needed at end of reconcile loop
-	defer func() {
-		if needToRequeue {
-			result.Requeue = true
-		}
-	}()
 
 	reqLogger.Info("Reconciling Authentication")
 
@@ -262,63 +254,44 @@ func (r *AuthenticationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return
 	}
 
+	originalAuthCR := instance.DeepCopy()
 	// Be sure to update status before returning if Authentication is found (but only if the Authentication hasn't
 	// already been updated, e.g. finalizer update
 	defer func() {
-		reqLogger.Info("Update status before finishing loop.")
-		if reflect.DeepEqual(instance.Status, operatorv1alpha1.AuthenticationStatus{}) {
-			instance.Status = operatorv1alpha1.AuthenticationStatus{
-				Nodes: []string{},
-			}
+		if needToRequeue {
+			result.Requeue = true
 		}
-		currentServiceStatus := r.getCurrentServiceStatus(ctx, r.Client, instance)
-		if !reflect.DeepEqual(currentServiceStatus, instance.Status.Service) {
-			instance.Status.Service = currentServiceStatus
-			reqLogger.Info("Current status does not reflect current state; updating")
+		err = r.setAuthenticationStatus(ctx, instance)
+		if reflect.DeepEqual(originalAuthCR.Status, instance.Status) {
+			goto statuslog
 		}
-		statusUpdateErr := r.Client.Status().Update(ctx, instance)
-		if statusUpdateErr != nil {
-			reqLogger.Error(statusUpdateErr, "Failed to update status; trying again")
-			currentInstance := &operatorv1alpha1.Authentication{}
-			r.Client.Get(ctx, req.NamespacedName, currentInstance)
-			currentInstance.Status.Service = currentServiceStatus
-			statusUpdateErr = r.Client.Status().Update(ctx, currentInstance)
-			if statusUpdateErr != nil {
-				reqLogger.Error(statusUpdateErr, "Retry failed; returning error")
-				return
-			}
+		reqLogger.Info("Update status before finishing loop.")
+		if err = r.Client.Status().Update(ctx, instance); err != nil {
+			reqLogger.Error(err, "Failed to update status")
 		} else {
 			reqLogger.Info("Updated status")
 		}
-		reqLogger.V(1).Info("Final result", "result", result, "err", err)
+	statuslog:
+		reqLogger.Info("Reconciliation complete")
 	}()
 
 	var subResult *ctrl.Result
 	if subResult, err = r.addMongoMigrationFinalizers(reconcileCtx, req); subreconciler.ShouldHaltOrRequeue(subResult, err) {
-		reqLogger.V(1).Info("Should halt or requeue after addMongoMigrationFinalizers", "result", result, "err", err)
-		result, err = subreconciler.Evaluate(subResult, err)
-		return
+		return subreconciler.Evaluate(subResult, err)
 	}
 
 	if subResult, err = r.overrideMongoDBBootstrap(reconcileCtx, req); subreconciler.ShouldHaltOrRequeue(subResult, err) {
-		reqLogger.V(1).Info("Should halt or requeue after overrideMongoDBBootstrap", "result", result, "err", err)
-		result, err = subreconciler.Evaluate(subResult, err)
-		return
+		return subreconciler.Evaluate(subResult, err)
 	}
 
 	if subResult, err = r.handleOperandRequest(reconcileCtx, req); subreconciler.ShouldHaltOrRequeue(subResult, err) {
-		reqLogger.V(1).Info("Should halt or requeue after handleOperandRequest", "result", result, "err", err)
-		result, err = subreconciler.Evaluate(subResult, err)
-		return
+		return subreconciler.Evaluate(subResult, err)
 	}
 
 	if subResult, err = r.createEDBShareClaim(reconcileCtx, req); subreconciler.ShouldHaltOrRequeue(subResult, err) {
-		reqLogger.V(1).Info("Should halt or requeue after createEDBShareClaim", "result", result, "err", err)
-		result, err = subreconciler.Evaluate(subResult, err)
-		return
+		return subreconciler.Evaluate(subResult, err)
 	}
 
-	// Check if this Certificate already exists and create it if it doesn't
 	reqLogger.Info("Creating ibm-iam-operand-restricted serviceaccount")
 	currentSA := &corev1.ServiceAccount{}
 	err = r.createSA(instance, currentSA, &needToRequeue)
@@ -389,7 +362,6 @@ func (r *AuthenticationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	}
 
 	if result, err := r.handleMongoDBCleanup(reconcileCtx, req); subreconciler.ShouldHaltOrRequeue(result, err) {
-		reqLogger.V(1).Info("Should halt or requeue after handleMongoDBCleanup", "result", result, "err", err)
 		return subreconciler.Evaluate(result, err)
 	}
 
@@ -418,7 +390,7 @@ func (r *AuthenticationReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		return subreconciler.Evaluate(subResult, err)
 	}
 
-	return
+	return subreconciler.Evaluate(subreconciler.DoNotRequeue())
 }
 
 // SetupWithManager sets up the controller with the Manager.

controllers/operator/deployment.go

@@ -19,7 +19,6 @@ package operator
 import (
 	"context"
 	"os"
-	"reflect"
 
 	operatorv1alpha1 "github.com/IBM/ibm-iam-operator/apis/operator/v1alpha1"
 	"github.com/IBM/ibm-iam-operator/controllers/common"
@@ -30,7 +29,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
@@ -165,18 +163,6 @@ func (r *AuthenticationReconciler) handleDeployment(instance *operatorv1alpha1.A
 		}
 	}
 
-	podList := &corev1.PodList{}
-	listOpts := []client.ListOption{
-		client.InNamespace(instance.Namespace),
-		client.MatchingLabels(map[string]string{"k8s-app": deployment}),
-	}
-	if err = r.Client.List(context.TODO(), podList, listOpts...); err != nil {
-		reqLogger.Error(err, "Failed to list pods", "Authentication.Namespace", instance.Namespace, "Authentication.Name", deployment)
-		return err
-	}
-	reqLogger.Info("CS??? get pod names")
-	podNames := getPodNames(podList.Items)
-
 	// Deployment already exists - don't requeue
 	reqLogger.Info("Skip reconcile: Deployment already exists", "Deployment.Namespace", instance.Namespace, "Deployment.Name", deployment)
 
@@ -229,21 +215,6 @@ func (r *AuthenticationReconciler) handleDeployment(instance *operatorv1alpha1.A
 		}
 	}
 
-	podListMgr := &corev1.PodList{}
-	listOptsMgr := []client.ListOption{
-		client.InNamespace(instance.Namespace),
-		client.MatchingLabels(map[string]string{"k8s-app": managerDeployment}),
-	}
-	if err = r.Client.List(context.TODO(), podListMgr, listOptsMgr...); err != nil {
-		reqLogger.Error(err, "Failed to list pods", "Authentication.Namespace", instance.Namespace, "Authentication.Name", managerDeployment)
-		return err
-	}
-	reqLogger.Info("CS??? get pod names")
-	podNamesMgr := getPodNames(podListMgr.Items)
-	for _, pod := range podNamesMgr {
-		podNames = append(podNames, pod)
-	}
-
 	// Deployment already exists - don't requeue
 	reqLogger.Info("Skip reconcile: Manager deployment already exists", "Deployment.Namespace", instance.Namespace, "Deployment.Name", managerDeployment)
 	// reconcile provider
@@ -295,48 +266,12 @@ func (r *AuthenticationReconciler) handleDeployment(instance *operatorv1alpha1.A
 		}
 	}
 
-	podListProv := &corev1.PodList{}
-	listOptsProv := []client.ListOption{
-		client.InNamespace(instance.Namespace),
-		client.MatchingLabels(map[string]string{"k8s-app": providerDeployment}),
-	}
-	if err = r.Client.List(context.TODO(), podListProv, listOptsProv...); err != nil {
-		reqLogger.Error(err, "Failed to list pods", "Authentication.Namespace", instance.Namespace, "Authentication.Name", providerDeployment)
-		return err
-	}
-	reqLogger.Info("CS??? get pod names")
-	podNamesProv := getPodNames(podListProv.Items)
-	for _, pod := range podNamesProv {
-		podNames = append(podNames, pod)
-	}
-	// Deployment already exists - don't requeue
-	reqLogger.Info("Final pod names", "Pod names:", podNames)
-	// Update status.Nodes if needed
-	if !reflect.DeepEqual(podNames, instance.Status.Nodes) {
-		instance.Status.Nodes = podNames
-		reqLogger.Info("CS??? put pod names in status")
-		err := r.Client.Status().Update(context.TODO(), instance)
-		if err != nil {
-			reqLogger.Error(err, "Failed to update Authentication status")
-			return err
-		}
-	}
 	// Deployment already exists - don't requeue
 	reqLogger.Info("Skip reconcile: Provider deployment already exists", "Deployment.Namespace", instance.Namespace, "Deployment.Name", providerDeployment)
 	return nil
 
 }
 
-func getPodNames(pods []corev1.Pod) []string {
-	reqLogger := log.WithValues("Request.Namespace", "CS??? namespace", "Request.Name", "CS???")
-	var podNames []string
-	for _, pod := range pods {
-		podNames = append(podNames, pod.Name)
-		reqLogger.Info("CS??? pod name=" + pod.Name)
-	}
-	return podNames
-}
-
 func generateDeploymentObject(instance *operatorv1alpha1.Authentication, scheme *runtime.Scheme, deployment string, icpConsoleURL string, saasCrnId string, imagePullSecret string) *appsv1.Deployment {
 
 	reqLogger := log.WithValues("deploymentForAuthentication", "Entry", "instance.Name", instance.Name)

controllers/operator/resourcestatus.go

@@ -18,6 +18,7 @@ package operator
 
 import (
 	"context"
+	"slices"
 
 	operatorv1alpha1 "github.com/IBM/ibm-iam-operator/apis/operator/v1alpha1"
 	zenv1 "github.com/IBM/ibm-iam-operator/apis/zen.cpd.ibm.com/v1"
@@ -41,6 +42,46 @@ const (
 	ResourceNotReadyState string = "NotReady"
 )
 
+func (r *AuthenticationReconciler) setAuthenticationStatus(ctx context.Context, authCR *operatorv1alpha1.Authentication) (err error) {
+	nodes, err := r.getNodesStatus(ctx, authCR)
+	if len(authCR.Status.Nodes) == 0 || !slices.Equal(authCR.Status.Nodes, nodes) {
+		authCR.Status.Nodes = nodes
+	}
+	authCR.Status.Service = r.getCurrentServiceStatus(ctx, r.Client, authCR)
+	return
+}
+
+// getNodesStatus returns a sorted list of IM Pods that is written to the Authentication CR's .status.nodes.
+func (r *AuthenticationReconciler) getNodesStatus(ctx context.Context, authCR *operatorv1alpha1.Authentication) (nodes []string, err error) {
+	log := logf.FromContext(ctx)
+	appNames := []string{"platform-auth-service", "platform-identity-management", "platform-identity-provider"}
+	nodes = []string{}
+	for _, appName := range appNames {
+		podList := &corev1.PodList{}
+		listOptsProv := []client.ListOption{
+			client.InNamespace(authCR.Namespace),
+			client.MatchingLabels(map[string]string{"k8s-app": appName}),
+		}
+		if err = r.Client.List(ctx, podList, listOptsProv...); err != nil {
+			log.Info("Failed to list pods by label and namespace", "k8s-app", appName, "namespace", authCR.Namespace)
+			return
+		}
+		nodes = append(nodes, getPodNames(podList.Items)...)
+	}
+	slices.Sort(nodes)
+	return
+}
+
+func getPodNames(pods []corev1.Pod) []string {
+	reqLogger := log.WithValues("Request.Namespace", "CS??? namespace", "Request.Name", "CS???")
+	var podNames []string
+	for _, pod := range pods {
+		podNames = append(podNames, pod.Name)
+		reqLogger.Info("CS??? pod name=" + pod.Name)
+	}
+	return podNames
+}
+
 func getServiceStatus(ctx context.Context, k8sClient client.Client, namespacedName types.NamespacedName) (status operatorv1alpha1.ManagedResourceStatus) {
 	reqLogger := logf.FromContext(ctx).WithName("getServiceStatus").V(1)
 	kind := "Service"