-
Notifications
You must be signed in to change notification settings - Fork 334
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
Respect AZURE_CLIENT_ID, ANSIBLE_AZURE_AUTH_SOURCE on inventory plugin #713
Conversation
@Fred-sun, any chance someone can review this PR? I'm in need this enhancement/fix and I really don't want to fork the repo. Thanks! |
@kingsleyadam Small change requests. |
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Updated, thanks for reviewing! |
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Changes:
|
@kingsleyadam can you provider test result of the fix/enhancement? I cannot get this work with MSI authorization |
hi any news on this? #712 is still present on azure.azcollection 1.12.0 |
@testotxt This PR still needs to be improved by contributors, so the merger cannot be promoted for the time being. Thank you very much! |
@kingsleyadam Kindly ping! |
@Fred-sun, @xuzhang3, finally getting back to this. Sorry for the delay. Here's the testing I've done from an Azure VM with multiple Managed/User Assigned Identities. No Fix, setting MSIThis is the code before any changes. Setting
Fix with MSI but no CLIENT IDThe below includes the recommended fix. But without setting the In this case the system assigned identity does not have any access. The error shows an attempt to use MSI, but failed due to access.
Fix with MSI and CLIENT IDThe following fix tests setting both
|
Friendly ping @Fred-sun |
@@ -223,8 +224,9 @@ def parse(self, inventory, loader, path, cache=True): | |||
raise | |||
|
|||
def _credential_setup(self): | |||
auth_source = environ.get('ANSIBLE_AZURE_AUTH_SOURCE', None) or self.get_option('auth_source') |
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.
@kingsleyadam I think this order should be adjusted, because the auth_source of the configuration is fetched first, and if the fetch is auto, the environment variable is fetched, thanks!
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.
@kingsleyadam I think this order should be adjusted, because the auth_source of the configuration is fetched first, and if the fetch is auto, the environment variable is fetched, thanks!
That's the issue I was running into. If I run self.get_option('auth_source')
first, it has a default value of auto
. Which won't fetch the environment variable ANSIBLE_AZURE_AUTH_SOURCE
value. To avoid this my logic here is to first check the environment variable for the auth source, if it's not set then fetch it from the config.
@kingsleyadam Are you still paying attention to this PR? There is a conflict in this PR, can you help solve the conflict? I will push forward the merger of this PR as soon as possible, thank you! |
Thanks for the heads up, I've merged the upstream/dev branch to mine and resolved the conflict. Should be good now. |
ansible-collections#713) * Attemp to pull environment variables if not set. * Set ansible azure auth source further upstream * Updates to documentation * Update plugins/doc_fragments/azure.py Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> * doc_fragments/azure.py Documentation update * Update plugins/doc_fragments/azure.py Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> * Move all logic within azure_rm_common.py * Removed unused import * Move back to setting auth source in inventory --------- Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
SUMMARY
Fixes #712, set the inventory plugin to respect the ANSIBLE_AZURE_AUTH_SOURCE environment variable, and ensures AZURE_CLIENT_ID environment variable is pulled in if set.
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION
Before Change with ANSIBLE_AZURE_AUTH_SOURCE and AZURE_CLIENT_ID set
After Change, a complete list of the inventory using
ansible-inventory
command.