Skip to content

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork#62490

Merged
BrennanConroy merged 28 commits intodotnet:mainfrom
WeihanLi:patch-1
Jul 19, 2025
Merged

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork#62490
BrennanConroy merged 28 commits intodotnet:mainfrom
WeihanLi:patch-1

Conversation

@WeihanLi
Copy link
Copy Markdown
Contributor

@WeihanLi WeihanLi commented Jun 27, 2025 •
edited
Loading

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork to prefer System.Net.IPNetwork

Description

  • Update ForwardedHeadersOptions, obsolete KnownNetworks, and add KnownIPNetworks to prefer System.Net.IPNetwork
  • Mark Microsoft.AspNetCore.HttpOverrides.IPNetwork as Obsolete

fixes #46157

@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 27, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 27, 2025
@build-analysis build-analysis bot mentioned this pull request Jun 28, 2025
Open
@MihaZupan
Copy link
Copy Markdown
Member

MihaZupan commented Jun 30, 2025

I don't think this change should go through until dotnet/runtime#117114 is addressed.
As-is this is bound to break people due to differences in behavior.

@MihaZupan MihaZupan added the blocked The work on this issue is blocked due to some dependency label Jun 30, 2025
@WeihanLi
Copy link
Copy Markdown
Contributor Author

WeihanLi commented Jun 30, 2025

Yeah, thanks, that's why still makes it as draft

stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
stephentoub
stephentoub reviewed Jul 11, 2025
View reviewed changes
@build-analysis build-analysis bot mentioned this pull request Jul 11, 2025
Open
@halter73
Copy link
Copy Markdown
Member

halter73 commented Jul 11, 2025

I agree we should obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork type to avoid confusion, but we may never actually delete it. I think we can keep any methods used convert from the old type to System.Net.IPNetwork for the purposes of backwards compatibility internal. It's not like HttpOverrides.IPNetwork was an exchange type, and it's very similar to the new type, so it should be pretty easy to migrate code to use a new System.Net.IPNetwork-property without needing us to provide public conversion methods.

The big question is what to name the new System.Net-based version of the ForwardedHeadersOptions.KnownNetworks property now that the good name has been burned by the to-be-obsoleted type. We cannot simply change the type and retain binary compatibility with an API we've shipped since at least .NET Core 2.

How does KnownIPNetworks sound? We could update #46157 to be an API proposal issue with this name and mark it api-ready-for-review. Using the [Obsolete] attribute on the existing KnownNetworks property can point people to the right popertiy to use.

@WeihanLi
Copy link
Copy Markdown
Contributor Author

WeihanLi commented Jul 14, 2025

We could update #46157 to be an API proposal issue with this name and mark it api-ready-for-review. Using the [Obsolete] attribute on the existing KnownNetworks property can point people to the right popertiy to use.

@halter73 I assume I should wait for the API approval to get approved before moving ahead

@BrennanConroy
Copy link
Copy Markdown
Member

BrennanConroy commented Jul 16, 2025

I assume I should wait for the API approval to get approved before moving ahead

API has been approved, please move ahead with the change! 😃

@BrennanConroy BrennanConroy removed the blocked The work on this issue is blocked due to some dependency label Jul 17, 2025
WeihanLi added 5 commits July 18, 2025 08:29
@WeihanLi
Merge branch 'patch-1' of https://github.com/WeihanLi/aspnetcore into…
51dba56 
… patch-1

# Conflicts:
#	src/Middleware/HttpOverrides/src/ForwardedHeadersOptions.cs
#	src/Middleware/HttpOverrides/src/IPNetwork.cs
#	src/Middleware/HttpOverrides/src/PublicAPI.Unshipped.txt
#	src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs
@WeihanLi
feat: add KnownIPNetworks
8795915
@WeihanLi
feat: update PublicAPI.Unshipped.txt
22748db
@WeihanLi
feat: add back Clear for legacy IPNetwork
5f59285
@WeihanLi
feat: revert Microsoft.AspNetCore.HttpOverrides.IPNetwork
720d723
@WeihanLi
Copy link
Copy Markdown
Contributor Author

WeihanLi commented Jul 18, 2025

There're still some differences between System.Net.IPNetwork and Microsoft.AspNetCore.HttpOverrides.IPNetwork like the Prefix/BaseAddress, which would break the test cases, and there's no Obsolete for Microsoft.AspNetCore.HttpOverrides.IPNetwork from the API approval, so I reverted the change for Microsoft.AspNetCore.HttpOverrides.IPNetwork

@WeihanLi WeihanLi marked this pull request as ready for review July 18, 2025 05:19
@WeihanLi WeihanLi requested a review from halter73 as a code owner July 18, 2025 05:19
@BrennanConroy
Copy link
Copy Markdown
Member

BrennanConroy commented Jul 18, 2025

Oh that's a mistake, we definitely want to obsolete the HttpOverrides.IpNetwork type.

BrennanConroy
BrennanConroy reviewed Jul 18, 2025
View reviewed changes
@BrennanConroy BrennanConroy merged commit 0371d87 into dotnet:main Jul 19, 2025
30 checks passed
@BrennanConroy
Copy link
Copy Markdown
Member

BrennanConroy commented Jul 19, 2025

Thanks @WeihanLi !

@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview7 milestone Jul 19, 2025
@WeihanLi WeihanLi deleted the patch-1 branch July 19, 2025 02:45
@wtgodbe wtgodbe modified the milestones: 10.0-preview7, 10.0-rc1 Jul 23, 2025
sebastienros
sebastienros reviewed Sep 12, 2025
View reviewed changes
src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs
{
return true;
}
foreach (var network in _options.KnownIPNetworks)
Copy link
Copy Markdown
Member

@sebastienros sebastienros Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue with this part of the change when users can only call Clear() on a single collection?

Take this sample in an issue that was filed recently.

The properties KnownIPNetworks and KnownNetworks are both initialized with a default value of IPAddress.Loopback.

If ForwardedHeadersOptionsSetup is not invoked then one of the collections won't be cleared and then taken into account.

Copy link
Copy Markdown
Contributor Author

@WeihanLi WeihanLi Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we remove the init value for KnownNetworks?

Copy link
Copy Markdown
Member

@sebastienros sebastienros Sep 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My analysis seems to be confirmed.

I don't think changing the default for one collection is the solution. One that could work would be to keep exposing the two properties, but backed by the same collection instance. Then in the internal code only manipulate the non-obsolete (for instance). Or have public IList<AspNetIPNetwork> KnownNetworks { get; } => KnownIPNetworks;, same end-result.

@sebastienros sebastienros mentioned this pull request Sep 12, 2025
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastienros sebastienros sebastienros left review comments

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

10.0-rc1

Development

Successfully merging this pull request may close these issues.

Obsolete IPNetwork

7 participants

@WeihanLi @MihaZupan @halter73 @BrennanConroy @sebastienros @stephentoub @wtgodbe