From 3198751fd920021bd33d6ed299f392aaa97e29ad Mon Sep 17 00:00:00 2001 From: "Andrew J. Stone" Date: Tue, 7 Jan 2025 23:53:46 +0000 Subject: [PATCH] Add a Diff Visitor for BlueprintZoneConfigs This visitor walks a diffus generated diff and provides callbacks for users to hook into the parts of the diff that they care about. To keep things minimal, we don't provide callback trait methods for copies (when nothing changes), and we don't provide methods for most diffus_derive generated types (the ones that start with `Edited`). We anticipate composing these visitors to build up a full diff of a Blueprint. They can also be used individually as demonstrated with the included property based test. In order to easily generate `BlueprintZoneConfigs` for property tests we derived `Arbitrary` for `BlueprintZoneConfigs` and its fields using the `test-strategy` crate. Some of the types in the hierarchy don't actually implement `Arbitrary` and they come from foreign crates. In this case a corresponding generation strategy was added that allows those types to be generated based on more primitive types that do implement arbitrary. This PR is just one of many coming to build up and use automated blueprint diffs. It builds on the following PRs which must be merged in first: * https://github.com/oxidecomputer/newtype-uuid/pull/56 * https://github.com/oxidecomputer/diffus/pull/8 --- Cargo.lock | 33 +- Cargo.toml | 6 +- common/src/api/external/mod.rs | 41 ++- common/src/api/internal/shared.rs | 47 ++- common/src/zpool_name.rs | 2 + nexus-sled-agent-shared/Cargo.toml | 5 + nexus-sled-agent-shared/src/inventory.rs | 1 + nexus/types/Cargo.toml | 8 + nexus/types/src/deployment.rs | 15 + nexus/types/src/deployment/diff_visitors.rs | 55 +++ .../visit_blueprint_zones_config.rs | 342 ++++++++++++++++++ .../types/src/deployment/network_resources.rs | 4 + nexus/types/src/deployment/zone_type.rs | 49 +++ uuid-kinds/Cargo.toml | 4 +- 14 files changed, 590 insertions(+), 22 deletions(-) create mode 100644 nexus/types/src/deployment/diff_visitors.rs create mode 100644 nexus/types/src/deployment/diff_visitors/visit_blueprint_zones_config.rs diff --git a/Cargo.lock b/Cargo.lock index 7c4f36da46..aba40971a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2396,11 +2396,10 @@ checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" [[package]] name = "diffus" version = "0.10.0" -source = "git+https://github.com/oxidecomputer/diffus?branch=oxide/main#f6abe39bffd875b5fb1ebabf8144da15e3effe16" dependencies = [ "diffus-derive", "itertools 0.10.5", - "newtype-uuid", + "newtype-uuid 1.1.3", "oxnet", "uuid", ] @@ -2408,7 +2407,6 @@ dependencies = [ [[package]] name = "diffus-derive" version = "0.10.0" -source = "git+https://github.com/oxidecomputer/diffus?branch=oxide/main#f6abe39bffd875b5fb1ebabf8144da15e3effe16" dependencies = [ "proc-macro2", "quote", @@ -5153,7 +5151,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4979f22fdb869068da03c9f7528f8297c6fd2606bc3a4affe42e6a823fdb8da4" dependencies = [ "cfg-if", - "windows-targets 0.52.6", + "windows-targets 0.48.5", ] [[package]] @@ -5685,14 +5683,22 @@ dependencies = [ [[package]] name = "newtype-uuid" version = "1.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4c8781e2ef64806278a55ad223f0bc875772fd40e1fe6e73e8adbf027817229d" dependencies = [ + "proptest", "schemars", "serde", "uuid", ] +[[package]] +name = "newtype-uuid" +version = "1.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c8781e2ef64806278a55ad223f0bc875772fd40e1fe6e73e8adbf027817229d" +dependencies = [ + "uuid", +] + [[package]] name = "newtype_derive" version = "0.1.6" @@ -6090,7 +6096,7 @@ dependencies = [ "internal-dns-resolver", "internal-dns-types", "ipnet", - "newtype-uuid", + "newtype-uuid 1.1.3", "nexus-config", "nexus-db-model", "nexus-db-queries", @@ -6236,11 +6242,13 @@ dependencies = [ "omicron-passwords", "omicron-uuid-kinds", "omicron-workspace-hack", + "proptest", "schemars", "serde", "serde_json", "sled-hardware-types", "strum", + "test-strategy", "thiserror 1.0.69", "uuid", ] @@ -6342,7 +6350,7 @@ dependencies = [ "illumos-utils", "internal-dns-types", "ipnetwork", - "newtype-uuid", + "newtype-uuid 1.1.3", "newtype_derive", "nexus-sled-agent-shared", "omicron-common", @@ -7413,8 +7421,9 @@ name = "omicron-uuid-kinds" version = "0.1.0" dependencies = [ "diffus", - "newtype-uuid", + "newtype-uuid 1.1.3", "paste", + "proptest", "schemars", ] @@ -7489,7 +7498,7 @@ dependencies = [ "managed", "memchr", "mio", - "newtype-uuid", + "newtype-uuid 1.1.3 (registry+https://github.com/rust-lang/crates.io-index)", "nom", "num-bigint-dig", "num-integer", @@ -8141,7 +8150,7 @@ dependencies = [ [[package]] name = "oxnet" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/oxnet#7dacd265f1bcd0f8b47bd4805250c4f0812da206" +source = "git+https://github.com/oxidecomputer/oxnet#49ee85dcd294901031c3395ee773363db1cdc289" dependencies = [ "ipnetwork", "schemars", @@ -12427,7 +12436,7 @@ checksum = "82205ffd44a9697e34fc145491aa47310f9871540bb7909eaa9365e0a9a46607" name = "typed-rng" version = "0.1.0" dependencies = [ - "newtype-uuid", + "newtype-uuid 1.1.3", "omicron-workspace-hack", "rand", "rand_core", diff --git a/Cargo.toml b/Cargo.toml index 8ea13cce5b..02db524c5d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -369,7 +369,8 @@ derive-where = "1.2.7" # Having the i-implement-... feature here makes diesel go away from the workspace-hack diesel = { version = "2.2.4", features = ["i-implement-a-third-party-backend-and-opt-into-breaking-changes", "postgres", "r2d2", "chrono", "serde_json", "network-address", "uuid"] } diesel-dtrace = "0.4.2" -diffus = { git = "https://github.com/oxidecomputer/diffus", branch = "oxide/main", features = ["uuid-impl", "derive", "newtype-uuid-impl", "oxnet-impl"] } +#diffus = { git = "https://github.com/oxidecomputer/diffus", branch = "oxide/main", features = ["uuid-impl", "derive", "newtype-uuid-impl", "oxnet-impl"] } +diffus = { path = "../diffus/diffus", features = ["uuid-impl", "derive", "newtype-uuid-impl", "oxnet-impl"] } dns-server = { path = "dns-server" } dns-server-api = { path = "dns-server-api" } dns-service-client = { path = "clients/dns-service-client" } @@ -679,7 +680,8 @@ zone = { version = "0.3", default-features = false, features = ["async"] } # the kinds). However, uses of omicron-uuid-kinds _within omicron_ will have # std and the other features enabled because they'll refer to it via # omicron-uuid-kinds.workspace = true. -newtype-uuid = { version = "1.1.3", default-features = false } +#newtype-uuid = { version = "1.1.3", default-features = false } +newtype-uuid = { path = "../newtype-uuid/crates/newtype-uuid", default-features = false } omicron-uuid-kinds = { path = "uuid-kinds", features = ["serde", "schemars08", "uuid-v4"] } # NOTE: The test profile inherits from the dev profile, so settings under diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 4c8f032fcb..2aaff627d8 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -46,6 +46,9 @@ use std::num::{NonZeroU16, NonZeroU32}; use std::str::FromStr; use uuid::Uuid; +#[cfg(any(test, feature = "testing"))] +use proptest::{arbitrary::any, strategy::Strategy}; + // The type aliases below exist primarily to ensure consistency among return // types for functions in the `nexus::Nexus` and `nexus::DataStore`. The // type argument `T` generally implements `Object`. @@ -213,6 +216,7 @@ impl<'a> TryFrom<&DataPageParams<'a, NameOrId>> for DataPageParams<'a, Uuid> { #[display("{0}")] #[serde(try_from = "String")] #[derive(Diffus)] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct Name(String); /// `Name::try_from(String)` is the primary method for constructing an Name @@ -751,10 +755,28 @@ impl From for i64 { PartialEq, PartialOrd, Serialize, - Diffus, )] +#[cfg_attr(feature = "testing", derive(test_strategy::Arbitrary))] pub struct Generation(u64); +// We have to manually implement `Diffable` because this is newtype with private +// data, and we want to see the diff on the newtype not the inner data. +impl<'a> Diffable<'a> for Generation { + type Diff = (&'a Generation, &'a Generation); + + fn diff(&'a self, other: &'a Self) -> edit::Edit<'a, Self> { + if self == other { + edit::Edit::Copy(self) + } else { + edit::Edit::Change { + before: self, + after: other, + diff: (self, other), + } + } + } +} + impl Generation { pub const fn new() -> Generation { Generation(1) @@ -1939,7 +1961,15 @@ impl JsonSchema for L4PortRange { SerializeDisplay, Hash, )] -pub struct MacAddr(pub macaddr::MacAddr6); +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] +pub struct MacAddr( + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::<(u8, u8, u8, u8, u8, u8)>() + .prop_map(|(a,b,c,d,e,f)| macaddr::MacAddr6::new(a,b,c,d,e,f)))) + ] + pub macaddr::MacAddr6, +); impl<'a> Diffable<'a> for MacAddr { type Diff = (&'a Self, &'a Self); @@ -1947,7 +1977,11 @@ impl<'a> Diffable<'a> for MacAddr { if self == other { edit::Edit::Copy(self) } else { - edit::Edit::Change((self, other)) + edit::Edit::Change { + before: self, + after: other, + diff: (self, other), + } } } } @@ -2117,6 +2151,7 @@ impl JsonSchema for MacAddr { JsonSchema, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct Vni(u32); impl Vni { diff --git a/common/src/api/internal/shared.rs b/common/src/api/internal/shared.rs index 588117c71c..e0ccda4738 100644 --- a/common/src/api/internal/shared.rs +++ b/common/src/api/internal/shared.rs @@ -22,6 +22,8 @@ use strum::EnumCount; use uuid::Uuid; use super::nexus::HostIdentifier; +#[cfg(any(test, feature = "testing"))] +use proptest::{arbitrary::any, strategy::Strategy}; /// The type of network interface #[derive( @@ -39,13 +41,32 @@ use super::nexus::HostIdentifier; Diffus, )] #[serde(tag = "type", rename_all = "snake_case")] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum NetworkInterfaceKind { /// A vNIC attached to a guest instance - Instance { id: Uuid }, + Instance { + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::().prop_map(Uuid::from_u128))) + ] + id: Uuid, + }, /// A vNIC associated with an internal service - Service { id: Uuid }, + Service { + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::().prop_map(Uuid::from_u128))) + ] + id: Uuid, + }, /// A vNIC associated with a probe - Probe { id: Uuid }, + Probe { + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::().prop_map(Uuid::from_u128))) + ] + id: Uuid, + }, } /// Information required to construct a virtual network interface @@ -62,17 +83,34 @@ pub enum NetworkInterfaceKind { Hash, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct NetworkInterface { + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::().prop_map(Uuid::from_u128))) + ] pub id: Uuid, pub kind: NetworkInterfaceKind, pub name: Name, pub ip: IpAddr, pub mac: external::MacAddr, + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::<(IpAddr, u8)>() + .prop_map(|(addr, prefix)| IpNet::new_unchecked(addr, prefix)))) + ] pub subnet: IpNet, pub vni: Vni, pub primary: bool, pub slot: u8, #[serde(default)] + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::>() + .prop_map(|v| v.into_iter() + .map(|(addr, prefix)| + IpNet::new_unchecked(addr, prefix)).collect()))) + ] pub transit_ips: Vec, } @@ -93,6 +131,7 @@ pub struct NetworkInterface { Hash, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct SourceNatConfig { /// The external address provided to the instance or service. pub ip: IpAddr, @@ -898,7 +937,7 @@ pub struct ExternalIpGatewayMap { #[derive( Debug, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, EnumCount, Diffus, )] -#[cfg_attr(feature = "testing", derive(test_strategy::Arbitrary))] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum DatasetKind { // Durable datasets for zones Cockroach, diff --git a/common/src/zpool_name.rs b/common/src/zpool_name.rs index 9852b46120..eec2fe4c5d 100644 --- a/common/src/zpool_name.rs +++ b/common/src/zpool_name.rs @@ -19,6 +19,7 @@ pub const ZPOOL_INTERNAL_PREFIX: &str = "oxi_"; Copy, Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, JsonSchema, Diffus, )] #[serde(rename_all = "snake_case")] +#[cfg_attr(feature = "testing", derive(test_strategy::Arbitrary))] pub enum ZpoolKind { // This zpool is used for external storage (u.2) External, @@ -31,6 +32,7 @@ pub enum ZpoolKind { /// This expects that the format will be: `ox{i,p}_` - we parse the prefix /// when reading the structure, and validate that the UUID can be utilized. #[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord, Diffus)] +#[cfg_attr(feature = "testing", derive(test_strategy::Arbitrary))] pub struct ZpoolName { id: ZpoolUuid, kind: ZpoolKind, diff --git a/nexus-sled-agent-shared/Cargo.toml b/nexus-sled-agent-shared/Cargo.toml index ea83cb67ae..408dcf5130 100644 --- a/nexus-sled-agent-shared/Cargo.toml +++ b/nexus-sled-agent-shared/Cargo.toml @@ -13,11 +13,16 @@ omicron-common.workspace = true omicron-passwords.workspace = true omicron-uuid-kinds.workspace = true omicron-workspace-hack.workspace = true +proptest = { workspace = true, optional = true } # TODO: replace uses of propolis_client with local types schemars.workspace = true serde.workspace = true serde_json.workspace = true sled-hardware-types.workspace = true strum.workspace = true +test-strategy = { workspace = true, optional = true } thiserror.workspace = true uuid.workspace = true + +[features] +testing = ["proptest", "test-strategy"] diff --git a/nexus-sled-agent-shared/src/inventory.rs b/nexus-sled-agent-shared/src/inventory.rs index b2b1001f9b..1ccf49c4ca 100644 --- a/nexus-sled-agent-shared/src/inventory.rs +++ b/nexus-sled-agent-shared/src/inventory.rs @@ -186,6 +186,7 @@ impl OmicronZoneConfig { Hash, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct OmicronZoneDataset { pub pool_name: ZpoolName, } diff --git a/nexus/types/Cargo.toml b/nexus/types/Cargo.toml index edf22679ef..8c4db1b302 100644 --- a/nexus/types/Cargo.toml +++ b/nexus/types/Cargo.toml @@ -30,6 +30,7 @@ openssl.workspace = true oxql-types.workspace = true oxnet.workspace = true parse-display.workspace = true +proptest = { workspace = true, optional = true } schemars = { workspace = true, features = ["chrono", "uuid1"] } serde.workspace = true serde_json.workspace = true @@ -38,6 +39,7 @@ slog.workspace = true slog-error-chain.workspace = true steno.workspace = true strum.workspace = true +test-strategy = { workspace = true, optional = true } thiserror.workspace = true newtype-uuid.workspace = true update-engine.workspace = true @@ -55,5 +57,11 @@ omicron-workspace-hack.workspace = true # common to both, put them in `omicron-common` or `nexus-sled-agent-shared`. [dev-dependencies] +nexus-sled-agent-shared = { workspace = true, features = ["testing"] } +omicron-uuid-kinds = { workspace = true, features = ["testing"] } +omicron-common = { workspace = true, features = ["testing"] } proptest.workspace = true test-strategy.workspace = true + +[features] +testing = ["proptest", "test-strategy"] diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 33b8e102c5..09c80a7058 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -51,9 +51,13 @@ use strum::EnumIter; use strum::IntoEnumIterator; use uuid::Uuid; +#[cfg(any(test, feature = "testing"))] +use proptest::{arbitrary::any, strategy::Strategy}; + mod blueprint_diff; mod blueprint_display; mod clickhouse; +pub mod diff_visitors; pub mod execution; mod network_resources; mod planning_input; @@ -61,6 +65,7 @@ mod tri_map; mod zone_type; pub use clickhouse::ClickhouseClusterConfig; +pub use diff_visitors::BpVisitorContext; pub use network_resources::AddNetworkResourceError; pub use network_resources::OmicronZoneExternalFloatingAddr; pub use network_resources::OmicronZoneExternalFloatingIp; @@ -548,6 +553,7 @@ impl<'a> fmt::Display for BlueprintDisplay<'a> { #[derive( Debug, Clone, Eq, PartialEq, JsonSchema, Deserialize, Serialize, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct BlueprintZonesConfig { /// Generation number of this configuration. /// @@ -556,6 +562,13 @@ pub struct BlueprintZonesConfig { pub generation: Generation, /// The set of running zones. + // For test generation the key IDs must match the value IDs. + #[cfg_attr( + any(test, feature = "testing"), + strategy(any::>() + .prop_map(|configs| configs.into_iter() + .map(|c| (c.id, c)).collect()))) + ] pub zones: BTreeMap, } @@ -645,6 +658,7 @@ fn zone_sort_key(z: &T) -> impl Ord { Serialize, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct BlueprintZoneConfig { /// The disposition (desired state) of this zone recorded in the blueprint. pub disposition: BlueprintZoneDisposition, @@ -714,6 +728,7 @@ impl From for OmicronZoneConfig { EnumIter, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] #[serde(rename_all = "snake_case")] pub enum BlueprintZoneDisposition { /// The zone is in-service. diff --git a/nexus/types/src/deployment/diff_visitors.rs b/nexus/types/src/deployment/diff_visitors.rs new file mode 100644 index 0000000000..b04d608042 --- /dev/null +++ b/nexus/types/src/deployment/diff_visitors.rs @@ -0,0 +1,55 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! An API for visiting diffs between blueprints generated via [diffus](https://github.com/oxidecomputer/diffus) +//! +//! Modelled after [`syn::visit`](https://docs.rs/syn/1/syn/visit). + +pub mod visit_blueprint_zones_config; + +use diffus::edit::enm; +use omicron_uuid_kinds::{OmicronZoneUuid, SledUuid}; + +use super::{ + BlueprintZoneConfig, BlueprintZoneDisposition, BlueprintZoneType, + BlueprintZonesConfig, EditedBlueprintZoneConfig, +}; + +/// A context for blueprint related visitors +#[derive(Debug, Clone, Default)] +pub struct BpVisitorContext { + pub sled_id: Option, + pub zone_id: Option, +} + +#[derive(Debug, Clone, Copy)] +pub struct Change<'e, T> { + pub before: &'e T, + pub after: &'e T, +} + +impl<'e, T> Change<'e, T> { + pub fn new(before: &'e T, after: &'e T) -> Change<'e, T> { + Change { before, after } + } +} + +impl<'e, T> From<(&'e T, &'e T)> for Change<'e, T> { + fn from(value: (&'e T, &'e T)) -> Self { + Change::new(value.0, value.1) + } +} + +impl<'e, T, Diff> From<&enm::Edit<'e, T, Diff>> for Change<'e, T> { + fn from(value: &enm::Edit<'e, T, Diff>) -> Self { + match value { + enm::Edit::VariantChanged(before, after) => { + Change::new(before, after) + } + enm::Edit::AssociatedChanged { before, after, .. } => { + Change::new(before, after) + } + } + } +} diff --git a/nexus/types/src/deployment/diff_visitors/visit_blueprint_zones_config.rs b/nexus/types/src/deployment/diff_visitors/visit_blueprint_zones_config.rs new file mode 100644 index 0000000000..0ba672076c --- /dev/null +++ b/nexus/types/src/deployment/diff_visitors/visit_blueprint_zones_config.rs @@ -0,0 +1,342 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! A visitor for `BlueprintZonesConfig` + +use super::{ + BlueprintZoneConfig, BlueprintZoneDisposition, BlueprintZoneType, + BlueprintZonesConfig, BpVisitorContext, Change, EditedBlueprintZoneConfig, +}; + +use diffus::edit::{map, Edit}; +use omicron_common::{api::external::Generation, zpool_name::ZpoolName}; + +/// A trait to visit a [`BlueprintZonesConfig`] +pub trait VisitBlueprintZonesConfig<'e> { + fn visit_root( + &mut self, + ctx: &mut BpVisitorContext, + node: Edit<'e, BlueprintZonesConfig>, + ) { + visit_root(self, ctx, node); + } + + /// A change to `BlueprintZonesConfig::generation` + fn visit_generation_change( + &mut self, + _ctx: &mut BpVisitorContext, + _change: Change<'e, Generation>, + ) { + // Leaf node + } + + /// An insert to `BlueprintZonesConfig::zones` + fn visit_zones_insert( + &mut self, + _ctx: &mut BpVisitorContext, + _node: &BlueprintZoneConfig, + ) { + // Leaf node + } + + /// A removal from `BlueprintZonesConfig::zones` + fn visit_zones_remove( + &mut self, + _ctx: &mut BpVisitorContext, + _node: &BlueprintZoneConfig, + ) { + // Leaf node + } + + // A change in a value in `BlueprintZonesConfig::zones` + fn visit_zone_change( + &mut self, + _ctx: &mut BpVisitorContext, + _change: Change<'e, BlueprintZoneConfig>, + ) { + // Leaf node + } + + // The representation of a diffus generated `EditedBlueprintZoneConfig` + // which contains recursive edits for each field when there is at least + // one change. + // + // This the equivalent node in the tree to `visit_zone_change`, but gives a + // diffus_derive generated structure rather than the before and after of the + // original structs that were diffed. + fn visit_zone_edit( + &mut self, + ctx: &mut BpVisitorContext, + node: &EditedBlueprintZoneConfig<'e>, + ) { + visit_zone_edit(self, ctx, node); + } + + /// A change in `BlueprintZoneConfig::disposition` + fn visit_zone_disposition_change( + &mut self, + _ctx: &mut BpVisitorContext, + _change: Change<'e, BlueprintZoneDisposition>, + ) { + // Leaf node + } + + /// A change in a `BlueprintZoneConfig::filesystem_pool` + fn visit_zone_filesystem_pool_change( + &mut self, + _ctx: &mut BpVisitorContext, + _change: Change<'e, Option>, + ) { + // Leaf node + } + + /// A change in a `BlueprintZoneConfig::zone_type` + /// + /// A `BlueprintZoneType` is a complicated structure. In order to keep the + /// first version of this visitor tractable, we just return the a `Change` + /// rather than a diffus `EditedBlueprintZoneType` that we have to walk. + /// We'll likely want to add this to get full coverage, but it can come + /// later, and should probably live in its own visitor that we call from + /// this point. + fn visit_zone_zone_type_change( + &mut self, + _ctx: &mut BpVisitorContext, + _change: Change<'e, BlueprintZoneType>, + ) { + // Leaf node - for now + } +} + +/// The root of the diff for a `BlueprintZonesConfig` +pub fn visit_root<'e, V>( + v: &mut V, + ctx: &mut BpVisitorContext, + node: Edit<'e, BlueprintZonesConfig>, +) where + V: VisitBlueprintZonesConfig<'e> + ?Sized, +{ + if let Edit::Change { diff: bp_zones_config, .. } = node { + if let Edit::Change { diff, .. } = bp_zones_config.generation { + v.visit_generation_change(ctx, diff.into()); + } + if let Edit::Change { diff, .. } = bp_zones_config.zones { + for (&zone_id, bp_zone_config_edit) in &diff { + ctx.zone_id = Some(*zone_id); + match bp_zone_config_edit { + map::Edit::Copy(_) => {} + map::Edit::Insert(bp_zone_config) => { + v.visit_zones_insert(ctx, bp_zone_config); + } + map::Edit::Remove(bp_zone_config) => { + v.visit_zones_remove(ctx, bp_zone_config); + } + map::Edit::Change { + before, + after, + diff: edited_bp_zone_config, + .. + } => { + v.visit_zone_change(ctx, Change::new(before, after)); + v.visit_zone_edit(ctx, edited_bp_zone_config); + } + } + } + // Reset the context + ctx.zone_id = None; + } + } +} + +pub fn visit_zone_edit<'e, V>( + v: &mut V, + ctx: &mut BpVisitorContext, + node: &EditedBlueprintZoneConfig<'e>, +) where + V: VisitBlueprintZonesConfig<'e> + ?Sized, +{ + if let Edit::Change { diff, .. } = &node.disposition { + v.visit_zone_disposition_change(ctx, diff.into()); + } + if let Edit::Change { diff, .. } = &node.filesystem_pool { + v.visit_zone_filesystem_pool_change(ctx, diff.into()); + } + if let Edit::Change { diff, .. } = &node.zone_type { + v.visit_zone_zone_type_change(ctx, diff.into()); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use diffus::Diffable; + use std::collections::BTreeSet; + use test_strategy::proptest; + + struct TestVisitor<'a> { + before: &'a BlueprintZonesConfig, + after: &'a BlueprintZonesConfig, + total_inserts: usize, + total_removes: usize, + } + + impl<'a> TestVisitor<'a> { + pub fn new( + before: &'a BlueprintZonesConfig, + after: &'a BlueprintZonesConfig, + ) -> Self { + TestVisitor { before, after, total_inserts: 0, total_removes: 0 } + } + } + + impl<'e> VisitBlueprintZonesConfig<'e> for TestVisitor<'e> { + fn visit_generation_change( + &mut self, + ctx: &mut BpVisitorContext, + change: Change<'e, Generation>, + ) { + assert_eq!(change.before, &self.before.generation); + assert_eq!(change.after, &self.after.generation); + assert_ne!(self.before.generation, self.after.generation); + + // We aren't operating on a particular zone + assert!(ctx.zone_id.is_none()); + } + + fn visit_zones_insert( + &mut self, + ctx: &mut BpVisitorContext, + node: &BlueprintZoneConfig, + ) { + let before: BTreeSet<_> = self.before.zones.keys().collect(); + let after: BTreeSet<_> = self.after.zones.keys().collect(); + assert!(!before.contains(&node.id)); + assert!(after.contains(&node.id)); + + // The inserted node is the same as what's in `after` + assert_eq!(node, self.after.zones.get(&node.id).unwrap()); + + // The key for the current zone id was filled in + assert_eq!(ctx.zone_id, Some(node.id)); + + self.total_inserts += 1; + } + + fn visit_zones_remove( + &mut self, + ctx: &mut BpVisitorContext, + node: &BlueprintZoneConfig, + ) { + let before: BTreeSet<_> = self.before.zones.keys().collect(); + let after: BTreeSet<_> = self.after.zones.keys().collect(); + assert!(before.contains(&node.id)); + assert!(!after.contains(&node.id)); + + // The removed node is the same as what's in `before` + assert_eq!(node, self.before.zones.get(&node.id).unwrap()); + + // The key for the current zone id was filled in + assert_eq!(ctx.zone_id, Some(node.id)); + + self.total_removes += 1; + } + + fn visit_zone_change( + &mut self, + ctx: &mut BpVisitorContext, + change: Change<'e, BlueprintZoneConfig>, + ) { + // The key for the current zone id was filled in and + // the zone with the same id was changed + assert_eq!(ctx.zone_id, Some(change.before.id)); + assert_eq!(ctx.zone_id, Some(change.after.id)); + + // The change is actually correct + assert_eq!( + self.before.zones.get(&ctx.zone_id.unwrap()), + Some(change.before) + ); + assert_eq!( + self.after.zones.get(&ctx.zone_id.unwrap()), + Some(change.after) + ); + } + + fn visit_zone_disposition_change( + &mut self, + ctx: &mut BpVisitorContext, + change: Change<'e, BlueprintZoneDisposition>, + ) { + assert_ne!(change.before, change.after); + assert_eq!( + self.before + .zones + .get(&ctx.zone_id.unwrap()) + .unwrap() + .disposition, + *change.before + ); + assert_eq!( + self.after + .zones + .get(&ctx.zone_id.unwrap()) + .unwrap() + .disposition, + *change.after + ); + } + + fn visit_zone_filesystem_pool_change( + &mut self, + ctx: &mut BpVisitorContext, + change: Change<'e, Option>, + ) { + assert_ne!(change.before, change.after); + assert_eq!( + self.before + .zones + .get(&ctx.zone_id.unwrap()) + .unwrap() + .filesystem_pool, + *change.before + ); + assert_eq!( + self.after + .zones + .get(&ctx.zone_id.unwrap()) + .unwrap() + .filesystem_pool, + *change.after + ); + } + + fn visit_zone_zone_type_change( + &mut self, + ctx: &mut BpVisitorContext, + change: Change<'e, BlueprintZoneType>, + ) { + assert_ne!(change.before, change.after); + assert_eq!( + self.before.zones.get(&ctx.zone_id.unwrap()).unwrap().zone_type, + *change.before + ); + assert_eq!( + self.after.zones.get(&ctx.zone_id.unwrap()).unwrap().zone_type, + *change.after + ); + } + } + + #[proptest] + fn diff(before: BlueprintZonesConfig, after: BlueprintZonesConfig) { + let mut ctx = BpVisitorContext::default(); + let mut visitor = TestVisitor::new(&before, &after); + let diff = before.diff(&after); + visitor.visit_root(&mut ctx, diff); + + assert_eq!( + visitor.total_inserts.wrapping_sub(visitor.total_removes), + after.zones.len().wrapping_sub(before.zones.len()) + ); + } +} diff --git a/nexus/types/src/deployment/network_resources.rs b/nexus/types/src/deployment/network_resources.rs index b08e5a2582..daf6150262 100644 --- a/nexus/types/src/deployment/network_resources.rs +++ b/nexus/types/src/deployment/network_resources.rs @@ -160,6 +160,7 @@ impl OmicronZoneNetworkResources { Serialize, Deserialize, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum OmicronZoneExternalIp { Floating(OmicronZoneExternalFloatingIp), Snat(OmicronZoneExternalSnatIp), @@ -224,6 +225,7 @@ pub enum OmicronZoneExternalIpKey { Deserialize, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct OmicronZoneExternalFloatingIp { pub id: ExternalIpUuid, pub ip: IpAddr, @@ -243,6 +245,7 @@ pub struct OmicronZoneExternalFloatingIp { Deserialize, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct OmicronZoneExternalFloatingAddr { pub id: ExternalIpUuid, pub addr: SocketAddr, @@ -273,6 +276,7 @@ impl OmicronZoneExternalFloatingAddr { Deserialize, Diffus, )] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub struct OmicronZoneExternalSnatIp { pub id: ExternalIpUuid, pub snat_cfg: SourceNatConfig, diff --git a/nexus/types/src/deployment/zone_type.rs b/nexus/types/src/deployment/zone_type.rs index c1ecd18158..2223a40b38 100644 --- a/nexus/types/src/deployment/zone_type.rs +++ b/nexus/types/src/deployment/zone_type.rs @@ -35,6 +35,7 @@ use std::net::SocketAddrV6; Diffus, )] #[serde(tag = "type", rename_all = "snake_case")] +#[cfg_attr(any(test, feature = "testing"), derive(test_strategy::Arbitrary))] pub enum BlueprintZoneType { BoundaryNtp(blueprint_zone_type::BoundaryNtp), Clickhouse(blueprint_zone_type::Clickhouse), @@ -359,6 +360,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct BoundaryNtp { pub address: SocketAddrV6, pub ntp_servers: Vec, @@ -382,6 +387,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct Clickhouse { pub address: SocketAddrV6, pub dataset: OmicronZoneDataset, @@ -399,6 +408,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct ClickhouseKeeper { pub address: SocketAddrV6, pub dataset: OmicronZoneDataset, @@ -417,6 +430,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct ClickhouseServer { pub address: SocketAddrV6, pub dataset: OmicronZoneDataset, @@ -434,6 +451,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct CockroachDb { pub address: SocketAddrV6, pub dataset: OmicronZoneDataset, @@ -451,6 +472,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct Crucible { pub address: SocketAddrV6, pub dataset: OmicronZoneDataset, @@ -468,6 +493,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct CruciblePantry { pub address: SocketAddrV6, } @@ -484,6 +513,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct ExternalDns { pub dataset: OmicronZoneDataset, /// The address at which the external DNS server API is reachable. @@ -506,6 +539,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct InternalDns { pub dataset: OmicronZoneDataset, pub http_address: SocketAddrV6, @@ -535,6 +572,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct InternalNtp { pub address: SocketAddrV6, } @@ -551,6 +592,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct Nexus { /// The address at which the internal nexus server is reachable. pub internal_address: SocketAddrV6, @@ -576,6 +621,10 @@ pub mod blueprint_zone_type { Serialize, Diffus, )] + #[cfg_attr( + any(test, feature = "testing"), + derive(test_strategy::Arbitrary) + )] pub struct Oximeter { pub address: SocketAddrV6, } diff --git a/uuid-kinds/Cargo.toml b/uuid-kinds/Cargo.toml index 0a80fab9c9..dd12500ae3 100644 --- a/uuid-kinds/Cargo.toml +++ b/uuid-kinds/Cargo.toml @@ -14,12 +14,14 @@ workspace = true [dependencies] diffus.workspace = true newtype-uuid.workspace = true -schemars = { workspace = true, optional = true } paste.workspace = true +proptest = { workspace = true, optional = true } +schemars = { workspace = true, optional = true } [features] default = ["std"] serde = ["newtype-uuid/serde"] schemars08 = ["newtype-uuid/schemars08", "schemars", "std"] std = ["newtype-uuid/std"] +testing = ["newtype-uuid/proptest1"] uuid-v4 = ["newtype-uuid/v4"]