[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

@@ -60,14 +60,14 @@ impl From<CertificateError> 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(),
             },

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<C>(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<String>) -> 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<String>,
+        message: impl Into<String>,
+    ) -> 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<String>,
+        internal_message: impl Into<String>,
+    ) -> 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<String>) -> 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<Error> 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<Error> 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,
-                )
+                }
             }
         }
     }

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<Name, Error> {
-        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"
+            ))
         );
     }
 

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<Self, Error> {
         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, Self::Err> {
-        Self::new(s.parse().map_err(|e| Error::InvalidValue {
-            label: s.to_string(),
-            message: format!("{}", e),
-        })?)
+        Self::new(
+            s.parse::<u16>()
+                .map_err(|e| Error::invalid_value(s, e.to_string()))?,
+        )
     }
 }