POC: Remove Azure Identity from Microsoft.Data.SqlClient #2247
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2247 +/- ##
==========================================
- Coverage 72.66% 72.44% -0.22%
==========================================
Files 310 309 -1
Lines 61877 61963 +86
==========================================
- Hits 44960 44887 -73
- Misses 16917 17076 +159
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -206,6 +200,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{4F3CD363-B1E | |||
| EndProject | |||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.SqlServer.Server", "Microsoft.SqlServer.Server\Microsoft.SqlServer.Server.csproj", "{A314812A-7820-4565-A2A8-ABBE391C11E4}" | |||
| EndProject | |||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Data.SqlClient.Core", "Microsoft.Data.SqlClient\netcore\src\Microsoft.Data.SqlClient.Core.csproj", "{638A00EA-9648-45F1-B2FF-94E9A111AD73}" | |||
There was a problem hiding this comment.
I think a better name for the "Core" package is Microsoft.Data.SqlClient and instead document that Azure people must now reference Microsoft.Data.SqlClient.Azure for the Azure bits which would reference the "Core" package for them.
There was a problem hiding this comment.
Agreed that additional Azure functionality would be (far) better integrated via an additional package with Azure in its name. The problem with that, of course, is that it would be a breaking change for existing users; but it may be acceptable given that the mitigation is extremely trivial.
There was a problem hiding this comment.
I also agree, but that would be a huge breaking change. I also just did this to explore where the Azure dependencies are and maybe enable adding an additional package over time
There was a problem hiding this comment.
Sure, though I'll just say it doesn't seem like a more significant breaking change than e.g. when encryption was set to true by default. Having SqlClient take a dependency on Azure packages was a problematic decision from the very beginning and is the more highly-voted issue on this repro.
|
doesnt this bifurcate the ecosystem? eg if i want to write a lib that extends/uses sql, i now need two packages, one that targets each sql package |
Not at all, the Azure version of the package could depend on the base version, and the extension libs could use the base version like how it does now, just with user versions not including the azure libs. This way it all acts similar to how creating extendable/extension libs using |
| @@ -36,6 +36,7 @@ private static void SetDefaultAuthProviders(SqlAuthenticationProviderManager ins | |||
| { | |||
| if (instance != null) | |||
| { | |||
| #if !CORE | |||
There was a problem hiding this comment.
After looking further, can't this file be excluded from "Core" builds and only include this file on the Azure version of the library, then it could reference the "Core" version directly without breaking any functionality at all?
There was a problem hiding this comment.
Also, this is not a PR, just a demo POC. Needs design!
There was a problem hiding this comment.
Yeah, I know, just offering up some ideas that could help contribute to a design.
| @@ -2312,6 +2312,7 @@ internal bool TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataRead | |||
| } | |||
| break; | |||
| } | |||
| #if !CORE | |||
There was a problem hiding this comment.
I feel like a generic type in the "Core" assembly that the Azure assembly could initialize values on (default null without the application "initializing" them to different values from the Azure assembly version) would be better in these internal types (TdsParser, SqlInternalConnectionTds, ADP (AdapterUtil.cs).
The new generic type also being internal as well so only the azure assembly could use it as well.
For the case of TdaParer, it can store the member information needed to Invoke the code for that case (with some null checking first to default values of said internal type to null in cases where the azure assembly is not used directly by the application. CreateSqlException can also fall under this as well, same for the other file where an enum is used to call into Active Directory code (EnclaveDelegate in EnclaveDelegate.Crypto.cs.
but "creating an Azure version of the package " is not what is being proposed here. this is slicing off a new Microsoft.Data.SqlClient.Core package. so it means, to avoid the azure bits, the stack of libraries that currently use/extend Microsoft.Data.SqlClient would need to move to Microsoft.Data.SqlClient.Core. for example Microsoft.EntityFrameworkCore.SqlServer. and that would be considered a breaking change for each of those libs, so would likely have a very long tail until top level apps could remove Microsoft.Data.SqlClient while breaking out a Microsoft.Data.SqlClient.Azure package would be a breaking change for this project, it does seem like that would have less combine fallout for consumers unless u misunderstand what is being proposed here |
Except it would be breaking for anyone using Azure auth today... |
yeah. no idea which group is more common |
|
OK - once 5.2 has been released, I will try this approach (initially .NET only)
This will require anyone using Azure auth methods to use an explict reference to:
Attempts to use Azure auth with Microsoft.Data.SqlClient should result in an actionable error message with links to a suitable docs page Thoughts? @SimonCropp @roji @AraHaan @David-Engel |
Sounds good, yeah. |
We should err on the side of compatibility. The reverse would be far less disruptive. People already expect M.D.SqlClient to support Azure connectivity. Those few who object to the current long dependency tail can move to a new M.D.SqlClient.Core package, which just has the base functionality. Base functionality in my mind is SQL auth, integrated Windows auth, and token auth (AccessToken, AccessTokenCallback). |
to achieve that all libs using Microsoft.Data.SqlClient would need to make a breaking change. |
Yes. i think that is the best compromise of all the options discussed |
@SimonCropp |
Basically libs the once depended on the single package would be broken for a bit because they would need to then reference the "Core" package to then remove the azure bits, vs having all the core bits pushed in a new MAJOR version of the current package and split the azure bits to a separate version with As such I feel like from the start the azure bits should have been a separate opt-in Also the new major version then could have something like this in as a package msbuild file that would be imported by the tooling system upon resolving it: <Message Importance="high" Text="The azure bits that was in this package has been migrated to the Microsoft.Data.SqlClient.Azure package, if you depend on these parts change the package reference from Microsoft.Data.SqlClient to Microsoft.Data.SqlClient.Azure. If you do not depend on the azure functionality you can suppress this warning with setting 'DisableAzureSqlClientMessage' to 'true'." Condition="'$(DisableAzureSqlClientMessage)' != 'true'" /> |
This still isn't making sense to me. Why do we need to break anyone? In my scenario, those that currently depend on MDS (Azure or not) should require no change. MDS currently supports Azure and will continue to. The only ones that need to change should be those that really don't want Azure (the "feature" we are removing) because they want a slimmed down dependency tree. Think of the existing MDS package as the "all in" package. For those who don't want everything, they can make changes.
I'm not going to disagree with that. But it doesn't matter. We are here now and we aren't going to make a big breaking change for Azure users (or any other users) that depend on MDS. |
|
@David-Engel i think we should avoid that long tail of breaking changes. and just do on in this repo also, to fully remove the azure bits, the fact that "change to different nuget" is significantly more works and more difficult o communicate to all consuming libs. when compared to "do a nuget update" which they are used to regularly doing also the Microsoft.Data.SqlClient + Microsoft.Data.SqlClient.Azure approach means top level apps can remove the azure bits with only a single top level ref update. where the Microsoft.Data.SqlClient + Microsoft.Data.SqlClient.Core approach requires all intermediate nugets to be updated to remove the azure bits |
|
let me clarify that specific example
now multiply that over all the libs that extend Microsoft.Data.SqlClient. ie it might not break when they update Microsoft.EntityFrameworkCore.SqlServer. it might break when they update ErikEJ.SqlClient.Extensions, or Serilog.Sinks.MSSqlServer, etc. but evernually they will update the last sql client lib that removes Microsoft.Data.SqlClient (and hence azure) and their project will break |
|
I think I see the disconnect. You are trying to make it easier for Azure to be an opt-in for everyone, including those who currently only have it via a transitive dependency. But you are proposing breaking Azure for everyone all the way down the line in order to do it. So in your scenario, everyone (EF, for example) would only depend on/get non-Azure functionality (and all their Azure users would be broken). To get Azure authentication, apps (likely at the tail end unless libs like EF want to offer a ".Azure" variant) would simply need to add a dependency on the .Azure package. I think you underestimate the disruption this would cause to existing Azure users. Given that Microsoft owns this library, uses it heavily internally (think about all the code that must be behind MS/Azure products and services), heavily promotes MDS in their docs (that's a lot of updating docs - if version >= 6, use M.D.S.Azure), and has an interest in keeping it simple and easy to use against Azure, I can't see how this change would be welcomed. |
Or do like the Another Example: Try installing using var connection = new SqlConnection(this.SqlConnectionString);
var db = new Server(new ServerConnection(connection));
db.ConnectionContext.ExecuteNonQuery(File.ReadAllText(this.SqlOutputFile));(allows loading sql code from a file on a server with As such having the Azure stuff split separate from it where the application has to |
r u proposing only a runtime api level change ? if so, this cant be only a run time change. we already have one significant bug cause by sqlclient nuget pulling in azure refs AzureAD/microsoft-authentication-library-for-dotnet#4468 i know because it broke our integration environment it has to be nuget change |
Basically yes, have the azure bits after they install the azure version of the library (which has a dependency on the base version) which:
|
not exactly. i am pointing out that even we move to "Microsoft.Data.SqlClient + Microsoft.Data.SqlClient.Core" to "prevent breaking changes for azure users". it is till a braking change for a subset of azure users. ie those that leverage Microsoft.Data.SqlClient identity transitively through something like Microsoft.EntityFrameworkCore.SqlServer. since both approaches are breaking for at least a subset of azure users. i think we should chose the approach that gets us into the desired state of "Microsoft.Data.SqlClient + Microsoft.Data.SqlClient.Azure". and also has less impact for all non-azure users.. |
ahh. yes. thats what i want |
|
if we do go with the Microsoft.Data.SqlClient + Microsoft.Data.SqlClient.Core approach. the doco will be a fun read for everyone moving forward
and that when people even bother to read and understand that doco. because the entire internet history of blogposts, stackoverflow, and training will continue to be pointing to "Microsoft.Data.SqlClient is the one you want" IMO the technical debt and significant number of people who will continue to pick the incorrect package should be reason enough to rule out that approach |
|
Thanks for the interesting discussion, everyone. I think it is obviuos that if this is going to happen (and a PR is going to be accepted),the only way to do this is via MDS.Core. There is a too long history of just having Azure auth available. Yes, it will take years for consumers to adapt to this, and some may not. So be it. Base functionality in .Core will be SQL auth, integrated Windows auth, and token auth (AccessToken, AccessTokenCallback). I will close this draft, and maybe revisit after 5.2 has been released, as next planned version seems to be 6.0 now. |
Why? As you said in a previous comment, attempts to use Azure auth with Microsoft.Data.SqlClient should result in an actionable error message with links to a suitable docs page. Wouldn't that be the best solution? I remember that Microsoft.Data.SqlClient version 4 was released with Encrypt=true by default and this really caused havoc. It had a good error message:
But it did not have a actionable error message. For this breaking change, having the error message mentions that adding a package reference to |
Because any change needs to be approved by the responsible maintainers of this repo... |
|
@ErikEJ so all the maintainers have chimed in? |
100% this. Microsoft is breaking .NET users all the time and it's never a problem. All of a sudden users want to remove the Azure fat from a package and it's a huge issue (apparently) to have a super minor breaking change which is super easy to fix. Sounds like hypocrisy and poor excuses to basically justify the shoehorning of Azure into .NET as much as possible as a .NET agenda. |
The why was about the only way to do this is via MDS.Core Only @David-Engel has argued about this route, all other participants in this thread are arguing towards But David is the lead for SQL Server drivers so I don't have high expectations on the weight of random internet people arguments. ¯\_(ツ)_/¯ |
|
A few notes/thoughts/summary from me...
|
|
Thanks for joining, @roji - your opinion is valuable to me. So your are in favor of my proposal above ❤️ Let's stand down, try to not get too emotional and think about this one more time... I will share my proposal and collect more feedback |
ErikEJ
changed the title
Not to mention from |
I am just re-opening to collect more feedback - and yes, I am pragmatic. |
The answer is no. Discussions are not based on merit. Microsoft has an agenda, people are short sighted and they essentially do what they want to the detriment of .NET if it serves themselves or the progression of Azure. Only if another MSFT employee with internal clout disagrees it carries weight, because now it's a stand-off between two employees and the one who feels inferior pulls his tiny weeny back in and makes space for the other one. It's corporate politics played out in the open, nothing else. |
|
This is what I feel should happen:
It might look that way on the surface, some areas of MSFT are unaware of what pains other areas of MSFT, so that is why sometimes those other areas have to put in their inputs to then have merit because then they would realize that it would make maintaining their other codebases that much more harder (not to mention waste a lot of money for them in terms of wasted developer's time). We all know that happy developers with minimal conflicts with maintaining code makes up the the most amount of revenue that a company can generate 😄. |
|
@AraHaan > add: Microsoft.Extensions.DependencyInjection.Abstractions That will add a new trail of dependencies for .NET Framework, so... |
But won't it be ok for .NET Framework though, even if it is used as an implementation detail internally? |
|
This is great although it will be a breaking change for me. Thanks for pointing this out |
|
@AraHaan Making this change and adding new dependencies does not seem right |
|
Everyone, just to be clear - I work at Microsoft but I don't make any decisions where SqlClient is concerned. @David-Engel is the owner here and has the final say, and I know he has SqlClient's best interests in mind, as well as the best knowledge about who uses SqlClient, how, and what breaking changes make sense in that context. Although I have my opinion, I fully trust @David-Engel will make the right choice here etc. @dustinmoris your comment really does not reflect the reality of the situation; there's no "corporate politics" or an employee show-down with clout. There really are just two Microsoft engineers here, discussing what to do the best for SqlClient and for the .NET ecosystem as a whole. We regularly engage with the community on important design decisions, and very frequently change our minds based on community feedback that we get. It's very common for people on the outside to assume "corporate politics" and various dark "behind the scenes" dealings at Microsoft - I hear this all the time - but we're really just a bunch of passionate engineers discussing and trying to arrive at the best outcome given the situation. So again, I'm just voicing my opinion here, with the added weight that I have the EF perspective (and EF is an important factor, being a major way that SqlClient is used). @AraHaan for the record, I'm against the direction of introducing DI into SqlClient. Unlike EF, SqlClient (and other ADO.NET providers) aren't currently based on DI concepts, and rewriting SqlClient to start using DI internally is a far-reaching change that doesn't really seem related to this discussion in any way. I do agree that better integration with DI is a good idea, but I'd do that in a separate package (much like how Npgsql has Npgsql.DependencyInjection for people who want it). Enabling Azure functionality via an external package (regardless of how we call it) is most probably doable without introducing DI. |
…dependencies (to demonstrate where dependencies are used and test a build) relates to dotnet#1108
|
Published payload with .NET 8 target:
|
|
Make this change, increment the version to 6.0, and break it. Why would anyone running on-prem SQL Server want all of the Azure bloat in their apps? It's super annoying that the SqlClient dragged in all these dependencies, and honestly the Microsoft team should have thought of this at the beginning. This is a great solution. Break it, fix it, move on. |
|
Thanks for all the input folks. I've just been stating my opinion based on my own experiences. It's good to hear different opinions. I'm not against having my mind changed. After reading all the comments, I'm inclined to agree that MDS + MDS.Azure seems to make more sense. (Implementation details TBD.) I'll have to discuss this with other internal stakeholders to see what their thoughts are. I don't want to make people do a bunch of work that won't ever be merged. |
|
@David-Engel thanks so much for re-considering |
|
Anything new about this yet? It has been a few months since the last comment on this. |
(to demonstrate where dependencies are used and test a build)
relates to #1108
Following linked files were removed from the .csproj: