Conversation
09abd4c to
2f54552
Compare
Signed-off-by: Michelle Dhanani <mdhanani@akamai.com>
2f54552 to
e4799de
Compare
itowlson
left a comment
There was a problem hiding this comment.
Good stuff, thanks for getting this under way!
| | `environments_variables` | Environment variables the component needs. Each entry is a bare name, or `{ name, default }`. | | ||
| | `files` | Files the component may read: a glob, or `{ source, destination }` mount. | | ||
|
|
||
| `[requires]` is descriptive: it documents what an application must provide when it |
There was a problem hiding this comment.
I like this emphasis. The component cannot grant itself the permissions because we can't trust it, but we need to know what it wants from us.
(no action, just wanted to applaud)
|
|
||
| ### Building a component | ||
|
|
||
| `spin build` recognizes a component manifest, runs its `[build].command`, and embeds |
There was a problem hiding this comment.
Recognises it by the manifest version declaration?
There was a problem hiding this comment.
by the file name and the manifest version declaration. I'll make that clearer.
| pre-built). | ||
|
|
||
| Only `spin build` operates on component manifests. `spin up`, `spin deploy`, and | ||
| similar commands continue to require an application manifest, because a lone |
There was a problem hiding this comment.
PEDANTRY CORNER: spin up -f bare.wasm does work. (but your point is still right of course)
| only a `source`. A `watch` field (globs for `spin watch`) is intentionally *not* | ||
| included yet — `spin watch` does not operate on component manifests, so the field | ||
| would have no effect. It will be added together with `spin watch` support for | ||
| component manifests (see [Future work](#future-work)). |
There was a problem hiding this comment.
Correct to say a standalone component cannot have dependencies?
There was a problem hiding this comment.
For now at least. Not forever.
There was a problem hiding this comment.
Noted in the future work section
| ``` | ||
|
|
||
| - The version portion is a semver requirement; when omitted, the latest | ||
| non-yanked release is pulled. |
There was a problem hiding this comment.
I'm getting a bit confused across OCI and wkg at this point, sorry. The draft uses OCI terminology and references in some places, and wkg in others (which is what is mentioned in the preamble).
There was a problem hiding this comment.
updated the wording to make it less confusing
| - The version portion is a semver requirement; when omitted, the latest | ||
| non-yanked release is pulled. | ||
| - `--output` selects where the component Wasm is written; it defaults to | ||
| `<name>.wasm` in the current directory. |
There was a problem hiding this comment.
spin registry pull with applications puts them in the Spin cache I believe.
|
Great stuff @michelleN Although I am on the same page because it would streamline What about keeping everything in
I drafted a bit and ended up with something like this (not really different from the current approach, but adds more granular variable handling). # ...
[variables]
app_name = { default = "My Spin App" }
[[trigger.http]]
route = "/api/..."
component = "api"
[component.api]
source = "api/target/wasm32-wasip2/release/api.wasm"
allowed_outbound_hosts = []
key_value_stores = ["default"]
dependencies = ["cors", "correlation-id", "header-injection"]
[component.cors]
source = { path = "../cors/target/wasm32-wasip2/release/cors.wasm" }
[component.cors.variables]
allowed_methods = "POST,GET,PUT,DELETE"
allowed_origings = "*"
# allowed_headers is unset (component might have smart defaults)
[component.correlation-id]
source = { path = "../cors/target/wasm32-wasip2/release/cid.wasm"}
[component.correlation-id.variables]
header_name = "X-CORRELATION-ID"
[component.header-injection]
source = {path = "./components/hi/target/wasm32-wasip2/release/hi.wasm" }
[component.header-injection.build]
workdir = "./components/hi"
command = "cargo build --target wasm32-wasip2 --release"
[component.header-injection.variables]
header_name = "X-APP-NAME"
header_value = "{{ app_name }}" |
| | `sql_variables` | SQLite database labels the component accesses. | | ||
| | `ai_models` | AI models the component accesses. | | ||
| | `allowed_outbound_hosts` | Outbound network destinations the component is allowed to reach. | | ||
| | `environments_variables` | Environment variables the component needs. Each entry is a bare name, or `{ name, default }`. | |
There was a problem hiding this comment.
| | `environments_variables` | Environment variables the component needs. Each entry is a bare name, or `{ name, default }`. | | |
| | `environment` | Environment variables the component needs. Each entry is a bare name, or `{ name, default }`. | |
| #### `[requires]` — host capabilities (optional) | ||
|
|
||
| Declares the capabilities the component expects the host application to grant it. | ||
| These mirror the capability fields of a `spin.toml` component: |
There was a problem hiding this comment.
I maybe wouldn't say these mirror the capability fields in spin.toml -- it's likely, at least some of these, will diverge from the schema associated with the name used in spin.toml (e.g. environment is divergent).
| | `ai_models` | AI models the component accesses. | | ||
| | `allowed_outbound_hosts` | Outbound network destinations the component is allowed to reach. | | ||
| | `environments_variables` | Environment variables the component needs. Each entry is a bare name, or `{ name, default }`. | | ||
| | `files` | Files the component may read: a glob, or `{ source, destination }` mount. | |
There was a problem hiding this comment.
We should probably not allow glob patterns here; globs are relative to the consuming app's directory and the dependency component has no idea what's there. What the component actually knows is the guest path it expects to read so i'd consider restricting this to { destination }.
There was a problem hiding this comment.
That's an interesting thought @fibonacci1729. I like the idea of being able to say "I expect to find whatever files you want me to use at /foo, but it's up to you where you source them."
There is another use case, but I am not sure about generality: static assets. Here the component / dep / middleware doesn't want to put it on the app to supply files, because it already has specific ones it wants to use. E.g. the HTML, JavaScript and CSS for the "authorisation failed" page, or a CSV file of internal data. However, while this (I think) makes sense for a local component (a TOML file), we can't currently publish it as a standalone Wasm binary.
There was a problem hiding this comment.
I think for static assets that a lib dev would require (where they don't want to put the burden on the app to supply) we could, at least initially, document/encourage them to embed their assets into the binary.
There was a problem hiding this comment.
We would also have the option of embeddeding static assets in their own custom sections (e.g. named spin-file:path/to/file).
|
|
||
| ### Building a component | ||
|
|
||
| `spin build` recognizes a component manifest, runs its `[build].command`, and embeds |
There was a problem hiding this comment.
I think we'll need to enable push to embed metadata since build is optional.
|
@thorstenhans we can't keep everything in the
Strongly agree and that's the argument for a standalone manifest. The moment you "generalize and distribute independently," you need an independent build/version/publish story, which is what |
Co-authored-by: Brian <brian.hardock@fermyon.com>
|
@michelleN What's the relationship between this and #3664? |
Signed-off-by: Michelle Dhanani <mdhanani@akamai.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
Co-authored-by: Brian <brian.hardock@fermyon.com>
Signed-off-by: Michelle Dhanani <mdhanani@akamai.com>
Signed-off-by: Michelle Dhanani <mdhanani@akamai.com>
|
Would appreciate another round of eyes on this @itowlson @fibonacci1729 |
|
@michelleN Are we reviewing the implementation alongside the SIP, or is the implementation WIP that was just more convenient to do on the same branch? |
itowlson
left a comment
There was a problem hiding this comment.
Basically looks good, although I remain a bit muddled about the wkg interop story. But really this will be very cool to have. I really like the requires approach and the way that propagates through the binary!
| embed required configuration in component binary (see [Alternatives considered](#alternatives-considered)), and extend `spin registry` to | ||
| publish and fetch components to and from an OCI registry. | ||
|
|
||
| ### The component manifest (`component.toml`) |
There was a problem hiding this comment.
super mega nit: promote this and other H3s to to H2 (and H4s to H3). These chunks are the top levels of the SIP, not mere subsections of Proposal.
| version = "0.1.0" | ||
| description = "HTTP middleware that gates requests behind GitHub OAuth" | ||
| authors = ["Michelle Dhanani <mdhanani@akamai.com>"] | ||
| repository = "https://github.com/michellen/github-oauth-middleware" |
There was a problem hiding this comment.
Possible thing, or maybe future idea: a site or url or docs field with the semantics of "go here to learn about this component." Not sure
There was a problem hiding this comment.
I would imagine the repository would have the same semantics right?
There was a problem hiding this comment.
Not necessarily: the author might have a docs site or configuration guide. The semantics are certainly different: "go here for source code" vs "go here to learn." (Cf. docs.rs which calls this "homepage" vs "repository.") But yes, the first line of the repo README could point the docs/guide site, which is why it's definitely not a must-have on day one.
| description = "HTTP middleware that gates requests behind GitHub OAuth" | ||
| authors = ["Michelle Dhanani <mdhanani@akamai.com>"] | ||
| repository = "https://github.com/michellen/github-oauth-middleware" | ||
| license = "Apache-2.0" |
There was a problem hiding this comment.
For the source code, the licence is usually a licence file in the repo. For the binary distribution, I'm not sure if it makes sense? (e.g. what does it mean to GPL a binary) (But if this is a known and understood thing with binaries then no worries.)
| ai_models = ["llama2-chat"] | ||
| environment = ["staging", { name = "region", default = "us" }] | ||
| files = [{ destination = "/mounted/path" }] | ||
| ``` |
There was a problem hiding this comment.
Should standalone components support profiles, like apps do? I am not sure whether it makes sense from a distribution POV, but maybe useful for "I want a debug build" nevertheless?
Maybe it should exist but be more limited than app profiles, e.g. only the build-command and source fields?
| | `version` | for publishing | Semver version. Used as the version when publishing. | | ||
| | `description`, `authors`, `repository`, `license` | no | Human-readable metadata. | | ||
|
|
||
| `version` is only required when the component is published; a component that is only built locally may omit them. |
There was a problem hiding this comment.
Is the idea here for if I am building a component only for inclusion in an app manifest (and that will never have any independent existence)? I'm tempted to make version mandatory so that when someone comes to publish they don't get a nasty surprise, but I can see this use case.
(ETA: your note on watch fields suggests "for inclusion in an app manifest" is not an intended use case in this iteration)
| ### Building a component | ||
|
|
||
| `spin build` recognizes a component manifest by file name and manifest version declaration, runs its `[build].command`, and embeds | ||
| the component manifest (omitted the `[build]` section) as JSON in a custom section of the built binary: |
There was a problem hiding this comment.
You talked earlier about republishing components from a registry etc. In such a case the component won't have a build command. So it seems like spin build needs to work in the absence of a build command, just to write the custom section?
| - `--build` performs a default `spin build` (component-aware) before publishing. | ||
| - `spin registry push` detects a component manifest and takes the component path; | ||
| an application manifest continues to be pushed as a Spin application OCI | ||
| artifact (with its registry reference argument), unchanged. |
There was a problem hiding this comment.
So a component published this way will not be usable via wkg, am I reading that right? We are defining separate conventions outside the standards? If so, I am wary.
Oh wait I see later on (in the "Alternatives Considered" section) you say that this is wkg. I am confused because I thought wkg used the package name rather than requiring a separate OCI reference. Perhaps worth expanding the SIP to talk more about how these components work with wkg?
|
I've only looked at the SIP, not any of the implementation, but the SIP looks great! It largely subsumes #3073, too, which I'm very pleased by :) I also really like the I think the feedback that follows is pretty much entirely about making this SIP fully subsume #3073, because I don't think we'll want multiple separate approaches to modularizing manifests. Same as individual crates from a cargo workspace can be published without publishing the entire workspace, I think we could, with only small tweaks, make this serve the same purpose as a cargo workspace and support publishing of individual components. As such, I think would be good to put a bit more work into either moving integration with #3073 had some small amount of that in the shape of supporting things like [component.my-packaged-component]
path = "relative/path/to/dir/containing/the/manifest"I also think that we shouldn't postpone reasoning through how My thinking on that is, very roughly, that the Speaking of dependencies, it might also be good to put a bit of thought into nested dependencies before moving that topic to future work. I initially thought "why not just say that it works just as in Which in turn makes me realize that providing default values for One approach could be to not accept defaults ... by default. Instead, a default would have to be accepted explicitly with something like [my-comp.dependencies.my-dep]
path = "my-dep"
requires = {
allowed_outbound_hosts.default = true
}(Yay multi-line nested tables or what those curly brace things are called!) I still don't particularly like this though, because it still means that the default value is invisible, and might be changed behind the scenes. We might be able to handle that on the tooling side, though, by remembering the current default value ... somewhere ... and warning if it changes. Alternatively, there is no # spin build
Error: `my-comp`'s dependency `my-dep` requires `allowed_outbound_hosts` to be configured,
but no configuration is provided.
Accept the default value of `["https://github.com", "https://api.github.com"]`?
[y/n]Hitting Problem with that approach: allowed outbound hosts are something the component typically really needs, so if the default changes, chances are the setting in So yeah, this would be good to think through a bit more 😬 |
No description provided.