Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for arbitrary (or at least missing) ecp environment variables #336

Open
katef opened this issue Dec 12, 2023 · 2 comments
Open

Support for arbitrary (or at least missing) ecp environment variables #336

katef opened this issue Dec 12, 2023 · 2 comments
Labels
feature-prod Related to matching the production C@E environment

Comments

@katef
Copy link
Contributor

katef commented Dec 12, 2023

#146 requested exposing the host machine's environment within the wasm runtime, and was rejected because that behaviour would not match the production network. Quoting @cratelyn there:

keeping the environment in which a Wasm program runs in parity with production is an important goal for Viceroy

We're missing a few environment variables that we have in prod on ecp proper:
https://developer.fastly.com/reference/compute/ecp-env/

We could hardcode these to dummy values:

@@ -115,10 +115,19 @@ pub(crate) fn create_store(
 fn make_wasi_ctx(ctx: &ExecuteCtx, session: &Session) -> Result<WasiCtx, anyhow::Error> {
     let mut wasi_ctx = WasiCtxBuilder::new();
 
-    // Viceroy provides a subset of the `FASTLY_*` environment variables that the production
+    // Viceroy provides the same `FASTLY_*` environment variables that the production
     // Compute platform provides:
 
     wasi_ctx
+        // These variables are stubbed out for compatibility
+        .env("FASTLY_CACHE_GENERATION", "0")?
+        .env("FASTLY_CUSTOMER_ID", "0000000000000000000000")?
+        .env("FASTLY_POP", "XXX")?
+        .env("FASTLY_REGION", "Somewhere")?
+        .env("FASTLY_SERVICE_ID", "0000000000000000000000")?
+        .env("FASTLY_SERVICE_VERSION", "0")?
+        .env("FASTLY_TRACE_ID", "00000000-0000-0000-0000-000000000000")?
+

But I think adding support for an array of environment variables in the config syntax might make sense, which gives the ability to set these explicitly. I think that's in the spirit of parity with prod, without exposing the entire host machine's environment. I don't know how to do this, or if it's actually a good idea.

@katef katef added the feature-prod Related to matching the production C@E environment label Dec 12, 2023
@fgsch
Copy link
Member

fgsch commented Dec 13, 2023

I think as a first step we should provide some defaults as you suggested above.
We can have a separate discussion whether we want or makes sense to expose this in the configuration.

katef added a commit that referenced this issue Dec 13, 2023
We already had a few here, with values that make sense. I expect these ones were missing because their values don't make sense, I'm just hardcoding something reasonable. What matters for me is that they're present at all, and to match production ecp in what's present.

My intention is that a user would then be able to override these stub values by explicitly setting things from the config file (see #336 about that), but that there's no way to ever have these not set. Because that matches production.
katef added a commit that referenced this issue Dec 13, 2023
We already had a few here, with values that make sense. I expect these ones were missing because their values don't make sense, I'm just hardcoding something reasonable. What matters for me is that they're present at all, and to match production ecp in what's present.

My intention is that a user would then be able to override these stub values by explicitly setting things from the config file (see #336 about that), but that there's no way to ever have these not set. Because that matches production.
@katef
Copy link
Contributor Author

katef commented Dec 13, 2023

@fgsch I agree, I implemented the stubbed-out values in #337. I'll leave this ticket open for the config file overrides.

katef added a commit that referenced this issue Dec 13, 2023
…#337)

We already had a few here, with values that make sense. I expect these ones were missing because their values don't make sense, I'm just hardcoding something reasonable. What matters for me is that they're present at all, and to match production ecp in what's present.

My intention is that a user would then be able to override these stub values by explicitly setting things from the config file (see #336 about that), but that there's no way to ever have these not set. Because that matches production.
cmckendry pushed a commit to 1stdibs/Viceroy that referenced this issue Feb 8, 2024
…fastly#337)

We already had a few here, with values that make sense. I expect these ones were missing because their values don't make sense, I'm just hardcoding something reasonable. What matters for me is that they're present at all, and to match production ecp in what's present.

My intention is that a user would then be able to override these stub values by explicitly setting things from the config file (see fastly#336 about that), but that there's no way to ever have these not set. Because that matches production.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-prod Related to matching the production C@E environment
Projects
None yet
Development

No branches or pull requests

2 participants