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

A Trust DNS based Recursor #1710

Merged
merged 23 commits into from
Aug 24, 2022
Merged

A Trust DNS based Recursor #1710

merged 23 commits into from
Aug 24, 2022

Conversation

bluejekyll
Copy link
Member

@bluejekyll bluejekyll commented May 22, 2022

This is pretty rough right now, probably a lot of work to do, but it does work for some basic queries, for example:

cargo run --all-features --bin recurse -- --debug -n 192.5.5.241:53 -t A www.example.com.

need some cleanup on output, but this is the current result:

Recursing for www.example.com. A from hints
Success for query Lookup { query: Query { name: Name("www.example.com."), query_type: A, query_class: IN, mdns_unicast_response: false }, records: [Record { name_labels: Name("www.example.com."), rr_type: A, dns_class: IN, ttl: 86400, rdata: Some(A(93.184.216.34)), mdns_cache_flush: false }], valid_until: Instant { t: 808620694303584 } }
        www.example.com. 86400 IN A 93.184.216.34

I don't think this is ready for review yet, lots to cleanup.

fixes: #1440

@bluejekyll
Copy link
Member Author

This now wired up end to end, using this branch to checkout, and run from the root of the project directory, the recursor can be tested with:

cargo run --all-features --bin trust-dns -- -p 8853 --zonedir tests/test-data/named_test_configs --config tests/test-data/named_test_configs/example_recursor.toml

And then I used the resolve command to test with:

resolve -n 127.0.0.1:8853 www.example.com

The first call timed out as the recursion took longer than the timeout in the resolve command, but the second worked as it was cached at that point:

Querying for www.example.com A from tcp:127.0.0.1:8853, udp:127.0.0.1:8853
Success for query name: www.example.com type: A class: IN
	www.example.com. 86400 IN A 93.184.216.34

@bluejekyll bluejekyll marked this pull request as ready for review May 26, 2022 19:35
@bluejekyll bluejekyll force-pushed the recursor branch 2 times, most recently from 7a2eee9 to 9c99458 Compare June 5, 2022 14:26
@bluejekyll
Copy link
Member Author

Sorry for the delay in landing this. I want to try and build some tests to help verify this code before we land it. Those are taking me a bit of time to get in place.

@bluejekyll
Copy link
Member Author

@djc, I'd like to land this so that we can do a release. I've marked it as experimental since we have very little test coverage on the new recursor crate at the moment. I think the most controversial thing here is that we're exposing a few more things from the resolver to reuse for this, but those are very useful here.

Once this is passing I think it's ready to go.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

I've skimmed this and it mostly seems reasonable.

use crate::{recursor_pool::RecursorPool, Error, ErrorKind};

/// Set of nameservers by the zone name
type NameServerCache = LruCache<Name, RecursorPool>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: I've kind of soured on type aliases; in my experience they make the code harder to understand (because of more opaque types) at not much benefit (no isolation of the inner type).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure on this one. I agree in principle, the issue is that the type becomes a bit unwieldy with all the generics, and that can cause a readability problem as well.

crates/recursor/src/recursor.rs Outdated Show resolved Hide resolved
crates/recursor/src/recursor.rs Outdated Show resolved Hide resolved
crates/recursor/src/recursor.rs Outdated Show resolved Hide resolved
crates/recursor/src/recursor.rs Outdated Show resolved Hide resolved
@@ -133,12 +132,10 @@ pub struct DnsResponse(Message);
// TODO: when `impl Trait` lands in stable, remove this, and expose FlatMap over answers, et al.
impl DnsResponse {
/// Retrieves the SOA from the response. This will only exist if it was an authoritative response.
pub fn soa(&self) -> Option<SOA> {
pub fn soa(&self) -> Option<&Record> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this? I can see that it's nice to avoid the clone, but it seems like returning &SOA would make life easier for downstream users? What do we need out of the Record in callers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I needed all the fields of Record. I was thinking about introducing a new TypedRecord... not sure if nows a good time for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@djc, I started working on resolving the issue here. I think it's going to take a new type, that I'll plan to call TypedRecord. This will take the form of TypedRecord<R: RecordData> which will also introduce a new trait RecordData that all the RData types will implement. To facilitate the reference form, I think we might also need a TypedRecordRef<'_, R>, which would carry a reference to the interior RecordData and Name.

So I'd like to merge this as is, and then follow up with a PR to introduce that type, which may be a bigger refactor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense! Is the idea that we're getting rid of the RData enum in favor of a trait-based (that is, open) API, or will we keep both?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we'd keep both? We need something generic in the Record, and I like the known sizes of all the types. Otherwise we'd need to box the types I think... the enum is generic across them all, so I was thinking TypedRecord<RData> would be the generic version. You can convince me otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this is the best place for this discussion but would like to understand what problem TypedRecord and/or TypedRecordRef are supposed to solve.

@bluejekyll bluejekyll force-pushed the recursor branch 3 times, most recently from b97e9a7 to 53faeb3 Compare August 24, 2022 14:31
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.

Implement a recursive resolver
2 participants