Spin Improvement Proposal: Modularizing spin.toml #3073
Conversation
This PR adds a SIP describing changes that'd allow developers to split the `spin.toml` manifest up into a main application manifest and sub-manifests for individual contained components. Signed-off-by: Till Schneidereit <[email protected]>
tschneidereit
force-pushed
the
sip-modular-manifests
branch
from
0bbb647 to
3d0edb8
Compare
There was a problem hiding this comment.
My sense here is of a slightly uneasy tension between "I'm just providing a handy-dandy way to split an app across files" vs "I'm providing a way to define rich components in isolation from an app." There are a few things in the proposal that say "rich component in isolation" but there are others that say "just splitting an app." The reason this matters is that isolated components imply different sandboxing behaviours from split apps - e.g. should a sub-manifest have access to application variables.
It might also be good to reorganise a bit so that the motivation for some of the design decisions is more visible from where the design decisions are listed.
| - Triggers must be defined in the top-level manifest | ||
| - Variables can be contained in all manifests | ||
| - A variable defined in the top-level manifest is visible in all manifests | ||
| - A variable defined in a sub-manifest is only visible in that manifest |
There was a problem hiding this comment.
It would be good to elaborate on this scoping rule, as it requires Spin to track visibility in a way we currently don't. At the moment, spin up assembles everything into a single lockfile, which is passed to the trigger: components' template values are resolved when the trigger loads the lockfile. Enforcing visibility rules seems like it would mean either multiple lockfiles or tracking provenance in the lockfile. Maybe we can get around it by munging names, but then we need to map them back again to get values from providers.
Another facet of this (and, I'd guess, part of the motivation) is what happens if two sub-manifests define the same variable name. The proposed scoping rule appears to allow this, which is good because it means I can pull in off-the-shelf sub-manifests and not worry about them clashing. But again we need to better understand how we envisage representing that name in the lockfile.
In theory, I gues we could unify variables with the same name, since they will end up backing onto the same provider-level value (e.g. environment variable or Vault key). But in practice different defaults will make this tricky.
There was a problem hiding this comment.
What is our collective appetite 🌯 for revving the locked app format to codify these visibility rules ? In any case, I agree that this rule needs to be ironed out a bit.
There was a problem hiding this comment.
I think my vote would be to drop these scoping rules for now and require variables be defined in the root manifest.
There was a problem hiding this comment.
@fibonacci1729 I think @tschneidereit is working on something that elaborates on his long-term "north star" thinking, which I believe is what is motivating these rules. I feel we should hold off until we see that and can decide whether to aim for it directly (at the cost of possibly revving the lockfile) or to do something expedient to solve the immediate problem (at the expense of possible rework and possible compatibility quirks when we do move towards the aspiration).
Specifically I get hints from this document of a vision of sub-manifested components as units of packaging, so that this becomes more than spicy lexical inclusion. And if so (big if!), there is some conceptual stuff we need to understand about sandboxing and permissions and scoping (e.g. can a component sniff values from the top level without an explicit grant, how do we namespace stuff so components don't need to coordinate on naming, etc.). I may be over-inferring though!
| - A variable defined in the top-level manifest is visible in all manifests | ||
| - A variable defined in a sub-manifest is only visible in that manifest | ||
| - Components can be defined in the top-level, and in sub-manifests | ||
| - Only a single component can be defined in each sub-manifest |
There was a problem hiding this comment.
It would be useful to say more about the reason for this restriction.
(ETA: you do, but much later - consider a forward link.)
|
|
||
| - Manifests can only be nested one level deep: an application can have a single top-level manifest, and zero or more direct sub-manifests, which cannot have any sub-manifests themselves | ||
| - Application-level metadata must fully be contained in the application’s top-level manifest | ||
| - Triggers must be defined in the top-level manifest |
There was a problem hiding this comment.
It would be useful to say more about the reason for this restriction.
I'm getting the sense that you have a mental model for a concrete thing that a sub-manifest represents, that motivates some of the decisions here? If so it would be useful to talk about it here, even if it is distant and aspirational!
| source = "target/spin-http-js.wasm" | ||
| allowed_outbound_hosts = [] | ||
| exclude_files = ["**/node_modules"] | ||
| key_value_stores = ["default"] |
There was a problem hiding this comment.
Permissions is an interesting one and this is another case where I'd find it useful to hear your long-term vision for this. If this is purely, purely, to help a developer organise a big file, then this is not a worry. But talking about "authors" and "version" fields implies that you envision an ecosystem and distribution mechanism for sub-manifests (dare I call them... components). And in that case it maybe feels like the application should be in charge of granting permissions. I don't want to upgrade "EvilComponent" and discover too late that it's invited itself to my production database...!
|
|
||
| 1. Triggers are application-level concerns; they describe the application’s structure and high-level organization. As such, keeping them “at the surface” seems right. | ||
| 2. Trigger definitions tend to be very short, so moving them into sub-manifests individually as is done for components seems very boilerplate-y. | ||
| 3. Trigger definitions don’t come with their own assets, so any folder containing a sub-manifest for them would only ever contain the manifest, and nothing else. |
There was a problem hiding this comment.
Mostly triggers don't come with component-related config, but we do have an oddity at the moment where WAGI execution has to be specified on the trigger not the component even though it's the component interface that mandates it. (I think this is to allow different trigger to set different argvs on the same component. Not sure though.)
|
|
||
| ### Inheriting definitions from the application manifest | ||
|
|
||
| Similar to [crates in a Cargo workspace](https://doc.rust-lang.org/1.85.1/cargo/reference/workspaces.html#the-package-table), component manifests can inherit certain definitions from the application manifest, such as the version, description, and authors: |
There was a problem hiding this comment.
Perhaps say more about why you feel this would be useful? Cargo cares because crates are units of distribution and deployment.
| ... | ||
| ``` | ||
|
|
||
| Any uses in template strings are adapted to use the prefixed variable names. |
There was a problem hiding this comment.
Is this saying that Spin has to rewrite template strings to the new names as part of lockfile creation?
How does this affect providers? Do secrets in Vault need to use the scoped name?
| ... | ||
| # Inherit the `my_variable` variable from the application | ||
| [variables] | ||
| my_variable.application = true |
There was a problem hiding this comment.
Why? This seems like busywork.
|
|
||
| In component manifests, the role of the latter is taken by the new `[variables.exposed]` table. | ||
|
|
||
| *Note: we should evaluate whether this table is really needed. Can we instead just expose all variables in a component manifest’s `[variables]` table to content directly?* |
There was a problem hiding this comment.
I don't think we can. The term "variable" has two meanings:
- A configuration knob exposed to the operator
- A value exposed to the guest
The decoupling is not just for sandboxing, but so that configuration knobs are not required to line up with what the component sees. E.g. if I publish a component which looks at a "db-url" value, and you publish a component that looks at "db-host" and "db-port" values, the decoupling allows the app developer to define two knobs for the operator, which the application surfaces in different ways to meet different component expectations.
This may have turned out to be generality that we don't need (or won't need until a broader component ecosystem evolves and uses wasi-config), but I'd be cautious about backing it out, and doubly cautious about backing it out for the special case of sub-manifests. It may be that sub-manifest local variables are so tightly internally coupled to their components that it's a reasonable call though - just want to be cautious in the evaluation!
|
I should add that I do like the implications of isolation rather than mere textual splitting. One of the challenges I ran into with the previous attempt at this (which was basically naive inclusion) was providing good locations for errors. Being able to parse and analyse sub-manifests separately, and combine them mostly-reliably only once parsed, should address that nicely. I am very keen that we think about how to give good pointers in failure cases in this work! |
|
Once I switched my app to a microservice-style structure, my spin.toml became bloated and complicated. |
| description = "A sentiment analysis API that demonstrates using LLM inference and | ||
|
|
||
| # Components defined in sub-manifests must be included in this list. | ||
| # Entries are treated as folder names, relative to the folder containing the application manifest. |
There was a problem hiding this comment.
So the directory would be laid out like follows?
├── spin.toml
├── sentiment-analysis
│ └─── spin.toml
└── kv-explorer
└─── spin.tomlIs there a way we could allow for file exact paths, too?
| # Certain keys can be inherited from the application manifest | ||
| authors.application = true | ||
| version.application = true | ||
| source = "target/spin-http-js.wasm" |
There was a problem hiding this comment.
It feels weird to me to have this be relative to the sub manifest. Shouldn't this already be defined in the sub manifest?
| version.application = true | ||
| ``` | ||
|
|
||
| ### Handling of variables |
There was a problem hiding this comment.
I am having a hard time understanding what make variables unique over other resources, such as environment variables, files, and kv stores. Can you explain why variables are component scoped but other resources are not?
|
I'm going to start an in-parallel WIP for this SIP as it currently stands. |
Co-authored-by: Kate Goldenring <[email protected]>
Co-authored-by: Kate Goldenring <[email protected]>
|
|
||
| `spin.toml` files contain multiple different kinds of items: application-level metadata, variables, trigger definitions, and component definitions. I propose to support modularizing manifests in the following way: | ||
|
|
||
| - Manifests can only be nested one level deep: an application can have a single top-level manifest, and zero or more direct sub-manifests, which cannot have any sub-manifests themselves |
There was a problem hiding this comment.
What's the motivation for restricting to 1-level nesting?
This PR adds a SIP describing changes that'd allow developers to split the
spin.tomlmanifest up into a main application manifest and sub-manifests for individual contained components.Signed-off-by: Till Schneidereit [email protected]