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

shuttle-hq · Sep 8, 2023 · d7b5b6a · d7b5b6a
1 parent fa86d5b
commit d7b5b6a
Show file tree

Hide file tree

Showing 7 changed files with 62 additions and 26 deletions.
diff --git a/common/src/database.rs b/common/src/database.rs
@@ -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))]
@@ -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))]
@@ -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))]

diff --git a/common/src/resource.rs b/common/src/resource.rs
@@ -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))]

diff --git a/deployer/src/deployment/run.rs b/deployer/src/deployment/run.rs
@@ -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();
 

diff --git a/deployer/src/handlers/error.rs b/deployer/src/handlers/error.rs
@@ -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),
 }

diff --git a/deployer/src/handlers/mod.rs b/deployer/src/handlers/mod.rs
@@ -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))

diff --git a/proto/src/lib.rs b/proto/src/lib.rs
@@ -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")?;
+            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)
         }
     }
 }
diff --git a/runtime/src/provisioner_factory.rs b/runtime/src/provisioner_factory.rs
@@ -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 {