serilog-mssql / serilog-sinks-mssqlserver

A Serilog sink that writes events to Microsoft SQL Server and Azure SQL
Apache License 2.0
283 stars 148 forks source link

Version 6.5.1 adds an unexpected dependency on the Microsoft.WindowsDesktop.App Runtime. #516

Closed rarowston closed 2 months ago

rarowston commented 9 months ago

Please clearly describe what the SQL Sink is doing incorrectly:

Updating from version 6.5.0 to version 6.5.1 adds an unexpected dependency on the Microsoft.WindowsDesktop.App Runtime.

Deploying the application to the server after upgrading the version causes the application to fail to start. Some digging revealed that the failure is due to a missing runtime. My testing seems to indicate that it is this specific version bump that added this dependency.

Please clearly describe the expected behaviour:

Version 6.5.1 can run successfully using only the Microsoft.AspNetCore.App and Microsoft.NETCore.App runtimes. Failing that, if this runtime dependency is required, a note in the release notes/readme to warn others that this is now a dependency that they will need to manually install on their hosting environment if they have not already.

List the names and versions of all Serilog packages used in the project:

Target framework and operating system:

.NET 8 Windows Server 2012

Author's note: This is not a critical issue as I can work around it by installing the runtime or not upgrading the version (though there are transitive vulnerabilities with this option). I am mostly posting this to help others more quickly diagnose this issue if they run into the same situation. Reducing dependencies is a nice-to-have as well.

ckadluba commented 9 months ago

Hi @rarowston,

thank you for reporting this and for your investigation.

We did not change any target framework definitions in the sink library. Also there was only one direct dependency which was upgraded from release 6.5.0 to 6.5.1 which was to fix a security vulnerability. This dependency is Microsoft.Data.SqlClient and it was upgraded from 5.0.1 to 5.1.4 (https://github.com/dotnet/SqlClient/compare/v5.0.1...v5.1.4#diff-2d39be8d043bee88a5034519b11cd2b45b0117b3baa8528f11def532bcdfe611).

This needs furhter investigation.

ckadluba commented 9 months ago

@rarowston what error message do you get when the application fails to start?

jonorossi commented 9 months ago

The bug (https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/4468) is in Microsoft.Identity.Client used by SqlClient. The library assumes *-windows TFM means desktop Windows.

Kolthor commented 9 months ago

We recently had the same issue after upgrading some NuGet packages. We didn't investigate it so far as to find out which package introduced the problem, but it was when this package was 6.5.0, and it is very possible that we used a more recent SqlClient than 6.5.0 required.

What we did to solve it, was changing the target framework in our projects from net6.0-windows to net6.0.

Even updating the dotnet hosting bundle on the server, didn't solve the problem.

ckadluba commented 9 months ago

@jonorossi thanks for the hint. SqlClient even had their own issue regarding this (https://github.com/dotnet/SqlClient/issues/2298) which is now closed because it has to be fixed in MSAL (with reference to the bug you posted).

So i thing we have to wait for that if it is not a critical issue for the MSSSQL sink.

rarowston commented 9 months ago

Well found @jonorossi The MSAL issue does indeed seem to be the root cause of this problem and I have been able to "fix" it with the workaround of removing the specific windows target. This adds a lot of warnings as there are some windows only functions in use, but this does work. It looks like they've added it to a milestone for Feb 8th (https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/milestone/124) so hopefully they add a fix soon and this can cascade back up.

SimonCropp commented 9 months ago

for context here is the related PR https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/4567

ckadluba commented 9 months ago

Very good. Thank you!

rarowston commented 5 months ago

The PR that fixes the root problem of this has been merged in and released, though It will likely take a while for this to cascade up through all affected packages. Eventually I suspect this will be fixed with a package update.

In the meantime, for the benefit of anyone else in the same position, I can now fix this issue by directly including a newer version of Azure.Identity.

ckadluba commented 2 months ago

According to this https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/releases/tag/4.61.0 this should be fixed in Microsoft.Client.Identity 4.61.0 which we just upgraded by using SqlClient 5.1.6 in our latest dev release:

https://www.nuget.org/packages/Serilog.Sinks.MSSqlServer/6.7.1-dev-00084

If anyone who experienced this issue could try it out and give us some feedback, we would highly appreciate this.

ckadluba commented 2 months ago

Fixed in release 6.7.1 https://github.com/serilog-mssql/serilog-sinks-mssqlserver/pull/555

rarowston commented 2 months ago

I have updated to 6.7.1 and removed specific version references to the Azure Identity package (which was the workaround while waiting for this fix to cascade all the way up).

I can confirm that I no longer have the issue after this update, so this indeed appears to be fully resolved.

Thanks for all your help

ckadluba commented 2 months ago

@rarowston thank you very much for your help and the nice words. I'm glad we could solve this issue. :)