[RSS] fix split-brain rack subnet

Prior to this commit, RSS accepted a set of parameters that included a `rack_subnet` and an optional (but not really) `rack_network_config`; the `rack_network_config` _also_ contained a `rack_subnet` property. The first `rack_subnet` was used by RSS to pick sled addresses, but the second is what it handed off to Nexus to record in the database. This PR makes three changes to parameters (the bulk of the diff is the expected fallout from them): * Removes the top-level `rack_subnet` field; only the `rack_network_config.rack_subnet` remains. * Makes `rack_network_config` non-optional. The handoff to Nexus would fail if this was `None`, so now it's always required. (This was only a little annoying in a few tests where we now have to cons up a fake network config.) * Changes wicket/wicket to accept a subset of `rack_network_config` that does _not_ include a `rack_subnet`; this is a value the control plane should choose on its own. One potentially-dangerous change is that the RSS parameters changed are not just passed when RSS is run; they're also serialized to disk as `rss-sled-plan.json`. We have a test to ensure changes don't affect the schema of this plan, but I believe the changes here are backwards compatible (an old plan that has a no-longer-present `rack_subet` is fine, and the JSON representation of the optional `RackNetworkConfig` is that it can be either `null` or an object; we'll fail to read any plans with `null`, but those would have failed to handoff to Nexus anyway as noted above). To check this is right, I pulled the `rss-sled-plan.json` off of madrid, censored the certs, replaced the password hash with one of our test hashes, and added a test that we can still read it. --- Changes that might make sense but I didn't attempt: * Changing the `rack_network_config` in the early networking bootstore to be non-optional. I think this would be correct, but is probably more trouble than it's worth to migrate. We might consider this the next time we make other, unrelated changes here though. * Removing the `rack_subnet` field not just from user -> wicket, but also from {wicket,developer} -> RSS. We could make RSS pick its own rack subnet, maybe? This seemed dubious enough I stopped. This does mean the TOML files used to automatically launch RSS still have a `rack_subnet` value, but now it's only one (under the rack network config) instead of two. * Changing the rack subnet choice to be random. wicket continues to use the hardcoded value we've been using. --- I also fixed a handful of places where we define the rack subnet `fd00:1122:3344:01::/56`; I believe this is just wrong / a typo. The `:01` at the end is equivalent to `:0001`, which is equivalent to the /56 `fd00:1122:3344:0000::/56`. Every place we had this we meant to use `fd00:1122:3344:0100::/56`, so I changed all them (I think!). Fixes #5009, but only for any racks that run RSS after this change. I am not attempting to retroactively correct any racks that had the wrong `rack_subnet` recorded in the database, as I believe all such deployed racks are dev systems that are frequently wiped and reinstalled.
oxidecomputer · Feb 7, 2024 · 81164dc · 81164dc
1 parent f2f2bfa
commit 81164dc
Show file tree

Hide file tree

Showing 28 changed files with 629 additions and 449 deletions.
diff --git a/clients/wicketd-client/src/lib.rs b/clients/wicketd-client/src/lib.rs
@@ -51,6 +51,7 @@ progenitor::generate_api!(
         CurrentRssUserConfigInsensitive = { derives = [ PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize ] },
         CurrentRssUserConfigSensitive = { derives = [ PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize ] },
         CurrentRssUserConfig = { derives = [ PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize ] },
+        UserSpecifiedRackNetworkConfig = { derives = [ PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize ] },
         GetLocationResponse = { derives = [ PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize ] },
     },
     replace = {

diff --git a/docs/how-to-run.adoc b/docs/how-to-run.adoc
@@ -277,7 +277,7 @@ The below example demonstrates a single static gateway route; in-depth explanati
 [rack_network_config]
 # An internal-only IPv6 address block which contains AZ-wide services.
 # This does not need to be changed.
-rack_subnet = "fd00:1122:3344:01::/56"
+rack_subnet = "fd00:1122:3344:0100::/56"
 # A range of IP addresses used by Boundary Services on the network.  In a real
 # system, these would be addresses of the uplink ports on the Sidecar.  With
 # softnpu, only one address is used.

diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs
diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs
@@ -288,13 +288,15 @@ impl nexus_test_interface::NexusServer for Server {
                             vec!["qsfp0".parse().unwrap()],
                         )]),
                     ),
-                    rack_network_config: Some(RackNetworkConfig {
-                        rack_subnet: "fd00:1122:3344:01::/56".parse().unwrap(),
+                    rack_network_config: RackNetworkConfig {
+                        rack_subnet: "fd00:1122:3344:0100::/56"
+                            .parse()
+                            .unwrap(),
                         infra_ip_first: Ipv4Addr::UNSPECIFIED,
                         infra_ip_last: Ipv4Addr::UNSPECIFIED,
                         ports: Vec::new(),
                         bgp: Vec::new(),
-                    }),
+                    },
                 },
             )
             .await

diff --git a/nexus/test-utils/src/lib.rs b/nexus/test-utils/src/lib.rs
@@ -65,7 +65,7 @@ pub const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc";
 pub const SWITCH_UUID: &str = "dae4e1f1-410e-4314-bff1-fec0504be07e";
 pub const OXIMETER_UUID: &str = "39e6175b-4df2-4730-b11d-cbc1e60a2e78";
 pub const PRODUCER_UUID: &str = "a6458b7d-87c3-4483-be96-854d814c20de";
-pub const RACK_SUBNET: &str = "fd00:1122:3344:01::/56";
+pub const RACK_SUBNET: &str = "fd00:1122:3344:0100::/56";
 
 /// Password for the user created by the test suite
 ///

diff --git a/nexus/tests/integration_tests/rack.rs b/nexus/tests/integration_tests/rack.rs
@@ -110,7 +110,7 @@ async fn test_sled_list_uninitialized(cptestctx: &ControlPlaneTestContext) {
     let baseboard = uninitialized_sleds.pop().unwrap().baseboard;
     let sled_uuid = Uuid::new_v4();
     let sa = SledAgentStartupInfo {
-        sa_address: "[fd00:1122:3344:01::1]:8080".parse().unwrap(),
+        sa_address: "[fd00:1122:3344:0100::1]:8080".parse().unwrap(),
         role: SledRole::Gimlet,
         baseboard: Baseboard {
             serial_number: baseboard.serial,

diff --git a/nexus/types/src/internal_api/params.rs b/nexus/types/src/internal_api/params.rs
@@ -263,7 +263,7 @@ pub struct RackInitializationRequest {
     /// The external qsfp ports per sidecar
     pub external_port_count: ExternalPortDiscovery,
     /// Initial rack network configuration
-    pub rack_network_config: Option<RackNetworkConfig>,
+    pub rack_network_config: RackNetworkConfig,
 }
 
 pub type DnsConfigParams = dns_service_client::types::DnsConfigParams;

diff --git a/openapi/bootstrap-agent.json b/openapi/bootstrap-agent.json
@@ -651,18 +651,13 @@
             }
           },
           "rack_network_config": {
-            "nullable": true,
             "description": "Initial rack network configuration",
             "allOf": [
               {
                 "$ref": "#/components/schemas/RackNetworkConfigV1"
               }
             ]
           },
-          "rack_subnet": {
-            "type": "string",
-            "format": "ipv6"
-          },
           "recovery_silo": {
             "description": "Configuration of the Recovery Silo (the initial Silo)",
             "allOf": [
@@ -688,7 +683,7 @@
           "external_dns_zone_name",
           "internal_services_ip_pool_ranges",
           "ntp_servers",
-          "rack_subnet",
+          "rack_network_config",
           "recovery_silo"
         ]
       },

diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json
@@ -5618,7 +5618,6 @@
             }
           },
           "rack_network_config": {
-            "nullable": true,
             "description": "Initial rack network configuration",
             "allOf": [
               {
@@ -5649,6 +5648,7 @@
           "external_port_count",
           "internal_dns_zone_config",
           "internal_services_ip_pool_ranges",
+          "rack_network_config",
           "recovery_silo",
           "services"
         ]

diff --git a/openapi/wicketd.json b/openapi/wicketd.json
@@ -1132,7 +1132,7 @@
             "nullable": true,
             "allOf": [
               {
-                "$ref": "#/components/schemas/RackNetworkConfigV1"
+                "$ref": "#/components/schemas/UserSpecifiedRackNetworkConfig"
               }
             ]
           }
@@ -2172,7 +2172,7 @@
             }
           },
           "rack_network_config": {
-            "$ref": "#/components/schemas/RackNetworkConfigV1"
+            "$ref": "#/components/schemas/UserSpecifiedRackNetworkConfig"
           }
         },
         "required": [
@@ -2190,46 +2190,6 @@
         "type": "string",
         "format": "uuid"
       },
-      "RackNetworkConfigV1": {
-        "description": "Initial network configuration",
-        "type": "object",
-        "properties": {
-          "bgp": {
-            "description": "BGP configurations for connecting the rack to external networks",
-            "type": "array",
-            "items": {
-              "$ref": "#/components/schemas/BgpConfig"
-            }
-          },
-          "infra_ip_first": {
-            "description": "First ip address to be used for configuring network infrastructure",
-            "type": "string",
-            "format": "ipv4"
-          },
-          "infra_ip_last": {
-            "description": "Last ip address to be used for configuring network infrastructure",
-            "type": "string",
-            "format": "ipv4"
-          },
-          "ports": {
-            "description": "Uplinks for connecting the rack to external networks",
-            "type": "array",
-            "items": {
-              "$ref": "#/components/schemas/PortConfigV1"
-            }
-          },
-          "rack_subnet": {
-            "$ref": "#/components/schemas/Ipv6Network"
-          }
-        },
-        "required": [
-          "bgp",
-          "infra_ip_first",
-          "infra_ip_last",
-          "ports",
-          "rack_subnet"
-        ]
-      },
       "RackOperationStatus": {
         "description": "Current status of any rack-level operation being performed by this bootstrap agent.\n\n<details><summary>JSON schema</summary>\n\n```json { \"description\": \"Current status of any rack-level operation being performed by this bootstrap agent.\", \"oneOf\": [ { \"type\": \"object\", \"required\": [ \"id\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackInitId\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"initializing\" ] } } }, { \"description\": \"`id` will be none if the rack was already initialized on startup.\", \"type\": \"object\", \"required\": [ \"status\" ], \"properties\": { \"id\": { \"allOf\": [ { \"$ref\": \"#/components/schemas/RackInitId\" } ] }, \"status\": { \"type\": \"string\", \"enum\": [ \"initialized\" ] } } }, { \"type\": \"object\", \"required\": [ \"id\", \"message\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackInitId\" }, \"message\": { \"type\": \"string\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"initialization_failed\" ] } } }, { \"type\": \"object\", \"required\": [ \"id\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackInitId\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"initialization_panicked\" ] } } }, { \"type\": \"object\", \"required\": [ \"id\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackResetId\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"resetting\" ] } } }, { \"description\": \"`reset_id` will be None if the rack is in an uninitialized-on-startup, or Some if it is in an uninitialized state due to a reset operation completing.\", \"type\": \"object\", \"required\": [ \"status\" ], \"properties\": { \"reset_id\": { \"allOf\": [ { \"$ref\": \"#/components/schemas/RackResetId\" } ] }, \"status\": { \"type\": \"string\", \"enum\": [ \"uninitialized\" ] } } }, { \"type\": \"object\", \"required\": [ \"id\", \"message\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackResetId\" }, \"message\": { \"type\": \"string\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"reset_failed\" ] } } }, { \"type\": \"object\", \"required\": [ \"id\", \"status\" ], \"properties\": { \"id\": { \"$ref\": \"#/components/schemas/RackResetId\" }, \"status\": { \"type\": \"string\", \"enum\": [ \"reset_panicked\" ] } } } ] } ``` </details>",
         "oneOf": [
@@ -4698,6 +4658,38 @@
           }
         ]
       },
+      "UserSpecifiedRackNetworkConfig": {
+        "description": "User-specified parts of [`RackNetworkConfig`](omicron_common::api::internal::shared::RackNetworkConfig).",
+        "type": "object",
+        "properties": {
+          "bgp": {
+            "type": "array",
+            "items": {
+              "$ref": "#/components/schemas/BgpConfig"
+            }
+          },
+          "infra_ip_first": {
+            "type": "string",
+            "format": "ipv4"
+          },
+          "infra_ip_last": {
+            "type": "string",
+            "format": "ipv4"
+          },
+          "ports": {
+            "type": "array",
+            "items": {
+              "$ref": "#/components/schemas/PortConfigV1"
+            }
+          }
+        },
+        "required": [
+          "bgp",
+          "infra_ip_first",
+          "infra_ip_last",
+          "ports"
+        ]
+      },
       "IgnitionCommand": {
         "description": "Ignition command.\n\n<details><summary>JSON schema</summary>\n\n```json { \"description\": \"Ignition command.\", \"type\": \"string\", \"enum\": [ \"power_on\", \"power_off\", \"power_reset\" ] } ``` </details>",
         "type": "string",

diff --git a/schema/rss-sled-plan.json b/schema/rss-sled-plan.json
@@ -466,7 +466,7 @@
         "external_dns_zone_name",
         "internal_services_ip_pool_ranges",
         "ntp_servers",
-        "rack_subnet",
+        "rack_network_config",
         "recovery_silo"
       ],
       "properties": {
@@ -521,19 +521,12 @@
         },
         "rack_network_config": {
           "description": "Initial rack network configuration",
-          "anyOf": [
+          "allOf": [
             {
               "$ref": "#/definitions/RackNetworkConfigV1"
-            },
-            {
-              "type": "null"
             }
           ]
         },
-        "rack_subnet": {
-          "type": "string",
-          "format": "ipv6"
-        },
         "recovery_silo": {
           "description": "Configuration of the Recovery Silo (the initial Silo)",
           "allOf": [

diff --git a/sled-agent/src/bootstrap/params.rs b/sled-agent/src/bootstrap/params.rs
@@ -14,7 +14,7 @@ use serde::{Deserialize, Serialize};
 use sha3::{Digest, Sha3_256};
 use sled_hardware::Baseboard;
 use std::borrow::Cow;
-use std::collections::HashSet;
+use std::collections::BTreeSet;
 use std::net::{IpAddr, Ipv6Addr, SocketAddrV6};
 use uuid::Uuid;
 
@@ -24,14 +24,13 @@ pub enum BootstrapAddressDiscovery {
     /// Ignore all bootstrap addresses except our own.
     OnlyOurs,
     /// Ignore all bootstrap addresses except the following.
-    OnlyThese { addrs: HashSet<Ipv6Addr> },
+    OnlyThese { addrs: BTreeSet<Ipv6Addr> },
 }
 
 // "Shadow" copy of `RackInitializeRequest` that does no validation on its
 // fields.
 #[derive(Clone, Deserialize)]
 struct UnvalidatedRackInitializeRequest {
-    rack_subnet: Ipv6Addr,
     trust_quorum_peers: Option<Vec<Baseboard>>,
     bootstrap_discovery: BootstrapAddressDiscovery,
     ntp_servers: Vec<String>,
@@ -41,7 +40,7 @@ struct UnvalidatedRackInitializeRequest {
     external_dns_zone_name: String,
     external_certificates: Vec<Certificate>,
     recovery_silo: RecoverySiloConfig,
-    rack_network_config: Option<RackNetworkConfig>,
+    rack_network_config: RackNetworkConfig,
 }
 
 /// Configuration for the "rack setup service".
@@ -53,8 +52,6 @@ struct UnvalidatedRackInitializeRequest {
 #[derive(Clone, Deserialize, Serialize, PartialEq, JsonSchema)]
 #[serde(try_from = "UnvalidatedRackInitializeRequest")]
 pub struct RackInitializeRequest {
-    pub rack_subnet: Ipv6Addr,
-
     /// The set of peer_ids required to initialize trust quorum
     ///
     /// The value is `None` if we are not using trust quorum
@@ -89,7 +86,7 @@ pub struct RackInitializeRequest {
     pub recovery_silo: RecoverySiloConfig,
 
     /// Initial rack network configuration
-    pub rack_network_config: Option<RackNetworkConfig>,
+    pub rack_network_config: RackNetworkConfig,
 }
 
 // This custom debug implementation hides the private keys.
@@ -98,7 +95,6 @@ impl std::fmt::Debug for RackInitializeRequest {
         // If you find a compiler error here, and you just added a field to this
         // struct, be sure to add it to the Debug impl below!
         let RackInitializeRequest {
-            rack_subnet,
             trust_quorum_peers: trust_qurorum_peers,
             bootstrap_discovery,
             ntp_servers,
@@ -112,7 +108,6 @@ impl std::fmt::Debug for RackInitializeRequest {
         } = &self;
 
         f.debug_struct("RackInitializeRequest")
-            .field("rack_subnet", rack_subnet)
             .field("trust_quorum_peers", trust_qurorum_peers)
             .field("bootstrap_discovery", bootstrap_discovery)
             .field("ntp_servers", ntp_servers)
@@ -155,7 +150,6 @@ impl TryFrom<UnvalidatedRackInitializeRequest> for RackInitializeRequest {
         }
 
         Ok(RackInitializeRequest {
-            rack_subnet: value.rack_subnet,
             trust_quorum_peers: value.trust_quorum_peers,
             bootstrap_discovery: value.bootstrap_discovery,
             ntp_servers: value.ntp_servers,
@@ -368,6 +362,7 @@ pub fn test_config() -> RackInitializeRequest {
 
 #[cfg(test)]
 mod tests {
+    use std::net::Ipv4Addr;
     use std::net::Ipv6Addr;
 
     use super::*;
@@ -395,7 +390,6 @@ mod tests {
     #[test]
     fn parse_rack_initialization_weak_hash() {
         let config = r#"
-            rack_subnet = "fd00:1122:3344:0100::"
             bootstrap_discovery.type = "only_ours"
             ntp_servers = [ "ntp.eng.oxide.computer" ]
             dns_servers = [ "1.1.1.1", "9.9.9.9" ]
@@ -480,7 +474,6 @@ mod tests {
         // Conjure up a config; we'll tweak the internal services pools and
         // external DNS IPs, but no other fields matter.
         let mut config = UnvalidatedRackInitializeRequest {
-            rack_subnet: Ipv6Addr::LOCALHOST,
             trust_quorum_peers: None,
             bootstrap_discovery: BootstrapAddressDiscovery::OnlyOurs,
             ntp_servers: Vec::new(),
@@ -494,7 +487,13 @@ mod tests {
                 user_name: "recovery".parse().unwrap(),
                 user_password_hash: "$argon2id$v=19$m=98304,t=13,p=1$RUlWc0ZxaHo0WFdrN0N6ZQ$S8p52j85GPvMhR/ek3GL0el/oProgTwWpHJZ8lsQQoY".parse().unwrap(),
             },
-            rack_network_config: None,
+            rack_network_config: RackNetworkConfig {
+                rack_subnet: Ipv6Addr::LOCALHOST.into(),
+                infra_ip_first: Ipv4Addr::LOCALHOST,
+                infra_ip_last: Ipv4Addr::LOCALHOST,
+                ports: Vec::new(),
+                bgp: Vec::new(),
+            },
         };
 
         // Valid configs: all external DNS IPs are contained in the IP pool