return hash status instead of custom errors

Author: jgallagher

Cargo.lock

@@ -16,8 +16,7 @@ use gateway_types::{
         SpState,
     },
     component_details::SpComponentDetails,
-    error::ComponentFlashError,
-    host::HostStartupOptions,
+    host::{ComponentFirmwareHashStatus, HostStartupOptions},
     ignition::{IgnitionCommand, SpIgnitionInfo},
     rot::{RotCfpa, RotCfpaSlot, RotCmpa, RotState},
     sensor::SpSensorReading,
@@ -28,7 +27,7 @@ use gateway_types::{
     },
 };
 use schemars::JsonSchema;
-use serde::{Deserialize, Serialize};
+use serde::Deserialize;
 use uuid::Uuid;
 
 /// This endpoint is used to upload SP and ROT Hubris archives as well as phase 1 host OS
@@ -273,7 +272,7 @@ pub trait GatewayApi {
     async fn sp_component_hash_firmware_start(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponentFirmwareSlot>,
-    ) -> Result<HttpResponseUpdatedNoContent, ComponentFlashError>;
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError>;
 
     /// Get a computed hash of a given slot of a component.
     ///
@@ -288,7 +287,7 @@ pub trait GatewayApi {
     async fn sp_component_hash_firmware_get(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponentFirmwareSlot>,
-    ) -> Result<HttpResponseOk<ComponentFirmwareHash>, ComponentFlashError>;
+    ) -> Result<HttpResponseOk<ComponentFirmwareHashStatus>, HttpError>;
 
     /// Abort any in-progress update an SP component
     ///
@@ -659,8 +658,3 @@ pub struct PathSpIgnitionCommand {
     /// Ignition command to perform on the targeted SP.
     pub command: IgnitionCommand,
 }
-
-#[derive(Serialize, Deserialize, JsonSchema)]
-pub struct ComponentFirmwareHash {
-    pub sha256: [u8; 32],
-}

gateway-api/src/lib.rs

@@ -9,7 +9,6 @@ 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
@@ -21,6 +20,5 @@ 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

@@ -56,3 +56,18 @@ impl From<StartupOptions> for HostStartupOptions {
         }
     }
 }
+
+#[derive(Serialize, Deserialize, JsonSchema)]
+#[serde(tag = "status", rename_all = "snake_case")]
+pub enum ComponentFirmwareHashStatus {
+    /// The hash is not available; the client must issue a separate request to
+    /// begin calculating the hash.
+    HashNotCalculated,
+    /// The hash is currently being calculated; the client should sleep briefly
+    /// then check again.
+    ///
+    /// We expect this operation to take a handful of seconds in practice.
+    HashInProgress,
+    /// The hash of the given firmware slot.
+    Hashed { sha256: [u8; 32] },
+}

gateway-types/src/error.rs

@@ -7,7 +7,6 @@
 pub mod caboose;
 pub mod component;
 pub mod component_details;
-pub mod error;
 pub mod host;
 pub mod ignition;
 pub mod rot;

gateway-types/src/host.rs

@@ -6,12 +6,10 @@
 
 use crate::management_switch::SpIdentifier;
 use dropshot::HttpError;
-use gateway_messages::HfError;
 use gateway_messages::SpError;
 use gateway_sp_comms::BindError;
 pub use gateway_sp_comms::error::CommunicationError;
 use gateway_sp_comms::error::UpdateError;
-use gateway_types::error::ComponentFlashError;
 use slog_error_chain::InlineErrorChain;
 use slog_error_chain::SlogInlineError;
 use std::error::Error;
@@ -234,33 +232,3 @@ pub(crate) fn http_err_with_message(
         headers: None,
     }
 }
-
-/// Convert a `CommunicationError` with a given `sp` into a
-/// `ComponentFlashError`.
-pub(crate) fn map_component_flash_error(
-    sp: SpIdentifier,
-    err: CommunicationError,
-) -> ComponentFlashError {
-    if let CommunicationError::SpError(err) = &err {
-        if let SpError::Hf(err) = err {
-            match err {
-                HfError::HashUncalculated => {
-                    return ComponentFlashError::HashUncalculated;
-                }
-                HfError::RecalculateHash => {
-                    return ComponentFlashError::HashStale;
-                }
-                HfError::HashInProgress => {
-                    return ComponentFlashError::HashInProgress;
-                }
-                HfError::NotMuxedToSp
-                | HfError::BadAddress
-                | HfError::QspiTimeout
-                | HfError::QspiTransferError => (),
-            }
-        }
-    }
-    ComponentFlashError::from(HttpError::from(
-        SpCommsError::SpCommunicationFailed { sp, err },
-    ))
-}

gateway-types/src/lib.rs

@@ -9,7 +9,6 @@
 use crate::ServerContext;
 use crate::error::SpCommsError;
 use crate::http_err_with_message;
-use crate::map_component_flash_error;
 use base64::Engine;
 use dropshot::ApiDescription;
 use dropshot::HttpError;
@@ -24,8 +23,10 @@ use dropshot::WebsocketEndpointResult;
 use dropshot::WebsocketUpgrade;
 use futures::TryFutureExt;
 use gateway_api::*;
+use gateway_messages::HfError;
 use gateway_messages::RotBootInfo;
 use gateway_messages::SpComponent;
+use gateway_messages::SpError;
 use gateway_sp_comms::HostPhase2Provider;
 use gateway_sp_comms::VersionedSpState;
 use gateway_sp_comms::error::CommunicationError;
@@ -37,7 +38,7 @@ use gateway_types::component::SpComponentList;
 use gateway_types::component::SpIdentifier;
 use gateway_types::component::SpState;
 use gateway_types::component_details::SpComponentDetails;
-use gateway_types::error::ComponentFlashError;
+use gateway_types::host::ComponentFirmwareHashStatus;
 use gateway_types::host::HostStartupOptions;
 use gateway_types::ignition::SpIgnitionInfo;
 use gateway_types::rot::RotCfpa;
@@ -541,28 +542,27 @@ impl GatewayApi for GatewayImpl {
     async fn sp_component_hash_firmware_start(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponentFirmwareSlot>,
-    ) -> Result<HttpResponseUpdatedNoContent, ComponentFlashError> {
+    ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
         let apictx = rqctx.context();
 
         let PathSpComponentFirmwareSlot { sp, component, firmware_slot } =
             path.into_inner();
         let sp_id = sp.into();
         let handler = async {
-            let sp = apictx.mgmt_switch.sp(sp_id).map_err(HttpError::from)?;
+            let sp = apictx.mgmt_switch.sp(sp_id)?;
             let component = component_from_str(&component)?;
 
             if component != SpComponent::HOST_CPU_BOOT_FLASH {
-                return Err(ComponentFlashError::from(
-                    HttpError::for_bad_request(
-                        Some("RequestUnsupportedForComponent".to_string()),
-                        "Only the host boot flash can be hashed".into(),
-                    ),
+                return Err(HttpError::for_bad_request(
+                    Some("RequestUnsupportedForComponent".to_string()),
+                    "Only the host boot flash can be hashed".into(),
                 ));
             }
 
-            sp.start_host_flash_hash(firmware_slot)
-                .await
-                .map_err(|err| map_component_flash_error(sp_id, err))?;
+            // TODO-john catch hash in progress?
+            sp.start_host_flash_hash(firmware_slot).await.map_err(|err| {
+                SpCommsError::SpCommunicationFailed { sp: sp_id, err }
+            })?;
 
             Ok(HttpResponseUpdatedNoContent())
         };
@@ -572,32 +572,48 @@ impl GatewayApi for GatewayImpl {
     async fn sp_component_hash_firmware_get(
         rqctx: RequestContext<Self::Context>,
         path: Path<PathSpComponentFirmwareSlot>,
-    ) -> Result<HttpResponseOk<ComponentFirmwareHash>, ComponentFlashError>
-    {
+    ) -> Result<HttpResponseOk<ComponentFirmwareHashStatus>, HttpError> {
         let apictx = rqctx.context();
 
         let PathSpComponentFirmwareSlot { sp, component, firmware_slot } =
             path.into_inner();
         let sp_id = sp.into();
         let handler = async {
-            let sp = apictx.mgmt_switch.sp(sp_id).map_err(HttpError::from)?;
+            let sp = apictx.mgmt_switch.sp(sp_id)?;
             let component = component_from_str(&component)?;
 
             if component != SpComponent::HOST_CPU_BOOT_FLASH {
-                return Err(ComponentFlashError::from(
-                    HttpError::for_bad_request(
-                        Some("RequestUnsupportedForComponent".to_string()),
-                        "Only the host boot flash can be hashed".into(),
-                    ),
+                return Err(HttpError::for_bad_request(
+                    Some("RequestUnsupportedForComponent".to_string()),
+                    "Only the host boot flash can be hashed".into(),
                 ));
             }
 
-            let sha256 = sp
-                .get_host_flash_hash(firmware_slot)
-                .await
-                .map_err(|err| map_component_flash_error(sp_id, err))?;
+            let status = match sp.get_host_flash_hash(firmware_slot).await {
+                // success
+                Ok(sha256) => ComponentFirmwareHashStatus::Hashed { sha256 },
+
+                // expected failure: hash needs to be calculated (or
+                // recalculated; either way the client operation is the same)
+                Err(CommunicationError::SpError(SpError::Hf(
+                    HfError::HashUncalculated | HfError::RecalculateHash,
+                ))) => ComponentFirmwareHashStatus::HashNotCalculated,
+
+                // expected failure: hashing is currently in progress; client
+                // needs to wait and try again later
+                Err(CommunicationError::SpError(SpError::Hf(
+                    HfError::HashInProgress,
+                ))) => ComponentFirmwareHashStatus::HashInProgress,
+
+                // other errors are failures
+                Err(err) => {
+                    return Err(HttpError::from(
+                        SpCommsError::SpCommunicationFailed { sp: sp_id, err },
+                    ));
+                }
+            };
 
-            Ok(HttpResponseOk(ComponentFirmwareHash { sha256 }))
+            Ok(HttpResponseOk(status))
         };
         apictx.latencies.instrument_dropshot_handler(&rqctx, handler).await
     }

gateway/src/error.rs

@@ -8,13 +8,11 @@
 //! ask it to start hashing, then we'll get "still hashing" errors for a few
 //! seconds, then we'll get the hash result.
 
-use gateway_client::Error;
 use gateway_client::SpComponent;
-use gateway_client::types::ComponentFlashError;
+use gateway_client::types::ComponentFirmwareHashStatus;
 use gateway_client::types::SpType;
 use gateway_messages::SpPort;
 use gateway_test_utils::setup as mgs_setup;
-use slog_error_chain::InlineErrorChain;
 
 #[tokio::test]
 async fn test_host_phase1_hashing() {
@@ -34,23 +32,18 @@ async fn test_host_phase1_hashing() {
     // We haven't yet started hashing; we should get the error we expect for
     // both slots.
     for firmware_slot in [0, 1] {
-        match mgs_client
+        let status = mgs_client
             .sp_component_hash_firmware_get(
                 sp_type,
                 sp_slot,
                 sp_component,
                 firmware_slot,
             )
             .await
-        {
-            Err(Error::ErrorResponse(err)) => match err.into_inner() {
-                ComponentFlashError::HashUncalculated => (),
-                other => panic!("unexpected error response: {other:?}"),
-            },
-            Err(err) => {
-                panic!("unexpected error: {}", InlineErrorChain::new(&err))
-            }
-            Ok(resp) => panic!("unexpected success: {resp:?}"),
+            .expect("got firmware hash status");
+        match status.into_inner() {
+            ComponentFirmwareHashStatus::HashNotCalculated => (),
+            other => panic!("unexpected status: {other:?}"),
         }
     }
 }