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

[API Proposal]: Move IPNetwork to System.Net and add a string constructor #86584

Closed
pedro823 opened this issue May 22, 2023 · 4 comments
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Milestone

Comments

@pedro823
Copy link

Background and motivation

IPNetwork currently sits in the Microsoft.AspNetCore.HttpOverrides namespace. However, this class is of great use for applications that don't necessarily run on ASP.NET.

For example, This console tool built on golang is able to de-duplicate IPs and merge CIDRs: https://github.com/zhanhb/cidr-merger.
Currently, if you'd want to implement this in dotnet, you'd either need to import aspnet into your console application, or copy-paste the IPNetwork class into your code.

Additionally, the IPNetwork class could either implement a string constructor or implement IParse<IPNetwork>, in order to reduce manual parsing from the user's side. Here is an IPHelper class which implements a simplified parser and helps us visualize a small nuance:

public static class IPHelper
{
    public static IPNetwork ParseCidr(string cidr)
    {
        var splitCidr = cidr.Split('/', 2);

        var ip = IPAddress.Parse(splitCidr[0]);

        var maskLength = (splitCidr.Length, ip.AddressFamily) switch
        {
            // if less than 2, the original string is a simple IP address. We must consider it as the full mask.
            (< 2, AddressFamily.InterNetwork) => 32,
            (< 2, AddressFamily.InterNetworkV6) => 128,
            _ => int.Parse(splitCidr[1])
        };

        return new IPNetwork(ip, maskLength);
    }
}

API Proposal

namespace System.Net;

public class IPNetwork
{
    public IPNetwork(string cidr) { /* implementation */ }
    // same class as Microsoft.AspNetCore.HttpOverrides.IPNetwork here
}

API Usage

namespace System.Net;

var cidr = IPNetwork.Parse("192.168.0.0/16")

Console.WriteLine(cidr.Contains("192.168.0.1"));

Alternative Designs

namespace System.Net;

public class IPNetwork : IParsable<IPNetwork>
{
    // Implement IParsable instead of a string constructor...
}

Risks

Current users of the IPNetwork class will have to change their imports. I don't know much about the process of whether that is a breaking change or not, but that's something to consider when applying this change.

Alternatively, we can keep the two classes, but since IPNetwork is such a recent addition, I feel like going the breaking change route may be the fastest way to move forward.

@pedro823 pedro823 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 22, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 22, 2023
@ghost
Copy link

ghost commented May 22, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

IPNetwork currently sits in the Microsoft.AspNetCore.HttpOverrides namespace. However, this class is of great use for applications that don't necessarily run on ASP.NET.

For example, This console tool built on golang is able to de-duplicate IPs and merge CIDRs: https://github.com/zhanhb/cidr-merger.
Currently, if you'd want to implement this in dotnet, you'd either need to import aspnet into your console application, or copy-paste the IPNetwork class into your code.

Additionally, the IPNetwork class could either implement a string constructor or implement IParse<IPNetwork>, in order to reduce manual parsing from the user's side. Here is an IPHelper class which implements a simplified parser and helps us visualize a small nuance:

public static class IPHelper
{
    public static IPNetwork ParseCidr(string cidr)
    {
        var splitCidr = cidr.Split('/', 2);

        var ip = IPAddress.Parse(splitCidr[0]);

        var maskLength = (splitCidr.Length, ip.AddressFamily) switch
        {
            // if less than 2, the original string is a simple IP address. We must consider it as the full mask.
            (< 2, AddressFamily.InterNetwork) => 32,
            (< 2, AddressFamily.InterNetworkV6) => 128,
            _ => int.Parse(splitCidr[1])
        };

        return new IPNetwork(ip, maskLength);
    }
}

API Proposal

namespace System.Net;

public class IPNetwork
{
    public IPNetwork(string cidr) { /* implementation */ }
    // same class as Microsoft.AspNetCore.HttpOverrides.IPNetwork here
}

API Usage

namespace System.Net;

var cidr = IPNetwork.Parse("192.168.0.0/16")

Console.WriteLine(cidr.Contains("192.168.0.1"));

Alternative Designs

namespace System.Net;

public class IPNetwork : IParsable<IPNetwork>
{
    // Implement IParsable instead of a string constructor...
}

Risks

Current users of the IPNetwork class will have to change their imports. I don't know much about the process of whether that is a breaking change or not, but that's something to consider when applying this change.

Alternatively, we can keep the two classes, but since IPNetwork is such a recent addition, I feel like going the breaking change route may be the fastest way to move forward.

Author: pedro823
Assignees: -
Labels:

api-suggestion, area-System.Net

Milestone: -

@MihaZupan
Copy link
Member

We are adding an IPNetwork type to System.Net in .NET 8.0.
See the approved API shape: #79946 (comment)
Is there some functionality that you would like to see that isn't covered by that?

@stephentoub
Copy link
Member

We are adding an IPNetwork type to System.Net in .NET 8.0.

And it's already implemented and in preview releases:
https://learn.microsoft.com/en-us/dotnet/api/system.net.ipnetwork?view=net-8.0
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Primitives/src/System/Net/IPNetwork.cs

@pedro823
Copy link
Author

Oh wow! Didn't realize it was already in the preview version. We're currently using .NET 7 in my project.

The functionality of allowing an IP address to be a CIDR only containing itself is custom enough to not make it worth to implement in the base System.Net namespace. Therefore, I feel like this is already solved on .NET 8.

Can I close this issue then?

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label May 22, 2023
@karelz karelz added this to the 8.0.0 milestone May 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net
Projects
None yet
Development

No branches or pull requests

4 participants