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

Low level send method in resolver #2125

Open
andrewbaxter opened this issue Jan 6, 2024 · 8 comments
Open

Low level send method in resolver #2125

andrewbaxter opened this issue Jan 6, 2024 · 8 comments

Comments

@andrewbaxter
Copy link

andrewbaxter commented Jan 6, 2024

Is your feature request related to a problem? Please describe.
I'm implementing a dns server that handles some requests and forwards the rest on to another server. I was using the lower level client APIs to do this since I could forward the request parameters verbatim, as well as the response details.

I was looking into adding DoT today but it seems much more complex - specifically IIUC the hickory-client interface doesn't do any connection pooling.

As an alternative I started looking into using resolver directly, but it doesn't accept any advanced request parameters and the Lookup response similarly doesn't have any response details.

Describe the solution you'd like

I'm not sure if there's complex handling in Resolver that prevents this, but to expose a send method that takes a Message or something as a parameter and returns the full response (what lookup is doing under the hood, at some level).

Describe alternatives you've considered

  • Implement my own, somehow using NameServerPool but I wasn't sure how many other internals would have to come with that - its use in Resolver is fairly indirect and it seemed like it might not be appropriate for UDP upstream servers, so I'd be splitting more code paths, etc.
  • Create a new connection for each request, but I think this could have dire performance and fd usage consequences

Additional context
Similar: #1613

@djc
Copy link
Collaborator

djc commented Jan 8, 2024

Just exposing a send() method that can send any message on a Resolver or AsyncResolver feels like a bit of a layering violation? I feel like the resolver abstraction kind of requires that the message is actually a query, for example.

@bluejekyll
Copy link
Member

I have started some work in this area specifically for recursive resolution. I'd be curious what other changes you think would be necessary, here's an example of how that's being used, here: https://github.com/hickory-dns/hickory-dns/blob/main/crates/recursor/src/recursor_pool.rs#L98C27-L98C27

That's based on the GenericNameServerPool, which is really just a NameServerPool with a predefined GenericNameServer: https://github.com/hickory-dns/hickory-dns/blob/main/crates/resolver/src/name_server/name_server_pool.rs#L51

The NameServerPool itself does implement DnsHandle, which has a send method for a Request: https://github.com/hickory-dns/hickory-dns/blob/main/crates/resolver/src/name_server/name_server_pool.rs#L232

That will return a single DnsResponse... In terms of advanced parameters, we have this type that can be used to pass information to the underlying protocol, though I've never been very happy with it, https://github.com/hickory-dns/hickory-dns/blob/main/crates/proto/src/xfer/dns_request.rs#L17

DnsRequest is really just a Message with the DnsRequestOptions, https://github.com/hickory-dns/hickory-dns/blob/main/crates/proto/src/xfer/dns_request.rs#L50

So I think this already exists?

@andrewbaxter
Copy link
Author

So I think this already exists?

Oh I didn't realize NameServerPool implemented DnsHandle, and yeah that looks like maybe it's exactly what I want? So is that basically Resolver but without the high level interface?

I'd be interested in trying it out in any case. It looks like it's pub(crate) though ATM and I'm not sure how many other internals would need to be opened up to use it.

@andrewbaxter
Copy link
Author

And yeah, didn't mean to imply that I specifically wanted this in Resolver.

@bluejekyll
Copy link
Member

I’d have to review all the details here, but I think it primarily will not do secondary lookups, like CNAME chain resolution (if it’s not in the original response packet). So I wouldn’t call it the resolver, it’s really just a connection pool (with some extra logic to deal with UDP upgrades to TCP in certain cases).

Since I’m using it across crates, the recursor pool is marked crate only, but the NameServerPool which I think is what you’re interested in should be available publicly. NameServerPool could theoretically be moved into the proto crate I think, and then be used with the Client.

@djc
Copy link
Collaborator

djc commented Jan 9, 2024

Moving NameServerPool down into the proto crate seems like an interesting idea. 👍

@andrewbaxter
Copy link
Author

andrewbaxter commented Jan 9, 2024

Oh awesome, I'll try that out! And thanks for the detailed replies!

So it looks like idiomatic usage (forwarding a request) is basically

let mut name_servers = NameServerConfigGroup::new();
name_servers.push(NameServerConfig::new(SocketAddr::new(...), Protocol::Udp));
let pool = NameServerPool::from_config(
            name_servers,
            ResolverOpts::default(),
            GenericConnector::new(TokioRuntimeProvider::new()),
);
...
let resp = self1.upstream.send(DnsRequest::new({
    let mut m = Message::new();
    m.add_query({
        let mut q =
            Query::query(
                Name::from(request.query().name()),
                request.query().query_type(),
            );
        q.set_query_class(request.query().query_class());
        q
    });
    m
}, DnsRequestOptions::default())).next().await;
match resp {
    Some(Ok(r)) => ...,
    Some(Err(e)) => ...,
    None => ...
}

@andrewbaxter
Copy link
Author

andrewbaxter commented Jan 18, 2024

This is working for me! The only trick was empty responses get turned into errors, but I think transforming them back is pretty safe and it had all the info I needed.

Should I leave this open/rename it about moving the type?

Also if I may, I think the name NameServerPool is confusing led to me skipping over it. I assumed it was a pool that returned configured name server information or something. At the moment the docs just say "Abstract interface for mocking purpose".

I think ideally there'd be something saying "If you need a lower level interface for name resolution, you can use implementations of DnsHandle" and allow users to look for implementers, but unfortunately docs.rs doesn't have the ability to search for implementations (and it's harder cross-crate).

DnsHandle docs say "A trait for implementing high level functions of DNS" which suggests it's intended for internal use only by the wording. I think it's a very usable interface and promoting it would make me less nervous.

I'd be happy to make doc changes if you can suggest the best places to put the info.

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

No branches or pull requests

3 participants