search

dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
834 stars 276 forks source link

Feature | Split Azure dependent functionality in a separate NuGet Package #1108

Open SimonCropp opened 3 years ago

SimonCropp commented 3 years ago

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

SimonCropp commented 3 years ago

found my answer https://github.com/dotnet/SqlClient/pull/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 commented 3 years ago

@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 commented 3 years ago

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

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 commented 3 years ago

i had the same expectation as @virzak

virzak commented 3 years ago

@SimonCropp reopen then? :)

SimonCropp commented 3 years ago

@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 commented 3 years ago

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

mtaylorfsmb commented 2 years ago

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 commented 2 years ago

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

MarkPflug commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

The following also seem to be meaningful only for 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 commented 2 years ago

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:

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

AraHaan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

AraHaan commented 2 years ago

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 commented 2 years ago

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

AraHaan commented 2 years ago

I doubt trimming is supported at the moment.

Dean-NC commented 2 years ago

I strongly agree that Azure and Identity shouldn't be in a base SqlClient library. It's frustrating for me to see the long list of assemblies when I'm only using basic data access with a local SQL Server.

AraHaan commented 2 years ago

Agreed, if I am using a local sql server or an sql server instance on my local network, chances are I do not need to use anything from the Azure SDK (as it is wasted disk space then). Especially if what I am using are:

ArminShoeibi commented 2 years ago

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.

Hi there, Is it possible for you to Make a PR that we can look at it and help?

RallyTuning commented 1 year ago

Would be nice to have one package (core) with just basic usage of common database methods and one package (full) with all those dlls, so people like me can choose the first one if they aren't going to use the others 22 of 23 dlls.

ErikEJ commented 1 year ago

@RallyTuning This will "only" get rid of 5 DLLs.

RallyTuning commented 1 year ago

@RallyTuning This will "only" get rid of 5 DLLs.

How about System.Data? Have less dlls? Or, there is a lighter alternative?

ErikEJ commented 1 year ago

@RallyTuning Not sure what you mean by "System.Data"

RallyTuning commented 1 year ago

@RallyTuning Not sure what you mean by "System.Data"

System.Data.SqlClient...

ErikEJ commented 1 year ago

System.Data.SqlClient is no longer maintained

virzak commented 9 months ago

With the latest preview 5.2.0-preview3.23201.1 of this library there are 4 new dependencies, which blow up the size even further.

Name Size ( bytes)
Microsoft.Web.WebView2.Core.dll 222136
Microsoft.Web.WebView2.WinForms.dll 32680
Microsoft.Web.WebView2.Wpf.dll 36792
WebView2Loader.dll 137144

The chain is: Microsoft.Data.SqlClient/5.2.0-preview3.23201.1 Azure.Identity/1.8.0 Microsoft.Identity.Client/4.53.0 Microsoft.Web.WebView2/1.0.864.35 contains the 4 deps above

SimonCropp commented 9 months ago

for a total of 6.5MB F__NxMabEAAHvJA

SimonCropp commented 9 months ago

Side note. if the team uses votes as a way of determining priority: this is the most voted open issue. and the second most voted of all issues.

ErikEJ commented 9 months ago

@SimonCropp Would creating a new Microsoft.Data.SqlClient.Core package that removes Azure.Identity be a viable path?

KalleOlaviNiemitalo commented 9 months ago

Perhaps one could add a trimming feature flag that lets the IL Linker delete everything Azure-related. But this would not help with application models that don't support trimming, like Windows Forms, or on .NET Framework.

SimonCropp commented 9 months ago

@ErikEJ

Would creating a new Microsoft.Data.SqlClient.Core package that removes Azure.Identity be a viable path?

not really since it would not work for transitive references

ErikEJ commented 9 months ago

@SimonCropp I am not sure I understand? I am proposing a "Core" package that does not depend on Azure.Identity, so can only be used in non-Azure AD scenarios. So it would not depend on the Azure.Identity and related packages.

ErikEJ commented 9 months ago

So basically"Split non-Azure dependent functionality in a separate NuGet Package" 😄

RallyTuning commented 9 months ago

@SimonCropp Would creating a new Microsoft.Data.SqlClient.Core package that removes Azure.Identity be a viable path?

The best solution, even if the most elaborate to create, would be to divide the various packages, starting from the complete one which includes all the references; and other packages of 1 or maximum 2 dlls for WinForms, 1 or 2 dlls for WPF, the same for WinUI, etc... So that those who use a single technology do not have to download and use dozens of dlls that they will never use. Making the project heavier both in terms of MB and execution times.

KalleOlaviNiemitalo commented 9 months ago

There's already the SqlConnection.RegisterColumnEncryptionKeyStoreProviders method for registering SqlColumnEncryptionAzureKeyVaultProvider from the separate Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider package. I imagine a similar method could be added for isolating the Azure.Identity dependency, instead of using Microsoft.Extensions.DependencyInjection. This would be a breaking change though, for software that currently uses Azure identity to log in to SQL Server and does not call the registration method that doesn't even exist yet. I think the breaking change would be tolerable.

mtaylorfsmb commented 9 months ago

It's not just Azure that is problematic. Besides Azure.Identity it also references System.Configuration.ConfigurationManager. Which in turn relies on System.Security.Permissions which in turn relies on System.Drawing.Common. Hence my task or API app which doesn't really have a UI still pulls in UI/Windows related features.

What makes this even worse is that we have a policy that we cannot use pre-release packages and we must remove dependencies on vulnerable packages. The current package relies on Azure.Identity 1.3.0 (at least that is what VS tells me) which is marked as vulnerable, The only workaround is to add an explicit dependency to your project (so you can change the version to one that isn't vulnerable). But now it is not clear that you don't really need this dependency in your code unless you also document it somewhere.

One workaround I could see is to have a "core" package (like ASP.NET) that contains the core types used by the package and then move the implementation details elsewhere. But now you're juggling even more packages and configuring everything to work for the simple case is now more complicated. That perhaps could be solved by having a meta package that pulls in Core and the most commonly needed extra packages but then you run into the issue of other packages taking dependencies on the metadata package instead of Core (see also ASP.NET).

We've honestly considered moving back to System.Data.SqlClient because this package has been nothing but problems since it was introduced (not copying native binaries initially, then not unloading binaries so apps can be deployed, and now a series of dependencies on packages that don't make sense for talking to SQL Server). All we need to do is CRUD to a relational database, it doesn't need to be this difficult.

KalleOlaviNiemitalo commented 9 months ago

Besides Azure.Identity it also references System.Configuration.ConfigurationManager. Which in turn relies on System.Security.Permissions which in turn relies on System.Drawing.Common.

That was fixed in .NET 8.

mtaylorfsmb commented 9 months ago

@KalleOlaviNiemitalo Correct but because it is a transitive dependency the only way I would be able to get that fix is to add an explicit dependency on the transitive dependency. This puts me in the same boat as mentioned earlier about adding explicit dependencies to packages I don't actually use.

KalleOlaviNiemitalo commented 9 months ago

CentralPackageTransitivePinningEnabled seems like a good way to raise the version of System.Configuration.ConfigurationManager without adding a direct dependency on it, if you can require a sufficiently new version of NuGet.

mtaylorfsmb commented 9 months ago

@KalleOlaviNiemitalo It seems like an option. I'll take a look. Thank you.

ErikEJ commented 9 months ago

This is the outcome of removing the Azure dependencies and building:

image

(POC PR coming up)

ErikEJ commented 9 months ago

POC PR - https://github.com/dotnet/SqlClient/pull/2247

MarkPflug commented 9 months ago

@ErikEJ Any idea why the System.Drawing.Common library is still getting picked up? Seems really strange that would still be needed.

Edit: nevermind. I see someone previously commented on why:

System.Security.Permissions which in turn relies on System.Drawing.Common

ErikEJ commented 9 months ago

@MarkPflug a .net 8 build will not contain it, but I encountered some issues that the team is currently fixing when attempting to target net 8