Use custom serde visitor to fix store error messages

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.

Author: marcus0x62

bin/src/hickory-dns.rs

@@ -61,7 +61,7 @@ use tracing_subscriber::{
 
 #[cfg(feature = "dns-over-tls")]
 use hickory_dns::dnssec::{self, TlsCertConfig};
-use hickory_dns::{Config, StoreConfig, StoreConfigContainer, ZoneConfig};
+use hickory_dns::{Config, StoreConfig, ZoneConfig};
 use hickory_proto::rr::Name;
 #[cfg(feature = "blocklist")]
 use hickory_server::store::blocklist::BlocklistAuthority;
@@ -167,23 +167,14 @@ async fn load_zone(
         warn!("allow_update is deprecated in [[zones]] section, it belongs in [[zones.stores]]");
     }
 
-    let mut normalized_stores = vec![];
-    if let Some(StoreConfigContainer::Single(store)) = &zone_config.stores {
-        normalized_stores.push(store);
-    } else if let Some(StoreConfigContainer::Chained(chained_stores)) = &zone_config.stores {
-        for store in chained_stores {
-            normalized_stores.push(store);
-        }
-    } else {
-        normalized_stores.push(&StoreConfig::Default);
-        debug!("No stores specified for {zone_name}, using default config processing");
-    }
-
-    // load the zone and build a vector of associated authorities to load in the catalog.
-    debug!("Loading authorities for {zone_name} with stores {normalized_stores:?}");
+    // load the zone and insert any configured authorities in the catalog.
+    debug!(
+        "loading authorities for {zone_name} with stores {:?}",
+        zone_config.stores
+    );
 
     let mut authorities: Vec<Arc<dyn AuthorityObject>> = vec![];
-    for store in normalized_stores {
+    for store in &zone_config.stores {
         let authority: Arc<dyn AuthorityObject> = match store {
             #[cfg(feature = "sqlite")]
             StoreConfig::Sqlite(config) => {

bin/src/lib.rs

@@ -9,16 +9,20 @@
 
 pub mod dnssec;
 
-use std::fs::File;
-use std::io::Read;
-use std::net::{AddrParseError, Ipv4Addr, Ipv6Addr};
-use std::path::{Path, PathBuf};
-use std::str::FromStr;
-use std::time::Duration;
+use std::{
+    fmt,
+    fs::File,
+    io::Read,
+    net::{AddrParseError, Ipv4Addr, Ipv6Addr},
+    path::{Path, PathBuf},
+    str::FromStr,
+    time::Duration,
+};
 
 use cfg_if::cfg_if;
 use ipnet::IpNet;
-use serde::{self, Deserialize};
+use serde::de::{self, MapAccess, SeqAccess, Visitor};
+use serde::{self, Deserialize, Deserializer};
 
 use hickory_proto::error::ProtoResult;
 use hickory_proto::rr::Name;
@@ -250,9 +254,14 @@ pub struct ZoneConfig {
     /// Keys for use by the zone
     #[serde(default)]
     pub keys: Vec<dnssec::KeyConfig>,
-    /// Store configurations, TODO: allow chained Stores
-    #[serde(default)]
-    pub stores: Option<StoreConfigContainer>,
+    /// Store configurations.  Note: we specify a default handler to get a Vec containing a
+    /// StoreConfig::Default, which is used for authoritative file-based zones and legacy sqlite
+    /// configurations. #[serde(default)] cannot be used, because it will invoke Default for Vec,
+    /// i.e., an empty Vec and we cannot implement Default for StoreConfig and return a Vec.  The
+    /// custom visitor is used to handle map (single store) or sequence (chained store) configurations.
+    #[serde(default = "store_config_default")]
+    #[serde(deserialize_with = "store_config_visitor")]
+    pub stores: Vec<StoreConfig>,
     /// The kind of non-existence proof provided by the nameserver
     #[cfg(feature = "dnssec")]
     pub nx_proof_kind: Option<NxProofKind>,
@@ -290,7 +299,7 @@ impl ZoneConfig {
             allow_axfr,
             enable_dnssec,
             keys,
-            stores: None,
+            stores: store_config_default(),
             #[cfg(feature = "dnssec")]
             nx_proof_kind,
         }
@@ -345,22 +354,6 @@ impl ZoneConfig {
     }
 }
 
-/// Enumeration over all Store configurations
-///
-/// This is the outer container enum, covering the single- and chained-store variants.
-/// The chained store variant is a vector of StoreConfigs that should be consulted in-order during the lookup process.
-/// An example of this (currently the only example,) is when the blocklist feature is used: the blocklist should be queried first, then
-/// a recursor or forwarder second if the blocklist authority does not match on the query.
-#[derive(Deserialize, PartialEq, Eq, Debug)]
-#[serde(untagged)]
-#[non_exhaustive]
-pub enum StoreConfigContainer {
-    /// For a zone with a single store
-    Single(StoreConfig),
-    /// For a zone with multiple stores.  E.g., a recursive or forwarding zone with block lists.
-    Chained(Vec<StoreConfig>),
-}
-
 /// Enumeration over all store types
 #[derive(Deserialize, PartialEq, Eq, Debug)]
 #[serde(tag = "type")]
@@ -388,8 +381,89 @@ pub enum StoreConfig {
     Default,
 }
 
+/// Create a default value for serde for StoreConfig.
+fn store_config_default() -> Vec<StoreConfig> {
+    vec![StoreConfig::Default]
+}
+
+/// Custom serde visitor that can deserialize a map (single configuration store, expressed as a TOML
+/// table) or sequence (chained configuration stores, expressed as a TOML array of tables.)
+/// This is used instead of an untagged enum because serde cannot provide variant-specific error
+/// messages when using an untagged enum.
+fn store_config_visitor<'de, D>(deserializer: D) -> Result<Vec<StoreConfig>, D::Error>
+where
+    D: Deserializer<'de>,
+{
+    struct MapOrSequence;
+
+    impl<'de> Visitor<'de> for MapOrSequence {
+        type Value = Vec<StoreConfig>;
+
+        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+            formatter.write_str("map or sequence")
+        }
+
+        fn visit_seq<S>(self, seq: S) -> Result<Vec<StoreConfig>, S::Error>
+        where
+            S: SeqAccess<'de>,
+        {
+            Deserialize::deserialize(de::value::SeqAccessDeserializer::new(seq))
+        }
+
+        fn visit_map<M>(self, map: M) -> Result<Vec<StoreConfig>, M::Error>
+        where
+            M: MapAccess<'de>,
+        {
+            match Deserialize::deserialize(de::value::MapAccessDeserializer::new(map)) {
+                Ok(seq) => Ok(vec![seq]),
+                Err(e) => Err(e),
+            }
+        }
+    }
+
+    deserializer.deserialize_any(MapOrSequence)
+}
+
 #[cfg(test)]
 mod tests {
+    const SINGLE_STORE_CONFIG_BAD: &str = r#"
+[[zones]]
+zone = "."
+zone_type = "Forward"
+
+[zones.stores]
+ype = "forward"
+"#;
+
+    const CHAINED_STORE_CONFIG_BAD: &str = r#"
+[[zones]]
+zone = "."
+zone_type = "Forward"
+
+[[zones.stores]]
+type = "forward"
+
+[[zones.stores.name_servers]]
+socket_addr = "8.8.8.8:53"
+protocol = "udp"
+trust_negative_responses = false
+
+[[zones.stores]]
+type = "forward"
+
+[[zones.stores.name_servers]]
+socket_addr = "1.1.1.1:53"
+rotocol = "udp"
+trust_negative_responses = false
+"#;
+
+    const EMPTY_STORE_CONFIG: &str = r#"
+[[zones]]
+zone = "localhost"
+zone_type = "Primary"
+file = "default/localhost.zone"
+"#;
+
     #[cfg(feature = "recursor")]
     #[test]
     fn example_recursor_config() {
@@ -398,4 +472,34 @@ mod tests {
         ))
         .unwrap();
     }
+
+    #[cfg(feature = "resolver")]
+    #[test]
+    fn single_store_config_error_message() {
+        match toml::from_str::<super::Config>(SINGLE_STORE_CONFIG_BAD) {
+            Ok(val) => panic!("expected error value; got ok: {val:?}"),
+            Err(e) => assert!(e.to_string().contains("missing field `type`")),
+        }
+    }
+
+    #[cfg(feature = "resolver")]
+    #[test]
+    fn chained_store_config_error_message() {
+        match toml::from_str::<super::Config>(CHAINED_STORE_CONFIG_BAD) {
+            Ok(val) => panic!("expected error value; got ok: {val:?}"),
+            Err(e) => assert!(e.to_string().contains("unknown field `rotocol`")),
+        }
+    }
+
+    #[cfg(feature = "resolver")]
+    #[test]
+    fn empty_store_default_value() {
+        match toml::from_str::<super::Config>(EMPTY_STORE_CONFIG) {
+            Ok(val) => {
+                assert_eq!(val.zones[0].stores.len(), 1);
+                assert_eq!(val.zones[0].stores[0], super::StoreConfig::Default);
+            }
+            Err(e) => panic!("expected successful parse: {e:?}"),
+        }
+    }
 }