From a4e92f2dac283c310d529bb632f5fce5dec07a9f Mon Sep 17 00:00:00 2001 From: "Shiva Krishna, Merla" Date: Fri, 16 Aug 2024 09:40:02 -0700 Subject: [PATCH] Create a common Role/RoleBinding/ServiceAccount per Namespace Signed-off-by: Shiva Krishna, Merla --- internal/controller/nimcache_controller.go | 176 ++++++++++-------- .../controller/nimcache_controller_test.go | 8 +- internal/utils/utils.go | 16 ++ 3 files changed, 122 insertions(+), 78 deletions(-) diff --git a/internal/controller/nimcache_controller.go b/internal/controller/nimcache_controller.go index 57a3cc36..6d5cbd6f 100644 --- a/internal/controller/nimcache_controller.go +++ b/internal/controller/nimcache_controller.go @@ -61,6 +61,15 @@ const ( // AllProfiles represents all profiles in the NIM manifest AllProfiles = "all" + + // NIMCacheRole is the name of the role for all NIMCache instances in the namespace + NIMCacheRole = "nim-cache-role" + + // NIMCacheRoleBinding is the name of the rolebinding for all NIMCache instances in the namespace + NIMCacheRoleBinding = "nim-cache-rolebinding" + + // NIMCacheServiceAccount is the name of the serviceaccount for all NIMCache instances in the namespace + NIMCacheServiceAccount = "nim-cache-sa" ) // NIMCacheReconciler reconciles a NIMCache object @@ -230,121 +239,146 @@ func (r *NIMCacheReconciler) cleanupNIMCache(ctx context.Context, nimCache *apps func (r *NIMCacheReconciler) reconcileRole(ctx context.Context, nimCache *appsv1alpha1.NIMCache) error { logger := r.GetLogger() - roleName := fmt.Sprintf("%s-role", nimCache.GetName()) + roleName := NIMCacheRole roleNamespacedName := types.NamespacedName{Name: roleName, Namespace: nimCache.GetNamespace()} + // Desired Role configuration + desiredRole := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleName, + Namespace: nimCache.GetNamespace(), + Labels: map[string]string{ + "app": "k8s-nim-operator", + }, + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{"security.openshift.io"}, + Resources: []string{"securitycontextconstraints"}, + ResourceNames: []string{"nonroot"}, + Verbs: []string{"use"}, + }, + }, + } + // Check if the Role already exists - role := &rbacv1.Role{} - err := r.Get(ctx, roleNamespacedName, role) + existingRole := &rbacv1.Role{} + err := r.Get(ctx, roleNamespacedName, existingRole) if err != nil && client.IgnoreNotFound(err) != nil { logger.Error(err, "Failed to get Role", "Name", roleName) return err } - // If Role does not exist, create a new one if err != nil { + // Role does not exist, create a new one logger.Info("Creating a new Role", "Name", roleName) - newRole := &rbacv1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleName, - Namespace: nimCache.GetNamespace(), - Labels: map[string]string{ - "app": nimCache.GetName(), - }, - }, - Rules: []rbacv1.PolicyRule{ - { - APIGroups: []string{"security.openshift.io"}, - Resources: []string{"securitycontextconstraints"}, - ResourceNames: []string{"nonroot"}, - Verbs: []string{"use"}, - }, - }, - } - - // Set the NIMCache instance as the owner and controller of the Role - if err := controllerutil.SetControllerReference(nimCache, newRole, r.GetScheme()); err != nil { - logger.Error(err, "Failed to set owner reference on Role", "Name", roleName) - return err - } - - // Create the Role - err = r.Create(ctx, newRole) + err = r.Create(ctx, desiredRole) if err != nil { logger.Error(err, "Failed to create Role", "Name", roleName) return err } logger.Info("Successfully created Role", "Name", roleName) + } else { + // Role exists, check if it needs to be updated + if !roleEqual(existingRole, desiredRole) { + logger.Info("Updating existing Role", "Name", roleName) + existingRole.Rules = desiredRole.Rules + + err = r.Update(ctx, existingRole) + if err != nil { + logger.Error(err, "Failed to update Role", "Name", roleName) + return err + } + + logger.Info("Successfully updated Role", "Name", roleName) + } } - // If the Role already exists, no action is needed return nil } +// Helper function to check if two Roles are equal +func roleEqual(existing, desired *rbacv1.Role) bool { + return utils.IsEqual(existing, desired, "Rules") +} + func (r *NIMCacheReconciler) reconcileRoleBinding(ctx context.Context, nimCache *appsv1alpha1.NIMCache) error { logger := r.GetLogger() - rbName := fmt.Sprintf("%s-rolebinding", nimCache.GetName()) + rbName := NIMCacheRoleBinding rbNamespacedName := types.NamespacedName{Name: rbName, Namespace: nimCache.GetNamespace()} + // Desired RoleBinding configuration + desiredRB := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: rbName, + Namespace: nimCache.GetNamespace(), + Labels: map[string]string{ + "app": "k8s-nim-operator", + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: NIMCacheRole, + }, + Subjects: []rbacv1.Subject{ + { + Kind: "ServiceAccount", + Name: NIMCacheServiceAccount, + Namespace: nimCache.GetNamespace(), + }, + }, + } + // Check if the RoleBinding already exists - rb := &rbacv1.RoleBinding{} - err := r.Get(ctx, rbNamespacedName, rb) + existingRB := &rbacv1.RoleBinding{} + err := r.Get(ctx, rbNamespacedName, existingRB) if err != nil && client.IgnoreNotFound(err) != nil { logger.Error(err, "Failed to get RoleBinding", "Name", rbName) return err } - // If RoleBinding does not exist, create a new one if err != nil { + // RoleBinding does not exist, create a new one logger.Info("Creating a new RoleBinding", "Name", rbName) - newRB := &rbacv1.RoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: rbName, - Namespace: nimCache.GetNamespace(), - Labels: map[string]string{ - "app": nimCache.GetName(), - }, - }, - RoleRef: rbacv1.RoleRef{ - APIGroup: "rbac.authorization.k8s.io", - Kind: "Role", - Name: fmt.Sprintf("%s-role", nimCache.GetName()), - }, - Subjects: []rbacv1.Subject{ - { - Kind: "ServiceAccount", - Name: nimCache.GetName(), - Namespace: nimCache.GetNamespace(), - }, - }, - } - - // Set the NIMCache instance as the owner and controller of the RoleBinding - if err := controllerutil.SetControllerReference(nimCache, newRB, r.GetScheme()); err != nil { - logger.Error(err, "Failed to set owner reference on RoleBinding", "Name", rbName) - return err - } - - // Create the RoleBinding - err = r.Create(ctx, newRB) + err = r.Create(ctx, desiredRB) if err != nil { logger.Error(err, "Failed to create RoleBinding", "Name", rbName) return err } logger.Info("Successfully created RoleBinding", "Name", rbName) + } else { + // RoleBinding exists, check if it needs to be updated + if !roleBindingEqual(existingRB, desiredRB) { + logger.Info("Updating existing RoleBinding", "Name", rbName) + existingRB.RoleRef = desiredRB.RoleRef + existingRB.Subjects = desiredRB.Subjects + + err = r.Update(ctx, existingRB) + if err != nil { + logger.Error(err, "Failed to update RoleBinding", "Name", rbName) + return err + } + + logger.Info("Successfully updated RoleBinding", "Name", rbName) + } } - // If the RoleBinding already exists, no action is needed return nil } +// Helper function to check if two RoleBindings are equal +func roleBindingEqual(existing, desired *rbacv1.RoleBinding) bool { + return utils.IsEqual(existing, desired, "RoleRef", "Subjects") +} + func (r *NIMCacheReconciler) reconcileServiceAccount(ctx context.Context, nimCache *appsv1alpha1.NIMCache) error { logger := r.GetLogger() - saName := nimCache.GetName() + saName := NIMCacheServiceAccount saNamespacedName := types.NamespacedName{Name: saName, Namespace: nimCache.GetNamespace()} sa := &corev1.ServiceAccount{} @@ -361,16 +395,10 @@ func (r *NIMCacheReconciler) reconcileServiceAccount(ctx context.Context, nimCac ObjectMeta: metav1.ObjectMeta{ Name: saName, Namespace: nimCache.GetNamespace(), - Labels: map[string]string{"app": nimCache.GetName()}, + Labels: map[string]string{"app": "k8s-nim-operator"}, }, } - // Set the NIMCache instance as the owner and controller of the ServiceAccount - if err := controllerutil.SetControllerReference(nimCache, newSA, r.GetScheme()); err != nil { - logger.Error(err, "Failed to set owner reference on ServiceAccount", "Name", saName) - return err - } - // Create the ServiceAccount err = r.Create(ctx, newSA) if err != nil { diff --git a/internal/controller/nimcache_controller_test.go b/internal/controller/nimcache_controller_test.go index 98e306e0..d71c33ff 100644 --- a/internal/controller/nimcache_controller_test.go +++ b/internal/controller/nimcache_controller_test.go @@ -250,7 +250,7 @@ var _ = Describe("NIMCache Controller", func() { // Check if the Role was created role := &rbacv1.Role{} - roleName := types.NamespacedName{Name: "test-nimcache-role", Namespace: "default"} + roleName := types.NamespacedName{Name: NIMCacheRole, Namespace: "default"} err = client.Get(ctx, roleName, role) Expect(err).NotTo(HaveOccurred()) @@ -286,15 +286,15 @@ var _ = Describe("NIMCache Controller", func() { // Check if the RoleBinding was created rb := &rbacv1.RoleBinding{} - rbName := types.NamespacedName{Name: "test-nimcache-rolebinding", Namespace: "default"} + rbName := types.NamespacedName{Name: NIMCacheRoleBinding, Namespace: "default"} err = client.Get(ctx, rbName, rb) Expect(err).NotTo(HaveOccurred()) // Check that the RoleBinding is bound to the correct Role - Expect(rb.RoleRef.Name).To(Equal("test-nimcache-role")) + Expect(rb.RoleRef.Name).To(Equal(NIMCacheRole)) Expect(rb.Subjects).To(HaveLen(1)) Expect(rb.Subjects[0].Kind).To(Equal("ServiceAccount")) - Expect(rb.Subjects[0].Name).To(Equal(nimCache.GetName())) + Expect(rb.Subjects[0].Name).To(Equal(NIMCacheServiceAccount)) Expect(rb.Subjects[0].Namespace).To(Equal(nimCache.GetNamespace())) }) diff --git a/internal/utils/utils.go b/internal/utils/utils.go index e0b18b37..2f7ca1d9 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -27,7 +27,9 @@ import ( "strings" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/util/rand" + "reflect" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -227,3 +229,17 @@ func ContainsElement[T comparable](slice []T, element T) bool { } return false } + +// IsEqual compares two Kubernetes objects based on their relevant fields. +func IsEqual[T client.Object](existing, desired T, fieldsToCompare ...string) bool { + for _, field := range fieldsToCompare { + existingValue := reflect.ValueOf(existing).Elem().FieldByName(field) + desiredValue := reflect.ValueOf(desired).Elem().FieldByName(field) + + if !reflect.DeepEqual(existingValue.Interface(), desiredValue.Interface()) { + return false + } + } + + return true +}