[3/n] [wicketd-commission-types] reject unknown fields in UserSpecifiedPortConfig

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`.