OCPBUGS-1769: Check for AWS STS installation before trying to get all IAM Roles #6666

sadasu · 2022-12-05T23:31:07Z

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles that Installer does not have access to.

openshift-ci-robot · 2022-12-05T23:31:10Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is invalid:

expected the bug to target the "4.13.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles created in the STS environment and also not try to find them in the first place. This way we are achieving this is by detecting if it is a STS cluster, identified by CredentialsMode == Manual. And when that is the case, we skip finding and deleting the IAM Roles in that cluster.

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/test-infra repository.

sadasu · 2022-12-05T23:31:54Z

/jira refresh

openshift-ci-robot · 2022-12-05T23:31:56Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is invalid:

expected the bug to target the "4.13.0" version, but it targets "4.13" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/jira refresh

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/test-infra repository.

sadasu · 2022-12-05T23:32:45Z

/jira refresh

openshift-ci-robot · 2022-12-05T23:32:52Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yuwan@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

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/test-infra repository.

r4f4 · 2022-12-06T09:17:39Z

pkg/destroy/aws/aws.go

CredentialsMode can be set to Manual without implying STS use: https://docs.openshift.com/container-platform/4.11/installing/installing_aws/manually-creating-iam.html#manually-creating-iam-aws. Do we also want to skip deleting IAM roles in that case?

r4f4 · 2022-12-06T09:18:59Z

pkg/destroy/aws/aws.go

Instead of skipping the finding of IAM roles entirely, we could also check the error for AccessDenied and ignore it in that case, either here or inside findIAMRoles since it's the GetRoleWithContext call that is failing.

Also, the installer can still create IAM roles for the control plane nodes when it is in manual mode: https://github.com/openshift/installer/blob/master/data/data/aws/cluster/master/main.tf#L18

Manual mode specifically disables IAM role creation by the cloud credential operator. So if we gate based on manual mode we will be leaking the IAM roles created by the installer.

openshift-ci-robot · 2022-12-21T19:28:35Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yuwan@redhat.com), skipping review request.

Details

In response to this:

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles that Installer does not have access to.

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/test-infra repository.

openshift-ci-robot · 2022-12-21T19:51:07Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yuwan@redhat.com), skipping review request.

Details

In response to this:

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles that Installer does not have access to. The current fix modifies a common method used by both findIAMRoles() and findIAMUsers()

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/test-infra repository.

openshift-ci-robot · 2022-12-22T15:08:56Z

@sadasu: This pull request references Jira Issue OCPBUGS-1769, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yuwan@redhat.com), skipping review request.

Details

In response to this:

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles that Installer does not have access to.

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/test-infra repository.

r4f4 · 2022-12-22T17:10:33Z

pkg/destroy/aws/iamhelpers.go

It looks a bit weird to import a quota package to check if an error is AccessDenied. Either IsUnauthorized should be moved out of the quota package so it can be shared with multiple packages, or we should just have our own isUnauthorized function in this file. Actually, I think this whole block would look better with a switch

Suggested change

if quotaaws.IsUnauthorized(err) {

// Installer does not have access to this IAM role

// Ignore this IAM Role and donot report this error via

// lastError

search.unmatched[*role.Arn] = exists

continue

}

var awsErr awserr.Error

if errors.As(err, &awsErr) {

switch awsErr.Code() {

case "AccessDeniedException":

// Installer does not have access to this IAM role

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

// lastError

search.logger.Debugf("AccessDenied to role %s. Expected if this is an STS install", *role.Arn)

fallthrough

case iam.ErrCodeNoSuchEntityException:

search.unmatched[*role.Arn] = exists

continue

default:

}

}

if lastError != nil {

search.logger.Debug(lastError)

}

lastError = errors.Wrapf(err, "get tags for %s", *role.Arn)

}

It would be nice to have one helper function which identifies unauthorized access for any AWS API's return code and that was the reason behind using the pre-existing method (unfortunately it exists in quota/aws).
Yes, this lends itself to using a switch statement.
I am concerned if logging a statement for the AccessDenied case would make the logs noisy hence stayed away from it. Happy to add it if it provides value to the customer (in non STS cases maybe?)

sadasu · 2022-12-23T16:07:13Z

/retest-required

sadasu · 2023-01-04T15:38:22Z

/retest-required

patrickdillon · 2023-01-10T15:16:57Z

/approve

Overall approach looks good. I agree that we could simplify the error checking a bit: we can just string match on the unauthorized error without bringing in another package.

openshift-ci · 2023-01-10T15:19:45Z

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

~~pkg/destroy/aws/OWNERS~~ [patrickdillon]

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

r4f4

Just one nitpicking comment but not worth blocking the merge on it.
/lgtm

r4f4 · 2023-01-10T22:33:16Z

pkg/destroy/aws/iamhelpers.go

 )

+const (
+	ErrCodeAccessDeniedException = "AccessDeniedException"


Nit: probably doesn't need to be exported outside the package

openshift-ci-robot · 2023-01-11T01:24:40Z

@sadasu: All pull requests linked via external trackers have merged:

openshift/installer#6666

Jira Issue OCPBUGS-1769 has been moved to the MODIFIED state.

Details

In response to this:

In the AWS cluster destroy/uninstall path, the Installer tries to find all the IAM Roles in the cluster and then attempts to delete the resources with the tag kubernetes.io/cluster/<cluster-name>. In the case of STS clusters, all IAM Roles are cleared outside the cluster (not by the Installer) and even trying to find them in the cluster results in errors because the Installer does not have the privileges to do that, let alone deleting them.

The goal of the fix is not attempt to delete these IAM Roles that Installer does not have access to.

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/test-infra repository.

openshift-ci · 2023-01-11T01:25:26Z

@sadasu: 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-ibmcloud-ovn	d958698c6d7f4b2f9521a22af1443f18b76a5a50	link	false	`/test e2e-ibmcloud-ovn`
ci/prow/okd-scos-e2e-aws-upgrade	d958698c6d7f4b2f9521a22af1443f18b76a5a50	link	false	`/test okd-scos-e2e-aws-upgrade`
ci/prow/e2e-libvirt	d958698c6d7f4b2f9521a22af1443f18b76a5a50	link	false	`/test e2e-libvirt`
ci/prow/okd-e2e-aws-ovn-upgrade	d958698c6d7f4b2f9521a22af1443f18b76a5a50	link	false	`/test okd-e2e-aws-ovn-upgrade`
ci/prow/e2e-vsphere-ovn	6d903af68a314db8477631144cebd002653a0493	link	true	`/test e2e-vsphere-ovn`
ci/prow/e2e-metal-ipi-ovn-ipv6	6d903af68a314db8477631144cebd002653a0493	link	true	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-openstack-ovn	6d903af68a314db8477631144cebd002653a0493	link	true	`/test e2e-openstack-ovn`
ci/prow/e2e-aws-ovn-disruptive	`54a7f13`	link	false	`/test e2e-aws-ovn-disruptive`
ci/prow/e2e-aws-ovn-proxy	`54a7f13`	link	false	`/test e2e-aws-ovn-proxy`

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/test-infra repository. I understand the commands that are listed here.

patrickdillon · 2023-02-13T14:47:52Z

/cherry-pick release-4.12

openshift-cherrypick-robot · 2023-02-13T14:49:38Z

@patrickdillon: new pull request created: #6847

Details

In response to this:

/cherry-pick release-4.12

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/test-infra repository.

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Dec 5, 2022

openshift-ci bot requested review from mtulio and r4f4 December 5, 2022 23:32

r4f4 reviewed Dec 6, 2022

View reviewed changes

sadasu force-pushed the aws-sts-uninstall branch from d958698 to f8ab810 Compare December 21, 2022 19:50

sadasu force-pushed the aws-sts-uninstall branch from f8ab810 to 6d903af Compare December 22, 2022 16:38

r4f4 reviewed Dec 22, 2022

View reviewed changes

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

sadasu force-pushed the aws-sts-uninstall branch from 6d903af to bf8531a Compare January 10, 2023 17:35

Ignore IAM Roles that the Installer is not authorized to access

54a7f13

sadasu force-pushed the aws-sts-uninstall branch from bf8531a to 54a7f13 Compare January 10, 2023 22:14

r4f4 reviewed Jan 10, 2023

View reviewed changes

openshift-ci bot assigned r4f4 Jan 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2023

openshift-merge-robot merged commit 14878f7 into openshift:master Jan 11, 2023

openshift-cherrypick-robot mentioned this pull request Feb 13, 2023

[release-4.12] OCPBUGS-7400: Check for AWS STS installation before trying to get all IAM Roles #6847

Closed

sadasu mentioned this pull request Apr 28, 2023

[release-4.12] OCPBUGS-7400: Check for AWS STS installation before trying to get all IAM Roles #7141

Merged

-					if quotaaws.IsUnauthorized(err) {
-						// Installer does not have access to this IAM role
-						// Ignore this IAM Role and donot report this error via
-						// lastError
-						search.unmatched[*role.Arn] = exists
-						continue
-					}
+                    var awsErr awserr.Error
+                    if errors.As(err, &awsErr) {
+                        switch awsErr.Code() {
+                        case "AccessDeniedException":
+						    // Installer does not have access to this IAM role
+						    // Ignore this IAM Role and do not report this error via
+						    // lastError
+						    search.logger.Debugf("AccessDenied to role %s. Expected if this is an STS install", *role.Arn)
+						    fallthrough
+					    case iam.ErrCodeNoSuchEntityException:
+						    search.unmatched[*role.Arn] = exists
+						    continue
+						default:
+						}
+			        }
+					if lastError != nil {
+					    search.logger.Debug(lastError)
+					}
+					lastError = errors.Wrapf(err, "get tags for %s", *role.Arn)
+				}

OCPBUGS-1769: Check for AWS STS installation before trying to get all IAM Roles #6666

OCPBUGS-1769: Check for AWS STS installation before trying to get all IAM Roles #6666

Uh oh!

Conversation

sadasu commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 5, 2022

Uh oh!

sadasu commented Dec 5, 2022

Uh oh!

openshift-ci-robot commented Dec 5, 2022

Uh oh!

sadasu commented Dec 5, 2022

Uh oh!

openshift-ci-robot commented Dec 5, 2022

Uh oh!

r4f4 Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

r4f4 Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon Dec 7, 2022 • 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!

openshift-ci-robot commented Dec 21, 2022

Uh oh!

openshift-ci-robot commented Dec 21, 2022

Uh oh!

openshift-ci-robot commented Dec 22, 2022

Uh oh!

r4f4 Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

sadasu Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

sadasu commented Dec 23, 2022

Uh oh!

sadasu commented Jan 4, 2023

Uh oh!

patrickdillon commented Jan 10, 2023

Uh oh!

openshift-ci bot commented Jan 10, 2023

Uh oh!

r4f4 left a comment

Choose a reason for hiding this comment

Uh oh!

r4f4 Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jan 11, 2023

Uh oh!

openshift-ci bot commented Jan 11, 2023

Uh oh!

patrickdillon commented Feb 13, 2023

Uh oh!

openshift-cherrypick-robot commented Feb 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sadasu commented Dec 5, 2022 •

edited

Loading

patrickdillon Dec 7, 2022 •

edited

Loading