Skip to content
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

fix: move component filtering methods to crates #2892

Merged
merged 2 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions Cargo.lock

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

6 changes: 6 additions & 0 deletions crates/app/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ authors = { workspace = true }
edition = { workspace = true }

[dependencies]
anyhow = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
spin-locked-app = { path = "../locked-app" }

[dev-dependencies]
toml = { workspace = true }
spin-factors-test = { path = "../factors-test" }
tokio = { workspace = true }
100 changes: 100 additions & 0 deletions crates/app/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

#![deny(missing_docs)]

use std::collections::HashSet;

use serde::Deserialize;
use serde_json::Value;
use spin_locked_app::MetadataExt;
Expand All @@ -27,6 +29,10 @@ pub const APP_DESCRIPTION_KEY: MetadataKey = MetadataKey::new("description");
/// MetadataKey for extracting the OCI image digest.
pub const OCI_IMAGE_DIGEST_KEY: MetadataKey = MetadataKey::new("oci_image_digest");

/// Validation function type for ensuring that applications meet requirements
/// even with components filtered out.
pub type ValidatorFn = dyn Fn(&App, &[String]) -> 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.

Nit: I typically find using a trait for stuff like this a bit more self documenting . That way you can give the function params names. For example:

trait ApplicationValidator {
  fn validate(&self, app: &App, retained_component_ids: &[String]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great approach. It does make the implementation for the runtime a little clunky though, having to define a trait and then add the validating function to it. I like the simplicity of passing in just a function, though we do lose the better documentation of params


/// An `App` holds loaded configuration for a Spin application.
#[derive(Debug, Clone)]
pub struct App {
Expand Down Expand Up @@ -160,6 +166,49 @@ impl App {
pub fn ensure_needs_only(&self, supported: &[&str]) -> std::result::Result<(), String> {
self.locked.ensure_needs_only(supported)
}

/// Scrubs the locked app to only contain the given list of components
/// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components
fn retain_components(
mut self,
retained_components: &[String],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think it might be nicer if retained_components here (and in other functions) was an impl Iterator<Item = &str> so that we could avoid a whole bunch of to_string() calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to find a way to satisfy lifetimes with this and expressing the function as a type as impl aliases are not allowed in type definitions. But i did change the parameters to take in &[&str] instead.

validators: &[&ValidatorFn],
) -> Result<LockedApp> {
self.validate_retained_components_exist(retained_components)?;
for validator in validators {
validator(&self, retained_components).map_err(Error::ValidationError)?;
}
let (component_ids, trigger_ids): (HashSet<String>, HashSet<String>) = self
.triggers()
.filter_map(|t| match t.component() {
Ok(comp) if retained_components.contains(&comp.id().to_string()) => {
Some((comp.id().to_owned(), t.id().to_owned()))
}
_ => None,
})
.collect();
self.locked
.components
.retain(|c| component_ids.contains(&c.id));
self.locked.triggers.retain(|t| trigger_ids.contains(&t.id));
Ok(self.locked)
}

/// Validates that all components specified to be retained actually exist in the app
fn validate_retained_components_exist(&self, retained_components: &[String]) -> Result<()> {
let app_components = self
.components()
.map(|c| c.id().to_string())
.collect::<HashSet<_>>();
for c in retained_components {
if !app_components.contains(c) {
return Err(Error::ValidationError(anyhow::anyhow!(
"Specified component \"{c}\" not found in application"
)));
}
}
Ok(())
}
}

/// An `AppComponent` holds configuration for a Spin application component.
Expand Down Expand Up @@ -266,3 +315,54 @@ impl<'a> AppTrigger<'a> {
struct CommonTriggerConfig {
component: Option<String>,
}

/// Scrubs the locked app to only contain the given list of components
/// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components
pub fn retain_components(
locked: LockedApp,
components: &[String],
validators: &[&ValidatorFn],
) -> Result<LockedApp> {
App::new("unused", locked).retain_components(components, validators)
}

#[cfg(test)]
mod test {
use spin_factors_test::build_locked_app;

use super::*;

fn does_nothing_validator(_: &App, _: &[String]) -> anyhow::Result<()> {
Ok(())
}

#[tokio::test]
async fn test_retain_components_filtering_for_only_component_works() {
let manifest = toml::toml! {
spin_manifest_version = 2

[application]
name = "test-app"

[[trigger.test-trigger]]
component = "empty"

[component.empty]
source = "does-not-exist.wasm"
};
let mut locked_app = build_locked_app(&manifest).await.unwrap();
locked_app = retain_components(
locked_app,
&["empty".to_string()],
&[&does_nothing_validator],
)
.unwrap();
let components = locked_app
.components
.iter()
.map(|c| c.id.to_string())
.collect::<HashSet<_>>();
assert!(components.contains("empty"));
assert!(components.len() == 1);
}
}
135 changes: 134 additions & 1 deletion crates/factor-outbound-networking/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::ops::Range;

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

const ALLOWED_HOSTS_KEY: MetadataKey<Vec<String>> = MetadataKey::new("allowed_outbound_hosts");
Expand Down Expand Up @@ -34,6 +34,46 @@ pub fn allowed_outbound_hosts(component: &AppComponent) -> anyhow::Result<Vec<St
Ok(allowed_hosts)
}

/// Validates that all service chaining of an app will be satisfied by the
/// supplied subset of components.
///
/// This does a best effort look up of components that are
/// allowed to be accessed through service chaining and will error early if a
/// component is configured to to chain to another component that is not
/// retained. All wildcard service chaining is disallowed and all templated URLs
/// are ignored.
pub fn validate_service_chaining_for_components(
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be desirable to give this a more specific name but the doc comment covers it well and the only names I can think of are a bit verbose (e.g. validate_retained_components_include_service_chaining_dependencies!). Not a blocker, just if inspiration strikes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though this is used by Spin when retaining only a set of components, technically this could be used elsewhere to establish that a set of components have satisfied service chaining. Maybe a runtime implementor would use this for app splitting ... but i like the vague name because technically the functionality could be used in another objective

app: &App,
retained_components: &[String],
) -> anyhow::Result<()> {
app
.triggers().try_for_each(|t| {
let Ok(component) = t.component() else { return Ok(()) };
if retained_components.contains(&component.id().to_string()) {
let allowed_hosts = allowed_outbound_hosts(&component).context("failed to get allowed hosts")?;
for host in allowed_hosts {
// Templated URLs are not yet resolved at this point, so ignore unresolvable URIs
if let Ok(uri) = host.parse::<http::Uri>() {
if let Some(chaining_target) = parse_service_chaining_target(&uri) {
if !retained_components.contains(&chaining_target) {
if chaining_target == "*" {
return Err(anyhow::anyhow!("Selected component '{}' cannot use wildcard service chaining: allowed_outbound_hosts = [\"http://*.spin.internal\"]", component.id()));
}
return Err(anyhow::anyhow!(
"Selected component '{}' cannot use service chaining to unselected component: allowed_outbound_hosts = [\"http://{}.spin.internal\"]",
component.id(), chaining_target
));
}
}
}
}
}
anyhow::Ok(())
})?;

Ok(())
}

/// 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 Expand Up @@ -818,4 +858,97 @@ mod test {
AllowedHostsConfig::parse(&["*://127.0.0.1/24:63551"], &dummy_resolver()).unwrap();
assert!(allowed.allows(&OutboundUrl::parse("tcp://127.0.0.1:63551", "tcp").unwrap()));
}

#[tokio::test]
async fn validate_service_chaining_for_components_fails() {
let manifest = toml::toml! {
spin_manifest_version = 2

[application]
name = "test-app"

[[trigger.test-trigger]]
component = "empty"

[component.empty]
source = "does-not-exist.wasm"
allowed_outbound_hosts = ["http://another.spin.internal"]

[[trigger.another-trigger]]
component = "another"

[component.another]
source = "does-not-exist.wasm"

[[trigger.third-trigger]]
component = "third"

[component.third]
source = "does-not-exist.wasm"
allowed_outbound_hosts = ["http://*.spin.internal"]
};
let locked_app = spin_factors_test::build_locked_app(&manifest)
.await
.expect("could not build locked app");
let app = App::new("unused", locked_app);
let Err(e) = validate_service_chaining_for_components(&app, &["empty".to_string()]) else {
panic!("Expected service chaining to non-retained component error");
};
assert_eq!(
e.to_string(),
"Selected component 'empty' cannot use service chaining to unselected component: allowed_outbound_hosts = [\"http://another.spin.internal\"]"
);
let Err(e) = validate_service_chaining_for_components(
&app,
&["third".to_string(), "another".to_string()],
) else {
panic!("Expected wildcard service chaining error");
};
assert_eq!(
e.to_string(),
"Selected component 'third' cannot use wildcard service chaining: allowed_outbound_hosts = [\"http://*.spin.internal\"]"
);
assert!(validate_service_chaining_for_components(&app, &["another".to_string()]).is_ok());
}

#[tokio::test]
async fn validate_service_chaining_for_components_with_templated_host_passes() {
let manifest = toml::toml! {
spin_manifest_version = 2

[application]
name = "test-app"

[variables]
host = { default = "test" }

[[trigger.test-trigger]]
component = "empty"

[component.empty]
source = "does-not-exist.wasm"

[[trigger.another-trigger]]
component = "another"

[component.another]
source = "does-not-exist.wasm"

[[trigger.third-trigger]]
component = "third"

[component.third]
source = "does-not-exist.wasm"
allowed_outbound_hosts = ["http://{{ host }}.spin.internal"]
};
let locked_app = spin_factors_test::build_locked_app(&manifest)
.await
.expect("could not build locked app");
let app = App::new("unused", locked_app);
assert!(validate_service_chaining_for_components(
&app,
&["empty".to_string(), "third".to_string()]
)
.is_ok());
}
}
3 changes: 2 additions & 1 deletion crates/factor-outbound-networking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ use std::{collections::HashMap, sync::Arc};

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

pub use runtime_config::ComponentTlsConfigs;
Expand Down
14 changes: 9 additions & 5 deletions crates/factors-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,14 @@ impl<T: RuntimeFactors> TestEnvironment<T> {
}

pub async fn build_locked_app(&self) -> anyhow::Result<LockedApp> {
let toml_str = toml::to_string(&self.manifest).context("failed serializing manifest")?;
let dir = tempfile::tempdir().context("failed creating tempdir")?;
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
build_locked_app(&self.manifest).await
}
}

pub async fn build_locked_app(manifest: &toml::Table) -> anyhow::Result<LockedApp> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a TryFrom<toml::Toml> for LockedApp impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont see us using it anywhere outside of tests so it may make sense to avoid moving up to the crate

let toml_str = toml::to_string(manifest).context("failed serializing manifest")?;
let dir = tempfile::tempdir().context("failed creating tempdir")?;
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
}
Loading
Loading