Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.1k stars 1.21k forks source link

[Text Analytics]It will return error when use textAnalyticsClient with AAD authentication to run integration tests on UsGov cloud and China cloud #17563

Closed zzhxiaofeng closed 8 months ago

zzhxiaofeng commented 3 years ago

We are running live Tests against other clouds like US Gov and Azure China Cloud. The goal is to check whether new azure sdk package work with other clouds or not.

Error Description: When we use textAnalyticsClient with azure active directory credential to run integration tests on UsGov cloud and China cloud, it will return an error. The error message is shown as following, for more details please check here: image

Error Track: The failed reason is that when we use azure active directory credential, the scope of credential is just supported on Public cloud. The scope of credential defines the set of permissions being requested by the application. In text analytics client, the value of scope is here. image

Expected Behavior: Add different scopes for different clouds in Text Analytics SDK. The value of the scope in different clouds is follow: Public cloud: https://cognitiveservices.azure.com/.default UsGov cloud: https://cognitiveservices.azure.us/.default China cloud: https://cognitiveservices.azure.cn/.default

@benbp, @deyaaeldeen, @ramya-rao-a, @meeraharidasa for notification.

ramya-rao-a commented 3 years ago

@deyaaeldeen I would imagine other SDKs for cognitive services would have this issue as well, like Form Recognizer and Metrics Advisor

It is worth checking with the feature crews to see if our SDKs in other languages would have the same problem as well

deyaaeldeen commented 3 years ago

/cc @witemple-msft @joheredi @KarishmaGhiya. I am digging deeper into this and will share my findings.

ramya-rao-a commented 3 years ago

cc @witemple-msft and @KarishmaGhiya if they can share any history around this from the perspective of Form Recognizer and Metrics Advisor

benbp commented 3 years ago

@ramya-rao-a @deyaaeldeen FYI the cognitiveServicesEndpointSuffix arm template parameter will get auto-populated based on cloud in the live test pipeline.

You can see example usage from the NET tests: https://github.com/Azure/azure-sdk-for-net/blob/4162f6fa2445b2127468b9cfd080f01c9da88eba/sdk/formrecognizer/test-resources.json#L23-L26

ramya-rao-a commented 3 years ago

@benbp The ARM template will only help with the deployment. The problem here is that we are passing the wrong scope when trying to authenticate the client

benbp commented 3 years ago

@ramya-rao-a I see. If you set that scope dynamically as an ARM template output, then you can load it in from the environment when you construct it, something like:

var endpointSuffix = process.env.COGNITIVE_SERVICES_ENDPOINT_SUFFIX || ".cognitiveservices.azure.com";
const DEFAULT_COGNITIVE_SCOPE = "https://" + endpointSuffix + "/.default";

This is easier than having to do a switch on authority host, but also probably doesn't make it easier for customers, so perhaps it is better to do the switch.

EDIT: looks like something similar is done here: https://github.com/Azure/azure-sdk-for-js/pull/17443/files#diff-7a62be417c0ab079a338a0153787d9522a6246ec46b5e05bde866d0366fb7a2aR56

ramya-rao-a commented 3 years ago

@sarangan12 fyi, you will likely face the same problem in @azure/search-documents due to the hard-coded scope

deyaaeldeen commented 3 years ago

Quick update:

The scopes for TA are:

AzCloud: cognitiveservices.azure.com UsGov: cognitiveservices.azure.us China: cognitiveservices.azure.cn ~Germany: TBD but most likely cognitiveservices.azure.de~ deprecated

I looked at using the authority host to infer the scope but found that you can actually use the authority host for Azure Cloud to authenticate into a sovereign cloud resource. I found that container registry exposes an audience option that can be used to determine the scope: https://github.com/Azure/azure-sdk-for-js/blob/809f05c652ac9bb0fc08ce7048d8b5e33b9fc3e0/sdk/containerregistry/container-registry/src/containerRegistryClient.ts#L36-L41

benbp commented 3 years ago

@deyaaeldeen Germany is a deprecated cloud that we don't support for testing.

deyaaeldeen commented 3 years ago

@schaabs confirmed that it is not likely but it is valid for the authority host to be in one cloud while the resource being authenticated into is in another.

zzhxiaofeng commented 3 years ago

@benbp When we run text analytics integration tests in pipeline, some environment variables used in tests are not imported. For more details please check here. Please help to add these variables when you are free? These variables are shown as following: https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/textanalytics/ai-text-analytics/tests.yml#L8 d5be52e6-25dd-4aa5-98de-23191bfcba51

KarishmaGhiya commented 2 years ago

@deyaaeldeen @ramya-rao-a We did not run the Metrics Advisor tests on Sovereign or US Gov, I don't think the service team expected any customers using their service on these clouds.

v-xuto commented 2 years ago

@deyaaeldeen @ramya-rao-a Is there any progress on this issue? When is it planned to be fixed?

deyaaeldeen commented 2 years ago

.NET TA client library added an audience option to support other clouds in https://github.com/Azure/azure-sdk-for-net/pull/26583. I am planning on doing the same and will try to finish it in this milestone.

v-xuto commented 1 year ago

@deyaaeldeen Is there any progress on this issue?

deyaaeldeen commented 1 year ago

@v-xuto There is still in discussion, we need to design a way to support of sovereign clouds in the library.

github-actions[bot] commented 8 months ago

Hi @zzhxiaofeng, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.