Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Delete access keys before deleting IAM user #7766

Merged
merged 5 commits into from
Jul 25, 2016
Merged

provider/aws: Delete access keys before deleting IAM user #7766

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
717ec3c
provider/aws: Delete access keys before deleting IAM user
dtolnay Jul 21, 2016
cd4532b
provider/aws: Put IAM key removal behind force_destroy option
dtolnay Jul 22, 2016
4fb2623
provider/aws: Move all access key deletion under force_destroy
dtolnay Jul 22, 2016
cb9e4a3
Add iam_user force_destroy to website
dtolnay Jul 22, 2016
8808ad5
provider/aws: Improve clarity of looping over pages in delete IAM user
dtolnay Jul 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 44 additions & 25 deletions builtin/providers/aws/resource_aws_iam_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ func resourceAwsIamUser() *schema.Resource {
Default: "/",
ForceNew: true,
},
"force_destroy": &schema.Schema{
Type: schema.TypeBool,
Optional: true,
Default: false,
Description: "Delete user even if it has non-Terraform-managed IAM access keys",
},
},
}
}
Expand Down Expand Up @@ -132,39 +138,25 @@ func resourceAwsIamUserUpdate(d *schema.ResourceData, meta interface{}) error {
}
return nil
}

func resourceAwsIamUserDelete(d *schema.ResourceData, meta interface{}) error {
iamconn := meta.(*AWSClient).iamconn

// IAM Users must be removed from all groups before they can be deleted
var groups []string
var marker *string
truncated := aws.Bool(true)

for *truncated == true {
listOpts := iam.ListGroupsForUserInput{
UserName: aws.String(d.Id()),
}

if marker != nil {
listOpts.Marker = marker
}

r, err := iamconn.ListGroupsForUser(&listOpts)
if err != nil {
return err
}

for _, g := range r.Groups {
listGroups := &iam.ListGroupsForUserInput{
UserName: aws.String(d.Id()),
}
pageOfGroups := func(page *iam.ListGroupsForUserOutput, lastPage bool) (shouldContinue bool) {
for _, g := range page.Groups {
groups = append(groups, *g.GroupName)
}

// if there's a marker present, we need to save it for pagination
if r.Marker != nil {
*marker = *r.Marker
}
*truncated = *r.IsTruncated
return true
Copy link
Member

@radeksimko radeksimko Jul 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this going to loop forever if shouldContinue = true? Shouldn't this be something like

if !lastPage {
  return true
}

or possibly return !lastPage?

The previous implementation used ListGroupsForUserOutput.IsTruncated to make decisions when paginating. I don't mind you changing the implementation as long as the behaviour remains the same. Can you confirm the behaviour is exactly the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did confirm that the ListAccessKeysPages function is smart about not looping forever. My account is limited to 2 access keys per user so I set MaxItems to 1, changed this line to return false and confirmed that only 1 access key was deleted, then changed this line to return true and confirmed that both access keys were deleted (and it did not loop forever).

I have not tested ListGroupsForUserPages specifically but it goes through the same codepath. I can test later today or I can change it back if you prefer.

The shouldContinue is really only for iterating over a limited number of pages or searching for something until you find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed both to return !lastPage for clarity.

}
err := iamconn.ListGroupsForUserPages(listGroups, pageOfGroups)
if err != nil {
return fmt.Errorf("Error removing user %q from all groups: %s", d.Id(), err)
}

for _, g := range groups {
// use iam group membership func to remove user from all groups
log.Printf("[DEBUG] Removing IAM User %s from IAM Group %s", d.Id(), g)
Expand All @@ -173,6 +165,33 @@ func resourceAwsIamUserDelete(d *schema.ResourceData, meta interface{}) error {
}
}

// All access keys for the user must be removed
if d.Get("force_destroy").(bool) {
var accessKeys []string
listAccessKeys := &iam.ListAccessKeysInput{
UserName: aws.String(d.Id()),
}
pageOfAccessKeys := func(page *iam.ListAccessKeysOutput, lastPage bool) (shouldContinue bool) {
for _, k := range page.AccessKeyMetadata {
accessKeys = append(accessKeys, *k.AccessKeyId)
}
return true
}
err = iamconn.ListAccessKeysPages(listAccessKeys, pageOfAccessKeys)
if err != nil {
return fmt.Errorf("Error removing access keys of user %s: %s", d.Id(), err)
}
for _, k := range accessKeys {
_, err := iamconn.DeleteAccessKey(&iam.DeleteAccessKeyInput{
UserName: aws.String(d.Id()),
AccessKeyId: aws.String(k),
})
if err != nil {
return fmt.Errorf("Error deleting access key %s: %s", k, err)
}
}
}

request := &iam.DeleteUserInput{
UserName: aws.String(d.Id()),
}
Expand Down