CNTRLPLANE-2205: Auto-detect shared role use and enable for e2e

cmd/cluster/aws/destroy.go

@@ -105,7 +105,6 @@ func destroyPlatformSpecifics(ctx context.Context, o *core.DestroyOptions) error
 			CredentialsSecretData:        secretData,
 			VPCOwnerCredentialsOpts:      o.AWSPlatform.VPCOwnerCredentials,
 			PrivateZonesInClusterAccount: o.AWSPlatform.PrivateZonesInClusterAccount,
-			SharedRole:                   o.AWSPlatform.SharedRole,
 		}
 		if err := destroyOpts.Run(ctx); err != nil {
 			return fmt.Errorf("failed to destroy IAM: %w", err)

cmd/infra/aws/destroy_iam.go

@@ -29,7 +29,6 @@ type DestroyIAMOptions struct {
 
 	VPCOwnerCredentialsOpts      awsutil.AWSCredentialsOptions
 	PrivateZonesInClusterAccount bool
-	SharedRole                   bool
 
 	CredentialsSecretData *util.CredentialsSecretData
 }
@@ -50,7 +49,6 @@ func NewDestroyIAMCommand() *cobra.Command {
 	cmd.Flags().StringVar(&opts.InfraID, "infra-id", opts.InfraID, "Infrastructure ID to use for AWS resources.")
 	cmd.Flags().StringVar(&opts.Region, "region", opts.Region, "Region where cluster infra lives")
 	cmd.Flags().BoolVar(&opts.PrivateZonesInClusterAccount, "private-zones-in-cluster-account", opts.PrivateZonesInClusterAccount, "In shared VPC infrastructure, delete roles for private hosted zones from cluster account")
-	cmd.Flags().BoolVar(&opts.SharedRole, "shared-role", opts.SharedRole, "Delete the shared role instead of individual component roles")
 
 	opts.AWSCredentialsOpts.BindFlags(cmd.Flags())
 	opts.VPCOwnerCredentialsOpts.BindVPCOwnerFlags(cmd.Flags())
@@ -148,64 +146,67 @@ func (o *DestroyIAMOptions) DestroyOIDCResources(ctx context.Context, iamClient
 		}
 	}
 
-	if o.SharedRole {
-		// Delete the shared role
-		if err = o.DestroyOIDCRole(iamClient, "shared-role", false); err != nil {
-			return err
-		}
-	} else {
-		// Delete individual component roles
-		if err = o.DestroyOIDCRole(iamClient, "openshift-ingress", true); err != nil {
-			return err
-		}
-		if err = o.DestroyOIDCRole(iamClient, "openshift-image-registry", false); err != nil {
-			return err
-		}
-		if err = o.DestroyOIDCRole(iamClient, "aws-ebs-csi-driver-controller", false); err != nil {
-			return err
-		}
-		if err = o.DestroyOIDCRole(iamClient, "cloud-controller", false); err != nil {
-			return err
-		}
-		if err = o.DestroyOIDCRole(iamClient, "node-pool", false); err != nil {
-			return err
-		}
-		if err = o.DestroyOIDCRole(iamClient, "control-plane-operator", true); err != nil {
-			return err
-		}
-		if err := o.DestroyOIDCRole(iamClient, "cloud-network-config-controller", false); err != nil {
-			return err
-		}
-		if err := o.DestroyOIDCRole(iamClient, "kms-provider", false); err != nil {
-			return err
-		}
-		if err := o.DestroyOIDCRole(iamClient, "karpenter", false); err != nil {
-			return err
-		}
+	// Delete the shared role
+	removed := false
+	if removed, err = o.DestroyOIDCRole(iamClient, "shared-role"); err != nil {
+		return err
+	}
+	if removed {
+		// The cluster was created with a single shared role, so we are done.
+		// Save on additional API calls and just return here.
+		return nil
+	}
+	// Delete individual component roles
+	if _, err = o.DestroyOIDCRole(iamClient, "openshift-ingress"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "openshift-image-registry"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "aws-ebs-csi-driver-controller"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "cloud-controller"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "node-pool"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "control-plane-operator"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "cloud-network-config-controller"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "kms-provider"); err != nil {
+		return err
+	}
+	if _, err = o.DestroyOIDCRole(iamClient, "karpenter"); err != nil {
+		return err
 	}
 
 	return nil
 }
 
 // DestroyOIDCRole deletes an IAM Role with all its policies
-func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string, includeAssumePolicy bool) error {
+func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string) (removed bool, reterr error) {
 	roleName := fmt.Sprintf("%s-%s", o.InfraID, name)
 	role, err := existingRole(client, roleName)
 	if err != nil {
-		return fmt.Errorf("cannot check for existing role: %w", err)
+		return false, fmt.Errorf("cannot check for existing role: %w", err)
 	}
 
 	if role == nil {
 		o.Log.Info("Role already deleted!", "role", roleName)
-		return nil
+		return false, nil
 	}
 
 	// Detach managed policies
 	attachedPolicies, err := client.ListAttachedRolePolicies(&iam.ListAttachedRolePoliciesInput{
 		RoleName: aws.String(roleName),
 	})
 	if err != nil {
-		return fmt.Errorf("failed to list attached policies for role %s: %w", roleName, err)
+		return false, fmt.Errorf("failed to list attached policies for role %s: %w", roleName, err)
 	}
 
 	for _, policy := range attachedPolicies.AttachedPolicies {
@@ -214,7 +215,7 @@ func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string,
 			RoleName:  aws.String(roleName),
 		})
 		if err != nil {
-			return fmt.Errorf("failed to detach policy %s from role %s: %w", *policy.PolicyArn, roleName, err)
+			return false, fmt.Errorf("failed to detach policy %s from role %s: %w", *policy.PolicyArn, roleName, err)
 		}
 		o.Log.Info("Detached role policy", "role", roleName, "policy", *policy.PolicyArn)
 	}
@@ -224,7 +225,7 @@ func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string,
 		RoleName: aws.String(roleName),
 	})
 	if err != nil {
-		return fmt.Errorf("failed to list inline policies for role %s: %w", roleName, err)
+		return false, fmt.Errorf("failed to list inline policies for role %s: %w", roleName, err)
 	}
 
 	for _, policyName := range listPoliciesOutput.PolicyNames {
@@ -236,11 +237,11 @@ func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string,
 			if aerr, ok := err.(awserr.Error); ok {
 				if aerr.Code() != iam.ErrCodeNoSuchEntityException {
 					o.Log.Error(aerr, "Error deleting role policy", "role", roleName, "policy", *policyName)
-					return aerr
+					return false, aerr
 				}
 			} else {
 				o.Log.Error(err, "Error deleting role policy", "role", roleName, "policy", *policyName)
-				return err
+				return false, err
 			}
 		} else {
 			o.Log.Info("Deleted role policy", "role", roleName, "policy", *policyName)
@@ -252,11 +253,11 @@ func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string,
 		RoleName: aws.String(roleName),
 	})
 	if err != nil {
-		return fmt.Errorf("failed to delete role %s: %w", roleName, err)
+		return false, fmt.Errorf("failed to delete role %s: %w", roleName, err)
 	}
 	o.Log.Info("Deleted role", "role", roleName)
 
-	return nil
+	return true, nil
 }
 
 func (o *DestroyIAMOptions) DestroyWorkerInstanceProfile(client iamiface.IAMAPI) error {
@@ -357,10 +358,10 @@ func (o *DestroyIAMOptions) DestroySharedVPCRoles(ctx context.Context, iamClient
 	if o.PrivateZonesInClusterAccount {
 		ingressRoleClient = iamClient
 	}
-	if err = o.DestroyOIDCRole(ingressRoleClient, "shared-vpc-ingress", true); err != nil {
+	if _, err = o.DestroyOIDCRole(ingressRoleClient, "shared-vpc-ingress"); err != nil {
 		return err
 	}
-	if err = o.DestroyOIDCRole(vpcOwnerIAMClient, "shared-vpc-control-plane", true); err != nil {
+	if _, err = o.DestroyOIDCRole(vpcOwnerIAMClient, "shared-vpc-control-plane"); err != nil {
 		return err
 	}
 	return nil

cmd/infra/aws/iam.go

@@ -1029,18 +1029,31 @@ func (o *CreateIAMOptions) CreateSharedOIDCRole(ctx context.Context, client iami
 		return "", fmt.Errorf("failed to create shared role: %w", err)
 	}
 
-	// Add all policies to the shared role as inline policies
+	// Add all policies to the shared role
 	for _, binding := range bindings {
-		policyName := fmt.Sprintf("%s-%s", roleName, binding.name)
-		_, err = client.PutRolePolicyWithContext(ctx, &iam.PutRolePolicyInput{
-			PolicyName:     aws.String(policyName),
-			PolicyDocument: aws.String(binding.policy),
-			RoleName:       aws.String(roleName),
-		})
-		if err != nil {
-			return "", fmt.Errorf("failed to add policy %q to shared role: %w", binding.name, err)
+		if o.UseROSAManagedPolicies && binding.rosaManagedPolicyARN != "" {
+			// Attach ROSA managed policy
+			_, err = client.AttachRolePolicyWithContext(ctx, &iam.AttachRolePolicyInput{
+				PolicyArn: aws.String(binding.rosaManagedPolicyARN),
+				RoleName:  aws.String(roleName),
+			})
+			if err != nil {
+				return "", fmt.Errorf("failed to attach managed policy %q to shared role: %w", binding.rosaManagedPolicyARN, err)
+			}
+			logger.Info("Attached managed policy to shared role", "policy", binding.rosaManagedPolicyARN, "role", roleName)
+		} else {
+			// Add inline policy
+			policyName := fmt.Sprintf("%s-%s", roleName, binding.name)
+			_, err = client.PutRolePolicyWithContext(ctx, &iam.PutRolePolicyInput{
+				PolicyName:     aws.String(policyName),
+				PolicyDocument: aws.String(binding.policy),
+				RoleName:       aws.String(roleName),
+			})
+			if err != nil {
+				return "", fmt.Errorf("failed to add policy %q to shared role: %w", binding.name, err)
+			}
+			logger.Info("Added inline policy to shared role", "policy", binding.name, "role", roleName)
 		}
-		logger.Info("Added policy to shared role", "policy", binding.name, "role", roleName)
 	}
 
 	// Add assume role policy if needed

test/e2e/create_cluster_test.go

@@ -2313,6 +2313,10 @@ func TestCreateCluster(t *testing.T) {
 	if !e2eutil.IsLessThan(e2eutil.Version418) {
 		clusterOpts.FeatureSet = string(configv1.TechPreviewNoUpgrade)
 	}
+	if globalOpts.Platform == hyperv1.AWSPlatform {
+		// Use this test cluster to validate per-component roles
+		clusterOpts.AWSPlatform.SharedRole = false
+	}
 
 	clusterOpts.PodsLabels = map[string]string{
 		"hypershift-e2e-test-label": "test",

test/e2e/util/options.go

@@ -284,6 +284,7 @@ func (o *Options) DefaultAWSOptions() hypershiftaws.RawCreateOptions {
 		MultiArch:              o.ConfigurableClusterOptions.AWSMultiArch,
 		PublicOnly:             true,
 		UseROSAManagedPolicies: true,
+		SharedRole:             true,
 	}
 	if IsLessThan(semver.MustParse("4.16.0")) {
 		opts.PublicOnly = false