Add support for custom IP networks

[PetrichorIT]

This PR adds support for custom IP networks instead of just 192.168.0.0/16 or fe80::/64.
Applications may behave differently, based on the bound IP addresses, especially with Ipv6 addresses.
fe80:: link-local-addresses may be treated differently than ffc0:: unicast-local or 200x:: unicast-global
in some applications.

Therefore the parameter IpVersion (renamed to IpNetwork) was extended to support custom subnets
as a parameter. Default cfg remains Ipv4 192.168.0.0./16.

Changes:

• IpVersion renamed to IpNetwork
• added enum variants V4(Ipv4Network) and V6(Ipv6Network)
• Builder::ip_version renamed to Builder::ip_network
• address iterator changed to reflect new parameters

Open Questions:

• If a node is created with a static address aka. not using the address iterator, should the address be checked, to guarantee, it's in the specified subnet? Or should all addresses be allowed?
• A globally available function that returns the nodes IpAddress may be useful, to retrieve the nodes identity, even deep in application code. Opinions?

[mcches]

I don't have free cycles this week to review in detail, but even with the great PR description I'm wondering why turmoil needs to support this? Most of the use cases I encounter actually prefer the string hostnames because it makes debugging a lot easier. Are you trying to test specific logic based on binds? If so, is turmoil the right place to do that?

[PetrichorIT]

I think using IpAddr instead of hostnames still is relevant in many systems, especially if the system has to create connection to e.g. config a new node added to a computation pool. In general, the simulated system defines the allowed addressing schemes for nodes, and many systems only support raw addressing. This addressing scheme also aids when simulating a dynamic range of nodes. Thus especially with Ipv6 the classes of addresses do matter, since real system will have to check against invalid addresses, so the simulation should not need changes to combat these mechanisms.

The current issue is, that while non fe80::/64 addresses can be assigned and used, they cannot have hostnames, which makes debugging harder. On a related note, having hostname-nodes poll their addresses from a fe80::/64 or 192.168.0.0/16 while nodes outside this subnet can still be created manually, seems inconsistent.

Its not so much about the nodes own identity, but rather the IP of incoming requests

[mcches]

Thanks for the detailed explanation. I will prioritize this PR early next week.

[mcches]

If a node is created with a static address aka. not using the address iterator, should the address be checked, to guarantee, it's in the specified subnet? Or should all addresses be allowed?

I think it needs to be checked.

A globally available function that returns the nodes IpAddress may be useful, to retrieve the nodes identity, even deep in application code. Opinions?

I'm not sure about deep in the application code. What would the non-turmoil equivalent be and how would you switch based on the environment the code is running in?

[PetrichorIT]

I'm not sure about deep in the application code. What would the non-turmoil equivalent be and how would you switch based on the environment the code is running in?

Non-turmoil based implementations would likely read the local ip addr from either a config file
or ifconfig. In general, multiple nodes may run the same software, thus having some kind of globally unique identifier
accessible everywhere would be nice.

[marcbowes]

This is a really useful feature addition, thank you.

I left a comment about the IpNetwork abstraction, which is more for @mcches than you. I think there going to be usecases where we need to mix v4 and v6 even without supporting routing between subnets.

[marcbowes]

I think this is a reasonable, pragmatic change, but it doesn't seem like this is what we really want to end up with. For example, what if I want to have a setup where a single machine has an admin process on it that listens with v4 and creates N processes (in response to API calls) with v6 addresses. I think this is the sort of setup you're aiming to support (is that right?), and so I think there is a design smell here.

I'm not suggesting you change this for this review (again, your change is pragmatic), but we should think about how this plays out and be mindful it's likely to cause maintenance problems.

[mcches]

This is a breaking change, requiring a version bump, so to echo what @marcbowes said... perhaps we can get closer to the desired state (at least at the public API) with this change to avoid subsequent breaking changes.

[PetrichorIT]

I think the core question is, whether turmoil should support mixed Ip traffic in its subnet.
If yes, we should add support for multiple ip address bindings on a node. However currently the "public"
ip is an unique identifier for each node, and that would not really work in a mixed ip scenario.

Since we most likely want to simulate a mixed Ipv4/Ipv6 subnet with independent address ranges,
each subnet part must be independently configurable. Something like

struct SubnetsCfg {
    v4: Ipv4Subnet,
    v6: Ipv6Subnet,
}

impl Builder {
    fn subnets(&mut self, cfg: SubnetsCfg) { ... }
}

would do the job, however this would not work with the current networking implementation.

In short, I think the current networking implementation and this desired goal are too different, to create an API now, that
will not break once we support multiple active subnets.

However breaking the API now, and that breaking it later a second time seems redundant, so should be try
to implement mixed ip subnets within the context of this PR?

[mcches]

However breaking the API now, and that breaking it later a second time seems redundant, so should be try to implement mixed ip subnets within the context of this PR?

It likely requires a full restructuring of top.rs that includes supporting loopback, v4, and v6 links. And then perhaps we still have a builder method for subnet(s).

I don't have a clear picture, but if you want to take a stab at a proposal that would be great.

See #79 (comment).

For faster discussion, we could also do this on the tokio discord server.

[PetrichorIT]

I have closed this PR, since the nessecary refactoring exceeds the scope of this PR.
See Issue #132 for further discussion