Service chaining

[itowlson]

Extremely WIP and not ready for review! (This initial commit, for example, is just the core dispatch, none of the validation or precautions, and only minimally tested.)

Tasks:

• Testing and (inevitably) fixing
  • Paths ✔️
  • Headers ✔️
  • Streaming bodies ✔️
  • Concurrency ✔️
• host_requirements section and validation
  • spin.toml [N/A]
  • Lockfile ✔️
  • Propagation ✔️
• Strip any Host: *.spin.internal header from routed requests ✔️
• Headers to provide on chained requests
  • Standard Spin request headers ✔️
  • Additional headers for chaining (TBD) [N/A]

[itowlson]

@lann This has a proposal for the lockfile stuff, which declares v0 and v1 serialisation formats plus a new wrapper that handles deserialisation from either format and serialisation to the v0 or v1 format depending on whether v1 features are needed. The wrapper format is no longer Serialize or Deserialize - this was to get around code that performed serialisation or deserialisation directly, which would have bypassed the version understander, although it could be restored as a custom implementation. The PR as a whole isn't ready yet (and, in particular, the "must-understand" stuff isn't processed), but it would be good to know whether this aligns with your thinking and whether it's going to be friendly to Cloud. Thanks!

[lann]

I had been thinking that the version field could just be relaxed to allow 0 or 1 since the format is (syntactically) backward-compatible.

[itowlson]

@lann That would work for deserialising, but would require custom serialisation. Is that your preference?

[lann]

Ugh serde does not make this easy does it?

[lann]

After a closer look it does seem like custom serialization might be the least-bad option here from a maintenance perspective.

[itowlson]

@lann Okay I have been exploring the custom serialisation (in a different branch, which I can share if it would help) and I am measurably dubious because it feels like either:

1. Everywhere that deserialises one of these things has to check it understands the must-understand stuff; or
2. We also have to do custom deserialisation and check the must-understand stuff within that

I don't trust myself or anybody else to do 1 correctly. So this approach seems to leave us with custom deserialisation and serde errors, which feels like brittle work for a worse UX. I am happy to press on with custom serialisation/deserialisation if you feel it's the right thing, but I wanted to touch base and make sure you still felt it was better than the wrapper type.

[lann]

I think deserialization can be handled with a serde enum; something like:

// LockedApp {
    must_understand: Vec<MustUnderstand>,
// }

#[derive(Deserialize)
#[serde(rename_all = "snake_case")]
enum MustUnderstand {
    HostRequirements,
}

It also could make sense to have a custom #[serde(deserialize_with = "deserialize_must_understand")] for nicer error messages at some point.

[itowlson]

Okay this has mostly come together except for streaming. I have a test case where the front end streams its request to the back end, and streams the response from the back. When I route the front-to-back request, it works. When I chain the two, it deadlocks.

I was SO CLOSE.

[itowlson]

Okay, I have two-way streaming working. I understand why my first attempt to test it deadlocked when chained, but I don't understand why it worked when routed - so I'm still concerned, because there is a behavioural difference, and that could trip folks up if they have a working streaming interaction and attempt to chain it. Still, unblocked for now I think.

[lann]

I'd be happy for someone with a better mental model of how wasi http works to review this (chain_request in particular). @dicej?

[lann]

This is an empty struct. Any particular reason to pass it around?

[itowlson]

We could instantiate it within the function I guess, but conceptually what this is saying is "I want to use the exact same engine and executor that the HTTP trigger has set up." That the executor has no state today is an implementation detail that this function shouldn't have to care about.

Really I should probably Arc it rather than cloning it, in case changes are made to the executor after I capture it...

[itowlson]

@lann I'm still not happy with the LockedApp stuff. At the moment I check host requirements during deserialisation and that just doesn't work (because the spin_locked_app crate doesn't know what the caller of the deserialiser actually supports). So I am back to either:

• A wrapper type, such that consumers of LockedApp can only get at it via a function that checks requirements; or
• All consumers making sure to check that they can meet the host requirements after deserialisation.

We've shied away from the first option, which leaves us with the second. (Or I'm missing something. Always plausible.) For triggers I can encapsulate the check in the trigger loader - which is probably the 90% case - but anything else will rely on the programmer knowing they need to do the check. Are we okay with that? Or did you already have another plan in mind for this? grin

[dicej]

I mostly skimmed the loader bits (assuming Lann knows those best) and focused on the trigger-http parts. Looks good to me, and it's actually simpler than I would have expected. Nice work!

[itowlson]

Added a "check that the app doesn't need anything I don't provide" function and implemented it in the base trigger code. Example message: Error: This application requires the following features that are not available in this version of the 'redis' trigger: local_service_chaining

Note that because capabilities are checked at application level, this can misfire in mixed-trigger apps. A mixed app whose HTTP component allows spin.internal but whose Redis component does not will still fail saying the app as a whole needs service chaining. This isn't ideal: we need to decide if the compatibility risk of iterating on it later justifies holding this up.

cc @lann for opinioning

[itowlson]

(The alternative would be for the Redis trigger not to check, which would enable the "HTTP component chains but Redis does not" scenario, but would mean the "Redis component chains" scenario would sneak through and fail when the request was made.
If we think that's better then I will just back out the last commit!)