Merge pull request #3136 from itowlson/host-reqs-on-components-waah-waah-waah

Fix subsets inheriting service chaining requirement from whole app

Author: itowlson

Cargo.lock

@@ -169,8 +169,12 @@ impl App {
     /// Checks that the application does not have any host requirements
     /// outside the supported set. The error case returns a comma-separated
     /// list of unmet requirements.
-    pub fn ensure_needs_only(&self, supported: &[&str]) -> std::result::Result<(), String> {
-        self.locked.ensure_needs_only(supported)
+    pub fn ensure_needs_only(
+        &self,
+        trigger_type: &str,
+        supported: &[&str],
+    ) -> std::result::Result<(), String> {
+        self.locked.ensure_needs_only(trigger_type, supported)
     }
 
     /// Scrubs the locked app to only contain the given list of components

crates/app/src/lib.rs

@@ -84,8 +84,6 @@ impl LocalLoader {
 
         let metadata = locked_metadata(application, triggers.keys().cloned())?;
 
-        let app_requires_service_chaining = components.values().any(requires_service_chaining);
-
         let variables = variables
             .into_iter()
             .map(|(name, v)| Ok((name.to_string(), locked_variable(v)?)))
@@ -111,19 +109,16 @@ impl LocalLoader {
         }))
         .await?;
 
-        let mut host_requirements = ValuesMapBuilder::new();
-        if app_requires_service_chaining {
-            host_requirements.string(
-                spin_locked_app::locked::SERVICE_CHAINING_KEY,
-                spin_locked_app::locked::HOST_REQ_REQUIRED,
-            );
-        }
-        let host_requirements = host_requirements.build();
+        let host_requirements = spin_locked_app::values::ValuesMap::new();
 
         let mut must_understand = vec![];
         if !host_requirements.is_empty() {
             must_understand.push(spin_locked_app::locked::MustUnderstand::HostRequirements);
         }
+        if components.iter().any(|c| !c.host_requirements.is_empty()) {
+            must_understand
+                .push(spin_locked_app::locked::MustUnderstand::ComponentHostRequirements);
+        }
 
         drop(sloth_guard);
 
@@ -150,6 +145,8 @@ impl LocalLoader {
         spin_factor_outbound_networking::AllowedHostsConfig::validate(&allowed_outbound_hosts)
             .context("`allowed_outbound_hosts` is malformed")?;
 
+        let component_requires_service_chaining = requires_service_chaining(&component);
+
         let metadata = ValuesMapBuilder::new()
             .string("description", component.description)
             .string_array("allowed_outbound_hosts", allowed_outbound_hosts)
@@ -213,6 +210,15 @@ impl LocalLoader {
             .map(|(k, v)| (k.into(), v))
             .collect();
 
+        let mut host_requirements = ValuesMapBuilder::new();
+        if component_requires_service_chaining {
+            host_requirements.string(
+                spin_locked_app::locked::SERVICE_CHAINING_KEY,
+                spin_locked_app::locked::HOST_REQ_REQUIRED,
+            );
+        }
+        let host_requirements = host_requirements.build();
+
         Ok(LockedComponent {
             id: id.as_ref().into(),
             metadata,
@@ -221,6 +227,7 @@ impl LocalLoader {
             files,
             config,
             dependencies,
+            host_requirements,
         })
     }
 

crates/loader/src/local.rs

@@ -1,7 +1,7 @@
 {
   "spin_lock_version": 1,
   "must_understand": [
-    "host_requirements"
+    "component_host_requirements"
   ],
   "metadata": {
     "authors": [
@@ -18,9 +18,6 @@
     "triggers": {},
     "version": "6.11.2"
   },
-  "host_requirements": {
-    "local_service_chaining": "required"
-  },
   "triggers": [
     {
       "id": "four-lights-http-trigger",
@@ -65,6 +62,9 @@
       "env": {
         "env1": "first",
         "env2": "second"
+      },
+      "host_requirements": {
+        "local_service_chaining": "required"
       }
     },
     {

crates/loader/tests/ui/service-chaining.lock

@@ -7,6 +7,7 @@ edition = { workspace = true }
 [dependencies]
 anyhow = { workspace = true }
 async-trait = { workspace = true }
+itertools = { workspace = true }
 serde = { workspace = true }
 serde_json = { workspace = true }
 spin-serde = { path = "../serde" }

crates/locked-app/Cargo.toml

@@ -1,7 +1,8 @@
 //! Spin lock file (spin.lock) serialization models.
 
-use std::path::PathBuf;
+use std::{collections::HashSet, path::PathBuf};
 
+use itertools::Itertools;
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use spin_serde::{DependencyName, FixedVersionBackwardCompatible};
@@ -25,21 +26,19 @@ pub const HOST_REQ_OPTIONAL: &str = "optional";
 /// Indicates that a host feature is required.
 pub const HOST_REQ_REQUIRED: &str = "required";
 
+// TODO: it turns out that using an enum for this results in bad
+// errors by non-understanders (unknown variant rather than "I'm sorry
+// Dave I can't do that")
 /// Identifies fields in the LockedApp that the host must process if present.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 pub enum MustUnderstand {
     /// If present in `must_understand`, the host must support all features
     /// in the app's `host_requirements` section.
     HostRequirements,
-}
-
-/// Features or capabilities the application requires the host to support.
-#[derive(Clone, Debug, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-pub enum HostRequirement {
-    /// The application requires local service chaining.
-    LocalServiceChaining,
+    /// If present in `must_understand`, the host must support all features
+    /// in components' `host_requirements` section.
+    ComponentHostRequirements,
 }
 
 /// A LockedApp represents a "fully resolved" Spin application.
@@ -187,10 +186,26 @@ impl LockedApp {
     /// Checks that the application does not have any host requirements
     /// outside the supported set. The error case returns a comma-separated
     /// list of unmet requirements.
-    pub fn ensure_needs_only(&self, supported: &[&str]) -> Result<(), String> {
-        let unmet_requirements = self
-            .host_requirements
-            .keys()
+    pub fn ensure_needs_only(&self, trigger_type: &str, supported: &[&str]) -> Result<(), String> {
+        let app_host_requirements = self.host_requirements.keys();
+
+        let component_ids = self
+            .triggers
+            .iter()
+            .filter(|t| t.trigger_type == trigger_type)
+            .flat_map(|t| t.trigger_config.get("component"))
+            .filter_map(|v| v.as_str())
+            .collect::<HashSet<_>>();
+        let components = self
+            .components
+            .iter()
+            .filter(|c| component_ids.contains(c.id.as_str()));
+        let component_host_requirements = components.flat_map(|c| c.host_requirements.keys());
+
+        let all_host_requirements = app_host_requirements.chain(component_host_requirements);
+
+        let unmet_requirements = all_host_requirements
+            .unique()
             .filter(|hr| !supported.contains(&hr.as_str()))
             .map(|s| s.to_string())
             .collect::<Vec<_>>();
@@ -225,6 +240,13 @@ pub struct LockedComponent {
     /// Component dependencies
     #[serde(default, skip_serializing_if = "BTreeMap::is_empty")]
     pub dependencies: BTreeMap<DependencyName, LockedComponentDependency>,
+    /// Host requirements
+    #[serde(
+        default,
+        skip_serializing_if = "ValuesMap::is_empty",
+        deserialize_with = "deserialize_host_requirements"
+    )]
+    pub host_requirements: ValuesMap,
 }
 
 /// A LockedDependency represents a "fully resolved" Spin component dependency.

crates/locked-app/src/locked.rs

@@ -184,7 +184,7 @@ impl<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> FactorsTriggerCommand<T,
         };
 
         // Validate required host features
-        if let Err(unmet) = app.ensure_needs_only(&T::supported_host_requirements()) {
+        if let Err(unmet) = app.ensure_needs_only(T::TYPE, &T::supported_host_requirements()) {
             anyhow::bail!("This application requires the following features that are not available in this version of the '{}' trigger: {unmet}", T::TYPE);
         }
 

crates/trigger/src/cli.rs

@@ -96,6 +96,66 @@ mod integration_tests {
         Ok(())
     }
 
+    #[test]
+    #[cfg(feature = "extern-dependencies-tests")]
+    #[allow(dependency_on_unit_never_type_fallback)]
+    /// Test that if you have an app where some but not all components use service
+    /// chaining, the non-service-chained subset can run in a non-service-chainable
+    /// trigger.
+    fn can_split_service_chained_app() -> anyhow::Result<()> {
+        use anyhow::Context;
+        use redis::Commands;
+        run_test(
+            "multi-trigger",
+            SpinConfig {
+                binary_path: spin_binary(),
+                spin_up_args: vec!["-c".into(), "hello".into()],
+                app_type: SpinAppType::Redis,
+            },
+            ServicesConfig::new(vec!["redis"])?,
+            move |env| {
+                let redis_port = env
+                    .services_mut()
+                    .get_port(6379)?
+                    .context("no redis port was exposed by test services")?;
+
+                let mut redis = redis::Client::open(format!("redis://localhost:{redis_port}"))
+                    .context("could not connect to redis in test")?;
+                redis
+                    .publish("my-channel", "msg-from-test")
+                    .context("could not publish test message to redis")?;
+                assert_eventually!(
+                    {
+                        match env.read_file(".spin/logs/hello_stdout.txt") {
+                            Ok(logs) => {
+                                let logs = String::from_utf8_lossy(&logs);
+                                logs.contains("Got message: 'msg-from-test'")
+                            }
+                            Err(e)
+                                if e.downcast_ref()
+                                    .map(|e: &std::io::Error| {
+                                        e.kind() == std::io::ErrorKind::NotFound
+                                    })
+                                    .unwrap_or_default() =>
+                            {
+                                false
+                            }
+                            Err(e) => {
+                                return Err(
+                                    anyhow::anyhow!("could not read stdout file: {e}").into()
+                                )
+                            }
+                        }
+                    },
+                    2
+                );
+                Ok(())
+            },
+        )?;
+
+        Ok(())
+    }
+
     #[test]
     #[cfg(feature = "extern-dependencies-tests")]
     #[allow(dependency_on_unit_never_type_fallback)]

tests/integration.rs

@@ -0,0 +1,48 @@
+spin_manifest_version = 2
+
+[application]
+authors = ["Fermyon Engineering <[email protected]>"]
+description = "A simple redis application that exercises the Rust SDK in the current branch"
+name = "multi-trigger"
+version = "1.0.0"
+
+[application.trigger.redis]
+address = "redis://localhost:%{port=6379}"
+
+[[trigger.redis]]
+channel = "my-channel"
+component = "hello"
+
+[[trigger.http]]
+route = "/..."
+component = "front"
+
+[[trigger.http]]
+route = { private = true }
+component = "middle"
+
+[[trigger.http]]
+route = { private = true }
+component = "back"
+
+[component.hello]
+source = "%{source=redis-smoke-test}"
+[component.hello.build]
+command = "cargo build --target wasm32-wasip1 --release"
+
+[component.front]
+source = "%{source=internal-http-front}"
+allowed_outbound_hosts = ["http://middle.spin.internal"]
+[component.front.build]
+command = "cargo build --target wasm32-wasip1 --release"
+
+[component.middle]
+source = "%{source=internal-http-middle}"
+allowed_outbound_hosts = ["http://*.spin.internal"]
+[component.middle.build]
+command = "cargo build --target wasm32-wasip1 --release"
+
+[component.back]
+source = "%{source=internal-http-back}"
+[component.back.build]
+command = "cargo build --target wasm32-wasip1 --release"