diff --git a/Cargo.lock b/Cargo.lock index 4642285a74..1577327a91 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3822,14 +3822,14 @@ version = "0.3.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e30165d31df606f5726b090ec7592c308a0eaf61721ff64c9a3018e344a8753e" dependencies = [ - "portable-atomic 1.3.1", + "portable-atomic 1.3.2", ] [[package]] name = "portable-atomic" -version = "1.3.1" +version = "1.3.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1bbda379e6e462c97ea6afe9f6233619b202bbc4968d7caa6917788d2070a044" +checksum = "dc59d1bcc64fc5d021d67521f818db868368028108d37f0e98d74e33f68297b5" [[package]] name = "postgres-native-tls" @@ -4823,6 +4823,12 @@ version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f27f6278552951f1f2b8cf9da965d10969b2efdea95a6ec47987ab46edfe263a" +[[package]] +name = "similar" +version = "2.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "420acb44afdae038210c99e69aae24109f32f15500aa708e81d46c9f29d55fcf" + [[package]] name = "simple_asn1" version = "0.6.2" @@ -5016,6 +5022,7 @@ dependencies = [ "spin-build", "spin-common", "spin-config", + "spin-doctor", "spin-http", "spin-key-value", "spin-key-value-sqlite", @@ -5102,6 +5109,22 @@ dependencies = [ "wasmtime-wasi", ] +[[package]] +name = "spin-doctor" +version = "0.1.0" +dependencies = [ + "anyhow", + "async-trait", + "serde", + "similar", + "spin-loader", + "tempfile", + "tokio", + "toml 0.7.3", + "toml_edit 0.19.8", + "tracing", +] + [[package]] name = "spin-http" version = "1.2.0-pre0" @@ -5862,8 +5885,20 @@ checksum = "4fb9d890e4dc9298b70f740f615f2e05b9db37dce531f6b24fb77ac993f9f217" dependencies = [ "serde", "serde_spanned", - "toml_datetime", - "toml_edit", + "toml_datetime 0.5.1", + "toml_edit 0.18.1", +] + +[[package]] +name = "toml" +version = "0.7.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b403acf6f2bb0859c93c7f0d967cb4a75a7ac552100f9322faf64dc047669b21" +dependencies = [ + "serde", + "serde_spanned", + "toml_datetime 0.6.1", + "toml_edit 0.19.8", ] [[package]] @@ -5875,6 +5910,15 @@ dependencies = [ "serde", ] +[[package]] +name = "toml_datetime" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ab8ed2edee10b50132aed5f331333428b011c99402b5a534154ed15746f9622" +dependencies = [ + "serde", +] + [[package]] name = "toml_edit" version = "0.18.1" @@ -5885,7 +5929,20 @@ dependencies = [ "nom8", "serde", "serde_spanned", - "toml_datetime", + "toml_datetime 0.5.1", +] + +[[package]] +name = "toml_edit" +version = "0.19.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "239410c8609e8125456927e6707163a3b1fdb40561e4b803bc041f466ccfdc13" +dependencies = [ + "indexmap", + "serde", + "serde_spanned", + "toml_datetime 0.6.1", + "winnow", ] [[package]] @@ -7033,6 +7090,15 @@ version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1a515f5799fe4961cb532f983ce2b23082366b898e52ffbce459c86f67c8378a" +[[package]] +name = "winnow" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae8970b36c66498d8ff1d66685dc86b91b29db0c7739899012f63a63814b4b28" +dependencies = [ + "memchr", +] + [[package]] name = "winreg" version = "0.10.1" diff --git a/Cargo.toml b/Cargo.toml index 61795d68cb..de078ba475 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,6 +52,7 @@ spin-bindle = { path = "crates/bindle" } spin-build = { path = "crates/build" } spin-common = { path = "crates/common" } spin-config = { path = "crates/config" } +spin-doctor = { path = "crates/doctor" } spin-http = { path = "crates/http" } spin-trigger-http = { path = "crates/trigger-http" } spin-loader = { path = "crates/loader" } diff --git a/crates/doctor/Cargo.toml b/crates/doctor/Cargo.toml new file mode 100644 index 0000000000..c4b223b01f --- /dev/null +++ b/crates/doctor/Cargo.toml @@ -0,0 +1,18 @@ +[package] +name = "spin-doctor" +version = "0.1.0" +edition = "2021" + +[dependencies] +anyhow = "1" +async-trait = "0.1" +serde = { version = "1", features = ["derive"] } +similar = "2" +spin-loader = { path = "../loader" } +tokio = "1" +toml = "0.7" +toml_edit = "0.19" +tracing = { workspace = true } + +[dev-dependencies] +tempfile = "3" diff --git a/crates/doctor/src/lib.rs b/crates/doctor/src/lib.rs new file mode 100644 index 0000000000..7f11803db5 --- /dev/null +++ b/crates/doctor/src/lib.rs @@ -0,0 +1,184 @@ +//! Spin doctor: check and automatically fix problems with Spin apps. +#![deny(missing_docs)] + +use std::{fmt::Debug, fs, future::Future, path::PathBuf, pin::Pin, sync::Arc}; + +use anyhow::{ensure, Context, Result}; +use async_trait::async_trait; +use tokio::sync::Mutex; +use toml_edit::Document; + +/// Diagnoses for app manifest format problems. +pub mod manifest; +/// Test helpers. +pub mod test; +/// Diagnoses for Wasm source problems. +pub mod wasm; + +/// Configuration for an app to be checked for problems. +pub struct Checkup { + manifest_path: PathBuf, + diagnostics: Vec>, +} + +impl Checkup { + /// Return a new checkup for the app manifest at the given path. + pub fn new(manifest_path: impl Into) -> Self { + let mut checkup = Self { + manifest_path: manifest_path.into(), + diagnostics: vec![], + }; + checkup.add_diagnostic::(); + checkup.add_diagnostic::(); + checkup.add_diagnostic::(); + checkup + } + + /// Add a detectable problem to this checkup. + pub fn add_diagnostic(&mut self) -> &mut Self { + self.diagnostics.push(Box::::default()); + self + } + + fn patient(&self) -> Result { + let path = &self.manifest_path; + ensure!( + path.is_file(), + "No Spin app manifest file found at {path:?}" + ); + + let contents = fs::read_to_string(path) + .with_context(|| format!("Couldn't read Spin app manifest file at {path:?}"))?; + + let manifest_doc: Document = contents + .parse() + .with_context(|| format!("Couldn't parse manifest file at {path:?} as valid TOML"))?; + + Ok(PatientApp { + manifest_path: path.into(), + manifest_doc, + }) + } + + /// Find problems with the configured app, calling the given closure with + /// each problem found. + pub async fn for_each_diagnosis(&self, mut f: F) -> Result + where + F: for<'a> FnMut( + Box, + &'a mut PatientApp, + ) -> Pin> + 'a>>, + { + let patient = Arc::new(Mutex::new(self.patient()?)); + let mut count = 0; + for diagnostic in &self.diagnostics { + let patient = patient.clone(); + let diags = diagnostic + .diagnose_boxed(&*patient.lock().await) + .await + .unwrap_or_else(|err| { + tracing::debug!("Diagnose failed: {err:?}"); + vec![] + }); + count += diags.len(); + for diag in diags { + let mut patient = patient.lock().await; + f(diag, &mut patient).await?; + } + } + Ok(count) + } +} + +/// An app "patient" to be checked for problems. +#[derive(Clone)] +pub struct PatientApp { + /// Path to an app manifest file. + pub manifest_path: PathBuf, + /// Parsed app manifest TOML document. + pub manifest_doc: Document, +} + +/// The Diagnose trait implements the detection of a particular Spin app problem. +#[async_trait] +pub trait Diagnostic: Send + Sync { + /// A [`Diagnosis`] representing the problem(s) this can detect. + type Diagnosis: Diagnosis; + + /// Check the given [`Patient`], returning any problem(s) found. + /// + /// If multiple _independently addressable_ problems are found, this may + /// return multiple instances. If two "logically separate" problems would + /// have the same fix, they should be represented with the same instance. + async fn diagnose(&self, patient: &PatientApp) -> Result>; +} + +/// The Diagnosis trait represents a detected problem with a Spin app. +pub trait Diagnosis: Debug + Send + Sync + 'static { + /// Return a human-friendly description of this problem. + fn description(&self) -> String; + + /// Return true if this problem is "critical", i.e. if the app's + /// configuration or environment is invalid. Return false for + /// "non-critical" problems like deprecations. + fn is_critical(&self) -> bool { + true + } + + /// Return a [`Treatment`] that can (potentially) fix this problem, or + /// None if there is no automatic fix. + fn treatment(&self) -> Option<&dyn Treatment> { + None + } +} + +/// The Treatment trait represents a (potential) fix for a detected problem. +#[async_trait] +pub trait Treatment: Sync { + /// Return a short (single line) description of what this fix will do, as + /// an imperative, e.g. "Upgrade the library". + fn summary(&self) -> String; + + /// Return a detailed description of what this fix will do, such as a file + /// diff or list of commands to be executed. + /// + /// May return `Err(DryRunNotSupported.into())` if no such description is + /// available, which is the default implementation. + async fn dry_run(&self, patient: &PatientApp) -> Result { + let _ = patient; + Err(DryRunNotSupported.into()) + } + + /// Attempt to fix this problem. Return Ok only if the problem is + /// successfully fixed. + async fn treat(&self, patient: &mut PatientApp) -> Result<()>; +} + +/// Error returned by [`Treatment::dry_run`] if dry run isn't supported. +#[derive(Debug)] +pub struct DryRunNotSupported; + +impl std::fmt::Display for DryRunNotSupported { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "dry run not implemented for this treatment") + } +} + +impl std::error::Error for DryRunNotSupported {} + +#[async_trait] +trait BoxingDiagnostic { + async fn diagnose_boxed(&self, patient: &PatientApp) -> Result>>; +} + +#[async_trait] +impl BoxingDiagnostic for Factory { + async fn diagnose_boxed(&self, patient: &PatientApp) -> Result>> { + Ok(self + .diagnose(patient) + .await? + .into_iter() + .map(|diag| Box::new(diag) as Box) + .collect()) + } +} diff --git a/crates/doctor/src/manifest.rs b/crates/doctor/src/manifest.rs new file mode 100644 index 0000000000..bbb14943ca --- /dev/null +++ b/crates/doctor/src/manifest.rs @@ -0,0 +1,50 @@ +use std::fs; + +use anyhow::{Context, Result}; +use async_trait::async_trait; +use toml_edit::Document; + +use crate::Treatment; + +/// Diagnose app manifest trigger config problems. +pub mod trigger; +/// Diagnose app manifest version problems. +pub mod version; + +/// ManifestTreatment helps implement [`Treatment`]s for app manifest problems. +#[async_trait] +pub trait ManifestTreatment { + /// Return a short (single line) description of what this fix will do, as + /// an imperative, e.g. "Add default trigger config". + fn summary(&self) -> String; + + /// Attempt to fix this problem. See [`Treatment::treat`]. + async fn treat_manifest(&self, doc: &mut Document) -> Result<()>; +} + +#[async_trait] +impl Treatment for T { + fn summary(&self) -> String { + ManifestTreatment::summary(self) + } + + async fn dry_run(&self, patient: &crate::PatientApp) -> Result { + let mut after_doc = patient.manifest_doc.clone(); + self.treat_manifest(&mut after_doc).await?; + let before = patient.manifest_doc.to_string(); + let after = after_doc.to_string(); + let diff = similar::udiff::unified_diff(Default::default(), &before, &after, 1, None); + Ok(format!( + "Apply the following diff to {:?}:\n{}", + patient.manifest_path, diff + )) + } + + async fn treat(&self, patient: &mut crate::PatientApp) -> Result<()> { + let doc = &mut patient.manifest_doc; + self.treat_manifest(doc).await?; + let path = &patient.manifest_path; + fs::write(path, doc.to_string()) + .with_context(|| format!("failed to write fixed manifest to {path:?}")) + } +} diff --git a/crates/doctor/src/manifest/trigger.rs b/crates/doctor/src/manifest/trigger.rs new file mode 100644 index 0000000000..1bf0e62f18 --- /dev/null +++ b/crates/doctor/src/manifest/trigger.rs @@ -0,0 +1,255 @@ +use anyhow::{bail, ensure, Context, Result}; +use async_trait::async_trait; +use toml::Value; +use toml_edit::{Document, InlineTable, Item, Table}; + +use crate::{Diagnosis, Diagnostic, PatientApp, Treatment}; + +use super::ManifestTreatment; + +/// TriggerDiagnostic detects problems with app trigger config. +#[derive(Default)] +pub struct TriggerDiagnostic; + +#[async_trait] +impl Diagnostic for TriggerDiagnostic { + type Diagnosis = TriggerDiagnosis; + + async fn diagnose(&self, patient: &PatientApp) -> Result> { + let manifest: toml::Value = toml_edit::de::from_document(patient.manifest_doc.clone())?; + + let mut diags = vec![]; + + // Top-level trigger config + diags.extend(TriggerDiagnosis::for_app_trigger(manifest.get("trigger"))); + + // Component-level HTTP trigger config + let trigger_type = manifest + .get("trigger") + .and_then(|item| item.get("type")) + .and_then(|item| item.as_str()); + if let Some("http") = trigger_type { + if let Some(Value::Array(components)) = manifest.get("component") { + let single_component = components.len() == 1; + for component in components { + let id = component + .get("id") + .and_then(|value| value.as_str()) + .unwrap_or("") + .to_string(); + diags.extend(TriggerDiagnosis::for_http_component_trigger( + id, + component.get("trigger"), + single_component, + )); + } + } + } + + Ok(diags) + } +} + +/// TriggerDiagnosis represents a problem with app trigger config. +#[derive(Debug)] +pub enum TriggerDiagnosis { + /// Missing app trigger section + MissingAppTrigger, + /// Invalid app trigger config + InvalidAppTrigger(&'static str), + /// HTTP trigger missing base field + HttpAppTriggerMissingBase, + /// HTTP component trigger missing route field + HttpComponentTriggerMissingRoute(String, bool), + /// Invalid HTTP component trigger config + InvalidHttpComponentTrigger(String, &'static str), +} + +impl TriggerDiagnosis { + fn for_app_trigger(trigger: Option<&Value>) -> Option { + let Some(trigger) = trigger else { + return Some(Self::MissingAppTrigger); + }; + let Some(trigger) = trigger.as_table() else { + return Some(Self::InvalidAppTrigger("not a table")); + }; + let Some(trigger_type) = trigger.get("type") else { + return Some(Self::InvalidAppTrigger("trigger table missing type")); + }; + let Some(trigger_type) = trigger_type.as_str() else { + return Some(Self::InvalidAppTrigger("type must be a string")); + }; + if trigger_type == "http" && trigger.get("base").is_none() { + return Some(Self::HttpAppTriggerMissingBase); + } + None + } + + fn for_http_component_trigger( + id: String, + trigger: Option<&Value>, + single_component: bool, + ) -> Option { + let Some(trigger) = trigger else { + return Some(Self::HttpComponentTriggerMissingRoute(id, single_component)); + }; + let Some(trigger) = trigger.as_table() else { + return Some(Self::InvalidHttpComponentTrigger(id, "not a table")); + }; + let Some(route) = trigger.get("route") else { + return Some(Self::HttpComponentTriggerMissingRoute(id, single_component)); + }; + if route.as_str().is_none() { + return Some(Self::InvalidHttpComponentTrigger( + id, + "route is not a string", + )); + } + None + } +} + +impl Diagnosis for TriggerDiagnosis { + fn description(&self) -> String { + match self { + Self::MissingAppTrigger => "missing top-level trigger config".into(), + Self::InvalidAppTrigger(msg) => { + format!("Invalid app trigger config: {msg}") + } + Self::HttpAppTriggerMissingBase => "http trigger config missing base".into(), + Self::HttpComponentTriggerMissingRoute(id, _) => { + format!("HTTP component {id:?} missing trigger.route") + } + Self::InvalidHttpComponentTrigger(id, msg) => { + format!("Invalid trigger config for http component {id:?}: {msg}") + } + } + } + + fn treatment(&self) -> Option<&dyn Treatment> { + match self { + Self::MissingAppTrigger | Self::HttpAppTriggerMissingBase => Some(self), + // We can reasonably fill in default "route" iff there is only one component + Self::HttpComponentTriggerMissingRoute(_, single_component) if *single_component => { + Some(self) + } + _ => None, + } + } +} + +#[async_trait] +impl ManifestTreatment for TriggerDiagnosis { + fn summary(&self) -> String { + match self { + TriggerDiagnosis::MissingAppTrigger => "Add default HTTP trigger config".into(), + TriggerDiagnosis::HttpAppTriggerMissingBase => { + "Set default HTTP trigger base '/'".into() + } + TriggerDiagnosis::HttpComponentTriggerMissingRoute(id, _) => { + format!("Set trigger.route '/...' for component {id:?}") + } + _ => "[invalid treatment]".into(), + } + } + + async fn treat_manifest(&self, doc: &mut Document) -> anyhow::Result<()> { + match self { + Self::MissingAppTrigger | Self::HttpAppTriggerMissingBase => { + // Get or insert missing trigger config + if doc.get("trigger").is_none() { + doc.insert("trigger", Item::Value(InlineTable::new().into())); + } + let trigger = doc + .get_mut("trigger") + .unwrap() + .as_table_like_mut() + .context("existing trigger value is not a table")?; + + // Get trigger type or insert default "http" + let trigger_type = trigger.entry("type").or_insert(Item::Value("http".into())); + if let Some("http") = trigger_type.as_str() { + // Strip "type" trailing space + if let Some(decor) = trigger_type.as_value_mut().map(|v| v.decor_mut()) { + if let Some(suffix) = decor.suffix().and_then(|s| s.as_str()) { + decor.set_suffix(suffix.to_string().trim()); + } + } + // Set missing "base" + trigger.entry("base").or_insert(Item::Value("/".into())); + } + } + Self::HttpComponentTriggerMissingRoute(_, true) => { + // Get the only component + let components = doc + .get_mut("component") + .context("missing components")? + .as_array_of_tables_mut() + .context("component sections aren't an 'array of tables'")?; + ensure!( + components.len() == 1, + "can only set default trigger route if there is exactly one component; found {}", + components.len() + ); + let component = components.get_mut(0).unwrap(); + + // Get or insert missing trigger config + if component.get("trigger").is_none() { + component.insert("trigger", Item::Table(Table::new())); + } + let trigger = component + .get_mut("trigger") + .unwrap() + .as_table_like_mut() + .context("existing trigger value is not a table")?; + + // Set missing "route" + trigger.entry("route").or_insert(Item::Value("/...".into())); + } + _ => bail!("cannot be fixed"), + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::test::{run_broken_test, run_correct_test}; + + use super::*; + + #[tokio::test] + async fn test_correct() { + run_correct_test::("manifest_trigger").await; + } + + #[tokio::test] + async fn test_missing_app_trigger() { + let diag = + run_broken_test::("manifest_trigger", "missing_app_trigger").await; + assert!(matches!(diag, TriggerDiagnosis::MissingAppTrigger)); + } + + #[tokio::test] + async fn test_http_app_trigger_missing_base() { + let diag = run_broken_test::( + "manifest_trigger", + "http_app_trigger_missing_base", + ) + .await; + assert!(matches!(diag, TriggerDiagnosis::HttpAppTriggerMissingBase)); + } + + #[tokio::test] + async fn test_http_component_trigger_missing_route() { + let diag = run_broken_test::( + "manifest_trigger", + "http_component_trigger_missing_route", + ) + .await; + assert!(matches!( + diag, + TriggerDiagnosis::HttpComponentTriggerMissingRoute(_, _) + )); + } +} diff --git a/crates/doctor/src/manifest/version.rs b/crates/doctor/src/manifest/version.rs new file mode 100644 index 0000000000..1bb78d73e6 --- /dev/null +++ b/crates/doctor/src/manifest/version.rs @@ -0,0 +1,135 @@ +use anyhow::{Context, Result}; +use async_trait::async_trait; +use serde::Deserialize; +use toml::Value; +use toml_edit::{de::from_document, Document, Item}; + +use crate::{Diagnosis, Diagnostic, PatientApp, Treatment}; + +use super::ManifestTreatment; + +const SPIN_MANIFEST_VERSION: &str = "spin_manifest_version"; +const SPIN_VERSION: &str = "spin_version"; + +/// VersionDiagnostic detects problems with the app manifest version field. +#[derive(Default)] +pub struct VersionDiagnostic; + +#[async_trait] +impl Diagnostic for VersionDiagnostic { + type Diagnosis = VersionDiagnosis; + + async fn diagnose(&self, patient: &PatientApp) -> Result> { + let doc = &patient.manifest_doc; + let test: VersionProbe = + from_document(doc.clone()).context("failed to decode VersionProbe")?; + + if let Some(value) = test.spin_manifest_version.or(test.spin_version.clone()) { + if value.as_str() != Some("1") { + return Ok(vec![VersionDiagnosis::WrongValue(value)]); + } else if test.spin_version.is_some() { + return Ok(vec![VersionDiagnosis::OldVersionKey]); + } + } else { + return Ok(vec![VersionDiagnosis::MissingVersion]); + } + Ok(vec![]) + } +} + +#[derive(Debug, Deserialize)] +struct VersionProbe { + spin_manifest_version: Option, + spin_version: Option, +} + +/// VersionDiagnosis represents a problem with the app manifest version field. +#[derive(Debug)] +pub enum VersionDiagnosis { + /// Missing any known version key + MissingVersion, + /// Using old spin_version key + OldVersionKey, + /// Wrong version value + WrongValue(Value), +} + +impl Diagnosis for VersionDiagnosis { + fn description(&self) -> String { + match self { + Self::MissingVersion => "Manifest missing 'spin_manifest_version' key".into(), + Self::OldVersionKey => "Manifest using old 'spin_version' key".into(), + Self::WrongValue(val) => { + format!(r#"Manifest 'spin_manifest_version' must be "1", not {val}"#) + } + } + } + + fn is_critical(&self) -> bool { + !matches!(self, Self::OldVersionKey) + } + + fn treatment(&self) -> Option<&dyn Treatment> { + Some(self) + } +} + +#[async_trait] +impl ManifestTreatment for VersionDiagnosis { + fn summary(&self) -> String { + match self { + Self::MissingVersion => "Add spin_manifest_version to manifest", + Self::OldVersionKey => "Replace 'spin_version' with 'spin_manifest_version'", + Self::WrongValue(_) => r#"Set manifest version to "1""#, + } + .into() + } + + async fn treat_manifest(&self, doc: &mut Document) -> anyhow::Result<()> { + doc.remove(SPIN_VERSION); + let item = Item::Value("1".into()); + if let Some(existing) = doc.get_mut(SPIN_MANIFEST_VERSION) { + *existing = item; + } else { + doc.insert(SPIN_MANIFEST_VERSION, item); + // (ab)use stable sorting to move the inserted item to the top + doc.sort_values_by(|k1, _, k2, _| { + let k1_is_version = k1.get() == SPIN_MANIFEST_VERSION; + let k2_is_version = k2.get() == SPIN_MANIFEST_VERSION; + // true > false + k2_is_version.cmp(&k1_is_version) + }) + } + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::test::{run_broken_test, run_correct_test}; + + use super::*; + + #[tokio::test] + async fn test_correct() { + run_correct_test::("manifest_version").await; + } + + #[tokio::test] + async fn test_missing() { + let diag = run_broken_test::("manifest_version", "missing_key").await; + assert!(matches!(diag, VersionDiagnosis::MissingVersion)); + } + + #[tokio::test] + async fn test_old_key() { + let diag = run_broken_test::("manifest_version", "old_key").await; + assert!(matches!(diag, VersionDiagnosis::OldVersionKey)); + } + + #[tokio::test] + async fn test_wrong_value() { + let diag = run_broken_test::("manifest_version", "wrong_value").await; + assert!(matches!(diag, VersionDiagnosis::WrongValue(_))); + } +} diff --git a/crates/doctor/src/test.rs b/crates/doctor/src/test.rs new file mode 100644 index 0000000000..892b52a781 --- /dev/null +++ b/crates/doctor/src/test.rs @@ -0,0 +1,116 @@ +#![cfg(test)] +#![allow(clippy::expect_fun_call)] + +use std::{fs, io::Write, path::Path}; + +use tempfile::{NamedTempFile, TempPath}; +use toml::Value; + +use super::*; + +/// Asserts that the manifest at "tests/data/_correct.toml" does +/// not have the given [`ManifestCondition`]. +pub async fn run_correct_test(prefix: &str) { + let patient = TestPatient::from_file(test_file_path(prefix, "correct")); + let diags = D::default() + .diagnose(&patient) + .await + .expect("diagnose failed"); + assert!(diags.is_empty(), "expected correct file; got {diags:?}"); +} + +/// Asserts that the manifest at "tests/data/_broken.toml" has +/// the given [`ManifestCondition`]. Also asserts that after fixing the +/// problem the manifest matches "tests/data/_fixed.toml". +pub async fn run_broken_test(prefix: &str, suffix: &str) -> D::Diagnosis { + let mut patient = TestPatient::from_file(test_file_path(prefix, suffix)); + + let diag = assert_single_diagnosis::(&patient).await; + let treatment = diag + .treatment() + .expect(&format!("{diag:?} should be treatable")); + + treatment + .treat(&mut patient) + .await + .expect("treatment should succeed"); + + let correct_path = test_file_path(prefix, "correct"); + let fixed_contents = + fs::read_to_string(&correct_path).expect(&format!("reading {correct_path:?} failed")); + assert_eq!( + patient.manifest_doc.to_string().trim_end(), + fixed_contents.trim_end() + ); + + diag +} + +pub async fn assert_single_diagnosis( + patient: &PatientApp, +) -> D::Diagnosis { + let diags = D::default() + .diagnose(patient) + .await + .expect("diagnose should succeed"); + assert!(diags.len() == 1, "expected one diagnosis, got {diags:?}"); + diags.into_iter().next().unwrap() +} + +fn test_file_path(prefix: &str, suffix: &str) -> PathBuf { + format!("tests/data/{prefix}_{suffix}.toml").into() +} + +pub struct TestPatient { + inner: PatientApp, + _manifest_temp: TempPath, +} + +impl TestPatient { + fn new(manifest_temp: TempPath) -> Result { + let inner = Checkup::new(&manifest_temp).patient()?; + Ok(Self { + inner, + _manifest_temp: manifest_temp, + }) + } + + pub fn from_file(manifest_path: impl AsRef) -> Self { + let manifest_temp = NamedTempFile::new() + .expect("creating tempfile") + .into_temp_path(); + + let manifest_path = manifest_path.as_ref(); + std::fs::copy(manifest_path, &manifest_temp) + .expect(&format!("copying {manifest_path:?} to tempfile")); + + Self::new(manifest_temp).expect(&format!("{manifest_path:?} should be a valid test file")) + } + + pub fn from_toml(manifest: impl Into) -> Self { + let mut manifest_file = NamedTempFile::new().expect("creating tempfile"); + let content = toml::to_string(&manifest.into()).expect("valid TOML"); + manifest_file + .write_all(content.as_bytes()) + .expect("writing TOML"); + Self::new(manifest_file.into_temp_path()).unwrap() + } + + pub fn from_toml_str(manifest: impl AsRef) -> Self { + Self::from_toml(toml::from_str::(manifest.as_ref()).expect("valid TOML")) + } +} + +impl std::ops::Deref for TestPatient { + type Target = PatientApp; + + fn deref(&self) -> &Self::Target { + &self.inner + } +} + +impl std::ops::DerefMut for TestPatient { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.inner + } +} diff --git a/crates/doctor/src/wasm.rs b/crates/doctor/src/wasm.rs new file mode 100644 index 0000000000..03c6225a7e --- /dev/null +++ b/crates/doctor/src/wasm.rs @@ -0,0 +1,74 @@ +/// Diagnose missing Wasm sources. +pub mod missing; + +use std::path::Path; + +use anyhow::Result; +use async_trait::async_trait; +use spin_loader::{local::config::RawComponentManifest, local::config::RawModuleSource}; + +use crate::{Diagnosis, Diagnostic, PatientApp}; + +/// PatientWasm represents a Wasm source to be checked for problems. +#[derive(Debug)] +pub struct PatientWasm(RawComponentManifest); + +#[allow(missing_docs)] // WIP +impl PatientWasm { + pub fn component_id(&self) -> &str { + &self.0.id + } + + pub fn source(&self) -> WasmSource { + match &self.0.source { + RawModuleSource::FileReference(path) => WasmSource::Local(path), + _ => WasmSource::Other, + } + } + + pub fn has_build(&self) -> bool { + self.0.build.is_some() + } +} + +/// WasmSource is a source (e.g. file path) of a Wasm binary. +#[derive(Debug)] +#[non_exhaustive] +pub enum WasmSource<'a> { + /// Local file source path. + Local(&'a Path), + /// Other source (currently unsupported) + Other, +} + +/// WasmDiagnose helps implement [`Diagnose`] for Wasm source problems. +#[async_trait] +pub trait WasmDiagnostic { + /// A [`Diagnosis`] representing the problem(s) this can detect. + type Diagnosis: Diagnosis; + + /// Check the given [`PatientWasm`], returning any problem(s) found. + async fn diagnose_wasm( + &self, + app: &PatientApp, + wasm: PatientWasm, + ) -> Result>; +} + +#[async_trait] +impl Diagnostic for T { + type Diagnosis = ::Diagnosis; + + async fn diagnose(&self, patient: &PatientApp) -> Result> { + let path = &patient.manifest_path; + let manifest = spin_loader::local::raw_manifest_from_file(&path) + .await? + .into_v1(); + let mut diagnoses = vec![]; + for component in manifest.components { + let wasm = PatientWasm(component); + diagnoses.extend(self.diagnose_wasm(patient, wasm).await?); + } + Ok(diagnoses) + } +} diff --git a/crates/doctor/src/wasm/missing.rs b/crates/doctor/src/wasm/missing.rs new file mode 100644 index 0000000000..c1a440b578 --- /dev/null +++ b/crates/doctor/src/wasm/missing.rs @@ -0,0 +1,123 @@ +use std::process::Command; + +use anyhow::{ensure, Context, Result}; +use async_trait::async_trait; + +use crate::{Diagnosis, PatientApp, Treatment}; + +use super::{PatientWasm, WasmDiagnostic, WasmSource}; + +/// WasmMissingDiagnostic detects missing Wasm sources. +#[derive(Default)] +pub struct WasmMissingDiagnostic; + +#[async_trait] +impl WasmDiagnostic for WasmMissingDiagnostic { + type Diagnosis = WasmMissing; + + async fn diagnose_wasm( + &self, + _app: &PatientApp, + wasm: PatientWasm, + ) -> anyhow::Result> { + if let WasmSource::Local(path) = wasm.source() { + if !path.exists() { + return Ok(vec![WasmMissing(wasm)]); + } + } + Ok(vec![]) + } +} + +/// WasmMissing represents a missing Wasm source. +#[derive(Debug)] +pub struct WasmMissing(PatientWasm); + +impl WasmMissing { + fn build_cmd(&self, patient: &PatientApp) -> Result { + let spin_bin = std::env::current_exe().context("Couldn't find spin executable")?; + let mut cmd = Command::new(spin_bin); + cmd.arg("build") + .arg("-f") + .arg(&patient.manifest_path) + .arg("--component-id") + .arg(self.0.component_id()); + Ok(cmd) + } +} + +impl Diagnosis for WasmMissing { + fn description(&self) -> String { + let id = self.0.component_id(); + let WasmSource::Local(path) = self.0.source() else { + unreachable!("unsupported source"); + }; + format!("Component {id:?} source {path:?} is missing") + } + + fn treatment(&self) -> Option<&dyn Treatment> { + self.0.has_build().then_some(self) + } +} + +#[async_trait] +impl Treatment for WasmMissing { + fn summary(&self) -> String { + "Run `spin build`".into() + } + + async fn dry_run(&self, patient: &PatientApp) -> anyhow::Result { + let args = self + .build_cmd(patient)? + .get_args() + .map(|arg| arg.to_string_lossy()) + .collect::>() + .join(" "); + Ok(format!("Run `spin {args}`")) + } + + async fn treat(&self, patient: &mut PatientApp) -> anyhow::Result<()> { + let mut cmd = self.build_cmd(patient)?; + let status = cmd.status()?; + ensure!(status.success(), "Build command {cmd:?} failed: {status:?}"); + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use crate::test::{assert_single_diagnosis, TestPatient}; + + use super::*; + + const MINIMUM_VIABLE_MANIFEST: &str = r#" + spin_manifest_version = "1" + name = "wasm-missing-test" + version = "0.0.0" + trigger = { type = "test" } + [[component]] + id = "missing-source" + source = "does-not-exist.wasm" + trigger = {} + "#; + + #[tokio::test] + async fn test_without_build() { + let patient = TestPatient::from_toml_str(MINIMUM_VIABLE_MANIFEST); + let diag = assert_single_diagnosis::(&patient).await; + assert!(diag.treatment().is_none()); + } + + #[tokio::test] + async fn test_with_build() { + let manifest = format!("{MINIMUM_VIABLE_MANIFEST}\nbuild.command = 'true'"); + let patient = TestPatient::from_toml_str(manifest); + let diag = assert_single_diagnosis::(&patient).await; + assert!(diag.treatment().is_some()); + assert!(diag + .build_cmd(&patient) + .unwrap() + .get_args() + .any(|arg| arg == "missing-source")); + } +} diff --git a/crates/doctor/tests/data/manifest_trigger_correct.toml b/crates/doctor/tests/data/manifest_trigger_correct.toml new file mode 100644 index 0000000000..5b9a97bdc6 --- /dev/null +++ b/crates/doctor/tests/data/manifest_trigger_correct.toml @@ -0,0 +1,7 @@ +trigger = { type = "http", base = "/" } + +[[component]] +id = "http-component" + +[component.trigger] +route = "/..." \ No newline at end of file diff --git a/crates/doctor/tests/data/manifest_trigger_http_app_trigger_missing_base.toml b/crates/doctor/tests/data/manifest_trigger_http_app_trigger_missing_base.toml new file mode 100644 index 0000000000..9c11470253 --- /dev/null +++ b/crates/doctor/tests/data/manifest_trigger_http_app_trigger_missing_base.toml @@ -0,0 +1,7 @@ +trigger = { type = "http" } + +[[component]] +id = "http-component" + +[component.trigger] +route = "/..." \ No newline at end of file diff --git a/crates/doctor/tests/data/manifest_trigger_http_component_trigger_missing_route.toml b/crates/doctor/tests/data/manifest_trigger_http_component_trigger_missing_route.toml new file mode 100644 index 0000000000..2a6b6da881 --- /dev/null +++ b/crates/doctor/tests/data/manifest_trigger_http_component_trigger_missing_route.toml @@ -0,0 +1,4 @@ +trigger = { type = "http", base = "/" } + +[[component]] +id = "http-component" \ No newline at end of file diff --git a/crates/doctor/tests/data/manifest_trigger_missing_app_trigger.toml b/crates/doctor/tests/data/manifest_trigger_missing_app_trigger.toml new file mode 100644 index 0000000000..63518be6a0 --- /dev/null +++ b/crates/doctor/tests/data/manifest_trigger_missing_app_trigger.toml @@ -0,0 +1,6 @@ + +[[component]] +id = "http-component" + +[component.trigger] +route = "/..." \ No newline at end of file diff --git a/crates/doctor/tests/data/manifest_version_correct.toml b/crates/doctor/tests/data/manifest_version_correct.toml new file mode 100644 index 0000000000..de852fe450 --- /dev/null +++ b/crates/doctor/tests/data/manifest_version_correct.toml @@ -0,0 +1,4 @@ +spin_manifest_version = "1" + +# comment preserved +name = "app-name" \ No newline at end of file diff --git a/crates/doctor/tests/data/manifest_version_missing_key.toml b/crates/doctor/tests/data/manifest_version_missing_key.toml new file mode 100644 index 0000000000..68afe7628a --- /dev/null +++ b/crates/doctor/tests/data/manifest_version_missing_key.toml @@ -0,0 +1,3 @@ + +# comment preserved +name = "app-name" diff --git a/crates/doctor/tests/data/manifest_version_old_key.toml b/crates/doctor/tests/data/manifest_version_old_key.toml new file mode 100644 index 0000000000..666a214bcd --- /dev/null +++ b/crates/doctor/tests/data/manifest_version_old_key.toml @@ -0,0 +1,4 @@ +spin_version = "1" + +# comment preserved +name = "app-name" diff --git a/crates/doctor/tests/data/manifest_version_wrong_value.toml b/crates/doctor/tests/data/manifest_version_wrong_value.toml new file mode 100644 index 0000000000..e9227e110b --- /dev/null +++ b/crates/doctor/tests/data/manifest_version_wrong_value.toml @@ -0,0 +1,4 @@ +spin_manifest_version = 2 + +# comment preserved +name = "app-name" diff --git a/src/bin/spin.rs b/src/bin/spin.rs index 985e84ddd7..20d5e08b8d 100644 --- a/src/bin/spin.rs +++ b/src/bin/spin.rs @@ -6,6 +6,7 @@ use spin_cli::commands::{ build::BuildCommand, cloud::CloudCommands, deploy::DeployCommand, + doctor::DoctorCommand, external::execute_external_subcommand, login::LoginCommand, new::{AddCommand, NewCommand}, @@ -70,6 +71,7 @@ enum SpinApp { #[clap(external_subcommand)] External(Vec), Watch(WatchCommand), + Doctor(DoctorCommand), } #[derive(Subcommand)] @@ -99,6 +101,7 @@ impl SpinApp { Self::Plugins(cmd) => cmd.run().await, Self::External(cmd) => execute_external_subcommand(cmd, SpinApp::command()).await, Self::Watch(cmd) => cmd.run().await, + Self::Doctor(cmd) => cmd.run().await, } } } diff --git a/src/commands.rs b/src/commands.rs index 902acffe9b..0df451d0ec 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -6,6 +6,8 @@ pub mod build; pub mod cloud; /// Command to package and upload an application to the Fermyon Platform. pub mod deploy; +/// Command for running the Spin Doctor. +pub mod doctor; /// Commands for external subcommands (i.e. plugins) pub mod external; /// Command for logging into the Fermyon Platform. diff --git a/src/commands/build.rs b/src/commands/build.rs index d5f574c608..e2f01c2c94 100644 --- a/src/commands/build.rs +++ b/src/commands/build.rs @@ -23,8 +23,12 @@ pub struct BuildCommand { )] pub app_source: PathBuf, + /// Component ID to build. The default is all components. + #[clap(short = 'c', long)] + pub component_id: Option, + /// Run the application after building. - #[clap(name = BUILD_UP_OPT, short = 'u', long = "up")] + #[clap(name = BUILD_UP_OPT, short = 'u', long)] pub up: bool, #[clap(requires = BUILD_UP_OPT)] diff --git a/src/commands/doctor.rs b/src/commands/doctor.rs new file mode 100644 index 0000000000..6017cdb1ae --- /dev/null +++ b/src/commands/doctor.rs @@ -0,0 +1,119 @@ +use std::{fmt::Debug, path::PathBuf}; + +use anyhow::Result; +use clap::Parser; +use dialoguer::{console::Emoji, Confirm, Select}; +use futures::FutureExt; +use spin_doctor::{Diagnosis, DryRunNotSupported}; + +use crate::opts::DEFAULT_MANIFEST_FILE; + +#[derive(Parser, Debug)] +#[clap(hide = true, about = "Detect and fix problems with Spin applications")] +pub struct DoctorCommand { + #[clap(short = 'f', long = "file", default_value = DEFAULT_MANIFEST_FILE)] + manifest_file: PathBuf, +} + +impl DoctorCommand { + pub async fn run(self) -> Result<()> { + let manifest_file = crate::manifest::resolve_file_path(&self.manifest_file)?; + + println!("{icon}The Spin Doctor is in.", icon = Emoji("📟 ", "")); + println!( + "{icon}Checking {}...", + manifest_file.display(), + icon = Emoji("🩺 ", "") + ); + + let count = spin_doctor::Checkup::new(manifest_file) + .for_each_diagnosis(move |diagnosis, patient| { + async move { + show_diagnosis(&*diagnosis); + + if let Some(treatment) = diagnosis.treatment() { + let dry_run = match treatment.dry_run(patient).await { + Ok(desc) => Some(desc), + Err(err) => { + if !err.is::() { + show_error("Treatment dry run failed: ", err); + } + return Ok(()); + } + }; + + let should_treat = prompt_treatment(treatment.summary(), dry_run) + .unwrap_or_else(|err| { + show_error("Prompt error: ", err); + false + }); + + if should_treat { + match treatment.treat(patient).await { + Ok(()) => { + println!("{icon}Treatment applied!", icon = Emoji("❤ ", "")); + } + Err(err) => { + show_error("Treatment failed: ", err); + } + } + } + } + Ok(()) + } + .boxed() + }) + .await?; + if count == 0 { + println!("{icon}No problems found.", icon = Emoji("❤ ", "")); + } + Ok(()) + } +} + +fn show_diagnosis(diagnosis: &dyn Diagnosis) { + let icon = if diagnosis.is_critical() { + Emoji("❗ ", "") + } else { + Emoji("⚠ ", "") + }; + println!("\n{icon}Diagnosis: {}", diagnosis.description()); +} + +fn prompt_treatment(summary: String, dry_run: Option) -> Result { + let prompt = format!( + "{icon}The Spin Doctor can help! Would you like to", + icon = Emoji("🩹 ", "") + ); + let mut items = vec![summary.as_str(), "Do nothing"]; + if dry_run.is_some() { + items.push("See more details about the recommended changes"); + } + let selection = Select::new() + .with_prompt(prompt) + .items(&items) + .default(0) + .interact_opt()?; + + match selection { + Some(2) => { + println!( + "\n{icon}{}\n", + dry_run.unwrap_or_default().trim_end(), + icon = Emoji("📋 ", "") + ); + Ok(Confirm::new() + .with_prompt("Would you like to apply this fix?") + .default(true) + .interact_opt()? + .unwrap_or_default()) + } + Some(0) => Ok(true), + _ => Ok(false), + } +} + +fn show_error(prefix: &str, err: impl Debug) { + let icon = Emoji("⁉️ ", ""); + println!("{icon}{prefix}{err:?}"); +}