Compatibility with new Azure SDK

[chrmarti]

When I import the azure-arm-containerservice package, the azure-account credentials work fine with it, but I get a deprecation notice:

#########################################################################################
# Active development of this SDK has been moved to @azure/arm-containerservice package. #
#       This package is in maintenance mode and will be deprecated in July 2019.        #
#         Visit https://aka.ms/azure-sdk-for-js-migration for more information.         #
#########################################################################################

If I import @azure/arm-containerservice, it appears to use a different credential type, and doesn’t play nicely with the azure-account typings.

[ianphil]

@chrmarti @chrisdias I am also seeing this with Key Vault:
let client = new SecretClient('https://${this.state.keyVault}.vault.azure.net/', this.state.currentSubscription.session.credentials);

Has error:

Argument of type 'ServiceClientCredentials' is not assignable to parameter of type 'TokenCredential'.
  Property 'getToken' is missing in type 'ServiceClientCredentials' but required in type 'TokenCredential'.ts(2345)
core-auth.d.ts(75, 5): 'getToken' is declared here.

cc: @jwendl

[chrmarti]

A possible workaround might be to extract the tokens from the old credentials object and use these to create a credentials object compatible with the new SDK.

That might also be an approach the extension could take to support the new SDK without breaking existing extensions.

[bwateratmsft]

@chrmarti Aside from ServiceClientCredentials are you aware of any other incompatibilities?

[chrmarti]

@bwateratmsft That's the main incompatibility. We also surface SubscriptionModels.Subscription from the old SDK, not sure how much that has changed with the new SDK, but that is potentially easy to absorb.

[bwateratmsft]

Looks like also AzureEnvironment, which does not exist in the new SDK (or at least, I didn't find it).

Maybe vscode-azure-account needs to implement both new and old SDKs for the time being under separate methods/etc.; until we can move all of the consumers of the old to the new.

Side note: I also noticed that the way @ianphil is using @azure/keyvault-secrets + vscode-azure-account is technically incorrect. @azure/keyvault-secrets uses ServiceClientCredentials from @azure/core-http (in particular, TokenCredential from the @azure/core-auth [note the lack of S at the end of TokenCredential, this is not the same as TokenCredentials from ms-rest, and the interfaces are different]), whereas this extension is delivering ServiceClientCredentials from ms-rest (in particular, DeviceTokenCredentials from ms-rest-azure). The interfaces are identical or at least close, but that shouldn't be considered more than coincidence. KeyVault expects a getToken() method which does not exist on either ServiceClientCredentials, but does exist on TokenCredential--and coincidentally, also exists on DeviceTokenCredentials, so things work (maybe).

I'm not really sure why the @azure/core-auth / @azure/core-http packages need to provide nearly identical interfaces, but I can see that leading to a host of problems--especially because it's only nearly identical.

[RMacfarlane]

I think the least-error prone thing to do is expose the new credentials in the extension API, leaving the old credentials there as well, like:

export interface AzureSession {
	readonly environment: AzureEnvironment;
	readonly userId: string;
	readonly tenantId: string;
	readonly credentials: ServiceClientCredentials;
	readonly credentialsV2: DeviceTokenCredentials;
}

This way breaking changes are avoided but it's still possible for anyone to upgrade. I'm not really a fan of this naming, as eventually I think we would want to remove the old credentials. Perhaps "azureJSCredentials"? @chrmarti any thoughts on this?

For SubscriptionModels.Subscription, the new type is identical except that it has a string enum for state instead of just string. AzureEnvironment is now exposed as Environment in @azure/ms-rest-azure-env, it is also almost identical, except that many properties are now marked as optional. Upgrading the extension to use the new packages and exposing the new types seems reasonable to me, since I believe these objects could still be used with the old sdks, and would require very minimal changes to fix ts compilation.

[chrmarti]

Maybe just credentials2 to avoid the V for version which isn't really correct since the old SDK was at version 2 and the new SDK starts over with version numbering. Removing the old property would imply that we make sure all extensions have moved to the new SDK which might take a long time, so maybe removal of the old property is not too much of a concern assuming maintenance cost is low.

You probably thought of this: To move to broader testing, you could publish the update requiring the Insiders version of VS Code (maybe do this early in a milestone).

[RMacfarlane]

Yeah, publishing it to insiders first sounds good. I will update the required VS Code version to the new insiders after out next release, so it will be available in 1.48.