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

Jump to bottom

iam_role : support managing max session duration and deleting the instance profile it creates #62014

Merged

jillr merged 7 commits into ansible:devel from tremble:role_sessions

Sep 20, 2019

Contributor

tremble commented Sep 9, 2019 •

edited

Loading

SUMMARY

Add support for managing max session duration and deleting the instance profile we create

Additionally add a test suite for iam_role and iam_role_info

Split from basic testing in sts_assume_role
More comprehensive iam_role tests
iam_role_info tests

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

lib/ansible/modules/cloud/amazon/iam_role.py
lib/ansible/modules/cloud/amazon/iam_role_info.py

ADDITIONAL INFORMATION

Contributor

ansibot commented Sep 9, 2019

cc @bekelchik @piontas @wimnat
click here for bot help

ansibot added WIP affects_2.10 cloud feature needs_triage support:community aws module support:core labels

This comment has been minimized.

Sign in to view

ansibot added the ci_verified label

tremble force-pushed the role_sessions branch from 91c6c22 to 74d3743 Compare

September 10, 2019 12:55

ansibot removed the ci_verified label

tremble force-pushed the role_sessions branch from 74d3743 to ee67339 Compare

September 12, 2019 11:57

This comment has been minimized.

Sign in to view

ansibot added the ci_verified label

tremble force-pushed the role_sessions branch from ee67339 to 9eea7c5 Compare

September 12, 2019 12:19

ansibot removed the ci_verified label

tremble mentioned this pull request

iam_role: Deletion fails if inline policies aren't removed first #62278

Closed

tremble force-pushed the role_sessions branch from 9eea7c5 to 0fab7e8 Compare

September 13, 2019 18:11

tremble changed the title ~~WIP: iam_role iam_role_info tests and support managing max session duration~~ iam_role : support managing max session duration and deleting the instance profile it creates

ansibot added core_review has_issue and removed WIP labels

tremble added a commit to tremble/ansible that referenced this pull request


          iam_role : ansible#62014 Changelog

566168c

tremble added a commit to tremble/ansible that referenced this pull request


          iam_role : ansible#62014 Changelog

tremble force-pushed the role_sessions branch from 566168c to 3389368 Compare

September 13, 2019 18:25

Contributor Author

tremble commented Sep 14, 2019

Note: While this should possibly be split up into multiple PRs, I chose not to for the following reasons:

The change I actually wanted was supporting managing MaxSessionDuration
While writing the remaining test cases I ran into weird issues if the instance profiles weren't cleaned up, specifically because the roles had the same name but different paths. Hence the addition of support for removing the instance policy we create.
Because of the lack of existing (comprehensive) test suite the new features would need to depend on the test suite PRs and I'd end up rebasing over and over.

sivel removed the needs_triage label

gundalow added the pr_day label

jillr reviewed

View reviewed changes

Contributor

jillr left a comment

Overall this lgtm, a few very minor notes. Thank you so much for all these tests @tremble!

lib/ansible/modules/cloud/amazon/iam_role.py Outdated Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml Outdated Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml

+                    - iam_role is changed
+                - name: Short pause for role creation to finish
+                  pause:
+                    seconds: 10

Contributor

jillr Sep 19, 2019

This pause shouldn't be necessary in check mode.

Contributor Author

tremble Sep 20, 2019

I'd put the sleep in there to allow for the case of check_mode not being honoured and there being a delay before the role showed up. I don't mind removing it.

Contributor Author

tremble Sep 20, 2019

@jillr I've made the pauses optional. Interestingly I've not hit any issues yet, maybe something's been slowed down just enough, so I'm tempted to leave them all disabled.

If we remove the 'unsupported' flag we can keep an eye open for flakes and re-enable them easily.

Contributor

jillr Sep 20, 2019

I think the pauses on the cases we expect to do real work are fine, if we can get this test suite supported I'd definitely want them disabled on the check mode tests but happy to leave them off everywhere and see how it goes. If check mode actually makes changes that's a bug, so the risk of that bug is preferable to having every CI run take longer.

test/integration/targets/iam_role/tasks/main.yml Outdated Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml Outdated Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml Show resolved Hide resolved

test/integration/targets/iam_role/tasks/main.yml

+                # Inline Policy (test _info behaviour)
+                # XXX Not sure if it's a bug in Ansible or a "quirk" of AWS, but these two
+                # policies need to have at least different Sids or the second doesn't show

Contributor

jillr Sep 19, 2019

The console will let me have both of these policies with the same Sid and the docs suggest uniqueness is only required within a single JSON doc so I'm going to assume it's something we're doing. Do you think this is worth a bug to investigate? My inclination is that it's still less than desirable to duplicate Sids but I could be overlooking some use case.

Contributor Author

tremble Sep 20, 2019 •

edited

Loading

After taking a quick look at the iam_policy code, I'm inclined to suspect it's a bug in iam_policy. Possibly one we've had for a while: #30071 (comment)

Contributor Author

tremble Sep 21, 2019 •

edited

Loading

Ergh, FYI, not a bug just annoying default behaviour:

skip_duplicates:
By default the module looks for any policies that match the document you pass in, if there is a match it will not make a new policy object with the same rules. You can override this by specifying false which would allow for two policy objects with different names but same rules.

test/integration/targets/iam_role/tasks/main.yml Outdated

+                # Cleanup
+                # XXX iam_role fails to remove inline policies before deleting the role
+                - name: 'Attach inline policy a'

Contributor

jillr Sep 19, 2019

Suggested change

      
              - name: 'Attach inline policy a'
          
              - name: 'Detach inline policy a'

And below

tremble added 4 commits

September 20, 2019 09:59


          iam_role: Add support for managing MaxSessionDuration

1beddd2


          iam_role: Add support for deleting the IAM Instance Profiles we created

71c2d94


          iam_role: migrate all boto failures to fail_json_aws for consistency

2e42743


          iam_role: test validity of path so we can throw a more understandable…

c5d9aaa

… error

tremble added a commit to tremble/ansible that referenced this pull request


          iam_role : ansible#62014 Changelog

5101e44

tremble force-pushed the role_sessions branch from 3389368 to 5101e44 Compare

September 20, 2019 09:24


          iam_role: (integration tests) Split iam_role integration tests from s…

aaf6871

…ts_assume_role tests

- Make the iam_role tests more comprehensive
- Add tests for iam_role_info

tremble added a commit to tremble/ansible that referenced this pull request


          iam_role : ansible#62014 Changelog

bb6c47b

tremble force-pushed the role_sessions branch from 5101e44 to bb6c47b Compare

September 20, 2019 09:29

tremble added 2 commits

September 20, 2019 11:35


          iam_role: (integration tests) Make some of our pauses optional

aa903c7

If the tests appear to be flakey we may need to enable standard_pauses


          iam_role : ansible#62014 Changelog

04400c0

tremble force-pushed the role_sessions branch from bb6c47b to 04400c0 Compare

September 20, 2019 09:37

jillr approved these changes

View reviewed changes

Contributor

jillr left a comment

Tests are passing, thanks again!

jillr merged commit 40660e7 into ansible:devel

anas-shami pushed a commit to anas-shami/ansible that referenced this pull request


          iam_role : support managing max session duration and deleting the ins…

7124ea1

…tance profile it creates (ansible#62014)

* iam_role: Add support for managing MaxSessionDuration

* iam_role: Add support for deleting the IAM Instance Profiles we created

* iam_role: migrate all boto failures to fail_json_aws for consistency

* iam_role: test validity of path so we can throw a more understandable error

* iam_role: (integration tests) Split iam_role integration tests from sts_assume_role tests

- Make the iam_role tests more comprehensive
- Add tests for iam_role_info

* iam_role: (integration tests) Make some of our pauses optional

If the tests appear to be flakey we may need to enable standard_pauses

tremble deleted the role_sessions branch

October 8, 2019 08:08

ansible locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects_2.10 aws cloud core_review feature has_issue module pr_day support:community support:core