Skip to content

CNAME resolution support for the recursor.#2339

Merged
marcus0x62 merged 4 commits into
hickory-dns:mainfrom
marcus0x62:recursor_cname
Sep 14, 2024
Merged

CNAME resolution support for the recursor.#2339
marcus0x62 merged 4 commits into
hickory-dns:mainfrom
marcus0x62:recursor_cname

Conversation

@marcus0x62
Copy link
Copy Markdown
Collaborator

@marcus0x62 marcus0x62 commented Aug 4, 2024

This PR adds support for CNAME resolution to the recursor, along with a set of related changes:

  1. A new extend_records method in crates/resolver/src/lookup.rs to facilitate appending the resolved CNAMES to the original Lookup result.
  2. A change to the DNS LRU insert_records function to use the original query type when inserting a CNAME record, to facilitate later retrieval of the record.
  3. Core CNAME lookup logic for the recursor and a configurable recursion depth limit (default: 12 nested lookups) to prevent lookup loops
  4. CNAME-related tests for single CNAME queries, CNAME chain queries, and the recursion depth limit.

@justahero
Copy link
Copy Markdown

The e2e test suite fails to compile, for example Record::cname or Record::CNAME are not defined. The Record is the one defined in the dns-test package.

Maybe you missed something to commit?

Comment thread tests/test-data/test_configs/example_recursor.toml Outdated
@marcus0x62
Copy link
Copy Markdown
Collaborator Author

The e2e test suite fails to compile, for example Record::cname or Record::CNAME are not defined. The Record is the one defined in the dns-test package.

Maybe you missed something to commit?

No, there is a separate PR open to add the CNAME record. See PR #2338.

@marcus0x62 marcus0x62 force-pushed the recursor_cname branch 5 times, most recently from 0360c63 to 2e43a49 Compare September 9, 2024 21:32
@marcus0x62 marcus0x62 marked this pull request as ready for review September 9, 2024 22:08
@bluejekyll
Copy link
Copy Markdown
Member

Is it possible for us to reuse the logic here in the caching_client for this? There's a lot of annoying details with CNAME recursion and other record types that also need to be resolved like CNAME.

https://github.com/hickory-dns/hickory-dns/blob/main/crates/resolver/src/caching_client.rs#L336-L472

@marcus0x62
Copy link
Copy Markdown
Collaborator Author

@bluejekyll I've been looking into this today, and while I need to do some more research, the first challenge I see is I don't think the record caching behavior (bundling the entire cname chain as one cache entry with the TTL set to the smallest TTL from the chain) is going to be optimal for the recursor, where having the records from the chain cached individually and according to the individual record TTL would be valuable/more efficient. As one example, a lot of CDN intermediate records are generic and will be used across multiple domains.

Secondly, and while it isn't necessarily a "problem" for the recursor (in terms of not working,) the caching client CNAME implementation seems to be built around the idea that CNAME chains will be returned more or less complete and that encountering a partial CNAME chain is the exception to be handled at the end , whereas the recursor implementation is the opposite - it expects to not get the full chain and early in its loop kicks off a search for missing links.

@marcus0x62 marcus0x62 force-pushed the recursor_cname branch 2 times, most recently from 9bab1a3 to f7ef5cd Compare September 10, 2024 22:55
Comment thread crates/resolver/src/dns_lru.rs Outdated
Comment thread crates/recursor/src/recursor.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread tests/e2e-tests/src/recursor/cname/scenarios.rs Outdated
@djc djc mentioned this pull request Sep 11, 2024
Copy link
Copy Markdown
Member

@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.

LGTM overall, some small suggestions.

@marcus0x62 marcus0x62 force-pushed the recursor_cname branch 3 times, most recently from a6efdcc to e3e99ad Compare September 11, 2024 16:41
Comment thread crates/recursor/src/recursor.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread crates/recursor/src/recursor_dns_handle.rs Outdated
Comment thread tests/e2e-tests/src/recursor/cname/scenarios.rs Outdated
@bluejekyll
Copy link
Copy Markdown
Member

bluejekyll commented Sep 12, 2024

the caching client CNAME implementation seems to be built around the idea that CNAME chains will be returned more or less complete and that encountering a partial CNAME chain is the exception to be handled at the end

I wonder if we can create a method for doing this that would be reusable? The CachingClient implementation will issue additional requests if the chain is incomplete, fwiw. What I'm worried about is that we're going to have to maintain two different areas of code that will have nuances around CNAME, HTTPS, SVCB, etc, records. I'd love it if we could have that logic abstracted. If not part of this PR, maybe something in the future.

@marcus0x62
Copy link
Copy Markdown
Collaborator Author

I wonder if we can create a method for doing this that would be reusable? The CachingClient implementation will issue additional requests if the chain is incomplete, fwiw. What I'm worried about is that we're going to have to maintain two different areas of code that will have nuances around CNAME, HTTPS, SVCB, etc, records. I'd love it if we could have that logic abstracted. If not part of this PR, maybe something in the future.

I think that's doable, but I'd like to tackle it as a separate effort. Another thing that probably makes sense to unify is the DNSSEC post-processing we're doing in both places. The differences in caching behavior can probably be eliminated - it doesn't seem critical for the caching client to store the entire CNAME chain as a single record. The other big difference is the working copy of the response records at the time of CNAME resolution is in a DnsResponse on the caching client side and a LookupResult on the recursor side. I think that could be abstracted, or the CNAME resolution on the recursor side could be moved into lookup instead of resolve...

@marcus0x62 marcus0x62 marked this pull request as draft September 12, 2024 19:36
@marcus0x62 marcus0x62 marked this pull request as ready for review September 12, 2024 19:41
@marcus0x62 marcus0x62 force-pushed the recursor_cname branch 3 times, most recently from 98234dd to 41742aa Compare September 12, 2024 22:41
@djc djc enabled auto-merge September 13, 2024 07:34
@djc djc added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 13, 2024
@marcus0x62 marcus0x62 added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 13, 2024
@marcus0x62 marcus0x62 added this pull request to the merge queue Sep 14, 2024
Merged via the queue into hickory-dns:main with commit e0b24aa Sep 14, 2024
@marcus0x62 marcus0x62 deleted the recursor_cname branch September 14, 2024 13:56
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.

4 participants