-
Notifications
You must be signed in to change notification settings - Fork 296
feat: Implement environment variable isolation for components #3401
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
base: main
Are you sure you want to change the base?
Changes from all commits
02d759c
70191b6
1a56ba4
3f77394
ab68f65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,6 +213,12 @@ impl<'a, L: ComponentSourceLoader> Composer<'a, L> { | |
|
|
||
| let prepared = self.prepare_dependencies(world_id, component).await?; | ||
|
|
||
| // Apply env isolation when there is at least one dependency. | ||
| if self.should_apply_env_isolation(&prepared) { | ||
| self.apply_env_isolation(component.id(), instantiation_id, world_id, &prepared) | ||
| .map_err(ComposeError::PrepareError)?; | ||
| } | ||
|
|
||
| let arguments = self | ||
| .build_instantiation_arguments(world_id, prepared) | ||
| .await?; | ||
|
|
@@ -466,6 +472,115 @@ impl<'a, L: ComponentSourceLoader> Composer<'a, L> { | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Detect the WASI CLI environment version from a registered component's imports. | ||
| fn detect_wasi_env_version(&self, world_id: WorldId) -> Option<String> { | ||
| self.graph.types()[world_id] | ||
| .imports | ||
| .keys() | ||
| .find_map(|name| name.strip_prefix("wasi:cli/environment@").map(String::from)) | ||
| } | ||
|
|
||
| /// Returns true when environment isolation should be applied. | ||
| /// | ||
| /// Environment isolation is applied if there is at least one dependency. | ||
| // Note: In principle this is overly conservative since we could check | ||
| // whether the main component or any dependency imports `wasi:cli/environment`, | ||
| // but in practice almost all components will import `wasi:cli/environment`, | ||
| // so we go with this simpler heuristic for now. | ||
| fn should_apply_env_isolation(&self, prepared: &IndexMap<String, DependencyInfo>) -> bool { | ||
| prepared.values().len() > 0 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason for preferring this over |
||
| } | ||
|
|
||
| /// Apply environment variable isolation. | ||
| /// | ||
| /// Generates an isolator component shared across all targets, plus a small | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does "target" mean "dependency" here? (Edit: I think is means "component" in the sense of any component, main or dependency.) |
||
| /// wrapper component per target that bundles flat functions back into a | ||
| /// `wasi:cli/environment` instance. | ||
| fn apply_env_isolation( | ||
| &mut self, | ||
| component_id: &str, | ||
| main_instance: NodeId, | ||
| main_world: WorldId, | ||
| prepared: &IndexMap<String, DependencyInfo>, | ||
| ) -> anyhow::Result<()> { | ||
| use spin_env_isolator::isolator::{generate_isolator, IsolationTarget}; | ||
| use spin_env_isolator::wrapper::build_env_wrapper_component; | ||
|
|
||
| // Collect isolation targets (components that import wasi:cli/environment). | ||
| // Each target tracks the specific wasi:cli/environment version it imports, | ||
| // since different components may import different versions. | ||
| let mut targets = Vec::new(); | ||
| let mut target_instances: Vec<(String, NodeId, String)> = Vec::new(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might merit being a struct with field names, in particular because I for one will mix up the order of the two strings! |
||
|
|
||
| // Track the highest WASI env version seen; used for the isolator's import. | ||
| let mut max_wasi_env_version: Option<String> = None; | ||
|
|
||
| // Main component | ||
| if let Some(version) = self.detect_wasi_env_version(main_world) { | ||
| let prefix = spin_env_isolator::compute_prefix(component_id); | ||
| targets.push(IsolationTarget { | ||
| name: component_id.to_string(), | ||
| prefix, | ||
| }); | ||
| target_instances.push((component_id.to_string(), main_instance, version.clone())); | ||
| max_wasi_env_version = Some(version); | ||
| } | ||
|
|
||
| // Dependencies importing wasi:cli/environment (deduplicated by manifest name) | ||
| let mut seen_deps = std::collections::HashSet::new(); | ||
| for (_, dep_info) in prepared { | ||
| if seen_deps.insert(&dep_info.manifest_name) { | ||
| let dep_name = dep_info.manifest_name.to_string(); | ||
| if let Some(version) = self.detect_wasi_env_version(dep_info.world_id) { | ||
| let prefix = spin_env_isolator::compute_prefix(&dep_name); | ||
| targets.push(IsolationTarget { | ||
| name: dep_name.clone(), | ||
| prefix, | ||
| }); | ||
| target_instances.push((dep_name, dep_info.instantiation_id, version.clone())); | ||
| if is_higher_version(&version, max_wasi_env_version.as_deref()) { | ||
| max_wasi_env_version = Some(version); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if targets.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| let imported_wasi_env_version = max_wasi_env_version.unwrap(); | ||
|
|
||
| let isolator_bytes = generate_isolator(&targets, &imported_wasi_env_version)?; | ||
| let (_, isolator_instance) = self.register_package("env-isolator", None, isolator_bytes)?; | ||
|
|
||
| // For each target, create a wrapper and wire isolator → wrapper → target. | ||
| // Each wrapper uses the target's own wasi:cli/environment version. | ||
| for (target_name, target_instance, target_wasi_env_version) in &target_instances { | ||
| let wrapper_bytes = build_env_wrapper_component(target_name, target_wasi_env_version)?; | ||
| let wrapper_pkg_name = format!("env-wrapper-{target_name}"); | ||
| let (_, wrapper_instance) = | ||
| self.register_package(&wrapper_pkg_name, None, wrapper_bytes)?; | ||
|
|
||
| // Wire isolator → wrapper (filtered get-environment) | ||
| let export_name = format!("environment-{target_name}-get-environment"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two sources of truth for this format string? (cf isolator line 180) |
||
| let node = self | ||
| .graph | ||
| .alias_instance_export(isolator_instance, &export_name)?; | ||
| self.graph | ||
| .set_instantiation_argument(wrapper_instance, &export_name, node)?; | ||
|
|
||
| // Wire wrapper → target (wasi:cli/environment instance) | ||
| let env_import = format!("wasi:cli/environment@{target_wasi_env_version}"); | ||
| let node = self | ||
| .graph | ||
| .alias_instance_export(wrapper_instance, &env_import)?; | ||
| self.graph | ||
| .set_instantiation_argument(*target_instance, &env_import, node)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
|
|
@@ -482,6 +597,18 @@ struct DependencyInfo { | |
| export_name: Option<String>, | ||
| } | ||
|
|
||
| /// Returns true if `candidate` is a higher semver version than `current` | ||
| /// (or if `current` is `None`). | ||
| fn is_higher_version(candidate: &str, current: Option<&str>) -> bool { | ||
| let Some(current) = current else { | ||
| return true; | ||
| }; | ||
| match (Version::parse(candidate), Version::parse(current)) { | ||
| (Ok(c), Ok(cur)) => c > cur, | ||
| _ => candidate > current, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lexical comparison? I'm a bit doubtful about that. I'd feel safer with something like "bad candidate => false, bad current => true" maybe? |
||
| } | ||
| } | ||
|
|
||
| fn apply_deny_all_adapter( | ||
| dependency_name: &str, | ||
| dependency_source: &[u8], | ||
|
|
@@ -639,4 +766,19 @@ mod test { | |
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_higher_version() { | ||
| // Basic comparisons | ||
| assert!(is_higher_version("0.2.10", Some("0.2.9"))); | ||
| assert!(!is_higher_version("0.2.9", Some("0.2.10"))); | ||
| assert!(!is_higher_version("0.2.9", Some("0.2.9"))); | ||
|
|
||
| // Major/minor differences | ||
| assert!(is_higher_version("1.0.0", Some("0.9.9"))); | ||
| assert!(is_higher_version("0.3.0", Some("0.2.99"))); | ||
|
|
||
| // None means no current version, so any candidate is higher | ||
| assert!(is_higher_version("0.2.6", None)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| [package] | ||
| name = "spin-env-isolator" | ||
| version.workspace = true | ||
| authors.workspace = true | ||
| edition.workspace = true | ||
| license.workspace = true | ||
| homepage.workspace = true | ||
| repository.workspace = true | ||
| rust-version.workspace = true | ||
|
|
||
| [dependencies] | ||
| anyhow = { workspace = true } | ||
| wasm-encoder = { workspace = true, features = ["component-model"] } | ||
| wasmparser = { workspace = true, features = ["validate", "component-model"] } | ||
|
|
||
| [dev-dependencies] | ||
| wasmtime = { workspace = true } | ||
|
|
||
| [lints] | ||
| workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I am misunderstanding this or if I have missed the relevant change elsewhere, but this seems to happen after the deny-all adapter has been applied (in
prepare_dependencies). So my understanding would be that when the dep calledget-environment, that would be denied by the adapter and never reach the isolator. I know you've tested this and it works, so this is probably more for my education, but what am I missing? (Sorry if this is a stupid question.)