NO-JIRA: cleanup iamRoles as part of nightly test cleanup #4382
Conversation
NiniOak
added
pr/no-changelog
NiniOak
requested review from
a team,
nathancoleman and
xwa153
and removed request for
a team
NiniOak
changed the title
| func(page *iam.ListRolesOutput, lastPage bool) bool { | ||
| for _, role := range page.Roles { | ||
| roleName := aws.StringValue(role.RoleName) | ||
| if strings.HasPrefix(roleName, rolePrefix) { | ||
| rolesToDelete = append(rolesToDelete, role) | ||
| } | ||
| } | ||
| return !lastPage | ||
| }) |
There was a problem hiding this comment.
could we extract this to a named function and pass that in to make this a little more readable?
| }, func(page *iam.ListAttachedRolePoliciesOutput, lastPage bool) bool { | ||
| for _, policy := range page.AttachedPolicies { | ||
| _, err := iamClient.DetachRolePolicy(&iam.DetachRolePolicyInput{ | ||
| RoleName: roleName, | ||
| PolicyArn: policy.PolicyArn, | ||
| }) | ||
| if err != nil { | ||
| fmt.Printf("Failed to detach policy %s from role %s: %v", *policy.PolicyArn, *roleName, err) | ||
| } | ||
| } | ||
| return !lastPage | ||
| }) |
There was a problem hiding this comment.
similar idea here, can we move this to a named function so that this is a bit more readable
| PolicyArn: policy.PolicyArn, | ||
| }) | ||
| if err != nil { | ||
| fmt.Printf("Failed to detach policy %s from role %s: %v", *policy.PolicyArn, *roleName, err) |
There was a problem hiding this comment.
do we need to worry about the nil pointer deref here?
There was a problem hiding this comment.
As we are not de-ref the err and just check the nullity of the var, I think it's OK. Please correct me if I were wrong.
And the PR looks all good from my side! Thank you for your great work Anita!
There was a problem hiding this comment.
the issue is on *roleName and *policy.PolicyArn which could potentially be a nil pointer
There was a problem hiding this comment.
They are coming from page.AttachedPolicies and page.Roles which are offered by the aws-sdk-go package, I checked the source, both are list of pointers. So maybe we could trust aws? 😂
There was a problem hiding this comment.
it's probably better to not trust that we're getting good intput and code defensively here, aws provides functions for dealing with pointers and we should probably use them here to avoid the potential nil derefs https://docs.aws.amazon.com/sdk-for-go/api/aws/#StringValue
There was a problem hiding this comment.
Yeah, I saw that and used that function to deref those values. Thanks for pointing it out
| PolicyArn: policy.PolicyArn, | ||
| }) | ||
| if err != nil { | ||
| fmt.Printf("Failed to detach policy %s from role %s: %v\n", aws.StringValue(policy.PolicyArn), *roleName, err) |
There was a problem hiding this comment.
Sorry, since we are also de-ref roleName, do we also need the aws.StringValue to wrap it? The same applies to line 835.
| var rolesToDelete []*iam.Role | ||
| rolePrefix := "consul-k8s-" | ||
| err := iamClient.ListRolesPagesWithContext(ctx, &iam.ListRolesInput{}, | ||
| func(page *iam.ListRolesOutput, lastPage bool) bool { | ||
| return filterRolesWithPrefix(page, lastPage, &rolesToDelete, rolePrefix) | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list roles: %v", err) | ||
| } |
There was a problem hiding this comment.
#nit I think this and the filterRolesWithPrefix func get a little easier to understand with less dereferencing and whatnot if we just let the func that we're calling out to collect the roles itself.
rolesToDelete, err := filterIAMRolesWithPrefix(ctx, iamClient, "consul-k8s-")
if err != nil {
return fmt.Errorf("failed to list roles: %w", err)
}func filterIAMRolesWithPrefix(ctx context.Context, iamClient *iam.IAM, prefix string) ([]*iam.Role, error) {
var roles []*iam.Role
err := iamClient.ListRolesPagesWithContext(ctx, &iam.ListRolesInput{},
func(page *iam.ListRolesOutput, lastPage bool) bool {
for _, role := range page.Roles {
name := aws.StringValue(role.RoleName)
if strings.HasPrefix(name, prefix) {
roles = append(roles, role)
}
}
// can just always return true here since we always want to see all of the roles
return true
})
if err != nil {
return nil, err
}
return roles, nil
}| if len(rolesToDelete) == 0 { | ||
| fmt.Println("Found no iamRoles to clean up") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
#nit might be useful to log how many roles we're attempting to clean up in the else case
* NO-JIRA: cleanup iam_roles as part of nightly test cleanup * remove reference to log, use fmt * fix panic in cleanup code * code review feedback * code review feedback
* NO-JIRA: cleanup iam_roles as part of nightly test cleanup * remove reference to log, use fmt * fix panic in cleanup code * code review feedback * code review feedback
Changes proposed in this PR
PROBLEM
Acceptance test in CI are currently failing with error
LimitExceeded: Cannot exceed quota for RolesPerAccount: 1000.SOLUTION
Update the
aws-acceptance-test-cleanupscript to include cleaning up iamroles created as part of test environment setup.How I've tested this PR
Tested locally to ensure all exisiting iAMRoles created as part of previous acceptance test runs were cleaned up using this script
How I expect reviewers to test this PR
Checklist