Supporting multiple network interfaces #134

PetrichorIT · 2023-08-10T10:00:27Z

This PR changes:

adds NodeIdentifiers to decouple node addresses and node identities
adds IpSubnets to model different subnets (subnets are not yet supported, see Support multiple network interfaces #132 for progress)
adds 1:n mappings between names and IpAddr in dns.rs
decouple dns registration from dns lookups (lookups of nonexistent names will no longer return a newly allocated address)
integrales reverse dns lookups into ToIpAddr trait

relates to #132

mcches · 2023-08-15T03:44:23Z

Apologies for the delay in review. I have this queued up for tomorrow morning (08/15).

mcches

Thanks for getting started on this.

mcches · 2023-08-15T17:22:31Z

src/dns.rs

+    fn to_ip_addr(&self, dns: &Dns) -> Option<IpAddr>;
+
+    #[doc(hidden)]
+    fn to_name(&self, dns: &Dns) -> String;


This doesn't seem to be in the right place given the name of and responsibilities of the trait.

This part is only temporarily necessary.

The problem is, that we need a name to create a dns mapping. However not all nodes have a name using the
current API since Sim::client expects a impl ToIpAddr. So to get a name for the dns registration,
this trait must temporarily be able to create such a name.

In the future all nodes should have explicit names, independent of their bound addresses, so this will only be temporary. Explicit names could be archived with the proposed builder API for node creation.

I see, so you want to avoid a breaking change in this first change? If so, can you comment that here saying we'll remove this.

I'm inclined to just break things and work in a remote branch if that allows for less churn.

src/dns.rs

mcches · 2023-08-15T17:31:51Z

src/ip.rs

+pub enum IpSubnetParsingError {
+    AddrParseError(AddrParseError),
+    IntParseError(ParseIntError),
+    InvalidSubnetSyntax,
+}


Are the variants necessary? Could we do something simpler like: https://doc.rust-lang.org/std/net/struct.AddrParseError.html ?

Since this parsing implementation will not be used internally and is just for convenience, it's not strictly necessary.
Whether we should stick to a more complex, but more expressiv API or to a simple easy API, idk

src/ip.rs

mcches · 2023-08-15T17:38:39Z

src/node.rs

+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub(crate) struct NodeIdentifer {
+    node_name: Arc<str>,


I'm wondering if having something like a uuid or a u64 would be better as an internal identifier. I can't think of any places that break with str right now. It seems from the PR title you were considering uuid:

Adding internal UUIDs...

The advantage of using str is the fact, that a NodeIdentifier not only acts as a unique identifier, but can also be used to create tracing spans. So for the time being, I think we should stick to strs.

You can have both, right? A uuid does all the network mapping and the host that is active in the sim loop still has a human readable hostname that is set on the span before each tick.

src/ip.rs

PetrichorIT · 2023-10-02T12:04:17Z

So in the spirit of breaking things, this PR fully supports multiple IP subnets, thus multiple bound addresses.

Changes

Nodes are identified by a NodeIdentifier, thus they always have a human-readable name
All nodes in the simulation are now connected through a list of subnets provided to Builder::ip_subnets
- The default subnets are (IPv4 192.168.0.0/16 + IPv6 fe80::/64)
- Each node is assigned an address in every subnet. Addresses can be specified with NodeBuilder::with_addr or are automatically assigned in the node creation process
Node creation can now be managed using the NodeBuilder API
- Sim::client and Sim::host are still supported, but may be removed
- Sim::client and Sim::host no longer support impl ToIpAddr as input, only impl AsRef<str> is supported, since every node needs are name (provided input is only used as a name, not longer as an addr)
DNS changes
- lookup will no longer be used to register nodes (consider it read-only from now on)
- Dns::register can be used to register new nodes in the DNS resolution
- since every node has a name, every node is represented in the DNS struct
- Reverse lookups now return a node identifier, instead of a string
- removed trait ToIpAddr, since all queries may return multiple IP address
- the public API was already using trait ToIpAddrs, so no changes there
- internally for_pairs was replaced by for_connected_pairs, since only IPs in the same subnet are connected
- trait ToSocketAddrs can now return multiple addrs
Topology
- Only IPs in the same subnet are connected in the topology
- endpoints keeps track of all addresses in one subnet, for easier node creation (maybe broadcast in the future?)
Host specific
- Hosts are now bound to multiple addrs
- net::tcp and net::udp bind / connect to appropriate addresses
removed IpVersion and Builder::ip_version
removed *_many APIs

Open questions

Builder API

The node builder API is somewhat cumbersome, when creating very simple nodes. Notably build_client or build_host
seems very verbose.

sim.node("my-simple-node").build_client(async { ... });

We could implement the API in such a way to allow these statements,

sim.client("my-simple-node").build(async { ... });
sim.host("my-simple-host").build(async { ... });

but that would use up the fn names Sim::client and Sim::host. Alternatively
we could keep the original Sim::client / Sim::host as simple aliases to the fully qualified form.

mcches · 2023-11-17T20:18:21Z

I've not gotten the time to review this, many applogies. Another issue just came up that can provide further motivation for prioritizing this work.

See #153

progwriter · 2024-01-22T16:57:57Z

Hi folks, any update here?

This looks like a super useful feature for modeling systems in networking, where multi-interfaces are a must. Would be great to see this available in turmoil!

mcches · 2024-01-22T17:05:34Z

Hi folks, any update here?

This looks like a super useful feature for modeling systems in networking, where multi-interfaces are a must. Would be great to see this available in turmoil!

Thanks for checking in. This is low priority at the moment. I've got a decent refactor underway for the net modules and after that is in I can pick up contributing to this or guiding the changes.

Can you please elaborate on your use cases/requirements so that we take them into consideration?

progwriter · 2024-01-23T01:26:30Z

Can you please elaborate on your use cases/requirements so that we take them into consideration?

At a high level, doing some testing of a network routing protocol. Since the protocol uses a mix of tcp + udp, was hoping to use turmoil to remove networking non-determinism and enable contained testing + fault injection.

Being able to treat routers as having multiple interfaces is pretty important (for link-local and loopback-to-loopback communication)

mcches · 2024-09-02T16:47:39Z

Closing due to inactivity and because we need to refactor some core things for this to work.

PetrichorIT added 5 commits July 27, 2023 13:52

introduced internal node uids

3c7da79

added ip subnet structs

d34170d

added internal multi ip support to dns.rs

5ed18a9

dns.rs fully with seperated registration mechanism

2ab7f7d

integrated reverse lookups into trait

a3ee96f

PetrichorIT mentioned this pull request Aug 10, 2023

Support multiple network interfaces #132

Open

8 tasks

mcches requested review from mcches and marcbowes August 14, 2023 16:46

mcches reviewed Aug 15, 2023

View reviewed changes

PetrichorIT added 16 commits August 24, 2023 09:40

added unit tests to ip.rs

67ed2b3

added builder API

394b566

added multi IP binds to config & internal structures

dad96b2

removed reverse lookups from ToIpAddr

cc957b3

added topological support for multi ip

2d93faf

removed IpVersionIter

bd1b096

added multi interface support to UDP

8eb3711

some tests

c970363

tcp support

24a3cc2

removed IpVersion

564b4f1

docs

1a6a8b2

removed ToIpAddr

8b96709

just renaming + clippy

e1f4403

fixed regex

0583c9b

fixed and reordered tests

9ddb1e8

clippy fixes

0ae8df4

PetrichorIT changed the title ~~Adding internal UUIDs and internal multi-interface dns mappings~~ Supporting multiple network interfaces Oct 2, 2023

cleanup debug statements

a1c6664

PetrichorIT marked this pull request as ready for review October 10, 2023 08:36

mcches closed this Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting multiple network interfaces #134

Supporting multiple network interfaces #134

PetrichorIT commented Aug 10, 2023

mcches commented Aug 15, 2023

mcches left a comment

mcches Aug 15, 2023

PetrichorIT Aug 16, 2023

mcches Aug 18, 2023

mcches Aug 15, 2023

PetrichorIT Aug 16, 2023

mcches Aug 15, 2023

PetrichorIT Aug 16, 2023

mcches Aug 18, 2023

PetrichorIT commented Oct 2, 2023

mcches commented Nov 17, 2023

progwriter commented Jan 22, 2024

mcches commented Jan 22, 2024

progwriter commented Jan 23, 2024

mcches commented Sep 2, 2024

Supporting multiple network interfaces #134

Supporting multiple network interfaces #134

Conversation

PetrichorIT commented Aug 10, 2023

mcches commented Aug 15, 2023

mcches left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PetrichorIT commented Oct 2, 2023

Changes

Open questions

Builder API

mcches commented Nov 17, 2023

progwriter commented Jan 22, 2024

mcches commented Jan 22, 2024

progwriter commented Jan 23, 2024

mcches commented Sep 2, 2024