Implement wasi-config #2869

itowlson · 2024-09-27T03:14:37Z

This has the same considerations as our wasi-observe and wasi-keyvalue implementations in terms of versioning the Spin worlds - I mashed it into the existing Spin 2 worlds for development and testing but know that is not sustainable. (At this point, I'm assuming all this lot will be punted into a new Spin 3 world.)

I'm publishing this mostly to:

see if it works
give us the chance to look at implementation considerations and decide if we need to offer feedback to upstream proposal

(Regarding 2, it felt like we have some errors that don't map nicely to the proposal. I ended up using the io error variant for anything that wasn't a provider error, which feels potentially misleading. @kate-goldenring I'd value your thoughts on this!)

rylev · 2024-09-27T08:49:10Z

crates/factor-variables/src/lib.rs

@@ -31,6 +31,7 @@ impl Factor for VariablesFactor {
    fn init<T: Send + 'static>(&mut self, mut ctx: InitContext<T, Self>) -> anyhow::Result<()> {
        ctx.link_bindings(spin_world::v1::config::add_to_linker)?;
        ctx.link_bindings(spin_world::v2::variables::add_to_linker)?;
+        ctx.link_bindings(spin_world::wasi::config::store::add_to_linker)?;


I think we'll want to consider whether we want to make this configurable for users of the factor - i.e., do we want to force all runtimes that use the VariablesFactor to link all three interfaces? We could instead make this linking conditional based on some configuration provided when the factor is constructed.

Crate feature flag?

Seems like the feature is light-weight enough that we could just do a dynamic check instead of a compile time feature flag, but I don't feel strongly either way.

rylev · 2024-09-27T08:51:04Z

tests/test-components/components/wasi-config/src/lib.rs

+        ];
+        ensure_matches!(get_all(), Ok(val) if val == expected_all);
+
+        ensure_matches!(get("invalid-name"), Err(Error::Io(_)));


Does wasi-config actually say anything about how keys are supposed to be formed? Is it possible that some code that targets wasi-config and runs properly on other runtimes would error on Spin?

No it doesn't: https://github.com/WebAssembly/wasi-runtime-config/blob/f4d699bc6dd77adad99fa1a2246d482225ec6485/wit/store.wit#L17

This is a good call out of how it should probably be opinionated about string fmt or support invalid-name error

rylev · 2024-09-27T08:51:57Z

tests/test-components/components/wasi-config/README.md

@@ -0,0 +1,8 @@
+# Variables


We should consider adding a conformance test for this. The runtime tests here are meant to be Spin CLI specific but this test seems like it should be applied to all Spin compliant runtimes.

rylev · 2024-09-27T09:02:15Z

crates/expressions/src/lib.rs

@@ -52,6 +52,21 @@ impl ProviderResolver {
        self.resolve_template(template).await
    }

+    pub async fn resolve_all(&self, component_id: &str) -> Result<Vec<(String, String)>> {


Nit: doc comment.

rylev · 2024-09-27T09:02:49Z

crates/expressions/src/lib.rs

+                .map(|r| r.map(|value| (key.to_string(), value)))
+        });
+
+        futures::future::try_join_all(resolve_futs).await


I am slightly worried about performance - at some point we might want to see if adding some sort of caching mechanism to providers is worth it.

We should probably push this down into the provider as e.g. (untested):

trait Provider { async fn get(&self, key: &Key) -> anyhow::Result<Option<String>>; async fn get_many(&self, keys: impl IntoIterator<Item = &Key>) -> anyhow::Result<impl IntoIterator<Option<String>>> { try_join_all(keys.into_iter().map(|key| self.get(key)).await } }

rylev · 2024-09-27T09:04:02Z

crates/factor-variables/src/host.rs

+        match <Self as variables::Host>::get(self, key).await {
+            Ok(value) => Ok(Some(value)),
+            // Err(e) if matches!(e, variables::Error::Undefined(_)) => Ok(Ok(None)),
+            Err(e) => {


FWIW you could:

Err(variables::Error::Undefined(_)) => Ok(None)

kate-goldenring · 2024-09-30T16:46:38Z

crates/factor-variables/src/host.rs

+                variables::Error::Undefined(msg) => spin_world::wasi::config::store::Error::Io(msg), // TODO: this should presumably not occur here
+                variables::Error::InvalidName(msg) => {
+                    spin_world::wasi::config::store::Error::Io(msg)
+                } // TODO: this doesn't feel ideal, but wasi-config only allows two error cases


Regarding 2, it felt like we have some errors that don't map nicely to the proposal. I ended up using the io error variant for anything that wasn't a provider error, which feels potentially misleading.

@itowlson referencing your PR comment and this comment inline, I talked to @thomastaylor312 and InvalidName seems like a good example of an error message we'd want to update WASI config to use as it is a fairly universal case

kate-goldenring · 2024-09-30T16:52:30Z

crates/factor-variables/src/host.rs

+                variables::Error::Provider(msg) => {
+                    spin_world::wasi::config::store::Error::Upstream(msg)
+                }
+                variables::Error::Other(msg) => spin_world::wasi::config::store::Error::Io(msg), // TODO: again, hard to know how to map this (and the original expressions::Error is not more helpful)


WASI config does seem to define IO errors as everything else. Is the concern you are raising is that we are labeling this "other" error as related to IO when it might not be?

@kate-goldenring Yes, that's exactly my worry. I appreciate that ""other" is a kind of unhelpful bucket though...!

kate-goldenring · 2024-09-30T17:00:24Z

tests/test-components/components/wasi-config/src/lib.rs

+        ];
+        ensure_matches!(get_all(), Ok(val) if val == expected_all);
+
+        ensure_matches!(get("invalid-name"), Err(Error::Io(_)));


No it doesn't: https://github.com/WebAssembly/wasi-runtime-config/blob/f4d699bc6dd77adad99fa1a2246d482225ec6485/wit/store.wit#L17

This is a good call out of how it should probably be opinionated about string fmt or support invalid-name error

itowlson · 2024-09-30T18:50:05Z

@kate-goldenring re #2869 (comment) can you clarify "no it doesn't" please? I didn't follow how the linked WIT comment applied - are you thinking of this as "we can't possibly find a key for an invalid name so should return Ok(None)" kind of thing?

kate-goldenring · 2024-09-30T22:08:22Z

@kate-goldenring re #2869 (comment) can you clarify "no it doesn't" please? I didn't follow how the linked WIT comment applied - are you thinking of this as "we can't possibly find a key for an invalid name so should return Ok(None)" kind of thing?

@itowlson I was aiming to point out that this is a weak spot of the WASI config interface and that we should propose updating it to either (A) be opinionated about string formatting or (B) support and invalid key error. My preference is that latter; however, the former would help achieve @rylev's point of enabling more portability across host implementatations.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson requested a review from kate-goldenring September 27, 2024 03:14

rylev reviewed Sep 27, 2024

View reviewed changes

kate-goldenring reviewed Sep 30, 2024

View reviewed changes

itowlson force-pushed the wasi-config branch from 33553af to 11848df Compare October 1, 2024 02:24

Implement wasi-config

b20d169

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson force-pushed the wasi-config branch from 11848df to b20d169 Compare October 7, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement wasi-config #2869

Implement wasi-config #2869

itowlson commented Sep 27, 2024

rylev Sep 27, 2024

lann Sep 27, 2024

rylev Oct 7, 2024

rylev Sep 27, 2024

kate-goldenring Sep 30, 2024

rylev Sep 27, 2024

rylev Sep 27, 2024

rylev Sep 27, 2024

lann Sep 27, 2024

rylev Sep 27, 2024

kate-goldenring Sep 30, 2024

kate-goldenring Sep 30, 2024

itowlson Sep 30, 2024

kate-goldenring Sep 30, 2024

itowlson commented Sep 30, 2024

kate-goldenring commented Sep 30, 2024

Implement wasi-config #2869

Are you sure you want to change the base?

Implement wasi-config #2869

Conversation

itowlson commented Sep 27, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itowlson commented Sep 30, 2024

kate-goldenring commented Sep 30, 2024