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 #3169] Simplification of the Authority trait #1149

New issue

Open

opened 2026-03-16 01:43:56 +03:00 by kerem · 7 comments

kerem commented

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

Owner

Originally created by @divergentdave on GitHub (Jul 30, 2025).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/3169

The Authority trait has a lot of methods that serve overlapping purposes. It seems like Authority::lookup() is mainly used in tests and in specialty methods that look up particular record types in order to fill out the authorities section of authoritative responses. On the other hand, Authority::search() takes in the entire request, not just a query name and query type. I think we could significantly simplify the Authority trait if we refactored server code so that the Authority trait just takes in a request message and produces a response message, not an AuthLookup. This would entail the following changes.

Define a separate method for handling AXFR transfers. The AXFR sub-protocol is different enough from typical query-response DNS that I think it justifies use of a different method signature. For example, AXFR requires use of TCP, and AXFR responses use different compression rules (see RFC 5936 section 3.4). The rest of the server needs to use a different code path for AXFR, so removing support for AXFR from Authority::search() seems like a win-win. This would let us remove the AuthLookup::AXFR variant as well.
Change Authority methods to return response messages instead of Lookup, and fold build_authoritative_response() and build_forwarded_response() into their respective authorities. This will also enable further simplifications and bug fixes by removing the intermediate Lookup or AuthLookup.
Remove Authority::ns(), Authority::soa(), and Authority::soa_secure(), which are all used only in build_authoritative_response() or AXFR code.
Remove Authority::lookup(), and rewrite tests to construct mock requests to pass to Authority::search().
Similarly remove unnecessary DNSSEC-related methods on Authority, as appropriate.

Originally created by @divergentdave on GitHub (Jul 30, 2025). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/3169 The Authority trait has a lot of methods that serve overlapping purposes. It seems like `Authority::lookup()` is mainly used in tests and in specialty methods that look up particular record types in order to fill out the authorities section of authoritative responses. On the other hand, `Authority::search()` takes in the entire request, not just a query name and query type. I think we could significantly simplify the Authority trait if we refactored server code so that the `Authority` trait just takes in a request message and produces a response message, not an `AuthLookup`. This would entail the following changes. * Define a separate method for handling AXFR transfers. The AXFR sub-protocol is different enough from typical query-response DNS that I think it justifies use of a different method signature. For example, AXFR requires use of TCP, and AXFR responses use different compression rules (see RFC 5936 section 3.4). The rest of the server needs to use a different code path for AXFR, so removing support for AXFR from `Authority::search()` seems like a win-win. This would let us remove the `AuthLookup::AXFR` variant as well. * Change `Authority` methods to return response messages instead of `Lookup`, and fold `build_authoritative_response()` and `build_forwarded_response()` into their respective authorities. This will also enable further simplifications and bug fixes by removing the intermediate `Lookup` or `AuthLookup`. * Remove `Authority::ns()`, `Authority::soa()`, and `Authority::soa_secure()`, which are all used only in `build_authoritative_response()` or AXFR code. * Remove `Authority::lookup()`, and rewrite tests to construct mock requests to pass to `Authority::search()`. * Similarly remove unnecessary DNSSEC-related methods on `Authority`, as appropriate. See also https://github.com/hickory-dns/hickory-dns/pull/3164#discussion_r2243244017

kerem commented

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

Author

Owner

@djc commented on GitHub (Jul 30, 2025):

I think this is a good direction. I'm a little wary of letting the trait methods return a "bigger" value than they do currently because for one thing, conceptually an "authority" should be more like just the storage and less of the surrounding logic -- but maybe that just means we need multiple traits which can be layered.

@djc commented on GitHub (Jul 30, 2025): I think this is a good direction. I'm a little wary of letting the trait methods return a "bigger" value than they do currently because for one thing, conceptually an "authority" should be more like just the storage and less of the surrounding logic -- but maybe that just means we need multiple traits which can be layered.

kerem commented

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

Author

Owner

@divergentdave commented on GitHub (Jul 30, 2025):

Yeah, I think a separate trait for just different authoritative server storage backends would make sense.

@divergentdave commented on GitHub (Jul 30, 2025): Yeah, I think a separate trait for just different authoritative server storage backends would make sense.

kerem commented

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

Author

Owner

@divergentdave commented on GitHub (Aug 5, 2025):

I noticed that the server AXFR implementation is currently limited to a single response message. See #2049 and #351. I tried adding more records to the existing AXFR integration tests, and they eventually produced a response with TC=1. The changes above will likely make it easier to fix this in the future, by creating separate AXFR and query-response code paths.

@divergentdave commented on GitHub (Aug 5, 2025): I noticed that the server AXFR implementation is currently limited to a single response message. See #2049 and #351. I tried adding more records to the existing AXFR integration tests, and they eventually produced a response with `TC=1`. The changes above will likely make it easier to fix this in the future, by creating separate AXFR and query-response code paths.

kerem commented

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

Author

Owner

@divergentdave commented on GitHub (Aug 26, 2025):

The name "Authority" is misleading as well, what do folks think about renaming the trait to "ZoneHandler"?

@divergentdave commented on GitHub (Aug 26, 2025): The name "Authority" is misleading as well, what do folks think about renaming the trait to "ZoneHandler"?

kerem commented

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

Author

Owner

@djc commented on GitHub (Aug 26, 2025):

The name "Authority" is misleading as well, what do folks think about renaming the trait to "ZoneHandler"?

Sounds like an improvement to me!

@djc commented on GitHub (Aug 26, 2025): > The name "Authority" is misleading as well, what do folks think about renaming the trait to "ZoneHandler"? Sounds like an improvement to me!

kerem commented

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

Author

Owner

@cpu commented on GitHub (Aug 26, 2025):

I like it too 👍

@cpu commented on GitHub (Aug 26, 2025): I like it too 👍

kerem commented

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

Author

Owner

@bluejekyll commented on GitHub (Aug 30, 2025):

I like all the discussion here. I know I'm late to the conversation. For what it's worth, if it wasn't obvious, the name Authority comes directly from the RFC for the Zone Authority. I really like that you all are figuring out ways to simplify this. I know I ended up with a lot of awkward abstractions to deal with various needs.

@bluejekyll commented on GitHub (Aug 30, 2025): I like all the discussion here. I know I'm late to the conversation. For what it's worth, if it wasn't obvious, the name Authority comes directly from the RFC for the Zone Authority. I really like that you all are figuring out ways to simplify this. I know I ended up with a lot of awkward abstractions to deal with various needs.