Fix AWS iam_user remove

[tinproject]

SUMMARY

Fixes #59078

As Boto3 documentation tells (https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/iam.html#IAM.Client.delete_user) before delete an user it is needed to delete some user's associated resources, like login profile(console password), access keys, etc..

Password ( DeleteLoginProfile )
Access keys ( DeleteAccessKey )
Signing certificate ( DeleteSigningCertificate )
SSH public key ( DeleteSSHPublicKey )
Git credentials ( DeleteServiceSpecificCredential )
Multi-factor authentication (MFA) device ( DeactivateMFADevice , DeleteVirtualMFADevice )
Inline policies ( DeleteUserPolicy )
Attached managed policies ( DetachUserPolicy )
Group memberships ( RemoveUserFromGroup )

The current implementation of iam_user don't take most of this into account so user's removal fails.
This PR removes all the user's associated resources before deletes the user.

Rationale of PR:

• I refactor the code a bit to be more clear.
• I don't include every step in it's own try/except as I don't believe it adds value over the already present: 'user can't be removed' with included stack-trace.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

iam_user

ADDITIONAL INFORMATION

To test this I've created manually the following IAM users and remove them with the iam_user module of this PR:

• test1 -> simple user with access keys
• test2 -> simple user with login profile (console password)
• test3 -> user with: access key, group membership, virtual MFA, attached policy, ssh public key, signing certificate, git https credentials.
• test4 -> user with: login profile, group membership, U2F MFA, inline policy, ssh public key, git https credentials.

[ansibot]

cc @joshsouza
click here for bot help

[ansibot]

@tinproject, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

[ansibot]

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/modules/cloud/amazon/iam_user.py:305:26: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/iam_user.py:334:30: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/iam_user.py:346:30: E128 continuation line under-indented for visual indent

click here for bot help

[jillr]

tested with a user that has keys, console login, and policies, lgtm

[tinproject]

Any more requested changes to this PR?
Ping @s-hertel @willthames

[tremble]

Sorry for the slow/late niggles.

[tremble]

@tinproject needs a rebase because #23382 finally landed

[s-hertel]

Couple suggestions to clean up the exception handling, but looks good to me once those are addressed.

[tremble]

@tinproject it looks like the rebase has dropped the tests you'd put in place to test behaviour when deleting users which still have policies/group membership attached to them.

Please would you add them back.

[tinproject]

I made this PR to FIX the iam_user deletion because I was tired to do it manually every time there's the need to remove an user on my job. I was so naive to use this module to automate IAM because it never worked for real IAM users removal.

I've done this on my free time, and my allotted free time is well pass through. My original intention is to provide a fix to user deletion, nor to update the module to use AnsibleAWSModule, nor to add integration tests that not exist before and then conflict completely with others, with completely different style, of other module that use this module test file..

I believe Red Hat Ansible must rethink it's relationship with the community about what is needed on PR and what are nice to haves. More than three months to add a fix, it have no sense.

@tremble Feel free to recover the tests. I'm moving my IAM user process to use Terraform/Cloudformation.

[s-hertel]

@tinproject I understand your frustration. There is only one Red Hatter working (mostly) full time on AWS pull requests and it's really a community-driven process. Most of the active community is in the IRC channel #ansible-aws on irc.freenode.net. It's a good place to ping people if you feel a pull request is stuck or feel that unreasonable demands have been made or just need some help implementing changes.

Updating the module to use AnsibleAWSModule was not required, but it did simplify the exception handling code.

Integration tests are required for new modules and features, but depending on the urgency of the bugfix may also be excluded. Generally tests are requested for bugs and are considered more important than just a "nice to have" because they prevent the use case being broken in the future. As most of these modules are community maintained and often have new contributors, breakage happens easily without tests. You can see from the history of https://github.com/ansible/community/blob/master/group-aws/integration.md that we are progressing, though it's slow work.

It's often difficult to get the balance right between getting a PR in as-is, and forcing contributors to spend a lot of time improving it, refactoring other bits of code, and adding tests, we may have failed you in this occurrence, and for that we apologize.

Given the reviews so far, and the detailed manual testing you provided I think we are good to merge your PR as is.

[tinproject]

Thanks!