From 39000aab64ec628249963cfe91bd56b96a058616 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 24 Mar 2022 15:50:33 -0400 Subject: [PATCH 1/4] Return errors rather than panicking for callable APIs --- common/src/api/external/error.rs | 11 +++++++++++ sled-agent/src/sled_agent.rs | 18 ++++++++++++------ 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index f66de9cd301..939340d3753 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -51,6 +51,11 @@ pub enum Error { /// The system encountered an unhandled operational error. #[error("Internal Error: {internal_message}")] InternalError { internal_message: String }, + + /// The system encountered an error due to incomplete implementation. + #[error("Not Implemented")] + NotImplemented, + /// The system (or part of it) is unavailable. #[error("Service Unavailable: {internal_message}")] ServiceUnavailable { internal_message: String }, @@ -102,6 +107,7 @@ impl Error { | Error::InvalidValue { .. } | Error::Forbidden | Error::MethodNotAllowed { .. } + | Error::NotImplemented | Error::InternalError { .. } => false, } } @@ -219,6 +225,11 @@ impl From for HttpError { String::from("Forbidden"), ), + Error::NotImplemented => HttpError::for_status( + Some(String::from("Not Implemented")), + http::StatusCode::NOT_IMPLEMENTED, + ), + Error::InternalError { internal_message } => { HttpError::for_internal_error(internal_message) } diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 99bb25ec6d1..9018083fb1f 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -17,8 +17,8 @@ use crate::params::{ use crate::services::ServiceManager; use crate::storage_manager::StorageManager; use omicron_common::api::{ - internal::nexus::DiskRuntimeState, internal::nexus::InstanceRuntimeState, - internal::nexus::UpdateArtifact, + external::Error as ExternalError, internal::nexus::DiskRuntimeState, + internal::nexus::InstanceRuntimeState, internal::nexus::UpdateArtifact, }; use slog::Logger; use std::net::SocketAddr; @@ -54,12 +54,18 @@ pub enum Error { #[error("Error updating: {0}")] Download(#[from] crate::updates::Error), + + #[error("Not yet implemented")] + NotImplemented, } -impl From for omicron_common::api::external::Error { +impl From for ExternalError { fn from(err: Error) -> Self { - omicron_common::api::external::Error::InternalError { - internal_message: err.to_string(), + match err { + Error::NotImplemented => ExternalError::NotImplemented, + _ => ExternalError::InternalError { + internal_message: err.to_string(), + }, } } } @@ -198,7 +204,7 @@ impl SledAgent { _initial_state: DiskRuntimeState, _target: DiskStateRequested, ) -> Result { - todo!("Disk attachment not yet implemented"); + Err(Error::NotImplemented) } /// Downloads and applies an artifact. From 8280b204f5e2a8eee5bd2ceec113d91615ceaf52 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 25 Mar 2022 11:31:51 -0400 Subject: [PATCH 2/4] 501 -> 405 --- common/src/api/external/error.rs | 10 ---------- sled-agent/src/sled_agent.rs | 4 +++- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index 939340d3753..c8c65901954 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -52,10 +52,6 @@ pub enum Error { #[error("Internal Error: {internal_message}")] InternalError { internal_message: String }, - /// The system encountered an error due to incomplete implementation. - #[error("Not Implemented")] - NotImplemented, - /// The system (or part of it) is unavailable. #[error("Service Unavailable: {internal_message}")] ServiceUnavailable { internal_message: String }, @@ -107,7 +103,6 @@ impl Error { | Error::InvalidValue { .. } | Error::Forbidden | Error::MethodNotAllowed { .. } - | Error::NotImplemented | Error::InternalError { .. } => false, } } @@ -225,11 +220,6 @@ impl From for HttpError { String::from("Forbidden"), ), - Error::NotImplemented => HttpError::for_status( - Some(String::from("Not Implemented")), - http::StatusCode::NOT_IMPLEMENTED, - ), - Error::InternalError { internal_message } => { HttpError::for_internal_error(internal_message) } diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 9018083fb1f..7eb32e86fd6 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -62,7 +62,9 @@ pub enum Error { impl From for ExternalError { fn from(err: Error) -> Self { match err { - Error::NotImplemented => ExternalError::NotImplemented, + Error::NotImplemented => ExternalError::MethodNotAllowed { + internal_message: "Method not implemented".to_string(), + }, _ => ExternalError::InternalError { internal_message: err.to_string(), }, From df7645d24018e4900753474cfa54b5d9980f4a6f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 25 Mar 2022 11:32:45 -0400 Subject: [PATCH 3/4] newline --- common/src/api/external/error.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index c8c65901954..f66de9cd301 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -51,7 +51,6 @@ pub enum Error { /// The system encountered an unhandled operational error. #[error("Internal Error: {internal_message}")] InternalError { internal_message: String }, - /// The system (or part of it) is unavailable. #[error("Service Unavailable: {internal_message}")] ServiceUnavailable { internal_message: String }, From 9651f52d468393fcc53089e79a9cc97baa66c4fd Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 25 Mar 2022 16:03:32 -0400 Subject: [PATCH 4/4] Make it a 500 --- sled-agent/src/sled_agent.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index 7eb32e86fd6..f8bf8db31ff 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -62,7 +62,7 @@ pub enum Error { impl From for ExternalError { fn from(err: Error) -> Self { match err { - Error::NotImplemented => ExternalError::MethodNotAllowed { + Error::NotImplemented => ExternalError::InternalError { internal_message: "Method not implemented".to_string(), }, _ => ExternalError::InternalError {