tokio-rs / turmoil

Add hardship to your tests
MIT License
764 stars 46 forks source link

Support multiple network interfaces #132

Open PetrichorIT opened 1 year ago

PetrichorIT commented 1 year ago

This refactor aims to introduce the ability of nodes to have multiple addresses in distinct subnets.

Immediate Goals

Challenges

I have tried to implement this, and come to the conlsuion that some major changes internally AND externally would be nessecary. Notably:

In my opinion, this amount of changes would exceed the scope of one PR, so it might be benifical to make step by step changes to the public API. However this warrants discussion.

Some related thoughts

As a reference, my current test implementation can be found here. I have closed the corresponding PR #125, since it is already out of date.

Progress

mcches commented 1 year ago

Linking #79 for context, and closing it to track in one place.

mcches commented 1 year ago

I think we should set the goal as: Support multiple network interfaces. And then a piece of that is supporting subnets.

mcches commented 1 year ago

Nodes, and thus Rt/Host can no longer by identified by a single IpAddr. The best possible solution would be to identify them by something like a MAC addr, but that would warrant major internal changes.

I think we should use a unique identifier for each host, which could be a uuid. This would be an internal concept and our search capabilities would take string or IpAddr to resolve.

It'd be good to decouple the DNS "lookup" function from the network manipulation ones. So lookup should always return one IpAddr or panic, but the others could resolve ids and and manipulate the network.

And then lastly, we need to change how hosts are added to the simulation to allow for different addressing schemes. Perhaps a builder pattern works best here:

sim.host()
    .with_ipv4(...)
    ...
    .build(<software>)

In terms of iterating on this we could create a new remote so that main is available for bug fixes.

PetrichorIT commented 1 year ago

It'd be good to decouple the DNS "lookup" function from the network manipulation ones. So lookup should always return one IpAddr or panic, but the others could resolve ids and and manipulate the network.

Since each node has two possible addresses, lookup should return a set of addresses.

I think we should use a unique identifier for each host, which could be a uuid. This would be an internal concept and our search capabilities would take string or IpAddr to resolve.

And then lastly, we need to change how hosts are added to the simulation to allow for different addressing schemes. Perhaps a builder pattern works best here:

As an idea, if create a builder pattern like this,

sim.host(<name>)
    ...
    .build(<software>)

we could ensure that each node has a human-readable name. Since these names should always be unique, we could them as a uuid. This would more expressive than a simple numeric uuid.

Additionally, we could change the API of the network manipulation functions like partition to take in a set of nodenames instead of impl ToIpAddrs, removing the need for that trait, and making the API overall simpler. This would also reduce the complexity of ToIpAddr to be only used in simple dns lookups or APIs that internally use dns lookups like TcpStream::connect.

Of course, reverse dns lookups would need to be publicly accessible, to indirectly allow network manipulation with IpAddrs, but that should not be a problem.

mcches commented 1 year ago

Since each node has two possible addresses, lookup should return a set of addresses.

Makes sense.

removing the need for that trait, and making the API overall simpler

We might still need a trait to support the regex use cases.

PetrichorIT commented 1 year ago

We might still need a trait to support the regex use cases.

Yeah, but we can completly detach this trait from dns resolutions

PetrichorIT commented 1 year ago

Basically using ToIpAddr for dns resultions, and something like ToNodeNames for network manipulation

PetrichorIT commented 1 year ago

I think a good first step would be the integration of uuids and the internal support of more complex dns queries. See PR #134.

On a related note, will we ever support async dns queries (see #69)? I don't really see the point, since all queries in turmoil are sync either way. Additionally async lookups would prevent use of lookup in non-async contexts, like fn main...