Credentials fix #1090

sarangan12 · 2017-10-24T23:37:12Z

This PR is to fix the issue pointed in Stuart's email. Please review and approve.
Addressing #1092

veronicagg

Questions/comments inline.

veronicagg · 2017-10-25T17:49:57Z

generators/profilegen/src/resources/common/configurable.rb

+    #
+    # configures configurable options to default values
+    #
+    def setup_options


should this be called setup_default_options?

Sure. WIll change the name and regen in a few minutes

veronicagg · 2017-10-25T18:37:41Z

generators/profilegen/src/resources/common/configurable.rb

+        instance_variable_set(:"@#{key}", options.fetch(key, default_value))
+      end
+
+      default_value = get_credentials(self.tenant_id, self.client_id, self.client_secret, self.active_directory_settings)


is this needed?
if credentials are not passed in the options, should the "fetch" above get "credentials" from Configurable and set the value to the same thing that "get_credentials" below is doing?

I wonder why we need to get credentials in Configurable and remove it from Default, where it was.

As mentioned in the previous comment, the user may or may not provide credentials in the options. If he provides it, we will use it. Else, we will derive it from the other values supplied.

The 2 fetch are different. The first one (in setup_options) is getting the credentials from the default value and the second one is from the user supplied values (not the environment variables).

veronicagg · 2017-10-25T18:40:22Z

README.md

-  credentials: credentials,
+  tenant_id: ENV['AZURE_TENANT_ID'],
+  client_id: ENV['AZURE_CLIENT_ID'],
+  client_secret: ENV['AZURE_CLIENT_SECRET'],
  subscription_id: ENV['AZURE_SUBSCRIPTION_ID']
 }



would our guidelines from before profile changes not work for initializing credentials, I believe we were doing:

provider = MsRestAzure::ApplicationTokenProvider.new( ENV['AZURE_TENANT_ID'], ENV['AZURE_CLIENT_ID'], ENV['AZURE_CLIENT_SECRET']) credentials = MsRest::TokenCredentials.new(provider) @client = Azure::ARM::Resources::ResourceManagementClient.new(credentials) @client.subscription_id = @subscription_id

If so, should we leave them as they were, just updating the namespace for ResourceManagementClient? so we demonstrate the minimum change?

BTW, after this PR is done, we'd need to re-update the samples that were updated with the last release too.

The detailed explanation would be that we always want the user (of the SDK) to provide us with the tenant_id, client_id, client_secret and subscription_id. There is no escaping that. The credentials is just a derived value from these. The user may or may not provide it.

But, when you think about it, why would a user want to provide a derived value when he/she is supplying the original values anyway? That is the reason, I am updating the examples in readme and removed the credentials. But, if a user wants to provide it, then he is free to do that.

So... Credentials are not a bad thing to provide. This abstraction allows any token provider to be used.

sarangan12 · 2017-10-25T19:28:31Z

@veronicagg Changed the name of the method and regened everything. Please review and approve

sarangan12 · 2017-10-26T19:29:46Z

@veronicagg Modified per offline discussion

veronicagg · 2017-10-26T20:37:05Z

README.md

+* AZURE_TENANT_ID
+* AZURE_CLIENT_ID
+* AZURE_SUBSCRIPTION_ID
+* AZURE_CLIENT_SECRET


can we put simple instructions on how to set them in mac and windows? (I think we had this before)

veronicagg · 2017-10-26T20:38:47Z

README.md

+* AZURE_CLIENT_SECRET
+
+### Option 2 - Options Hash
+You can set the (above) values using the options hash:


I think we need to give context on what the options hash is - for example "the options hash getting passed as parameter when constructing a client for the service/profile"

veronicagg · 2017-10-26T20:51:38Z

README.md

@@ -63,6 +63,41 @@ Note: x64 Ruby for Windows is known to have some compatibility issues.

 # Getting Started with Azure Resource Manager Usage (Preview)

+## Prerequisite


I think would place this section after the "Install the rubygem packages" section, as that one provides context on authentication. Then we can talk about how to set this for usage in the packages.'

veronicagg · 2017-10-26T20:57:05Z

README.md

+}
+```
+
+### Option 3 - Combination of Environment Variables & Options Hash


what's the recommendation for the case in which the user wants to create credentials themselves? either from TokenProvider or MSITokenProvider? if they were to pass them in the hash, they would still need to pass in the 4 values specified above? so if credentials are passed, there are 5 values required for the hash?

veronicagg · 2017-10-26T20:57:56Z

generators/profilegen/src/resources/common/configurable.rb

@@ -50,7 +50,16 @@ def reset!(options = {})
        instance_variable_set(:"@#{key}", options.fetch(key, default_value))
      end

-      default_value = get_credentials(self.tenant_id, self.client_id, self.client_secret, self.active_directory_settings)
+      fail ArgumentError, 'tenant_id is nil' if self.tenant_id.nil?


can we give an indication on how to fix it? set the env vars or pass it in options?

also maybe match the message from

azure-sdk-for-ruby/runtime/ms_rest_azure/lib/ms_rest_azure/credentials/application_token_provider.rb

Line 50 in ab9cdb6

fail ArgumentError, 'Tenant id cannot be nil' if tenant_id.nil?

veronicagg · 2017-10-26T20:59:56Z

generators/profilegen/src/resources/common/configurable.rb

+      fail ArgumentError, 'client_id is nil' if self.client_id.nil?
+      fail ArgumentError, 'client_secret is nil' if self.client_secret.nil?
+      fail ArgumentError, 'subscription_id is nil' if self.subscription_id.nil?
+      fail ArgumentError, 'active_directory_settings is nil' if self.active_directory_settings.nil?


could this be nil at this point? this is not something we're asking the user to pass in, right?

Technically, yes. They will not be nil. I just wanted to be cautious that one of our users intentionally passing nil value for active_directory_settings in the options hash.

veronicagg

LGTM

stuartpreston · 2017-11-06T12:40:51Z

I had a look at this and it seems that whilst we can now use our own credentials rather than environment variables to connect, somewhere along the way we've pushed the ApplicationTokenProvider out of the way. Instead you now require us to send in the credentials and the settings directly to the client, whereas before we simply created an ApplicationTokenProvider with for the correct cloud environment and passed that in. Is this change in approach by design?

/cc @devigned @veronicagg @russellseymour

sarangan12 · 2017-11-06T23:16:25Z

@stuartpreston

Thanks for providing the feedback. I would like to request some clarifications. You have mentioned that we have pushed the ApplicationTokenProvider out of the way. Before the release of the profiles, we accessed the individual gems, say azure_mgmt_resources like this:

subscription_id = ENV['AZURE_SUBSCRIPTION_ID'] 
  provider = MsRestAzure::ApplicationTokenProvider.new(
      ENV['AZURE_TENANT_ID'],
      ENV['AZURE_CLIENT_ID'],
      ENV['AZURE_CLIENT_SECRET'])
credentials = MsRest::TokenCredentials.new(provider)
client = Azure::ARM::Resources::ResourceManagementClient.new(credentials)
client.subscription_id = subscription_id

So, we always passed the credentials to the client object. With the profiles, we could initialize the client as:

subscription_id = ENV['AZURE_SUBSCRIPTION_ID']
provider = MsRestAzure::ApplicationTokenProvider.new(
      ENV['AZURE_TENANT_ID'],
      ENV['AZURE_CLIENT_ID'],
      ENV['AZURE_CLIENT_SECRET'])
credentials = MsRest::TokenCredentials.new(provider)

options = {
      credentials: credentials,
      subscription_id: subscription_id
}

client = Azure::Resources::Profiles::Latest::Mgmt::Client.new(options)

So, both before and after, we have always used credentials and not the ApplicationTokenProvider. I would like to know if we are missing any scenarios.

If possible, I would like to setup a call where we could discuss your questions in detail. Please let me know.

stuartpreston · 2017-11-07T02:44:58Z

Ok, so our new symptoms are that if I use your code but use implicit values rather than references to environment variables, the new Client does not recognize the credentials passed through in the credentials object. Instead, we must set them explicitly:

require 'azure_mgmt_resources'

subscription_id = '1e0b427a-xxmy-subs-crip-ee558463ebbf'
provider = MsRestAzure::ApplicationTokenProvider.new(
      'a2b2d6bc-xxmy-tena-ntid-f97a7ac416d7',
      '020fb5ca-xxmy-clie-ntid-40ac3b94b40a',
      'my-client-secret')
credentials = MsRest::TokenCredentials.new(provider)

options = {
      credentials: credentials,
      subscription_id: subscription_id
}

client = Azure::Resources::Profiles::Latest::Mgmt::Client.new(options)

C:\projects\other\azuresdk015> chef exec ruby test2.rb
C:/Users/spreston/AppData/Local/chefdk/gem/ruby/2.4.0/gems/azure_mgmt_resources-0.15.1/lib/profil
es/common/configurable.rb:53:in `reset!': tenant_id is nil (ArgumentError)
        from C:/Users/spreston/AppData/Local/chefdk/gem/ruby/2.4.0/gems/azure_mgmt_resources-0.15
.1/lib/profiles/latest/modules/resources_profile_module.rb:78:in `initialize'
        from C:/Users/spreston/AppData/Local/chefdk/gem/ruby/2.4.0/gems/azure_mgmt_resources-0.15
.1/lib/profiles/latest/resources_latest_profile_client.rb:19:in `initialize'
        from test2.rb:15:in `new'
        from test2.rb:15:in `<main>'
C:\projects\other\azuresdk015>

So it appears the defaults are still loaded from the environment variables if credentials are not passed. Hence my comment that we can only use the client if we pass credentials (tenant_id, client_id, client_secret) directly and not through the TokenCredentials object, which leads to the question about why credentials passed along from a TokenCredentials provider seem to be ignored.

I am travelling this week so we'll have to stay asynchronous on this, next week looks a bit clearer in my calendar.

veronicagg · 2017-11-14T19:05:27Z

@stuartpreston at the moment we require that the options contain values for tenant_id, client_id, and client_secret. If you're providing credentials yourself, that's good too, you'd add credentials key,value pair as well in the options (as you have above). We should be picking up the credentials you pass in this case, and only rely on the rest of the values to calculate default credentials. We should be making a change to avoid having to pass the tenant_id, client_id, client_secret, when credentials are passed in, then your code should work as is (#1129)
Please see azure_mgmt_resources readme specifically on current usage https://github.com/Azure/azure-sdk-for-ruby/tree/master/management/azure_mgmt_resources#option-1---using-the-resources-profiles
Would this work for you?

stuartpreston · 2017-11-15T10:58:50Z

@veronicagg This is ok, but my thinking was that would want to abstract away from receiving credentials directly so that different auth methods can be used in future (e.g. we want to use a Certificate rather than a Client Secret). Otherwise the purpose of TokenCredentials isn't that clear any more.

As long as we can specify credentials (and not only have them inferred from the environment), we're OK - our problem now is keeping up with these changes in the SDK. The change from 0.14 to 0.15 was painful for us.

PS: Is there a way of using the certificate to authenticate today in the SDK? (as per https://gist.github.com/devigned/dae74a7ca54000f7b714)

sarangan12 requested review from devigned, veronicagg and salameer October 24, 2017 23:37

sarangan12 added 2 commits October 24, 2017 23:08

Code changes to fix issues with credentials & options

b1064fe

Regen everything

258ab1c

sarangan12 force-pushed the CredentialsFix branch from 26f8f7b to 258ab1c Compare October 25, 2017 06:50

veronicagg suggested changes Oct 25, 2017

View reviewed changes

sarangan12 added 2 commits October 25, 2017 12:03

PR Comments to change name of setup_options

d110575

Regen everything

1634b0c

veronicagg added P0 Sprint-108 labels Oct 26, 2017

sarangan12 added 2 commits October 26, 2017 12:00

PR Comments

7f3f5cc

Regen everything

511f774

Minor addition to readme file

2f87e29

veronicagg suggested changes Oct 26, 2017

View reviewed changes

sarangan12 added 2 commits October 26, 2017 14:49

PR Comments

c6c093b

Move position of the section

9a7b70e

veronicagg approved these changes Oct 26, 2017

View reviewed changes

sarangan12 merged commit 3b3c233 into Azure:master Oct 26, 2017

sarangan12 mentioned this pull request Oct 30, 2017

ARM namespace seems out of date for configurable and Default #1068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credentials fix #1090

Credentials fix #1090

sarangan12 commented Oct 24, 2017 •

edited by veronicagg

Loading

veronicagg left a comment

veronicagg Oct 25, 2017

sarangan12 Oct 25, 2017

veronicagg Oct 25, 2017

veronicagg Oct 25, 2017

sarangan12 Oct 25, 2017

veronicagg Oct 25, 2017

veronicagg Oct 25, 2017

sarangan12 Oct 25, 2017

devigned Oct 25, 2017

sarangan12 commented Oct 25, 2017

sarangan12 commented Oct 26, 2017

veronicagg Oct 26, 2017

sarangan12 Oct 26, 2017

veronicagg Oct 26, 2017

sarangan12 Oct 26, 2017

veronicagg Oct 26, 2017

veronicagg Oct 26, 2017

sarangan12 Oct 26, 2017

veronicagg Oct 26, 2017

veronicagg Oct 26, 2017

veronicagg Oct 26, 2017 •

edited

Loading

sarangan12 Oct 26, 2017

veronicagg left a comment

stuartpreston commented Nov 6, 2017

sarangan12 commented Nov 6, 2017

stuartpreston commented Nov 7, 2017

veronicagg commented Nov 14, 2017

stuartpreston commented Nov 15, 2017

		@@ -63,6 +63,41 @@ Note: x64 Ruby for Windows is known to have some compatibility issues.

		# Getting Started with Azure Resource Manager Usage (Preview)

		## Prerequisite

Credentials fix #1090

Credentials fix #1090

Conversation

sarangan12 commented Oct 24, 2017 • edited by veronicagg Loading

veronicagg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarangan12 commented Oct 25, 2017

sarangan12 commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veronicagg left a comment

Choose a reason for hiding this comment

stuartpreston commented Nov 6, 2017

sarangan12 commented Nov 6, 2017

stuartpreston commented Nov 7, 2017

veronicagg commented Nov 14, 2017

stuartpreston commented Nov 15, 2017

sarangan12 commented Oct 24, 2017 •

edited by veronicagg

Loading

veronicagg Oct 26, 2017 •

edited

Loading