Skip to content

[KeyVault] - Migrate Key Vault Admin package to Core V2 #15881

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

Merged
maorleger merged 17 commits into from
Jun 23, 2021
Merged

[KeyVault] - Migrate Key Vault Admin package to Core V2 #15881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fd26291
[KeyVault] - Migrate KeyVault Admin to Core V2
maorleger Jun 10, 2021
1d7c9ca
use create function
maorleger Jun 20, 2021
b59c5a4
refactor challenge auth callbacks
maorleger Jun 21, 2021
7621b58
remove console.count
maorleger Jun 21, 2021
0c430a8
update package dependencies
maorleger Jun 21, 2021
1546943
bring core-rest-pipeline back to preview11 of core-tracing so all cor…
maorleger Jun 21, 2021
36e57fd
fixup! bring core-rest-pipeline back to preview11 of core-tracing so …
maorleger Jun 21, 2021
678c995
remove duplicate package-version
maorleger Jun 21, 2021
10fa79b
move scopes to constants
maorleger Jun 21, 2021
5e05262
fix lint errors
maorleger Jun 21, 2021
8327656
no need to get recorder involved here
maorleger Jun 21, 2021
91e09ff
use hardcoded base url
maorleger Jun 21, 2021
f27812f
sort dependencies
maorleger Jun 22, 2021
6f6d147
regenerate with service version capabilities and apply feedback
maorleger Jun 22, 2021
d651fd1
fix format
maorleger Jun 22, 2021
fcf5176
bump core-lro dependencies to 1.0.6 and above
maorleger Jun 22, 2021
c3a334f
bump everyone to core-lro ^1.0.6
maorleger Jun 22, 2021
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
2 changes: 0 additions & 2 deletions common/config/rush/common-versions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,10 @@
"@azure/ms-rest-nodeauth": ["^0.9.2"],
// Idenity is moving from v1 to v2. Moving all packages to v2 is going to take a bit of time, in the mean time we could use v2 on the perf-identity tests.
"@azure/identity": ["^2.0.0-beta.4", "2.0.0-beta.3", "^1.1.0"],
// App Config uses keyvault-secrets in a sample, switch to latest once the preview becomes GA
// Issue #14771 tracks updating to these versions
"@microsoft/api-extractor": ["7.13.2"],
"prettier": ["2.2.1"],
// All packages should move to 1.0.0 once core-rest-pipeline 1.1.0 GAs
"@azure/core-tracing": ["1.0.0-preview.11"]

}
}
2 changes: 1 addition & 1 deletion sdk/communication/communication-identity/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
"@azure/communication-common": "^1.0.0",
"@azure/core-auth": "^1.3.0",
"@azure/core-http": "^1.2.0",
"@azure/core-lro": "^1.0.2",
"@azure/core-lro": "^1.0.6",
"@azure/core-paging": "^1.1.1",
"@azure/core-tracing": "1.0.0-preview.12",
"@azure/logger": "^1.0.0",
Expand Down
2 changes: 1 addition & 1 deletion sdk/communication/communication-phone-numbers/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"@azure/abort-controller": "^1.0.0",
"@azure/core-auth": "^1.3.0",
"@azure/core-http": "^1.2.0",
"@azure/core-lro": "^1.0.2",
"@azure/core-lro": "^1.0.6",
"@azure/core-paging": "^1.1.1",
"@azure/core-tracing": "1.0.0-preview.12",
"@azure/logger": "^1.0.0",
Expand Down
2 changes: 1 addition & 1 deletion sdk/formrecognizer/ai-form-recognizer/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
"prettier": "@azure/eslint-plugin-azure-sdk/prettier.json",
"dependencies": {
"@azure/core-auth": "^1.3.0",
"@azure/core-lro": "^1.0.2",
"@azure/core-lro": "^1.0.6",
"@azure/core-paging": "^1.1.1",
"@azure/core-http": "^1.2.0",
"@azure/core-tracing": "1.0.0-preview.12",
Expand Down
18 changes: 11 additions & 7 deletions sdk/keyvault/keyvault-admin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,21 @@
"sideEffects": false,
"dependencies": {
"@azure/abort-controller": "^1.0.0",
"@azure/core-auth": "^1.3.0",
"@azure/core-client": "^1.0.0",
"@azure/core-http": "^1.2.0",
Comment thread
maorleger marked this conversation as resolved.
Outdated
"@azure/core-lro": "^1.0.2",
"@azure/core-lro": "^1.0.6",
"@azure/core-paging": "^1.1.1",
"@azure/core-tracing": "1.0.0-preview.12",
"@azure/core-rest-pipeline": "1.1.0-beta.4",
"@azure/core-tracing": "1.0.0-preview.11",
"@azure/logger": "^1.0.0",
"@types/uuid": "^8.0.0",
"uuid": "^8.3.0",
"tslib": "^2.2.0"
"tslib": "^2.2.0",
"uuid": "^8.3.0"
},
"devDependencies": {
"@azure/abort-controller": "^1.0.0",
"@azure/core-util": "^1.0.0-beta.1",
"@azure/dev-tool": "^1.0.0",
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@azure/identity": "2.0.0-beta.4",
Expand All @@ -128,9 +132,9 @@
"@rollup/plugin-replace": "^2.2.0",
"@types/chai": "^4.1.6",
"@types/chai-as-promised": "^7.1.0",
"@types/sinon": "^9.0.4",
"@types/mocha": "^7.0.2",
"@types/node": "^8.0.0",
"@types/sinon": "^9.0.4",
"assert": "^1.4.1",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
Expand All @@ -150,7 +154,7 @@
"rollup-plugin-visualizer": "^4.0.4",
"sinon": "^9.0.2",
"source-map-support": "^0.5.9",
"typescript": "~4.2.0",
"typedoc": "0.15.2"
"typedoc": "0.15.2",
"typescript": "~4.2.0"
}
}
27 changes: 14 additions & 13 deletions sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,36 @@

```ts

import * as coreHttp from '@azure/core-http';
import { CommonClientOptions } from '@azure/core-client';
import { OperationOptions } from '@azure/core-client';
import { PagedAsyncIterableIterator } from '@azure/core-paging';
import { PollerLike } from '@azure/core-lro';
import { PollOperationState } from '@azure/core-lro';
import { TokenCredential } from '@azure/core-http';
import { TokenCredential } from '@azure/core-auth';

// @public
export interface AccessControlClientOptions extends coreHttp.PipelineOptions {
export interface AccessControlClientOptions extends CommonClientOptions {
Copy link
Copy Markdown
Contributor

@sadasant sadasant Jun 22, 2021

Choose a reason for hiding this comment

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

Maor and I have talked about whether this is a breaking change. I don’t think so, but Let’s get @xirzec ‘s thoughts

Copy link
Copy Markdown
Member

@xirzec xirzec Jun 22, 2021

Choose a reason for hiding this comment

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

There are 3 relatively minor breaking changes:

  • Optional property mode was removed from retryOptions since it only ever had a single enum value and was never used.
  • handleRedirects was removed from redirectOptions, as the policy can now be controlled by removing it from the pipeline or setting maxRedirects to 0.
  • keepAliveOptions were removed. Keep alive can be disabled on a per-request basis with disableKeepAlive.

These were all fairly niche options that tended to be used internally by our clients rather than set by consumers, but since we did expose them, we should consider what versioning implication this poses.

Alongside the change to remove _response, a strict semver interpretation would be to major version the package, but debatably a minor bump could be sufficient. /cc @chradek @ramya-rao-a @jeremymeng - I think we should have broad agreement on what our policy is here.

Copy link
Copy Markdown
Member Author

@maorleger maorleger Jun 22, 2021

Choose a reason for hiding this comment

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

I do plan to merge this PR, but leaving this conversation open if folks want to chime in. I'll also add it to the list of team meeting topics I have here 👍

Copy link
Copy Markdown
Member

@xirzec xirzec Jun 22, 2021

Choose a reason for hiding this comment

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

Since this package is still a beta I am less worried about it and don't think the discussion is blocking this PR

Copy link
Copy Markdown
Contributor

@ramya-rao-a ramya-rao-a Jun 23, 2021

Choose a reason for hiding this comment

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

Can we log an issue to ensure the discussion is continued and we come to a conclusion before this package goes GA?

Copy link
Copy Markdown
Member Author

@maorleger maorleger Jun 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, opened #15916

serviceVersion?: SUPPORTED_API_VERSIONS;
}

// @public
export interface CreateRoleAssignmentOptions extends coreHttp.OperationOptions {
export interface CreateRoleAssignmentOptions extends OperationOptions {
}

// @public
export interface DeleteRoleAssignmentOptions extends coreHttp.OperationOptions {
export interface DeleteRoleAssignmentOptions extends OperationOptions {
}

// @public
export interface DeleteRoleDefinitionOptions extends coreHttp.OperationOptions {
export interface DeleteRoleDefinitionOptions extends OperationOptions {
}

// @public
export interface GetRoleAssignmentOptions extends coreHttp.OperationOptions {
export interface GetRoleAssignmentOptions extends OperationOptions {
}

// @public
export interface GetRoleDefinitionOptions extends coreHttp.OperationOptions {
export interface GetRoleDefinitionOptions extends OperationOptions {
}

// @public
Expand Down Expand Up @@ -68,15 +69,15 @@ export class KeyVaultBackupClient {
}

// @public
export interface KeyVaultBackupClientOptions extends coreHttp.PipelineOptions {
export interface KeyVaultBackupClientOptions extends CommonClientOptions {
serviceVersion?: SUPPORTED_API_VERSIONS;
}

// @public
export type KeyVaultBackupOperationState = KeyVaultAdminPollOperationState<KeyVaultBackupResult>;

// @public
export interface KeyVaultBackupPollerOptions extends coreHttp.OperationOptions {
export interface KeyVaultBackupPollerOptions extends OperationOptions {
intervalInMs?: number;
resumeFrom?: string;
}
Expand Down Expand Up @@ -204,7 +205,7 @@ export enum KnownKeyVaultRoleScope {
export const LATEST_API_VERSION = "7.2";

// @public
export interface ListRoleAssignmentsOptions extends coreHttp.OperationOptions {
export interface ListRoleAssignmentsOptions extends OperationOptions {
}

// @public
Expand All @@ -213,7 +214,7 @@ export interface ListRoleAssignmentsPageSettings {
}

// @public
export interface ListRoleDefinitionsOptions extends coreHttp.OperationOptions {
export interface ListRoleDefinitionsOptions extends OperationOptions {
}

// @public
Expand All @@ -225,7 +226,7 @@ export interface ListRoleDefinitionsPageSettings {
export const SDK_VERSION: string;

// @public
export interface SetRoleDefinitionOptions extends coreHttp.OperationOptions {
export interface SetRoleDefinitionOptions extends OperationOptions {
assignableScopes?: KeyVaultRoleScope[];
description?: string;
permissions?: KeyVaultPermission[];
Expand Down
40 changes: 17 additions & 23 deletions sdk/keyvault/keyvault-admin/src/accessControlClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,12 @@
// Licensed under the MIT license.
/// <reference lib="esnext.asynciterable" />

import {
TokenCredential,
isTokenCredential,
signingPolicy,
createPipelineFromOptions,
InternalPipelineOptions
} from "@azure/core-http";
import { TokenCredential } from "@azure/core-auth";
import { PagedAsyncIterableIterator } from "@azure/core-paging";

import { challengeBasedAuthenticationPolicy, createTraceFunction } from "../../keyvault-common/src";
import { createTraceFunction } from "./tracingHelpers";
Comment thread
maorleger marked this conversation as resolved.
Outdated
import { KeyVaultClient } from "./generated/keyVaultClient";
import {
KeyVaultClientOptionalParams,
RoleAssignmentsListForScopeOptionalParams
} from "./generated/models";
import { RoleAssignmentsListForScopeOptionalParams } from "./generated/models";

import {
CreateRoleAssignmentOptions,
Expand All @@ -35,10 +26,12 @@ import {
DeleteRoleDefinitionOptions
} from "./accessControlModels";

import { SDK_VERSION, LATEST_API_VERSION } from "./constants";
import { SDK_VERSION, LATEST_API_VERSION, authenticationScopes } from "./constants";
import { mappings } from "./mappings";
import { logger } from "./log";
import { v4 as v4uuid } from "uuid";
import { bearerTokenAuthenticationPolicy } from "@azure/core-rest-pipeline";
import { createChallengeCallbacks } from "./challengeAuthenticationCallbacks";

const withTrace = createTraceFunction("Azure.KeyVault.Admin.KeyVaultAccessControlClient");

Expand Down Expand Up @@ -94,28 +87,29 @@ export class KeyVaultAccessControlClient {
: libInfo
};

const authPolicy = isTokenCredential(credential)
? challengeBasedAuthenticationPolicy(credential)
: signingPolicy(credential);
const serviceVersion = options.serviceVersion || LATEST_API_VERSION;

const internalPipelineOptions: InternalPipelineOptions = {
const clientOptions = {
...options,
loggingOptions: {
logger: logger.info,
allowedHeaderNames: [
additionalAllowedHeaderNames: [
"x-ms-keyvault-region",
"x-ms-keyvault-network-info",
"x-ms-keyvault-service-version"
]
}
};

const params: KeyVaultClientOptionalParams = createPipelineFromOptions(
internalPipelineOptions,
authPolicy
this.client = new KeyVaultClient(serviceVersion, clientOptions);

this.client.pipeline.addPolicy(
bearerTokenAuthenticationPolicy({
credential,
scopes: authenticationScopes,
challengeCallbacks: createChallengeCallbacks()
})
);
params.apiVersion = options.serviceVersion || LATEST_API_VERSION;
this.client = new KeyVaultClient(params);
}

/**
Expand Down
Loading