[3/n] [wicketd-commission-types] reject unknown fields in UserSpecifiedPortConfig (#10808)

`serde(untagged)` is a bit fragile here. I'm not sure if we're going to
switch this to internal tagging in the future, but this code can be
removed if we make that change.

Depends on:

* #10806
* #10807

Author: sunshowers

openapi/wicketd.json

@@ -7978,12 +7978,20 @@
         }
       },
       "UserSpecifiedPortConfig": {
+        "description": "A user-specified port configuration.",
         "anyOf": [
           {
-            "$ref": "#/components/schemas/ManualPortConfig"
+            "description": "A manually-configured port.",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/ManualPortConfig"
+              }
+            ]
           },
           {
-            "type": "object"
+            "description": "A port configured automatically via DDM.",
+            "type": "object",
+            "additionalProperties": false
           }
         ]
       },

wicket/src/cli/rack_setup/config_toml.rs

@@ -349,9 +349,13 @@ fn populate_uplink_table(cfg: &UserSpecifiedPortConfig) -> Table {
     // This style ensures that if a new field is added, this fails loudly.
     let manual_port_config = match cfg {
         UserSpecifiedPortConfig::Manual(manual) => manual,
-        UserSpecifiedPortConfig::DdmAutoPortConfig {} => {
+        UserSpecifiedPortConfig::DdmAutoPortConfig => {
+            // A DDM-auto port is encoded as an empty table (the comment is
+            // operator-facing).
             let mut uplink = Table::new();
-            uplink.insert("type", string_item("ddm_auto_port_config"));
+            uplink.decor_mut().set_prefix(
+                "\n# This port is configured automatically via DDM.\n",
+            );
             return uplink;
         }
     };

wicketd-commission-types/versions/src/impls/rack_setup.rs

@@ -53,14 +53,14 @@ impl UserSpecifiedRackNetworkConfig {
             UserSpecifiedPortConfig::Manual(cfg) => {
                 Some((SwitchSlot::Switch0, port.as_str(), cfg))
             }
-            UserSpecifiedPortConfig::DdmAutoPortConfig {} => None,
+            UserSpecifiedPortConfig::DdmAutoPortConfig => None,
         });
 
         let iter1 = self.switch1.iter().filter_map(|(port, cfg)| match cfg {
             UserSpecifiedPortConfig::Manual(cfg) => {
                 Some((SwitchSlot::Switch1, port.as_str(), cfg))
             }
-            UserSpecifiedPortConfig::DdmAutoPortConfig {} => None,
+            UserSpecifiedPortConfig::DdmAutoPortConfig => None,
         });
 
         iter0.chain(iter1)
@@ -89,14 +89,14 @@ impl UserSpecifiedPortConfig {
     pub fn manual(&self) -> Option<&ManualPortConfig> {
         match self {
             Self::Manual(cfg) => Some(cfg),
-            Self::DdmAutoPortConfig {} => None,
+            Self::DdmAutoPortConfig => None,
         }
     }
 
     pub fn manual_mut(&mut self) -> Option<&mut ManualPortConfig> {
         match self {
             Self::Manual(cfg) => Some(cfg),
-            Self::DdmAutoPortConfig {} => None,
+            Self::DdmAutoPortConfig => None,
         }
     }
 }
@@ -179,8 +179,9 @@ impl From<UserSpecifiedImportExportPolicy> for ImportExportPolicy {
 #[cfg(test)]
 mod tests {
     use crate::latest::rack_setup::{
-        UplinkAddress, UserSpecifiedImportExportPolicy,
-        UserSpecifiedRouterPeerAddr,
+        LinkFec, LinkSpeed, ManualPortConfig, UplinkAddress,
+        UserSpecifiedImportExportPolicy, UserSpecifiedPortConfig,
+        UserSpecifiedRouterPeerAddr, UserSpecifiedUplinkAddressConfig,
     };
     use crate::v1::rack_setup::uplink_address_serde;
     use serde::{Deserialize, Serialize};
@@ -357,4 +358,101 @@ mod tests {
         #[serde(with = "uplink_address_serde")]
         pub addr: UplinkAddress,
     }
+
+    #[test]
+    fn empty_map_deserializes_to_ddm_auto() {
+        let from_json: UserSpecifiedPortConfig =
+            serde_json::from_str("{}").unwrap();
+        assert_eq!(from_json, UserSpecifiedPortConfig::DdmAutoPortConfig);
+
+        let from_toml: PortConfigWrapper =
+            toml::from_str("port = {}\n").unwrap();
+        assert_eq!(from_toml.port, UserSpecifiedPortConfig::DdmAutoPortConfig);
+    }
+
+    #[test]
+    fn ddm_auto_serializes_to_empty_map() {
+        let config = UserSpecifiedPortConfig::DdmAutoPortConfig;
+
+        let json = serde_json::to_string(&config).unwrap();
+        assert_eq!(json, "{}");
+        let roundtripped: UserSpecifiedPortConfig =
+            serde_json::from_str(&json).unwrap();
+        assert_eq!(roundtripped, config);
+
+        let wrapper = PortConfigWrapper { port: config.clone() };
+        let toml_str = toml::to_string(&wrapper).unwrap();
+        let roundtripped: PortConfigWrapper =
+            toml::from_str(&toml_str).unwrap();
+        assert_eq!(roundtripped.port, config);
+    }
+
+    #[test]
+    fn manual_config_roundtrips() {
+        let expected = UserSpecifiedPortConfig::Manual(ManualPortConfig {
+            routes: vec![],
+            addresses: vec![UserSpecifiedUplinkAddressConfig::without_vlan(
+                "1.1.1.0/24".parse().unwrap(),
+            )],
+            uplink_port_speed: LinkSpeed::Speed40G,
+            uplink_port_fec: Some(LinkFec::Rs),
+            autoneg: false,
+            bgp_peers: vec![],
+            lldp: None,
+            tx_eq: None,
+        });
+
+        eprintln!("** testing JSON round-trip");
+        let json = serde_json::to_string(&expected).unwrap();
+        eprintln!("serialized JSON: {json}");
+        let from_json: UserSpecifiedPortConfig =
+            serde_json::from_str(&json).unwrap();
+        assert_eq!(from_json, expected);
+        assert!(from_json.manual().is_some());
+
+        eprintln!("** testing TOML deserialization");
+        let toml_manual = r#"
+            routes = []
+            addresses = [{ address = "1.1.1.0/24" }]
+            uplink_port_speed = "speed40_g"
+            uplink_port_fec = "rs"
+            autoneg = false
+        "#;
+        let from_toml: UserSpecifiedPortConfig =
+            toml::from_str(toml_manual).unwrap();
+        assert_eq!(from_toml, expected);
+    }
+
+    #[test]
+    fn misspelled_field_names_unknown_field() {
+        let err =
+            serde_json::from_str::<UserSpecifiedPortConfig>(r#"{"route": []}"#)
+                .expect_err("misspelled field should fail to deserialize");
+        let err = err.to_string();
+        assert!(
+            err.contains("unknown field `route`"),
+            "error should name the unknown field, got: {err}"
+        );
+        assert!(
+            err.contains("expected one of"),
+            "error should list the expected fields, got: {err}"
+        );
+    }
+
+    #[test]
+    fn non_map_input_fails_cleanly() {
+        let err =
+            serde_json::from_str::<UserSpecifiedPortConfig>(r#""not-a-map""#)
+                .expect_err("a string is not a valid port configuration");
+        let err = err.to_string();
+        assert!(
+            err.contains("invalid type: string"),
+            "error should report an invalid type, got: {err}"
+        );
+    }
+
+    #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)]
+    struct PortConfigWrapper {
+        port: UserSpecifiedPortConfig,
+    }
 }

wicketd-commission-types/versions/src/initial/rack_setup.rs

@@ -101,16 +101,178 @@ pub struct ManualPortConfig {
     pub tx_eq: Option<TxEqConfig>,
 }
 
-// We use `serde(untagged)` here, since the variants are vastly different.
-// This prevents having backwards incompatible changes in the RSS config before
-// multirack ships. Once multirack ships, we may wish to use internal tagging,
-// but it's not mandatory.
-#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, JsonSchema)]
-#[serde(rename_all = "snake_case", untagged)]
+/// A user-specified port configuration.
+///
+/// An empty map is serialized and deserialized as an auto config.
+#[derive(Clone, Debug, PartialEq, Eq)]
 #[allow(clippy::large_enum_variant)]
 pub enum UserSpecifiedPortConfig {
+    /// A manually-configured port.
     Manual(ManualPortConfig),
-    DdmAutoPortConfig {},
+    /// A port configured automatically via DDM.
+    DdmAutoPortConfig,
+}
+
+// Hand-roll the Serialize and Deserialize impls so we don't have to use
+// serde(untagged), under which invalid manual configs would silently fall back
+// to the auto variant.
+//
+// We may wish to switch this to internal tagging in the future, but that will
+// cause changes to the TOML config as well as the JSON schema.
+impl Serialize for UserSpecifiedPortConfig {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        match self {
+            Self::Manual(cfg) => cfg.serialize(serializer),
+            Self::DdmAutoPortConfig => {
+                use serde::ser::SerializeMap;
+                serializer.serialize_map(Some(0))?.end()
+            }
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for UserSpecifiedPortConfig {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        struct PortConfigVisitor;
+
+        impl<'de> serde::de::Visitor<'de> for PortConfigVisitor {
+            type Value = UserSpecifiedPortConfig;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                formatter.write_str(
+                    "a map of manual port configuration fields, or an empty \
+                     map for a DDM-automatic port",
+                )
+            }
+
+            fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
+            where
+                A: serde::de::MapAccess<'de>,
+            {
+                let Some(first_key) = map.next_key::<String>()? else {
+                    return Ok(UserSpecifiedPortConfig::DdmAutoPortConfig);
+                };
+
+                let replay =
+                    ReplayFirstKey { first_key: Some(first_key), inner: map };
+                let manual = ManualPortConfig::deserialize(
+                    serde::de::value::MapAccessDeserializer::new(replay),
+                )?;
+                Ok(UserSpecifiedPortConfig::Manual(manual))
+            }
+        }
+
+        deserializer.deserialize_map(PortConfigVisitor)
+    }
+}
+
+/// A `MapAccess` adaptor that yields the already-consumed first key before
+/// delegating the rest of the map to the inner `MapAccess`.
+struct ReplayFirstKey<A> {
+    first_key: Option<String>,
+    inner: A,
+}
+
+impl<'de, A> serde::de::MapAccess<'de> for ReplayFirstKey<A>
+where
+    A: serde::de::MapAccess<'de>,
+{
+    type Error = A::Error;
+
+    fn next_key_seed<K>(
+        &mut self,
+        seed: K,
+    ) -> Result<Option<K::Value>, Self::Error>
+    where
+        K: serde::de::DeserializeSeed<'de>,
+    {
+        match self.first_key.take() {
+            Some(first_key) => {
+                use serde::de::IntoDeserializer;
+                let de = first_key.into_deserializer();
+                seed.deserialize(de).map(Some)
+            }
+            None => self.inner.next_key_seed(seed),
+        }
+    }
+
+    fn next_value_seed<V>(&mut self, seed: V) -> Result<V::Value, Self::Error>
+    where
+        V: serde::de::DeserializeSeed<'de>,
+    {
+        self.inner.next_value_seed(seed)
+    }
+
+    fn size_hint(&self) -> Option<usize> {
+        let inner = self.inner.size_hint();
+        match self.first_key {
+            Some(_) => inner.map(|n| n + 1),
+            None => inner,
+        }
+    }
+}
+
+// The descriptions and shape here must stay in sync with the variant doc
+// comments and the hand-rolled Serialize/Deserialize impls above.
+impl JsonSchema for UserSpecifiedPortConfig {
+    fn schema_name() -> String {
+        "UserSpecifiedPortConfig".to_string()
+    }
+
+    fn json_schema(
+        generator: &mut schemars::r#gen::SchemaGenerator,
+    ) -> schemars::schema::Schema {
+        use schemars::schema::InstanceType;
+        use schemars::schema::Metadata;
+        use schemars::schema::ObjectValidation;
+        use schemars::schema::Schema;
+        use schemars::schema::SchemaObject;
+        use schemars::schema::SubschemaValidation;
+
+        let mut manual =
+            generator.subschema_for::<ManualPortConfig>().into_object();
+        manual.metadata().description =
+            Some("A manually-configured port.".to_string());
+
+        let ddm_auto = SchemaObject {
+            metadata: Some(Box::new(Metadata {
+                description: Some(
+                    "A port configured automatically via DDM.".to_string(),
+                ),
+                ..Default::default()
+            })),
+            instance_type: Some(InstanceType::Object.into()),
+            object: Some(Box::new(ObjectValidation {
+                additional_properties: Some(Box::new(Schema::Bool(false))),
+                ..Default::default()
+            })),
+            ..Default::default()
+        };
+
+        SchemaObject {
+            metadata: Some(Box::new(Metadata {
+                description: Some(
+                    "A user-specified port configuration.".to_string(),
+                ),
+                ..Default::default()
+            })),
+            subschemas: Some(Box::new(SubschemaValidation {
+                any_of: Some(vec![
+                    Schema::Object(manual),
+                    Schema::Object(ddm_auto),
+                ]),
+                ..Default::default()
+            })),
+            ..Default::default()
+        }
+        .into()
+    }
 }
 
 /// User-specified version of `UplinkAddressConfig`.