code review comments addressed

m00g3n · Aug 13, 2024 · e0c541d · e0c541d
1 parent 6b8df6f
commit e0c541d
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 34 deletions.
diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go b/internal/controller/runtime/fsm/runtime_fsm_apply_clusterrolebindings.go
@@ -81,9 +81,11 @@ var GetShootClient = func(ctx context.Context,
 	return shootClientWithAdmin, nil
 }
 
-func isRBACUserKind(s rbacv1.Subject) bool {
-	return s.Kind == rbacv1.UserKind &&
-		s.APIGroup == rbacv1.GroupName
+func isRBACUserKindOneOf(names []string) func(rbacv1.Subject) bool {
+	return func(s rbacv1.Subject) bool {
+		return s.Kind == rbacv1.UserKind &&
+			slices.Contains(names, s.Name)
+	}
 }
 
 func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rbacv1.ClusterRoleBinding) {
@@ -94,16 +96,16 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb
 			continue
 		}
 
-		index := slices.IndexFunc(crb.Subjects, isRBACUserKind)
-		if index < 0 {
-			// cluster role binding does not contain user subject
+		if crb.RoleRef.Kind != "ClusterRole" && crb.RoleRef.Name != "cluster-admin" {
 			continue
 		}
 
-		subjectUserName := crb.Subjects[index].Name
-		if slices.Contains(admins, subjectUserName) {
+		index := slices.IndexFunc(crb.Subjects, isRBACUserKindOneOf(admins))
+		if index >= 0 {
+			// cluster role binding does not contain user subject
 			continue
 		}
+
 		// administrator was removed
 		removed = append(removed, crb)
 	}
@@ -114,15 +116,8 @@ func getRemoved(crbs []rbacv1.ClusterRoleBinding, admins []string) (removed []rb
 //nolint:gochecknoglobals
 var newContainsAdmin = func(admin string) func(rbacv1.ClusterRoleBinding) bool {
 	return func(r rbacv1.ClusterRoleBinding) bool {
-		for _, subject := range r.Subjects {
-			if !isRBACUserKind(subject) || subject.Name != admin {
-				continue
-			}
-			// admin found
-			return true
-		}
-		// admin not found in the slice
-		return false
+		isAdmin := isRBACUserKindOneOf([]string{admin})
+		return slices.ContainsFunc(r.Subjects, isAdmin)
 	}
 }
 

diff --git a/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go b/internal/controller/runtime/fsm/runtime_fsm_apply_crb_test.go
@@ -21,23 +21,6 @@ var _ = Describe(`runtime_fsm_apply_crb`, Label("applyCRB"), func() {
 
 	var testErr = fmt.Errorf("test error")
 
-	DescribeTable("isRBACUserKind",
-		func(s rbacv1.Subject, expected bool) {
-			actual := isRBACUserKind(s)
-			Expect(actual).To(Equal(expected))
-		},
-		Entry("shoud detect if subject is not user kind", rbacv1.Subject{}, false),
-		Entry("shoud detect if subject is from invalid group",
-			rbacv1.Subject{
-				Kind: rbacv1.UserKind,
-			}, false),
-		Entry("shoud detect if subject user from valid group",
-			rbacv1.Subject{
-				APIGroup: rbacv1.GroupName,
-				Kind:     rbacv1.UserKind,
-			}, true),
-	)
-
 	DescribeTable("getMissing",
 		func(tc tcGetCRB) {
 			actual := getMissing(tc.crbs, tc.admins)