diff --git a/crates/wadm-types/src/lib.rs b/crates/wadm-types/src/lib.rs index 2e63098d..840f2a4b 100644 --- a/crates/wadm-types/src/lib.rs +++ b/crates/wadm-types/src/lib.rs @@ -82,6 +82,52 @@ impl Manifest { self.spec.components.iter() } + /// Helper function to find shared components that are missing from the given list of + /// deployed applications + pub fn missing_shared_components(&self, deployed_apps: &Vec<&Manifest>) -> Vec<&Component> { + self.spec + .components + .iter() + .filter_map(|shared_component| { + match &shared_component.properties { + Properties::Capability { + properties: + CapabilityProperties { + image: None, + application: Some(shared_app), + .. + }, + } + | Properties::Component { + properties: + ComponentProperties { + image: None, + application: Some(shared_app), + .. + }, + } => { + if deployed_apps.iter().filter(|a| a.shared()).any(|m| { + m.metadata.name == shared_app.name + && m.components().any(|c| { + c.name == shared_app.component + // This compares just the enum variant, not the actual properties + // For example, if we reference a shared component that's a capability, + // we want to make sure the deployed component is a capability. + && std::mem::discriminant(&c.properties) + == std::mem::discriminant(&shared_component.properties) + }) + }) { + None + } else { + Some(shared_component) + } + } + _ => None, + } + }) + .collect() + } + /// Returns only the WebAssembly components in the manifest pub fn wasm_components(&self) -> impl Iterator { self.components() diff --git a/crates/wadm-types/src/validation.rs b/crates/wadm-types/src/validation.rs index 9466df93..0f47ea2a 100644 --- a/crates/wadm-types/src/validation.rs +++ b/crates/wadm-types/src/validation.rs @@ -610,6 +610,7 @@ pub fn validate_component_properties(application: &Manifest) -> Vec Vec match (image, application) { @@ -645,6 +647,15 @@ pub fn validate_component_properties(application: &Manifest) -> Vec { + failures.push(ValidationFailure::new( + ValidationFailureLevel::Error, + format!( + "Shared component '{}' cannot specify additional 'secrets'", + shared_properties.name + ), + )); + } // Shared application components already have scale properties defined in their original manifest (None, Some(shared_properties)) if component diff --git a/crates/wadm/src/scaler/convert.rs b/crates/wadm/src/scaler/convert.rs index 9ecd1354..a44719b1 100644 --- a/crates/wadm/src/scaler/convert.rs +++ b/crates/wadm/src/scaler/convert.rs @@ -197,7 +197,7 @@ fn component_scalers( ); config_names.append(&mut secret_names.clone()); - // TODO: Consider a way to report on status of a shared component + // TODO(#451): Consider a way to report on status of a shared component match (trt.trait_type.as_str(), &trt.properties, &properties.image) { // Shared application components already have their own spread/daemon scalers, you // cannot modify them from another manifest diff --git a/crates/wadm/src/server/handlers.rs b/crates/wadm/src/server/handlers.rs index ef4e4085..6814bc8a 100644 --- a/crates/wadm/src/server/handlers.rs +++ b/crates/wadm/src/server/handlers.rs @@ -69,8 +69,7 @@ impl Handler

{ self.send_error( msg.reply, format!( - "Manifest name {} contains invalid characters. Manifest names can only contain alphanumeric characters, dashes, and underscores.", - manifest_name + "Manifest name {manifest_name} contains invalid characters. Manifest names can only contain alphanumeric characters, dashes, and underscores.", ), ) .await; @@ -89,66 +88,46 @@ impl Handler

{ } }; - if let Some(error_message) = validate_manifest(manifest.clone()).await.err() { + if let Some(error_message) = validate_manifest(&manifest).await.err() { self.send_error(msg.reply, error_message.to_string()).await; return; } - let all_manifests: Vec = self + let all_stored_manifests = self .store .list(account_id, lattice_id) .await - .unwrap_or_default() - .into_iter() - .filter(|manifest| manifest.get_current().shared()) - // TODO: Do we need to just get deployed here? I assume that would be a warning if not deployed - .map(|manifest| manifest.get_current().clone()) + .unwrap_or_default(); + let deployed_shared_apps: Vec<&Manifest> = all_stored_manifests + .iter() + // Only keep deployed, shared applications + .filter(|manifest| { + manifest.deployed_version().is_some() && manifest.get_current().shared() + }) + .map(|manifest| manifest.get_current()) .collect(); - // TODO: We are also going to have to ensure that links don't clobber other links. Specifically - // this relates to the uniqueness properties of a source/target/interface/etc for a link. - - // Ensure that all shared components are present in other (deployed?) manifests that are - // marked as shared applications - let mut shared_components = manifest - .components() - // Find all shared components - .filter_map(|c| match &c.properties { - Properties::Capability { - properties: - CapabilityProperties { - image: None, - application: Some(manifest_properties), - .. - }, - } => Some(manifest_properties), - Properties::Component { - properties: - ComponentProperties { - image: None, - application: Some(manifest_properties), - .. - }, - } => Some(manifest_properties), - _ => None, - }); - - // Ensure all shared components point to a component that is deployed in another manifest - // TODO: would be better to find matching bad shared components - if !shared_components.all(|properties| { - // TODO: ensure that the type of the property is preserved, e.g. component -> component - // TODO: ensure that the manifest is deployed - all_manifests.iter().any(|m| { - m.metadata.name == properties.name - && m.components().any(|c| c.name == properties.component) - }) - }) { - self.send_error(msg.reply, "shared component not found".to_string()) - .await; - return; - } - // Explicitly drop to let the compiler know we're done with the manifest borrow - drop(shared_components); + // NOTE(brooksmtownsend): You can put an application with missing shared components, because + // the time where you truly need them is when you deploy the application. This can cause a bit + // of friction when it comes to deploy, but it avoids the frustrating race condition where you + // - Put the application looking for a deployed shared component + // - Undeploy the application with the shared component + // - Deploy the new application looking for the shared component (error) + let missing_shared_components = manifest.missing_shared_components(&deployed_shared_apps); + let message = if missing_shared_components.is_empty() { + format!( + "Successfully put manifest {} {}", + manifest_name, + current_manifests.current_version().to_owned() + ) + } else { + format!( + "Successfully put manifest {} {}, but some shared components are not deployed: {:?}", + manifest_name, + current_manifests.current_version().to_owned(), + missing_shared_components + ) + }; let incoming_version = manifest.version().to_owned(); if !current_manifests.add_version(manifest) { @@ -170,11 +149,7 @@ impl Handler

{ }, name: manifest_name.clone(), total_versions: current_manifests.count(), - message: format!( - "Successfully put manifest {} {}", - manifest_name, - current_manifests.current_version() - ), + message, }; trace!(total_manifests = %resp.total_versions, "Storing manifests"); @@ -437,6 +412,7 @@ impl Handler

{ } } }; + // TODO(#451): if shared and deployed, make sure that no other shared apps are using it let reply_data = if let Some(version) = req.version { match self.store.get(account_id, lattice_id, name).await { Ok(Some((mut current, current_revision))) => { @@ -729,6 +705,26 @@ impl Handler

{ } } + // TODO(#451): If this app is shared, or the previous version was, make sure that shared + // components that have dependent applications are still present + + let missing_shared_components = staged_model.missing_shared_components( + &stored_models + .iter() + .filter(|a| a.deployed_version().is_some() && a.get_current().shared()) + .map(|a| a.get_current()) + .collect(), + ); + + // Ensure all shared components point to a valid component that is deployed in another application + if !missing_shared_components.is_empty() { + self.send_error( + msg.reply, + format!("Application contains shared components that are not deployed in other applications: {:?}", missing_shared_components.iter().map(|c| &c.name).collect::>()) + ).await; + return; + } + if !manifests.deploy(req.version.clone()) { trace!("Requested version does not exist"); self.send_reply( @@ -857,6 +853,7 @@ impl Handler

{ return; } }; + // TODO(#451): if shared, make sure that no other shared apps are using it let reply = if manifests.undeploy() { trace!("Manifest undeployed. Storing updated manifest"); @@ -1019,7 +1016,7 @@ fn summary_from_manifest_status(manifest: StoredManifest, status: Status) -> Mod } // Manifest validation -pub(crate) async fn validate_manifest(manifest: Manifest) -> anyhow::Result<()> { +pub(crate) async fn validate_manifest(manifest: &Manifest) -> anyhow::Result<()> { let failures = wadm_types::validation::validate_manifest(&manifest).await?; for failure in failures { if matches!( @@ -1055,12 +1052,12 @@ mod test { let correct_manifest = deserialize_yaml("../../tests/fixtures/manifests/simple.yaml") .expect("Should be able to parse"); - assert!(validate_manifest(correct_manifest).await.is_ok()); + assert!(validate_manifest(&correct_manifest).await.is_ok()); let manifest = deserialize_yaml("../../tests/fixtures/manifests/incorrect_component.yaml") .expect("Should be able to parse"); - match validate_manifest(manifest).await { + match validate_manifest(&manifest).await { Ok(()) => panic!("Should have detected incorrect component"), Err(e) => { assert!(e @@ -1072,7 +1069,7 @@ mod test { let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_component.yaml") .expect("Should be able to parse"); - match validate_manifest(manifest).await { + match validate_manifest(&manifest).await { Ok(()) => panic!("Should have detected duplicate component"), Err(e) => assert!(e .to_string() @@ -1082,7 +1079,7 @@ mod test { let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_id1.yaml") .expect("Should be able to parse"); - match validate_manifest(manifest).await { + match validate_manifest(&manifest).await { Ok(()) => { panic!("Should have detected duplicate component ID in provider properties") } @@ -1094,7 +1091,7 @@ mod test { let manifest = deserialize_yaml("../../tests/fixtures/manifests/duplicate_id2.yaml") .expect("Should be able to parse"); - match validate_manifest(manifest).await { + match validate_manifest(&manifest).await { Ok(()) => panic!("Should have detected duplicate component ID in component properties"), Err(e) => assert!(e .to_string() @@ -1105,7 +1102,7 @@ mod test { deserialize_yaml("../../tests/fixtures/manifests/missing_capability_component.yaml") .expect("Should be able to parse"); - match validate_manifest(manifest).await { + match validate_manifest(&manifest).await { Ok(()) => panic!("Should have detected missing capability component"), Err(e) => assert!(e .to_string() @@ -1118,7 +1115,7 @@ mod test { #[tokio::test] async fn manifest_name_long_image_ref() -> Result<()> { validate_manifest( - deserialize_yaml("../../tests/fixtures/manifests/long_image_refs.yaml") + &deserialize_yaml("../../tests/fixtures/manifests/long_image_refs.yaml") .context("failed to deserialize YAML")?, ) .await diff --git a/tests/e2e.rs b/tests/e2e.rs index 7d83c9bf..bc4ec838 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -24,7 +24,6 @@ use wadm_types::{ use wasmcloud_control_interface::HostInventory; const LOG_DIR: &str = "tests/e2e_log"; -pub const DEFAULT_LATTICE_ID: &str = "default"; // Due to download times and the time needed to stabilize, we still need to wait for just a little bit // This number comes from 30s (max backoff time) + 5s of wiggle room pub const DEFAULT_WAIT_TIME: Duration = Duration::from_secs(35); diff --git a/tests/e2e_shared.rs b/tests/e2e_shared.rs index a89592e9..08743126 100644 --- a/tests/e2e_shared.rs +++ b/tests/e2e_shared.rs @@ -141,7 +141,7 @@ async fn test_shared_providers(client_info: &ClientInfo) { .get_links() .await .map_err(|e| anyhow::anyhow!("{e:?}"))? - .response + .into_data() .context("Should have links")?; ensure!(links.is_empty(), "Shouldn't have any links"); @@ -181,7 +181,7 @@ async fn test_shared_providers(client_info: &ClientInfo) { .get_config("shared_http_dev-httpaddr") .await .map_err(|e| anyhow::anyhow!("should have http provider source config {e}"))? - .response + .into_data() .context("should have http provider source config response")?; assert_eq!( config, @@ -197,7 +197,7 @@ async fn test_shared_providers(client_info: &ClientInfo) { .get_links() .await .map_err(|e| anyhow::anyhow!("{e:?}"))? - .response + .into_data() .context("Should have links")?; ensure!( @@ -208,12 +208,12 @@ async fn test_shared_providers(client_info: &ClientInfo) { if !links.iter().any(|ld| { // This is checking that the source ID and the target // come from the correct generated manifest IDs - ld.source_id == "shared_http-httpserver" - && ld.target == "shared_http_dev-hello" - && ld.wit_namespace == "wasi" - && ld.wit_package == "http" - && ld.interfaces == vec!["incoming-handler"] - && ld.name == "default" + ld.source_id() == "shared_http-httpserver" + && ld.target() == "shared_http_dev-hello" + && ld.wit_namespace() == "wasi" + && ld.wit_package() == "http" + && ld.interfaces() == &vec!["incoming-handler"] + && ld.name() == "default" }) { anyhow::bail!( "Link between http server provider and hello component should exist: {:#?}", @@ -223,12 +223,12 @@ async fn test_shared_providers(client_info: &ClientInfo) { if !links.iter().any(|ld| { // This is checking that the source ID and the target // come from the correct generated manifest IDs - ld.source_id == "shared_http_dev-hello" - && ld.target == "shared_http-httpclient" - && ld.wit_namespace == "wasi" - && ld.wit_package == "http" - && ld.interfaces == vec!["outgoing-handler"] - && ld.name == "default" + ld.source_id() == "shared_http_dev-hello" + && ld.target() == "shared_http-httpclient" + && ld.wit_namespace() == "wasi" + && ld.wit_package() == "http" + && ld.interfaces() == &vec!["outgoing-handler"] + && ld.name() == "default" }) { anyhow::bail!( "Link between hello component and http client provider should exist: {:#?}", @@ -253,7 +253,7 @@ async fn test_shared_providers(client_info: &ClientInfo) { .await .unwrap(); - // TODO(#381): Additional validation tests coming in a follow-up PR + // TODO(#451): Additional validation tests coming in a follow-up PR // // You can't undeploy an application that is depended on // assert!(client.undeploy_manifest("shared-http").await.is_err()); // assert!(client.delete_manifest("shared-http", None).await.is_err()); @@ -291,7 +291,7 @@ async fn test_shared_components(client_info: &ClientInfo) { .get_config("shared_component-defaults") .await .map_err(|e| anyhow::anyhow!("should have http provider source config {e}"))? - .response + .into_data() .context("should have http provider source config response")?; assert_eq!( config, @@ -305,7 +305,7 @@ async fn test_shared_components(client_info: &ClientInfo) { .get_links() .await .map_err(|e| anyhow::anyhow!("{e:?}"))? - .response + .into_data() .context("Should have links")?; ensure!(links.is_empty(), "Shouldn't have any links"); @@ -352,7 +352,7 @@ async fn test_shared_components(client_info: &ClientInfo) { .get_config("shared_component_dev-someconfig") .await .map_err(|e| anyhow::anyhow!("should have http provider source config {e}"))? - .response + .into_data() .context("should have http provider source config response")?; assert_eq!( config, @@ -364,38 +364,38 @@ async fn test_shared_components(client_info: &ClientInfo) { .get_links() .await .map_err(|e| anyhow::anyhow!("{e:?}"))? - .response + .into_data() .context("Should have links")?; ensure!(links.len() == 3, "Should have three links"); if !links.iter().any(|ld| { - ld.source_id == "shared_component_dev-hello" - && ld.target == "shared_component-link_to_meee" - && ld.wit_namespace == "custom" - && ld.wit_package == "package" - && ld.interfaces == vec!["inter", "face"] - && ld.name == "default" + ld.source_id() == "shared_component_dev-hello" + && ld.target() == "shared_component-link_to_meee" + && ld.wit_namespace() == "custom" + && ld.wit_package() == "package" + && ld.interfaces() == &vec!["inter", "face"] + && ld.name() == "default" }) { anyhow::bail!("Link between hello components should exist: {:#?}", links) } if !links.iter().any(|ld| { - ld.source_id == "shared_component-link_to_meee" - && ld.target == "shared_component_dev-hello" - && ld.wit_namespace == "custom" - && ld.wit_package == "package" - && ld.interfaces == vec!["inter", "face"] - && ld.name == "default" + ld.source_id() == "shared_component-link_to_meee" + && ld.target() == "shared_component_dev-hello" + && ld.wit_namespace() == "custom" + && ld.wit_package() == "package" + && ld.interfaces() == &vec!["inter", "face"] + && ld.name() == "default" }) { anyhow::bail!("Link between hello components should exist: {:#?}", links) } if !links.iter().any(|ld| { - ld.source_id == "shared_component_dev-httpserver" - && ld.target == "shared_component-link_to_meee" - && ld.wit_namespace == "wasi" - && ld.wit_package == "http" - && ld.interfaces == vec!["incoming-handler"] - && ld.name == "default" + ld.source_id() == "shared_component_dev-httpserver" + && ld.target() == "shared_component-link_to_meee" + && ld.wit_namespace() == "wasi" + && ld.wit_package() == "http" + && ld.interfaces() == &vec!["incoming-handler"] + && ld.name() == "default" }) { anyhow::bail!( "Link between http server provider and hello component should exist: {:#?}",