Bug 1855088: generate unique role and binding names When same permission rules are defined for different ServiceAccounts, OLM would generate same name for (Cluster)Role and (Cluster)RoleBinding related to this rules. This change should ensure that unique names are generated, taking into account ServiceAccount name and CSV Name and CSV namespace (for ClusterRole)

Author: matskiv

pkg/controller/registry/resolver/rbac.go

@@ -93,7 +93,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create Role
 		role := &rbacv1.Role{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            generateName(csv.GetName(), permission.Rules),
+				Name:            generateName(fmt.Sprintf("%s-%s", csv.GetName(), permission.ServiceAccountName), []interface{}{csv.GetName(), permission}),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
@@ -105,7 +105,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create RoleBinding
 		roleBinding := &rbacv1.RoleBinding{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            generateName(role.GetName(), permission),
+				Name:            role.GetName(),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
@@ -137,7 +137,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create ClusterRole
 		role := &rbacv1.ClusterRole{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   generateName(csv.GetName(), permission.Rules),
+				Name:   generateName(csv.GetName(), []interface{}{csv.GetName(), csv.GetNamespace(), permission}),
 				Labels: ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},
 			Rules: permission.Rules,
@@ -147,7 +147,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create ClusterRoleBinding
 		roleBinding := &rbacv1.ClusterRoleBinding{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      generateName(role.GetName(), permission),
+				Name:      role.GetName(),
 				Namespace: csv.GetNamespace(),
 				Labels:    ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},

pkg/controller/registry/resolver/rbac_test.go

@@ -70,9 +70,10 @@ func TestGeneratesWithinRange(t *testing.T) {
 	require.NoError(t, quick.Check(f, nil))
 }
 
-func TestRBACBindings(t *testing.T) {
+func TestRBACForClusterServiceVersion(t *testing.T) {
 	serviceAccount1 := "test-service-account"
-	serviceAccount2 := "second-account"
+	serviceAccount2 := "second-service-account"
+	csvName := "test-csv.v1.1.0"
 
 	rules := []rbacv1.PolicyRule{
 		{
@@ -82,16 +83,20 @@ func TestRBACBindings(t *testing.T) {
 		},
 	}
 
+	// Note: two CSVs have same name and permissions for a cluster role, this is chosen intentionally,
+	// to verify that ClusterRole and ClusterRoleBinding have different names when the same CSV is installed
+	// twice in the same cluster, but in different namespaces.
 	tests := []struct {
 		name string
 		csv  v1alpha1.ClusterServiceVersion
 		want map[string]*OperatorPermissions
 	}{
 		{
-			name: "RoleBinding",
+			name: "RoleBindings and one ClusterRoleBinding",
 			csv: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "test-csv-1.1.0",
+					Name:      csvName,
+					Namespace: "test-namespace",
 				},
 				Spec: v1alpha1.ClusterServiceVersionSpec{
 					InstallStrategy: v1alpha1.NamedInstallStrategy{
@@ -117,13 +122,20 @@ func TestRBACBindings(t *testing.T) {
 									Rules:              rules,
 								},
 							},
+							ClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: serviceAccount1,
+									Rules:              rules,
+								},
+							},
 						},
 					},
 				},
 			},
 			want: map[string]*OperatorPermissions{
 				serviceAccount1: {
-					RoleBindings: []*rbacv1.RoleBinding{{}, {}},
+					RoleBindings:        []*rbacv1.RoleBinding{{}, {}},
+					ClusterRoleBindings: []*rbacv1.ClusterRoleBinding{{}},
 				},
 				serviceAccount2: {
 					RoleBindings: []*rbacv1.RoleBinding{{}},
@@ -134,7 +146,8 @@ func TestRBACBindings(t *testing.T) {
 			name: "ClusterRoleBinding",
 			csv: v1alpha1.ClusterServiceVersion{
 				ObjectMeta: metav1.ObjectMeta{
-					Name: "second-csv-1.1.0",
+					Name:      csvName,
+					Namespace: "second-namespace",
 				},
 				Spec: v1alpha1.ClusterServiceVersionSpec{
 					InstallStrategy: v1alpha1.NamedInstallStrategy{
@@ -164,13 +177,18 @@ func TestRBACBindings(t *testing.T) {
 			},
 		},
 	}
+
+	// declared here to verify that names are unique when same csv is install in different namespaces
+	clusterRoleBindingNames := map[string]bool{}
+	clusterRolesNames := map[string]bool{}
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			result, err := RBACForClusterServiceVersion(&tt.csv)
 			require.NoError(t, err)
 
 			roleBindingNames := map[string]bool{}
-			clusterRoleBindingNames := map[string]bool{}
+			rolesNames := map[string]bool{}
 			for serviceAccount, permissions := range tt.want {
 				// Check that correct number of bindings is created
 				require.Equal(t, len(permissions.RoleBindings), len(result[serviceAccount].RoleBindings))
@@ -197,6 +215,20 @@ func TestRBACBindings(t *testing.T) {
 					require.False(t, crbWithNameExists, "ClusterRoleBinding with the same name already generated")
 					clusterRoleBindingNames[clusterRoleBinding.Name] = true
 				}
+
+				// Check that Roles are created with unique names
+				for _, role := range result[serviceAccount].Roles {
+					_, roleWithNameExists := rolesNames[role.Name]
+					require.False(t, roleWithNameExists, "Role with the same name already generated")
+					rolesNames[role.Name] = true
+				}
+
+				// Check that ClusterRoles are created with unique names
+				for _, clusterRole := range result[serviceAccount].ClusterRoles {
+					_, crWithNameExists := clusterRolesNames[clusterRole.Name]
+					require.False(t, crWithNameExists, "ClusterRole with the same name already generated")
+					clusterRolesNames[clusterRole.Name] = true
+				}
 			}
 		})
 	}