Skip to content
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.

Sign up for GitHub

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

[Form Recognizer] Sovereign clouds support #20522

Closed
wants to merge 34 commits into from
Closed

[Form Recognizer] Sovereign clouds support #20522

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
c01f303
Sovereign cloulds support
HarshaNalluru Feb 24, 2022
1e224f2
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Feb 24, 2022
6fb733c
SupportedClouds: 'Public,UsGov,China'
HarshaNalluru Feb 28, 2022
4b60789
Merge branch 'harshan/issue/19442' of https://github.com/HarshaNallur…
HarshaNalluru Feb 28, 2022
1396d04
remove location
HarshaNalluru Feb 28, 2022
e5bfacc
subscription().subscriptionId
HarshaNalluru Feb 28, 2022
31d73e0
function getAudience(): KnownFormRecognizerAudience
HarshaNalluru Feb 28, 2022
0a2fd3f
tests.yml update
HarshaNalluru Feb 28, 2022
72b494d
dummy commit (trigger pipeline)
HarshaNalluru Mar 1, 2022
202f01c
env file
HarshaNalluru Mar 1, 2022
118a7c7
Revert "env file"
HarshaNalluru Mar 1, 2022
9b85338
changes from feedback
HarshaNalluru Mar 1, 2022
cd5f600
Update sdk/formrecognizer/ai-form-recognizer/test/public/training.spe…
HarshaNalluru Mar 1, 2022
6a6ebf4
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Mar 1, 2022
8339675
more feedback
HarshaNalluru Mar 1, 2022
4f91175
improve docs
HarshaNalluru Mar 1, 2022
8a72062
elaborate
HarshaNalluru Mar 1, 2022
5e7fbd8
export { FormRecognizerAudience } from "./constants";
HarshaNalluru Mar 1, 2022
9d5e322
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Mar 2, 2022
b748056
MORE FEEDBACK
HarshaNalluru Mar 2, 2022
61a3a1d
Update sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
HarshaNalluru Mar 3, 2022
10acd7d
Update sdk/formrecognizer/ai-form-recognizer/tests.yml
HarshaNalluru Mar 3, 2022
b6c7973
testing unparallel sample runs
HarshaNalluru Mar 9, 2022
51efcf7
chnages
HarshaNalluru Feb 1, 2023
d22872a
Merge branch 'harshan/issue/19442' of https://github.com/HarshaNallur…
HarshaNalluru Feb 1, 2023
4929132
merge main
HarshaNalluru Feb 1, 2023
764c398
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Feb 2, 2023
014da41
fix build
HarshaNalluru Feb 2, 2023
acf524d
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-js into…
HarshaNalluru Feb 3, 2023
cde7237
custom forms test
HarshaNalluru Feb 3, 2023
89f473b
pdf file stream
HarshaNalluru Feb 3, 2023
143ae75
recordings
HarshaNalluru Feb 3, 2023
5326004
w2-single.png
HarshaNalluru Feb 3, 2023
baee1b6
Merge branch 'harshan/fix-FR-test-failures' of https://github.com/Har…
HarshaNalluru Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions sdk/formrecognizer/ai-form-recognizer/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@

### Features Added

- Supports the Sovereign clouds (AzureUSGovernment/AzureChinaCloud), pass the `audience` parameter in the client options for `DocumentAnalysisClient | DocumentModelAdministrationClient` to authenticate with a resource in a [Sovereign Cloud](https://docs.microsoft.com/azure/active-directory/develop/authentication-national-cloud).
HarshaNalluru marked this conversation as resolved.
Show resolved Hide resolved
Import and leverage the `KnownFormRecognizerAudience` enum to get the endpoint. Possible values for audience as of now.
HarshaNalluru marked this conversation as resolved.
Show resolved Hide resolved

- `KnownFormRecognizerAudience.AzureResourceManagerChina` (`"https://cognitiveservices.azure.cn/.default"`),
- `KnownFormRecognizerAudience.AzureResourceManagerGovernment` (`"https://cognitiveservices.azure.us/.default"`),
- `KnownFormRecognizerAudience.AzureResourceManagerPublicCloud` (`"https://cognitiveservices.azure.com/.default"`),
HarshaNalluru marked this conversation as resolved.
Show resolved Hide resolved

If `audience` is undefined, it is considered to be a PublicCloud (`"https://cognitiveservices.azure.com/.default"`)
HarshaNalluru marked this conversation as resolved.
Show resolved Hide resolved

### Breaking Changes

### Bugs Fixed
Expand Down
14 changes: 13 additions & 1 deletion sdk/formrecognizer/ai-form-recognizer/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/**
* Defines known cloud audiences for Form Recognizer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be helpful to customers to have a brief description of what this means, practically speaking.

*/
export enum KnownFormRecognizerAudience {
/** Azure China */
AzureResourceManagerChina = "https://cognitiveservices.azure.cn/.default",
Copy link
Member Author

@HarshaNalluru HarshaNalluru Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing special about these links relating to FormRecognizer, should it be KnownCognitiveServicesAudience instead and declared at some common place?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense if we had a common package for cognitive services which we don't...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can duplicate the code and use better name for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there ARM in the name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Container registry wasn't exactly arm, but also had the same name.

I just went with consistency, is there a better name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://management.chinacloudapi.cn sounds like an ARM scope but https://cognitiveservices.azure.cn is not. Perhaps use cognitive instead, e.g. CognitiveChina?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we use ARM in the name because those are management audiences that are used by the service team.

Copy link
Member Author

@HarshaNalluru HarshaNalluru Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options

  • KnownFormRecognizerAudience.AzureResourceManagerChina
  • KnownFormRecognizerAudience.AzureCognitiveServicesChina
  • KnownCognitiveServicesAudience.AzureChina

Copy link
Member

@jsquire jsquire Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By architect recommendation, .NET is using:

public static DocumentAnalysisAudience AzureChina { get; } 
public static DocumentAnalysisAudience AzureGovernment { get; } 
public static DocumentAnalysisAudience AzurePublicCloud { get; }

This aligns better with the naming in the Azure portal and documentation, as well as the pattern started by Azure.Identity::AzureAuthorityHosts. The ACR values are named that way specifically because they are using the ARM audience and plan on adding ARC-specific values.

For more context, please see the discussion here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second what .NET TA does here. @samvaity could you update Java to follow this recommended pattern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments. Going with this.

KnownFormRecognizerAudience

  • AzureChina
  • AzureGovernment
  • AzurePublicCloud

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop the "Known"? Doesn't add anything IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@witemple-msft having Known in the type name is a pattern the code generator is using for some time now (and transitively, all auto-generated clients) but I agree that it should be dropped. I understand this type is generated but does not get used to type anything in the public surface. Instead related places are typed as string so there should not be a name collision concern. @joheredi do you remember why we have that Known prefix?

/** Azure Government */
AzureResourceManagerGovernment = "https://cognitiveservices.azure.us/.default",
/** Azure Public Cloud */
AzureResourceManagerPublicCloud = "https://cognitiveservices.azure.com/.default",
}

/**
* The default AAD permissions scope for Cognitive Services.
* @internal
*/
export const DEFAULT_COGNITIVE_SCOPE = "https://cognitiveservices.azure.com/.default";
export const DEFAULT_COGNITIVE_SCOPE = KnownFormRecognizerAudience.AzureResourceManagerPublicCloud;

/**
* @internal
Expand Down
6 changes: 6 additions & 0 deletions sdk/formrecognizer/ai-form-recognizer/src/options/FormRecognizerClientOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ export interface FormRecognizerCommonClientOptions extends CommonClientOptions {
* Default: FormRecognizerApiVersion.Latest ("2021-09-30-preview")
*/
apiVersion?: FormRecognizerApiVersion;
/**
* Gets or sets the audience to use for authentication with Azure Active Directory.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc string would be clearer if it communicated that it is only necessary to override this setting if you're 1. using AAD/TokenCredential and 2. are using a cloud other than Azure Public.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gets/sets feels out of place for a property of an object that is consumed by the client constructor. Setting it after the fact won't change the scope used, right?

The rest of this doc string is great, but for the first line, how about just:

"The audience (scope) to use for authentication with Azure Active Directory."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gets/sets feels out of place for a property of an object that is consumed by the client constructor.

Copied this from some other package.

* The authentication scope will be set from this audience.
* See {@link KnownFormRecognizerAudience} for known audience values.
*/
audience?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the accepted approach to set the scope across all the different packages/languages?

Copy link
Member Author

@HarshaNalluru HarshaNalluru Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference:
I went through the notes, issues, references, and the recorded Arch board meeting on this.
And this was the approach followed in other places, which aligns with the discussion from the arch board meeting.

I'll however get it reviewed by architects and others. 🙂

}

/**
Expand Down
9 changes: 6 additions & 3 deletions sdk/formrecognizer/ai-form-recognizer/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import { bearerTokenAuthenticationPolicy } from "@azure/core-rest-pipeline";
import { createFormRecognizerAzureKeyCredentialPolicy } from "./azureKeyCredentialPolicy";
import { DEFAULT_COGNITIVE_SCOPE } from "./constants";
import { GeneratedClient, GeneratedClientOptionalParams } from "./generated";
import { DEFAULT_GENERATED_CLIENT_OPTIONS } from "./options/FormRecognizerClientOptions";
import {
DEFAULT_GENERATED_CLIENT_OPTIONS,
FormRecognizerCommonClientOptions,
} from "./options/FormRecognizerClientOptions";

import * as Mappers from "./generated/models/mappers";
import { createSerializer } from "@azure/core-client";
Expand Down Expand Up @@ -99,7 +102,7 @@ export const maybemap = <T1, T2>(value: T1 | undefined, f: (v: T1) => T2): T2 |
export function makeServiceClient(
endpoint: string,
credential: KeyCredential | TokenCredential,
options: GeneratedClientOptionalParams
options: GeneratedClientOptionalParams & Pick<FormRecognizerCommonClientOptions, "audience">
): GeneratedClient {
const client = new GeneratedClient(endpoint?.replace(/\/$/, ""), {
...DEFAULT_GENERATED_CLIENT_OPTIONS,
Expand All @@ -109,7 +112,7 @@ export function makeServiceClient(
const authPolicy = isTokenCredential(credential)
? bearerTokenAuthenticationPolicy({
credential,
scopes: DEFAULT_COGNITIVE_SCOPE,
scopes: options.audience ?? DEFAULT_COGNITIVE_SCOPE,
})
: createFormRecognizerAzureKeyCredentialPolicy(credential);

Expand Down