Skip to content

Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork #62490

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

Merged
merged 28 commits into from
Jul 19, 2025

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Jun 27, 2025

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
@MihaZupan
Copy link
Member

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
Contributor Author

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

@halter73
Copy link
Member

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
Contributor Author

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
Member

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
… 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
Copy link
Contributor Author

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
Member

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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Obsolete IPNetwork
5 participants