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

feat(deployer): handle errors from corrupted resource data #1208

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions common/src/database.rs
Expand Up @@ -5,7 +5,7 @@ use strum::{Display, EnumString};
#[cfg(feature = "openapi")]
use utoipa::ToSchema;

#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Deserialize, Serialize, Eq, PartialEq)]
#[serde(rename_all = "lowercase")]
#[cfg_attr(feature = "openapi", derive(ToSchema))]
#[cfg_attr(feature = "openapi", schema(as = shuttle_common::database::Type))]
Expand All @@ -14,7 +14,7 @@ pub enum Type {
Shared(SharedEngine),
}

#[derive(Clone, Debug, Deserialize, Display, Serialize, EnumString, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Deserialize, Display, Serialize, EnumString, Eq, PartialEq)]
#[serde(rename_all = "lowercase")]
#[strum(serialize_all = "lowercase")]
#[cfg_attr(feature = "openapi", derive(ToSchema))]
Expand All @@ -24,7 +24,7 @@ pub enum AwsRdsEngine {
MariaDB,
}

#[derive(Clone, Debug, Deserialize, Display, Serialize, EnumString, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Deserialize, Display, Serialize, EnumString, Eq, PartialEq)]
#[serde(rename_all = "lowercase")]
#[strum(serialize_all = "lowercase")]
#[cfg_attr(feature = "openapi", derive(ToSchema))]
Expand Down
2 changes: 1 addition & 1 deletion common/src/resource.rs
Expand Up @@ -25,7 +25,7 @@ pub struct Response {
pub data: Value,
}

#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
#[derive(Clone, Copy, Debug, Deserialize, Serialize, Eq, PartialEq)]
#[serde(rename_all = "lowercase")]
#[cfg_attr(feature = "openapi", derive(ToSchema))]
#[cfg_attr(feature = "openapi", schema(as = shuttle_common::resource::Type))]
Expand Down
14 changes: 12 additions & 2 deletions deployer/src/deployment/run.rs
Expand Up @@ -304,10 +304,20 @@ async fn load(
let resources = resource_manager
.get_resources(&service_id, claim.clone())
.await
.unwrap()
.map_err(|err| Error::Load(err.to_string()))?
.resources
.into_iter()
.map(resource::Response::from)
.map(resource::Response::try_from)
// We ignore and trace the errors for resources with corrupted data, returning just the
// valid resources.
// TODO: investigate how the resource data can get corrupted.
.filter_map(|resource| {
resource
.map_err(|err| {
error!(error = ?err, "failed to parse resource data");
})
.ok()
})
.map(resource::Response::into_bytes)
.collect();

Expand Down
4 changes: 2 additions & 2 deletions deployer/src/handlers/error.rs
Expand Up @@ -23,8 +23,8 @@ pub enum Error {
},
#[error("{0}, try running `cargo shuttle deploy`")]
NotFound(String),
#[error("Custom error: {0}")]
Custom(#[from] anyhow::Error),
#[error("Internal error: {0}")]
Internal(#[from] anyhow::Error),
#[error("Missing header: {0}")]
MissingHeader(String),
}
Expand Down
12 changes: 11 additions & 1 deletion deployer/src/handlers/mod.rs
Expand Up @@ -303,7 +303,17 @@ pub async fn get_service_resources(
.await?
.resources
.into_iter()
.map(Into::into)
.map(shuttle_common::resource::Response::try_from)
// We ignore and trace the errors for resources with corrupted data, returning just the
// valid resources.
// TODO: investigate how the resource data can get corrupted.
.filter_map(|resource| {
resource
.map_err(|err| {
error!(error = ?err, "failed to parse resource data");
})
.ok()
})
.collect();

Ok(Json(resources))
Expand Down
48 changes: 32 additions & 16 deletions proto/src/lib.rs
Expand Up @@ -296,31 +296,47 @@ pub mod runtime {
}

pub mod resource_recorder {
use anyhow::Context;
use std::str::FromStr;

include!("generated/resource_recorder.rs");

impl From<record_request::Resource> for shuttle_common::resource::Response {
fn from(resource: record_request::Resource) -> Self {
shuttle_common::resource::Response {
r#type: shuttle_common::resource::Type::from_str(resource.r#type.as_str())
.expect("to have a valid resource string"),
impl TryFrom<record_request::Resource> for shuttle_common::resource::Response {
type Error = anyhow::Error;

fn try_from(resource: record_request::Resource) -> Result<Self, Self::Error> {
let r#type = shuttle_common::resource::Type::from_str(resource.r#type.as_str())
.map_err(anyhow::Error::msg)
.context("resource type should have a valid resource string")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Proper error handling. This is nice 🤩

let response = shuttle_common::resource::Response {
r#type,
config: serde_json::from_slice(&resource.config)
.expect("to have JSON valid config"),
data: serde_json::from_slice(&resource.data).expect("to have JSON valid data"),
}
.context(format!("{} resource config should be valid JSON", r#type))?,
data: serde_json::from_slice(&resource.data)
.context(format!("{} resource data should be valid JSON", r#type))?,
};

Ok(response)
}
}

impl From<Resource> for shuttle_common::resource::Response {
fn from(resource: Resource) -> Self {
shuttle_common::resource::Response {
r#type: shuttle_common::resource::Type::from_str(resource.r#type.as_str())
.expect("to have a valid resource string"),
impl TryFrom<Resource> for shuttle_common::resource::Response {
type Error = anyhow::Error;

fn try_from(resource: Resource) -> Result<Self, Self::Error> {
let r#type = shuttle_common::resource::Type::from_str(resource.r#type.as_str())
.map_err(anyhow::Error::msg)
.context("resource type should have a valid resource string")?;

let response = shuttle_common::resource::Response {
r#type,
config: serde_json::from_slice(&resource.config)
.expect("to have JSON valid config"),
data: serde_json::from_slice(&resource.data).expect("to have JSON valid data"),
}
.context(format!("{} resource config should be valid JSON", r#type))?,
data: serde_json::from_slice(&resource.data)
.context(format!("{} resource data should be valid JSON", r#type))?,
};

Ok(response)
}
}
}
2 changes: 1 addition & 1 deletion runtime/src/provisioner_factory.rs
Expand Up @@ -52,7 +52,7 @@ impl Factory for ProvisionerFactory {

let mut request = Request::new(DatabaseRequest {
project_name: self.service_name.to_string(),
db_type: Some(db_type.clone().into()),
db_type: Some(db_type.into()),
});

if let Some(claim) = &self.claim {
Expand Down