From 9ee2be89b6f69a7c617147ed8c189d3c600f2e1f Mon Sep 17 00:00:00 2001 From: itowlson Date: Mon, 17 Jul 2023 13:42:22 +1200 Subject: [PATCH] Do not install latest plugin if incompatible Signed-off-by: itowlson --- crates/plugins/Cargo.toml | 6 +- crates/plugins/src/badger/mod.rs | 2 +- crates/plugins/src/error.rs | 3 + crates/plugins/src/lookup.rs | 171 ++++++++++++++++-- crates/plugins/src/manager.rs | 8 +- crates/plugins/src/manifest.rs | 4 + .../tests/.spin-plugins/_spin_test_dot_git | 1 + .../some-spin-ver-some-not.json | 27 +++ .../some-spin-ver-some-not@98.0.0.json | 27 +++ .../some-spin-ver-some-not@98.1.0.json | 15 ++ .../some-spin-ver-some-not@99.0.0.json | 27 +++ .../some-spin-ver-some-not.json | 21 +++ .../some-spin-ver-some-not@98.0.0.json | 21 +++ .../some-spin-ver-some-not@98.1.0.json | 15 ++ .../some-spin-ver-some-not@99.0.0.json | 21 +++ src/commands/plugins.rs | 25 ++- 16 files changed, 372 insertions(+), 22 deletions(-) create mode 100644 crates/plugins/tests/.spin-plugins/_spin_test_dot_git create mode 100644 crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not.json create mode 100644 crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json create mode 100644 crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json create mode 100644 crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json create mode 100644 crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not.json create mode 100644 crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json create mode 100644 crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json create mode 100644 crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json diff --git a/crates/plugins/Cargo.toml b/crates/plugins/Cargo.toml index 5ce5c631d..0816ef3d6 100644 --- a/crates/plugins/Cargo.toml +++ b/crates/plugins/Cargo.toml @@ -7,14 +7,14 @@ edition = { workspace = true } [dependencies] anyhow = "1.0" bytes = "1.1" -chrono = "0.4" +chrono = { version = "0.4", features = ["serde"] } dirs = "4.0" fd-lock = "3.0.12" flate2 = { version = "1.0.17", features = ["zlib-ng"], default-features = false } is-terminal = "0.4" path-absolutize = "3.0.11" reqwest = { version = "0.11", features = ["json"] } -semver = "1.0" +semver = { version = "1.0", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" spin-common = { path = "../common" } @@ -24,4 +24,4 @@ terminal = { path = "../terminal" } thiserror = "1" tokio = { version = "1.23", features = [ "fs", "process", "rt", "macros" ] } tracing = { workspace = true } -url = "2.2.2" +url = { version = "2.2.2", features = ["serde"] } diff --git a/crates/plugins/src/badger/mod.rs b/crates/plugins/src/badger/mod.rs index fe5ffaf94..b4ee7d74d 100644 --- a/crates/plugins/src/badger/mod.rs +++ b/crates/plugins/src/badger/mod.rs @@ -170,7 +170,7 @@ impl BadgerEvaluator { let latest_version = { let latest_lookup = crate::lookup::PluginLookup::new(&self.plugin_name, None); let latest_manifest = latest_lookup - .get_manifest_from_repository(store.get_plugins_directory()) + .resolve_manifest_exact(store.get_plugins_directory()) .await .ok(); latest_manifest.and_then(|m| semver::Version::parse(m.version()).ok()) diff --git a/crates/plugins/src/error.rs b/crates/plugins/src/error.rs index c784b7dac..a8d2c148d 100644 --- a/crates/plugins/src/error.rs +++ b/crates/plugins/src/error.rs @@ -14,6 +14,9 @@ pub enum Error { #[error("URL parse error {0}")] UrlParseError(#[from] url::ParseError), + + #[error("{0}")] + Other(#[from] anyhow::Error), } /// Contains error details for when a plugin resource cannot be found at expected location diff --git a/crates/plugins/src/lookup.rs b/crates/plugins/src/lookup.rs index 9c50da750..f3c71c334 100644 --- a/crates/plugins/src/lookup.rs +++ b/crates/plugins/src/lookup.rs @@ -30,7 +30,34 @@ impl PluginLookup { } } - pub async fn get_manifest_from_repository( + pub async fn resolve_manifest( + &self, + plugins_dir: &Path, + skip_compatibility_check: bool, + spin_version: &str, + ) -> PluginLookupResult { + let exact = self.resolve_manifest_exact(plugins_dir).await?; + if skip_compatibility_check + || self.version.is_some() + || exact.is_compatible_spin_version(spin_version) + { + return Ok(exact); + } + + let store = crate::store::PluginStore::new(plugins_dir.to_owned()); + + // TODO: This is very similar to some logic in the badger module - look for consolidation opportunities. + let manifests = store.catalogue_manifests()?; + let relevant_manifests = manifests.into_iter().filter(|m| m.name() == self.name); + let compatible_manifests = relevant_manifests + .filter(|m| m.has_compatible_package() && m.is_compatible_spin_version(spin_version)); + let highest_compatible_manifest = + compatible_manifests.max_by_key(|m| m.try_version().unwrap_or_else(|_| null_version())); + + Ok(highest_compatible_manifest.unwrap_or(exact)) + } + + pub async fn resolve_manifest_exact( &self, plugins_dir: &Path, ) -> PluginLookupResult { @@ -41,22 +68,48 @@ impl PluginLookup { .map_err(|e| { Error::ConnectionFailed(ConnectionFailedError::new(url.to_string(), e.to_string())) })?; + + self.resolve_manifest_exact_from_good_repo(plugins_dir) + } + + // This is split from resolve_manifest_exact because it may recurse (once) and that makes + // Rust async sad. So we move the potential recursion to a sync helper. + #[allow(clippy::let_and_return)] + pub fn resolve_manifest_exact_from_good_repo( + &self, + plugins_dir: &Path, + ) -> PluginLookupResult { let expected_path = spin_plugins_repo_manifest_path(&self.name, &self.version, plugins_dir); - let file = File::open(&expected_path).map_err(|e| { - Error::NotFound(NotFoundError::new( - Some(self.name.clone()), - expected_path.display().to_string(), - e.to_string(), - )) - })?; - let manifest: PluginManifest = serde_json::from_reader(file).map_err(|e| { - Error::InvalidManifest(InvalidManifestError::new( + + let not_found = |e: std::io::Error| { + Err(Error::NotFound(NotFoundError::new( Some(self.name.clone()), expected_path.display().to_string(), e.to_string(), - )) - })?; - Ok(manifest) + ))) + }; + + let manifest = match File::open(&expected_path) { + Ok(file) => serde_json::from_reader(file).map_err(|e| { + Error::InvalidManifest(InvalidManifestError::new( + Some(self.name.clone()), + expected_path.display().to_string(), + e.to_string(), + )) + }), + Err(e) if e.kind() == std::io::ErrorKind::NotFound && self.version.is_some() => { + // If a user has asked for a version by number, and the path doesn't exist, + // it _might_ be because it's the latest version. This checks for that case. + let latest = Self::new(&self.name, None); + match latest.resolve_manifest_exact_from_good_repo(plugins_dir) { + Ok(manifest) if manifest.try_version().ok() == self.version => Ok(manifest), + _ => not_found(e), + } + } + Err(e) => not_found(e), + }; + + manifest } } @@ -64,6 +117,16 @@ pub fn plugins_repo_url() -> Result { Url::parse(SPIN_PLUGINS_REPO) } +#[cfg(not(test))] +fn accept_as_repo(git_root: &Path) -> bool { + git_root.join(".git").exists() +} + +#[cfg(test)] +fn accept_as_repo(git_root: &Path) -> bool { + git_root.join(".git").exists() || git_root.join("_spin_test_dot_git").exists() +} + pub async fn fetch_plugins_repo( repo_url: &Url, plugins_dir: &Path, @@ -71,7 +134,7 @@ pub async fn fetch_plugins_repo( ) -> anyhow::Result<()> { let git_root = plugin_manifests_repo_path(plugins_dir); let git_source = GitSource::new(repo_url, None, &git_root); - if git_root.join(".git").exists() { + if accept_as_repo(&git_root) { if update { git_source.pull().await?; } @@ -110,3 +173,83 @@ pub fn spin_plugins_repo_manifest_dir(plugins_dir: &Path) -> PathBuf { .join(PLUGINS_REPO_LOCAL_DIRECTORY) .join(PLUGINS_REPO_MANIFESTS_DIRECTORY) } + +fn null_version() -> semver::Version { + semver::Version::new(0, 0, 0) +} + +#[cfg(test)] +mod tests { + use super::*; + + const TEST_NAME: &str = "some-spin-ver-some-not"; + const TESTS_STORE_DIR: &str = "tests"; + + fn tests_store_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join(TESTS_STORE_DIR) + } + + #[tokio::test] + async fn if_no_version_given_and_latest_is_compatible_then_latest() -> PluginLookupResult<()> { + let lookup = PluginLookup::new(TEST_NAME, None); + let resolved = lookup + .resolve_manifest(&tests_store_dir(), false, "99.0.0") + .await?; + assert_eq!("99.0.1", resolved.version); + Ok(()) + } + + #[tokio::test] + async fn if_no_version_given_and_latest_is_not_compatible_then_highest_compatible( + ) -> PluginLookupResult<()> { + // NOTE: The setup assumes you are NOT running Windows on aarch64, so as to check 98.1.0 is not + // offered. If that assumption fails then this test will fail with actual version being 98.1.0. + // (We use this combination because the OS and architecture enums don't allow for fake operating systems!) + let lookup = PluginLookup::new(TEST_NAME, None); + let resolved = lookup + .resolve_manifest(&tests_store_dir(), false, "98.0.0") + .await?; + assert_eq!("98.0.0", resolved.version); + Ok(()) + } + + #[tokio::test] + async fn if_version_given_it_gets_used_regardless() -> PluginLookupResult<()> { + let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("99.0.0").unwrap())); + let resolved = lookup + .resolve_manifest(&tests_store_dir(), false, "98.0.0") + .await?; + assert_eq!("99.0.0", resolved.version); + Ok(()) + } + + #[tokio::test] + async fn if_latest_version_given_it_gets_used_regardless() -> PluginLookupResult<()> { + let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("99.0.1").unwrap())); + let resolved = lookup + .resolve_manifest(&tests_store_dir(), false, "98.0.0") + .await?; + assert_eq!("99.0.1", resolved.version); + Ok(()) + } + + #[tokio::test] + async fn if_no_version_given_but_skip_compat_then_highest() -> PluginLookupResult<()> { + let lookup = PluginLookup::new(TEST_NAME, None); + let resolved = lookup + .resolve_manifest(&tests_store_dir(), true, "98.0.0") + .await?; + assert_eq!("99.0.1", resolved.version); + Ok(()) + } + + #[tokio::test] + async fn if_non_existent_version_given_then_error() -> PluginLookupResult<()> { + let lookup = PluginLookup::new(TEST_NAME, Some(semver::Version::parse("177.7.7").unwrap())); + lookup + .resolve_manifest(&tests_store_dir(), true, "99.0.0") + .await + .expect_err("Should have errored because plugin v177.7.7 does not exist"); + Ok(()) + } +} diff --git a/crates/plugins/src/manager.rs b/crates/plugins/src/manager.rs index 13c68e882..2d6a81d24 100644 --- a/crates/plugins/src/manager.rs +++ b/crates/plugins/src/manager.rs @@ -184,6 +184,8 @@ impl PluginManager { pub async fn get_manifest( &self, manifest_location: &ManifestLocation, + skip_compatibility_check: bool, + spin_version: &str, ) -> PluginLookupResult { let plugin_manifest = match manifest_location { ManifestLocation::Remote(url) => { @@ -232,7 +234,11 @@ impl PluginManager { } ManifestLocation::PluginsRepository(lookup) => { lookup - .get_manifest_from_repository(self.store().get_plugins_directory()) + .resolve_manifest( + self.store().get_plugins_directory(), + skip_compatibility_check, + spin_version, + ) .await? } }; diff --git a/crates/plugins/src/manifest.rs b/crates/plugins/src/manifest.rs index b7f7c21b8..9a0fe43d8 100644 --- a/crates/plugins/src/manifest.rs +++ b/crates/plugins/src/manifest.rs @@ -64,6 +64,10 @@ impl PluginManifest { Err(_) => false, } } + + pub fn try_version(&self) -> Result { + semver::Version::parse(&self.version) + } } /// Describes compatibility and location of a plugin source. diff --git a/crates/plugins/tests/.spin-plugins/_spin_test_dot_git b/crates/plugins/tests/.spin-plugins/_spin_test_dot_git new file mode 100644 index 000000000..a60a97e03 --- /dev/null +++ b/crates/plugins/tests/.spin-plugins/_spin_test_dot_git @@ -0,0 +1 @@ +Fake file for preventing the plugin system from pulling from the production repo into this directory during tests diff --git a/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not.json b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not.json new file mode 100644 index 000000000..d325dc8ec --- /dev/null +++ b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not.json @@ -0,0 +1,27 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "99.0.1", + "spinCompatibility": ">=99.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json new file mode 100644 index 000000000..e3db68191 --- /dev/null +++ b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json @@ -0,0 +1,27 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "98.0.0", + "spinCompatibility": ">=98.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json new file mode 100644 index 000000000..1db8f9014 --- /dev/null +++ b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json @@ -0,0 +1,15 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "98.1.0", + "spinCompatibility": ">=98.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "windows", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json new file mode 100644 index 000000000..bf255be66 --- /dev/null +++ b/crates/plugins/tests/.spin-plugins/manifests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json @@ -0,0 +1,27 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "99.0.0", + "spinCompatibility": ">=99.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not.json b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not.json new file mode 100644 index 000000000..eafeb34c0 --- /dev/null +++ b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not.json @@ -0,0 +1,21 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "99.0.1", + "spinCompatibility": ">=99.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json new file mode 100644 index 000000000..fb313522c --- /dev/null +++ b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.0.0.json @@ -0,0 +1,21 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "98.0.0", + "spinCompatibility": ">=98.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json new file mode 100644 index 000000000..1db8f9014 --- /dev/null +++ b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@98.1.0.json @@ -0,0 +1,15 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "98.1.0", + "spinCompatibility": ">=98.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "windows", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json new file mode 100644 index 000000000..cf2bd99f9 --- /dev/null +++ b/crates/plugins/tests/some-spin-ver-some-not/some-spin-ver-some-not@99.0.0.json @@ -0,0 +1,21 @@ +{ + "name": "some-spin-ver-some-not", + "description": "A plugin where only some versions work with the test harness version of Spin.", + "version": "99.0.0", + "spinCompatibility": ">=99.0", + "license": "Apache-2.0", + "packages": [ + { + "os": "linux", + "arch": "amd64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + }, + { + "os": "macos", + "arch": "aarch64", + "url": "https://example.com/doesnt-exist", + "sha256": "11111111" + } + ] +} diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index a5dbe5619..31ca9f8b5 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -114,7 +114,13 @@ impl Install { let manager = PluginManager::try_default()?; // Downgrades are only allowed via the `upgrade` subcommand let downgrade = false; - let manifest = manager.get_manifest(&manifest_location).await?; + let manifest = manager + .get_manifest( + &manifest_location, + self.override_compatibility_check, + SPIN_VERSION, + ) + .await?; try_install( &manifest, &manager, @@ -250,7 +256,14 @@ impl Upgrade { .to_string(); let manifest_location = ManifestLocation::PluginsRepository(PluginLookup::new(&name, None)); - let manifest = match manager.get_manifest(&manifest_location).await { + let manifest = match manager + .get_manifest( + &manifest_location, + self.override_compatibility_check, + SPIN_VERSION, + ) + .await + { Err(Error::NotFound(e)) => { log::info!("Could not upgrade plugin '{name}': {e:?}"); continue; @@ -283,7 +296,13 @@ impl Upgrade { self.version, )), }; - let manifest = manager.get_manifest(&manifest_location).await?; + let manifest = manager + .get_manifest( + &manifest_location, + self.override_compatibility_check, + SPIN_VERSION, + ) + .await?; try_install( &manifest, &manager,