Test to determine network class in RankIpAddress is not comprehensive and could be removed

[brudo]

I see an issue in the code for RankIpAddress, where it seems to embed some arbitrary choices which bump up the priority for certain private IP addresses, one example from each class, but not to other IP addresses that belong to the same class.

As far as I can tell this is only used to sort the list of current connected clients, and for that matter the order the clients are returned in probably doesn't matter in most cases, so the impact appears to be small.

Maybe this is an attempt to get non-Internet clients listed ahead of Internet clients for some reason, although I'm not sure why. The test could be made more complete by checking the full ranges, e.g. 192.168.0.0 - 192.168.255.255, 172.16.0.0 - 172.31.255.255, also checking 127.0.0.1, etc. (and what about IPv6?), it could also be appropriate to remove the ranking code altogether, or just sort them lexically if you want them returned in a consistent order.

For my case, our connected clients do not come from any of the special network prefixes that were selected, so they are currently all the same rank as far as we're concerned. Doesn't seem to cause any harm.

[BrandonPotter]

A little background on that one - I agree it is a little bit of a hack. The reasoning is that a lot of times in your application, you want to display the IP address(es) you are listening on.

In a lot of my cases, you add VPN adapters and VMware adapters to your system and there's not really a good way to tell which ones are actually usable from the outside LAN, without either a complicated analysis of your routine tables, or this ranking that just says "these are the most normal looking networks". Open to suggestions for making this better.

[brudo]

I would say that there are other addresses that are just as normal as the ones your coat declares "most normal". Even for VPN adaptors, local VM networks and such, if there is a connected client coming from that address it is in some sense being used normally.

You could use metrics from the active local routing table (for the local endpoint of the connection) to determine an appropriate order for the clients to be listed, but this would also be somewhat arbitrary.

Of course, if you really care which network is being used, you can bind to specific address, or use a firewall to block unwanted addresses.

So my suggestion is that removing this functionality will make it better :).

[BrandonPotter]

Thinking through this - if we remove it from this library I have quite a few apps that will just be moving that same type of logic to a different place. Playing devil's advocate, if you're suggesting that just taking the order that the OS returns them is better (which I would guess is different on every system), that's just as arbitrary as this hacky sort (maybe less predictable from system to system), so what difference would it make?

I'm inclined to possibly reference some standard like RFC 1918 or something as a way of at least adhering to some standard, and making it more clear in the library that you intend to use that standard to prioritize IP's. Perhaps an overloaded enum where you can specify the type of prioritization. But I dunno if RFC1918 covers IPv6 and so forth so there's another can of worms.

Maybe expand on your routing table idea a bit as well. I'd like to think of this as "how do we find the address which is most likely to be the one a user could send to someone else to connect to their app across a LAN?" and solve that in the best way possible.

[brudo]

The order could even vary from one call to the next, depending what kind of collection is used to hold the clients, who has connected or disconnected since the last call, etc. But current sorting algorithm will have a lot of ties, which may still present in an arbitrary / random / platform dependent order.

The simplest order, if it must be consistent, it's just to sort them alphabetically (lexically) by their string representations. But if you want clients connected to your preferred IP address to be listed first, that should be possible by looking at the local endpoint from the connected socket.

To your point about knowing which address to tell someone so that they can connect: I am not looking at the code right now, but isn't this method used only to sort clients that are already connected?

[BrandonPotter]

The ranking doesn't have anything to do with client connections - it is used on the server side to list which IPs were successfully bound for listening (assuming you did not explicitly specify any).

[brudo]

OK, I see what you mean and I did miss that in both places where the ranking function is used, it is referencing local IP addresses. I thought one of the uses was referencing remote client addresses, but it is referencing the listeners that may or may not have clients. I just didn't look at that piece of code closely enough to understand it.

This raises the question of why not bind to 0.0.0.0, and have just one listener, instead of binding each NIC separately, but perhaps that is for Linux / Mono compatibility?

For simple consistent ordering, perhaps just rank the NIC used for the default gateway (e.g. first 'Up' NIC with 'Any' GatewayAddresses) above the others, with the address from the same subnet as the default gateway coming first, and otherwise list them in the same order as provided by the OS?

[brudo]

In reference to the point on 0.0.0.0, that can easily be achieved with SimpleTCP using Start(IPAddress.Any, portNum). My pull request referenced above is for an alternate ranking implementation, where:

• strong preference is given to addresses from the first 'up' NIC containing a default gateway.
• a slight boost is given to IPv4 over IPv6 as before
• a low ranking is given to all loopback addresses
• a zero ranking is given to addresses from the range that gets used when DHCP does not provide an address, 169.254.*.

When used with IPAddress.Any as described above, the ranking is seen if I print out the result from GetIPAddresses(). In this case of course, GetListeningIps() just returns the one listener entry, for 0.0.0.0, but accepts connections on all interfaces (on Windows at least).

[BrandonPotter]

Apologies for the delay in getting this PR merged, my time has been a little more constrained than planned - added you as collaborator so you can see this through :)

[brudo]

Thanks Brendan. I see that you wanted to do some tests with existing apps before you merge this. Have you done those tests? Is it all OK to merge?

Sent with Outlookhttps://aka.ms/sdimjr for iPad

---

From: Brandon Potter notifications@github.com Sent: Saturday, July 22, 2017 9:30:58 AM To: BrandonPotter/SimpleTCP Cc: (William) Bruce Dodson; Author Subject: Re: [BrandonPotter/SimpleTCP] Test to determine network class in RankIpAddress is not comprehensive and could be removed (#14)

Apologies for the delay in getting this PR merged, my time has been a little more constrained than planned - added you as collaborator so you can see this through :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/BrandonPotter/SimpleTCP/issues/14#issuecomment-317194539, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AA1Qne9D7FzYTz4hRghM-vUpjQQGGZTrks5sQiPCgaJpZM4NhDQG.

[BrandonPotter]

Tested this on a machine with some undesirable NICs and 10.x is the network I care most about and got the following sorted list, which does indeed put what I care about on top:

10.0.133.156
fe80::55b5:d1e7:3e07:9b9f%4
192.168.137.1
192.168.72.1
fe80::41ae:b4fe:bf79:b055%9
fe80::8565:b7a8:91dc:f2a9%10
::1
127.0.0.1

So, that seems fine to me - the only thing I would do is put a try/catch around the code requesting info from the OS for network interfaces; we don't want the entire GetIPAddresses() method to fail if for some reason the sortation method fails on GetIPProperties or something. Maybe just return a low score if one of those calls fails?

[brudo]

PR#25 should close off the remaining to-do that I left unaddressed in PR#15. This is now in master.