From 12d1b9982a29ca7bc782e587f1735e82987da86f Mon Sep 17 00:00:00 2001 From: Seth Jennings Date: Wed, 3 Dec 2025 21:49:19 -0600 Subject: [PATCH 1/3] feat(aws): enable shared IAM role in e2e Enable the use of a single shared IAM role for all test clusters except TestCreateCluster. --- test/e2e/create_cluster_test.go | 4 ++++ test/e2e/util/options.go | 1 + 2 files changed, 5 insertions(+) diff --git a/test/e2e/create_cluster_test.go b/test/e2e/create_cluster_test.go index 02fe6cbb0f29..9355a770154e 100644 --- a/test/e2e/create_cluster_test.go +++ b/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", diff --git a/test/e2e/util/options.go b/test/e2e/util/options.go index d434cd01d6b4..29f5ad2753a7 100644 --- a/test/e2e/util/options.go +++ b/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 From a6373a5937578e8ba56c501e37e2e4fba98086fd Mon Sep 17 00:00:00 2001 From: Seth Jennings Date: Thu, 4 Dec 2025 09:20:04 -0600 Subject: [PATCH 2/3] fix(aws): fix managed role policies when using shared role Original commit that introduced the shared role did not include handling for using ROSA managed policies. The commit adds code to handle it. --- cmd/infra/aws/iam.go | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/cmd/infra/aws/iam.go b/cmd/infra/aws/iam.go index 3873f03f645e..f046e8ed8619 100644 --- a/cmd/infra/aws/iam.go +++ b/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 From 74f3691795cdf371a6ad9f1e439e4e4fa4f691d3 Mon Sep 17 00:00:00 2001 From: Seth Jennings Date: Tue, 9 Dec 2025 08:35:31 -0600 Subject: [PATCH 3/3] feat(aws): auto-detect use of shared role and remove flag We added a --shared-role flag to the iam destroy path, but this creates and issue where the create and destroy must be run with the same flags, otherwise the cluster doesn't clean up properly. We can detect if a cluster was created with --shared-role in the destory path so just Do The Right Thing and remove the flag. --- cmd/cluster/aws/destroy.go | 1 - cmd/infra/aws/destroy_iam.go | 97 ++++++++++++++++++------------------ 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/cmd/cluster/aws/destroy.go b/cmd/cluster/aws/destroy.go index c7f1dc51370b..acb7e6404d47 100644 --- a/cmd/cluster/aws/destroy.go +++ b/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) diff --git a/cmd/infra/aws/destroy_iam.go b/cmd/infra/aws/destroy_iam.go index c536247c3e55..8aa3ee90a371 100644 --- a/cmd/infra/aws/destroy_iam.go +++ b/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,56 +146,59 @@ 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 @@ -205,7 +206,7 @@ func (o *DestroyIAMOptions) DestroyOIDCRole(client iamiface.IAMAPI, name string, 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