Skip to content

Spin up should check required variables #3213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 11 additions & 10 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,13 @@ spin-oci = { path = "crates/oci" }
spin-plugins = { path = "crates/plugins" }
spin-runtime-factors = { path = "crates/runtime-factors" }
spin-telemetry = { path = "crates/telemetry", features = [
"tracing-log-compat",
"tracing-log-compat",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few of these unrelated layout changes - maybe turn off auto formatting for this file?

] }
spin-templates = { path = "crates/templates" }
spin-trigger = { path = "crates/trigger" }
spin-trigger-http = { path = "crates/trigger-http" }
spin-trigger-redis = { path = "crates/trigger-redis" }
spin-variables = { path = "crates/variables" }
terminal = { path = "crates/terminal" }

[target.'cfg(target_os = "linux")'.dependencies]
Expand All @@ -95,10 +96,10 @@ testing-framework = { path = "tests/testing-framework" }
[build-dependencies]
cargo-target-dep = { git = "https://github.com/fermyon/cargo-target-dep", rev = "482f269eceb7b1a7e8fc618bf8c082dd24979cf1" }
vergen = { version = "^8.2.1", default-features = false, features = [
"build",
"git",
"gitcl",
"cargo",
"build",
"git",
"gitcl",
"cargo",
] }

[features]
Expand All @@ -111,10 +112,10 @@ llm-cublas = ["llm", "spin-runtime-factors/llm-cublas"]

[workspace]
members = [
"crates/*",
"tests/conformance-tests",
"tests/runtime-tests",
"tests/testing-framework",
"crates/*",
"tests/conformance-tests",
"tests/runtime-tests",
"tests/testing-framework",
]

[workspace.dependencies]
Expand Down Expand Up @@ -186,4 +187,4 @@ blocks_in_conditions = "allow"

[[bin]]
name = "spin"
path = "src/bin/spin.rs"
path = "src/bin/spin.rs"
12 changes: 12 additions & 0 deletions crates/common/src/env.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//! Environment variable utilities

/// Defines the default environment variable prefix used by Spin.
pub const DEFAULT_ENV_PREFIX: &str = "SPIN_VARIABLE";

/// Creates an environment variable key based on the given prefix and key.
pub fn env_key(prefix: Option<String>, key: &str) -> String {
let prefix = prefix.unwrap_or_else(|| DEFAULT_ENV_PREFIX.to_string());
let upper_key = key.to_ascii_uppercase();
let key = format!("{prefix}_{upper_key}");
key
}
1 change: 1 addition & 0 deletions crates/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

pub mod arg_parser;
pub mod data_dir;
pub mod env;
pub mod paths;
pub mod sha256;
pub mod sloth;
Expand Down
1 change: 1 addition & 0 deletions crates/factors-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ edition = { workspace = true }

[dependencies]
spin-app = { path = "../app" }
spin-common = { path = "../common" }
spin-factors = { path = "../factors" }
spin-loader = { path = "../loader" }
spin-telemetry = { path = "../telemetry", features = ["testing"] }
Expand Down
3 changes: 3 additions & 0 deletions crates/factors-test/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use spin_app::locked::LockedApp;
use spin_common::env::env_key;
use spin_factors::{
anyhow::{self, Context},
wasmtime::{component::Linker, Config, Engine},
Expand Down Expand Up @@ -100,6 +101,8 @@ impl<T: RuntimeFactors> TestEnvironment<T> {
pub async fn build_locked_app(manifest: &toml::Table) -> anyhow::Result<LockedApp> {
let toml_str = toml::to_string(manifest).context("failed serializing manifest")?;
let dir = tempfile::tempdir().context("failed creating tempdir")?;
// `foo` variable is set to require. As we're not providing a default value, env is checked.
std::env::set_var(env_key(None, "foo"), "baz");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still say we should be able to transform a TOML manifest into a LockedApp without doing validation on variables.

let path = dir.path().join("spin.toml");
std::fs::write(&path, toml_str).context("failed writing manifest")?;
spin_loader::from_file(&path, FilesMountStrategy::Direct, None).await
Expand Down
1 change: 1 addition & 0 deletions crates/loader/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ spin-locked-app = { path = "../locked-app" }
spin-manifest = { path = "../manifest" }
spin-outbound-networking-config = { path = "../outbound-networking-config" }
spin-serde = { path = "../serde" }
spin-variables = { path = "../variables" }
tempfile = { workspace = true }
terminal = { path = "../terminal" }
tokio = { workspace = true }
Expand Down
5 changes: 4 additions & 1 deletion crates/runtime-factors/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use spin_runtime_config::ResolvedRuntimeConfig;
use spin_trigger::cli::{
FactorsConfig, InitialKvSetterHook, KeyValueDefaultStoreSummaryHook, MaxInstanceMemoryHook,
RuntimeFactorsBuilder, SqlStatementExecutorHook, SqliteDefaultStoreSummaryHook,
StdioLoggingExecutorHooks,
StdioLoggingExecutorHooks, VariableSorterExecutorHooks,
};

/// A [`RuntimeFactorsBuilder`] for [`TriggerFactors`].
Expand Down Expand Up @@ -58,6 +58,9 @@ impl RuntimeFactorsBuilder for FactorsBuilder {
executor.add_hooks(InitialKvSetterHook::new(args.key_values.clone()));
executor.add_hooks(SqliteDefaultStoreSummaryHook);
executor.add_hooks(KeyValueDefaultStoreSummaryHook);
executor.add_hooks(VariableSorterExecutorHooks::new(
runtime_config.toml.clone(),
));

let max_instance_memory = args
.max_instance_memory
Expand Down
2 changes: 2 additions & 0 deletions crates/trigger/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ spin-factor-wasi = { path = "../factor-wasi" }
spin-factors = { path = "../factors" }
spin-factors-executor = { path = "../factors-executor" }
spin-telemetry = { path = "../telemetry" }
spin-variables = { path = "../variables" }
tokio = { workspace = true, features = ["fs", "rt"] }
toml = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Ryan had tried to push TOML dependencies into the runtime config crate, so that the trigger crate was abstracted from the physical representation of the runtime config? (For e.g. SpinKube scenarios.) I am not sure though.

tracing = { workspace = true }

[dev-dependencies]
Expand Down
2 changes: 2 additions & 0 deletions crates/trigger/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ mod max_instance_memory;
mod sqlite_statements;
mod stdio;
mod summary;
mod variables;

use std::path::PathBuf;
use std::{future::Future, sync::Arc};
Expand All @@ -25,6 +26,7 @@ pub use sqlite_statements::SqlStatementExecutorHook;
use stdio::FollowComponents;
pub use stdio::StdioLoggingExecutorHooks;
pub use summary::{KeyValueDefaultStoreSummaryHook, SqliteDefaultStoreSummaryHook};
pub use variables::VariableSorterExecutorHooks;

pub const APP_LOG_DIR: &str = "APP_LOG_DIR";
pub const SPIN_TRUNCATE_LOGS: &str = "SPIN_TRUNCATE_LOGS";
Expand Down
44 changes: 44 additions & 0 deletions crates/trigger/src/cli/variables.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
use spin_core::async_trait;
use spin_factors::RuntimeFactors;
use spin_factors_executor::ExecutorHooks;
use spin_variables::{VariableProviderConfiguration, VariableSourcer};

/// Implements TriggerHooks, sorting required variables
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting? Maybe comment on why they need sorting?

pub struct VariableSorterExecutorHooks {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the naming here. Why is it an "Executor" hook?

table: toml::Table,
}

impl VariableSorterExecutorHooks {
pub fn new(table: toml::Table) -> Self {
Self { table }
}
}

#[async_trait]
impl<F: RuntimeFactors, U> ExecutorHooks<F, U> for VariableSorterExecutorHooks {
async fn configure_app(
&self,
configured_app: &spin_factors::ConfiguredApp<F>,
) -> anyhow::Result<()> {
for (key, variable) in configured_app.app().variables() {
self.variable_env_checker(key.clone(), variable.clone())?;
}
Ok(())
}
}

impl VariableSourcer for VariableSorterExecutorHooks {
fn variable_env_checker(&self, key: String, val: spin_app::Variable) -> anyhow::Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable_env_checker is a noun. I think the idea here is something like check_variable_has_value or check_variable_resolves.

let configs = spin_variables::variable_provider_config_from_toml(&self.table)?;

if let Some(config) = configs.into_iter().next() {
let (dotenv_path, prefix) = match config {
VariableProviderConfiguration::Env(env) => (env.dotenv_path, env.prefix),
_ => (None, None),
};
return self.check(key, val, dotenv_path, prefix);
}

Err(anyhow::anyhow!("No environment variable provider found"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what "provider" refers to in this context. Have I misunderstood the purpose of this function and it's actually trying to check if the env provider is present?

}
}
2 changes: 2 additions & 0 deletions crates/variables/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ azure_identity = { git = "https://github.com/azure/azure-sdk-for-rust", rev = "8
azure_security_keyvault = { git = "https://github.com/azure/azure-sdk-for-rust", rev = "8c4caa251c3903d5eae848b41bb1d02a4d65231c" }
dotenvy = "0.15"
serde = { workspace = true }
spin-common = { path = "../common" }
spin-expressions = { path = "../expressions" }
spin-factor-variables = { path = "../factor-variables" }
spin-factors = { path = "../factors" }
spin-locked-app = { path = "../locked-app" }
spin-world = { path = "../world" }
tokio = { workspace = true, features = ["rt-multi-thread"] }
tracing = { workspace = true }
Expand Down
12 changes: 2 additions & 10 deletions crates/variables/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::{
};

use serde::Deserialize;
use spin_common::env::env_key;
use spin_expressions::{Key, Provider};
use spin_factors::anyhow::{self, Context as _};
use spin_world::async_trait;
Expand All @@ -25,8 +26,6 @@ pub struct EnvVariablesConfig {
pub dotenv_path: Option<PathBuf>,
}

const DEFAULT_ENV_PREFIX: &str = "SPIN_VARIABLE";

type EnvFetcherFn = Box<dyn Fn(&str) -> Result<String, VarError> + Send + Sync>;

/// A [`Provider`] that uses environment variables.
Expand Down Expand Up @@ -71,14 +70,7 @@ impl EnvVariablesProvider {

/// Gets the value of a variable from the environment.
fn get_sync(&self, key: &Key) -> anyhow::Result<Option<String>> {
let prefix = self
.prefix
.clone()
.unwrap_or_else(|| DEFAULT_ENV_PREFIX.to_string());

let upper_key = key.as_ref().to_ascii_uppercase();
let env_key = format!("{prefix}_{upper_key}");

let env_key = env_key(self.prefix.clone(), key.as_ref());
self.query_env(&env_key)
}

Expand Down
53 changes: 53 additions & 0 deletions crates/variables/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ mod env;
mod statik;
mod vault;

use std::path::PathBuf;

pub use azure_key_vault::*;
pub use env::*;
use spin_common::{env::env_key, ui::quoted_path};
use spin_locked_app::Variable;
pub use statik::*;
pub use vault::*;

Expand Down Expand Up @@ -38,6 +42,24 @@ pub fn runtime_config_from_toml(table: &impl GetTomlValue) -> anyhow::Result<Run
Ok(RuntimeConfig { providers })
}

pub fn variable_provider_config_from_toml(
table: &impl GetTomlValue,
) -> anyhow::Result<Vec<VariableProviderConfiguration>> {
if let Some(array) = table
.get("variables_provider")
.or_else(|| table.get("config_provider"))
{
array
.clone()
.try_into::<Vec<VariableProviderConfiguration>>()
.map_err(|e| anyhow::anyhow!("Failed to parse variable provider configuration: {}", e))
} else {
Ok(vec![VariableProviderConfiguration::Env(
EnvVariablesConfig::default(),
)])
}
}

/// A runtime configuration used in the Spin CLI for one type of variable provider.
#[derive(Debug, Deserialize)]
#[serde(rename_all = "snake_case", tag = "type")]
Expand Down Expand Up @@ -70,3 +92,34 @@ impl VariableProviderConfiguration {
Ok(provider)
}
}

pub trait VariableSourcer {
fn variable_env_checker(&self, key: String, val: Variable) -> anyhow::Result<()>;

fn check(
&self,
key: String,
mut val: Variable,
dotenv_path: Option<PathBuf>,
prefix: Option<String>,
) -> anyhow::Result<()> {
if val.default.is_some() {
return Ok(());
}

if let Some(path) = dotenv_path {
_ = std::env::set_current_dir(path);
}

match std::env::var(env_key(prefix, &key)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't do this. There are providers other than the environment one. It is legal for a variable to come from Vault or Azure KeyVault or the static provider rather than the environment one. I tried to stress this in my previous review, sorry - this is fundamental.

Ok(v) => {
val.default = Some(v);
Ok(())
}
Err(_) => Err(anyhow::anyhow!(
"Variable data not provided for {}",
quoted_path(key)
)),
}
}
}
6 changes: 6 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,13 @@ mod integration_tests {
use std::collections::HashMap;

use crate::testcases::run_test_inited;
use spin_common::env::env_key;

const VAULT_ROOT_TOKEN: &str = "root";

// `password` variable is set to require. As we're not providing a default value, env is checked.
std::env::set_var(env_key(None, "password"), "password");

run_test_inited(
"vault-variables-test",
SpinConfig {
Expand Down
Loading