Merge pull request #1629 from matskiv/unique-binding-names

Bug 1855088: generate unique (Cluster)RoleBinding names

Author: openshift-merge-robot

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(), role),
+				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(), role),
+				Name:      role.GetName(),
 				Namespace: csv.GetNamespace(),
 				Labels:    ownerutil.OwnerLabel(csv, v1alpha1.ClusterServiceVersionKind),
 			},

pkg/controller/registry/resolver/rbac_test.go

@@ -1,12 +1,18 @@
 package resolver
 
 import (
-	"github.com/stretchr/testify/require"
 	"math/rand"
 	"reflect"
 	"strings"
 	"testing"
 	"testing/quick"
+
+	"github.com/stretchr/testify/require"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 )
 
 func TestGenerateName(t *testing.T) {
@@ -63,3 +69,167 @@ func TestGeneratesWithinRange(t *testing.T) {
 	}
 	require.NoError(t, quick.Check(f, nil))
 }
+
+func TestRBACForClusterServiceVersion(t *testing.T) {
+	serviceAccount1 := "test-service-account"
+	serviceAccount2 := "second-service-account"
+	csvName := "test-csv.v1.1.0"
+
+	rules := []rbacv1.PolicyRule{
+		{
+			Verbs:     []string{"get"},
+			APIGroups: []string{""},
+			Resources: []string{"pods"},
+		},
+	}
+
+	// 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: "RoleBindings and one ClusterRoleBinding",
+			csv: v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      csvName,
+					Namespace: "test-namespace",
+				},
+				Spec: v1alpha1.ClusterServiceVersionSpec{
+					InstallStrategy: v1alpha1.NamedInstallStrategy{
+						StrategyName: v1alpha1.InstallStrategyNameDeployment,
+						StrategySpec: v1alpha1.StrategyDetailsDeployment{
+							Permissions: []v1alpha1.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: serviceAccount1,
+									Rules:              rules,
+								},
+								{
+									ServiceAccountName: serviceAccount1,
+									Rules: []rbacv1.PolicyRule{
+										{
+											Verbs:     []string{"get"},
+											APIGroups: []string{""},
+											Resources: []string{"deployments"},
+										},
+									},
+								},
+								{
+									ServiceAccountName: serviceAccount2,
+									Rules:              rules,
+								},
+							},
+							ClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: serviceAccount1,
+									Rules:              rules,
+								},
+							},
+						},
+					},
+				},
+			},
+			want: map[string]*OperatorPermissions{
+				serviceAccount1: {
+					RoleBindings:        []*rbacv1.RoleBinding{{}, {}},
+					ClusterRoleBindings: []*rbacv1.ClusterRoleBinding{{}},
+				},
+				serviceAccount2: {
+					RoleBindings: []*rbacv1.RoleBinding{{}},
+				},
+			},
+		},
+		{
+			name: "ClusterRoleBinding",
+			csv: v1alpha1.ClusterServiceVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      csvName,
+					Namespace: "second-namespace",
+				},
+				Spec: v1alpha1.ClusterServiceVersionSpec{
+					InstallStrategy: v1alpha1.NamedInstallStrategy{
+						StrategyName: v1alpha1.InstallStrategyNameDeployment,
+						StrategySpec: v1alpha1.StrategyDetailsDeployment{
+							ClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: serviceAccount1,
+									Rules:              rules,
+								},
+								{
+									ServiceAccountName: serviceAccount2,
+									Rules:              rules,
+								},
+							},
+						},
+					},
+				},
+			},
+			want: map[string]*OperatorPermissions{
+				serviceAccount1: {
+					ClusterRoleBindings: []*rbacv1.ClusterRoleBinding{{}},
+				},
+				serviceAccount2: {
+					ClusterRoleBindings: []*rbacv1.ClusterRoleBinding{{}},
+				},
+			},
+		},
+	}
+
+	// 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{}
+			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))
+				require.Equal(t, len(permissions.ClusterRoleBindings), len(result[serviceAccount].ClusterRoleBindings))
+
+				// Check that testing ServiceAccount is the Subject of RoleBindings
+				for _, roleBinding := range result[serviceAccount].RoleBindings {
+					require.Len(t, roleBinding.Subjects, 1)
+					require.Equal(t, serviceAccount, roleBinding.Subjects[0].Name)
+
+					// Check that RoleBindings are created with unique names
+					_, rbWithNameExists := roleBindingNames[roleBinding.Name]
+					require.False(t, rbWithNameExists, "RoleBinding with the same name already generated")
+					roleBindingNames[roleBinding.Name] = true
+				}
+
+				// Check that testing ServiceAccount is the Subject of ClusterrRoleBindings
+				for _, clusterRoleBinding := range result[serviceAccount].ClusterRoleBindings {
+					require.Len(t, clusterRoleBinding.Subjects, 1)
+					require.Equal(t, serviceAccount, clusterRoleBinding.Subjects[0].Name)
+
+					// Check that ClusterRoleBindings are created with unique names
+					_, crbWithNameExists := clusterRoleBindingNames[clusterRoleBinding.Name]
+					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
+				}
+			}
+		})
+	}
+}