-
Notifications
You must be signed in to change notification settings - Fork 341
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.
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
Support for multiple assume roles to get collective inventory #1830
Support for multiple assume roles to get collective inventory #1830
Conversation
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 27s |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 00s |
Hi Team, can we please get a review on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chandrakanthkannam thank you for submitting this PR! It will need a couple of things to be merged, a changelog fragment and integration tests. These tests will need to be marked as unsupported (see example here) since we don't have multiple AWS accounts available to our CI setup, but we still need tests that can be run locally for verification. Let me know if you have questions about either of those requirements, I'm happy to help.
- List of ARNs which can be assumed to get collective inventory result. | ||
- This conflicts with `assume_role_arn`. | ||
- You should still provide AWS credentials with enough privilege to perform the AssumeRole action. | ||
aliases: ["iam_role_arns"] | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
""" | |
version_added: 7.2.0 | |
""" |
if assume_role_arns: | ||
instances = [] | ||
for role in assume_role_arns: | ||
super().collective_roles(role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super().collective_roles(role) | |
self.collective_roles(role) |
It's already possible to pull from multiple accounts by having multiple inventory files in the inventory folder (or passed with multiple As such I'm not sure this is a feature we should add, especially since we can't properly test it. |
Oh interesting. This makes sense now that I'm reading through the general inventory plugins documentation, but it wasn't obvious to me from the ec2 lookup plugin documentation. I wonder if it would be worth updating the plugin doc to make this use case clear? |
I would be wary of duplicating too much of the documentation as it'll be liable to bit-rot, but it may be worth linking to the inventory guide and hinting that things like multiple inventory files (even using the same plugin) are possible. |
Thank you both for sharing your thoughts on this, I have not tested the scenario myself with multiple inventory files, I will give it a go shortly. Yeah reading through the plugin documentation didn't leave an impression that we can use multiple inventory files. |
with placing multiple inventory files in a folder and using the directory path is giving out the collective inventory, that is exactly I'm trying to achieve here with the changes. I guess I don't see any additional advantages ATM with my changes and not taking the directory approach. Anyway thanks @hakbailey for mentioning the missing pieces from the PR, I will keep them in mind for future. And thank you @tremble for pointing out that this is something achievable with the current version itself. |
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 10s |
Thanks for taking the time to open this PR. As already discussed, we can already do this using multiple inventory files. As such I'm going to close this PR, especially given we wouldn't be able to properly test it. |
SUMMARY
This change is to implement a ability to get a collective inventory.
Motivation came from our setup, we have a centralized management account from which things can be managed in underlying accounts and we been struggling to attain a central inventory from all accounts in one go, which lead to implement this and we strongly believe this can be useful to community and others can take advantage of it.
ISSUE TYPE
COMPONENT NAME
New option included to the plugin,
assume_role_arns: []
This can be taken advantage of if ansible controller is running from a central location and have ability to assume multiple roles in different accounts, this will result in a collective inventory from all the locations
example:
This will give out the inventory from all the 3 accounts, assuming the controller its running from have access to assume these roles.
NOTE: this will still expect a base credential setup, that is, either AWS Keys or EC2 instance profile(when running from an EC2)
ADDITIONAL INFORMATION
This new option conflicts with an existing option,
assume_role_arn
, we can use either one but not bothTo test this here is an example file I used,
and when running
ansible-inventory -i <file-name>--graph -vvv
gives me inventory from both accounts across 3 regions, and i used a base credential which has assuming access to these 2 roles used.Exception:
It fails with conflict error when used both
assume_role_arn
andassume_role_arns
like this in a fileerror:
Conflict here, cannot have not assume_role_arn and assume_role_arns used together