Feature | Split Azure dependent functionality in a separate NuGet Package

[SimonCropp]

why does Microsoft.Data.SqlClient v3 reference Azure.Identity?

[SimonCropp]

found my answer #1010. so will close this. it still seems very weird to me that a sql client should have a dependency on a cloud infrastructure library. For hypothetical comparison, would it be ok to also take dependencies on google and aws libraries.

[David-Engel]

@SimonCropp Microsoft.Data.SqlClient also provides connectivity to Azure cloud services (Azure SQL DB, Azure Synapse Analytics, etc) that are very similar to SQL Server on-premise. The Azure.Identity library provides authentication functionality for those Azure services.

[virzak]

I was also wondering if there is a way to separate functionality into multiple DLLs, i.e.

• Microsoft.Data.SqlClient
• Microsoft.Data.SqlClient.Azure

With .Net 5, a project which uses this library and doesn't use Azure gets at least 3 new unused DLLs in the bin directory:
Azure.Core.dll
Azure.Identity.dll
Microsoft.Bcl.AsyncInterfaces.dll

[SimonCropp]

i had the same expectation as @virzak

[virzak]

@SimonCropp reopen then? :)

[SimonCropp]

@virzak i think i have raised my concerns. if the sql client team think they should be address, then they can re-open the issue

[cheenamalhotra]

We are considering this internally, timeline not certain yet. But we can reopen for sure!

[mtaylorfsmb]

Agree with others. This library is supposed to be replacement for the standard library already in the framework. Azure support is fine but shouldn't be auto-included in a "core" package. This would be equivalent to having the core framework depend upon Azure as well. If my app is using AWS or GCP then having an Azure dependency looks wrong.

[aersam]

Agree, too. Mostly because of the dependencies Azure.Identity brings in. There are really many packages needed for Microsoft.Data.SqlClient

[MarkPflug]

FWIW, I also just wasted a bunch of time tracking down why these Azure assemblies were being included in my build output. I'm not using anything Azure related and so it made me worried some malicious package had snuck into my dependency graph. Took me a while to figure out that MDS was the culprit. It was annoying enough to me that I decided to switch back to SDS, which is probably not something y'all want to encourage.

[wastaz]

Agree on this as well. Azure support should be opt-in and a separate thing. Wasted a decent amount of time today again in trying to figure out why my solution that has nothing to do with azure was referencing azure packages.

[guillaume86]

Just noticed this because of some versions conflicts on packages that seemed completely unrelated to SqlClient (OpenIdConnect).
Azure dependencies should definitely be moved out in another package.

[m-gallesio]

Agreeing on this.

Comparing the dependencies of Microsoft.Data.SqlClient with those of the classic System.Data.SqlClient, the new library adds some more other than Azure which should be not needed in at least some scenarios:

• Microsoft.Identity.Client
• Microsoft.IdentityModel.JsonWebTokens
• Microsoft.IdentityModel.Protocols.OpenIdConnect

The following also seem to be meaningful only for Windows:

• Microsoft.Win32.Registry
• System.Security.Principal.Windows

I do not know how heavy those libraries are and whether their presence might stem from simple refactorings, but I guess their presence in a core package should be evaluated.

[m-gallesio]

As someone who does not know the internals of this project at all, I tried downloading the code and simply looking for those package names via CTRL+F in *.cs files. Here are some results:

• Azure.Identity, Azure.Core:
  • add-ons/AzureKeyValueProvider (seems normal)
  • ActiveDirectoryAuthenticationProvider
  • test code
• Microsoft.Identity.Client
  • SqlInternalConnectionTds
  • ref directories (I do not know what these are for)
  • ActiveDirectoryAuthenticationProvider
  • test code
• Microsoft.IdentityModel.JsonWebTokens, Microsoft.IdentityModel.Protocols.OpenIdConnect
  • AzureAttestationEnclaveProvider
• Microsoft.Win32.Registry
  • no apparent usage in non-test code
  • tests/AlwaysEncrypted
• System.Security.Principal.Windows
  • no apparent usage at all

Overall the "offending" packages seem quite well-encapsulated.

[AraHaan]

I think I might see if I can make a local clone of sqlclient and see what I can do to open an pr to move all azure dependencies into their own package where one must append .Azure to the end of the SqlClient package name to get them (where the Azure version references the normal sqlclient package so you can use the normal one as well as a convenience.

This is because I also face this issue where I do not use Azure stuff at all (however I do use efcore).

As for win32.Registry that package is supposed to come from the ref pack that is included by default for the default runtime that ships with the .NET SDK. Same for System.Security.Principal.Windows. I have an pull request that removes the packages for .NET Core/5 and 6+ applications however it is blocked for now until they can upgrade the CI to install the .NET 6 SDK everywhere so the build of that pull request will all pass.

If I do split them it would help eliminate even more dependencies that cause me pain which would always be good for all of us.

Also note: when I do the split I will also upgrade it to the latest version of the Azure SDK because they recommend always keeping it up with the latest stable.

As for .NET Framework, I am not sure if I should make any changes to it since I do not really care for .NET Framework at this point.

[AraHaan]

Bad news, the Azure Enclave Provider is used in EnclaveDelegate.Crypto.cs which I do not know for sure if that file is only for Azure or not. This has made it a lot harder to separate it.

Luckily it looks like TDS is some sort of remote specific stuff? Perhaps Azure specific?

[AraHaan]

It looks like Azure cannot be split because it's to far engraved into the dependencies of basically everything in SqlClient needlessly.

At least currently it just does not seem possible at the moment.

[m-gallesio]

the Azure Enclave Provider is used in EnclaveDelegate.Crypto.cs

I tried taking a quick look at this and AzureAttestationEnclaveProvider.cs despite its name does not seem to explicitly reference Azure packages.
As I noted before it does require several Microsoft.IdentityModel packages, though.

[AraHaan]

Yeah, I tried to split that into Microsoft.Data.SqlClient.Azure so the IdentityModel packages would move to that project (and package)

[AraHaan]

Alternatively I could rig it up to where it could look for enclaves providers registered with DependencyInjection however that would add another dependency and obtain the enclaves providers registered there everywhere they are needed (however I already use Dependency Injection significantly so it does not bother me). However it would bother someone who is not using Dependency Injection but is using SqlClient.

[ian-cowley]

Would the unused elements of this get stripped at compile time, with trimming options?

[roji]

And then provide an actionable error message with a link to good docs when attempting to use Azure auth with the "base" package...

Yes, exactly! As long as you provide crystal clear fail-fast behavior with an informative exception pointing to a good page explaining what to do to (switch to M.D.S.Azure, add this in your code), I really think it's a very reasonable situation... When this is possible, it's the "best kind" of breaking change (compared to breaking changes where the behavior silently changes or whatever). Users will still complain (some will complain no matter what), but the majority will simply do the change quickly and move on.

[roji]

BTW if you do go with the plugin approach, I'm not sure there's value in actually publishing an M.D.S.Azure package (as in #2823) before it's fully implemented; that wouldn't be very useful, given that users will have to add the code gesture anyway when the work is fully done. So rather than telling users to switch to M.D.S.Azure today, and then add the code gesture tomorrow, you might as well just publish M.D.S.Azure tomorrow and tell them to do both at the same time?

[saurabh500]

@roji The plugin approach makes most sense to me.

Also, I would like to stay away from the re-branding of any SqlClient to SqlClient for Azure.
There are features which were offered in Azure SQL MI/DB which have made their way into Sql On prem, e.g. Entra ID auth in Sql 2022.

This means that there is only one client with extensions/plugins to help light up the added scenarios in Az SQL DB/On prem, but apps / ORMs like EF only care about one package i.e. MDS, which remains the gateway into SQL Server.

Of course, all this while solving the core problem at hand, i.e. Azure related dependencies should be optional for those who don't need them.

This effort should look at

1. Extracting Entra ID auth out of MDS and creating a new Nuget Package.
2. Due to Enclaves, if there are dependencies, those should be extracted out.
3. Anything else that I may have missed.

I propose that MDS vNext, should remove the auth providers for atleast Entra ID auth (AAD auth), out of the code.

I had a chat with @David-Engel and he mentioned that there are dependencies on MSALException for retry of token fetching, and there are few other capabilities for which the driver may have taken a dependency on the internals of the AUth providers.
These will need to be encapsulated in the base class. SqlAuthenticationProvider and if needed the SqlAuthenticationProviderManager as well.

Hence we will have MDS and M.D.S.Azure.Authentication.

MDS.Az.Auth depends on MDS (for the provider base classes) and Azure libraries.

vNext Experience:

1. Customers take MDS as a dependency.
2. Those who need Entra ID auth, get a runtime exception with actionable error message which has been hinted in the discussion.
3. The app authors will then add a dep to MDS.Az.Auth.

For those who dont care about azure related capabilities: Just upgrade to MDS vNext

At this point, we still need to make MDS aware about the MDS.Az.Auth providers

1. Expose a static API to register the Az Auth provider. (I am not sure if it exists already, likely does)
2. There will be friction to make the code change in app owners. In that case MDS could look at "MDS.Az.Auth" assembly by name, and load the providers if the assembly is found. This would make the experience nicer, where the app owners only add a dependency to MDS.Az.Auth, and let it bring in the Az dependencies. This basically means that MDS gives first preferential treatment to MDS.Az.Auth by looking it up, in the absence of any providers.

And yes, we need to look into the behavior with SqlDataSource as well. This is a classic case of leveraging the data source builder, but I first want to focus on the declarative way of making this work as well a NetFx users.

[saurabh500]

Though I have tagged @roji, I would like to invite comments from @edwardneal and @ErikEJ who have gone into the depths of this implementation, about the feasibility of the proposal.

[edwardneal]

Thanks for this @saurabh500. I hadn't actually spotted the extra dependency on MSALException, but that's correct. For everyone's reference, this is largely in SqlInternalConnectionTds.GetFedAuthToken, which hasn't yet been merged between .NET Core and .NET Framework but is almost identical.

In terms of the vNext (and the v6) experience: looking solely at logistics, I'm a little concerned that we could end up telling developers to migrate from SDS to MDS, then to add another reference to MDS.Az.Auth six months later. My main reason for asking for the front-loading of the extra package was to be able to simplify that and immediately tell migrators that they should use MDS if they use on-premise servers and MDS.Az if they use Azure SQL (or other Azure components.)

To lift that same motive to a plugin model: would there be enough time to provide a v6 MDS.Az.Auth package consisting of nothing but an empty API surface? This'd provide migrators from SDS with a single choice to make, and would give existing users of MDS a full major version to bring their dependencies into line.

Splitting into two packages in time for vNext definitely sounds good to me. I think there are a few pieces of work I can help with, pending any comments.

[ErikEJ]

telling developers to migrate from SDS to MDS, then to add another reference to MDS.Az.Auth six months later

I think most developers at this point already use MDS, actually

[ErikEJ]

Splitting into two packages in time for vNext definitely sounds good to me. I think there are a few pieces of work I can help with, pending any comments.

I don't see why all of this could not be done for vNext given there is resources to implement it.

[saurabh500]

It seems feasible to me.
I need to chat with @David-Engel about some of the internal logistics around releasing new packages. I don't understand that part too well.

Also I would like to eventually convert this issue to a discussion and open a new issue with the exact proposal in the description and link that to PR(s)

We can divide and conquer this. I will reach out for help with some of the items.

[MichaelKetting]

telling developers to migrate from SDS to MDS, then to add another reference to MDS.Az.Auth six months later

I think most developers at this point already use MDS, actually

@ErikEJ I can't object to that statement but would like to qualify that this likely means developers who do in-house develpment instead of ISVs. For us, MDS is currently a no-go due to the dependencies and we cannot migrate our products from SDS to MDS until the dependency problem has been solved.

[ernstscheithauer]

@MichaelKetting +1 from my side. This dependency in combination with the necessary security updates is a pain, preventing us from migrating to MDS as well. Having software installed on-prem (not having a single SAAS instance) requires stability that is lacking there.

[cheenamalhotra]

Feature branch for capturing progress: feat/azure-split

[ErikEJ]

Awesome!! Great to see you back on the team, @cheenamalhotra

[m-gallesio]

@cheenamalhotra .NET 9 is out and the branch has not been updated since the announcement, would it be possible to get some status update?

[cheenamalhotra]

We have a solution and plan to address this, but it requires major refactoring in project architecture and the milestone I targeted before wasn't going to be enough time, so its paused until the next stable release is shipped.

Will resume on it post the 6.0 release planned for this month.

[SimonCropp]

@cheenamalhotra any chance of getting a v7 out quicker than the usual major cadence. i think people would prefer not to wait a year for this because it missed the v6 cutoff.

side note. given this is the highest voted issue (almost doubling the next highest), IMO the v6 release should be held off until this can be included. it not it basically makes a mockery of any issue prioritization practice the project has

[cheenamalhotra]

v6.0.0 will go as per plan for other features to be released.

And yes v7 will not take as long as a year, but as soon as we're ready with this change, we will prioritize v7 to be released.

[SimonCropp]

@cheenamalhotra thanks