[ms_rest_azure] Introduce support for acquiring token from Managed Service Identity#889
Conversation
|
@devigned @veronicagg @sarangan12 Feel free to review as you get chance. Thanks! |
|
@vishrutshah do you know what's going on with CI failing? what's the issue with nokogiri? is it just a CI problem? |
veronicagg
left a comment
veronicagg
left a comment
There was a problem hiding this comment.
Is there a sample or end to end scenario that shows this working for the purpose it's being created?
Code looks good, just a couple of minor questions.
| def self.get_settings(azure_environment = MsRestAzure::AzureEnvironments::Azure) | ||
| settings = ActiveDirectoryServiceSettings.new | ||
| settings.authentication_endpoint = azure_environment.active_directory_endpoint_url | ||
| settings.token_audience = azure_environment.resource_manager_endpoint_url |
There was a problem hiding this comment.
is the token_audience expected to be different from the ActiveDirectoryServiceSettings? how do we know which one should be?
There was a problem hiding this comment.
I guess yes from the documentation describing the MSI authentication flow, default token audience is https://management.azure.com/ and not https://management.core.windows.net/. but let me verify again because it might be just another audience that works
There was a problem hiding this comment.
As usual, you're right...it's just an another resource token audience. The one we have with ActiveDirectoryServiceSettings also works for acquiring token ...I'll refactor it then. thanks!
| it 'should initialize with Azure Cloud properties' do | ||
| msi_msi_aad_settings = MSIActiveDirectoryServiceSettings.get_azure_settings | ||
|
|
||
| expect(msi_msi_aad_settings).to be_a(ActiveDirectoryServiceSettings) |
There was a problem hiding this comment.
should this expect "MSIActiveDirectoryServiceSettings"?
There was a problem hiding this comment.
Good catch. Supposed to be tested for sub class instead of super.
|
vishrutshah
force-pushed
the
msi-support
branch
from
53bae34 to
3267304
Compare
Using latest bundler to resolve dependency
vishrutshah
force-pushed
the
msi-support
branch
from
ae60403 to
8738e1d
Compare
|
Waiting for release of azure-core version #Done |
Gemfile
Outdated
| gem 'azure_mgmt_traffic_manager', path: 'management/azure_mgmt_traffic_manager' | ||
| gem 'azure_mgmt_web', path: 'management/azure_mgmt_web' | ||
| gem 'azure_sdk', path: 'azure_sdk' | ||
| gem 'azure-core', git: 'https://github.com/katmsft/azure-ruby-asm-core.git', branch: 'master' |
There was a problem hiding this comment.
Revert use of private fork after release of azure-core version 0.1.11
|
@veronicagg Feel free to re-review as you get a chance. Thanks! |
|
@veronicagg Thanks for the review!! @sarangan12 Let me know if you have any feedback :) |
|
Thanks everyone for the review!! |
vishrutshah
changed the title
4 participants
Addresses #884