Skip to content

Fix subsets inheriting service chaining requirement from whole app #3093

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix subsets inheriting service chaining requirement from whole app #3093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 40 additions & 3 deletions crates/factor-outbound-networking/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ use std::ops::Range;

use anyhow::{bail, ensure, Context};
use spin_factors::{App, AppComponent};
use spin_locked_app::MetadataKey;
use spin_locked_app::{
locked::{LockedApp, LockedComponent},
MetadataKey,
};

const ALLOWED_HOSTS_KEY: MetadataKey<Vec<String>> = MetadataKey::new("allowed_outbound_hosts");
const ALLOWED_HTTP_KEY: MetadataKey<Vec<String>> = MetadataKey::new("allowed_http_hosts");
Expand All @@ -14,8 +17,15 @@ pub const SERVICE_CHAINING_DOMAIN_SUFFIX: &str = ".spin.internal";
///
/// This has support for converting the old `allowed_http_hosts` key to the new `allowed_outbound_hosts` key.
pub fn allowed_outbound_hosts(component: &AppComponent) -> anyhow::Result<Vec<String>> {
allowed_outbound_hosts_locked(component.locked)
}

fn allowed_outbound_hosts_locked(component: &LockedComponent) -> anyhow::Result<Vec<String>> {
use spin_locked_app::MetadataExt;

let mut allowed_hosts = component
.get_metadata(ALLOWED_HOSTS_KEY)
.metadata
.get_typed(ALLOWED_HOSTS_KEY)
.with_context(|| {
format!(
"locked app metadata was malformed for key {}",
Expand All @@ -24,7 +34,8 @@ pub fn allowed_outbound_hosts(component: &AppComponent) -> anyhow::Result<Vec<St
})?
.unwrap_or_default();
let allowed_http = component
.get_metadata(ALLOWED_HTTP_KEY)
.metadata
.get_typed(ALLOWED_HTTP_KEY)
.map(|h| h.unwrap_or_default())
.unwrap_or_default();
let converted =
Expand Down Expand Up @@ -74,6 +85,32 @@ pub fn validate_service_chaining_for_components(
Ok(())
}

/// If, after we have retained only a subset of components in an app, the remaining
/// ones no longer require service chaining, remove the host requirement from the app.
pub fn update_service_chaining_host_requirement(app: &mut LockedApp) {
use spin_locked_app::locked::SERVICE_CHAINING_KEY;

if !app.host_requirements.contains_key(SERVICE_CHAINING_KEY) {
// The app doesn't use service chaining so save checking.
return;
}

fn uses_service_chaining(c: &LockedComponent) -> bool {
let Ok(allowed_outbound_hosts) = allowed_outbound_hosts_locked(c) else {
return true; // err on the side of keeping the previously inferred requirement
};
allowed_outbound_hosts
.iter()
.filter_map(|h| h.parse().ok()) // don't consider templated hosts
.any(|h| parse_service_chaining_target(&h).is_some())
}

let app_uses_service_chaining = app.components.iter().any(uses_service_chaining);
if !app_uses_service_chaining {
app.host_requirements.remove(SERVICE_CHAINING_KEY);
}
}

/// An address is a url-like string that contains a host, a port, and an optional scheme
#[derive(Eq, Debug, Clone)]
pub struct AllowedHostConfig {
Expand Down
4 changes: 2 additions & 2 deletions crates/factor-outbound-networking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use std::{collections::HashMap, sync::Arc};

pub use config::{
allowed_outbound_hosts, is_service_chaining_host, parse_service_chaining_target,
validate_service_chaining_for_components, AllowedHostConfig, AllowedHostsConfig, HostConfig,
OutboundUrl, SERVICE_CHAINING_DOMAIN_SUFFIX,
update_service_chaining_host_requirement, validate_service_chaining_for_components,
AllowedHostConfig, AllowedHostsConfig, HostConfig, OutboundUrl, SERVICE_CHAINING_DOMAIN_SUFFIX,
};

pub use runtime_config::ComponentTlsConfigs;
Expand Down
101 changes: 74 additions & 27 deletions src/commands/up.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ use clap::{CommandFactory, Parser};
use reqwest::Url;
use spin_app::locked::LockedApp;
use spin_common::ui::quoted_path;
use spin_factor_outbound_networking::validate_service_chaining_for_components;
use spin_factor_outbound_networking::{
update_service_chaining_host_requirement, validate_service_chaining_for_components,
};
use spin_loader::FilesMountStrategy;
use spin_oci::OciLoader;
use spin_trigger::cli::{LaunchMetadata, SPIN_LOCAL_APP_DIR, SPIN_LOCKED_URL, SPIN_WORKING_DIR};
Expand Down Expand Up @@ -184,7 +186,7 @@ impl UpCommand {
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, None, &[]).await?;
_ = help_process.wait().await;
}
return Ok(());
Expand Down Expand Up @@ -213,6 +215,8 @@ impl UpCommand {
)?;
}

self.update_locked_app(&mut locked_app);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This moved up because the borrow of the trigger type strings (in the next stanza) now lasts longer, so I have to get the mutable borrow out the way first, or the compiler gets mad.


let trigger_types: HashSet<&str> = locked_app
.triggers
.iter()
Expand All @@ -225,13 +229,10 @@ impl UpCommand {
.with_context(|| format!("Couldn't find trigger executor for {app_source}"))?;
let is_multi = trigger_cmds.len() > 1;

self.update_locked_app(&mut locked_app);
let locked_url = self.write_locked_app(&locked_app, &working_dir).await?;

let local_app_dir = app_source.local_app_dir().map(Into::into);

let run_opts = RunTriggerOpts {
locked_url,
locked_app: locked_app.clone(),
working_dir,
local_app_dir,
};
Expand Down Expand Up @@ -284,26 +285,26 @@ impl UpCommand {

async fn get_trigger_launch_metas(
&self,
trigger_cmds: &[Vec<String>],
trigger_cmds: &[TriggerCommand<'_>],
) -> anyhow::Result<HashMap<Vec<String>, LaunchMetadata>> {
let mut metas = HashMap::new();

for trigger_cmd in trigger_cmds {
for t in trigger_cmds {
let mut meta_cmd = tokio::process::Command::new(std::env::current_exe().unwrap());
meta_cmd.args(trigger_cmd);
meta_cmd.args(&t.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::<LaunchMetadata>(&meta_out.stderr)?;
metas.insert(trigger_cmd.clone(), meta);
metas.insert(t.cmd.clone(), meta);
}

Ok(metas)
}

async fn start_trigger_processes(
self,
trigger_cmds: Vec<Vec<String>>,
trigger_cmds: Vec<TriggerCommand<'_>>,
run_opts: RunTriggerOpts,
) -> anyhow::Result<Vec<tokio::process::Child>> {
let is_multi = trigger_cmds.len() > 1;
Expand Down Expand Up @@ -333,13 +334,13 @@ impl UpCommand {
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 meta = trigger_metas.as_ref().and_then(|ms| ms.get(&cmd.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()), &trigger_args)
.start_trigger(cmd, Some(run_opts.clone()), &trigger_args)
.await
.context("Failed to start trigger process")?;
trigger_processes.push(child);
Expand All @@ -357,21 +358,25 @@ impl UpCommand {

async fn start_trigger(
&self,
trigger_cmd: Vec<String>,
trigger: TriggerCommand<'_>,
opts: Option<RunTriggerOpts>,
trigger_args: &[&OsString],
) -> Result<tokio::process::Child, anyhow::Error> {
// 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.
let mut cmd = tokio::process::Command::new(std::env::current_exe().unwrap());
cmd.args(&trigger_cmd);
cmd.args(&trigger.cmd);

if let Some(RunTriggerOpts {
locked_url,
locked_app,
working_dir,
local_app_dir,
}) = opts
{
let locked_url = self
.write_locked_app_for_trigger(trigger.type_, &locked_app, &working_dir)
.await?;

cmd.env(SPIN_LOCKED_URL, locked_url)
.env(SPIN_WORKING_DIR, &working_dir)
.args(trigger_args);
Expand Down Expand Up @@ -430,19 +435,33 @@ impl UpCommand {
!self.trigger_args.is_empty() && !self.trigger_args[0].to_string_lossy().starts_with('-')
}

async fn write_locked_app(
async fn write_locked_app_for_trigger(
&self,
trigger_type: &str,
locked_app: &LockedApp,
working_dir: &Path,
) -> Result<String, anyhow::Error> {
let locked_path = working_dir.join("spin.lock");
let locked_app_for_trigger = get_locked_app_for_trigger(trigger_type, locked_app)?;
let locked_path = working_dir.join(format!("spin-{trigger_type}.lock"));
Self::write_locked_app(&locked_app_for_trigger, &locked_path).await
}

async fn write_locked_app(
locked_app: &LockedApp,
lock_file_path: &Path,
) -> Result<String, anyhow::Error> {
let locked_app_contents =
serde_json::to_vec_pretty(&locked_app).context("failed to serialize locked app")?;
tokio::fs::write(&locked_path, locked_app_contents)
tokio::fs::write(&lock_file_path, locked_app_contents)
.await
.with_context(|| format!("failed to write {}", quoted_path(&locked_path)))?;
let locked_url = Url::from_file_path(&locked_path)
.map_err(|_| anyhow!("cannot convert to file URL: {}", quoted_path(&locked_path)))?
.with_context(|| format!("failed to write {}", quoted_path(&lock_file_path)))?;
let locked_url = Url::from_file_path(lock_file_path)
.map_err(|_| {
anyhow!(
"cannot convert to file URL: {}",
quoted_path(&lock_file_path)
)
})?
.to_string();

Ok(locked_url)
Expand Down Expand Up @@ -550,6 +569,23 @@ impl UpCommand {
}
}

fn get_locked_app_for_trigger(
trigger_type: &str,
locked_app: &LockedApp,
) -> Result<LockedApp, anyhow::Error> {
let components = locked_app
.triggers
.iter()
.filter(|t| t.trigger_type == trigger_type)
.filter_map(|t| t.trigger_config.get("component"))
.filter_map(|j| j.as_str())
.collect::<Vec<_>>();
let mut locked_app_for_trigger =
spin_app::retain_components(locked_app.clone(), &components, &[])?;
update_service_chaining_host_requirement(&mut locked_app_for_trigger);
Ok(locked_app_for_trigger)
}

fn is_flag_arg(arg: &OsString) -> bool {
if let Some(s) = arg.to_str() {
s.starts_with('-')
Expand Down Expand Up @@ -602,7 +638,7 @@ fn kill_child_processes(pids: &[nix::unistd::Pid]) {

#[derive(Clone)]
struct RunTriggerOpts {
locked_url: String,
locked_app: LockedApp,
working_dir: PathBuf,
local_app_dir: Option<PathBuf>,
}
Expand Down Expand Up @@ -663,18 +699,29 @@ fn resolve_trigger_plugin(trigger_type: &str) -> Result<String> {
}
}

fn trigger_command(trigger_type: &str) -> Vec<String> {
vec!["trigger".to_owned(), trigger_type.to_owned()]
struct TriggerCommand<'a> {
type_: &'a str,
cmd: Vec<String>,
}

fn trigger_command(trigger_type: &str) -> TriggerCommand {
TriggerCommand {
type_: trigger_type,
cmd: vec!["trigger".to_owned(), trigger_type.to_owned()],
}
}

fn trigger_commands_for_trigger_types(trigger_types: Vec<&str>) -> Result<Vec<Vec<String>>> {
fn trigger_commands_for_trigger_types(trigger_types: Vec<&str>) -> Result<Vec<TriggerCommand>> {
trigger_types
.iter()
.map(|&t| match t {
"http" | "redis" => Ok(trigger_command(t)),
_ => {
let cmd = resolve_trigger_plugin(t)?;
Ok(vec![cmd])
Ok(TriggerCommand {
type_: t,
cmd: vec![cmd],
})
}
})
.collect()
Expand Down
Loading