transport: Allow changing DNS resolver config #384

ZettaScript · 2025-05-02T21:32:37Z

Fixes #383

I don't exactly know the implications of storing the resolver there but it looks reasonable to me (more than recreating it at each lookup).

Default config has the same behavior as before (Google servers), but now it can be changed.

I guess it's a semver-breaking change but unless using something like a static OnceCell I see no other way.

Also, please consider using (and enforcing) cargo clippy, it helps contributors cleaning their edits without having to scroll over preexisting warnings.

dmitry-markin

Hey @ZettaScript, IMO an option to use a system resolver makes sense and is also useful when local resolution should be different from what 8.8.8.8 provides.

The PR looks good, but should be extended to all the transports to not configure the resolver individually for TCP, WS, WebRTC, etc. The best place for config would be Litep2pConfig (and its ConfigBuilder), from where the resolver can be distributed to all the transports.

dmitry-markin · 2025-05-05T07:11:58Z

src/transport/common/listener.rs

-    pub async fn lookup_ip(self) -> Result<SocketAddr, DnsError> {
+    pub async fn lookup_ip(
+        self,
+        resolver: impl Borrow<TokioAsyncResolver>,


What is the motivation behind Borrow here? I don't think there are other types than TokioAsyncResolver that can be borrowed as such.

To be able to pass TokioAsyncResolver, or a reference, or an Arc. I often find this pattern convenient, but I can simply use a reference if it's not needed.

I think there is no need to use impl in the function definition here, as we know exactly what type we are using.

dmitry-markin · 2025-05-05T07:13:08Z

src/transport/tcp/config.rs

+
+    /// DNS resolver config.
+    pub resolver_config: hickory_resolver::config::ResolverConfig,
+
+    /// DNS resolver options.
+    pub resolver_opts: hickory_resolver::config::ResolverOpts,


Ideally this should be universal to all the transports, including WS, QUIC, etc.

I'm not sure how to do that. Should TransportBuilder::new accept a new argument for the resolver? or a new field in TransportHandle?

Sorry, missed this. Yes, TransportBuilder:new accepting a new argument is fine.

dmitry-markin · 2025-05-05T07:27:06Z

Thanks for reporting regarding clippy — seems the clippy CI step is not enforcing things.

lexnv · 2025-05-06T11:46:37Z

src/transport/tcp/config.rs

+    /// DNS resolver config.
+    pub resolver_config: hickory_resolver::config::ResolverConfig,
+
+    /// DNS resolver options.
+    pub resolver_opts: hickory_resolver::config::ResolverOpts,
 }


The alternative here would be to have a new dedicated option to read from the system config, similar to:

/// Defaults to false (ie current behavior). pub use_system_dns_config: bool,

The advantage is that we can change the hickory DNS resolver under the hood without breaking the API. We are getting close to the finish line and we should strive to keep the API as stable as possible.

Indeed I would also lean towards @dmitry-markin suggestion here, we should make this available on the main litep2p config.

One option to achive this: transform the pub use_system_dns_config into pub(crate) use_system_dns_config flag in the TCP config (ie users cannot modify it directly). Add the flag to the main litep2p config, and populate the tcp_config.use_system_dns_config from the litep2p_config.use_system_dns_config. We have a precedent for the identify configs to showcase this in more details

Now hickory types are private, only the flag is public.

If both Litep2pConfig and transport's config have the flag, it can be modified from different places and the outcome won't be obvious. For example, what if websocket_config.use_system_dns_config is false, tcp_config.use_system_dns_config is true and litep2p_config.use_system_dns_config is true?

Since transport's config must be added explicitly to the builder anyway (if I get it right), having a common flag seems mostly noise.

Also, if reading system's config fails, should it return a new Error variant? I don't want to silently fallback to default, because it's troublesome to debug for users.

dmitry-markin · 2025-05-06T12:47:06Z

Hey @ZettaScript, I have updated hickory-resolver in #386. Let's base the refactoring on the latest version.

Our CI step `Run clippy` did not treat warnings as errors. Therefore, the process terminated with exit code zero even when the clippy rules were not followed. This PR ensures the clippy rules are enforced by our CI. While at it, this PR fixes the clippy rules by `--fix` and manually fixes bits where needed. Since the code inside tokio::select and logs is not formatted by rustfmt, I've only paid attention to our production code. There might be a few places in our tests where these lines are long or not properly formatted. Detected by: #384 --------- Signed-off-by: Alexandru Vasile <[email protected]>

ZettaScript · 2025-06-21T14:07:15Z

The branch is rebased on master. I am still unsure about the duplicated use_system_dns_config which is unused.

dmitry-markin

Thank you for rebasing on master! One thing you could do in a similar situation is merge master. Force-pushing breaks the review history in an already opened PRs.

See the inline comment for dealing with duplicate use_system_dns_config and early failure.

dmitry-markin · 2025-06-23T09:51:47Z

src/transport/tcp/config.rs

@@ -80,6 +80,9 @@ pub struct Config {
    /// How long should litep2p wait for a substream to be opened before considering
    /// the substream rejected.
    pub substream_open_timeout: std::time::Duration,
+
+    /// Use system's DNS config.
+    pub use_system_dns_config: bool,


We can remove use_system_dns_config from transport configs completely. Ideally, we want to initialize the resolver early to fail the initialization of Litep2p object in case we can't read the system config. For this, we can initialize the resolver once in Litep2p::new (returning a new error type there) and pass it to every transport via TransportBuilder::new. The arguments of the latter will need to be extended with an explicit resolver argument to not clutter Config with a private field.

ZettaScript · 2025-06-29T08:07:26Z

Now the resolver is configured and initialized in Litep2p::new. However webrtc and quic don't seem to need a resolver. Should TransportBuilder::new take an Option of the resolver, or a generic type (which could be ())? or we don't care because the resolver is needed for tcp anyway and passing the unused arg is no-op?

src/error.rs

src/lib.rs

src/transport/common/listener.rs

src/transport/tcp/mod.rs

src/transport/websocket/mod.rs

dmitry-markin · 2025-06-30T08:18:53Z

Now the resolver is configured and initialized in Litep2p::new. However webrtc and quic don't seem to need a resolver. Should TransportBuilder::new take an Option of the resolver, or a generic type (which could be ())? or we don't care because the resolver is needed for tcp anyway and passing the unused arg is no-op?

Hey, looks good!

Passing the unused arg is fine.

WebRTC in litep2p only implements "server mode", so no resolver is needed. With QUIC, can you investigate, please, why the resolver is not used, so we don't end up in a situation where different transport resolvers work differently?

Co-authored-by: Dmitry Markin <[email protected]>

ZettaScript · 2025-06-30T17:19:01Z

Thanks!

WebRTC in litep2p only implements "server mode", so no resolver is needed. With QUIC, can you investigate, please, why the resolver is not used, so we don't end up in a situation where different transport resolvers work differently?

I'll investigate. (never worked with QUIC before)

ZettaScript · 2025-06-30T17:35:50Z

QuicListener::get_socket_address only accepts IPv4 or IPv6 for the input address: &Multiaddr. For comparison, TcpTransport::dial uses TcpAddress::multiaddr_to_socket_address which handles names as well.

The are a few issues about QUIC but none about QUIC+DNS.

dmitry-markin · 2025-07-02T15:07:05Z

QuicListener::get_socket_address only accepts IPv4 or IPv6 for the input address: &Multiaddr. For comparison, TcpTransport::dial uses TcpAddress::multiaddr_to_socket_address which handles names as well.

The are a few issues about QUIC but none about QUIC+DNS.

You are right, QUIC doesn't currently support dialing DNS addresses. I've created an issue for this: #406

No need to address anything in this PR.

dmitry-markin

Looks good, thank you! I will let @lexnv also review the PR, and after this we can merge it.

src/error.rs

src/transport/websocket/mod.rs

Co-authored-by: Dmitry Markin <[email protected]>

dmitry-markin reviewed May 5, 2025

View reviewed changes

lexnv reviewed May 6, 2025

View reviewed changes

lexnv mentioned this pull request May 8, 2025

ci: Enforce and apply clippy rules #388

Merged

ZettaScript added 3 commits June 21, 2025 16:00

transport: Allow changing DNS resolver config

f005b16

use boolean flag, websocket config, Litep2pConfig

5b32479

Fix after rebase

8dd7a38

ZettaScript force-pushed the dns-config branch from bd9a87b to 8dd7a38 Compare June 21, 2025 14:01

dmitry-markin reviewed Jun 23, 2025

View reviewed changes

Init resolver early

3980f4a

dmitry-markin reviewed Jun 30, 2025

View reviewed changes

ZettaScript and others added 2 commits June 30, 2025 19:01

Apply suggestions from code review

c09c062

Co-authored-by: Dmitry Markin <[email protected]>

clippy, fmt

5be3bd8

dmitry-markin approved these changes Jul 2, 2025

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

src/transport/websocket/mod.rs Outdated Show resolved Hide resolved

dmitry-markin requested a review from lexnv July 2, 2025 15:07

ZettaScript and others added 2 commits July 2, 2025 17:21

Apply suggestions from code review

0e63a53

Co-authored-by: Dmitry Markin <[email protected]>

add missing Arc

7af787c

transport: Allow changing DNS resolver config #384

Are you sure you want to change the base?

transport: Allow changing DNS resolver config #384

Conversation

ZettaScript commented May 2, 2025

Uh oh!

dmitry-markin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-markin commented May 5, 2025

Uh oh!

lexnv May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-markin commented May 6, 2025

Uh oh!

ZettaScript commented Jun 21, 2025

Uh oh!

dmitry-markin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZettaScript commented Jun 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmitry-markin commented Jun 30, 2025

Uh oh!

ZettaScript commented Jun 30, 2025

Uh oh!

ZettaScript commented Jun 30, 2025

Uh oh!

dmitry-markin commented Jul 2, 2025

Uh oh!

dmitry-markin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lexnv May 6, 2025 •

edited

Loading