[GH-ISSUE #2570] Separate high level errors and referrals out of ProtoErrorKind #1018

New issue

Open

opened 2026-03-16 01:17:38 +03:00 by kerem · 2 comments

kerem commented

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

Owner

Originally created by @divergentdave on GitHub (Nov 12, 2024).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2570

Basically NoError responses should not result in a ProtoError IMO -- that stuff seems confusing. Instead, we should have some kind of enum representing response modalities ... So that resolution would yield Result<ResolveResponse<RData>, ResolveError> (naming TBD).

Originally posted by @djc in https://github.com/hickory-dns/hickory-dns/issues/2546#issuecomment-2456693095

I agree that we should remove some of these variants from ProtoErrorKind, and put them in a different enum. Specifically, Nsec, NoError, NoRecordsFound, should be removed, while transport-level errors and parsing errors should stay. Likewise, the Forward and ForwardNS variants should be removed from hickory_recursor::ErrorKind. Propagation of negative responses and referrals has been bug-prone because of how high-level and low-level concerns are combined in these error types. The immediate cause of many such bugs has been a failure to match on specific error kind variants in various places. If ProtoError only contains transport errors and the like, then it will be safe to just propagate it with ?.

In the case of the recursor, recursor errors have to be transformed into ProtoErrors before being passed to the DNSSEC validator, due to the use of the DnsHandle trait, which requires complicated conversions between variants of different error types. If we had one common enum to represent presence of records, absence of records, absence of domain, or a referral, then we could reuse this throughout different crates, and crate-wide error representations would not need to be involved.

The DnsLru cache would benefit from such a change as well, since it currently uses a Result<Lookup, ProtoError>, but only ever populates the error case with NoRecordsFound errors.

Originally created by @divergentdave on GitHub (Nov 12, 2024). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/2570 > Basically `NoError` responses should not result in a `ProtoError` IMO -- that stuff seems confusing. Instead, we should have some kind of enum representing response modalities ... So that resolution would yield `Result<ResolveResponse<RData>, ResolveError>` (naming TBD). _Originally posted by @djc in https://github.com/hickory-dns/hickory-dns/issues/2546#issuecomment-2456693095_ I agree that we should remove some of these variants from `ProtoErrorKind`, and put them in a different enum. Specifically, `Nsec`, `NoError`, `NoRecordsFound`, should be removed, while transport-level errors and parsing errors should stay. Likewise, the `Forward` and `ForwardNS` variants should be removed from `hickory_recursor::ErrorKind`. Propagation of negative responses and referrals has been bug-prone because of how high-level and low-level concerns are combined in these error types. The immediate cause of many such bugs has been a failure to match on specific error kind variants in various places. If `ProtoError` only contains transport errors and the like, then it will be safe to just propagate it with `?`. In the case of the recursor, recursor errors have to be transformed into `ProtoError`s before being passed to the DNSSEC validator, due to the use of the `DnsHandle` trait, which requires complicated conversions between variants of different error types. If we had one common enum to represent presence of records, absence of records, absence of domain, or a referral, then we could reuse this throughout different crates, and crate-wide error representations would not need to be involved. The DnsLru cache would benefit from such a change as well, since it currently uses a `Result<Lookup, ProtoError>`, but only ever populates the error case with `NoRecordsFound` errors.

kerem added the

cleanup

label

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

kerem commented

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

Author

Owner

@divergentdave commented on GitHub (Nov 12, 2024):

Here's a related blog post: https://sled.rs/errors.html

@divergentdave commented on GitHub (Nov 12, 2024): Here's a related blog post: https://sled.rs/errors.html

kerem commented

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

Author

Owner

@djc commented on GitHub (Nov 13, 2024):

In the case of the recursor, recursor errors have to be transformed into ProtoErrors before being passed to the DNSSEC validator, due to the use of the DnsHandle trait, which requires complicated conversions between variants of different error types. If we had one common enum to represent presence of records, absence of records, absence of domain, or a referral, then we could reuse this throughout different crates, and crate-wide error representations would not need to be involved.

Yes, I think we should maybe stop using the DnsHandle trait in all the places or make it more flexible? Started pulling on that thread on a branch but there's a lot of tight coupling and a bunch of low-value abstractions that make stuff harder.

https://github.com/hickory-dns/hickory-dns/compare/handle-error?expand=1

@djc commented on GitHub (Nov 13, 2024): > In the case of the recursor, recursor errors have to be transformed into `ProtoError`s before being passed to the DNSSEC validator, due to the use of the `DnsHandle` trait, which requires complicated conversions between variants of different error types. If we had one common enum to represent presence of records, absence of records, absence of domain, or a referral, then we could reuse this throughout different crates, and crate-wide error representations would not need to be involved. Yes, I think we should maybe stop using the `DnsHandle` trait in all the places or make it more flexible? Started pulling on that thread on a branch but there's a lot of tight coupling and a bunch of low-value abstractions that make stuff harder. https://github.com/hickory-dns/hickory-dns/compare/handle-error?expand=1

kerem referenced this issue

2026-03-16 02:52:04 +03:00

[PR #1018] [MERGED] Bump webpki-roots from 0.18.0 to 0.19.0 #1875