Merge pull request #687 from ecordell/fix-perms

fix(permissions): Generate unique Names for permissions

Author: openshift-merge-robot

Makefile

@@ -110,7 +110,7 @@ container:
 clean-e2e:
 	kubectl delete crds --all
 	kubectl delete apiservices.apiregistration.k8s.io v1alpha1.packages.apps.redhat.com
-	kubectl delete -f test/e2e/resources/0000_30_00-namespace.yaml
+	kubectl delete -f test/e2e/resources/0000_50_00-namespace.yaml
 
 clean:
 	@rm -rf cover.out

pkg/controller/registry/resolver/rbac.go

@@ -6,12 +6,17 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apiserver/pkg/storage/names"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
+var generateName = func(base string) string {
+	return names.SimpleNameGenerator.GenerateName(base + "-")
+}
+
 type OperatorPermissions struct {
 	ServiceAccount      *corev1.ServiceAccount
 	Roles               []*rbacv1.Role
@@ -63,7 +68,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 	}
 
 	// Resolve Permissions
-	for i, permission := range strategyDetailsDeployment.Permissions {
+	for _, permission := range strategyDetailsDeployment.Permissions {
 		// Create ServiceAccount if necessary
 		if _, ok := permissions[permission.ServiceAccountName]; !ok {
 			serviceAccount := &corev1.ServiceAccount{}
@@ -76,7 +81,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create Role
 		role := &rbacv1.Role{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            fmt.Sprintf("%s-%d", csv.GetName(), i),
+				Name:            generateName(csv.GetName()),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv),
@@ -88,7 +93,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create RoleBinding
 		roleBinding := &rbacv1.RoleBinding{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName),
+				Name:            generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv),
@@ -107,7 +112,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 	}
 
 	// Resolve ClusterPermissions as StepResources
-	for i, permission := range strategyDetailsDeployment.ClusterPermissions {
+	for _, permission := range strategyDetailsDeployment.ClusterPermissions {
 		// Create ServiceAccount if necessary
 		if _, ok := permissions[permission.ServiceAccountName]; !ok {
 			serviceAccount := &corev1.ServiceAccount{}
@@ -120,7 +125,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create ClusterRole
 		role := &rbacv1.ClusterRole{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            fmt.Sprintf("%s-%d", csv.GetName(), i),
+				Name:            generateName(csv.GetName()),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv),
 			},
@@ -131,7 +136,7 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri
 		// Create ClusterRoleBinding
 		roleBinding := &rbacv1.ClusterRoleBinding{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:            fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName),
+				Name:            generateName(fmt.Sprintf("%s-%s", role.GetName(), permission.ServiceAccountName)),
 				Namespace:       csv.GetNamespace(),
 				OwnerReferences: []metav1.OwnerReference{ownerutil.NonBlockingOwner(csv)},
 				Labels:          ownerutil.OwnerLabel(csv),

pkg/controller/registry/resolver/resolver_test.go

@@ -348,6 +348,10 @@ func TestNamespaceResolver(t *testing.T) {
 }
 
 func TestNamespaceResolverRBAC(t *testing.T) {
+	generateName = func(base string) string {
+		return "a"
+	}
+
 	namespace := "catsrc-namespace"
 	catalog := CatalogKey{"catsrc", namespace}
 
@@ -363,7 +367,7 @@ func TestNamespaceResolverRBAC(t *testing.T) {
 			},
 		},
 	}
-
+	bundle := bundleWithPermissions("a.v1", "a", "alpha", "", nil, nil, nil, nil, simplePermissions, simplePermissions)
 	type out struct {
 		steps [][]*v1alpha1.Step
 		subs  []*v1alpha1.Subscription
@@ -380,12 +384,10 @@ func TestNamespaceResolverRBAC(t *testing.T) {
 			clusterState: []runtime.Object{
 				newSub(namespace, "a", "alpha", catalog),
 			},
-			bundlesInCatalog: []*opregistry.Bundle{
-				bundleWithPermissions("a.v1", "a", "alpha", "", nil, nil, nil, nil, simplePermissions, simplePermissions),
-			},
+			bundlesInCatalog: []*opregistry.Bundle{bundle},
 			out: out{
 				steps: [][]*v1alpha1.Step{
-					bundleSteps(bundleWithPermissions("a.v1", "a", "alpha", "", nil, nil, nil, nil, simplePermissions, simplePermissions), namespace, catalog),
+					bundleSteps(bundle, namespace, catalog),
 				},
 				subs: []*v1alpha1.Subscription{
 					updatedSub(namespace, "a.v1", "a", "alpha", catalog),

test/e2e/installplan_e2e_test.go

@@ -687,13 +687,13 @@ func TestCreateInstallPlanWithPermissions(t *testing.T) {
 
 	// Expect correct RBAC resources to be resolved and created
 	expectedSteps := map[registry.ResourceKey]struct{}{
-		registry.ResourceKey{Name: crd.Name, Kind: "CustomResourceDefinition"}:                                            {},
-		registry.ResourceKey{Name: stableCSVName, Kind: "ClusterServiceVersion"}:                                          {},
-		registry.ResourceKey{Name: serviceAccountName, Kind: "ServiceAccount"}:                                            {},
-		registry.ResourceKey{Name: fmt.Sprintf("%s-0", stableCSVName), Kind: "Role"}:                                      {},
-		registry.ResourceKey{Name: fmt.Sprintf("%s-0-%s", stableCSVName, serviceAccountName), Kind: "RoleBinding"}:        {},
-		registry.ResourceKey{Name: fmt.Sprintf("%s-0", stableCSVName), Kind: "ClusterRole"}:                               {},
-		registry.ResourceKey{Name: fmt.Sprintf("%s-0-%s", stableCSVName, serviceAccountName), Kind: "ClusterRoleBinding"}: {},
+		registry.ResourceKey{Name: crd.Name, Kind: "CustomResourceDefinition"}:   {},
+		registry.ResourceKey{Name: stableCSVName, Kind: "ClusterServiceVersion"}: {},
+		registry.ResourceKey{Name: serviceAccountName, Kind: "ServiceAccount"}:   {},
+		registry.ResourceKey{Name: stableCSVName, Kind: "Role"}:                  {},
+		registry.ResourceKey{Name: stableCSVName, Kind: "RoleBinding"}:           {},
+		registry.ResourceKey{Name: stableCSVName, Kind: "ClusterRole"}:           {},
+		registry.ResourceKey{Name: stableCSVName, Kind: "ClusterRoleBinding"}:    {},
 	}
 
 	require.Equal(t, len(expectedSteps), len(fetchedInstallPlan.Status.Plan), "number of expected steps does not match installed")
@@ -703,15 +703,19 @@ func TestCreateInstallPlanWithPermissions(t *testing.T) {
 			Name: step.Resource.Name,
 			Kind: step.Resource.Kind,
 		}
-		_, ok := expectedSteps[key]
-		require.True(t, ok)
-
-		// Remove the entry from the expected steps set (to ensure no duplicates in resolved plan)
-		delete(expectedSteps, key)
+		for expected := range expectedSteps {
+			if expected == key {
+				delete(expectedSteps, expected)
+			} else if strings.HasPrefix(key.Name, expected.Name) && key.Kind == expected.Kind {
+				delete(expectedSteps, expected)
+			} else {
+				t.Logf("%v, %v: %v && %v", key, expected, strings.HasPrefix(key.Name, expected.Name), key.Kind == expected.Kind)
+			}
+		}
 	}
 
 	// Should have removed every matching step
-	require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected")
+	require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected: %#v", expectedSteps)
 
 }