Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reverse dns lookups on static binds #120

Merged
merged 4 commits into from
Jun 2, 2023

Conversation

PetrichorIT
Copy link
Contributor

Turmoil supports the creation of clients/hosts bound to any address that impl ToIpAddr.
This allows for then creation of nodes, bound to static IP addresses. Such nodes
will not appear in the internal mappings of Dns, since they have no hostname.

Dns::reverse however expects that all nodes (all inputs would be more correct) are contained
in the dns mapping. If this assumption is broken, Dns::reverse panics. This means that any use of
Dns::reverse in combination with a statically bound address will panic.

The problem: If a tracing subscriber is active, internal traces will attempt to use Dns::reverse once
when the node is initialized, thus crashing the simulation (world.rs:107).

Solutions

I See three solutions:

A (currently implemented)

If a reverse lookup does not find any entry in the dns mapping, it returns the input address as a String.
This would keep API changes small (&str to String). However all input addresses that have not appeared
in the mapping would be handled this way, not just statically bound addresses.

B

Add static binds to dns mapping. We could implement node creation in a way, that a dns entry
mapping the stringified static address to the real static address is created. This would not
change the Dns::reverse, but major changes in address resolution / node creation would be nessecary.

C

Change the API of Dns::reverse to reflect the failable nature of such lookups. By changing the return type to either
Option<&str> or Result<&str> we could deal with failing lookups without panicking. This would be the most expressiv
option, but it would change the public API of Dns::reverse and Sim::reverse_lookup_pair.

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Let me know what you think about the hybrid approach.

src/dns.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! Thanks for all the recent improvements.

tests/dns.rs Outdated Show resolved Hide resolved
@mcches mcches merged commit 78ca18d into tokio-rs:main Jun 2, 2023
@PetrichorIT PetrichorIT deleted the direct-address-binds branch July 27, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants