CORS-4058: Migrate AWS Destroy to SDK v2 by barbacbd · Pull Request #9939 · openshift/installer

barbacbd · 2025-09-11T17:39:01Z

Reintroduce the changes for the AWS Destroy code.

The Big changes between this and the original PR #9736:

The Max Retry Attempts is default set (this was the major issue and reason for removing the changes before).
The default config and client creation happens through functions in the installconfig. Take advantage of these and use them where we can.
Cleanup some of the fixups from before.
Cleaup potential merge conflicts with packages being imported

** the bulk of the changes are to the ec2helpers file. All of the sdk v1 imports are removed except for session as this one is engrained too many files currently. pkg/destroy/aws/aws.go ** Add a client for ELB ELBV2 and IAM to the Cluster Removal Struct. Even though these changes are mainly to ec2helpers, the other clients were required in for certain operations. ** The rest of the file updates are alter ARN import to come from aws sdk v2.

** Remove/Change all imports from AWS sdk v1 to v2. pkg/destroy/aws/errors.go pkg/destroy/aws/ec2helpers.go ** Remove the Error checking/formatting function from ec2helpers and put the function in the errors.go file.

** Remove all SDK v1 imports from elb helpers.

** Update Route53, s3, and efs services to sdk v2. This is slowly removing the requirement for aws session.

** This caused updates to other packages such as aws/config, credentials, stscreds, and a list of aws internal packages.

… clients in destroyer.

openshift-ci-robot · 2025-09-11T17:39:05Z

@barbacbd: This pull request references CORS-4058 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "4.20.0" instead.

Details

In response to this:

Reintroduce the changes for the AWS Destroy code.

The Big changes between this and the original PR #9736:

The Max Retry Attempts is default set (this was the major issue and reason for removing the changes before).

The default config and client creation happens through functions in the installconfig. Take advantage of these and use them where we can.

Cleanup some of the fixups from before.

Cleaup potential merge conflicts with packages being imported

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

barbacbd · 2025-09-11T17:39:13Z

/cc @patrickdillon
/cc @tthvo

tthvo · 2025-09-15T19:11:39Z

pkg/destroy/aws/aws.go

+	cfg, err := configv2.LoadDefaultConfig(context.TODO(), configv2.WithRegion(region))
+	if err != nil {
+		return nil, fmt.Errorf("failed loading default config: %w", err)
+	}
+	return createResourceTaggingClientWithConfig(cfg, region, endpoints), nil
+}


For the v2 Config, we should get them from: GetConfigWithOptions(ctx, configv2.WithRegion(region).

This has the common settings (i.e. maxRetries) that we probably want to have.

pkg/destroy/aws: ** Alter the function name from HandleErrorCode to handleErrorCode. The initial thought was that this function could be used in other areas of the code, but it will remain in destroy for now. pkg/destroy/aws/shared.go: ** Remove the session import and uses in the file.

** Remove session from the imports. Added the agent handler to the configurations.

tthvo · 2025-09-16T18:30:36Z

/test gofmt

tthvo

Looks good overall to me 🚀 !

I had some more questions and suggestions 🤔 I think there are a few more places where we can refactor:

Move client constructs to the common client file pkg/asset/installconfig/aws/clients.go
Move error handling and error constants to a common file pkg/asset/installconfig/aws/awserrors.go

These can be handled in CORS-4078 as a final touch :D

pkg/destroy/aws/shared.go

tthvo · 2025-09-16T19:42:29Z

pkg/destroy/aws/shared.go

-					var awsErr awserr.Error
-					ok := errors.As(err, &awsErr)
-					if ok && awsErr.Code() == resourcegroupstaggingapi.ErrorCodeInvalidParameterException {
+					if strings.Contains(HandleErrorCode(err), "InvalidParameter") {


I think this way of error checking (i.e. parsing error code) and the one above (i.e. errors.As to a specific error type) is equivalent.

Though, should we use the same way for consistency?

It seems previously we were checking to the error code in both places.

@tthvo I believe that this is already the equivalent. HandleErrorCode will run errors.As and return the code.

pkg/destroy/aws/shared.go

pkg/destroy/aws/aws.go

tthvo · 2025-09-17T00:33:41Z

pkg/destroy/aws/aws.go

+	route53Client, err := awssession.NewRoute53Client(ctx, awssession.EndpointOptions{
+		Region:    region,
+		Endpoints: metadata.AWS.ServiceEndpoints,
+	}, "") // FIXME: Do we need an ARN here?


AFAIK, we don't need to set the hostedZoneRole arn here for this common route53 client as it is used for deleting "owned" hosted zone.

However, we do need to recreate the client with the role arn in file: pkg/destroy/aws/shared.go for "shared" hosted zone. See lines below:

https://github.com/openshift/installer/pull/9939/files#diff-fd89b44e69dab26b843e9e0a31cd945164e7aab3c8d15780fb5eafcfb1d9b836L209-L217

That block was deleted and we need to add that logic back.

tthvo · 2025-09-17T00:36:04Z

pkg/destroy/aws/shared.go

-	publicZoneClient := route53.New(session)
-	privateZoneClient := route53.New(session)
-	if o.HostedZoneRole != "" {
-		creds := stscreds.NewCredentials(session, o.HostedZoneRole)
-		privateZoneClient = route53.New(session, &aws.Config{Credentials: creds})
-		logger.Infof("Assuming role %s to destroy records in private hosted zone", o.HostedZoneRole)
-	}


Unfortunately, this block is still needed. If not, the handler will use the common Route53 and completely ignore the hosted zone role.

Thus, we need to recreate the route53 client here:

A publicZoneClient: use the common Route53 client.

A privateZoneClient: if hostZoneRole is specified, create a separate client with AWS STS.

tthvo · 2025-09-17T00:51:32Z

/test e2e-aws-ovn-custom-iam-profile e2e-aws-ovn-public-ipv4-pool e2e-aws-ovn-upi e2e-aws-ovn-heterogeneous

Set a Destroy User Agent. Cleanup pointer references to use the aws sdk.

tthvo

/lgtm

Nice, we have still have a discussion for #9939 (comment). But this looks good!

patrickdillon · 2025-09-17T19:07:14Z

I'm not sure about the other CI jobs--we can check back when they finish running--but I have some concerns about the destroy in the aws custom dns job: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_installer/9939/pull-ci-openshift-installer-main-e2e-aws-custom-dns-techpreview/1968339873618202624/artifacts/e2e-aws-custom-dns-techpreview/ipi-deprovision-deprovision/artifacts/

The endpoint log messages are overwhelming
It looks like the VPC is not deleting because of a dependency

tthvo · 2025-09-17T19:15:43Z

The endpoint log messages are overwhelming

The destroy finished just now I think:

time="2025-09-17T18:08:54Z" level=info msg=Deleted id=vpc-0d44eceb3e9433e2f resourceType=vpc

It looks like the VPC is not deleting because of a dependency

time="2025-09-17T18:08:53Z" level=debug msg="Custom EC2 endpoint not found, using default endpoint"
time="2025-09-17T18:08:53Z" level=debug msg="Custom EC2 endpoint not found, using default endpoint"

These logs come from endpoint resolver for AWS SDK v2. They have been so noisy. Let me remove them in #9907

patrickdillon · 2025-09-17T19:29:40Z

The destroy finished just now I think:

time="2025-09-17T18:08:54Z" level=info msg=Deleted id=vpc-0d44eceb3e9433e2f resourceType=vpc

Oh, yeah I think I actually just lost it in the error messages. You're right, no issues with destroy!

It looks like the VPC is not deleting because of a dependency
time="2025-09-17T18:08:53Z" level=debug msg="Custom EC2 endpoint not found, using default endpoint"
time="2025-09-17T18:08:53Z" level=debug msg="Custom EC2 endpoint not found, using default endpoint"
These logs come from endpoint resolver for AWS SDK v2. They have been so noisy. Let me remove them in #9907

Ah, that will be a separate PR, so this LGTM.

Thanks!

/approve

openshift-ci · 2025-09-17T19:30:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [patrickdillon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tthvo · 2025-09-17T19:36:54Z

/cc @yunjiang29

tthvo · 2025-09-17T20:18:59Z

/test e2e-aws-ovn

tthvo · 2025-09-17T20:19:22Z

/test e2e-aws-ovn-custom-iam-profile e2e-aws-ovn-public-ipv4-pool e2e-aws-ovn-upi e2e-aws-ovn-heterogeneous

barbacbd · 2025-09-18T10:40:16Z

/retest-required

openshift-ci · 2025-09-18T14:28:26Z

@barbacbd: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`c4f0a7a`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-vsphere-static-ovn	`c4f0a7a`	link	false	`/test e2e-vsphere-static-ovn`
ci/prow/e2e-aws-ovn-heterogeneous	`c4f0a7a`	link	false	`/test e2e-aws-ovn-heterogeneous`
ci/prow/e2e-vsphere-ovn-multi-network	`c4f0a7a`	link	false	`/test e2e-vsphere-ovn-multi-network`
ci/prow/e2e-aws-custom-dns-techpreview	`c4f0a7a`	link	false	`/test e2e-aws-custom-dns-techpreview`
ci/prow/e2e-aws-ovn-single-node	`c4f0a7a`	link	false	`/test e2e-aws-ovn-single-node`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

barbacbd · 2025-09-19T10:31:15Z

/verified by ci/prow/e2e-aws-ovn

openshift-ci-robot · 2025-09-19T10:31:27Z

@barbacbd: This PR has been marked as verified by ci/prow/e2e-aws-ovn.

Details

In response to this:

/verified by ci/prow/e2e-aws-ovn

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

…ease-4.21" This reverts commit bad1b52, reversing changes made to beebc39.

no-jira: Revert "Merge pull request #9939 from barbacbd/CORS-4058-release-4.21"

sadasu · 2025-09-30T18:30:28Z

pkg/destroy/aws/aws.go

-				resourcegroupstaggingapi.New(awsSession, aws.NewConfig().WithRegion(endpoints.UsGovWest1RegionID)))
+	case endpointUSGovEast1, endpointUSGovWest1:
+		if o.Region != endpointUSGovWest1 {
+			tagClient, err := createResourceTaggingClient(endpointUSGovWest1, o.endpoints)


Is this correct? When the region is not endpointUSGovWest1 , we create a resource tagging client for the same? If it is, could you please add a comment why?

I actually have little context, but that block has always been there. For example, in release 4.19:

installer/pkg/destroy/aws/aws.go

Lines 166 to 170 in 7e30b7d

case endpoints.UsGovEast1RegionID, endpoints.UsGovWest1RegionID:

if o.Region != endpoints.UsGovWest1RegionID {

tagClients = append(tagClients,

resourcegroupstaggingapi.New(awsSession, aws.NewConfig().WithRegion(endpoints.UsGovWest1RegionID)))

}

Git Blame returns PR #4042. IIUC, for government cloud partition, there are route53 resources that are only available in us-west-gov-1, that will have to be cleaned up.

barbacbd added 7 commits September 11, 2025 11:59

pkg/destroy/aws/iamhelpers.go

d4ff78f

** Remove/Change all imports from AWS sdk v1 to v2. pkg/destroy/aws/errors.go pkg/destroy/aws/ec2helpers.go ** Remove the Error checking/formatting function from ec2helpers and put the function in the errors.go file.

pkg/destroy/aws/elbhelpers.go

3e57ad0

** Remove all SDK v1 imports from elb helpers.

Add reference to correct HandleErrorCode function.

6b7b8ea

pkg/destroy/aws/aws.go

64d94c5

** Update Route53, s3, and efs services to sdk v2. This is slowly removing the requirement for aws session.

** Vendor updates for S3 and EFS services.

6e79cba

** This caused updates to other packages such as aws/config, credentials, stscreds, and a list of aws internal packages.

Clean up references and use the exported config creator to create new…

f562ed8

… clients in destroyer.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 11, 2025

openshift-ci bot requested review from andfasano, patrickdillon and tthvo September 11, 2025 17:39

tthvo reviewed Sep 15, 2025

View reviewed changes

barbacbd force-pushed the CORS-4058-release-4.21 branch from 13ecc48 to 831afca Compare September 16, 2025 14:53

barbacbd added 4 commits September 16, 2025 13:27

Fix references to HandleErrorCode.

3e5b4da

kg/destroy/aws/aws.go:

86d7d58

** Remove session from the imports. Added the agent handler to the configurations.

Fix package updates for vendoring.

4fcea32

barbacbd force-pushed the CORS-4058-release-4.21 branch from 831afca to 4fcea32 Compare September 16, 2025 17:27

barbacbd requested a review from tthvo September 16, 2025 19:04

tthvo reviewed Sep 17, 2025

View reviewed changes

Use the correct private and public zone clients.

c4f0a7a

Set a Destroy User Agent. Cleanup pointer references to use the aws sdk.

tthvo reviewed Sep 17, 2025

View reviewed changes

openshift-ci bot assigned tthvo Sep 17, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2025

openshift-ci bot requested a review from yunjiang29 September 17, 2025 19:36

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Sep 19, 2025

openshift-merge-bot bot merged commit bad1b52 into openshift:main Sep 19, 2025
28 of 34 checks passed

patrickdillon added a commit to patrickdillon/installer that referenced this pull request Sep 29, 2025

Revert "Merge pull request openshift#9939 from barbacbd/CORS-4058-rel…

418324a

…ease-4.21" This reverts commit bad1b52, reversing changes made to beebc39.

patrickdillon mentioned this pull request Sep 29, 2025

no-jira: Revert "Merge pull request #9939 from barbacbd/CORS-4058-release-4.21" #9974

Merged

patrickdillon added a commit that referenced this pull request Sep 30, 2025

Merge pull request #9974 from patrickdillon/revert-aws-destroy-v2

a745ded

no-jira: Revert "Merge pull request #9939 from barbacbd/CORS-4058-release-4.21"

tthvo mentioned this pull request Sep 30, 2025

CORS-4058: Migrate AWS Destroy to SDK v2 #9982

Merged

sadasu reviewed Sep 30, 2025

View reviewed changes

	case endpoints.UsGovEast1RegionID, endpoints.UsGovWest1RegionID:
	if o.Region != endpoints.UsGovWest1RegionID {
	tagClients = append(tagClients,
	resourcegroupstaggingapi.New(awsSession, aws.NewConfig().WithRegion(endpoints.UsGovWest1RegionID)))
	}

Conversation

barbacbd commented Sep 11, 2025

Uh oh!

openshift-ci-robot commented Sep 11, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barbacbd commented Sep 11, 2025

Uh oh!

tthvo Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo commented Sep 16, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tthvo Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

barbacbd Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tthvo Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo commented Sep 17, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Sep 17, 2025

Uh oh!

tthvo commented Sep 17, 2025

Uh oh!

patrickdillon commented Sep 17, 2025

Uh oh!

openshift-ci bot commented Sep 17, 2025

Uh oh!

tthvo commented Sep 17, 2025

Uh oh!

tthvo commented Sep 17, 2025

Uh oh!

tthvo commented Sep 17, 2025

Uh oh!

barbacbd commented Sep 18, 2025

Uh oh!

openshift-ci bot commented Sep 18, 2025

Uh oh!

barbacbd commented Sep 19, 2025

Uh oh!

openshift-ci-robot commented Sep 19, 2025

Uh oh!

Uh oh!

sadasu Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tthvo Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

openshift-ci-robot commented Sep 11, 2025 •

edited by openshift-ci bot

Loading

sadasu Sep 30, 2025 •

edited

Loading