Adds property baseUri on the ServiceClientOptions #457

sadasant · 2021-07-27T20:09:54Z

Added a new property baseUri on the ServiceClientOptions, so that when a TokenCredential is sent to ServiceClient, it now automatically sets the scope of future token requests based on the options.baseUri property.

This should address Azure/azure-sdk-for-js#15945

…if a token credential is passed

lib/serviceClient.ts

Changelog.md

lib/serviceClient.ts

ramya-rao-a · 2021-07-28T00:09:27Z

Any concerns with subsequent interfaces extending ServiceClientOptions and redeclaring the baseUri property?

sadasant · 2021-07-28T16:38:43Z

@ramya-rao-a I don’t know of use cases like that. This approach is simpler than others we’ve talked, but it’s less considerate of edge cases. We could have something more automated, like a function on the adapter that gets called on getToken and lets you replace the scope cc: @chradek

Changelog.md

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

sadasant · 2021-07-29T20:41:55Z

Oh I understood now that you meant if nothing breaks if the baseUri property is re-declared on constructors (I thought you meant late assignments). Testing to be sure!

chradek · 2021-07-29T20:57:20Z

Any concerns with subsequent interfaces extending ServiceClientOptions and redeclaring the baseUri property?

In our repo there are several instances where ServiceClientOptions is extended and the sub-interface defines baseUri?: string. This is compatible with the change.

ApplicationInsightsDataClientOptions
LocalSearchClientOptions
LogAnalyticsClientOptions
ServiceFabricClientOptions

However I did notice that more common that baseUri was endpoint. endpoint is used to help construct the baseUri for the services that use it (cognitiveservices-entitysearch is an example).
https://github.com/Azure/azure-sdk-for-js/blob/ce2c1f5310bf0e71dbf8bc2b29d5731a9dfd961f/sdk/cognitiveservices/cognitiveservices-entitysearch/src/entitySearchClientContext.ts#L40-L48

    super(credentials, options);

    this.endpoint = 'https://api.cognitive.microsoft.com';
    this.baseUri = "{Endpoint}/bing/v7.0";
    this.requestContentType = "application/json; charset=utf-8";
    this.credentials = credentials;
    if (options.endpoint !== null && options.endpoint !== undefined) {
      this.endpoint = options.endpoint;
    }

So @sadasant, I don't know if you'll also need to make more changes to support clients that expose endpoint. With regards to the IdentityAdapter, where does the scope come from if you don't override it in the constructor?

sadasant · 2021-07-29T21:32:42Z

@chradek In the AzureIdentityCredentialAdapter (part of this repo) the scope is either the default one, or is assigned at the constructor.

On Identity, the scope is received on each getToken call, which is why the translation is a bit weird. I think that if we wanted to do something robust that could work for every possible use case, we would allow the adapter to overwrite the constructor scope on getToken. However, since our target audience for this adapter is the generated SDKs, I believe the current approach of this PR to be a reasonable one, since baseUri is always sent through by the children classes.

Let me know if I’m missing something 🤔

sadasant · 2021-07-29T21:35:06Z

@chradek for the case you presented here: https://github.com/Azure/azure-sdk-for-js/blob/ce2c1f5310bf0e71dbf8bc2b29d5731a9dfd961f/sdk/cognitiveservices/cognitiveservices-entitysearch/src/entitySearchClientContext.ts#L40-L48

I believe that scope is specific to the service version they use. For them, the default scope is likely sufficient, unless they’re working on external clouds.

But to confirm I would need to test it, or speak with somebody owning the cognitive search client. Let me see if I can find someone.

chradek · 2021-07-29T21:53:51Z

I believe that scope is specific to the service version they use. For them, the default scope is likely sufficient, unless they’re working on external clouds.

For what it's worth, I don't think what cognitive services is doing with endpoint precludes us from exposing baseUri.
It's possible that for cognitive services, we'd have to construct the baseUri to pass to the ServiceClient if the user specified a custom endpoint, but we'd need to know what the scopes look like for non-public clouds for those services. But that can be done if needed and I don't think has to be part of this PR.

lib/serviceClient.ts

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

…m/sadasant/ms-rest-js into fix/azure-sdk-for-js-issues-15945

sadasant · 2021-07-30T01:13:38Z

@ramya-rao-a sounds good! thank you!

ramya-rao-a · 2021-08-12T02:52:35Z

On another set of second thoughts...

For other Azure clouds, the authority host needs to be set when creating the TokenCredential from @azure/identity. If the authority host was made a read only property on the credential, then can we not do something like the below in the adapter?

switch (azureTokenCredential.authorityHost) {
      case "https://login.chinacloudapi.cn":
        this.scopes = "https://management.chinacloudapi.cn/.default"
        break;
      // and so on for other clouds
      default:
        break;
    }

ramya-rao-a · 2021-08-12T20:38:02Z

@deyaaeldeen has been experimenting with the @azure/batch package and we concluded that we cannot reverse engineer the scope from the baseUri. Therefore, we are going to keep this @azure/identity support in Track 1 code gen only for the mgmt plane packages. This would mean that us hardcoding the mgmt related scope in the adapter in ms-rest-js is ok as long as we find a way to deal with different clouds. And so comes my suggestion in the previous comment on utilizing the authority host passed to the credential constructor to determine the right scope for mgmt plane packages

sadasant · 2021-08-12T21:00:13Z

@ramya-rao-a Ok I’ll work on a PR for v1

sadasant · 2021-08-12T21:13:23Z

@ramya-rao-a @deyaaeldeen on second thought

From the perspective of Identity, authorityHost does not fully overlap with scopes. Scopes are about what one wants to gain access to, and authorityHost is about who we want to authenticate with.

I think that’s part of the problem on ms-rest-js, baseUri is not a reliable scope for the same reason. It would be better if we could derive the scope during the authentication request, and not at the constructor.

ramya-rao-a · 2021-08-13T02:57:40Z

Fair enough

In that case, lets guard the changes in this PR to only take affect if the baseUri matches the ARM scope
That way we atleast reduce the scope of this change to something we are sure should be updated.

We have logged Azure/autorest.typescript#1153 to ensure that only the ARM packages get the TokenCredential support in the client constructor so that we don't have to worry what scope to use for data plane packages

…cloud

sadasant · 2021-08-14T00:20:00Z

I pushed a change that makes it so the baseUri is used as the scope only if it matches the management clouds. Please let me know if there’s anything else I can do!

ramya-rao-a · 2021-08-14T00:29:08Z

lib/serviceClient.ts

+      if (
+        options?.baseUri &&
+        azureManagementClouds.find((cloud) => options!.baseUri!.indexOf(cloud) > -1)
+      ) {


Can we simplify this?

Suggested change

if (

options?.baseUri &&

azureManagementClouds.find((cloud) => options!.baseUri!.indexOf(cloud) > -1)

) {

if (

options?.baseUri &&

azureManagementClouds.includes(options?.baseUri)

) {

Includes breaks in the browser, I think https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

Well, InternetExplorer, so I guess it’s ok to use it

I merged it, but keep in mind that includes will work only for exact matches.

And that is good right? We want exact matches

My brain is kinda fried right now. 🙇‍♂️ 🙇‍♂️ slowly leaves

As far as we know, the baseuri indeed needs to match the resource manager endpoints we have listed here. If there are cases where this is not true, I would much rather hear that from a customer than make assumptions ourselves

Addressed here: 7224651

Changelog.md

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

Changelog.md

ramya-rao-a · 2021-08-18T05:38:12Z

lib/serviceClient.ts

    let serviceClientCredentials: ServiceClientCredentials | undefined;
    if (isTokenCredential(credentials)) {
-      serviceClientCredentials = new AzureIdentityCredentialAdapter(credentials);
+      let scope: string | undefined = undefined;
+      const azureManagementClouds = [


Nit: This const can be defined outside of this class to avoid initialization every time a ServiceClient is created

Addressed here: 7224651

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

ramya-rao-a · 2021-08-18T05:43:24Z

Another nit: Is the term resource management endpoint preferred over resource manager endpoints?
https://github.com/Azure/ms-rest-azure-env/blob/master/lib/azureEnvironment.ts#L271 uses the latter

And official docs refer to the latter as well. See https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/overview

…nditional improvement

sadasant · 2021-08-18T17:58:31Z

@ramya-rao-a I think I’ve addressed all the feedback through this commit: 7224651

lib/serviceClient.ts

Changelog.md

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

sadasant added 2 commits July 26, 2021 23:11

draft for how to dynamically set the scope on the service credential …

3d22a0b

…if a token credential is passed

suggestion from Christopher Radek

1d2efa5

sadasant requested review from ramya-rao-a and chradek July 27, 2021 20:09

sadasant self-assigned this Jul 27, 2021

version update

c0c10f4

chradek reviewed Jul 27, 2021

View reviewed changes

lib/serviceClient.ts Outdated Show resolved Hide resolved

assigning baseUri based on the options

9f57b8d

sadasant marked this pull request as ready for review July 27, 2021 23:34

sadasant requested review from deyaaeldeen, jeremymeng and xirzec as code owners July 27, 2021 23:34

ramya-rao-a suggested changes Jul 28, 2021

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

lib/serviceClient.ts Outdated Show resolved Hide resolved

version to 2.6.0, better documentation

557e537

ramya-rao-a reviewed Jul 29, 2021

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

Update Changelog.md

ca175f5

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

added a test

5bfb351

sadasant requested a review from ramya-rao-a July 29, 2021 21:54

ramya-rao-a reviewed Jul 29, 2021

View reviewed changes

lib/serviceClient.ts Outdated Show resolved Hide resolved

sadasant and others added 4 commits July 29, 2021 19:20

Update lib/serviceClient.ts

1d4884b

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

better test

d1dd132

Merge branch 'fix/azure-sdk-for-js-issues-15945' of https://github.co…

8bce5e5

…m/sadasant/ms-rest-js into fix/azure-sdk-for-js-issues-15945

changelog date increase

dd32d33

sadasant mentioned this pull request Aug 11, 2021

Cannot create Storage Container in National Cloud (China) Azure/azure-sdk-for-js#16861

Closed

3 tasks

sadasant force-pushed the fix/azure-sdk-for-js-issues-15945 branch from 3946f75 to dd32d33 Compare August 13, 2021 23:56

sadasant added 2 commits August 14, 2021 00:12

only applying baseUri as the scope if it matches an Azure management …

5ed7aca

…cloud

fixed the related test

26a5d15

added another test

e3f3cdb

ramya-rao-a reviewed Aug 14, 2021

View reviewed changes

sadasant and others added 4 commits August 13, 2021 20:48

Update Changelog.md

3d3deab

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

using the endpoints without .core

0257ff1

better comments

5149f99

better comments

caaba1f

sadasant requested a review from ramya-rao-a August 18, 2021 01:17

ramya-rao-a suggested changes Aug 18, 2021

View reviewed changes

Update Changelog.md

c13b5d0

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

addressed feedback: constant to outer scope, manager wording and a co…

7224651

…nditional improvement

sadasant requested a review from ramya-rao-a August 18, 2021 18:00

ramya-rao-a suggested changes Aug 18, 2021

View reviewed changes

lib/serviceClient.ts Outdated Show resolved Hide resolved

Changelog.md Outdated Show resolved Hide resolved

sadasant and others added 2 commits August 18, 2021 15:31

Update Changelog.md

09ec307

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

Update lib/serviceClient.ts

6155051

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

ramya-rao-a approved these changes Aug 18, 2021

View reviewed changes

sadasant merged commit 45f89c9 into Azure:master Aug 18, 2021

sadasant deleted the fix/azure-sdk-for-js-issues-15945 branch August 18, 2021 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds property baseUri on the ServiceClientOptions #457

Adds property baseUri on the ServiceClientOptions #457

sadasant commented Jul 27, 2021

ramya-rao-a commented Jul 28, 2021

sadasant commented Jul 28, 2021 •

edited

Loading

sadasant commented Jul 29, 2021

chradek commented Jul 29, 2021 •

edited

Loading

sadasant commented Jul 29, 2021

sadasant commented Jul 29, 2021

chradek commented Jul 29, 2021

sadasant commented Jul 30, 2021

ramya-rao-a commented Aug 12, 2021 •

edited

Loading

ramya-rao-a commented Aug 12, 2021

sadasant commented Aug 12, 2021

sadasant commented Aug 12, 2021

ramya-rao-a commented Aug 13, 2021

sadasant commented Aug 14, 2021

ramya-rao-a Aug 14, 2021

sadasant Aug 14, 2021

sadasant Aug 14, 2021

ramya-rao-a Aug 14, 2021

sadasant Aug 14, 2021

ramya-rao-a Aug 18, 2021

sadasant Aug 18, 2021

ramya-rao-a Aug 18, 2021

sadasant Aug 18, 2021

ramya-rao-a commented Aug 18, 2021

sadasant commented Aug 18, 2021

Adds property baseUri on the ServiceClientOptions #457

Adds property baseUri on the ServiceClientOptions #457

Conversation

sadasant commented Jul 27, 2021

ramya-rao-a commented Jul 28, 2021

sadasant commented Jul 28, 2021 • edited Loading

sadasant commented Jul 29, 2021

chradek commented Jul 29, 2021 • edited Loading

sadasant commented Jul 29, 2021

sadasant commented Jul 29, 2021

chradek commented Jul 29, 2021

sadasant commented Jul 30, 2021

ramya-rao-a commented Aug 12, 2021 • edited Loading

ramya-rao-a commented Aug 12, 2021

sadasant commented Aug 12, 2021

sadasant commented Aug 12, 2021

ramya-rao-a commented Aug 13, 2021

sadasant commented Aug 14, 2021

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

ramya-rao-a commented Aug 18, 2021

sadasant commented Aug 18, 2021

sadasant commented Jul 28, 2021 •

edited

Loading

chradek commented Jul 29, 2021 •

edited

Loading

ramya-rao-a commented Aug 12, 2021 •

edited

Loading