CCO-430: Use per-project custom roles instead of per-cluster custom roles

pkg/cmd/provisioning/gcp/create_all.go

@@ -60,7 +60,7 @@ func createAllCmd(cmd *cobra.Command, args []string) {
 // validationForCreateAllCmd will validate the arguments to the command, ensure the destination directory
 // is ready to receive the generated files, and will create the directory if necessary.
 func validationForCreateAllCmd(cmd *cobra.Command, args []string) {
-	if len(CreateWorkloadIdentityPoolOpts.Name) > 32 {
+	if len(CreateAllOpts.Name) > 32 {
 		log.Fatalf("Name can be at most 32 characters long")
 	}
 

pkg/cmd/provisioning/gcp/create_service_accounts.go

@@ -128,14 +128,17 @@ func createServiceAccount(ctx context.Context, client gcp.Client, name string, c
 	}
 
 	// The role ID has a max length of 64 chars and can include only letters, numbers, period and underscores
-	// we sanitize infraName and crName to make them alphanumeric and then
-	// split role ID into 29_28_5 where the resulting string becomes:
-	// <infraName chopped to 29 chars>_<crName chopped to 28 chars>_<random 5 chars>
-	roleID, err := actuator.GenerateRoleID(name, credReq.Name)
+	// we sanitize projectName and crName to make them alphanumeric and then
+	// split role ID into 32_31 where the resulting string becomes:
+	// <projectName chopped to 32 chars>_<crName chopped to 31 chars>
+	roleID, err := actuator.GenerateRoleID(client.GetProjectName(), credReq.Name)
+	if err != nil {
+		return "", fmt.Errorf("error generating custom role id: %v", err)
+	}
 
 	// The role name field has a 100 char max, so generate a name consisting of the
-	// infraName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
-	roleName, err := utils.GenerateNameWithFieldLimits(name, 50, credReq.Name, 49)
+	// projectName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
+	roleName, err := actuator.GenerateRoleName(client.GetProjectName(), credReq.Name)
 	if err != nil {
 		return "", fmt.Errorf("error generating custom role name: %v", err)
 	}
@@ -158,8 +161,7 @@ func createServiceAccount(ctx context.Context, client gcp.Client, name string, c
 	identityProviderBindingNames := getIdentityProviderBindingNames(projectNum, workloadIdentityPool, credReq.Spec.SecretRef.Namespace, credReq.Spec.ServiceAccountNames)
 
 	var encodedCredentialsConfig string
-	switch generateOnly {
-	case true:
+	if generateOnly {
 		// Create shell script to create IAM service account
 		createSvcAcctScript := createShellScript([]string{
 			fmt.Sprintf(createServiceAccountCmd, serviceAccountID, serviceAccountName),
@@ -227,8 +229,7 @@ func createServiceAccount(ctx context.Context, client gcp.Client, name string, c
 		}
 
 		return "", nil
-
-	default:
+	} else {
 		createdByCcoctlForSvcAcct := fmt.Sprintf("%s for service account %s", createdByCcoctl, serviceAccountName)
 
 		var serviceAccount *iamadminpb.ServiceAccount
@@ -264,8 +265,15 @@ func createServiceAccount(ctx context.Context, client gcp.Client, name string, c
 				}
 			} else {
 				log.Printf("Existing IAM custom role %s found, updating permissions", role.Title)
-				if !actuator.AreSlicesEqualWithoutOrder(role.IncludedPermissions, gcpProviderSpec.Permissions) {
-					role.IncludedPermissions = gcpProviderSpec.Permissions
+				addedPermissions, removedPermissions := actuator.CalculateSliceDiff(role.IncludedPermissions, gcpProviderSpec.Permissions)
+
+				if len(removedPermissions) > 0 {
+					allRemovedPermissions := strings.Join(removedPermissions, ", ")
+					log.Printf("Unexpected permissions found on existing custom role %s: %s", role.Title, allRemovedPermissions)
+				}
+
+				if len(addedPermissions) > 0 {
+					role.IncludedPermissions = append(role.IncludedPermissions, addedPermissions...)
 					_, err := actuator.UpdateRole(client, role, role.Name)
 					if err != nil {
 						return "", errors.Wrapf(err, "Failed to update custom role %s", role.Title)
@@ -439,6 +447,10 @@ func createServiceAccountsCmd(cmd *cobra.Command, args []string) {
 // initEnvForCreateServiceAccountsCmd will ensure the destination directory is ready to receive the generated
 // files, and will create the directory if necessary.
 func initEnvForCreateServiceAccountsCmd(cmd *cobra.Command, args []string) {
+	if len(CreateServiceAccountsOpts.Name) > 32 {
+		log.Fatalf("Name can be at most 32 characters long")
+	}
+
 	if CreateServiceAccountsOpts.TargetDir == "" {
 		pwd, err := os.Getwd()
 		if err != nil {

pkg/cmd/provisioning/gcp/create_service_accounts_test.go

@@ -75,7 +75,7 @@ func TestCreateServiceAccounts(t *testing.T) {
 			generateOnly: true,
 			mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient {
 				mockGCPClient := mockgcp.NewMockClient(mockCtrl)
-				mockGetProjectName(mockGCPClient, 1)
+				mockGetProjectName(mockGCPClient, 3)
 				mockGetProject(mockGCPClient)
 				return mockGCPClient
 			},
@@ -106,7 +106,7 @@ func TestCreateServiceAccounts(t *testing.T) {
 				mockListServiceAccountsEmpty(mockGCPClient)
 				mockListRolesEmpty(mockGCPClient)
 				mockCreateServiceAccountSuccessful(mockGCPClient)
-				mockGetProjectName(mockGCPClient, 6)
+				mockGetProjectName(mockGCPClient, 8)
 				mockGetProject(mockGCPClient)
 				mockGetProjectIamPolicy(mockGCPClient)
 				mockSetProjectIamPolicy(mockGCPClient)
@@ -140,7 +140,7 @@ func TestCreateServiceAccounts(t *testing.T) {
 			generateOnly: false,
 			mockGCPClient: func(mockCtrl *gomock.Controller) *mockgcp.MockClient {
 				mockGCPClient := mockgcp.NewMockClient(mockCtrl)
-				mockGetProjectName(mockGCPClient, 2)
+				mockGetProjectName(mockGCPClient, 4)
 				mockGetProject(mockGCPClient)
 				mockListServiceAccountsEmpty(mockGCPClient)
 				mockCreateServiceAccountFailed(mockGCPClient)
@@ -164,7 +164,7 @@ func TestCreateServiceAccounts(t *testing.T) {
 				mockGCPClient := mockgcp.NewMockClient(mockCtrl)
 				mockListServiceAccountsNotEmpty(mockGCPClient)
 				mockListRolesNotEmpty(mockGCPClient)
-				mockGetProjectName(mockGCPClient, 6)
+				mockGetProjectName(mockGCPClient, 8)
 				mockGetProject(mockGCPClient)
 				mockGetProjectIamPolicy(mockGCPClient)
 				mockSetProjectIamPolicy(mockGCPClient)
@@ -290,7 +290,7 @@ func mockListRolesNotEmpty(mockGCPClient *mockgcp.MockClient) {
 		&iamadminpb.ListRolesResponse{
 			Roles: []*iamadminpb.Role{
 				{
-					Title: fmt.Sprintf("%s-%s", testName, testCredReqName),
+					Title: fmt.Sprintf("%s-%s", testProject, testCredReqName),
 				},
 			},
 		}, nil).Times(1)

pkg/cmd/provisioning/gcp/create_workload_identity_provider.go

@@ -248,7 +248,7 @@ func createIdentityProvider(ctx context.Context, client gcp.Client, name, projec
 // validationForCreateWorkloadIdentityProviderCmd will validate the arguments to the command, ensure the destination directory
 // is ready to receive the generated files, and will create the directory if necessary.
 func validationForCreateWorkloadIdentityProviderCmd(cmd *cobra.Command, args []string) {
-	if len(CreateWorkloadIdentityPoolOpts.Name) > 32 {
+	if len(CreateWorkloadIdentityProviderOpts.Name) > 32 {
 		log.Fatalf("Name can be at most 32 characters long")
 	}
 

pkg/cmd/provisioning/gcp/delete.go

@@ -16,10 +16,16 @@ import (
 	"github.com/openshift/cloud-credential-operator/pkg/operator/utils"
 )
 
+// Extend the options struct with an extra option
+type deleteOptions struct {
+	options
+	ForceDeleteCustomRoles bool
+}
+
 var (
 	// DeleteOpts captures the options that affect deletion
 	// of the generated objects.
-	DeleteOpts = options{}
+	DeleteOpts = deleteOptions{}
 )
 
 // deleteOIDCObjectsFromBucket deletes the objects in OIDC cloud storage bucket
@@ -99,7 +105,7 @@ func deleteServiceAccounts(ctx context.Context, client gcp.Client, namePrefix, c
 }
 
 // deleteCustomRoles deletes the IAM custom roles created by ccoctl
-func deleteCustomRoles(ctx context.Context, client gcp.Client, namePrefix, credReqDir string) error {
+func deleteCustomRoles(ctx context.Context, client gcp.Client, credReqDir string) error {
 	projectName := client.GetProjectName()
 	projectResourceName := fmt.Sprintf("projects/%s", projectName)
 
@@ -113,8 +119,8 @@ func deleteCustomRoles(ctx context.Context, client gcp.Client, namePrefix, credR
 	for _, cr := range credReqs {
 		// Generate role name from credentials request to fetch custom role if it exists
 		// The role name field has a 100 char max, so generate a name consisting of the
-		// infraName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
-		roleNameFromCredReq, err := utils.GenerateNameWithFieldLimits(namePrefix, 50, cr.Name, 49)
+		// projectName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
+		roleNameFromCredReq, err := actuator.GenerateRoleName(client.GetProjectName(), cr.Name)
 		if err != nil {
 			return errors.Wrapf(err, "Failed to generate custom role name from credentils request %s", cr.Name)
 		}
@@ -200,8 +206,10 @@ func deleteCmd(cmd *cobra.Command, args []string) {
 		log.Print(err)
 	}
 
-	if err := deleteCustomRoles(ctx, gcpClient, DeleteOpts.Name, DeleteOpts.CredRequestDir); err != nil {
-		log.Print(err)
+	if DeleteOpts.ForceDeleteCustomRoles {
+		if err := deleteCustomRoles(ctx, gcpClient, DeleteOpts.CredRequestDir); err != nil {
+			log.Print(err)
+		}
 	}
 
 	if err := deleteServiceAccounts(ctx, gcpClient, DeleteOpts.Name, DeleteOpts.CredRequestDir); err != nil {
@@ -213,13 +221,21 @@ func deleteCmd(cmd *cobra.Command, args []string) {
 	}
 }
 
+// validationForDeleteCmd will validate the arguments to the command
+func validationForDeleteCmd(cmd *cobra.Command, args []string) {
+	if len(DeleteOpts.Name) > 32 {
+		log.Fatalf("Name can be at most 32 characters long")
+	}
+}
+
 // NewDeleteCmd implements the "delete" command for the credentials provisioning
 func NewDeleteCmd() *cobra.Command {
 	deleteCmd := &cobra.Command{
-		Use:   "delete",
-		Short: "Delete credentials objects",
-		Long:  "Deleting objects related to cloud credentials",
-		Run:   deleteCmd,
+		Use:              "delete",
+		Short:            "Delete credentials objects",
+		Long:             "Deleting objects related to cloud credentials",
+		Run:              deleteCmd,
+		PersistentPreRun: validationForDeleteCmd,
 	}
 
 	deleteCmd.PersistentFlags().StringVar(&DeleteOpts.Name, "name", "", "User-defined name for all created google cloud resources (can be separate from the cluster's infra-id)")
@@ -228,6 +244,7 @@ func NewDeleteCmd() *cobra.Command {
 	deleteCmd.MarkPersistentFlagRequired("project")
 	deleteCmd.PersistentFlags().StringVar(&DeleteOpts.CredRequestDir, "credentials-requests-dir", "", "Directory containing files of CredentialsRequests to delete IAM Roles for (can be created by running 'oc adm release extract --credentials-requests --cloud=ibmcloud' against an OpenShift release image)")
 	deleteCmd.MarkPersistentFlagRequired("credentials-requests-dir")
+	deleteCmd.PersistentFlags().BoolVar(&DeleteOpts.ForceDeleteCustomRoles, "force-delete-custom-roles", false, "Delete per-project custom roles")
 
 	return deleteCmd
 }

pkg/gcp/actuator/actuator.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 
 	log "github.com/sirupsen/logrus"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -106,14 +107,7 @@ func (a *Actuator) Delete(ctx context.Context, cr *minterv1.CredentialsRequest)
 		return err
 	}
 
-	if gcpStatus.RoleID != "" {
-		logger.Infof("deleting custom role %s from GCP", gcpStatus.RoleID)
-		roleName := fmt.Sprintf("projects/%s/roles/%s", a.ProjectName, gcpStatus.RoleID)
-		_, err := DeleteRole(gcpClient, roleName)
-		if err != nil {
-			return fmt.Errorf("failed to delete custom role %s: %v", gcpStatus.RoleID, err)
-		}
-	}
+	// Do not delete per-project custom roles, as they may be used by other clusters within the project
 
 	svcAcct, err := GetServiceAccount(gcpClient, gcpStatus.ServiceAccountID)
 	if err != nil {
@@ -319,15 +313,15 @@ func (a *Actuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest
 
 	if gcpStatus.RoleID == "" && len(gcpSpec.Permissions) > 0 {
 		// The role ID has a max length of 64 chars and can include only letters, numbers, period and underscores
-		// we sanitize infraName and crName to make them alphanumeric and then
-		// split role ID into 29_28_5 where the resulting string becomes:
-		// <infraName chopped to 29 chars>_<crName chopped to 28 chars>_<random 5 chars>
-		roleID, err := GenerateRoleID(infraName, cr.Name)
+		// we sanitize projectName and crName to make them alphanumeric and then
+		// split role ID into 32_31 where the resulting string becomes:
+		// <projectName chopped to 32 chars>_<crName chopped to 31 chars>
+		roleID, err := GenerateRoleID(a.ProjectName, cr.Name)
 		if err != nil {
 			return fmt.Errorf("error generating role ID: %v", err)
 		}
 		gcpStatus.RoleID = roleID
-		logger.WithField("role", gcpStatus.RoleID).Info("generated random ID for GCP custom role")
+		logger.WithField("role", gcpStatus.RoleID).Info("generated ID for GCP custom role")
 
 	}
 
@@ -414,8 +408,8 @@ func (a *Actuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest
 			logger.WithField("role", gcpStatus.RoleID).Debug("custom role does not exist, creating")
 
 			// The role name field has a 100 char max, so generate a name consisting of the
-			// infraName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
-			roleName, err := utils.GenerateNameWithFieldLimits(infraName, 50, cr.Name, 49)
+			// projectName chopped to 50 chars + the crName chopped to 49 chars (separated by a '-').
+			roleName, err := GenerateRoleName(a.ProjectName, cr.Name)
 			if err != nil {
 				return fmt.Errorf("error generating custom role name: %v", err)
 			}
@@ -426,9 +420,16 @@ func (a *Actuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequest
 			}
 			roles = append(roles, role.Name)
 		} else {
-			if !AreSlicesEqualWithoutOrder(role.IncludedPermissions, gcpSpec.Permissions) {
+			addedPermissions, removedPermissions := CalculateSliceDiff(role.IncludedPermissions, gcpSpec.Permissions)
+
+			if len(removedPermissions) > 0 {
+				allRemovedPermissions := strings.Join(removedPermissions, ", ")
+				log.Printf("Unexpected permissions found on existing custom role %s: %s", role.Title, allRemovedPermissions)
+			}
+
+			if len(addedPermissions) > 0 {
 				logger.WithField("role", gcpStatus.RoleID).Info("custom role exists, updating the permissions")
-				role.IncludedPermissions = gcpSpec.Permissions
+				role.IncludedPermissions = append(role.IncludedPermissions, addedPermissions...)
 				_, err := UpdateRole(rootGCPClient, role, role.Name)
 				if err != nil {
 					return fmt.Errorf("error updating custom role %s: %v", role.Name, err)

pkg/gcp/actuator/policy.go

@@ -288,7 +288,8 @@ func serviceAccountNeedsPermissionsUpdate(gcpClient ccgcp.Client, serviceAccount
 			return true, fmt.Errorf("error fetching custom role: %v", err)
 		}
 
-		if !AreSlicesEqualWithoutOrder(role.IncludedPermissions, permissions) {
+		addedPermissions, _ := CalculateSliceDiff(role.IncludedPermissions, permissions)
+		if len(addedPermissions) > 0 {
 			return true, nil
 		}
 	}

pkg/gcp/actuator/role.go

@@ -26,8 +26,6 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 
-	utilrand "k8s.io/apimachinery/pkg/util/rand"
-
 	ccgcp "github.com/openshift/cloud-credential-operator/pkg/gcp"
 )
 
@@ -97,31 +95,57 @@ func DeleteRole(gcpClient ccgcp.Client, roleName string) (*iamadminpb.Role, erro
 	return role, err
 }
 
-// GenerateRoleID generates a unique ID for the role given infra name and credentials request name.
+// GenerateRoleID generates a unique ID for the role given project name and credentials request name.
 // The role ID has a max length of 64 chars and can include only letters, numbers, period and underscores
-// we sanitize infraName and crName to make them alphanumeric and then
-// split role ID into 29_28_5 where the resulting string becomes:
-// <infraName chopped to 29 chars>_<crName chopped to 28 chars>_<random 5 chars>
-func GenerateRoleID(infraName string, crName string) (string, error) {
-	infraName = makeAlphanumeric(infraName)
+// we sanitize projectName and crName to make them alphanumeric and then
+// split role ID into 32_31 where the resulting string becomes:
+// <projectName chopped to 32 chars>_<crName chopped to 31 chars>
+func GenerateRoleID(projectName string, crName string) (string, error) {
+	projectName = makeAlphanumeric(projectName)
 	crName = makeAlphanumeric(crName)
 
-	infraNameMaxLenForRoleName := 29
-	crNameMaxLenForRoleName := 28
+	projectNameMaxLenForRoleID := 32
+	crNameMaxLenForRoleID := 31
+
+	if projectName == "" {
+		return "", fmt.Errorf("empty project name")
+	}
 
 	if crName == "" {
 		return "", fmt.Errorf("empty credential request name")
 	}
 
-	if infraName != "" {
-		if len(infraName) > infraNameMaxLenForRoleName {
-			infraName = infraName[0:infraNameMaxLenForRoleName]
-		}
+	if len(projectName) > projectNameMaxLenForRoleID {
+		projectName = projectName[0:projectNameMaxLenForRoleID]
+	}
+	if len(crName) > crNameMaxLenForRoleID {
+		crName = crName[0:crNameMaxLenForRoleID]
+	}
+	return fmt.Sprintf("%s_%s", projectName, crName), nil
+}
+
+// GenerateRoleName generates a unique name for the role given project name and credentials request name.
+// The role name has a max length of 100 chars, so we split role ID into 50-49 where the resulting string becomes:
+// <projectName chopped to 50 chars>-<crName chopped to 49 chars>
+func GenerateRoleName(projectName string, crName string) (string, error) {
+	projectNameMaxLenForRoleName := 50
+	crNameMaxLenForRoleName := 49
+
+	if projectName == "" {
+		return "", fmt.Errorf("empty project name")
+	}
+
+	if crName == "" {
+		return "", fmt.Errorf("empty credential request name")
+	}
+
+	if len(projectName) > projectNameMaxLenForRoleName {
+		projectName = projectName[0:projectNameMaxLenForRoleName]
 	}
 	if len(crName) > crNameMaxLenForRoleName {
 		crName = crName[0:crNameMaxLenForRoleName]
 	}
-	return fmt.Sprintf("%s_%s_%s", infraName, crName, utilrand.String(5)), nil
+	return fmt.Sprintf("%s-%s", projectName, crName), nil
 }
 
 // makeAlphanumeric makes a given string alphanumeric