From 924779dddace863c59ff234f4e142cf1e75e8f0a Mon Sep 17 00:00:00 2001 From: vbadrina Date: Tue, 9 Jul 2024 13:52:19 +0530 Subject: [PATCH] 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 --- ...er-orchestrator.clusterserviceversion.yaml | 8 +++-- config/rbac/role.yaml | 6 +++- controllers/managedcluster_controller.go | 36 +++++++++++++++---- controllers/managedcluster_controller_test.go | 18 +++++----- controllers/mirrorpeer_controller.go | 2 +- controllers/utils/acm.go | 15 ++++---- controllers/utils/acm_test.go | 2 +- 7 files changed, 60 insertions(+), 27 deletions(-) diff --git a/bundle/manifests/odf-multicluster-orchestrator.clusterserviceversion.yaml b/bundle/manifests/odf-multicluster-orchestrator.clusterserviceversion.yaml index dda49127..3aa12f64 100644 --- a/bundle/manifests/odf-multicluster-orchestrator.clusterserviceversion.yaml +++ b/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: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 329c7e81..efee82c3 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -166,7 +166,11 @@ rules: resources: - managedclusterviews verbs: - - '*' + - create + - get + - list + - update + - watch - apiGroups: - work.open-cluster-management.io resources: diff --git a/controllers/managedcluster_controller.go b/controllers/managedcluster_controller.go index 239951d6..80042caf 100644 --- a/controllers/managedcluster_controller.go +++ b/controllers/managedcluster_controller.go @@ -3,6 +3,7 @@ package controllers import ( "context" "log/slog" + "reflect" "github.com/red-hat-storage/odf-multicluster-orchestrator/controllers/utils" viewv1beta1 "github.com/stolostron/multicloud-operators-foundation/pkg/apis/view/v1beta1" @@ -12,6 +13,7 @@ import ( 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" @@ -37,7 +39,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 +57,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 +66,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,10 +87,9 @@ 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) if err != nil { r.Logger.Info("ManagedClusterView does not exist, creating a new one") @@ -121,3 +122,24 @@ func (r *ManagedClusterReconciler) ensureManagedClusterViews(ctx context.Context 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 + }, +} diff --git a/controllers/managedcluster_controller_test.go b/controllers/managedcluster_controller_test.go index 089f6905..145e89c5 100644 --- a/controllers/managedcluster_controller_test.go +++ b/controllers/managedcluster_controller_test.go @@ -56,7 +56,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) @@ -71,13 +71,13 @@ func TestEnsureManagedClusterViews(t *testing.T) { } 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 +86,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,11 +99,11 @@ 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) }) @@ -111,7 +111,7 @@ func TestEnsureManagedClusterViews(t *testing.T) { 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 +124,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) }) } diff --git a/controllers/mirrorpeer_controller.go b/controllers/mirrorpeer_controller.go index f59a3123..fa3d83a0 100644 --- a/controllers/mirrorpeer_controller.go +++ b/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 diff --git a/controllers/utils/acm.go b/controllers/utils/acm.go index 4405831e..f98a06df 100644 --- a/controllers/utils/acm.go +++ b/controllers/utils/acm.go @@ -12,11 +12,16 @@ import ( ) const MCVLabelKey = "multicluster.odf.openshift.io/cluster" +const MCVNameTemplate = "odf-multicluster-mcv-%s" + +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, error) { mcv := &viewv1beta1.ManagedClusterView{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%s-mcv", clusterName), + Name: GetManagedClusterViewName(clusterName), Namespace: clusterName, }, } @@ -40,7 +45,7 @@ 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, fmt.Errorf("failed to create or update ManagedClusterView. %w", err) } return mcv, nil @@ -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}) } diff --git a/controllers/utils/acm_test.go b/controllers/utils/acm_test.go index 2b2a74d9..f82a119e 100644 --- a/controllers/utils/acm_test.go +++ b/controllers/utils/acm_test.go @@ -21,7 +21,7 @@ func TestCreateOrUpdateManagedClusterView(t *testing.T) { 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)