Add SIP for environment variable isolation#3400
Add SIP for environment variable isolation#3400tschneidereit wants to merge 1 commit intospinframework:mainfrom
Conversation
This implements the SIP introduced in spinframework#3400. Note: I made extensive use of GitHub Copilot in implementing this, but made sure that I fully understand the entire implementation. Additionally, I ensured that everything is thoroughly documented and tested, since some of the implementation is very subtle and low-level. In particular, the isolation itself is implemented by synthesizing a new component containing two, also synthesized, core modules, plus a wrapper component for each target component. This code makes substantial use of the `wasm-encoder` crate to generate the necessary WebAssembly components and modules, and the `wasmparser` crate to validate the generated components. By comparison, the integration into Spin is delightfully simple: manifest loading and composition are the only two pieces that needed updating, with everything else not needing to have any awareness of the isolation at all.
This implements the SIP introduced in spinframework#3400. Note: I made extensive use of GitHub Copilot in implementing this, but made sure that I fully understand the entire implementation. Additionally, I ensured that everything is thoroughly documented and tested, since some of the implementation is very subtle and low-level. In particular, the isolation itself is implemented by synthesizing a new component containing two, also synthesized, core modules, plus a wrapper component for each target component. This code makes substantial use of the `wasm-encoder` crate to generate the necessary WebAssembly components and modules, and the `wasmparser` crate to validate the generated components. By comparison, the integration into Spin is delightfully simple: manifest loading and composition are the only two pieces that needed updating, with everything else not needing to have any awareness of the isolation at all. Signed-off-by: Till Schneidereit <till@tillschneidereit.net>
This SIP proposes a new component isolation mechanism for environment variables, allowing components to have their own isolated view of the environment. This is achieved by synthesizing an isolator component applying filtering to expose only environment variables with a specified prefix to each component in a composition. Signed-off-by: Till Schneidereit <till@tillschneidereit.net>
tschneidereit
force-pushed
the
env-isolator-sip
branch
from
4a9842b to
15856ad
Compare
|
Oh, one very important thing I forgot to mention: I'm not entirely sure myself whether this should be accepted as-is. Perhaps the better thing to do would be to address component isolation more comprehensively: instead of doing env vars now, and something else later, we'd make one big change applying isolation to everything where it makes sense. That'd include things like Spin's own The downside is that that is quite substantially more work, and requires a lot more design decisions than are needed just for env vars. I think a lot of the isolation approach here would still apply, and many considerations would stay the same. But certainly not all would, and we'd all but certainly need to make pretty intrusive changes to interfaces implemented in factors, as opposed to just having to change manifest loading and component composition. |
itowlson
left a comment
itowlson
left a comment
There was a problem hiding this comment.
Looks good - thanks for noodling through all the details on this! I had a few thoughts, mostly about UI, compatibility (which turned out to be covered later!), and a couple of things that I was unclear on. But generally yep this looks splendid!
| |---|---| | ||
| | `main` | `MAIN_` | | ||
| | `my-service` | `MY_SERVICE_` | | ||
| | `acme:lib/helper` | `HELPER_` | |
There was a problem hiding this comment.
HELPER_ or ACME_LIB_HELPER_?
Also I believe we allow "import all" deps so the dependency could be acme:lib (check with Brian though).
There was a problem hiding this comment.
^ is correct: acme:lib is an acceptable form
| HELPER_HELPER_SETTING=42 | ||
| ``` | ||
|
|
||
| **NOTE**: Dynamically provided environment variables, e.g. those added using `spin up -e SOME=var` aren't automatically prefixed, and as a result not visible to any component, unless they already have the right prefix. E.g. to expose `SOME` to the `main` component, it has to be changed to `MAIN_SOME`. |
There was a problem hiding this comment.
This seems like a breaking change.
The current behaviour is that spin up -e SOME=var makes SOME appear in all components of the application. The proposed new behaviour would require separate THING_SOME for each component.
We should consider instead having Spin auto-munge -e variables so that they are applied to all components.
(If we do intentionally want to make -e selective - which should probably be a separate discussion - I'd look to decouple the external representation from the internal one. At the moment, we can change the internal representation as we see fit: I'd be very cautious about making it contractual.)
There was a problem hiding this comment.
Another potential woe:
Imagine two components, alice and bob. Both import carol.
With the current scheme, the syntax for setting environment variables on carol would be spin up -e CAROL_WHATEVER=thingy.
But there are two carols in play. We don't want to couple them together. So we need further disambiguation at the CLI. (Of course this is not an issue for the internal representation, as it's scoped to a particular composition).
|
|
||
| The result is that each component sees only its own environment variables, with unprefixed keys, as if those were the only variables that existed. | ||
|
|
||
| The isolator also passes through `get-arguments` and `initial-cwd` without modification, since these are process-level values that do not benefit from per-component filtering. |
There was a problem hiding this comment.
My first reaction is "why am I letting Semi-Trusted Steve scope out MY argument list" but I may be misunderstanding - is the idea that I can still block access via the normal "no inherit config" mechanism, but if I grant access then there is no point customising? If so then no worries.
| HELPER_HELPER_SETTING=42 | ||
| ``` | ||
|
|
||
| **NOTE**: Dynamically provided environment variables, e.g. those added using `spin up -e SOME=var` aren't automatically prefixed, and as a result not visible to any component, unless they already have the right prefix. E.g. to expose `SOME` to the `main` component, it has to be changed to `MAIN_SOME`. |
There was a problem hiding this comment.
As the isolator is only in play for components with at least one dependency, this suggests that SOME=var will work for components without dependencies? I'm not enthusiastic about having to guess which components have dependencies and customise syntax for them alone.
|
|
||
| ### Activation rule | ||
|
|
||
| Environment variable isolation is activated **automatically** for components with at least one dependency. When active, **all** components in the composition are isolated — including the main component. A component with no declared `environment` will see an empty environment when isolation is active. |
There was a problem hiding this comment.
A component with no declared
environmentwill see an empty environment when isolation is active.
This is the same as the current behaviour right? I am not quite sure what you are seeking to highlight here.
|
|
||
| ### Prefix is not user-configurable | ||
|
|
||
| The prefix is derived deterministically from the component or dependency name. Exposing the prefix to users would add configuration surface for an implementation detail that has no user-visible effect (components never see the prefix). Keeping it internal simplifies the UX and prevents misconfiguration. |
There was a problem hiding this comment.
I agree it should be user-configurable but the stuff about spin up -e gives me the impression that it does have a user-visible effect. (But as mentioned above I think we should avoid it having a user-visible effect.)
|
|
||
| ### Automatic activation | ||
|
|
||
| Environment isolation is activated implicitly when any component in a composition imports `wasi:cli/environment`, without an opt-out. This choice follows from the goal to make safe behavior the default or even enforced, but could be weakened by providing an opt-out. |
There was a problem hiding this comment.
Yeah I think care is needed here. Currently dependencies_inherit_configuration = true gives a dep access to all the composition's EVs. It's not clear to me what the semantics are after this change.
The combinations (and my guess at behaviour) are:
- "inherit = false" and no
envsection(s) - no EVs for dep (isolator in play but blocking everything) - "inherit = false" and yes
envsection(s) - only specified EVs, isolator in play - "inherit = true" and no
envsecition - ??? all inherited EVs as current ??? - "inherit = true" and yes
envsecition - ??? all inherited EVs and all dep EVs??? ???
But I am not quite sure what you envision for scenarios 3 and 4. If we change scenario 3 then it's breaking (which I'm not opposed to, we just need to plan and communicate).
|
|
||
| ### Backward compatibility | ||
|
|
||
| There are two compatibility-breaking aspects to this change: |
There was a problem hiding this comment.
Oops, should have read ahead before leaving those comments!
|
|
||
| There are two compatibility-breaking aspects to this change: | ||
| 1. The main component's `environment` isn't visible to dependencies anymore, so existing applications that assume it is won't work as expected. | ||
| 2. Environment variables provided dynamically, e.g. using the `spin up -e` need to be prefixed to be exposed to specific components. |
There was a problem hiding this comment.
As noted above I do not love this one especially if the way we do it couples the internal representation to the UI.
|
|
||
| ## Future possibilities | ||
|
|
||
| - **Template expressions in environment.** The `environment` field currently accepts only static strings. A future enhancement could allow Spin template expressions (e.g. `{{ my_variable }}`), leveraging the existing variables/expressions system for dynamic configuration. |
There was a problem hiding this comment.
This feature is completely independent of isolation though isn't it?
3 participants
This SIP proposes a new component isolation mechanism for environment variables, allowing components to have their own isolated view of the environment. This is achieved by synthesizing an isolator component applying filtering to expose only environment variables with a specified prefix to each component in a composition.
Rendered