Skip to content

Chained authority implementation#2161

Merged
marcus0x62 merged 3 commits intohickory-dns:mainfrom
marcus0x62:chained_authority
Sep 27, 2024
Merged

Chained authority implementation#2161
marcus0x62 merged 3 commits intohickory-dns:mainfrom
marcus0x62:chained_authority

Conversation

@marcus0x62
Copy link
Collaborator

@marcus0x62 marcus0x62 commented Feb 29, 2024 •
edited
Loading

This is a standalone PR for the chained authority feature discussed in PR #2113

It implements the cases discussed earlier, including the consult case where an authority can modify an answer -- positive or negative -- from a previous authority, except when LookupControlFlow::Break is returned, which will result in an immediate response being sent.

While working on this, I found that the LookupControlFlow logic in the catalog was spread out and duplicated across more of the code base than it really needed to be, so there's a separate commit here to clean that up. Hopefully it makes reviewing the core changes a bit easier.

closes: #2113

@bluejekyll
Copy link
Member

bluejekyll commented Mar 2, 2024

Can we close #2113 in favor of this?

@bluejekyll
Copy link
Member

bluejekyll commented Mar 16, 2024

let's land #2160 before reviewing this, as the changes here look quite large right now.

@marcus0x62 marcus0x62 marked this pull request as draft August 24, 2024 12:27
@marcus0x62 marcus0x62 force-pushed the chained_authority branch 6 times, most recently from 33c03e0 to b06c336 Compare September 15, 2024 17:04
@marcus0x62 marcus0x62 marked this pull request as ready for review September 15, 2024 17:05
@djc
Copy link
Member

djc commented Sep 16, 2024

While working on this, I found that the LookupControlFlow logic in the catalog was spread out and duplicated across more of the code base than it really needed to be, so there's a separate commit here to clean that up. Hopefully it makes reviewing the core changes a bit easier.

Maybe put that in a separate PR? This PR has a lot going on and IMO the commit history isn't that clean...

djc
djc reviewed Sep 16, 2024
View reviewed changes
Copy link
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.

Some initial feedback. Still processing the structural changes here...

@marcus0x62
Copy link
Collaborator Author

marcus0x62 commented Sep 16, 2024

While working on this, I found that the LookupControlFlow logic in the catalog was spread out and duplicated across more of the code base than it really needed to be, so there's a separate commit here to clean that up. Hopefully it makes reviewing the core changes a bit easier.

Maybe put that in a separate PR? This PR has a lot going on and IMO the commit history isn't that clean...

While working on this, I found that the LookupControlFlow logic in the catalog was spread out and duplicated across more of the code base than it really needed to be, so there's a separate commit here to clean that up. Hopefully it makes reviewing the core changes a bit easier.

Maybe put that in a separate PR? This PR has a lot going on and IMO the commit history isn't that clean...

I can move these changes into a separate PR, but it will probably make sense to do that first then come back to this PR. I think the chained authority logic will be much easier to review (functionally, at least,) with the catalog refactoring in place. The parts of the catalog that were changed relate directly to where the chained authority logic is inserted, and the original has had what looks like quite a bit of drift over time - functions that no longer do what their names imply, message building and sending split up in an inefficient way, etc.

@marcus0x62 marcus0x62 marked this pull request as draft September 16, 2024 22:01
@marcus0x62 marcus0x62 marked this pull request as ready for review September 19, 2024 16:00
@marcus0x62 marcus0x62 force-pushed the chained_authority branch 2 times, most recently from d0d9767 to c8ece53 Compare September 20, 2024 21:10
@djc djc mentioned this pull request Sep 21, 2024
Merged
djc
djc reviewed Sep 21, 2024
View reviewed changes
@djc djc mentioned this pull request Sep 25, 2024
Closed
2 tasks
@marcus0x62
Server and catalog changes to support chained authorities
6cf8ac9 
* Support single/chained StoreConfig variants, normalized internally to a vec
* Catalog changes to store authorities in a vec
* Changes to existing catalog tests
djc
djc approved these changes Sep 27, 2024
View reviewed changes
Copy link
Member

@djc djc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

(To be explicit: once approved feel free to merge once the comments addressed -- of course I'm always happy to do another round of feedback if you prefer.)

@marcus0x62 marcus0x62 added this pull request to the merge queue Sep 27, 2024
Merged via the queue into hickory-dns:main with commit 26598f3 Sep 27, 2024
@marcus0x62 marcus0x62 deleted the chained_authority branch September 27, 2024 22:13
marcus0x62 added a commit to marcus0x62/hickory-dns that referenced this pull request Oct 7, 2024
@marcus0x62
Use custom serde visitor to fix store error messages
049f8a5 
Background
----------

This change introduces a custom serde visitor to handle single-store and chained-store
variants while still producing useful error messages.  The
[chained authority PR](hickory-dns#2161) used the
serde [untagged enum representation](https://serde.rs/enum-representations.html#untagged)
in order to represent single-store (TOML table, serde map) or chained-store
(TOML array-of-tables, serde sequence) variants.  Unfortunately, serde is not able to
show useful error messages when a syntax error is encountered and untagged enums are being
used.  For example, misspelling the "type" option in a zone store configuration results
in this error message to the user:

```
1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum StoreConfigContainer
```

By using a minimal custom visitor, we can restore the previous, more useful error messages:

```
*Single Store*
1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml"
Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [zones.stores]
   | ^^^^^^^^^^^^^^
missing field `type`
```

```
*Chained Stores*
1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1
   |
47 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
missing field `type`
```

A side effect of this change is that StoreConfig is always represented as a Vec<StoreConfig>, which
simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs.

Other Options
-------------

* File a bug report with the serde team.  This has been done by others, and it appears the serde
maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address
this issue.  See, for example, [here](serde-rs/serde#1544)

* Use one of the work-around crates published by the serde team.
  ** [Path to Error](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/). This appears
     to work, but adds an additional crate to our dependency tree.

  ** [Serde Untagged](https://docs.rs/serde-untagged/latest/serde_untagged/). Has unacceptable type
     limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64,
     char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields)

* Make all stores an array-of-tables.  In addition to breaking existing configurations, this
  seems counterintuitive to me.

* Use a different configuration name for single- vs chained-stores: [zones.stores] for single
  stores and [[zones.chained_stores]] for chained stores.  This still seems kind of clunky to me,
  particularly with the existing "stores" being plural for a single-store configuration, but is
  probably the next-best alternative.
@marcus0x62 marcus0x62 mentioned this pull request Oct 7, 2024
Merged
marcus0x62 added a commit to marcus0x62/hickory-dns that referenced this pull request Oct 7, 2024
@marcus0x62
Use custom serde visitor to fix store error messages
2313691 
Background
----------

This change introduces a custom serde visitor to handle single-store and chained-store
variants while still producing useful error messages.  The
[chained authority PR](hickory-dns#2161) used the
serde [untagged enum representation](https://serde.rs/enum-representations.html#untagged)
in order to represent single-store (TOML table, serde map) or chained-store
(TOML array-of-tables, serde sequence) variants.  Unfortunately, serde is not able to
show useful error messages when a syntax error is encountered and untagged enums are being
used.  For example, misspelling the "type" option in a zone store configuration results
in this error message to the user:

```
1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum StoreConfigContainer
```

By using a minimal custom visitor, we can restore the previous, more useful error messages:

**Single Store**
```
1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml"
Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [zones.stores]
   | ^^^^^^^^^^^^^^
missing field `type`
```

**Chained Stores**
```
1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1
   |
47 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
missing field `type`
```

A side effect of this change is that StoreConfig is always represented as a Vec<StoreConfig>, which
simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs.

Other Options
-------------

* File a bug report with the serde team.  This has been done by others, and it appears the serde
maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address
this issue.  See, for example, [here](serde-rs/serde#1544)

* Use one of the work-around crates published by the serde team.
   1. [Path to Error](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/). This appears
      to work, but adds an additional crate to our dependency tree.

   2. [Serde Untagged](https://docs.rs/serde-untagged/latest/serde_untagged/). Has unacceptable type
      limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64,
      char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields)

* Make all stores an array-of-tables.  In addition to breaking existing configurations, this
  seems counterintuitive to me.

* Use a different configuration name for single- vs chained-stores: [zones.stores] for single
  stores and [[zones.chained_stores]] for chained stores.  This still seems kind of clunky to me,
  particularly with the existing "stores" being plural for a single-store configuration, but is
  probably the next-best alternative.
marcus0x62 added a commit to marcus0x62/hickory-dns that referenced this pull request Oct 7, 2024
@marcus0x62
Use custom serde visitor to fix store error messages
12eedb2 
Background
----------

This change introduces a custom serde visitor to handle single-store and chained-store
variants while still producing useful error messages.  The
[chained authority PR](hickory-dns#2161) used the
serde [untagged enum representation](https://serde.rs/enum-representations.html#untagged)
in order to represent single-store (TOML table, serde map) or chained-store
(TOML array-of-tables, serde sequence) variants.  Unfortunately, serde is not able to
show useful error messages when a syntax error is encountered and untagged enums are being
used.  For example, misspelling the "type" option in a zone store configuration results
in this error message to the user:

```
1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum StoreConfigContainer
```

By using a minimal custom visitor, we can restore the previous, more useful error messages:

**Single Store**
```
1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml"
Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [zones.stores]
   | ^^^^^^^^^^^^^^
missing field `type`
```

**Chained Stores**
```
1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1
   |
47 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
missing field `type`
```

A side effect of this change is that StoreConfig is always represented as a Vec<StoreConfig>, which
simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs.

Other Options
-------------

* File a bug report with the serde team.  This has been done by others, and it appears the serde
maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address
this issue.  See, for example, [here](serde-rs/serde#1544)

* Use one of the work-around crates published by the serde team.
   1. [Path to Error](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/). This appears
      to work, but adds an additional crate to our dependency tree.

   2. [Serde Untagged](https://docs.rs/serde-untagged/latest/serde_untagged/). Has unacceptable type
      limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64,
      char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields)

* Make all stores an array-of-tables.  In addition to breaking existing configurations, this
  seems counterintuitive to me.

* Use a different configuration name for single- vs chained-stores: [zones.stores] for single
  stores and [[zones.chained_stores]] for chained stores.  This still seems kind of clunky to me,
  particularly with the existing "stores" being plural for a single-store configuration, but is
  probably the next-best alternative.
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
@marcus0x62
Use custom serde visitor to fix store error messages
7887002 
Background
----------

This change introduces a custom serde visitor to handle single-store and chained-store
variants while still producing useful error messages.  The
[chained authority PR](#2161) used the
serde [untagged enum representation](https://serde.rs/enum-representations.html#untagged)
in order to represent single-store (TOML table, serde map) or chained-store
(TOML array-of-tables, serde sequence) variants.  Unfortunately, serde is not able to
show useful error messages when a syntax error is encountered and untagged enums are being
used.  For example, misspelling the "type" option in a zone store configuration results
in this error message to the user:

```
1727879524:INFO:hickory_dns:443:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum StoreConfigContainer
```

By using a minimal custom visitor, we can restore the previous, more useful error messages:

**Single Store**
```
1728310440:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/example_recursor.toml"
Error: failed to read config file from "tests/test-data/test_configs/example_recursor.toml": toml decode error: TOML parse error at line 38, column 1
   |
38 | [zones.stores]
   | ^^^^^^^^^^^^^^
missing field `type`
```

**Chained Stores**
```
1728311606:INFO:hickory_dns:431:loading configuration from: "tests/test-data/test_configs/chained_blocklist.toml"
Error: failed to read config file from "tests/test-data/test_configs/chained_blocklist.toml": toml decode error: TOML parse error at line 47, column 1
   |
47 | [[zones.stores]]
   | ^^^^^^^^^^^^^^^^
missing field `type`
```

A side effect of this change is that StoreConfig is always represented as a Vec<StoreConfig>, which
simplifies a bit of the code that iterates over the stores in bin/src/hickory-dns.rs.

Other Options
-------------

* File a bug report with the serde team.  This has been done by others, and it appears the serde
maintainers have no intention of fixing this in serde proper, rejecting multiple PRs that address
this issue.  See, for example, [here](serde-rs/serde#1544)

* Use one of the work-around crates published by the serde team.
   1. [Path to Error](https://docs.rs/serde_path_to_error/latest/serde_path_to_error/). This appears
      to work, but adds an additional crate to our dependency tree.

   2. [Serde Untagged](https://docs.rs/serde-untagged/latest/serde_untagged/). Has unacceptable type
      limitations (it only supports bool, i8, i16, i32, i64, i128, u8, u16, u32, u64, u128, f32, f64,
      char, string, borrowed_str, bytes, borrowed_bytes, byte_buf, unit, seq, map fields)

* Make all stores an array-of-tables.  In addition to breaking existing configurations, this
  seems counterintuitive to me.

* Use a different configuration name for single- vs chained-stores: [zones.stores] for single
  stores and [[zones.chained_stores]] for chained stores.  This still seems kind of clunky to me,
  particularly with the existing "stores" being plural for a single-store configuration, but is
  probably the next-best alternative.
@marcus0x62 marcus0x62 mentioned this pull request Nov 29, 2024
Closed
@marcus0x62 marcus0x62 mentioned this pull request Oct 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djc djc djc approved these changes

@japaric japaric Awaiting requested review from japaric

@justahero justahero Awaiting requested review from justahero

@listochkin listochkin Awaiting requested review from listochkin

@pvdrz pvdrz Awaiting requested review from pvdrz

@bluejekyll bluejekyll Awaiting requested review from bluejekyll bluejekyll is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marcus0x62 @bluejekyll @djc