[KeyVault] - Migrate KeyVault Certificates, Keys and Secrets to Core V2

[JonathanCrd]

Packages impacted by this PR

• KeyVault Certificates
• KeyVault Keys
• KeyVault Secrets
• KeyVault Common (Not a published package)
• KeyVault Admin

Issues associated with this PR

• Move remainder of Key Vault packages to core-rest-pipeline #15896
• Move all packages that use core-http to use the newer Azure Core packages #15594

Describe the problem that is addressed by this PR

This PR finishing updating all of KeyVault to use CoreV2 and removes all their dependencies to the old core. For more information about this type of migrations refer to this wiki.

Since these 3 packages depends on common, they needed to be migrated at the same time. The following is a summary of the main changes:

Dependencies and generation changes:

• Updated dependencies in package jsons.
• Removed all instances and import from core-http and replaced them with their core-client, core-rest-pipeline and core-auth equivalent.
• Regenerated clients with latest version of autorest and v2 flag set to true.
• isNode and delay are now imported from core-utils preview, since Admin client already depends on it.
• Added core-http-compat as a dependency to use ExtendedCommonClientOptions in all packages instead of CommonClientOptions from core-client.

Authentication changes:

• Challenge Based Auth removed from Admin and moved to Common.
• Removed SigningPolic from all packages.
• Using bearerTokenAuthenticationPolicy to authenticate.
• Removed isTokenCredential from imports.
• Added an auth scope in constants.ts
• Challenge authentication was not properly getting the auth scope from the server response, this was a bug that got fixed in this PR. Now the scope is gathered from the parsedChallenge.resource.

Convenience layer changes:

• Inside listing or pagination methods, updated operation calls that passes a continuation token to use the new generated methods that has the Next keyword in them. For example: getDeletedSecret(continuationToken, options ) to getDeletedSecretNext(vaultURI, continuationToken, options).

Test changes:

• Updated the recordings.
• Added the used of an XHR Http Client in TestAuthentication.ts
• Did required fixes for test that stop passing after the migration
• Challenge auth test were removed from Secrets, Certificates and Keys and left on Admin, since all of the KeyVault packages run on CI.

Recorder updates:

• KeyVault Keys has been upgraded to use the new recorder.

All of the packages build correctly, and tests pass as well.

Related PRs

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

Checklists

• Added impacted package name to the issue description
• Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
• Added a changelog (if necessary)

[JonathanCrd]

I have updated the PR description with the latest changes

[deyaaeldeen]

why is it re-exported?

[JonathanCrd]

I'm not sure, the only place where this is being exported is in index.ts on line 197.

CommonClientOptions from Core-http was also being exported there, I just renamed it to the equivalent in CoreV2.

[deyaaeldeen]

This change is breaking then but I wonder what was the point of re-exporting the old interface in the first place. I am thinking of biting the bullet and removing the re-export even though that is also a breaking change. @xirzec what do you think?

[JonathanCrd]

For the record, PipelineOptions from core-http (which has been removed in this PR) was being exported too in the same way as ExtendedCommonClientOptions.
You can find it here:
https://github.com/JonathanCrd/azure-sdk-for-js/blob/main/sdk/keyvault/keyvault-certificates/review/keyvault-certificates.api.md?plain=1#L433

I'm also curious why this behavior, however, wouldn't it be a breaking change anyway since PipelineOptions is no longer being exported?

[timovv]

I would like to get this PR merged in the coming days, but want to resolve this first. I did a bit of digging to try and understand the impact of the change. PipelineOptions was initially exported in this commit from 3 years back and hasn't been touched since. At the time of that commit, the CertificateClient constructor took PipelineOptions as a parameter, so you can see why the export was required then. This was extracted out to a CertificateClientOptions by Daniel here, which is what we have now.

I don't see the point in exporting the new ExtendedCommonClientOptions as is here -- let's remove that at least. If we were to remove the export of PipelineOptions, would we still be able to release this as a minor? I can think of some hackery that might be less breaking (re-exporting ExtendedCommonClientOptions as PipelineOptions...) but that doesn't sit very well with me since we're effectively giving the type an incorrect name.

Thoughts, @deyaaeldeen, @xirzec, @joheredi (and anybody else)?

[timovv]

To wrap this up: After an offline discussion, we're in agreement it would be ok to remove the export of PipelineOptions and still release this as a minor.

Cc @deyaaeldeen @joheredi @bterlson

[timovv]

🚀