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.

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

[Identity] Create a separate package for optional native components of @azure/identity #14346

Closed
2 tasks
sadasant opened this issue Mar 17, 2021 · 22 comments
Closed
2 tasks
Assignees
Labels
Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@sadasant
Copy link
Contributor

sadasant commented Mar 17, 2021

In Identity, we would like to avoid adding any packages with native binaries as dependencies (not even peer or optional dependencies).

  • VisualStudioCodeCredential depends on keytar, which uses native binaries to commmunicate with the platform credential manager.
  • persistence through msal-node-extensions requires a native DPAPI component that is installed with the package. As Mike reported here: Update native dependency install to install msal-node-extensions #14343 (comment) some customers refuse or are unable to install native dependencies.

We would like to provide these facilities through a separate package that can inject or provide these native module dependencies outside of the mainline identity package. This issue tracks that work.

@sadasant sadasant added Client This issue points to a problem in the data-plane of the library. Azure.Identity blocking-release Blocks release labels Mar 17, 2021
@sadasant sadasant added this to the [2021] April milestone Mar 17, 2021
@sadasant sadasant self-assigned this Mar 17, 2021
@mikeharder
Copy link
Member

mikeharder commented Mar 17, 2021

Making the dependency optional doesn't help much, as explained in #13950. Instead, I think the identity package should be split into one or more packages, one with native dependencies and one without.

@sadasant
Copy link
Contributor Author

@mikeharder that sounds about right. I've been thinking of two packages too!

@witemple-msft
Copy link
Member

I've updated the description and title of this issue to reflect the current proposed solution.

@octogonz
Copy link

octogonz commented Apr 22, 2021

@witemple-msft How does Make msal-node-extensions an optional dependency solve the originally stated problem of we would like to avoid adding any packages with native binaries as dependencies (not even peer or optional dependencies)?

FWIW Rush really cannot wait much longer. Today someone reported that Docker fails to install the Rush tool because keytar's postbuild-install fails to write to the /root/.npm/ folder when it's trying to download the native binaries.

If a solution isn't forthcoming, then it seems that Rush should consider instead eliminating Azure SDK. We already recently eliminated Amazon's SDK due to similar issues with problematic NPM dependencies.

@sadasant
Copy link
Contributor Author

@octogonz The Identity package will release a v2-beta in a couple of weeks without Keytar. The solution @willmtemple is working on is on having an entirely separate package that will contain the binaries. The main @azure/identity v2 won't have any binary dependency. This v2 will go GA in a couple of months though. Would you be able to work with the v2-beta we'll release in a couple of weeks? We'll make sure to work with you on the transition to v2 (though it should be seamless for Rush, but in case anything surprising comes up).

@iclanton
Copy link

The only thing Rush actually uses from @azure/identity is the device code login flow. That seems like it could be a pretty self-contained portion of the package. For us, it would be ideal if that functionality was in its own package. Would something like that be possible? Something like @azure/device-code-authentication.

@witemple-msft witemple-msft changed the title [Identity] Make msal-node-extensions an optional dependency [Identity] Create a separate package for optional native components of @azure/identity Apr 23, 2021
@octogonz
Copy link

This v2 will go GA in a couple of months though. Would you be able to work with the v2-beta we'll release in a couple of weeks?

@sadasant Yes -- given that the Azure build cache feature is still considered "experimental", Rush seems like a great candidate for testing a v2-beta.

@mikeharder
Copy link
Member

@octogonz: You might be able to use a v2-alpha even earlier.

@sadasant: How soon do you think we might have a v2-alpha without any native deps?

@witemple-msft
Copy link
Member

@octogonz My title change apparently wasn't saved. It's updated now. As far as timeline, we are pushing towards a GA solution within the next month and a half or so, beta availability for a stripped down package within a couple of weeks. @azure/identity 2.0.0 will not have any native package dependencies, peer, optional, or otherwise, and native extensions to identity will be moved into their own package.

@iclanton The principle of @azure/identity is to be a fixpoint within the Azure SDK for JavaScript for AAD authentication. In the backend, we are using MSAL. If you aren't using Identity to work with the Azure SDK data-plane libraries, you could consider using @azure/msal-node and its support for device code grants directly, but it's a more involved API than Identity is. We are reluctant to separate the components of identity at all, but the native dependencies present enough of a problem to justify removing the VS Code Credential.

Local dev builds of 2.0.0-beta.3 are about 25MB on disk, as we are able to throw off a bit by not installing keytar and prebuild-install. It's up to you all if that's acceptable. @mikeharder I hope to have a basic version of this merged tomorrow or Monday so we should have a dev artifact by Tuesday.

CC @schaabs, @bterlson

@ramya-rao-a
Copy link
Contributor

@octogonz, @iclanton,

While we wait for v2, if it is indeed the case that rush currently uses only the device code login flow and you do not care about whether the underlying implementation for this uses the newer MSAL or not, then another option for you can be to update the identity dependency to use ~1.0.0 instead of say ^1.0.0. The keytar dependency came into play when we added the VisualStudioCodeCredential in 1.1.0. So, versions 1.0.* should be safe for you to use device code login flow from @azure/identity.

Once we have resolved this issue for native components, you can get right back to using the latest identity package

@octogonz
Copy link

@ramya-rao-a I confirmed that your suggestion does eliminate the keytar dependency (PR microsoft/rushstack#2647)

@octogonz
Copy link

@ramya-rao-a Downgrading to ~1.0.0 did not work for us in PR microsoft/rushstack#2647:

Error: src/logic/buildCache/AzureStorageBuildCacheProvider.ts:15:10 - (TS2305) Module '"../../../node_modules/@azure/identity/types/identity"' has no exported member 'AzureAuthorityHosts'.

It seems that maybe we do need to wait for 2.0.0-beta.x.

CC @elliot-nelson

@octogonz
Copy link

Following up, we are still having trouble getting microsoft/rushstack#2647 to build.

How close are we to having a 2.0.0-beta.x beta release?

@ramya-rao-a
Copy link
Contributor

Update: The rush team has been unblocked for now. Please see microsoft/rushstack#2647 for details

@01
Copy link

01 commented May 4, 2021

Any guidance on work around until this is released?

@ramya-rao-a
Copy link
Contributor

@01 The workaround depends on which features of @azure/identity you depend on. Can you share which credential are you using? If using DefaultAzureCredential, which of the multiple credentials it uses internally is used by your code?

@01
Copy link

01 commented May 5, 2021

@01 The workaround depends on which features of @azure/identity you depend on. Can you share which credential are you using? If using DefaultAzureCredential, which of the multiple credentials it uses internally is used by your code?

Simply using DefaultAzureCredential with a system assigned managed identity for an Azure App Service

@ramya-rao-a
Copy link
Contributor

@01 In that case, please try the solution suggested at #14346 (comment).

@sadasant sadasant modified the milestones: [2021] May, [2021] June May 11, 2021
@octogonz
Copy link

octogonz commented May 15, 2021

FYI since we downgraded to "@azure/identity": "~1.0.0", we are now seeing this warning during installation:

npm WARN deprecated @opentelemetry/types@0.2.0: Package renamed to @opentelemetry/api, see https://github.com/open-telemetry/opentelemetry-js

It's only a minor annoyance, but we're looking forward to whenever @azure/identity 2.0.0 is ready so we can eliminate that warning.

@witemple-msft
Copy link
Member

@octogonz @iclanton -- Our latest beta release @azure/identity@2.0.0-beta.3 (tag: next) on NPM has removed the optional dependency on keytar as well. Our plan is to move forward with a base @azure/identity 2.0.0 package that is free of machine code dependencies, and the functionality that the machine code dependencies provided in Identity 1.x will be restored through a separate extension package.

We don't have the design of the extension package nailed down yet, but our mainline @azure/identity package will continue in this fashion without native deps.

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this issue Jun 9, 2021
Added DW Gen3 SqlPool Pause and Resume swagger (Azure#14346)

* Added DW Gen3 SqlPool Pause and Resume swagger

* Validation error fix

* Validation error fix

* Validation error fix

* Validation error fix

Co-authored-by: Jignesh Vavadiya <jivavadi@microsoft.com>
@sadasant sadasant modified the milestones: [2021] June, [2021] July Jun 15, 2021
@sadasant sadasant removed their assignment Jun 16, 2021
@witemple-msft
Copy link
Member

This has been implemented via. an extension API, and alpha releases of the new extension packages are available on NPM. They will be available in beta as of @azure/identity version 2.0.0-beta.4:

@ramya-rao-a
Copy link
Contributor

Beta releases for the above two new packages are now available

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity blocking-release Blocks release Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

7 participants