Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork #62490
Conversation
|
I don't think this change should go through until dotnet/runtime#117114 is addressed. |
|
Yeah, thanks, that's why still makes it as draft |
|
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 How does |
@halter73 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! 😃 |
… 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
|
There're still some differences between |
|
Oh that's a mistake, we definitely want to obsolete the HttpOverrides.IpNetwork type. |
|
Thanks @WeihanLi ! |
| { | ||
| return true; | ||
| } | ||
| foreach (var network in _options.KnownIPNetworks) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So should we remove the init value for KnownNetworks?
There was a problem hiding this comment.
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.
Obsolete Microsoft.AspNetCore.HttpOverrides.IPNetwork
Obsolete
Microsoft.AspNetCore.HttpOverrides.IPNetworkto preferSystem.Net.IPNetworkDescription
ForwardedHeadersOptions, obsoleteKnownNetworks, and addKnownIPNetworksto preferSystem.Net.IPNetworkMicrosoft.AspNetCore.HttpOverrides.IPNetworkasObsoletefixes #46157