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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from Sep 8, 2023
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8363b51
fix(deployer): handle corrupted resource data errors
oddgrd Sep 7, 2023
bda9a5f
refactor(deployer): remove explicit type
oddgrd Sep 7, 2023
dc63b20
Merge branch 'main' into feature/eng-1215-handle-errors-from-corrupte…
oddgrd Sep 7, 2023
d5ebba1
fix(runtime): clippy
oddgrd Sep 7, 2023
08af728
refactor(deployer): best effort conversion
oddgrd Sep 7, 2023
aacd602
fix(deployer): clippy
oddgrd Sep 7, 2023
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
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 9 additions & 2 deletions deployer/src/deployment/run.rs
View file
Open in desktop
Expand Up @@ -304,10 +304,17 @@ 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| {
resource::Response::try_from(resource).map_err(|err| Error::Load(err.to_string()))
})
// We collect into a Result so that if the response contains a resource with corrupted data,
// we terminate iteration and return error.
// TODO: investigate how the resource data can get corrupted.
.collect::<Result<Vec<_>>>()?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation errors on any corrupted resource, we could also tracing::error! the bad resource errors, and just return the OK ones. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think returning what converted successfully is a good idea. That way the corrupted ones can be recreated and fixed.

Copy link
Contributor Author

@oddgrd oddgrd Sep 7, 2023 •
edited

Choose a reason for hiding this comment

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

Refactored in 08af728, for deployment the user will see this error in their logs:

ERROR {error="database::shared::postgres resource config should be valid JSON\n\nCaused by:\n    expected `:` at line 1 column 13"} shuttle_deployer::deployment::run: failed to parse resource data

For the resource list the tracing error will not be recorded in the deployer sqlite (but maybe we will instrument it with deployment id for new logger?), I can only see it in the deployers docker logs:

2023-09-07T12:17:43.227645Z ERROR request{http.uri=/projects/postgres-rocket-app/services/postgres-rocket-app/resources http.method=GET request.path="/projects/:project_name/services/:service_name/resources" account.name="admin" request.params.service_name="postgres-rocket-app" request.params.project_name="postgres-rocket-app"}:get_service_resources{project_name=postgres-rocket-app service_name=postgres-rocket-app}: shuttle_deployer::handlers: failed to parse resource data error=database::shared::postgres resource config should be valid JSON

Caused by:
    expected `:` at line 1 column 13

.into_iter()
.map(resource::Response::into_bytes)
.collect();

Expand Down
4 changes: 2 additions & 2 deletions deployer/src/handlers/error.rs
View file
Open in desktop
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
10 changes: 8 additions & 2 deletions deployer/src/handlers/mod.rs
View file
Open in desktop
Expand Up @@ -303,8 +303,14 @@ pub async fn get_service_resources(
.await?
.resources
.into_iter()
.map(Into::into)
.collect();
.map(|resource| {
shuttle_common::resource::Response::try_from(resource)
.map_err(|err| anyhow::anyhow!(err.to_string()).into())
})
// We collect into a Result so that if the response contains a resource with corrupted
// data, we terminate iteration and return error.
// TODO: investigate how the resource data can get corrupted.
.collect::<Result<Vec<_>>>()?;

Ok(Json(resources))
} else {
Expand Down
48 changes: 32 additions & 16 deletions proto/src/lib.rs
View file
Open in desktop
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
View file
Open in desktop
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