CORS-4058: Remove AWS SDK V1 from destroy/aws by barbacbd · Pull Request #9736 · openshift/installer

barbacbd · 2025-05-21T19:01:42Z

No description provided.

openshift-ci-robot · 2025-05-21T19:01:46Z

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

Details

In response to this:

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-05-21T19:08:18Z

barbacbd · 2025-05-28T17:41:44Z

/cc @patrickdillon

barbacbd · 2025-06-02T19:52:31Z

/label platform/aws

patrickdillon · 2025-06-10T20:24:09Z

/approve

I will look at this again, mostly just sanity checking to tag.

openshift-ci · 2025-06-10T20:24:38Z

[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

barbacbd · 2025-06-12T12:00:16Z

/retest-required

tthvo

Thanks for handling such a substantial change! I have some questions/comments below but LGTM overall!

tthvo · 2025-06-13T07:49:33Z

pkg/destroy/aws/shared.go

+			if lastErr != nil {
+				return lastErr
+			}


Question: Should we requeue the tagclient for retrying if an error ocurrs? IIUC, previously we did that here?

If so, we should also check if the error is an AWS InvalidParameter error to skip requeueing. That said, I think, this PR, on line 73, the paginator error should be assigned to lastError and break the paging loop early.

pkg/destroy/aws/elbhelpers.go

pkg/destroy/aws/iamhelpers.go

pkg/destroy/aws/aws.go

tthvo · 2025-06-16T16:41:53Z

pkg/destroy/aws/iamhelpers.go

+				switch {
+				case strings.Contains(handleErrorCode(err), "NoSuchEntity"):
+					// The user does not exist.
+					// Ignore this IAM Role and do not report this error via lastError.


Suggested change

// Ignore this IAM Role and do not report this error via lastError.

// Ignore this IAM User and do not report this error via lastError.

tthvo · 2025-06-16T16:42:15Z

pkg/destroy/aws/iamhelpers.go

+					// Installer does not have access to this IAM role.
+					// Ignore this IAM Role and do not report this error via lastError.


Suggested change

// Installer does not have access to this IAM role.

// Ignore this IAM Role and do not report this error via lastError.

// Installer does not have access to this IAM user.

// Ignore this IAM User and do not report this error via lastError.

tthvo · 2025-06-16T16:44:26Z

pkg/destroy/aws/shared.go

+			for paginator.HasMorePages() {
+				page, err := paginator.NextPage(ctx)
+				if err != nil {
+					return fmt.Errorf("failed to get resources: %w", err)


I think the InvalidParameterException will likely come from here right? We should assign the error to lastError and break to check for requeueing?

patrickdillon · 2025-07-07T12:10:24Z

/hold cancel

I reviewed again and this looks good to me.

It needs a rebase in the vendoring. There are multiple vendoring commits, so this is a probably a good opportunity to squash those. There are also a few places where the changes are revised between multiple commits, so you may want to squash those too.

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

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

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

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.

patrickdillon · 2025-07-08T17:37:17Z

/lgtm

openshift-ci · 2025-07-09T00:50:40Z

@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/e2e-aws-private	`67c3770`	link	false	`/test e2e-aws-private`
ci/prow/e2e-vsphere-externallb-ovn	`9ecf060`	link	false	`/test e2e-vsphere-externallb-ovn`
ci/prow/okd-scos-images	`9ecf060`	link	false	`/test okd-scos-images`
ci/prow/okd-scos-e2e-aws-ovn	`9ecf060`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn-shared-vpc-edge-zones	`9ecf060`	link	false	`/test e2e-aws-ovn-shared-vpc-edge-zones`
ci/prow/e2e-aws-byo-subnet-role-security-groups	`9ecf060`	link	false	`/test e2e-aws-byo-subnet-role-security-groups`
ci/prow/e2e-azure-ovn-resourcegroup	`9ecf060`	link	false	`/test e2e-azure-ovn-resourcegroup`
ci/prow/e2e-aws-ovn-single-node	`9ecf060`	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.

openshift-bot · 2025-07-09T05:40:51Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer
This PR has been included in build ose-installer-container-v4.20.0-202507090413.p0.g4e8d67e.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-07-09T05:46:53Z

[ART PR BUILD NOTIFIER]

Distgit: ose-baremetal-installer
This PR has been included in build ose-baremetal-installer-container-v4.20.0-202507090413.p0.g4e8d67e.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-07-09T06:43:54Z

[ART PR BUILD NOTIFIER]

Distgit: ose-installer-artifacts
This PR has been included in build ose-installer-artifacts-container-v4.20.0-202507090413.p0.g4e8d67e.assembly.stream.el9.
All builds following this will include this PR.

This reverts commit 4e8d67e, reversing changes made to d772abf.

no-jira: Revert "Merge pull request #9736 from barbacbd/CORS-4058"

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

barbacbd changed the title ~~CORS-4058: Remove AWS SDK V1 from destroy/aws~~ WIP: CORS-4058: Remove AWS SDK V1 from destroy/aws May 21, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2025

openshift-ci bot requested review from rna-afk and tthvo May 21, 2025 19:04

barbacbd force-pushed the CORS-4058 branch 2 times, most recently from 585e593 to a027c22 Compare May 28, 2025 14:37

barbacbd changed the title ~~WIP: CORS-4058: Remove AWS SDK V1 from destroy/aws~~ CORS-4058: Remove AWS SDK V1 from destroy/aws May 28, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 28, 2025

barbacbd force-pushed the CORS-4058 branch from a027c22 to 3d22965 Compare May 28, 2025 17:40

openshift-ci bot requested a review from patrickdillon May 28, 2025 17:41

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2025

barbacbd force-pushed the CORS-4058 branch from 3d22965 to 67c3770 Compare June 2, 2025 19:25

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2025

openshift-ci bot added the platform/aws label Jun 2, 2025

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

tthvo reviewed Jun 13, 2025

View reviewed changes

barbacbd force-pushed the CORS-4058 branch 3 times, most recently from 5b2a3fa to 538c582 Compare June 16, 2025 15:22

tthvo reviewed Jun 16, 2025

View reviewed changes

barbacbd force-pushed the CORS-4058 branch 2 times, most recently from d10fbce to 00bbf4a Compare June 16, 2025 19:30

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2025

barbacbd added 4 commits July 8, 2025 07:23

** Vendor updates for IAM for AWS SDK V2

a89342f

pkg/destroy/aws/iamhelpers.go

7b466d9

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

b08089f

** Remove all SDK v1 imports from elb helpers.

barbacbd force-pushed the CORS-4058 branch from 00bbf4a to 8b97976 Compare July 8, 2025 12:19

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2025

barbacbd force-pushed the CORS-4058 branch from 8b97976 to bfb1c36 Compare July 8, 2025 14:28

barbacbd added 6 commits July 8, 2025 12:05

** Vendor updates for S3 and EFS services.

c3b190b

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

pkg/destroy/aws/aws.go

51eef71

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

** verify vendor updates for dependent packages.

50c6848

pkg/destroy/aws/aws.go:

e6eda3e

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

Removing s3/s3manager to satisfy vendor issues.

9ecf060

barbacbd force-pushed the CORS-4058 branch from 3bbf848 to 9ecf060 Compare July 8, 2025 16:06

openshift-ci bot assigned patrickdillon Jul 8, 2025

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

openshift-merge-bot bot merged commit 4e8d67e into openshift:main Jul 9, 2025
22 of 29 checks passed

patrickdillon added a commit to patrickdillon/installer that referenced this pull request Jul 13, 2025

Revert "Merge pull request openshift#9736 from barbacbd/CORS-4058"

492e9ac

This reverts commit 4e8d67e, reversing changes made to d772abf.

patrickdillon mentioned this pull request Jul 13, 2025

no-jira: Revert "Merge pull request #9736 from barbacbd/CORS-4058" #9837

Merged

jupierce added a commit that referenced this pull request Jul 14, 2025

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

184b556

no-jira: Revert "Merge pull request #9736 from barbacbd/CORS-4058"

barbacbd mentioned this pull request Sep 11, 2025

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

Merged

	// Ignore this IAM Role and do not report this error via lastError.
	// Ignore this IAM User and do not report this error via lastError.

		// Installer does not have access to this IAM role.
		// Ignore this IAM Role and do not report this error via lastError.

Conversation

barbacbd commented May 21, 2025

Uh oh!

openshift-ci-robot commented May 21, 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 May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barbacbd commented May 28, 2025

Uh oh!

barbacbd commented Jun 2, 2025

Uh oh!

patrickdillon commented Jun 10, 2025

Uh oh!

openshift-ci bot commented Jun 10, 2025

Uh oh!

barbacbd commented Jun 12, 2025

Uh oh!

tthvo left a comment

Choose a reason for hiding this comment

Uh oh!

tthvo Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tthvo Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

tthvo Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Jul 7, 2025

Uh oh!

patrickdillon commented Jul 8, 2025

Uh oh!

openshift-ci bot commented Jul 9, 2025

Uh oh!

Uh oh!

openshift-bot commented Jul 9, 2025

Uh oh!

openshift-bot commented Jul 9, 2025

Uh oh!

openshift-bot commented Jul 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

openshift-ci-robot commented May 21, 2025 •

edited by openshift-ci bot

Loading

barbacbd commented May 21, 2025 •

edited

Loading