fix: pass all required env vars to edge function invocations #342
Conversation
We weren't passing "internally" defined (within this codebase) env vars through because when we copied the logic over from Netlify CLI we refactored some mechanisms that ended up changing some behaviours unintentionally. For "internal" env vars (currently, this is `NETLIFY_BLOBS_CONTEXT`, `NETLIFY_PURGE_API_TOKEN`, and `BRANCH`), we were mutating `process.env` to set these in an earlier code path than one where we determine the `source` of each env var, which affects all sorts of logic. This ended up making it tagged as `usedSource: 'process'`, which resulted in them being filtered from the env var bag passed to the deno bridge. (It thought that these were coming from the developer's own machine, i.e. the parent process running this dev server, and should be excluded.)
| }, | ||
| "peerDependencies": { | ||
| "vite": "^5 || ^6" | ||
| "vite": "^5 || ^6 || ^7" |
| await fixture.destroy() | ||
| }) | ||
|
|
||
| test('Invoking an edge function', async () => { |
There was a problem hiding this comment.
There was no coverage for the unlinked site case. This is copied and tweaked from the linked site test.
It's rather important for env vars because there's a whole separate code path with the envelope stuff when linked.
packages/dev/src/lib/env.ts
Outdated
| value: envAPI.get(key) ?? '', | ||
| } | ||
|
|
||
| results[key] = result |
There was a problem hiding this comment.
Is the idea here that this key already existed in the object and you're not overwriting it with the right source?
There was a problem hiding this comment.
Not quite — these keys were not in variables (the bag we iterate over just below this), so they didn't end up in results at all. That loop is doing two things — building up results and populating the vars in envAPI — and it's correct that the latter is not needed for these keys, since they're already populated in the env, but we do need them in the results, otherwise they're totally lost and e.g. we can't include them in the deno invocation env bag.
There was a problem hiding this comment.
(Refactoring to make them end up in variables instead is a significant refactor, and we can always do it later)
| ...Object.keys(runtime.envSnapshot).reduce<Record<string, string>>( | ||
| (acc, key) => ({ | ||
| ...acc, | ||
| [key]: runtime.env.get(key) ?? '', |
There was a problem hiding this comment.
Does this fallback ever kick in?
There was a problem hiding this comment.
I don't think it should be possible, but the type returns | undefined.
serhalp
deleted the
serhalp/frb-1934-runtime-env-vars-are-not-available-in-edge-functions
branch
Bug
We weren't passing "internally" defined (within this codebase) env vars through because when we copied the logic over from Netlify CLI we refactored some mechanisms that ended up changing some behaviours unintentionally.
Root cause
For "internal" env vars (currently, this is
NETLIFY_BLOBS_CONTEXT,NETLIFY_PURGE_API_TOKEN, andBRANCH), we were mutatingprocess.envto set these in an earlier code path than one where we determine thesourceof each env var, which affects all sorts of logic. This ended up making it tagged asusedSource: 'process', which resulted in them being filtered from the env var bag passed to the deno bridge. (It thought that these were coming from the developer's own machine, i.e. the parent process running this dev server, and should be excluded.)Fix
We were already keeping track of these internal env vars we were setting, in a separate object called
envSnapshot. I just passed this through toinjectEnvVariablesand added them to the recorded set of injected env vars, from which they were missing previously.I also needed to fix the filtering issue. I ended up just removing it. As best I can tell (and the added test coverage agrees), it isn't needed:
'account': we want these'ui': we want these'addons': this is dead'configFile': we want this but only fordevcontext... but this context filtering has already been handled upstream'general': these are the platform env vars (DEPLOY_ID,DEPLOY_PRIME_URL, etc.) that come from@netlify/config, so we want these'internal': this was the named used in Netlify CLI for the "internal" env vars as I described above. We don't (can't, as currently implemented) use this; we handle these in a different way entirely.There were likely several different ways to fix this... I'm not sure this was an intended use of
envSnapshot, but it doesn't seem too awful, it works, and we can always refactor this now that we have full test coverage.