From 2a3db4154a73761b559f6e2d9bc2640de15d1c3d Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 8 Dec 2023 14:39:57 -0800 Subject: [PATCH] [nexus] improve external messages and make more available to clients (#4573) While developing #4520, I observed that we were producing a number of error messages that were: * 503 Service Unavailable, * With only an internal message attached * But where the message is both safe and useful to display to clients. This is my attempt to make the situation slightly better. To achieve this, I made a few changes: 1. Make all the client errors carry a new `MessagePair` struct, which consists of an external message. (Along the way, correct the definition of e.g. the `Conflict` variant: it actually is an external message, not an internal one.) 2. Define a new `InsufficientCapacity` variant that consists of both an external and an internal message. This variant resolves to a 507 Insufficient Storage error, and has a more helpful message than just "Service Unavailable". 3. Turn some current 503 errors into client errors so that the message is available externally. Looking for feedback on this approach! --- certificates/src/lib.rs | 16 +- common/src/api/external/error.rs | 222 ++++++++++++++---- common/src/api/external/mod.rs | 13 +- common/src/vlan.rs | 16 +- docs/http-status-codes.adoc | 3 +- nexus/db-model/src/instance_state.rs | 7 + nexus/db-model/src/semver_version.rs | 10 +- nexus/db-queries/src/db/datastore/disk.rs | 14 +- .../src/db/datastore/external_ip.rs | 16 +- nexus/db-queries/src/db/datastore/ip_pool.rs | 15 +- .../src/db/datastore/ipv4_nat_entry.rs | 12 +- nexus/db-queries/src/db/datastore/mod.rs | 4 +- nexus/db-queries/src/db/datastore/project.rs | 14 +- nexus/db-queries/src/db/datastore/saga.rs | 6 +- nexus/db-queries/src/db/datastore/silo.rs | 14 +- nexus/db-queries/src/db/datastore/sled.rs | 6 +- nexus/db-queries/src/db/datastore/vpc.rs | 39 ++- .../db-queries/src/db/queries/external_ip.rs | 28 ++- .../src/db/queries/region_allocation.rs | 10 +- nexus/src/app/address_lot.rs | 19 +- nexus/src/app/device_auth.rs | 4 +- nexus/src/app/disk.rs | 32 +-- nexus/src/app/external_endpoints.rs | 2 +- nexus/src/app/image.rs | 20 +- nexus/src/app/instance.rs | 75 +++--- nexus/src/app/rack.rs | 7 +- nexus/src/app/session.rs | 2 +- nexus/src/app/silo.rs | 38 ++- nexus/src/app/switch_interface.rs | 6 +- nexus/src/app/update/mod.rs | 12 +- nexus/src/app/vpc_router.rs | 23 +- nexus/src/external_api/http_entrypoints.rs | 5 +- nexus/tests/integration_tests/disks.rs | 6 +- nexus/tests/integration_tests/instances.rs | 16 +- .../tests/integration_tests/router_routes.rs | 2 +- nexus/tests/integration_tests/snapshots.rs | 2 +- .../integration_tests/volume_management.rs | 2 +- sled-agent/src/common/disk.rs | 26 +- sled-agent/src/instance.rs | 12 +- sled-agent/src/sim/collection.rs | 7 +- sled-agent/src/sim/instance.rs | 12 +- 41 files changed, 440 insertions(+), 355 deletions(-) diff --git a/certificates/src/lib.rs b/certificates/src/lib.rs index 6bd7fa32de..442a9cfdd5 100644 --- a/certificates/src/lib.rs +++ b/certificates/src/lib.rs @@ -60,14 +60,14 @@ impl From for Error { | InvalidValidationHostname(_) | ErrorValidatingHostname(_) | NoDnsNameMatchingHostname { .. } - | UnsupportedPurpose => Error::InvalidValue { - label: String::from("certificate"), - message: DisplayErrorChain::new(&error).to_string(), - }, - BadPrivateKey(_) => Error::InvalidValue { - label: String::from("private-key"), - message: DisplayErrorChain::new(&error).to_string(), - }, + | UnsupportedPurpose => Error::invalid_value( + "certificate", + DisplayErrorChain::new(&error).to_string(), + ), + BadPrivateKey(_) => Error::invalid_value( + "private-key", + DisplayErrorChain::new(&error).to_string(), + ), Unexpected(_) => Error::InternalError { internal_message: DisplayErrorChain::new(&error).to_string(), }, diff --git a/common/src/api/external/error.rs b/common/src/api/external/error.rs index e508b7ecba..2661db7bb6 100644 --- a/common/src/api/external/error.rs +++ b/common/src/api/external/error.rs @@ -35,16 +35,16 @@ pub enum Error { ObjectAlreadyExists { type_name: ResourceType, object_name: String }, /// The request was well-formed, but the operation cannot be completed given /// the current state of the system. - #[error("Invalid Request: {message}")] - InvalidRequest { message: String }, + #[error("Invalid Request: {}", .message.display_internal())] + InvalidRequest { message: MessagePair }, /// Authentication credentials were required but either missing or invalid. /// The HTTP status code is called "Unauthorized", but it's more accurate to /// call it "Unauthenticated". #[error("Missing or invalid credentials")] Unauthenticated { internal_message: String }, /// The specified input field is not valid. - #[error("Invalid Value: {label}, {message}")] - InvalidValue { label: String, message: String }, + #[error("Invalid Value: {label}, {}", .message.display_internal())] + InvalidValue { label: String, message: MessagePair }, /// The request is not authorized to perform the requested operation. #[error("Forbidden")] Forbidden, @@ -55,15 +55,86 @@ pub enum Error { /// The system (or part of it) is unavailable. #[error("Service Unavailable: {internal_message}")] ServiceUnavailable { internal_message: String }, - /// Method Not Allowed - #[error("Method Not Allowed: {internal_message}")] - MethodNotAllowed { internal_message: String }, + + /// There is insufficient capacity to perform the requested operation. + /// + /// This variant is translated to 507 Insufficient Storage, and it carries + /// both an external and an internal message. The external message is + /// intended for operator consumption and is intended to not leak any + /// implementation details. + #[error("Insufficient Capacity: {}", .message.display_internal())] + InsufficientCapacity { message: MessagePair }, #[error("Type version mismatch! {internal_message}")] TypeVersionMismatch { internal_message: String }, - #[error("Conflict: {internal_message}")] - Conflict { internal_message: String }, + #[error("Conflict: {}", .message.display_internal())] + Conflict { message: MessagePair }, +} + +/// Represents an error message which has an external component, along with +/// some internal context possibly attached to it. +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] +pub struct MessagePair { + external_message: String, + internal_context: String, +} + +impl MessagePair { + pub fn new(external_message: String) -> Self { + Self { external_message, internal_context: String::new() } + } + + pub fn new_full( + external_message: String, + internal_context: String, + ) -> Self { + Self { external_message, internal_context } + } + + pub fn external_message(&self) -> &str { + &self.external_message + } + + pub fn internal_context(&self) -> &str { + &self.internal_context + } + + fn with_internal_context(self, context: C) -> Self + where + C: Display + Send + Sync + 'static, + { + let internal_context = if self.internal_context.is_empty() { + context.to_string() + } else { + format!("{}: {}", context, self.internal_context) + }; + Self { external_message: self.external_message, internal_context } + } + + pub fn into_internal_external(self) -> (String, String) { + let internal = self.display_internal().to_string(); + (internal, self.external_message) + } + + // Do not implement `fmt::Display` for this enum because we don't want users to + // accidentally display the internal message to the client. Instead, use a + // private formatter. + fn display_internal(&self) -> MessagePairDisplayInternal<'_> { + MessagePairDisplayInternal(self) + } +} + +struct MessagePairDisplayInternal<'a>(&'a MessagePair); + +impl<'a> Display for MessagePairDisplayInternal<'a> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0.external_message)?; + if !self.0.internal_context.is_empty() { + write!(f, " (with internal context: {})", self.0.internal_context)?; + } + Ok(()) + } } /// Indicates how an object was looked up (for an `ObjectNotFound` error) @@ -119,7 +190,7 @@ impl Error { | Error::InvalidRequest { .. } | Error::InvalidValue { .. } | Error::Forbidden - | Error::MethodNotAllowed { .. } + | Error::InsufficientCapacity { .. } | Error::InternalError { .. } | Error::TypeVersionMismatch { .. } | Error::Conflict { .. } => false, @@ -151,8 +222,20 @@ impl Error { /// /// This should be used for failures due possibly to invalid client input /// or malformed requests. - pub fn invalid_request(message: &str) -> Error { - Error::InvalidRequest { message: message.to_owned() } + pub fn invalid_request(message: impl Into) -> Error { + Error::InvalidRequest { message: MessagePair::new(message.into()) } + } + + /// Generates an [`Error::InvalidValue`] error with the specific label and + /// message. + pub fn invalid_value( + label: impl Into, + message: impl Into, + ) -> Error { + Error::InvalidValue { + label: label.into(), + message: MessagePair::new(message.into()), + } } /// Generates an [`Error::ServiceUnavailable`] error with the specific @@ -166,6 +249,27 @@ impl Error { Error::ServiceUnavailable { internal_message: message.to_owned() } } + /// Generates an [`Error::InsufficientCapacity`] error with external and + /// and internal messages. + /// + /// This should be used for failures where there is insufficient capacity, + /// and where the caller must either take action or wait until capacity is + /// freed. + /// + /// In the future, we may want to provide more help here: e.g. a link to a + /// status or support page. + pub fn insufficient_capacity( + external_message: impl Into, + internal_message: impl Into, + ) -> Error { + Error::InsufficientCapacity { + message: MessagePair::new_full( + external_message.into(), + internal_message.into(), + ), + } + } + /// Generates an [`Error::TypeVersionMismatch`] with a specific message. /// /// TypeVersionMismatch errors are a specific type of error arising from differences @@ -186,8 +290,8 @@ impl Error { /// retried. The internal message should provide more information about the /// source of the conflict and possible actions the caller can take to /// resolve it (if any). - pub fn conflict(message: &str) -> Error { - Error::Conflict { internal_message: message.to_owned() } + pub fn conflict(message: impl Into) -> Error { + Error::Conflict { message: MessagePair::new(message.into()) } } /// Given an [`Error`] with an internal message, return the same error with @@ -201,9 +305,14 @@ impl Error { match self { Error::ObjectNotFound { .. } | Error::ObjectAlreadyExists { .. } - | Error::InvalidRequest { .. } - | Error::InvalidValue { .. } | Error::Forbidden => self, + Error::InvalidRequest { message } => Error::InvalidRequest { + message: message.with_internal_context(context), + }, + Error::InvalidValue { label, message } => Error::InvalidValue { + label, + message: message.with_internal_context(context), + }, Error::Unauthenticated { internal_message } => { Error::Unauthenticated { internal_message: format!( @@ -223,12 +332,9 @@ impl Error { ), } } - Error::MethodNotAllowed { internal_message } => { - Error::MethodNotAllowed { - internal_message: format!( - "{}: {}", - context, internal_message - ), + Error::InsufficientCapacity { message } => { + Error::InsufficientCapacity { + message: message.with_internal_context(context), } } Error::TypeVersionMismatch { internal_message } => { @@ -239,8 +345,8 @@ impl Error { ), } } - Error::Conflict { internal_message } => Error::Conflict { - internal_message: format!("{}: {}", context, internal_message), + Error::Conflict { message } => Error::Conflict { + message: message.with_internal_context(context), }, } } @@ -292,28 +398,29 @@ impl From for HttpError { internal_message, }, - Error::InvalidRequest { message } => HttpError::for_bad_request( - Some(String::from("InvalidRequest")), - message, - ), - - Error::InvalidValue { label, message } => { - let message = - format!("unsupported value for \"{}\": {}", label, message); - HttpError::for_bad_request( - Some(String::from("InvalidValue")), - message, - ) + Error::InvalidRequest { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::BAD_REQUEST, + error_code: Some(String::from("InvalidRequest")), + external_message, + internal_message, + } } - // TODO: RFC-7231 requires that 405s generate an Accept header to describe - // what methods are available in the response - Error::MethodNotAllowed { internal_message } => { - HttpError::for_client_error( - Some(String::from("MethodNotAllowed")), - http::StatusCode::METHOD_NOT_ALLOWED, + Error::InvalidValue { label, message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::BAD_REQUEST, + error_code: Some(String::from("InvalidValue")), + external_message: format!( + "unsupported value for \"{}\": {}", + label, external_message + ), internal_message, - ) + } } Error::Forbidden => HttpError::for_client_error( @@ -333,16 +440,35 @@ impl From for HttpError { ) } + Error::InsufficientCapacity { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + // Need to construct an `HttpError` explicitly to present both + // an internal and an external message. + HttpError { + status_code: http::StatusCode::INSUFFICIENT_STORAGE, + error_code: Some(String::from("InsufficientCapacity")), + external_message: format!( + "Insufficient capacity: {}", + external_message + ), + internal_message, + } + } + Error::TypeVersionMismatch { internal_message } => { HttpError::for_internal_error(internal_message) } - Error::Conflict { internal_message } => { - HttpError::for_client_error( - Some(String::from("Conflict")), - http::StatusCode::CONFLICT, + Error::Conflict { message } => { + let (internal_message, external_message) = + message.into_internal_external(); + HttpError { + status_code: http::StatusCode::CONFLICT, + error_code: Some(String::from("Conflict")), + external_message, internal_message, - ) + } } } } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 50516a5da4..a6d729593b 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -316,10 +316,7 @@ impl Name { /// `Name::try_from(String)` that marshals any error into an appropriate /// `Error`. pub fn from_param(value: String, label: &str) -> Result { - value.parse().map_err(|e| Error::InvalidValue { - label: String::from(label), - message: e, - }) + value.parse().map_err(|e| Error::invalid_value(label, e)) } /// Return the `&str` representing the actual name. @@ -2828,10 +2825,10 @@ mod test { assert!(result.is_err()); assert_eq!( result, - Err(Error::InvalidValue { - label: "the_name".to_string(), - message: "name requires at least one character".to_string() - }) + Err(Error::invalid_value( + "the_name", + "name requires at least one character" + )) ); } diff --git a/common/src/vlan.rs b/common/src/vlan.rs index 45776e09ac..5e5765ffe2 100644 --- a/common/src/vlan.rs +++ b/common/src/vlan.rs @@ -20,10 +20,10 @@ impl VlanID { /// Creates a new VLAN ID, returning an error if it is out of range. pub fn new(id: u16) -> Result { if VLAN_MAX < id { - return Err(Error::InvalidValue { - label: id.to_string(), - message: "Invalid VLAN value".to_string(), - }); + return Err(Error::invalid_value( + id.to_string(), + "Invalid VLAN value", + )); } Ok(Self(id)) } @@ -38,9 +38,9 @@ impl fmt::Display for VlanID { impl FromStr for VlanID { type Err = Error; fn from_str(s: &str) -> Result { - Self::new(s.parse().map_err(|e| Error::InvalidValue { - label: s.to_string(), - message: format!("{}", e), - })?) + Self::new( + s.parse::() + .map_err(|e| Error::invalid_value(s, e.to_string()))?, + ) } } diff --git a/docs/http-status-codes.adoc b/docs/http-status-codes.adoc index e02f9ea8a5..4628edcab5 100644 --- a/docs/http-status-codes.adoc +++ b/docs/http-status-codes.adoc @@ -18,7 +18,8 @@ This doc is aimed at the public API. For consistency, we should use the same er ** "403 Forbidden" is used when the user provided valid credentials (they were authenticated), but they're not authorized to access the resource, _and_ we don't mind telling them that the resource exists (e.g., accessing "/sleds"). ** "404 Not Found" is used when the user provided valid credentials (they were authenticated), but they're not authorized to access the resource, and they're not even allowed to know whether it exists (e.g., accessing a particular Project). * "500 Internal Server Error" is used for any kind of _bug_ or unhandled server-side condition. -* "503 Service unavailable" is used when the service (or an internal service on which the service depends) is overloaded or actually unavailable. +* "503 Service Unavailable" is used when the service (or an internal service on which the service depends) is overloaded or actually unavailable. +* "507 Insufficient Storage" is used if there isn't sufficient capacity available for a particular operation (for example, if there isn't enough disk space available to allocate a new virtual disk). There's more discussion about the 400-level and 500-level codes below. diff --git a/nexus/db-model/src/instance_state.rs b/nexus/db-model/src/instance_state.rs index 6baec7afbd..6b4c71da79 100644 --- a/nexus/db-model/src/instance_state.rs +++ b/nexus/db-model/src/instance_state.rs @@ -6,6 +6,7 @@ use super::impl_enum_wrapper; use omicron_common::api::external; use serde::Deserialize; use serde::Serialize; +use std::fmt; use std::io::Write; impl_enum_wrapper!( @@ -40,6 +41,12 @@ impl InstanceState { } } +impl fmt::Display for InstanceState { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.0) + } +} + impl From for sled_agent_client::types::InstanceState { fn from(s: InstanceState) -> Self { use external::InstanceState::*; diff --git a/nexus/db-model/src/semver_version.rs b/nexus/db-model/src/semver_version.rs index 966b436149..8e168e11a2 100644 --- a/nexus/db-model/src/semver_version.rs +++ b/nexus/db-model/src/semver_version.rs @@ -68,12 +68,10 @@ fn to_sortable_string(v: semver::Version) -> Result { let max = u64::pow(10, u32::from(PADDED_WIDTH)) - 1; if v.major > max || v.minor > max || v.patch > max { - return Err(external::Error::InvalidValue { - label: "version".to_string(), - message: format!( - "Major, minor, and patch version must be less than {max}" - ), - }); + return Err(external::Error::invalid_value( + "version", + format!("Major, minor, and patch version must be less than {max}"), + )); } let mut result = format!( diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index 26d439b350..94d950f86a 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -633,16 +633,12 @@ impl DataStore { // destroyed, don't throw an error. return Ok(disk); } else if !ok_to_delete_states.contains(disk_state.state()) { - return Err(Error::InvalidRequest { - message: format!( - "disk cannot be deleted in state \"{}\"", - disk.runtime_state.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "disk cannot be deleted in state \"{}\"", + disk.runtime_state.disk_state + ))); } else if disk_state.is_attached() { - return Err(Error::InvalidRequest { - message: String::from("disk is attached"), - }); + return Err(Error::invalid_request("disk is attached")); } else { // NOTE: This is a "catch-all" error case, more specific // errors should be preferred as they're more actionable. diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index e821082501..ddf396f871 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -219,9 +219,12 @@ impl DataStore { "Requested external IP address not available", )) } else { - TransactionError::CustomError(Error::invalid_request( - "No external IP addresses available", - )) + TransactionError::CustomError( + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), + ) } } DatabaseError(UniqueViolation, ..) if name.is_some() => { @@ -450,10 +453,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: "deletion failed due to concurrent modification" - .to_string(), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index fb300ef833..4497e3f2b4 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -194,11 +194,9 @@ impl DataStore { .optional() .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if range.is_some() { - return Err(Error::InvalidRequest { - message: - "IP Pool cannot be deleted while it contains IP ranges" - .to_string(), - }); + return Err(Error::invalid_request( + "IP Pool cannot be deleted while it contains IP ranges", + )); } // Delete the pool, conditional on the rcgen not having changed. This @@ -224,10 +222,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: "deletion failed due to concurrent modification" - .to_string(), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs index 1caf5617bb..a44fed4cdf 100644 --- a/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs +++ b/nexus/db-queries/src/db/datastore/ipv4_nat_entry.rs @@ -124,9 +124,7 @@ impl DataStore { if let Some(nat_entry) = result.first() { Ok(nat_entry.clone()) } else { - Err(Error::InvalidRequest { - message: "no matching records".to_string(), - }) + Err(Error::invalid_request("no matching records")) } } @@ -185,9 +183,7 @@ impl DataStore { if let Some(nat_entry) = result.first() { Ok(nat_entry.clone()) } else { - Err(Error::InvalidRequest { - message: "no matching records".to_string(), - }) + Err(Error::invalid_request("no matching records")) } } @@ -241,9 +237,7 @@ impl DataStore { match latest { Some(value) => Ok(value), - None => Err(Error::InvalidRequest { - message: "sequence table is empty!".to_string(), - }), + None => Err(Error::invalid_request("sequence table is empty!")), } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 2844285f40..761c3f995f 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -1046,7 +1046,7 @@ mod test { "Saw error: \'{err}\', but expected \'{expected}\'" ); - assert!(matches!(err, Error::ServiceUnavailable { .. })); + assert!(matches!(err, Error::InsufficientCapacity { .. })); } let _ = db.cleanup().await; @@ -1191,7 +1191,7 @@ mod test { "Saw error: \'{err}\', but expected \'{expected}\'" ); - assert!(matches!(err, Error::ServiceUnavailable { .. })); + assert!(matches!(err, Error::InsufficientCapacity { .. })); let _ = db.cleanup().await; logctx.cleanup_successful(); diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index ba0c64abfd..a9015ea943 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -78,9 +78,9 @@ macro_rules! generate_fn_to_ensure_none_in_project { "a" }; - return Err(Error::InvalidRequest { - message: format!("project to be deleted contains {article} {object}: {label}"), - }); + return Err(Error::invalid_request( + format!("project to be deleted contains {article} {object}: {label}") + )); } Ok(()) @@ -271,11 +271,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(err.bail(Error::InvalidRequest { - message: - "deletion failed due to concurrent modification" - .to_string(), - })); + return Err(err.bail(Error::invalid_request( + "deletion failed due to concurrent modification", + ))); } self.virtual_provisioning_collection_delete_on_connection( diff --git a/nexus/db-queries/src/db/datastore/saga.rs b/nexus/db-queries/src/db/datastore/saga.rs index 2ec0c40799..1cd41a9806 100644 --- a/nexus/db-queries/src/db/datastore/saga.rs +++ b/nexus/db-queries/src/db/datastore/saga.rs @@ -87,8 +87,8 @@ impl DataStore { match result.status { UpdateStatus::Updated => Ok(()), - UpdateStatus::NotUpdatedButExists => Err(Error::InvalidRequest { - message: format!( + UpdateStatus::NotUpdatedButExists => Err(Error::invalid_request( + format!( "failed to update saga {:?} with state {:?}: preconditions not met: \ expected current_sec = {:?}, adopt_generation = {:?}, \ but found current_sec = {:?}, adopt_generation = {:?}, state = {:?}", @@ -100,7 +100,7 @@ impl DataStore { result.found.adopt_generation, result.found.saga_state, ) - }), + )), } } diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index ab48ec458f..437c171fb0 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -351,9 +351,9 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; if project_found.is_some() { - return Err(Error::InvalidRequest { - message: "silo to be deleted contains a project".to_string(), - }); + return Err(Error::invalid_request( + "silo to be deleted contains a project", + )); } let now = Utc::now(); @@ -375,11 +375,9 @@ impl DataStore { })?; if updated_rows == 0 { - return Err(TxnError::CustomError(Error::InvalidRequest { - message: - "silo deletion failed due to concurrent modification" - .to_string(), - })); + return Err(TxnError::CustomError(Error::invalid_request( + "silo deletion failed due to concurrent modification", + ))); } self.virtual_provisioning_collection_delete_on_connection( diff --git a/nexus/db-queries/src/db/datastore/sled.rs b/nexus/db-queries/src/db/datastore/sled.rs index 023384a9bf..7b94d64418 100644 --- a/nexus/db-queries/src/db/datastore/sled.rs +++ b/nexus/db-queries/src/db/datastore/sled.rs @@ -217,8 +217,10 @@ impl DataStore { if let Some(err) = err.take() { match err { SledReservationError::NotFound => { - return external::Error::unavail( + return external::Error::insufficient_capacity( "No sleds can fit the requested instance", + "No sled targets found that had enough \ + capacity to fit the requested instance.", ); } } @@ -399,7 +401,7 @@ mod test { ) .await .unwrap_err(); - assert!(matches!(error, external::Error::ServiceUnavailable { .. })); + assert!(matches!(error, external::Error::InsufficientCapacity { .. })); // Now add a provisionable sled and try again. let sled_update = test_new_sled_update(); diff --git a/nexus/db-queries/src/db/datastore/vpc.rs b/nexus/db-queries/src/db/datastore/vpc.rs index 069ce63028..4f0245e283 100644 --- a/nexus/db-queries/src/db/datastore/vpc.rs +++ b/nexus/db-queries/src/db/datastore/vpc.rs @@ -339,7 +339,10 @@ impl DataStore { opctx.log, "failed to find a VNI after searching entire range"; ); - Err(Error::unavail("Failed to find a free VNI for this VPC")) + Err(Error::insufficient_capacity( + "No free virtual network was found", + "Failed to find a free VNI for this VPC", + )) } // Internal implementation for creating a VPC. @@ -469,11 +472,9 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? .is_some() { - return Err(Error::InvalidRequest { - message: String::from( - "VPC cannot be deleted while VPC Subnets exist", - ), - }); + return Err(Error::invalid_request( + "VPC cannot be deleted while VPC Subnets exist", + )); } // Delete the VPC, conditional on the subnet_gen not having changed. @@ -492,11 +493,9 @@ impl DataStore { ) })?; if updated_rows == 0 { - Err(Error::InvalidRequest { - message: String::from( - "deletion failed to to concurrent modification", - ), - }) + Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )) } else { Ok(()) } @@ -794,12 +793,10 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))? .is_some() { - return Err(Error::InvalidRequest { - message: String::from( - "VPC Subnet cannot be deleted while \ - network interfaces in the subnet exist", - ), - }); + return Err(Error::invalid_request( + "VPC Subnet cannot be deleted while network interfaces in the \ + subnet exist", + )); } // Delete the subnet, conditional on the rcgen not having changed. @@ -818,11 +815,9 @@ impl DataStore { ) })?; if updated_rows == 0 { - return Err(Error::InvalidRequest { - message: String::from( - "deletion failed to to concurrent modification", - ), - }); + return Err(Error::invalid_request( + "deletion failed due to concurrent modification", + )); } else { Ok(()) } diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 4e5f59e79c..2a76ea7408 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -997,9 +997,10 @@ mod tests { ); assert_eq!( err, - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); context.success().await; } @@ -1053,9 +1054,10 @@ mod tests { ); assert_eq!( res.unwrap_err(), - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); let res = context @@ -1075,9 +1077,10 @@ mod tests { ); assert_eq!( res.unwrap_err(), - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); context.success().await; } @@ -1306,9 +1309,10 @@ mod tests { .expect_err("Should have failed to allocate after pool exhausted"); assert_eq!( err, - Error::InvalidRequest { - message: String::from("No external IP addresses available"), - } + Error::insufficient_capacity( + "No external IP addresses available", + "NextExternalIp::new returned NotFound", + ), ); // But we should be able to allocate another SNat IP diff --git a/nexus/db-queries/src/db/queries/region_allocation.rs b/nexus/db-queries/src/db/queries/region_allocation.rs index 031be92c08..3c37bf6b2e 100644 --- a/nexus/db-queries/src/db/queries/region_allocation.rs +++ b/nexus/db-queries/src/db/queries/region_allocation.rs @@ -46,19 +46,23 @@ pub fn from_diesel(e: DieselError) -> external::Error { NOT_ENOUGH_UNIQUE_ZPOOLS_SENTINEL, ]; if let Some(sentinel) = matches_sentinel(&e, &sentinels) { + let external_message = "Not enough storage"; match sentinel { NOT_ENOUGH_DATASETS_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough datasets to allocate disks", ); } NOT_ENOUGH_ZPOOL_SPACE_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough zpool space to allocate disks. There may not be enough disks with space for the requested region. You may also see this if your rack is in a degraded state, or you're running the default multi-rack topology configuration in a 1-sled development environment.", ); } NOT_ENOUGH_UNIQUE_ZPOOLS_SENTINEL => { - return external::Error::unavail( + return external::Error::insufficient_capacity( + external_message, "Not enough unique zpools selected while allocating disks", ); } diff --git a/nexus/src/app/address_lot.rs b/nexus/src/app/address_lot.rs index b87ae1b09f..847021bdd4 100644 --- a/nexus/src/app/address_lot.rs +++ b/nexus/src/app/address_lot.rs @@ -94,10 +94,9 @@ fn validate_blocks(lot: ¶ms::AddressLotCreate) -> Result<(), Error> { validate_v6_block(first, last)? } _ => { - return Err(Error::InvalidRequest { - message: "Block bounds must be in same address family" - .into(), - }) + return Err(Error::invalid_request( + "Block bounds must be in same address family", + )); } } } @@ -106,18 +105,18 @@ fn validate_blocks(lot: ¶ms::AddressLotCreate) -> Result<(), Error> { fn validate_v4_block(first: &Ipv4Addr, last: &Ipv4Addr) -> Result<(), Error> { if first > last { - return Err(Error::InvalidRequest { - message: "Invalid range, first must be <= last".into(), - }); + return Err(Error::invalid_request( + "Invalid range, first must be <= last", + )); } Ok(()) } fn validate_v6_block(first: &Ipv6Addr, last: &Ipv6Addr) -> Result<(), Error> { if first > last { - return Err(Error::InvalidRequest { - message: "Invalid range, first must be <= last".into(), - }); + return Err(Error::invalid_request( + "Invalid range, first must be <= last", + )); } Ok(()) } diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index c9571ee91f..c70b339a36 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -114,9 +114,7 @@ impl super::Nexus { token, ) .await?; - Err(Error::InvalidRequest { - message: "device authorization request expired".to_string(), - }) + Err(Error::invalid_request("device authorization request expired")) } else { self.db_datastore .device_access_token_create( diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 5cfecc9f08..5dd49a2efb 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -140,48 +140,48 @@ impl super::Nexus { // Reject disks where the block size doesn't evenly divide the // total size if (params.size.to_bytes() % block_size) != 0 { - return Err(Error::InvalidValue { - label: String::from("size and block_size"), - message: format!( + return Err(Error::invalid_value( + "size and block_size", + format!( "total size must be a multiple of block size {}", block_size, ), - }); + )); } // Reject disks where the size isn't at least // MIN_DISK_SIZE_BYTES if params.size.to_bytes() < MIN_DISK_SIZE_BYTES as u64 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be at least {}", ByteCount::from(MIN_DISK_SIZE_BYTES) ), - }); + )); } // Reject disks where the MIN_DISK_SIZE_BYTES doesn't evenly // divide the size if (params.size.to_bytes() % MIN_DISK_SIZE_BYTES as u64) != 0 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be a multiple of {}", ByteCount::from(MIN_DISK_SIZE_BYTES) ), - }); + )); } // Reject disks where the size is greated than MAX_DISK_SIZE_BYTES if params.size.to_bytes() > MAX_DISK_SIZE_BYTES { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "total size must be less than {}", ByteCount::try_from(MAX_DISK_SIZE_BYTES).unwrap() ), - }); + )); } Ok(()) diff --git a/nexus/src/app/external_endpoints.rs b/nexus/src/app/external_endpoints.rs index f95c64d3eb..1ab33c5c9c 100644 --- a/nexus/src/app/external_endpoints.rs +++ b/nexus/src/app/external_endpoints.rs @@ -1539,7 +1539,7 @@ mod test { Err(Error::InvalidRequest { message }) => { assert_eq!(rx_label, "empty"); assert_eq!( - message, + message.external_message(), format!( "HTTP request for unknown server name {:?}", authority.host() diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs index 5e78b2a096..a7fe75a464 100644 --- a/nexus/src/app/image.rs +++ b/nexus/src/app/image.rs @@ -168,9 +168,11 @@ impl super::Nexus { // disk created from this image has to be larger than it. let size: u64 = 100 * 1024 * 1024; let size: external::ByteCount = - size.try_into().map_err(|e| Error::InvalidValue { - label: String::from("size"), - message: format!("size is invalid: {}", e), + size.try_into().map_err(|e| { + Error::invalid_value( + "size", + format!("size is invalid: {}", e), + ) })?; let new_image_volume = @@ -293,9 +295,9 @@ impl super::Nexus { ) .await } - ImageLookup::SiloImage(_) => Err(Error::InvalidRequest { - message: "Cannot promote a silo image".to_string(), - }), + ImageLookup::SiloImage(_) => { + Err(Error::invalid_request("Cannot promote a silo image")) + } } } @@ -321,9 +323,9 @@ impl super::Nexus { ) .await } - ImageLookup::ProjectImage(_) => Err(Error::InvalidRequest { - message: "Cannot demote a project image".to_string(), - }), + ImageLookup::ProjectImage(_) => { + Err(Error::invalid_request("Cannot demote a project image")) + } } } } diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 0edb2c5ea7..987a8ac794 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -211,13 +211,13 @@ impl super::Nexus { // Reject instances where the memory is not at least // MIN_MEMORY_BYTES_PER_INSTANCE if params.memory.to_bytes() < MIN_MEMORY_BYTES_PER_INSTANCE as u64 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be at least {}", ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE) ), - }); + )); } // Reject instances where the memory is not divisible by @@ -225,24 +225,24 @@ impl super::Nexus { if (params.memory.to_bytes() % MIN_MEMORY_BYTES_PER_INSTANCE as u64) != 0 { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be divisible by {}", ByteCount::from(MIN_MEMORY_BYTES_PER_INSTANCE) ), - }); + )); } // Reject instances where the memory is greater than the limit if params.memory.to_bytes() > MAX_MEMORY_BYTES_PER_INSTANCE { - return Err(Error::InvalidValue { - label: String::from("size"), - message: format!( + return Err(Error::invalid_value( + "size", + format!( "memory must be less than or equal to {}", ByteCount::try_from(MAX_MEMORY_BYTES_PER_INSTANCE).unwrap() ), - }); + )); } let saga_params = sagas::instance_create::Params { @@ -376,7 +376,7 @@ impl super::Nexus { } if instance.runtime().migration_id.is_some() { - return Err(Error::unavail("instance is already migrating")); + return Err(Error::conflict("instance is already migrating")); } // Kick off the migration saga @@ -785,12 +785,10 @@ impl super::Nexus { if allowed { Ok(InstanceStateChangeRequestAction::SendToSled(sled_id)) } else { - Err(Error::InvalidRequest { - message: format!( - "instance state cannot be changed from state \"{}\"", - effective_state - ), - }) + Err(Error::invalid_request(format!( + "instance state cannot be changed from state \"{}\"", + effective_state + ))) } } @@ -1231,10 +1229,9 @@ impl super::Nexus { // permissions on both) without verifying the shared hierarchy. To // mitigate that we verify that their parent projects have the same ID. if authz_project.id() != authz_project_disk.id() { - return Err(Error::InvalidRequest { - message: "disk must be in the same project as the instance" - .to_string(), - }); + return Err(Error::invalid_request( + "disk must be in the same project as the instance", + )); } // TODO(https://github.com/oxidecomputer/omicron/issues/811): @@ -1614,28 +1611,22 @@ impl super::Nexus { | InstanceState::Starting | InstanceState::Stopping | InstanceState::Stopped - | InstanceState::Failed => Err(Error::ServiceUnavailable { - internal_message: format!( - "cannot connect to serial console of instance in state \ - {:?}", - vmm.runtime.state.0 - ), - }), - InstanceState::Destroyed => Err(Error::ServiceUnavailable { - internal_message: format!( - "cannot connect to serial console of instance in state \ - {:?}", - InstanceState::Stopped), - }), + | InstanceState::Failed => { + Err(Error::invalid_request(format!( + "cannot connect to serial console of instance in state \"{}\"", + vmm.runtime.state.0, + ))) + } + InstanceState::Destroyed => Err(Error::invalid_request( + "cannot connect to serial console of destroyed instance", + )), } } else { - Err(Error::ServiceUnavailable { - internal_message: format!( - "instance is in state {:?} and has no active serial console \ + Err(Error::invalid_request(format!( + "instance is {} and has no active serial console \ server", - instance.runtime().nexus_state - ) - }) + instance.runtime().nexus_state + ))) } } diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 3804841feb..1643ac301d 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -211,10 +211,9 @@ impl super::Nexus { }; let rack_network_config = request.rack_network_config.as_ref().ok_or( - Error::InvalidRequest { - message: "cannot initialize a rack without a network config" - .into(), - }, + Error::invalid_request( + "cannot initialize a rack without a network config", + ), )?; self.db_datastore diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 891124e1ac..7adf1c9bdd 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -154,7 +154,7 @@ impl super::Nexus { | Error::Forbidden | Error::InternalError { .. } | Error::ServiceUnavailable { .. } - | Error::MethodNotAllowed { .. } + | Error::InsufficientCapacity { .. } | Error::TypeVersionMismatch { .. } | Error::Conflict { .. } => { Reason::UnknownError { source: error } diff --git a/nexus/src/app/silo.rs b/nexus/src/app/silo.rs index a6ffd8ef5e..f5f3fa00e7 100644 --- a/nexus/src/app/silo.rs +++ b/nexus/src/app/silo.rs @@ -822,25 +822,24 @@ impl super::Nexus { })?; let response = client.get(url).send().await.map_err(|e| { - Error::InvalidValue { - label: String::from("url"), - message: format!("error querying url: {}", e), - } + Error::invalid_value( + "url", + format!("error querying url: {e}"), + ) })?; if !response.status().is_success() { - return Err(Error::InvalidValue { - label: String::from("url"), - message: format!( - "querying url returned: {}", - response.status() - ), - }); + return Err(Error::invalid_value( + "url", + format!("querying url returned: {}", response.status()), + )); } - response.text().await.map_err(|e| Error::InvalidValue { - label: String::from("url"), - message: format!("error getting text from url: {}", e), + response.text().await.map_err(|e| { + Error::invalid_value( + "url", + format!("error getting text from url: {e}"), + ) })? } @@ -849,12 +848,11 @@ impl super::Nexus { &base64::engine::general_purpose::STANDARD, data, ) - .map_err(|e| Error::InvalidValue { - label: String::from("data"), - message: format!( - "error getting decoding base64 data: {}", - e - ), + .map_err(|e| { + Error::invalid_value( + "data", + format!("error getting decoding base64 data: {e}"), + ) })?; String::from_utf8_lossy(&bytes).into_owned() } diff --git a/nexus/src/app/switch_interface.rs b/nexus/src/app/switch_interface.rs index cfb0541742..0acb2b7fe7 100644 --- a/nexus/src/app/switch_interface.rs +++ b/nexus/src/app/switch_interface.rs @@ -95,9 +95,9 @@ impl super::Nexus { pub fn validate_switch_location(switch_location: &str) -> Result<(), Error> { if switch_location != "switch0" && switch_location != "switch1" { - return Err(Error::InvalidRequest { - message: "Switch location must be switch0 or switch1".into(), - }); + return Err(Error::invalid_request( + "Switch location must be switch0 or switch1", + )); } Ok(()) } diff --git a/nexus/src/app/update/mod.rs b/nexus/src/app/update/mod.rs index 5075e421ae..36d4dbcb9e 100644 --- a/nexus/src/app/update/mod.rs +++ b/nexus/src/app/update/mod.rs @@ -68,14 +68,10 @@ impl super::Nexus { opctx.authorize(authz::Action::Modify, &authz::FLEET).await?; let updates_config = self.updates_config.as_ref().ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; let base_url = self.tuf_base_url(opctx).await?.ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; let trusted_root = tokio::fs::read(&updates_config.trusted_root) .await @@ -158,9 +154,7 @@ impl super::Nexus { ) -> Result, Error> { let mut base_url = self.tuf_base_url(opctx).await?.ok_or_else(|| { - Error::InvalidRequest { - message: "updates system not configured".into(), - } + Error::invalid_request("updates system not configured") })?; if !base_url.ends_with('/') { base_url.push('/'); diff --git a/nexus/src/app/vpc_router.rs b/nexus/src/app/vpc_router.rs index 81577f88e8..523a450bbd 100644 --- a/nexus/src/app/vpc_router.rs +++ b/nexus/src/app/vpc_router.rs @@ -129,9 +129,7 @@ impl super::Nexus { // router kind cannot be changed, but it might be able to save us a // database round-trip. if db_router.kind == VpcRouterKind::System { - return Err(Error::MethodNotAllowed { - internal_message: "Cannot delete system router".to_string(), - }); + return Err(Error::invalid_request("Cannot delete system router")); } self.db_datastore.vpc_delete_router(opctx, &authz_router).await } @@ -229,14 +227,12 @@ impl super::Nexus { match db_route.kind.0 { RouterRouteKind::Custom | RouterRouteKind::Default => (), _ => { - return Err(Error::MethodNotAllowed { - internal_message: format!( - "routes of type {} from the system table of VPC {:?} \ + return Err(Error::invalid_request(format!( + "routes of type {} from the system table of VPC {:?} \ are not modifiable", - db_route.kind.0, - vpc.id() - ), - }) + db_route.kind.0, + vpc.id() + ))); } } self.db_datastore @@ -255,10 +251,9 @@ impl super::Nexus { // Only custom routes can be deleted // TODO Shouldn't this constraint be checked by the database query? if db_route.kind.0 != RouterRouteKind::Custom { - return Err(Error::MethodNotAllowed { - internal_message: "DELETE not allowed on system routes" - .to_string(), - }); + return Err(Error::invalid_request( + "DELETE not allowed on system routes", + )); } self.db_datastore.router_delete_route(opctx, &authz_route).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index a2e5f633df..a6fd7a3ccb 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -5398,10 +5398,7 @@ async fn role_list( WhichPage::First(..) => None, WhichPage::Next(RolePage { last_seen }) => { Some(last_seen.split_once('.').ok_or_else(|| { - Error::InvalidValue { - label: last_seen.clone(), - message: String::from("bad page token"), - } + Error::invalid_value(last_seen.clone(), "bad page token") })?) .map(|(s1, s2)| (s1.to_string(), s2.to_string())) } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index f7403275b1..807c054b64 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -992,7 +992,7 @@ async fn test_disk_backed_by_multiple_region_sets( .body(Some(&new_disk)) // TODO: this fails! the current allocation algorithm does not split // across datasets - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -1026,7 +1026,7 @@ async fn test_disk_too_big(cptestctx: &ControlPlaneTestContext) { NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&new_disk)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -1457,7 +1457,7 @@ async fn test_disk_size_accounting(cptestctx: &ControlPlaneTestContext) { NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&disk_two)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 33d4d15d23..9260006c81 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3190,7 +3190,7 @@ async fn test_instances_memory_greater_than_max_size( assert!(error.message.contains("memory must be less than")); } -async fn expect_instance_start_fail_unavailable( +async fn expect_instance_start_fail_507( client: &ClientTestContext, instance_name: &str, ) { @@ -3199,13 +3199,15 @@ async fn expect_instance_start_fail_unavailable( http::Method::POST, &get_instance_start_url(instance_name), ) - .expect_status(Some(http::StatusCode::SERVICE_UNAVAILABLE)); + .expect_status(Some(http::StatusCode::INSUFFICIENT_STORAGE)); NexusRequest::new(builder) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("Expected instance start to fail with SERVICE_UNAVAILABLE"); + .expect( + "Expected instance start to fail with 507 Insufficient Storage", + ); } async fn expect_instance_start_ok( @@ -3296,9 +3298,7 @@ async fn test_cannot_provision_instance_beyond_cpu_capacity( for config in &configs { match config.2 { Ok(_) => expect_instance_start_ok(client, config.0).await, - Err(_) => { - expect_instance_start_fail_unavailable(client, config.0).await - } + Err(_) => expect_instance_start_fail_507(client, config.0).await, } } @@ -3404,9 +3404,7 @@ async fn test_cannot_provision_instance_beyond_ram_capacity( for config in &configs { match config.2 { Ok(_) => expect_instance_start_ok(client, config.0).await, - Err(_) => { - expect_instance_start_fail_unavailable(client, config.0).await - } + Err(_) => expect_instance_start_fail_507(client, config.0).await, } } diff --git a/nexus/tests/integration_tests/router_routes.rs b/nexus/tests/integration_tests/router_routes.rs index 7a7a33d49d..10c594bba9 100644 --- a/nexus/tests/integration_tests/router_routes.rs +++ b/nexus/tests/integration_tests/router_routes.rs @@ -69,7 +69,7 @@ async fn test_router_routes(cptestctx: &ControlPlaneTestContext) { // It errors if you try to delete the default route let error: dropshot::HttpErrorResponseBody = NexusRequest::expect_failure( client, - StatusCode::METHOD_NOT_ALLOWED, + StatusCode::BAD_REQUEST, Method::DELETE, get_route_url("system", "default").as_str(), ) diff --git a/nexus/tests/integration_tests/snapshots.rs b/nexus/tests/integration_tests/snapshots.rs index a9ed1b7cb7..24b04bf718 100644 --- a/nexus/tests/integration_tests/snapshots.rs +++ b/nexus/tests/integration_tests/snapshots.rs @@ -793,7 +793,7 @@ async fn test_cannot_snapshot_if_no_space(cptestctx: &ControlPlaneTestContext) { }, disk: base_disk_name.into(), })) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 5454e1f68f..466cb5472e 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -363,7 +363,7 @@ async fn test_snapshot_prevents_other_disk( NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) .body(Some(&next_disk)) - .expect_status(Some(StatusCode::SERVICE_UNAVAILABLE)), + .expect_status(Some(StatusCode::INSUFFICIENT_STORAGE)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/sled-agent/src/common/disk.rs b/sled-agent/src/common/disk.rs index 57868937d0..54c56825eb 100644 --- a/sled-agent/src/common/disk.rs +++ b/sled-agent/src/common/disk.rs @@ -118,12 +118,10 @@ impl DiskStates { | DiskState::ImportingFromBulkWrites | DiskState::Destroyed | DiskState::Faulted => { - return Err(Error::InvalidRequest { - message: format!( - "cannot detach from {}", - self.current.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "cannot detach from {}", + self.current.disk_state + ))); } }; } @@ -134,9 +132,9 @@ impl DiskStates { // (which is a no-op anyway). DiskState::Attaching(id) | DiskState::Attached(id) => { if uuid != id { - return Err(Error::InvalidRequest { - message: "disk is already attached".to_string(), - }); + return Err(Error::invalid_request( + "disk is already attached", + )); } return Ok(None); } @@ -157,12 +155,10 @@ impl DiskStates { | DiskState::Detaching(_) | DiskState::Destroyed | DiskState::Faulted => { - return Err(Error::InvalidRequest { - message: format!( - "cannot attach from {}", - self.current.disk_state - ), - }); + return Err(Error::invalid_request(format!( + "cannot attach from {}", + self.current.disk_state + ))); } } } diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index a811678a48..057402c57a 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -863,13 +863,11 @@ impl Instance { } return Err(Error::Transition( - omicron_common::api::external::Error::Conflict { - internal_message: format!( - "wrong instance state generation: expected {}, got {}", - inner.state.instance().gen, - old_runtime.gen - ), - }, + omicron_common::api::external::Error::conflict(format!( + "wrong instance state generation: expected {}, got {}", + inner.state.instance().gen, + old_runtime.gen + )), )); } diff --git a/sled-agent/src/sim/collection.rs b/sled-agent/src/sim/collection.rs index bd6ed4aa90..8dae31863c 100644 --- a/sled-agent/src/sim/collection.rs +++ b/sled-agent/src/sim/collection.rs @@ -777,7 +777,7 @@ mod test { let error = disk.transition(DiskStateRequested::Attached(id2)).unwrap_err(); if let Error::InvalidRequest { message } = error { - assert_eq!("disk is already attached", message); + assert_eq!("disk is already attached", message.external_message()); } else { panic!("unexpected error type"); } @@ -829,7 +829,10 @@ mod test { let error = disk.transition(DiskStateRequested::Attached(id)).unwrap_err(); if let Error::InvalidRequest { message } = error { - assert_eq!("cannot attach from detaching", message); + assert_eq!( + "cannot attach from detaching", + message.external_message() + ); } else { panic!("unexpected error type"); } diff --git a/sled-agent/src/sim/instance.rs b/sled-agent/src/sim/instance.rs index 15ff83c969..8b00adce60 100644 --- a/sled-agent/src/sim/instance.rs +++ b/sled-agent/src/sim/instance.rs @@ -362,13 +362,11 @@ impl SimInstanceInner { } if self.state.instance().gen != old_runtime.gen { - return Err(Error::InvalidRequest { - message: format!( - "wrong Propolis ID generation: expected {}, got {}", - self.state.instance().gen, - old_runtime.gen - ), - }); + return Err(Error::invalid_request(format!( + "wrong Propolis ID generation: expected {}, got {}", + self.state.instance().gen, + old_runtime.gen + ))); } self.state.set_migration_ids(ids, Utc::now());