System.Net.IPAddress - open to adding more features?

HumanEquivalentUnit commented 5 years ago

IPAddress is quite basic, particularly trying to use it from high level scripting PowerShell world, it would be useful if it had more features. Could it have?

[Edit: the original part of this issue was about IpAddress.NetworkToHostOrder(long) and it turns out their implementation has bugs. They ought to reverse just the IP address bytes inside an Int64, but they incorrectly reverse the entire Int64. That would be a breaking change to fix, so I've cut a lot of that discussion out of here].

uint32 overloads?

[Edit: I asked for overloads which take [uint32] because I didn't understand how the Int64 overloads worked. Now I understand the way Int64 overloads should work, that would be OK - except the Int64 overloads are broken. From the discussion, unsigned ints are not CLS compliant, so this might not happen. Not sure what could be improved here for convenience now.]

IComparable

It would be nice to compare IP addresses, to sort them into human-friendly order, to compare if an IP is between address range start and end points, e.g. if this following code worked and returned $true. (PowerShell syntax):

PS C:\> [ipaddress]::parse('10.0.0.1') -lt [ipaddress]::parse('10.0.0.5')
Cannot compare "10.0.0.1" because it is not IComparable.

It is possible to compare IP addresses like this, but it's not implemented.

Subnet tests

Even without comprehensive subnet handling, a method for

PS C:\> [ipaddress]::parse('10.0.0.130').IsInSubnet('10.0.0.128/25')

would be useful.

Addition

Stepping through the IPs in a subnet is useful (the fact that IPv6 subnets are huge doesn't render it a useless operation). Without full and general subnet handling, a .PlusOne() instance method which returned this IP + 1, combined with the above IComparable would allow an easy "walk to a cutoff IP".

everything else Python does

https://docs.python.org/3/library/ipaddress.html

Full subnet support with enumeration of hosts (at least for smaller subnets), overlap testing, all kinds of convenience methods, etc.

Is any of that open to consideration / implementation? (Is .Net open to be as comprehensive as Python's "batteries included" libraries or is that explicitly not an approach it wants to go for?)

EgorBo commented 5 years ago

why whould you need to convert BE-long-ip to IPAddress, should not it be [ipaddress]::new([ipaddress]::NetworkToHostOrder(-4294967296)) instead?

however, it looks like ws2_32's htonl/ntohl converts 4294967295 to 4294967295 I suspect it does (data >> 32 & 0xFFFFFFFF) (casts to uint32_t?) before reversing bytes on LE.

HumanEquivalentUnit commented 5 years ago

why whould you need to convert BE-long-ip to IPAddress

I don't know that I would want to - my point was that 255.255.255.255 should be the same in BE and LE, but IPAddress doesn't see that.

Your comment about (data >> 32 & 0xFFFFFFFF) might be why; the htonl/ntohl code in IPAddress calls the same code each way, without trying to use the least significant 32 bits: https://github.com/dotnet/corefx/blob/master/src/System.Net.Primitives/src/System/Net/IPAddress.cs#L390

(Which might be a bug by itself?)

EgorBo commented 5 years ago

I don't even see (u)int64_t htonl/ntohl versions on *nix (e.g. https://linux.die.net/man/3/htonl) winsock's one does accept u_long but works like it accepts uint32_t

saucecontrol commented 5 years ago

The only reason those APIs use long at all is because uint isn't CLS compliant. uint is certainly the correct type to use there

EgorBo commented 5 years ago

@saucecontrol I'd suggest to add (uint) cast to https://github.com/dotnet/corefx/blob/master/src/System.Net.Primitives/src/System/Net/IPAddress.cs#L392 (I mean BinaryPrimitives.ReverseEndianness((uint)host)) but I am not sure if it's a breaking change or not.

saucecontrol commented 5 years ago

Yeah, that's messed up. It'd be right for the case where someone's using it for an IPAddress, but since it could be used elsewhere, I guess you can't. It might be worth an API proposal to add non-CLS-compliant unsigned overloads, though.

karelz commented 5 years ago

Can someone please clarify what is the goal of this API proposal? Can someone please propose it in the preferred API proposal form? https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

HumanEquivalentUnit commented 5 years ago

Can someone please clarify what is the goal of this API proposal?

Make PowerShell more convenient. System.Net.IPAddress has very few features, and scripting around IP Addresses would be easier if it had many features. As many as possible. As many as Python, even.

Can someone please propose it in the preferred API proposal form?

I am no C# developer, and don't know if I could come up with good API suggestions. Especially hesitant without understanding CoreFX's stance on the last question in the opening post: "Is any of that open to consideration / implementation? (Is .Net open to be as comprehensive as Python's "batteries included" libraries or is that explicitly not an approach it wants to go for?)"

If the team is going to rule "we don't want to make any changes except bugfixes" or "we want to keep it minimal", that would avoid a lot of back and forth.

karelz commented 5 years ago

I am not familiar with PowerShell (few people around are AFAIK), so that will make it "interesting" to design it accross languages. On one hand, I would love to help PowerShell customers, on the other hand, CoreFX / .NET is not platform only for PowerShell, so the APIs would have to make sense in larger context. If the APIs are truly scripting specific, I wonder if PowerShell should create some features on their end - for example extension methods.

I am open to ideas, but we will need more information and thinking through larger scenarios in this proposal to make it actionable, not just from PowerShell point of view. Our goal should not be to become the scripting API surface (which is likely what you meant by your Python reference) Does it help as answer?

AnthonyMastrean commented 5 years ago

The sorting/comparing situation really seems like it should be a Bug.

karelz commented 4 years ago

Triage:

IComparable seems very specific to printing - we think PowerShell can add extension. We do not see how it would be useful to general .NET devs.
IsInSubnet sounds potentially useful. Please break it out into specific standalone API proposal with use cases listed (from general .NET dev point of view).
- Concern: Passing in string is weird. We do not have general IP mask class. How to do it reasonably without adding lots of new types for such small addition?
Addition - seems like very rare use case. Maybe we don't understand it enough?

We looked at the Python reference, but it is not clear which additional APIs may be truly useful for general .NET devs - if you have opinions, please file separate API proposals for them with motivation / use case scenarios.

Closing issue as the actionable items should be broken into separate issue.

AnthonyMastrean commented 4 years ago

@karelz I am building networking related apps in .NET Core and ASP.NET Core (mostly webapi apps)... having IPAddress and Physical Address sort and compare properly is extremely important.

It would be killer if it had parity with PostgreSQL's inet functions in order to enforce model binding/validations and whatnot before hitting the storage layer.

karelz commented 4 years ago

@AnthonyMastrean to help make your case, we will need more information - details on what are the scenarios (not just a specific one), why it is best way, etc.

dotnet / runtime