diff --git a/crates/plugins/src/manifest.rs b/crates/plugins/src/manifest.rs index e4f73260cd..5143077472 100644 --- a/crates/plugins/src/manifest.rs +++ b/crates/plugins/src/manifest.rs @@ -1,3 +1,5 @@ +use std::io::IsTerminal; + use anyhow::{anyhow, Context, Result}; use semver::{Version, VersionReq}; use serde::{Deserialize, Serialize}; @@ -181,7 +183,9 @@ fn inner_warn_unsupported_version( if !is_version_fully_compatible(supported_on, spin_version)? { let version = Version::parse(spin_version)?; if !version.pre.is_empty() { - terminal::warn!("You're using a pre-release version of Spin ({spin_version}). This plugin might not be compatible (supported: {supported_on}). Continuing anyway."); + if std::io::stderr().is_terminal() { + terminal::warn!("You're using a pre-release version of Spin ({spin_version}). This plugin might not be compatible (supported: {supported_on}). Continuing anyway."); + } } else if override_compatibility_check { terminal::warn!("Plugin is not compatible with this version of Spin (supported: {supported_on}, actual: {spin_version}). Check overridden ... continuing to install or execute plugin."); } else { diff --git a/crates/trigger/src/cli.rs b/crates/trigger/src/cli.rs index 0e0e4b802a..008d87b39b 100644 --- a/crates/trigger/src/cli.rs +++ b/crates/trigger/src/cli.rs @@ -17,6 +17,9 @@ use crate::{ }; use crate::{TriggerExecutor, TriggerExecutorBuilder}; +mod launch_metadata; +pub use launch_metadata::LaunchMetadata; + pub const APP_LOG_DIR: &str = "APP_LOG_DIR"; pub const DISABLE_WASMTIME_CACHE: &str = "DISABLE_WASMTIME_CACHE"; pub const FOLLOW_LOG_OPT: &str = "FOLLOW_ID"; @@ -125,6 +128,9 @@ where #[clap(long = "help-args-only", hide = true)] pub help_args_only: bool, + + #[clap(long = "launch-metadata-only", hide = true)] + pub launch_metadata_only: bool, } /// An empty implementation of clap::Args to be used as TriggerExecutor::RunConfig @@ -147,6 +153,13 @@ where return Ok(()); } + if self.launch_metadata_only { + let lm = LaunchMetadata::infer::(); + let json = serde_json::to_string_pretty(&lm)?; + eprintln!("{json}"); + return Ok(()); + } + // Required env vars let working_dir = std::env::var(SPIN_WORKING_DIR).context(SPIN_WORKING_DIR)?; let locked_url = std::env::var(SPIN_LOCKED_URL).context(SPIN_LOCKED_URL)?; diff --git a/crates/trigger/src/cli/launch_metadata.rs b/crates/trigger/src/cli/launch_metadata.rs new file mode 100644 index 0000000000..0be69b2824 --- /dev/null +++ b/crates/trigger/src/cli/launch_metadata.rs @@ -0,0 +1,86 @@ +use clap::{Args, CommandFactory}; +use serde::{Deserialize, Serialize}; +use std::ffi::OsString; + +use crate::{cli::TriggerExecutorCommand, TriggerExecutor}; + +/// Contains information about the trigger flags (and potentially +/// in future configuration) that a consumer (such as `spin up`) +/// can query using `--launch-metadata-only`. +#[derive(Clone, Debug, Deserialize, Serialize)] +pub struct LaunchMetadata { + all_flags: Vec, +} + +// This assumes no triggers that want to participate in multi-trigger +// use positional arguments. This is a restriction we'll have to make +// anyway: suppose triggers A and B both take one positional arg, and +// the user writes `spin up 123 456` - which value would go to which trigger? +#[derive(Clone, Debug, PartialEq, Deserialize, Serialize)] +struct LaunchFlag { + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + short: Option, + #[serde(skip_serializing_if = "Option::is_none")] + #[serde(default)] + long: Option, +} + +impl LaunchMetadata { + pub fn infer() -> Self + where + Executor::RunConfig: Args, + { + let all_flags: Vec<_> = TriggerExecutorCommand::::command() + .get_arguments() + .map(LaunchFlag::infer) + .collect(); + + LaunchMetadata { all_flags } + } + + pub fn matches<'a>(&self, groups: &[Vec<&'a OsString>]) -> Vec<&'a OsString> { + let mut matches = vec![]; + + for group in groups { + if group.is_empty() { + continue; + } + if self.is_match(group[0]) { + matches.extend(group); + } + } + + matches + } + + fn is_match(&self, arg: &OsString) -> bool { + self.all_flags.iter().any(|f| f.is_match(arg)) + } + + pub fn is_group_match(&self, group: &[&OsString]) -> bool { + if group.is_empty() { + false + } else { + self.all_flags.iter().any(|f| f.is_match(group[0])) + } + } +} + +impl LaunchFlag { + fn infer(arg: &clap::Arg) -> Self { + Self { + long: arg.get_long().map(|s| format!("--{s}")), + short: arg.get_short().map(|ch| format!("-{ch}")), + } + } + + fn is_match(&self, candidate: &OsString) -> bool { + let Some(s) = candidate.to_str() else { + return false; + }; + let candidate = Some(s.to_owned()); + + candidate == self.long || candidate == self.short + } +} diff --git a/src/commands/up.rs b/src/commands/up.rs index abefa1ef90..ee6fbe30c1 100644 --- a/src/commands/up.rs +++ b/src/commands/up.rs @@ -1,9 +1,11 @@ mod app_source; use std::{ + collections::HashMap, ffi::OsString, fmt::Debug, path::{Path, PathBuf}, + process::Stdio, }; use anyhow::{anyhow, bail, Context, Result}; @@ -13,7 +15,7 @@ use spin_app::locked::LockedApp; use spin_common::ui::quoted_path; use spin_loader::FilesMountStrategy; use spin_oci::OciLoader; -use spin_trigger::cli::{SPIN_LOCAL_APP_DIR, SPIN_LOCKED_URL, SPIN_WORKING_DIR}; +use spin_trigger::cli::{LaunchMetadata, SPIN_LOCAL_APP_DIR, SPIN_LOCKED_URL, SPIN_WORKING_DIR}; use tempfile::TempDir; use crate::opts::*; @@ -140,7 +142,7 @@ impl UpCommand { if app_source == AppSource::None { if self.help { let mut child = self - .start_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None) + .start_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None, &[]) .await?; let _ = child.wait().await?; return Ok(()); @@ -172,22 +174,18 @@ impl UpCommand { if is_multi { // For now, only common flags are allowed on multi-trigger apps. let mut child = self - .start_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None) + .start_trigger(trigger_command(HELP_ARGS_ONLY_TRIGGER_TYPE), None, &[]) .await?; let _ = child.wait().await?; return Ok(()); } for cmd in trigger_cmds { - let mut help_process = self.start_trigger(cmd.clone(), None).await?; + let mut help_process = self.start_trigger(cmd.clone(), None, &[]).await?; _ = help_process.wait().await; } return Ok(()); } - if is_multi { - self.allow_only_common_flags()?; - } - let mut locked_app = self .load_resolved_app_source(resolved_app_source, &working_dir) .await?; @@ -249,17 +247,64 @@ impl UpCommand { Ok(working_dir_holder) } + async fn get_trigger_launch_metas( + &self, + trigger_cmds: &[Vec], + ) -> anyhow::Result, LaunchMetadata>> { + let mut metas = HashMap::new(); + + for trigger_cmd in trigger_cmds { + let mut meta_cmd = tokio::process::Command::new(std::env::current_exe().unwrap()); + meta_cmd.args(trigger_cmd); + meta_cmd.arg("--launch-metadata-only"); + meta_cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + let meta_out = meta_cmd.spawn()?.wait_with_output().await?; + let meta = serde_json::from_slice::(&meta_out.stderr)?; + metas.insert(trigger_cmd.clone(), meta); + } + + Ok(metas) + } + async fn start_trigger_processes( self, trigger_cmds: Vec>, run_opts: RunTriggerOpts, ) -> anyhow::Result> { let is_multi = trigger_cmds.len() > 1; + + let trigger_args = self.group_trigger_args(); + let trigger_metas = if is_multi { + match self.get_trigger_launch_metas(&trigger_cmds).await { + Ok(m) => Some(m), + Err(e) => { + tracing::warn!("Error getting trigger launch meta - allowing all. {e:?}"); + None + } + } + } else { + None + }; + + if let Some(trigger_metas) = trigger_metas.as_ref() { + for group in &trigger_args { + let is_accepted = trigger_metas.values().any(|m| m.is_group_match(group)); + if !is_accepted { + bail!("Unrecognised trigger option {:?}", group[0]); + } + } + } + let mut trigger_processes = Vec::with_capacity(trigger_cmds.len()); for cmd in trigger_cmds { + let meta = trigger_metas.as_ref().and_then(|ms| ms.get(&cmd)); + let trigger_args = match meta { + Some(m) => m.matches(&trigger_args), + None => self.trigger_args.iter().collect(), + }; let child = self - .start_trigger(cmd.clone(), Some(run_opts.clone())) + .start_trigger(cmd.clone(), Some(run_opts.clone()), &trigger_args) .await .context("Failed to start trigger process")?; trigger_processes.push(child); @@ -279,6 +324,7 @@ impl UpCommand { &self, trigger_cmd: Vec, opts: Option, + trigger_args: &[&OsString], ) -> Result { // The docs for `current_exe` warn that this may be insecure because it could be executed // via hard-link. I think it should be fine as long as we aren't `setuid`ing this binary. @@ -293,7 +339,7 @@ impl UpCommand { { cmd.env(SPIN_LOCKED_URL, locked_url) .env(SPIN_WORKING_DIR, &working_dir) - .args(&self.trigger_args); + .args(trigger_args); if let Some(local_app_dir) = local_app_dir { cmd.env(SPIN_LOCAL_APP_DIR, local_app_dir); @@ -424,56 +470,42 @@ impl UpCommand { } } - fn allow_only_common_flags(&self) -> anyhow::Result<()> { - allow_only_common_flags(&self.trigger_args) - } -} + fn group_trigger_args(&self) -> Vec> { + let mut groups = vec![]; -const COMMON_FLAGS: &[&str] = &[ - "-L", - "--log-dir", - "--disable-cache", - "--cache", - "--disable-pooling", - "--follow", - "-q", - "--quiet", - "--sh", - "--shush", - "--allow-transient-write", - "--runtime-config-file", - "--state-dir", - "--key-value", - "--sqlite", - "--help-args-only", -]; - -fn allow_only_common_flags(args: &[OsString]) -> anyhow::Result<()> { - for i in 0..(args.len()) { - let Some(value) = args[i].to_str() else { - // Err on the side of forgiveness - continue; - }; - if value.starts_with('-') { - // Flag candidate. Is it allowed? - if !is_common_flag(value) { - anyhow::bail!("Cannot use trigger option '{value}'. Apps with multiple trigger types do not yet support options specific to one trigger type."); - } - } else if i >= 1 { - // Value candidate. Does it immediately follow a flag? - let Some(prev) = args[i - 1].to_str() else { - continue; - }; - if !prev.starts_with('-') { - anyhow::bail!("Cannot use trigger option '{value}'. Apps with multiple trigger types do not yet support options specific to one trigger type."); + let mut pending_group: Option> = None; + + for arg in &self.trigger_args { + if is_flag_arg(arg) { + if let Some(prev_group) = pending_group.take() { + if !prev_group.is_empty() { + groups.push(prev_group); + } + } + pending_group = Some(vec![arg]); + } else if let Some(mut pending) = pending_group.take() { + // it's (probably) a value associated with the flag - append, push, group done + pending.push(arg); + groups.push(pending); + } else { + // it's positional, oh no + groups.push(vec![arg]); } } + if let Some(pending) = pending_group.take() { + groups.push(pending); + } + + groups } - Ok(()) } -fn is_common_flag(candidate: &str) -> bool { - COMMON_FLAGS.contains(&candidate) +fn is_flag_arg(arg: &OsString) -> bool { + if let Some(s) = arg.to_str() { + s.starts_with('-') + } else { + false + } } #[cfg(windows)] @@ -769,26 +801,72 @@ mod test { } #[test] - fn multi_accept_no_trigger_args() { - allow_only_common_flags(&[]).expect("should allow no trigger args"); + fn group_no_args_is_empty() { + let cmd = UpCommand::try_parse_from(["up"]).unwrap(); + let groups = cmd.group_trigger_args(); + assert!(groups.is_empty()); } #[test] - fn multi_accept_only_common_args() { - let args: Vec<_> = ["--log-dir", "/fie", "-q", "--sqlite", "SOME SQL"] - .iter() - .map(OsString::from) - .collect(); - allow_only_common_flags(&args).expect("should allow common trigger args"); + fn can_group_valueful_flags() { + let cmd = + UpCommand::try_parse_from(["up", "--listen", "127.0.0.1:39453", "-L", "/fie"]).unwrap(); + let groups = cmd.group_trigger_args(); + assert_eq!(2, groups.len()); + assert_eq!(2, groups[0].len()); + assert_eq!("--listen", groups[0][0]); + assert_eq!("127.0.0.1:39453", groups[0][1]); + assert_eq!(2, groups[1].len()); + assert_eq!("-L", groups[1][0]); + assert_eq!("/fie", groups[1][1]); } #[test] - fn multi_reject_trigger_specific_args() { - let args: Vec<_> = ["--log-dir", "/fie", "--listen", "some.address"] - .iter() - .map(OsString::from) - .collect(); - let e = allow_only_common_flags(&args).expect_err("should reject trigger-specific args"); - assert!(e.to_string().contains("'--listen'")); + fn can_group_valueful_flags_with_valueless() { + let cmd = UpCommand::try_parse_from([ + "up", + "--listen", + "127.0.0.1:39453", + "--shush", + "--allow-transient-writes", + "-L", + "/fie", + ]) + .unwrap(); + let groups = cmd.group_trigger_args(); + assert_eq!(4, groups.len()); + assert_eq!(2, groups[0].len()); + assert_eq!("--listen", groups[0][0]); + assert_eq!("127.0.0.1:39453", groups[0][1]); + assert_eq!(1, groups[1].len()); + assert_eq!("--shush", groups[1][0]); + assert_eq!(1, groups[2].len()); + assert_eq!("--allow-transient-writes", groups[2][0]); + assert_eq!(2, groups[3].len()); + assert_eq!("-L", groups[3][0]); + assert_eq!("/fie", groups[3][1]); + } + + #[test] + fn can_group_valueful_flags_with_extraneous_values() { + let cmd = UpCommand::try_parse_from([ + "up", + "--listen", + "127.0.0.1:39453", + "HELLO MUM", + "-L", + "/fie", + ]) + .unwrap(); + let groups = cmd.group_trigger_args(); + assert_eq!(3, groups.len()); + assert_eq!(2, groups[0].len()); + assert_eq!("--listen", groups[0][0]); + assert_eq!("127.0.0.1:39453", groups[0][1]); + assert_eq!(1, groups[1].len()); + assert_eq!("HELLO MUM", groups[1][0]); + assert_eq!(2, groups[2].len()); + assert_eq!("-L", groups[2][0]); + assert_eq!("/fie", groups[2][1]); } }