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 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 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