From 994757e017e145f483da68f2702d2e4b4359301f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 7 Aug 2024 16:50:34 -0700 Subject: [PATCH] Serialize via string, not tag --- common/src/api/internal/shared.rs | 96 +++++++++++++++++++++++++------ common/src/disk.rs | 5 +- openapi/nexus-internal.json | 2 +- openapi/sled-agent.json | 4 +- schema/omicron-datasets.json | 6 +- sled-storage/src/error.rs | 3 + sled-storage/src/manager.rs | 33 +++++++---- 7 files changed, 113 insertions(+), 36 deletions(-) diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 874d1a5866..403e0855a3 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -10,13 +10,14 @@ use crate::{ }; use oxnet::{IpNet, Ipv4Net, Ipv6Net}; use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{de, Deserialize, Deserializer, Serialize, Serializer}; use std::{ collections::{HashMap, HashSet}, fmt, net::{IpAddr, Ipv4Addr, Ipv6Addr}, str::FromStr, }; +use strum::EnumCount; use uuid::Uuid; /// The type of network interface @@ -704,16 +705,7 @@ pub struct ResolvedVpcRouteSet { /// Describes the purpose of the dataset. #[derive( - Debug, - Serialize, - Deserialize, - JsonSchema, - Clone, - PartialEq, - Eq, - Ord, - PartialOrd, - Hash, + Debug, JsonSchema, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, EnumCount, )] #[serde(tag = "type", rename_all = "snake_case")] pub enum DatasetKind { @@ -736,6 +728,25 @@ pub enum DatasetKind { Debug, } +impl Serialize for DatasetKind { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.to_string()) + } +} + +impl<'de> Deserialize<'de> for DatasetKind { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + s.parse().map_err(de::Error::custom) + } +} + impl DatasetKind { pub fn dataset_should_be_encrypted(&self) -> bool { match self { @@ -758,7 +769,7 @@ impl DatasetKind { } } - /// Returns the zone name, if this is dataset for a zone filesystem. + /// Returns the zone name, if this is a dataset for a zone filesystem. /// /// Otherwise, returns "None". pub fn zone_name(&self) -> Option<&str> { @@ -808,16 +819,22 @@ impl FromStr for DatasetKind { fn from_str(s: &str) -> Result { use DatasetKind::*; let kind = match s { + "cockroachdb" => Cockroach, "crucible" => Crucible, - "cockroachdb" | "cockroach_db" => Cockroach, "clickhouse" => Clickhouse, "clickhouse_keeper" => ClickhouseKeeper, "external_dns" => ExternalDns, "internal_dns" => InternalDns, - _ => { - return Err(DatasetKindParseError::UnknownDataset( - s.to_string(), - )) + "zone" => ZoneRoot, + "debug" => Debug, + other => { + if let Some(name) = other.strip_prefix("zone/") { + Zone { name: name.to_string() } + } else { + return Err(DatasetKindParseError::UnknownDataset( + s.to_string(), + )); + } } }; Ok(kind) @@ -846,6 +863,7 @@ pub struct SledIdentifiers { #[cfg(test)] mod tests { + use super::*; use crate::api::internal::shared::AllowedSourceIps; use oxnet::{IpNet, Ipv4Net, Ipv6Net}; use std::net::{Ipv4Addr, Ipv6Addr}; @@ -890,4 +908,48 @@ mod tests { serde_json::from_str(r#"{"allow":"any"}"#).unwrap(), ); } + + #[test] + fn test_dataset_kind_serialization() { + let kinds = [ + DatasetKind::Crucible, + DatasetKind::Cockroach, + DatasetKind::Clickhouse, + DatasetKind::ClickhouseKeeper, + DatasetKind::ExternalDns, + DatasetKind::InternalDns, + DatasetKind::ZoneRoot, + DatasetKind::Zone { name: String::from("myzone") }, + DatasetKind::Debug, + ]; + + assert_eq!(kinds.len(), DatasetKind::COUNT); + + for kind in &kinds { + // To string, from string + let as_str = kind.to_string(); + let from_str = + DatasetKind::from_str(&as_str).unwrap_or_else(|_| { + panic!("Failed to convert {kind} to and from string") + }); + assert_eq!( + *kind, from_str, + "{kind} failed to convert to/from a string" + ); + + // Serialize, deserialize + let ser = serde_json::to_string(&kind) + .unwrap_or_else(|_| panic!("Failed to serialize {kind}")); + let de: DatasetKind = serde_json::from_str(&ser) + .unwrap_or_else(|_| panic!("Failed to deserialize {kind}")); + assert_eq!(*kind, de, "{kind} failed serialization"); + + // Test that serialization is equivalent to stringifying. + assert_eq!( + format!("\"{as_str}\""), + ser, + "{kind} does not match stringification/serialization" + ); + } + } } diff --git a/common/src/disk.rs b/common/src/disk.rs index b9a259574e..86ee48c0f6 100644 --- a/common/src/disk.rs +++ b/common/src/disk.rs @@ -8,6 +8,7 @@ use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::ZpoolUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; use uuid::Uuid; use crate::{ @@ -180,12 +181,12 @@ pub struct DatasetsConfig { /// for a sled before any requests have been made. pub generation: Generation, - pub datasets: Vec, + pub datasets: BTreeMap, } impl Default for DatasetsConfig { fn default() -> Self { - Self { generation: Generation::new(), datasets: vec![] } + Self { generation: Generation::new(), datasets: BTreeMap::new() } } } diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index 2bd7172bf3..cfbe028f9c 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -2599,7 +2599,7 @@ "type": { "type": "string", "enum": [ - "cockroach_db" + "cockroachdb" ] } }, diff --git a/openapi/sled-agent.json b/openapi/sled-agent.json index c9e5c709e3..82f3c6c3ae 100644 --- a/openapi/sled-agent.json +++ b/openapi/sled-agent.json @@ -2374,8 +2374,8 @@ "type": "object", "properties": { "datasets": { - "type": "array", - "items": { + "type": "object", + "additionalProperties": { "$ref": "#/components/schemas/DatasetConfig" } }, diff --git a/schema/omicron-datasets.json b/schema/omicron-datasets.json index 1821eddac7..b675432172 100644 --- a/schema/omicron-datasets.json +++ b/schema/omicron-datasets.json @@ -8,8 +8,8 @@ ], "properties": { "datasets": { - "type": "array", - "items": { + "type": "object", + "additionalProperties": { "$ref": "#/definitions/DatasetConfig" } }, @@ -86,7 +86,7 @@ "type": { "type": "string", "enum": [ - "cockroach_db" + "cockroachdb" ] } } diff --git a/sled-storage/src/error.rs b/sled-storage/src/error.rs index 96fca65f57..988f7f363a 100644 --- a/sled-storage/src/error.rs +++ b/sled-storage/src/error.rs @@ -84,6 +84,9 @@ pub enum Error { current: Generation, }, + #[error("Invalid configuration (UUID mismatch in arguments)")] + ConfigUuidMismatch, + #[error("Dataset configuration out-of-date (asked for {requested}, but latest is {current})")] DatasetConfigurationOutdated { requested: Generation, current: Generation }, diff --git a/sled-storage/src/manager.rs b/sled-storage/src/manager.rs index c086d656d1..bc0cac2014 100644 --- a/sled-storage/src/manager.rs +++ b/sled-storage/src/manager.rs @@ -674,12 +674,19 @@ impl StorageManager { async fn datasets_ensure( &mut self, - mut config: DatasetsConfig, + config: DatasetsConfig, ) -> Result { let log = self.log.new(o!("request" => "datasets_ensure")); - // Ensure that the datasets arrive in a consistent order - config.datasets.sort_by(|a, b| a.id.partial_cmp(&b.id).unwrap()); + // As a small input-check, confirm that the UUID of the map of inputs + // matches the DatasetConfig. + // + // The dataset configs are sorted by UUID so they always appear in the + // same order, but this check prevents adding an entry of: + // - (UUID: X, Config(UUID: Y)), for X != Y + if !config.datasets.iter().all(|(id, config)| *id == config.id) { + return Err(Error::ConfigUuidMismatch); + } // We rely on the schema being stable across reboots -- observe // "test_datasets_schema" below for that property guarantee. @@ -764,7 +771,7 @@ impl StorageManager { config: &DatasetsConfig, ) -> DatasetsManagementResult { let mut status = vec![]; - for dataset in &config.datasets { + for dataset in config.datasets.values() { status.push(self.dataset_ensure_internal(log, dataset).await); } DatasetsManagementResult { status } @@ -1122,6 +1129,7 @@ mod tests { use omicron_common::ledger; use omicron_test_utils::dev::test_setup_log; use sled_hardware::DiskFirmware; + use std::collections::BTreeMap; use std::sync::atomic::Ordering; use uuid::Uuid; @@ -1621,13 +1629,16 @@ mod tests { let id = DatasetUuid::new_v4(); let zpool_name = ZpoolName::new_external(config.disks[0].pool_id); let name = DatasetName::new(zpool_name.clone(), DatasetKind::Crucible); - let datasets = vec![DatasetConfig { + let datasets = BTreeMap::from([( id, - name, - compression: None, - quota: None, - reservation: None, - }]; + DatasetConfig { + id, + name, + compression: None, + quota: None, + reservation: None, + }, + )]); // "Generation = 1" is reserved as "no requests seen yet", so we jump // past it. let generation = Generation::new().next(); @@ -1659,7 +1670,7 @@ mod tests { // However, calling it with a different input and the same generation // number should fail. config.generation = current_config_generation; - config.datasets[0].reservation = Some(1024); + config.datasets.values_mut().next().unwrap().reservation = Some(1024); let err = harness.handle().datasets_ensure(config.clone()).await.unwrap_err(); assert!(matches!(err, Error::DatasetConfigurationChanged { .. }));