[MGS] Reject SP reset requests to our own sled (#8830)

Fixes #8768 by implementing "option 1" from that issue's proposed
solutions:

* Extends MGS's config file slightly to specify which _sled_ it's
running on (once it determines which scrimlet position it's in).
* Adds a config option to disallow SP reset requests to the local
scrimlet.
* Adds a custom dropshot error type for `sp_component_reset()` that can
cleanly report this particular kind of failure.

This is definitely the first custom dropshot error in MGS (although we
need some others: #8013, #8014, #8350), and maybe the first in omicron
more generally? The server side is pretty straightforward, I think, but
the client side ended up being a little messy. I'm not sure how much of
that is because of the rework I had to do to `try_all_serially()` vs the
inherent changes (e.g., the fact that we have a different progenitor
error type for this one endpoint).

I'll definitely put this on a racklette before merging, but this should
be close enough to review, and I'm very interested in feedback about
integrating the custom error type.

Author: jgallagher

Cargo.lock

@@ -23,7 +23,7 @@ use gateway_types::{
     task_dump::TaskDump,
     update::{
         HostPhase2Progress, HostPhase2RecoveryImageId, InstallinatorImageId,
-        SpUpdateStatus,
+        SpComponentResetError, SpUpdateStatus,
     },
 };
 use schemars::JsonSchema;
@@ -219,7 +219,7 @@ pub trait GatewayApi {
     async fn sp_component_reset(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponent>,
-    ) -> Result<HttpResponseUpdatedNoContent, HttpError>;
+    ) -> Result<HttpResponseUpdatedNoContent, SpComponentResetError>;
 
     /// Update an SP component
     ///

gateway-api/src/lib.rs

@@ -33,11 +33,32 @@ max_attempts_reset = 10
 # sleep time between UDP RPC resends (up to `max_attempts_*`)
 per_attempt_timeout_millis = 1000
 
-[switch.location]
-# possible locations where MGS could be running; these names appear in logs and
-# are used in the remainder of the `[switch.*]` configuration to define port
-# mappings
-names = ["switch0", "switch1"]
+# Possible locations where MGS could be running.
+#
+# The `name` of each location description will appear in logs and in the
+# remainder of the `[switch.*]` configuration to define port mappings.
+#
+# The `local_sled` of each location description specifies the `slot` of the
+# `SpIdentifier` for the sled on which this MGS is running. (The
+# `SpIdentifier::typ` value is implicitly `SpType::Sled`.)
+#
+# `allow_local_sled_sp_reset` determines whether MGS will accept a request to
+# perform an SP reset on its own local sled. This is dangerous during SP
+# updates, because the "reset" operation involves a watchdog that requires MGS
+# to send a "disarm the watchdog" message _after_ the reset, which it can't do
+# if it just powered itself off. In production, we set this to `false` for both
+# sleds; this means a scrimlet's SP can only be reset via MGS on the _other_
+# scrimlet. (We allow this to be `true` in tests and other dev environments
+# where rejecting reset attempts is too restrictive.)
+[[switch.location.description]]
+name = "switch0"
+local_sled = 0
+allow_local_sled_sp_reset = true
+
+[[switch.location.description]]
+name = "switch1"
+local_sled = 1
+allow_local_sled_sp_reset = true
 
 # `[[switch.location.determination]]` is a list of switch ports we should
 # contact in order to determine our location; each port defines a subset of
@@ -57,7 +78,7 @@ sp_port_2 = ["switch1"]
 # `[[switch.port.*]]` defines the local data link address (in RFD 250 terms, the
 # interface configured to use VLAN tag  assigned to the given port) and the
 # logical ID of the remote SP ("sled 7", "switch 1", etc.), which must have an
-# entry for each member of `[switch.location]` above.
+# entry for each member of `[[switch.location.description]]` above.
 #
 # TODO This section has some concessions to local testing: ultimately we will
 # use a single multicast address, target port, and source port, but for now all

gateway-test-utils/configs/config.test.toml

@@ -127,19 +127,23 @@ fn expected_location(
     sp_port: SpPort,
 ) -> String {
     let config = &config.switch.location;
-    let mut locations = config.names.iter().cloned().collect::<HashSet<_>>();
+    let mut locations = config
+        .description
+        .iter()
+        .map(|d| d.name.as_str())
+        .collect::<HashSet<_>>();
 
     for determination in &config.determination {
         let refined = match sp_port {
             SpPort::One => &determination.sp_port_1,
             SpPort::Two => &determination.sp_port_2,
         };
 
-        locations.retain(|name| refined.contains(name));
+        locations.retain(|name| refined.iter().any(|s| s == name));
     }
 
     assert_eq!(locations.len(), 1);
-    locations.into_iter().next().unwrap()
+    locations.into_iter().next().unwrap().to_string()
 }
 
 pub async fn test_setup_with_config(

gateway-test-utils/src/setup.rs

@@ -9,6 +9,7 @@ workspace = true
 
 [dependencies]
 daft.workspace = true
+dropshot.workspace = true
 gateway-messages.workspace = true
 # Avoid depending on gateway-sp-comms! It is a pretty heavy dependency and
 # would only be used for From impls anyway. We put those impls in
@@ -19,5 +20,6 @@ omicron-uuid-kinds.workspace = true
 omicron-workspace-hack.workspace = true
 schemars.workspace = true
 serde.workspace = true
+thiserror.workspace = true
 tufaceous-artifact.workspace = true
 uuid.workspace = true

gateway-types/Cargo.toml

@@ -2,15 +2,63 @@
 // 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/.
 
-use std::time::Duration;
-
+use dropshot::{ErrorStatusCode, HttpError, HttpResponseError};
 use gateway_messages::UpdateStatus;
 use omicron_uuid_kinds::MupdateUuid;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
+use std::time::Duration;
 use tufaceous_artifact::ArtifactHash;
 use uuid::Uuid;
 
+/// The error type returned by the `sp_component_reset()` MGS endpoint.
+#[derive(
+    Debug, Clone, PartialEq, Eq, Serialize, JsonSchema, thiserror::Error,
+)]
+#[serde(tag = "state", rename_all = "snake_case")]
+pub enum SpComponentResetError {
+    /// MGS refuses to reset its own sled's SP.
+    #[error("cannot reset SP of the sled hosting MGS")]
+    ResetSpOfLocalSled,
+
+    /// Other dropshot errors.
+    #[error("{internal_message}")]
+    Other {
+        message: String,
+        error_code: Option<String>,
+
+        // Skip serializing these fields, as they are used for the
+        // `fmt::Display` implementation and for determining the status code,
+        // respectively, rather than included in the response body:
+        #[serde(skip)]
+        internal_message: String,
+        #[serde(skip)]
+        status: ErrorStatusCode,
+    },
+}
+
+impl HttpResponseError for SpComponentResetError {
+    fn status_code(&self) -> ErrorStatusCode {
+        match self {
+            SpComponentResetError::ResetSpOfLocalSled => {
+                ErrorStatusCode::BAD_REQUEST
+            }
+            SpComponentResetError::Other { status, .. } => *status,
+        }
+    }
+}
+
+impl From<HttpError> for SpComponentResetError {
+    fn from(err: HttpError) -> Self {
+        Self::Other {
+            message: err.external_message,
+            error_code: err.error_code,
+            internal_message: err.internal_message,
+            status: err.status_code,
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize, JsonSchema)]
 #[serde(tag = "state", rename_all = "snake_case")]
 pub enum SpUpdateStatus {

gateway-types/src/update.rs

@@ -23,6 +23,7 @@ gateway-types.workspace = true
 hex.workspace = true
 http.workspace = true
 hyper.workspace = true
+iddqd.workspace = true
 illumos-utils.workspace = true
 ipcc.workspace = true
 omicron-common.workspace = true

gateway/Cargo.toml

@@ -28,11 +28,32 @@ max_attempts_reset = 30
 # sleep time between UDP RPC resends (up to `max_attempts_*`)
 per_attempt_timeout_millis = 2000
 
-[switch.location]
-# possible locations where MGS could be running; these names appear in logs and
-# are used in the remainder of the `[switch.*]` configuration to define port
-# mappings
-names = ["switch0", "switch1"]
+# Possible locations where MGS could be running.
+#
+# The `name` of each location description will appear in logs and in the
+# remainder of the `[switch.*]` configuration to define port mappings.
+#
+# The `local_sled` of each location description specifies the `slot` of the
+# `SpIdentifier` for the sled on which this MGS is running. (The
+# `SpIdentifier::typ` value is implicitly `SpType::Sled`.)
+#
+# `allow_local_sled_sp_reset` determines whether MGS will accept a request to
+# perform an SP reset on its own local sled. This is dangerous during SP
+# updates, because the "reset" operation involves a watchdog that requires MGS
+# to send a "disarm the watchdog" message _after_ the reset, which it can't do
+# if it just powered itself off. In production, we set this to `false` for both
+# sleds; this means a scrimlet's SP can only be reset via MGS on the _other_
+# scrimlet. (We allow this to be `true` in tests and other dev environments
+# where rejecting reset attempts is too restrictive.)
+[[switch.location.description]]
+name = "switch0"
+local_sled = 0
+allow_local_sled_sp_reset = true
+
+[[switch.location.description]]
+name = "switch1"
+local_sled = 1
+allow_local_sled_sp_reset = true
 
 # `[[switch.location.determination]]` is a list of switch ports we should
 # contact in order to determine our location; each port defines a subset of
@@ -52,7 +73,7 @@ sp_port_2 = ["switch1"]
 # `[[switch.port.*]]` defines the local data link address (in RFD 250 terms, the
 # interface configured to use VLAN tag  assigned to the given port) and the
 # logical ID of the remote SP ("sled 7", "switch 1", etc.), which must have an
-# entry for each member of `[switch.location]` above.
+# entry for each member of `[[switch.location.description]]` above.
 [[switch.port]]
 kind = "simulated"
 fake-interface = "fake-switch0"

gateway/examples/config.toml

@@ -50,6 +50,7 @@ use gateway_types::task_dump::TaskDump;
 use gateway_types::update::HostPhase2Progress;
 use gateway_types::update::HostPhase2RecoveryImageId;
 use gateway_types::update::InstallinatorImageId;
+use gateway_types::update::SpComponentResetError;
 use gateway_types::update::SpUpdateStatus;
 use omicron_uuid_kinds::GenericUuid;
 use std::io::Cursor;
@@ -467,23 +468,43 @@ impl GatewayApi for GatewayImpl {
     async fn sp_component_reset(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponent>,
-    ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
+    ) -> Result<HttpResponseUpdatedNoContent, SpComponentResetError> {
         let apictx = rqctx.context();
         let PathSpComponent { sp, component } = path.into_inner();
         let sp_id = sp.into();
         let handler = async {
-            let sp = apictx.mgmt_switch.sp(sp_id)?;
+            let sp = apictx.mgmt_switch.sp(sp_id).map_err(HttpError::from)?;
             let component = component_from_str(&component)?;
 
+            // Our config may specifically disallow resetting the SP of our
+            // local sled. This is because resetting our local SP after an
+            // update doesn't work: `reset_component_trigger` below will issue a
+            // "reset with watchdog", wait for the SP to come back, then send a
+            // "disarm the watchdog" message. But if we've reset our own local
+            // sled, we won't be alive to disarm the watchdog, which will result
+            // in the SP (erroneously) rolling back the update. (In production
+            // we always disallow this; it's a config option to support dev/test
+            // environments that don't need this.)
+            if component == SpComponent::SP_ITSELF
+                && !apictx
+                    .mgmt_switch
+                    .allowed_to_reset_sp(sp_id)
+                    .map_err(HttpError::from)?
+            {
+                return Err(SpComponentResetError::ResetSpOfLocalSled);
+            }
+
             sp.reset_component_prepare(component)
                 // We always want to run with the watchdog when resetting as
-                // disabling the watchdog should be considered a debug only feature
+                // disabling the watchdog should be considered a debug only
+                // feature
                 .and_then(|()| sp.reset_component_trigger(component, false))
                 .await
                 .map_err(|err| SpCommsError::SpCommunicationFailed {
                     sp: sp_id,
                     err,
-                })?;
+                })
+                .map_err(HttpError::from)?;
 
             Ok(HttpResponseUpdatedNoContent {})
         };

gateway/src/http_entrypoints.rs

@@ -20,6 +20,7 @@ use futures::StreamExt;
 use futures::stream::FuturesUnordered;
 use gateway_sp_comms::InMemoryHostPhase2Provider;
 pub use management_switch::LocationConfig;
+pub use management_switch::LocationDescriptionConfig;
 pub use management_switch::LocationDeterminationConfig;
 pub use management_switch::ManagementSwitch;
 pub use management_switch::RetryConfig;