provider/aws: Delete access keys before deleting IAM user #7766
Conversation
|
Hi @dtolnay One of the core Terraform's principles is to not destroy/change things it doesn't manage however. It's sometimes hard to decide what is actually being managed - i.e. just user or user and their keys, but if keys can be managed separately via Terraform I think it's very clear that it should be that resource responsible for management of keys: The current behaviour with the current schema is as intended - i.e. Terraform should error out if there are existing access keys that are managed outside of Terraform. It is possible that they just simply manage the keys elsewhere and they forgot to delete those - these keys may potentially be in use and the user may want to verify that. We can however let users opt in to the destructive behaviour via special field, e.g. Would you mind making the behaviour opt-in - i.e. explicit rather than implicit? I saw that piece of code responsible for detaching the user from IAM Groups - and I suggest we treat that as a bug which will need to be fixed at some point. Generally speaking if Terraform is deleting/changing something outside the scope of a given resource, it's most likely (exceptions apply) a bug that should be reported and/or fixed. |
radeksimko
added
the
waiting-response
|
Makes sense, thanks for the feedback. I added a |
radeksimko
removed
the
waiting-response
| @@ -173,6 +184,16 @@ func resourceAwsIamUserDelete(d *schema.ResourceData, meta interface{}) error { | |||
| } | |||
| } | |||
|
|
|||
| for _, k := range accessKeys { | |||
There was a problem hiding this comment.
Would you mind moving this for loop along with var accessKeys []string into the force_destroy condition too?
It IMO makes sense from the context point of view.
| *marker = *r.Marker | ||
| } | ||
| *truncated = *r.IsTruncated | ||
| return true |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I changed both to return !lastPage for clarity.
|
I will have a look into how the pagination works in AWS SDK to verify it works as expected and we don't end up in endless loop or something like that. 😃 Your implementation may be completely correct - I just haven't seen that one anywhere else. Otherwise this is looking pretty good - just one small thing I'd like to ask you for - can you document this new field in https://github.com/hashicorp/terraform/blob/master/website/source/docs/providers/aws/r/iam_user.html.markdown ? Thanks. |
|
I added |
|
I have looked at the implementation of EachPage function in the SDK to confirm that our assumptions were correct here. It also explains why Thanks. 👍 |
* master: (34 commits) Update CHANGELOG.md provider/aws: Delete access keys before deleting IAM user (hashicorp#7766) Fix broken link to Consul demo (hashicorp#7789) provider/aws: `aws_redshift_cluster` `number_of_nodes` was having the (hashicorp#7771) provider/aws: Restore lost client.simpledbconn initialization Update vendored atlas client Make using `ssl_verify_mode` more robust (hashicorp#7769) Update CHANGELOG.md provider/aws: Rename the ECS Container Data Source test docs/azure: Small changes to remove the use of double Update docs to centralize on ARM-based Azure provider (hashicorp#7767) Update CHANGELOG.md Update CHANGELOG.md Add support for Kinesis streams shard-level metrics (hashicorp#7684) Update CHANGELOG.md Implementing aws_ami_launch_permission. (hashicorp#7365) Update CHANGELOG.md Add VersionString provider/aws: Set `storage_encrypted` to state in (hashicorp#7751) provider/fastly: Update go-fastly SDK (hashicorp#7747) ...
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Without this change:
We add access keys outside of Terraform because we do not want the secret keys in the tfstate.