aws / aws-ssm-data-protection-provider-for-aspnet

An extension library to assist with ASP.NET data protection in AWS Lambda.
Apache License 2.0
58 stars 21 forks source link

.NET 8 package version requires .NET 9 dependencies #78

Open dosolkowski-work opened 2 days ago

dosolkowski-work commented 2 days ago

Describe the bug

The Amazon.AspNetCore.DataProtection.SSM package is multi-targeted for both .NET 8 and .NET 9; however, both versions depend on the same 9.x major version dependencies (e.g. Microsoft.AspNetCore.DataProtection). This is difficult for projects using .NET 8 (the current LTS version) because it creates incompatibilities.

Regression Issue

Expected Behavior

Package targets should correspond with target framework versions.

Current Behavior

Package targets demand upgrades to .NET 9 packages even when still on .NET 8.

Reproduction Steps

Just add the package reference to a .NET 8 project.

Possible Solution

Make package references conditional based on target framework, as they were previously.

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

Amazon.AspNetCore.DataProtection.SSM 3.3.0

Targeted .NET Platform

.NET 8

Operating System and version

Windows 11

ashishdhingra commented 2 days ago

Looks like in https://github.com/aws/aws-ssm-data-protection-provider-for-aspnet/commit/c3aaa66b707cfda5e1097edc293f9585de9756c0, src/Amazon.AspNetCore.DataProtection.SSM/Amazon.AspNetCore.DataProtection.SSM.csproj was updated to use PackageReference for Microsoft.AspNetCore.DataProtection version 9.0.0 for all framework targets.

CC @normj for inputs.

normj commented 2 days ago

This PR explains why we made the changes we did.

The short answer was with .NET 9 release it identified transitive dependencies for this package that had vulnerabilities. We tried a few different approach but in the end we decided to follow the same pattern Microsoft do with the Microsoft.AspNetCore.DataProtection.StackExchangeRedis which it targets multiple frameworks but uses the 9.0.0 dependency for all targets. The 9.0.0 version of the Microsoft.AspNetCore.DataProtection package and its dependencies can still be used in applications targeting other frameworks besides .NET 9 because the package supports .NET Standard 2.0.

The other option is we could have done is for each target point to the latest version in that range. For example for the .NET 8 target depend on 8.0.11 Microsoft.AspNetCore.DataProtection. This still would have triggered Microsoft.AspNetCore.DataProtection and its dependencies to be copied into publish folder and the versions before 9.X have an unfortunate transitive dependency on System.Drawing.Common which I really didn't want to force users to have to carry around with them. In the 9.0.0 versions of the Microsoft.AspNetCore.DataProtection Microsoft removed the System.Drawing.Common transitive dependency.

Are you having runtime issues with the 9.0.0 version or was this concern seeing the version bump and wondering if we dropped support for previous .NET targets? Like I said we thought we were okay following the same pattern used for Microsoft.AspNetCore.DataProtection.StackExchangeRedis. It wasn't my ideal solution but to remove the vulnerability warnings which were critical it seems the safest choice.

dosolkowski-work commented 2 days ago

@normj thanks for the detailed explanation, I appreciate it.

The difference between the pattern Microsoft follows and what was done here is that Microsoft continues releasing new minor versions of old major version packages until end of support. Rather than a single version of Microsoft.AspNetCore.DataProtection.StackExchangeRedis that multi-targets net8.0 and net9.0 and uses the 9.0.0 dependency for all targets, there is instead a separate 8.0.x version range that targets net8.0 (and not net9.0) and depends on 8.0.x package versions. This approach gives maximum flexibility to users, at the cost of requiring more developer work, because multiple versions have to be maintained in parallel.

The alternative most projects choose, and I recommend, is to have one code base that multi-targets several frameworks, and references the latest version of dependencies matching the target major version (e.g. net8.0 depends on 8.0.x, net9.0 depends on 9.0.x). This is the most likely to match versions the user is already referencing and cause the fewest problems. I see the 8.0.11 version of Microsoft.AspNetCore.DataProtection having a dependency on System.Drawing.Common for net8.0 (see dependencies) so I don't think you need to worry about that.

normj commented 2 days ago

@dosolkowski-work That is a fair point that Microsoft do push also update the 8.X version and we are only going to maintain the tip version of our package. I'm still curious to know what issue using the 9.0.0 dependency cause when targeting .NET 8 or 6? From my testing I see no problems when testing .NET 6 and 8 applications.

Also are you saying 8.0.11 doesn't cause a dependency on System.Drawing.Common or it does and I should not worry about that dependency? Having something like System.Drawing.Common getting added to the deployment bundle because of our library that has nothing to do with drawing gives me a bad feeling, maybe I am over thinking it though.

dosolkowski-work commented 2 days ago

@normj in our case there is a collision trying to update to the latest version because we have direct dependencies on the data protection packages already; we could try upgrading, but I believe the 9.0.x packages have the same support lifecycle as .NET 9 itself, which means upgrading to them would actually reduce our support window (May 2026 instead of November 2026) which is not ideal.

When targeting .NET 8, version 8.0.11 of Microsoft.AspNetCore.DataProtection does not have a dependency on System.Drawing.Common anywhere in its dependency tree. If you are targeting a different, legacy framework, it might; version 9.0.0 is the same, and it's also no different from the prior version of this library. I'm not sure if there's any way to browse the package hierarchy on nuget.org to see this easily.

normj commented 2 days ago

@dosolkowski-work That is a valid point about the support cycle. And now I'm really confused why I was seeing System.Drawing.Common getting put in my publish folder. I can't recreate that happening.

I put out PR to have the .NET 8 target point to version 8.0.11. With the US holidays and re:Invent happening next week we are under restricted release rules and I won't be able to release this change till after re:Invent.