Manifest V2 #1780

lann · 2023-09-19T15:01:30Z

"I have only made this PR larger because I have not had the time to make it smaller."

Implements SIP-005 as updated in #1753.

Some highlights:

Completely refactored spin-manifest
- Application et al. are no more; there is only the AppManifest, which closely resembles to the parsed spin.toml
- There is also an AppManifestV1. Backward compat is provided by spin_manifest::compat::v1_to_v2_app.
Mostly-rewritten spin-loader crate takes an AppManifest and converts it into a LockedApp, ready to execute
A new ui-testing crate, supporting expanded "UI" (a.k.a. "golden file") test suites for both spin-manifest and spin-loader

There are some (intentional) changes open for debate:

Validation of component IDs is now more strict, requiring them to be valid component model kebab-case. The v1->v2 conversion should make this mostly transparent (search component_id_from_string if you care), but there may be some IDs floating around that won't parse now.
Rules for files patterns have been relaxed a bit; you can now use the "placement" form for individual files, e.g. { source = "content/some.txt", destination = "/other.txt" }
Added a "triggers" key to locked app metadata which is currently redundant but should help with forward-compatibility for multiple-trigger apps
Probably others that I have already forgotten or haven't yet noticed

Still to do is the spin doctor manifest upgrade feature, though hopefully that is pretty simple application of the v1_to_v2_app code. There are examples of what that should look like in crates/manifest/tests/ui/v1/*.toml.v2

crates/manifest/Cargo.toml

crates/manifest/src/schema/v1.rs

crates/manifest/tests/ui/invalid/bad_component_id.err

crates/oci/src/client.rs

crates/outbound-http/src/allowed_http_hosts.rs

src/commands/new.rs

src/commands/up.rs

itowlson · 2023-10-15T23:00:17Z

src/commands/up.rs

@@ -458,6 +461,17 @@ impl AppSource {
    }
 }

+impl Display for AppSource {


This reads more like a Debug implementation than a Display one.

This is responsible for the updated error message here: https://github.com/fermyon/spin/pull/1780/files#diff-82f776c9616e3a5ecda575278ca8de8a920780afe36b0fff8ab7252292425aad

src/commands/up.rs

vdice

All I can contribute are a couple eagle-eyed comment fixes 😂

Also wondering what our strategy is around updating example and template app manifests to v2? Perhaps as a follow-up PR before the 2.0 release? (I suppose we'll want to coordinate with developer docs updates as well.)

crates/loader/src/http.rs

crates/manifest/src/schema/common.rs

itowlson · 2023-10-17T04:09:54Z

I ~~set out to cause unrest~~ had a play with this as part of trying out deployment, and found a couple of minor oddities. These are with the "overnight" version - I know you are still fixing things so these may already be done in your working copy! - and are nits for the sad path not problems with the "normal" flow.

`type` in app trigger settings?

I wrote:

[application.trigger.http]
bayes = "/"

It correctly erred, but the message was "Error: metadata error: invalid metadata value for "trigger": Error("unknown field bayes, expected type or base", line: 0, column: 0)". Seems like type shouldn't be allowed, but here we go:

[application.trigger.http]
type = "redis"
base = "/"

And it was perfectly happy with that. I wondered if the http part was giving a name to the trigger which was of type redis, but no, the trigger was still type http.

Mismatched application and component triggers

When I wrote redis in my application trigger instead of http:

[application.trigger.redis]
base = "/"

[[trigger.http]]
component = "hello3"
route = "/..."

...it ran without warning.

I am guessing the concept is that all triggers are available via the trigger table array, and that the application.trigger merely applies global settings to those you happen to use, and if you choose to apply global settings that are never used, then what of it? Still it felt a bit surprising, especially with a property that doesn't apply to Redis (since HTTP rejected unknown settings). I'll maybe try to get a better sense of the intended mental model at some point, so we can communicate this correctly.

Mismatched trigger and configuration

I wrote this (to explore a theory about names and types):

[application.trigger.redis]
type = "http"
base = "/"

[[trigger.redis]]
component = "hello3"
route = "/..."

It understandably errored, with the message:

Error: invalid trigger configuration for "redis-1"

Caused by:
   0: json error: unknown field `route`, expected one of `component`, `channel`, `executor`
   1: unknown field `route`, expected one of `component`, `channel`, `executor`

The name "redis-1" does not refer to anything in my TOML file, and it was puzzling to see a "JSON error" when I was working only with TOML.

All of these can absolutely be deferred to issues for after the happy-path functionality has landed - they're probably better than the corresponding manifest v1 errors...!

lann · 2023-10-17T12:43:06Z

Also wondering what our strategy is around updating example and template app manifests to v2? Perhaps as a follow-up PR before the 2.0 release? (I suppose we'll want to coordinate with developer docs updates as well.)

Yeah I want to make sure there aren't any major changes to the implementation first. There will be at least a couple of followups.

lann · 2023-10-17T12:53:40Z

These problems with global trigger config will probably require some refactoring of spin-trigger that I'm hoping to avoid in this already-massive PR.

The name "redis-1" does not refer to anything in my TOML file

Suggestions for improving this? The name is secretly meaningful in that it is assigned to the 1st redis trigger.

vdice · 2023-10-17T14:43:04Z

Suggestions for improving this? The name is secretly meaningful in that it is assigned to the 1st redis trigger.

Would it be possible to point to the line in the manifest/toml causing the error (as follow-up, if even feasible)?

lann · 2023-10-17T15:17:15Z

Would it be possible to point to the line in the manifest/toml causing the error (as follow-up, if even feasible)?

Its tricky currently because the trigger config gets validated by the trigger executor which only gets the locked app JSON. I have a couple of ideas for how to improve the error messages with varying degrees of accuracy and complexity, like injecting parse span information from the original manifest into the locked app metadata. If I can't come up with something that works reasonably well for all trigger types (including custom), I can at least (as a fallback) go back to special-casing validation of http triggers.

crates/http/src/config.rs

rylev

Looks good!! I have some questions but mostly because I'm unfamiliar with a lot of this code.

Cargo.toml

rylev · 2023-10-17T16:02:10Z

crates/build/src/manifest.rs

+    #[serde(default)]
    pub id: String,


I'd don't love the pattern of deserializing with a defaulted, invalid field that gets updated later. I don't understand the code well enough to offer a suggestion and maybe this just the way it has to be, but if there's some way to rearrange it to not have to do this, that would be awesome.

It could be done differently with more code, but this bit is somewhat of hack to begin with, extracting partial data from a otherwise-possibly-invalid manifest.

crates/loader/tests/ui/invalid-digest.err

rylev · 2023-10-17T16:14:27Z

crates/loader/tests/ui/invalid-url-in-allowed-http-hosts.err

+    0: Failed to load component `hello`
+    1: One or more allowed_http_hosts entries was invalid:
+       ftp://random-data-api.fermyon.app isn't a valid host or host:port string
+       example.com/wib/wob contains a path, should be host and optional port only


Does this fail because of the lack of scheme? Perhaps we could make the error message clearer?

🤷 this is unchanged by this PR; I just added the test

@rylev The first one fails because there is a scheme (and it's not a HTTP scheme). The second fails because it contains a path, not just a host. Maybe they look too run-together at the moment which confuses matters? But as Lann says this error message already exists, I can noodle on it separately.

rylev · 2023-10-17T16:28:12Z

crates/loader/src/local.rs

+    }
+
+    // Copy files matching glob pattern or single file/directory path.
+    async fn copy_glob_or_path(


nit: these copy methods might be best in a separate module.

They use self...

rylev · 2023-10-17T16:28:56Z

crates/loader/tests/ui.rs

+use ui_testing::{Failed, Normalizer, UiTestsRunner};
+
+fn main() -> anyhow::Result<()> {
+    // Insert dummy wasm into cache to avoid network traffic


I'm confused by this. Why would the network be hit if we don't add the dummy?

valid-manifest.toml has a remote source which the loader would (try and fail to) download.

rylev · 2023-10-17T16:30:30Z

crates/manifest/src/normalize.rs

+/// Extracts each `ComponentSpec::Inline` into `AppManifest::component`s,
+/// replacing it with a `ComponentSpec::Reference`.
+pub fn normalize_inline_components(manifest: &mut AppManifest) {


Perhaps I'm confused by the name ComponentSpec, but I'm confused by what this function and normalize_trigger_ids actually do a high-level (I get that they set some missing values but why?).

It makes code that consumes manifests simpler by not needing to deal with the flexibility that these normalize.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Move spin-trigger UI tests to spin-loader. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

This collects some serde tools into a separate crate, avoiding some inter-crate dependencies. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

itowlson · 2023-10-17T18:34:43Z

@lann the "redis-1" error happens in this situation:

[[trigger.redis]]
component = "hello3"
route = "/..."

I suspect most users are going to think of that as 'the "hello3" trigger' or 'the "hello3" component'.

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

lann · 2023-10-17T20:07:27Z

@itowlson I added some nicer ID normalization; see normalization.toml (and adjacent normalization.toml.norm).

itowlson · 2023-10-17T20:27:13Z

@lann Thanks. That feels more helpful. I have ideas for further heuristics on identifying unnamed components, but the sort of errors I was raising were the result of malicious tinkering - probably better to wait and see if people ever run into them in the wild, and iterate on them at that point.

lann force-pushed the manifest-v2 branch 5 times, most recently from ebe9287 to 463c86e Compare September 22, 2023 20:15

lann changed the title ~~WIP: manifest V2~~ Manifest V2 Sep 27, 2023

lann force-pushed the manifest-v2 branch 14 times, most recently from 5a72369 to 2fe346d Compare October 13, 2023 20:37

lann marked this pull request as ready for review October 13, 2023 20:37

lann requested review from itowlson, dicej, rylev, michelleN, fibonacci1729, vdice and radu-matei October 13, 2023 20:38

itowlson reviewed Oct 15, 2023

View reviewed changes

lann force-pushed the manifest-v2 branch from 2fe346d to c4a2329 Compare October 16, 2023 15:40

vdice reviewed Oct 16, 2023

View reviewed changes

crates/loader/src/http.rs Outdated Show resolved Hide resolved

crates/manifest/src/schema/common.rs Outdated Show resolved Hide resolved

itowlson mentioned this pull request Oct 17, 2023

Manifest v2 support fermyon/cloud-plugin#132

Merged

lann force-pushed the manifest-v2 branch from c4a2329 to 3528f99 Compare October 17, 2023 13:22

lann force-pushed the manifest-v2 branch from 3528f99 to 80ef422 Compare October 17, 2023 15:24

fibonacci1729 reviewed Oct 17, 2023

View reviewed changes

crates/http/src/config.rs Show resolved Hide resolved

fibonacci1729 approved these changes Oct 17, 2023

View reviewed changes

rylev reviewed Oct 17, 2023

View reviewed changes

lann added 9 commits October 17, 2023 13:04

Add ui-testing crate

f6d50ff

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

manifest: Add v2 manifest format

5e2818b

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

loader: Update to manifest v2

dd58cb9

Move spin-trigger UI tests to spin-loader. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Upgrade everything else to manifest V2

f4dcf5d

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

build: Support V2 manifests

297078b

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Add 'spin-serde' crate

0eafbfc

This collects some serde tools into a separate crate, avoiding some inter-crate dependencies. Signed-off-by: Lann Martin <lann.martin@fermyon.com>

manifest: Remove leftover serialization code from v1 schema

72e2c96

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

manifest: Improve "wrong separator" error messages

15b8bfd

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

Address Manifest V2 review feedback

47fbba0

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

lann force-pushed the manifest-v2 branch from 80ef422 to 47fbba0 Compare October 17, 2023 17:38

manifest: Generate nicer IDs in normalization

426fa9a

Signed-off-by: Lann Martin <lann.martin@fermyon.com>

itowlson approved these changes Oct 17, 2023

View reviewed changes

lann merged commit 57b004c into spinframework:main Oct 17, 2023

Mossaka mentioned this pull request Nov 8, 2023

chore(spin): bump spin to 2.0.0 deislabs/containerd-wasm-shims#177

Merged

lann deleted the manifest-v2 branch July 23, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manifest V2 #1780

Manifest V2 #1780

lann commented Sep 19, 2023 •

edited

Loading

itowlson Oct 15, 2023

lann Oct 16, 2023

vdice left a comment

itowlson commented Oct 17, 2023

lann commented Oct 17, 2023

lann commented Oct 17, 2023

vdice commented Oct 17, 2023

lann commented Oct 17, 2023

rylev left a comment

rylev Oct 17, 2023

lann Oct 17, 2023

rylev Oct 17, 2023

lann Oct 17, 2023

itowlson Oct 17, 2023

rylev Oct 17, 2023

lann Oct 17, 2023

rylev Oct 17, 2023

lann Oct 17, 2023

rylev Oct 17, 2023

lann Oct 17, 2023 •

edited

Loading

itowlson commented Oct 17, 2023

lann commented Oct 17, 2023

itowlson commented Oct 17, 2023

Manifest V2 #1780

Manifest V2 #1780

Conversation

lann commented Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

itowlson commented Oct 17, 2023

type in app trigger settings?

Mismatched application and component triggers

Mismatched trigger and configuration

lann commented Oct 17, 2023

lann commented Oct 17, 2023

vdice commented Oct 17, 2023

lann commented Oct 17, 2023

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lann Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

itowlson commented Oct 17, 2023

lann commented Oct 17, 2023

itowlson commented Oct 17, 2023

lann commented Sep 19, 2023 •

edited

Loading

`type` in app trigger settings?

lann Oct 17, 2023 •

edited

Loading