Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should platform package pruning data represent GA of a framework or the serviced version? #44566

Open
ericstj opened this issue Oct 31, 2024 · 12 comments
Assignees
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Oct 31, 2024

This relates to NuGet/Home#13634

We have two options:

  1. Represent the GA version of the framework in this data, eg: 10.0.0, 8.0.0, etc.
  2. Represent the latest patched version of the framework in this data, eg: 10.0.1, 8.0.10

Today we update our conflict resolution data (PackageOverrides.txt, PlatformManifest.xml) in servicing (option 2). In the past this was intentionally pinned at GA (option 1).

The benefit of updating this data in servicing is that it means less stuff will ship in the app when the framework version will satisfy it. So even if someone brings in Microsoft.Extensions.Hosting 9.0.2 and it's closure, you can still drop all those packages when on targeting the Asp.NETCore framework 9.0.

The downside of this is that you might be missing a hotfix package that you actually want to carry app-local if you happen to be running on an older runtime.

I tend to think that our users prefer to carry less in their app, and instead count on a framework update to keep them up to date. We can always have the escape hatch of adding a direct reference. I'd like to get other's thoughts on this cc @richlander. Based on what we decide it will determine both what the shared framework providers do with conflict resolution data, and what the SDK does with the pruning data (including for down-level frameworks).

cc @dsplaisted @nkolev92 @ViktorHofer

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Workloads untriaged Request triage from a team member labels Oct 31, 2024
@marcpopMSFT marcpopMSFT removed the untriaged Request triage from a team member label Nov 12, 2024
@marcpopMSFT marcpopMSFT added this to the 9.0.2xx milestone Nov 12, 2024
@richlander
Copy link
Member

Sorry for being late on this important topic. I finally read the pruning spec, too. It looks good, although I don't claim to have all the required context to properly review it.

I think it is very important that we go with option 2. Option 1 provides considerably less value to the user and would AFAICT be a regression. Option 2 is also much easier to explain. I find its behavior quite intuitive. It also aligns with our plan to publish packages more often (per the freshness check).

We pin to .0 for framework dependencies for FDD apps. That's quite different, has a different set of tradeoffs, and continues to make sense. Perhaps we also did this in the "framework as packages" era for packages. I don't have that context.

@richlander
Copy link
Member

Judging by the labels, are we planning on targeting 9.0.200? I like that timing in terms of timeliness but wonder if we have sufficient ability to validate the behavior. The change is quite significant.

@marcpopMSFT
Copy link
Member

Judging by the labels, are we planning on targeting 9.0.200? I like that timing in terms of timeliness but wonder if we have sufficient ability to validate the behavior. The change is quite significant.

Depending on the solution we go with, we likely want to include metadata on these packages in an earlier release which would allow us to test out and validation assumptions.

@marcpopMSFT
Copy link
Member

I think it is very important that we go with option 2. Option 1 provides considerably less value to the user and would AFAICT be a regression. Option 2 is also much easier to explain. I find its behavior quite intuitive. It also aligns with our plan to publish packages more often (per the freshness check).

I'm not sure I agree. The primary benefit for customers is that the project assets json would no longer be carrying around a long list of runtime binaries you didn't need and those wouldn't get downloaded during the builds either. Doing the .0 version is the primary benefit and doing the .N version has potential marginal benefit on top.

Personally, I think the amount of work to make sure .N is correct and the confusion of how to explain that distinction isn't worth it. The .0 is simpler to explain and it likely what most customers want (ie the SDK shouldn't control what versions of framework assemblies you get, your shared framework install should)

@richlander
Copy link
Member

What would the behavior be if 10.0.1 STJ is in a package graph with a CVE and is being restored with the 10.0.102 SDK (which has a fix for the STJ CVE?

@marcpopMSFT
Copy link
Member

We probably need to articulate the behavior if it's a direct reference vs a transitive reference and what TFM you have to ouline the various scenarios. If I'm targeting net10.0 in an app, I shouldn't even have a reference to STJ 10.0.1. In my mind, if you're targeting 10, you don't want any 10.* STJ in your package graph regardless if it's direct or transitive as the customer would expect the shared framework to control that. It doesn't matter if you have an explicit reference to 10.0.1 or 10.0.19, we would remove it from the graph and rely on the framework. Only if you're targeting <10 would we include it.

@zivkan
Copy link
Member

zivkan commented Dec 6, 2024

If someone writing a package targets netstandard2.0, to use STJ, they need to reference the package. Perhaps version 10.0.1. Now a customer building a net10.0 app references the package and has STJ 10.0.1 as a transitive dependency. But if that gets a security vulnerability, then 10.0.2 needs to be referenced for NuGetAudit to avoid warnings.

For this reason, I think it's quite useful for the SDK to prune the serviced version, rather than the GA version.

We've seen, during the month that NuGetAuditMode was set to all, that there's a significant number of customers who do not want to promote transitive packages to top-level in order to resolve transitive package vulnerabilities. While CPM + transitive pinning is one alternative, it has an onboarding cost + other side-effects.

@richlander
Copy link
Member

But if that gets a security vulnerability, then 10.0.2 needs to be referenced for NuGetAudit to avoid warnings

That's unacceptable and what we need to avoid.

For this reason, I think it's quite useful for the SDK to prune the serviced version, rather than the GA version.

This is my thinking.

@dsplaisted
Copy link
Member

In a discussion with @marcpopMSFT, he had a good point: If we want to go with the serviced version for pruning packages, could we instead of updating the versions in the pruned package list every month have the logic be that ANY patch version of the package would be pruned. You could think of the pruned package version as 10.0.*, or 10.0.999.

If we're saying that we always want to update the pruned package list to the latest patch version, maybe we don't actually need to update the list, but we can assume that if we see any patch version of the same major/minor version that it should be pruned.

@ericstj @richlander

@richlander
Copy link
Member

We need to ensure we have correct behavior for a package ref to 10.0.2 on a 10.0.1 runtime. You mention major/minor but not patch. Would be good to understand your intent on patch handling/matching.

@dsplaisted
Copy link
Member

We need to ensure we have correct behavior for a package ref to 10.0.2 on a 10.0.1 runtime. You mention major/minor but not patch. Would be good to understand your intent on patch handling/matching.

What if you're building a framework dependent app on the SDK that corresponds to 10.0.2, and you have a reference to a 10.0.2 package. With option 2, then that package reference will be pruned (and if it wasn't, it would still be removed via conflict resolution). But if you deploy the app to an environment that only has the 10.0.1 runtime, then you'll end up using the 10.0.1 version of the binary instead of the 10.0.2 version which was referenced.

My understanding is that this is the behavior we want. Given that, maybe we should always prune any 10.0.x version of the package. Then you could end up in the same situation for a self-contained app, if you were building on an SDK that only knew about 10.0.1 but had a package reference to a 10.0.2 package, where you get a lower version of the DLL than you referenced. But given that it seems we're OK with that for framework-dependent, maybe we should be OK with it for self-contained.

Does that answer the question? What scenario where you asking about for a 10.0.1 runtime? Framework dependent, self-contained, or something else?

@richlander
Copy link
Member

richlander commented Dec 11, 2024

What scenario where you asking about for a 10.0.1 runtime?

The scenario I had in mind was using a 10.0.101 SDK to build an app that had a 10.0.2 package reference.

That said, you bring up good points. In essence, we cannot guess the target runtime patch for FDD apps and that's why we always target the .0 patch for the runtime.

I think your and Marc's idea is a good one for FDD apps. We always prune a 10.x package when targeting a 10.0 runtime, even if the packageref version is higher.

SCD apps are a more nuanced because we have perfect information and we could choose a different behavior. However, I think adopting the same behavior makes sense and is the easiest to explain. So, I agree.

The moral of this story is that if you have a package reference that overlaps with a fx binary and the major/minor version matches, you'll ALWAYS get the fx version. We only respect the packagereference if it is to a higher version, such as an 11.x packageref with a 10.x runtime. This feels like a solid default behavior.

The potential downside of this policy is the following:

  • We cannot fast-track fixes to packages.
  • Users must update their runtime to get fixes

We could consider an opt out for pruning as a mitigation, on a package-basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants