Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nexus] improve external messages and make more available to clients #4573

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions certificates/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
Expand Down
222 changes: 174 additions & 48 deletions common/src/api/external/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change in formatting intentional? It feels like kind of a big one but maybe I overestimate it. Do you happen to have any examples handy from testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't so much a formatting change as much as it is an addition. Previously, we just wouldn't track any internal context for the error variants that had associated external messages, and now we do.

In any case, this only affects internal logging; user-visible messages don't change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I misunderstood what this was used for. I saw that this comes from with_internal_context, which elsewhere I think prepends a context message, and I thought this was replacing that. I see that's basically still there at L110. So I think I was just confused -- sorry.

}
Ok(())
}
}

/// Indicates how an object was looked up (for an `ObjectNotFound` error)
Expand Down Expand Up @@ -119,7 +190,7 @@ impl Error {
| Error::InvalidRequest { .. }
| Error::InvalidValue { .. }
| Error::Forbidden
| Error::MethodNotAllowed { .. }
| Error::InsufficientCapacity { .. }
| Error::InternalError { .. }
| Error::TypeVersionMismatch { .. }
| Error::Conflict { .. } => false,
Expand Down Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general note: it feels like we've been backtracking on a lot of use of generics, especially in cross-crate interfaces, due to monomorphization causing long compile times. Does that apply here? Do we care?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's going to be at most 3 copies of this (String, &String, &str), and there isn't a big tree of nested functions underneath (where the true nastiness with compile times lies I think) so I doubt this is going to be an issue.

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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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!(
Expand All @@ -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 } => {
Expand All @@ -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),
},
}
}
Expand Down Expand Up @@ -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 } => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(applies to this whole function)

aside from the internal message, are any of these changing at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no changes.

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(
Expand All @@ -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,
)
}
}
}
}
Expand Down
13 changes: 5 additions & 8 deletions common/src/api/external/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"
))
);
}

Expand Down
16 changes: 8 additions & 8 deletions common/src/vlan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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()))?,
)
}
}
Loading
Loading