Fixes to functions, tests and adding new predicate

- Added a predicate function `managedClusterViewStatusChangePredicate` to filter updates based on status changes of ManagedClusterView.
- Refactored utility functions in `utils/acm.go` for consistency and added `GetManagedClusterViewName` function to generate ManagedClusterView names.
- Updated unit tests for `ManagedClusterReconciler` and utility functions to reflect the new logic and structure.
- Updated RBAC permissions to include specific verbs (create, get, list, update, watch) for ManagedClusterView resources in both `config/rbac/role.yaml` and `bundle/manifests/odf-multicluster-orchestrator.clusterserviceversion.yaml`.

Signed-off-by: vbadrina <[email protected]>

bundle/manifests/odf-multicluster-orchestrator.clusterserviceversion.yaml

@@ -36,7 +36,7 @@ metadata:
       ]
     capabilities: Basic Install
     console.openshift.io/plugins: '["odf-multicluster-console"]'
-    createdAt: "2024-07-04T11:28:14Z"
+    createdAt: "2024-07-09T07:46:31Z"
     olm.skipRange: ""
     operators.openshift.io/infrastructure-features: '["disconnected"]'
     operators.operatorframework.io/builder: operator-sdk-v1.34.1
@@ -229,7 +229,11 @@ spec:
           resources:
           - managedclusterviews
           verbs:
-          - '*'
+          - create
+          - get
+          - list
+          - update
+          - watch
         - apiGroups:
           - work.open-cluster-management.io
           resources:

config/rbac/role.yaml

@@ -166,7 +166,11 @@ rules:
   resources:
   - managedclusterviews
   verbs:
-  - '*'
+  - create
+  - get
+  - list
+  - update
+  - watch
 - apiGroups:
   - work.open-cluster-management.io
   resources:

controllers/managedcluster_controller.go

@@ -2,24 +2,25 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 	"log/slog"
+	"reflect"
 
 	"github.com/red-hat-storage/odf-multicluster-orchestrator/controllers/utils"
 	viewv1beta1 "github.com/stolostron/multicloud-operators-foundation/pkg/apis/view/v1beta1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 type ManagedClusterReconciler struct {
 	Client client.Client
-	Scheme *runtime.Scheme
 	Logger *slog.Logger
 }
 
@@ -37,7 +38,7 @@ func (r *ManagedClusterReconciler) Reconcile(ctx context.Context, req reconcile.
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
-	if err := r.ensureManagedClusterViews(ctx, managedCluster.Name); err != nil {
+	if err := r.processManagedClusterViews(ctx, managedCluster.Name); err != nil {
 		logger.Error("Failed to ensure ManagedClusterView", "error", err)
 		return ctrl.Result{}, err
 	}
@@ -55,7 +56,7 @@ func (r *ManagedClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(
 			&viewv1beta1.ManagedClusterView{},
 			handler.EnqueueRequestsFromMapFunc(r.managedClusterViewRequestMapper),
-			builder.WithPredicates(predicate.ResourceVersionChangedPredicate{}, predicate.GenerationChangedPredicate{}),
+			builder.WithPredicates(managedClusterViewStatusChangePredicate, predicate.GenerationChangedPredicate{}),
 		).
 		Complete(r)
 }
@@ -64,17 +65,17 @@ func (r *ManagedClusterReconciler) managedClusterViewRequestMapper(ctx context.C
 	mcv, ok := obj.(*viewv1beta1.ManagedClusterView)
 
 	if !ok {
-		r.Logger.Error("Something unexpected occurred when casting obj into ManagedClusterView")
+		r.Logger.Error("Received object is not a ManagedClusterView object")
 		return []reconcile.Request{}
 	}
-	logger := r.Logger.With("ManagedClusterView", mcv)
+	logger := r.Logger.With("ManagedClusterView", mcv.Name)
 	mcName, ok := mcv.Labels[utils.MCVLabelKey]
 
 	if !ok {
 		return []reconcile.Request{}
 	}
 
-	logger.Info("ManagedClusterView of interest just got updated. Raising a reconcile request for ManagedCluster", "ManagedCluster", mcName)
+	logger.Debug("ManagedClusterView of interest just got updated. Raising a reconcile request for ManagedCluster", "ManagedCluster", mcName)
 
 	return []reconcile.Request{
 		{
@@ -85,39 +86,37 @@ func (r *ManagedClusterReconciler) managedClusterViewRequestMapper(ctx context.C
 	}
 
 }
-func (r *ManagedClusterReconciler) ensureManagedClusterViews(ctx context.Context, clusterName string) error {
+func (r *ManagedClusterReconciler) processManagedClusterViews(ctx context.Context, clusterName string) error {
 	namespace := "openshift-storage"
 	resourceType := "ConfigMap"
-	r.Logger.Info("Processing PeerRef", "ClusterName", clusterName)
-	mcv, err := utils.GetManagedClusterViewByLabel(r.Client, utils.MCVLabelKey, clusterName, clusterName)
+
+	mcv, or, err := utils.CreateOrUpdateManagedClusterView(ctx, r.Client, odfConfigMapName, namespace, resourceType, clusterName)
 	if err != nil {
-		r.Logger.Info("ManagedClusterView does not exist, creating a new one")
-		mcv, err = utils.CreateOrUpdateManagedClusterView(ctx, r.Client, odfConfigMapName, namespace, resourceType, clusterName)
-		if err != nil {
-			r.Logger.Error("Failed to create or update ManagedClusterView", "error", err)
-			return err
-		}
-		r.Logger.Info("ManagedClusterView created or updated successfully", "ManagedClusterView", mcv.Name)
-	} else {
-		desiredSpec := viewv1beta1.ViewSpec{
-			Scope: viewv1beta1.ViewScope{
-				Name:      odfConfigMapName,
-				Namespace: namespace,
-				Resource:  resourceType,
-			},
-		}
-		if mcv.Spec != desiredSpec {
-			r.Logger.Info("ManagedClusterView spec does not match, updating", "ClusterName", clusterName)
-			mcv, err = utils.CreateOrUpdateManagedClusterView(ctx, r.Client, odfConfigMapName, namespace, resourceType, clusterName)
-			if err != nil {
-				r.Logger.Error("Failed to update ManagedClusterView", "error", err)
-				return err
-			}
-			r.Logger.Info("ManagedClusterView updated successfully", "ManagedClusterView", mcv.Name)
-		} else {
-			r.Logger.Info("ManagedClusterView spec matches, no update needed", "ManagedClusterView", mcv.Name)
-		}
+		r.Logger.Error("Failed to create or update ManagedClusterView", "error", err)
+		return err
 	}
+	r.Logger.Info(fmt.Sprintf("ManagedClusterView was %s", or), "ManagedClusterView", mcv.Name)
 
 	return nil
 }
+
+var managedClusterViewStatusChangePredicate = predicate.Funcs{
+	UpdateFunc: func(e event.UpdateEvent) bool {
+		oldObj, okOld := e.ObjectOld.(*viewv1beta1.ManagedClusterView)
+		newObj, okNew := e.ObjectNew.(*viewv1beta1.ManagedClusterView)
+		if !okOld || !okNew {
+			return false
+		}
+
+		return !reflect.DeepEqual(oldObj.Status, newObj.Status)
+	},
+	CreateFunc: func(e event.CreateEvent) bool {
+		return true
+	},
+	DeleteFunc: func(e event.DeleteEvent) bool {
+		return true
+	},
+	GenericFunc: func(e event.GenericEvent) bool {
+		return true
+	},
+}

controllers/managedcluster_controller_test.go

@@ -28,7 +28,6 @@ func TestManagedClusterReconcile(t *testing.T) {
 
 	reconciler := &ManagedClusterReconciler{
 		Client: client,
-		Scheme: scheme,
 		Logger: logger,
 	}
 
@@ -56,7 +55,7 @@ func TestManagedClusterReconcile(t *testing.T) {
 	})
 }
 
-func TestEnsureManagedClusterViews(t *testing.T) {
+func TestProcessManagedClusterViews(t *testing.T) {
 	scheme := runtime.NewScheme()
 	_ = clusterv1.AddToScheme(scheme)
 	_ = viewv1beta1.AddToScheme(scheme)
@@ -66,18 +65,17 @@ func TestEnsureManagedClusterViews(t *testing.T) {
 
 	reconciler := &ManagedClusterReconciler{
 		Client: client,
-		Scheme: scheme,
 		Logger: logger,
 	}
 
 	t.Run("ManagedClusterView does not exist. It should create it", func(t *testing.T) {
-		err := reconciler.ensureManagedClusterViews(context.TODO(), "test-cluster")
+		err := reconciler.processManagedClusterViews(context.TODO(), "test-cluster")
 		assert.NoError(t, err)
 
 		createdMCV := &viewv1beta1.ManagedClusterView{}
-		err = client.Get(context.TODO(), types.NamespacedName{Name: "test-cluster-mcv", Namespace: "test-cluster"}, createdMCV)
+		err = client.Get(context.TODO(), types.NamespacedName{Name: utils.GetManagedClusterViewName("test-cluster"), Namespace: "test-cluster"}, createdMCV)
 		assert.NoError(t, err)
-		assert.Equal(t, "test-cluster-mcv", createdMCV.Name)
+		assert.Equal(t, utils.GetManagedClusterViewName("test-cluster"), createdMCV.Name)
 		assert.Equal(t, "test-cluster", createdMCV.Namespace)
 		assert.Equal(t, "odf-info", createdMCV.Spec.Scope.Name)
 		assert.Equal(t, "openshift-storage", createdMCV.Spec.Scope.Namespace)
@@ -86,7 +84,7 @@ func TestEnsureManagedClusterViews(t *testing.T) {
 	t.Run("ManagedClusterView exists but spec does not match. Desired spec should be reached", func(t *testing.T) {
 		existingMCV := &viewv1beta1.ManagedClusterView{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "test-cluster-mcv",
+				Name:      utils.GetManagedClusterViewName("test-cluster"),
 				Namespace: "test-cluster",
 			},
 			Spec: viewv1beta1.ViewSpec{
@@ -99,19 +97,19 @@ func TestEnsureManagedClusterViews(t *testing.T) {
 		}
 		_ = client.Create(context.TODO(), existingMCV)
 
-		err := reconciler.ensureManagedClusterViews(context.TODO(), "test-cluster")
+		err := reconciler.processManagedClusterViews(context.TODO(), "test-cluster")
 		assert.NoError(t, err)
 
 		updatedMCV := &viewv1beta1.ManagedClusterView{}
-		_ = client.Get(context.TODO(), types.NamespacedName{Name: "test-cluster-mcv", Namespace: "test-cluster"}, updatedMCV)
+		_ = client.Get(context.TODO(), types.NamespacedName{Name: utils.GetManagedClusterViewName("test-cluster"), Namespace: "test-cluster"}, updatedMCV)
 		assert.Equal(t, "odf-info", updatedMCV.Spec.Scope.Name)
 		assert.Equal(t, "openshift-storage", updatedMCV.Spec.Scope.Namespace)
 	})
 
 	t.Run("ManagedClusterView exists and spec matches. Nothing should change. It should be error free", func(t *testing.T) {
 		existingMCV := &viewv1beta1.ManagedClusterView{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "test-cluster-mcv",
+				Name:      utils.GetManagedClusterViewName("test-cluster"),
 				Namespace: "test-cluster",
 			},
 			Spec: viewv1beta1.ViewSpec{
@@ -124,7 +122,7 @@ func TestEnsureManagedClusterViews(t *testing.T) {
 		}
 		_ = client.Create(context.TODO(), existingMCV)
 
-		err := reconciler.ensureManagedClusterViews(context.TODO(), "test-cluster")
+		err := reconciler.processManagedClusterViews(context.TODO(), "test-cluster")
 		assert.NoError(t, err)
 	})
 }

controllers/manager.go

@@ -144,7 +144,6 @@ func (o *ManagerOptions) runManager() {
 
 	if err = (&ManagedClusterReconciler{
 		Client: mgr.GetClient(),
-		Scheme: mgr.GetScheme(),
 		Logger: logger.With("controller", "ManagedClusterReconciler"),
 	}).SetupWithManager(mgr); err != nil {
 		logger.Error("Failed to create ManagedCluster controller", "error", err)

controllers/mirrorpeer_controller.go

@@ -70,8 +70,8 @@ const spokeClusterRoleBindingName = "spoke-clusterrole-bindings"
 //+kubebuilder:rbac:groups=addon.open-cluster-management.io,resources=managedclusteraddons;clustermanagementaddons,verbs=*
 //+kubebuilder:rbac:groups=addon.open-cluster-management.io,resources=managedclusteraddons/status,verbs=*
 //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;create;update;watch
-//+kubebuilder:rbac:groups=view.open-cluster-management.io,resources=managedclusterviews,verbs=*
 //+kubebuilder:rbac:groups=console.openshift.io,resources=consoleplugins,verbs=get;list;create;update;watch
+// +kubebuilder:rbac:groups=view.open-cluster-management.io,resources=managedclusterviews,verbs=get;list;watch;create;update
 //+kubebuilder:rbac:groups=core,resources=services,verbs=get;list;create;update;watch
 //+kubebuilder:rbac:groups=ramendr.openshift.io,resources=drclusters;drpolicies,verbs=get;list;create;update;watch
 //+kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=clusterrolebindings,verbs=get;list;create;update;delete;watch,resourceNames=spoke-clusterrole-bindings

controllers/utils/acm.go

@@ -12,16 +12,21 @@ import (
 )
 
 const MCVLabelKey = "multicluster.odf.openshift.io/cluster"
+const MCVNameTemplate = "odf-multicluster-mcv-%s"
 
-func CreateOrUpdateManagedClusterView(ctx context.Context, client ctrlClient.Client, resourceToFindName string, resourceToFindNamespace string, resourceToFindType string, clusterName string) (*viewv1beta1.ManagedClusterView, error) {
+func GetManagedClusterViewName(clusterName string) string {
+	return fmt.Sprintf(MCVNameTemplate, clusterName)
+}
+
+func CreateOrUpdateManagedClusterView(ctx context.Context, client ctrlClient.Client, resourceToFindName string, resourceToFindNamespace string, resourceToFindType string, clusterName string) (*viewv1beta1.ManagedClusterView, controllerutil.OperationResult, error) {
 	mcv := &viewv1beta1.ManagedClusterView{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%s-mcv", clusterName),
+			Name:      GetManagedClusterViewName(clusterName),
 			Namespace: clusterName,
 		},
 	}
 
-	_, err := controllerutil.CreateOrUpdate(ctx, client, mcv, func() error {
+	or, err := controllerutil.CreateOrUpdate(ctx, client, mcv, func() error {
 		mcv.Spec = viewv1beta1.ViewSpec{
 			Scope: viewv1beta1.ViewScope{
 				Name:      resourceToFindName,
@@ -40,10 +45,10 @@ func CreateOrUpdateManagedClusterView(ctx context.Context, client ctrlClient.Cli
 	})
 
 	if err != nil {
-		return nil, fmt.Errorf("failed to create or update ManagedClusterView: %w", err)
+		return nil, controllerutil.OperationResultNone, err
 	}
 
-	return mcv, nil
+	return mcv, or, nil
 }
 
 func GetManagedClusterViewByLabel(client ctrlClient.Client, labelKey, labelValue, namespace string) (*viewv1beta1.ManagedClusterView, error) {
@@ -55,7 +60,7 @@ func GetManagedClusterViewByLabel(client ctrlClient.Client, labelKey, labelValue
 
 	err := client.List(context.TODO(), mcvList, listOptions)
 	if err != nil {
-		return nil, fmt.Errorf("failed to list ManagedClusterViews: %w", err)
+		return nil, fmt.Errorf("failed to list ManagedClusterViews. %w", err)
 	}
 
 	if len(mcvList.Items) == 0 {
@@ -66,7 +71,5 @@ func GetManagedClusterViewByLabel(client ctrlClient.Client, labelKey, labelValue
 }
 
 func getLabelSelector(labelKey, labelValue string) labels.Selector {
-	return labels.SelectorFromSet(labels.Set{
-		labelKey: labelValue,
-	})
+	return labels.SelectorFromSet(labels.Set{labelKey: labelValue})
 }

controllers/utils/acm_test.go

@@ -18,10 +18,10 @@ func TestCreateOrUpdateManagedClusterView(t *testing.T) {
 	client := fake.NewClientBuilder().WithScheme(s).Build()
 
 	t.Run("Success", func(t *testing.T) {
-		mcv, err := CreateOrUpdateManagedClusterView(context.TODO(), client, "example-configmap", "default", "ConfigMap", "managed-cluster-1")
+		mcv, _, err := CreateOrUpdateManagedClusterView(context.TODO(), client, "example-configmap", "default", "ConfigMap", "managed-cluster-1")
 		assert.NoError(t, err)
 		assert.NotNil(t, mcv)
-		assert.Equal(t, "managed-cluster-1-mcv", mcv.Name)
+		assert.Equal(t, GetManagedClusterViewName("managed-cluster-1"), mcv.Name)
 		assert.Equal(t, "example-configmap", mcv.Spec.Scope.Name)
 		assert.Equal(t, "default", mcv.Spec.Scope.Namespace)
 		assert.Equal(t, "ConfigMap", mcv.Spec.Scope.Resource)