From 0e4c4c197578c4c4a34cfe2b857b8576122a002e Mon Sep 17 00:00:00 2001 From: Aminu Oluwaseun Joshua Date: Wed, 10 Sep 2025 15:45:57 +0100 Subject: [PATCH] Check required variables are potentially resolvable Adds new dynamism metadata for Providers. Allow for no default value before runtime if Provider is a Dynamic type. PR also includes unit tests Signed-off-by: Aminu Oluwaseun Joshua --- Cargo.lock | 1 + crates/expressions/src/lib.rs | 57 +++- crates/expressions/src/provider.rs | 11 + crates/expressions/tests/validation_test.rs | 267 +++++++++++++++++++ crates/factor-variables/src/lib.rs | 4 + crates/factor-variables/tests/factor_test.rs | 6 +- crates/runtime-factors/src/build.rs | 3 +- crates/trigger/Cargo.toml | 1 + crates/trigger/src/cli.rs | 2 + crates/trigger/src/cli/variable.rs | 23 ++ crates/variables-azure/src/lib.rs | 6 +- crates/variables-env/src/lib.rs | 6 +- crates/variables-static/src/lib.rs | 6 +- crates/variables-vault/src/lib.rs | 6 +- 14 files changed, 392 insertions(+), 7 deletions(-) create mode 100644 crates/expressions/tests/validation_test.rs create mode 100644 crates/trigger/src/cli/variable.rs diff --git a/Cargo.lock b/Cargo.lock index 07c1e43171..c0b5762148 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9158,6 +9158,7 @@ dependencies = [ "spin-core", "spin-factor-key-value", "spin-factor-sqlite", + "spin-factor-variables", "spin-factor-wasi", "spin-factors", "spin-factors-executor", diff --git a/crates/expressions/src/lib.rs b/crates/expressions/src/lib.rs index 2219044372..30eb200003 100644 --- a/crates/expressions/src/lib.rs +++ b/crates/expressions/src/lib.rs @@ -1,7 +1,7 @@ pub mod provider; mod template; -use std::{borrow::Cow, collections::HashMap, fmt::Debug}; +use std::{borrow::Cow, collections::HashMap, fmt::Debug, vec}; use spin_locked_app::Variable; @@ -11,6 +11,8 @@ pub use provider::Provider; use template::Part; pub use template::Template; +use crate::provider::ProviderVariableKind; + /// A [`ProviderResolver`] that can be shared. pub type SharedPreparedResolver = std::sync::Arc>>; @@ -90,6 +92,54 @@ impl ProviderResolver { Ok(PreparedResolver { variables }) } + /// Ensures that all required variables are resolvable at startup + pub async fn ensure_required_variables_resolvable(&self) -> Result<()> { + // If a dynamic provider is available, skip validation. + if self + .providers + .iter() + .any(|p| p.kind() == ProviderVariableKind::Dynamic) + { + return Ok(()); + } + + let mut unvalidated_keys = vec![]; + for key in self.internal.variables.keys() { + // If default value is provided, skip validation. + if let Some(value) = self.internal.variables.get(key) { + if value.default.is_some() { + continue; + } + } + + let mut resolved = false; + + for provider in &self.providers { + if provider + .get(&Key(key)) + .await + .map_err(Error::Provider)? + .is_some() + { + resolved = true; + break; + } + } + + if !resolved { + unvalidated_keys.push(key); + } + } + + if unvalidated_keys.is_empty() { + Ok(()) + } else { + Err(Error::Provider(anyhow::anyhow!( + "no provider resolved required variable(s): {unvalidated_keys:?}", + ))) + } + } + async fn resolve_variable(&self, key: &str) -> Result { for provider in &self.providers { if let Some(value) = provider.get(&Key(key)).await.map_err(Error::Provider)? { @@ -310,6 +360,7 @@ mod tests { use async_trait::async_trait; use super::*; + use crate::provider::ProviderVariableKind; #[derive(Debug)] struct TestProvider; @@ -323,6 +374,10 @@ mod tests { _ => Ok(None), } } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Static + } } async fn test_resolve(template: &str) -> Result { diff --git a/crates/expressions/src/provider.rs b/crates/expressions/src/provider.rs index f0eec76ab6..30ef1bd3c0 100644 --- a/crates/expressions/src/provider.rs +++ b/crates/expressions/src/provider.rs @@ -9,4 +9,15 @@ use crate::Key; pub trait Provider: Debug + Send + Sync { /// Returns the value at the given config path, if it exists. async fn get(&self, key: &Key) -> anyhow::Result>; + fn kind(&self) -> ProviderVariableKind; +} + +/// The dynamism of a Provider. +#[derive(Clone, Debug, Default, PartialEq)] +pub enum ProviderVariableKind { + /// Variable must be declared on start + Static, + /// Variable can be made available at runtime + #[default] + Dynamic, } diff --git a/crates/expressions/tests/validation_test.rs b/crates/expressions/tests/validation_test.rs new file mode 100644 index 0000000000..4da78cca3b --- /dev/null +++ b/crates/expressions/tests/validation_test.rs @@ -0,0 +1,267 @@ +use std::collections::HashMap; + +use async_trait::async_trait; +use spin_expressions::{provider::ProviderVariableKind, Key, Provider, ProviderResolver}; +use spin_locked_app::Variable; + +#[derive(Default)] +struct ResolverTester { + providers: Vec>, + variables: HashMap, +} + +impl ResolverTester { + fn new() -> Self { + Self::default() + } + + fn with_provider(mut self, provider: impl Provider + 'static) -> Self { + self.providers.push(Box::new(provider)); + self + } + + fn with_variable(mut self, key: &str, default: Option<&str>) -> Self { + self.variables.insert( + key.to_string(), + Variable { + description: None, + default: default.map(ToString::to_string), + secret: false, + }, + ); + self + } + + fn make_resolver(self) -> anyhow::Result { + let mut provider_resolver = ProviderResolver::new(self.variables)?; + + for provider in self.providers { + provider_resolver.add_provider(provider); + } + + Ok(provider_resolver) + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_single_static_provider_with_no_key_to_resolve_is_valid() -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_single_static_provider_has_data_for_variable_key_to_resolve_it_succeeds( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_variable("database_host", None) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_is_a_single_static_provider_and_it_does_not_contain_a_required_variable_then_validation_fails( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_variable("api_key", None) + .make_resolver()?; + + assert!(resolver + .ensure_required_variables_resolvable() + .await + .is_err()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_is_a_dynamic_provider_then_validation_succeeds_even_without_default_value_in_play( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(DynamicProvider) + .with_variable("api_key", None) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_is_a_dynamic_provider_and_static_provider_but_the_variable_to_be_resolved_is_not_in_play( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(DynamicProvider) + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_variable("api_key", None) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_there_is_a_variable_in_play( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(DynamicProvider) + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_variable("api_key", Some("super-secret-key")) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_are_two_static_providers_where_one_has_data_is_valid() -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_provider(StaticProvider::with_variable( + "api_key", + Some("super-secret-key"), + )) + .with_variable("database_host", None) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} +// Ensure that if there are two or more static providers and the first one does not have data for the variable to be resolved, +// but the second or subsequent one does, then validation still succeeds. +#[tokio::test(flavor = "multi_thread")] +async fn if_there_are_two_static_providers_where_first_provider_does_not_have_data_while_second_provider_does( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_provider(StaticProvider::with_variable( + "api_key", + Some("super-secret-key"), + )) + .with_variable("api_key", None) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn if_there_is_two_static_providers_neither_having_data_is_invalid() -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_provider(StaticProvider::with_variable( + "database_host", + Some("localhost"), + )) + .with_provider(StaticProvider::with_variable( + "api_key", + Some("super-secret-key"), + )) + .with_variable("hello", None) + .make_resolver()?; + + assert!(resolver + .ensure_required_variables_resolvable() + .await + .is_err()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn no_provider_data_available_but_variable_default_value_needed_is_invalid( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_variable("api_key", None) + .make_resolver()?; + + assert!(resolver + .ensure_required_variables_resolvable() + .await + .is_err()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn no_provider_data_available_but_variable_has_default_value_needed_is_valid( +) -> anyhow::Result<()> { + let resolver = ResolverTester::new() + .with_variable("api_key", Some("super-secret-key")) + .make_resolver()?; + + resolver.ensure_required_variables_resolvable().await?; + + Ok(()) +} + +#[derive(Debug)] +struct StaticProvider { + variables: HashMap>, +} + +impl StaticProvider { + fn with_variable(key: &str, value: Option<&str>) -> Self { + Self { + variables: HashMap::from([(key.into(), value.map(|v| v.into()))]), + } + } +} + +#[async_trait] +impl Provider for StaticProvider { + async fn get(&self, key: &Key) -> anyhow::Result> { + Ok(self.variables.get(key.as_str()).cloned().flatten()) + } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Static + } +} + +#[derive(Debug)] +struct DynamicProvider; + +#[async_trait] +impl Provider for DynamicProvider { + async fn get(&self, _key: &Key) -> anyhow::Result> { + panic!("validation should never call get for a dynamic provider") + } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Dynamic + } +} diff --git a/crates/factor-variables/src/lib.rs b/crates/factor-variables/src/lib.rs index fc60dfed24..b75824b9ba 100644 --- a/crates/factor-variables/src/lib.rs +++ b/crates/factor-variables/src/lib.rs @@ -85,6 +85,10 @@ impl AppState { let template = Template::new(expr)?; self.expression_resolver.resolve_template(&template).await } + + pub fn expression_resolver(&self) -> &Arc { + &self.expression_resolver + } } pub struct InstanceState { diff --git a/crates/factor-variables/tests/factor_test.rs b/crates/factor-variables/tests/factor_test.rs index 40e8be773d..bbdf3e2428 100644 --- a/crates/factor-variables/tests/factor_test.rs +++ b/crates/factor-variables/tests/factor_test.rs @@ -1,4 +1,4 @@ -use spin_expressions::{Key, Provider}; +use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; use spin_factor_variables::{runtime_config::RuntimeConfig, VariablesFactor}; use spin_factors::{anyhow, RuntimeFactors}; use spin_factors_test::{toml, TestEnvironment}; @@ -46,4 +46,8 @@ impl Provider for MockProvider { _ => Ok(None), } } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Static + } } diff --git a/crates/runtime-factors/src/build.rs b/crates/runtime-factors/src/build.rs index 11eb108493..c09ee39337 100644 --- a/crates/runtime-factors/src/build.rs +++ b/crates/runtime-factors/src/build.rs @@ -8,7 +8,7 @@ use spin_runtime_config::ResolvedRuntimeConfig; use spin_trigger::cli::{ FactorsConfig, InitialKvSetterHook, KeyValueDefaultStoreSummaryHook, MaxInstanceMemoryHook, RuntimeFactorsBuilder, SqlStatementExecutorHook, SqliteDefaultStoreSummaryHook, - StdioLoggingExecutorHooks, + StdioLoggingExecutorHooks, VariablesValidatorHook, }; use spin_variables_static::StaticVariablesProvider; @@ -72,6 +72,7 @@ impl RuntimeFactorsBuilder for FactorsBuilder { executor.add_hooks(InitialKvSetterHook::new(args.key_values.clone())); executor.add_hooks(SqliteDefaultStoreSummaryHook); executor.add_hooks(KeyValueDefaultStoreSummaryHook); + executor.add_hooks(VariablesValidatorHook); let max_instance_memory = args .max_instance_memory diff --git a/crates/trigger/Cargo.toml b/crates/trigger/Cargo.toml index 44db6ff910..7cffd25f02 100644 --- a/crates/trigger/Cargo.toml +++ b/crates/trigger/Cargo.toml @@ -28,6 +28,7 @@ spin-compose = { path = "../compose" } spin-core = { path = "../core" } spin-factor-key-value = { path = "../factor-key-value" } spin-factor-sqlite = { path = "../factor-sqlite" } +spin-factor-variables = { path = "../factor-variables" } spin-factor-wasi = { path = "../factor-wasi" } spin-factors = { path = "../factors" } spin-factors-executor = { path = "../factors-executor" } diff --git a/crates/trigger/src/cli.rs b/crates/trigger/src/cli.rs index 2f41ba1b72..bd14d44b8e 100644 --- a/crates/trigger/src/cli.rs +++ b/crates/trigger/src/cli.rs @@ -4,6 +4,7 @@ mod max_instance_memory; mod sqlite_statements; mod stdio; mod summary; +mod variable; use std::path::PathBuf; use std::{future::Future, sync::Arc}; @@ -25,6 +26,7 @@ pub use sqlite_statements::SqlStatementExecutorHook; use stdio::FollowComponents; pub use stdio::StdioLoggingExecutorHooks; pub use summary::{KeyValueDefaultStoreSummaryHook, SqliteDefaultStoreSummaryHook}; +pub use variable::VariablesValidatorHook; pub const APP_LOG_DIR: &str = "APP_LOG_DIR"; pub const SPIN_TRUNCATE_LOGS: &str = "SPIN_TRUNCATE_LOGS"; diff --git a/crates/trigger/src/cli/variable.rs b/crates/trigger/src/cli/variable.rs new file mode 100644 index 0000000000..71a0db3f81 --- /dev/null +++ b/crates/trigger/src/cli/variable.rs @@ -0,0 +1,23 @@ +use spin_factor_variables::VariablesFactor; +use spin_factors::RuntimeFactors; +use spin_factors_executor::ExecutorHooks; + +/// An executor hook that prepares the variables factor before runtime execution. +pub struct VariablesValidatorHook; + +#[spin_core::async_trait] +impl ExecutorHooks for VariablesValidatorHook { + async fn configure_app( + &self, + configured_app: &spin_factors::ConfiguredApp, + ) -> anyhow::Result<()> { + let variables_factor_app_state = configured_app.app_state::()?; + + let expression_resolver = variables_factor_app_state.expression_resolver(); + expression_resolver + .ensure_required_variables_resolvable() + .await?; + + Ok(()) + } +} diff --git a/crates/variables-azure/src/lib.rs b/crates/variables-azure/src/lib.rs index 691c980a2b..908c86c95b 100644 --- a/crates/variables-azure/src/lib.rs +++ b/crates/variables-azure/src/lib.rs @@ -4,7 +4,7 @@ use anyhow::Context as _; use azure_core::{auth::TokenCredential, Url}; use azure_security_keyvault::SecretClient; use serde::Deserialize; -use spin_expressions::{Key, Provider}; +use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; use spin_factors::anyhow; use spin_world::async_trait; use tracing::{instrument, Level}; @@ -151,6 +151,10 @@ impl Provider for AzureKeyVaultProvider { .context("Failed to read variable from Azure Key Vault")?; Ok(Some(secret.value)) } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Dynamic + } } impl From for Url { diff --git a/crates/variables-env/src/lib.rs b/crates/variables-env/src/lib.rs index 6976b00ca3..8ea94073a8 100644 --- a/crates/variables-env/src/lib.rs +++ b/crates/variables-env/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; use serde::Deserialize; -use spin_expressions::{Key, Provider}; +use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; use spin_factors::anyhow::{self, Context as _}; use spin_world::async_trait; use tracing::{instrument, Level}; @@ -132,6 +132,10 @@ impl Provider for EnvVariablesProvider { async fn get(&self, key: &Key) -> anyhow::Result> { tokio::task::block_in_place(|| self.get_sync(key)) } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Static + } } #[cfg(test)] diff --git a/crates/variables-static/src/lib.rs b/crates/variables-static/src/lib.rs index 4de067fc71..da69c77c35 100644 --- a/crates/variables-static/src/lib.rs +++ b/crates/variables-static/src/lib.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, hash::Hash, sync::Arc}; use serde::Deserialize; -use spin_expressions::{async_trait::async_trait, Key, Provider}; +use spin_expressions::{async_trait::async_trait, provider::ProviderVariableKind, Key, Provider}; use spin_factors::anyhow; pub use source::*; @@ -18,6 +18,10 @@ impl Provider for StaticVariablesProvider { async fn get(&self, key: &Key) -> anyhow::Result> { Ok(self.values.get(key.as_str()).cloned()) } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Static + } } impl StaticVariablesProvider { diff --git a/crates/variables-vault/src/lib.rs b/crates/variables-vault/src/lib.rs index 0bd7e8ed6c..bfdba99520 100644 --- a/crates/variables-vault/src/lib.rs +++ b/crates/variables-vault/src/lib.rs @@ -1,5 +1,5 @@ use serde::{Deserialize, Serialize}; -use spin_expressions::async_trait::async_trait; +use spin_expressions::{async_trait::async_trait, provider::ProviderVariableKind}; use spin_factors::anyhow::{self, Context as _}; use tracing::{instrument, Level}; use vaultrs::{ @@ -52,4 +52,8 @@ impl Provider for VaultVariablesProvider { Err(e) => Err(e).context("Failed to check Vault for config"), } } + + fn kind(&self) -> ProviderVariableKind { + ProviderVariableKind::Dynamic + } }