starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 03:05:51 +03:00

[GH-ISSUE #1863] Ability to construct DNS resolver with existing TcpStream/UdpSocket #795

New issue

Closed

opened 2026-03-16 00:16:01 +03:00 by kerem · 11 comments

kerem commented

2026-03-16 00:16:01 +03:00

Owner

Originally created by @XOR-op on GitHub (Dec 20, 2022).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1863

Is your feature request related to a problem? Please describe.
I was making a VPN-style proxy where trust-dns is used to resolve domain name in local machine. However, there is no such an API to operate on the underlying socket of resolvers. The similar issue was opened in #1335. The temporary solution #1586 suggested by @bluejekyll did not work in my case, where even if the client address is designated, the traffic still travels through the proxy in infinite loop.

Describe the solution you'd like
Add new arguments

pub tcp_stream: Option<std::net::TcpStream>,
pub udp_socket: Option<std::net::UdpSocket>,

to github.com/bluejekyll/trust-dns@422bbcf1fb/crates/resolver/src/config.rs (L392-L420)

Then for every implenetation in use of NameServerConfig, write corresponding code.

This solution should also fully address #1335.

Describe alternatives you've considered
I did not come up with great alternatives.

Originally created by @XOR-op on GitHub (Dec 20, 2022). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/1863 **Is your feature request related to a problem? Please describe.** I was making a VPN-style proxy where trust-dns is used to resolve domain name in local machine. However, there is no such an API to operate on the underlying socket of resolvers. The similar issue was opened in #1335. The temporary solution #1586 suggested by @bluejekyll did not work in my case, where even if the client address is designated, the traffic still travels through the proxy in infinite loop. **Describe the solution you'd like** Add new arguments ```rust pub tcp_stream: Option<std::net::TcpStream>, pub udp_socket: Option<std::net::UdpSocket>, ``` to https://github.com/bluejekyll/trust-dns/blob/422bbcf1fb222e39144adac971fe7fbcb8ca6489/crates/resolver/src/config.rs#L392-L420 Then for every implenetation in use of `NameServerConfig`, write corresponding code. This solution should also fully address #1335. **Describe alternatives you've considered** I did not come up with great alternatives.

kerem

2026-03-16 00:16:01 +03:00

closed this issue
added the
enhance
label

kerem commented

2026-03-16 00:16:17 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jan 2, 2023):

Sorry for the late response on this. I think one thing we could do is allow for generic lambdas to be passed in for creating the sockets. This would be similar to the ConnectionProvider. Maybe we could clean that up to instead of new_connection being the thing to implement, and instead just pass in NameServerConfig to methods for each socket type, like udp: Fn(&NameServerConfig) -> stu::net::UdpSocket and similar for TcpSocket. This might be simpler than the existing interfaces?

I didn't quite get the suggestion you had. We need to be able to create new sockets on a regular basis, so it's not enough to pass in a single socket at construction time.

@bluejekyll commented on GitHub (Jan 2, 2023): Sorry for the late response on this. I think one thing we could do is allow for generic lambdas to be passed in for creating the sockets. This would be similar to the ConnectionProvider. Maybe we could clean that up to instead of `new_connection` being the thing to implement, and instead just pass in NameServerConfig to methods for each socket type, like `udp: Fn(&NameServerConfig) -> stu::net::UdpSocket` and similar for TcpSocket. This might be simpler than the existing interfaces? I didn't quite get the suggestion you had. We need to be able to create new sockets on a regular basis, so it's not enough to pass in a single socket at construction time.

kerem commented

2026-03-16 00:16:22 +03:00

Author

Owner

@XOR-op commented on GitHub (Jan 3, 2023):

I agree that adopting generic lambdas is better way to solve this problem. Do you mean we have to create new interfaces to replace ConnectionProvider? I think we can add another implementation of ConnectionProvider, or even just to modify GenericConnectionProvider to achieve this.

The only usage of new_connection in current code is a private method Nameserver::connected_mut_client, meaning that users cannot call that. So we must set up those lambdas in advance, for example in constructors. Thus, the new_connection interface does not require a change. Even though adding another implementation sounds natural, current trust-dns-resolver code heavily relies on GenericConnectionProvider. So I prefer the latter solution.

My draft interface added to GenericConnectionProvider is the following:

pub fn new_with_callback(
            handle: R::Handle, 
            tcp_callback: Option<Fn(&NameServerConfig)->std::net::TcpSocket>, 
            udp_callback: Option<Fn(&NameServerConfig)->std::net::UdpSocket>
)

and we add corresponding constructors to AsyncResolver. For existing new_connection implementation, we could leave it as is when callbacks are None and rewrite others. Introducing this change does not break current interface as well as the mock test. What about your opinions and do I miss something important?

@XOR-op commented on GitHub (Jan 3, 2023): I agree that adopting generic lambdas is better way to solve this problem. Do you mean we have to create new interfaces to replace `ConnectionProvider`? I think we can add another implementation of `ConnectionProvider`, or even just to modify `GenericConnectionProvider` to achieve this. The only usage of `new_connection` in current code is a private method `Nameserver::connected_mut_client`, meaning that users cannot call that. So we must set up those lambdas in advance, for example in constructors. Thus, the `new_connection` interface does not require a change. Even though adding another implementation sounds natural, current trust-dns-resolver code heavily relies on `GenericConnectionProvider`. So I prefer the latter solution. My draft interface added to `GenericConnectionProvider` is the following: ```rust pub fn new_with_callback( handle: R::Handle, tcp_callback: Option<Fn(&NameServerConfig)->std::net::TcpSocket>, udp_callback: Option<Fn(&NameServerConfig)->std::net::UdpSocket> ) ``` and we add corresponding constructors to `AsyncResolver`. For existing `new_connection` implementation, we could leave it as is when callbacks are None and rewrite others. Introducing this change does not break current interface as well as the mock test. What about your opinions and do I miss something important?

kerem commented

2026-03-16 00:16:27 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jan 3, 2023):

I think a question I have is if the std::net sockets are always going to be the correct "base" interfaces to return in this cases. The RuntimeProvider trait has associated types that have specifics for the socket types used by that runtime, for example, the TokioRuntime references the tokio::net UdpSocket and TcpSocket. Both implement the TryFrom for the std::net. So we'd want to add a TryFrom<std::net::UdpSocket> and similar for the tcp type as well.

But this makes me wonder, do we have the necessary types but they are spread out too much and need to be consolidated? That is, we can probably get rid of the ConnectionProvider interface, and just use the RuntimeProvider. Right now, the tcp and udp features are behind types that are associated with that. What do you think? I'm wondering if we can simplify all of this, rather than adding more features that spread the surface area of all of this even more...

@bluejekyll commented on GitHub (Jan 3, 2023): I think a question I have is if the `std::net` sockets are always going to be the correct "base" interfaces to return in this cases. The `RuntimeProvider` trait has associated types that have specifics for the socket types used by that runtime, for example, the `TokioRuntime` references the `tokio::net` `UdpSocket` and `TcpSocket`. Both implement the TryFrom for the `std::net`. So we'd want to add a `TryFrom<std::net::UdpSocket>` and similar for the tcp type as well. But this makes me wonder, do we have the necessary types but they are spread out too much and need to be consolidated? That is, we can probably get rid of the `ConnectionProvider` interface, and just use the `RuntimeProvider`. Right now, the tcp and udp features are behind types that are associated with that. What do you think? I'm wondering if we can simplify all of this, rather than adding more features that spread the surface area of all of this even more...

kerem commented

2026-03-16 00:16:32 +03:00

Author

Owner

@XOR-op commented on GitHub (Jan 4, 2023):

For the first question, it looks like we can adopt the same as Udp: UdpSocket + Send and Tcp: Connect from RuntimeProvider, which would be greater than std::net in terms of generalization. I cannot have anything more general than those in my mind.

For the second question, I don't understand how we can only use RuntimeProvider. From my point of view, it's necessary to have an abstract "connection provider" to establish connections. However, it's worth discussing what "connection" the provider should offer. In my guess, you probably want to state that the ConnectionProvider interface too many details that end users are struggling implementing their own versions, but RuntimeProvider is easier to implement. Correct me if I misunderstand yours.

So based on my guess, here is my another draft of new interface designing:

pub trait RuntimeProvider: Clone + 'static {
    type Handle: Clone + Send + Spawn + Sync + Unpin;
    type Timer: Time + Send + Unpin;
    type Udp: UdpSocket + Send;
    type Tcp: Connect;
    fn new_tcp(&self, config: &NameServerConfig, options: &ResolverOpts)->io::Result<Self::Tcp>;
    fn new_udp(&self, config: &NameServerConfig, options: &ResolverOpts)->io::Result<Self::Udp>;
}

then we could get rid of the ConnectionProvider interface, putting remaining code to appropriate place. I want to hear from your opinion about the new design, or correct my misunderstanding.

@XOR-op commented on GitHub (Jan 4, 2023): For the first question, it looks like we can adopt the same as `Udp: UdpSocket + Send` and `Tcp: Connect` from `RuntimeProvider`, which would be greater than `std::net` in terms of generalization. I cannot have anything more general than those in my mind. For the second question, I don't understand how we can only use `RuntimeProvider`. From my point of view, it's necessary to have an abstract "connection provider" to establish connections. However, it's worth discussing what "connection" the provider should offer. In my guess, you probably want to state that the `ConnectionProvider` interface too many details that end users are struggling implementing their own versions, but `RuntimeProvider` is easier to implement. Correct me if I misunderstand yours. So based on my guess, here is my another draft of new interface designing: ```rust pub trait RuntimeProvider: Clone + 'static { type Handle: Clone + Send + Spawn + Sync + Unpin; type Timer: Time + Send + Unpin; type Udp: UdpSocket + Send; type Tcp: Connect; fn new_tcp(&self, config: &NameServerConfig, options: &ResolverOpts)->io::Result<Self::Tcp>; fn new_udp(&self, config: &NameServerConfig, options: &ResolverOpts)->io::Result<Self::Udp>; } ``` then we could get rid of the `ConnectionProvider` interface, putting remaining code to appropriate place. I want to hear from your opinion about the new design, or correct my misunderstanding.

kerem commented

2026-03-16 00:16:37 +03:00

Author

Owner

@bluejekyll commented on GitHub (Jan 5, 2023):

I think I like this last proposal. Would it allow you to achieve the goals you have?

@djc, do you have any input here? I'm good with the changes proposed to RuntimeProvider.

@bluejekyll commented on GitHub (Jan 5, 2023): I think I like this last proposal. Would it allow you to achieve the goals you have? @djc, do you have any input here? I'm good with the changes proposed to RuntimeProvider.

kerem commented

2026-03-16 00:16:42 +03:00

Author

Owner

@djc commented on GitHub (Jan 5, 2023):

Yes, adding methods for this to RuntimeProvider makes sense to me. I don't think new_tcp() and new_udp() are great names, but that's kind of a minor detail. I'd prefer connect() for TCP and bind() for UDP though perhaps that leans too much on the return type to disambiguate between the two protocols. So maybe connect_tcp() and bind_udp(), then? Anyway, would be good to have a PR for this!

@djc commented on GitHub (Jan 5, 2023): Yes, adding methods for this to `RuntimeProvider` makes sense to me. I don't think `new_tcp()` and `new_udp()` are great names, but that's kind of a minor detail. I'd prefer `connect()` for TCP and `bind()` for UDP though perhaps that leans too much on the return type to disambiguate between the two protocols. So maybe `connect_tcp()` and `bind_udp()`, then? Anyway, would be good to have a PR for this!

kerem commented

2026-03-16 00:16:48 +03:00

Author

Owner

@XOR-op commented on GitHub (Jan 6, 2023):

Well, let me give the new RuntimeProvider a try.

@XOR-op commented on GitHub (Jan 6, 2023): Well, let me give the new `RuntimeProvider` a try.

kerem commented

2026-03-16 00:16:53 +03:00

Author

Owner

@hottea773 commented on GitHub (Jan 9, 2023):

@XOR-op just wondering what your plans are here?
This is something I'd like to use (to fix https://github.com/bluejekyll/trust-dns/issues/1870) and am wondering whether I'm best to wait on this?

@hottea773 commented on GitHub (Jan 9, 2023): @XOR-op just wondering what your plans are here? This is something I'd like to use (to fix https://github.com/bluejekyll/trust-dns/issues/1870) and am wondering whether I'm best to wait on this?

kerem commented

2026-03-16 00:16:58 +03:00

Author

Owner

@XOR-op commented on GitHub (Jan 9, 2023):

@XOR-op just wondering what your plans are here? This is something I'd like to use (to fix #1870) and am wondering whether I'm best to wait on this?

Your need is the same as mine. I am implementing the new RuntimeProvider interface now, and I estimate that nearly half of the work has been done. However, I don't think the new interface will appear on the main stream in several weeks. If you can't wait, I suggest you adopt temporary workarounds including adding specific routing entry to dns server (as long as your interface does not go down e.g. affacted by DHCP) or letting traffic from resolver go through your VRF again and distinguish them by bindaddr.

@XOR-op commented on GitHub (Jan 9, 2023): > @XOR-op just wondering what your plans are here? This is something I'd like to use (to fix #1870) and am wondering whether I'm best to wait on this? Your need is the same as mine. I am implementing the new `RuntimeProvider` interface now, and I estimate that nearly half of the work has been done. However, I don't think the new interface will appear on the main stream in several weeks. If you can't wait, I suggest you adopt temporary workarounds including adding specific routing entry to dns server (as long as your interface does not go down e.g. affacted by DHCP) or letting traffic from resolver go through your VRF again and distinguish them by bindaddr.

kerem commented

2026-03-16 00:17:03 +03:00

Author

Owner

@djc commented on GitHub (Jan 9, 2023):

@XOR-op maybe start a draft PR already with what you've got so far? We're in the process of preparing the next API-incompatible release so it would probably be nice to include this there. If you share your work, others might be able to contribute.

@djc commented on GitHub (Jan 9, 2023): @XOR-op maybe start a draft PR already with what you've got so far? We're in the process of preparing the next API-incompatible release so it would probably be nice to include this there. If you share your work, others might be able to contribute.

kerem commented

2026-03-16 00:17:08 +03:00

Author

Owner

@djc commented on GitHub (Jan 11, 2023):

(The draft PR is now up in #1876 -- @hottea773 please coordinate if you want to get involved.)

@djc commented on GitHub (Jan 11, 2023): (The draft PR is now up in #1876 -- @hottea773 please coordinate if you want to get involved.)