Add support for secrets in the App Hosting emulator #8305
Conversation
src/emulator/apphosting/serve.ts
Outdated
| const secretResourceRegex = | ||
| /^projects\/([^/]+)\/secrets\/([^/]+)(?:\/versions\/((?:latest)|\d+))?$/; | ||
| const secretShorthandRegex = /^([^/@]+)(?:@((?:latest)|\d+))?$/; |
There was a problem hiding this comment.
Can there be comments that show examples of what these match
|
|
||
| await serve.start({ startCommand }); | ||
|
|
||
| expect(spawnWithCommandStringStub).to.be.called; | ||
| expect(spawnWithCommandStringStub.getCall(0).args[0]).to.eq(startCommand); | ||
| }); | ||
|
|
||
| it("Should pass plaintext environment varialbes", async () => { |
src/emulator/apphosting/serve.ts
Outdated
| @@ -55,13 +90,17 @@ async function serve( | |||
| for (const env of apphostingLocalConfig.environmentVariables) { | |||
| environmentVariablesAsRecord[env.variable] = env.value!; | |||
| } | |||
| await Promise.all( | |||
| apphostingLocalConfig.secrets?.map(async (secret) => { | |||
There was a problem hiding this comment.
We should update the logic for merge:
firebase-tools/src/apphosting/yaml.ts
Line 85 in bbc33ec
Where we remove secrets if they are overridden by an env var in apphosting.local.yaml
Otherwise, there is no way to override the secret to prevent it from fetching it during emulator start.
There was a problem hiding this comment.
I think I'm just going to remove the shorthand of variables vs secrets. The helper methods are a lot of work just to avoid a filter on a call site and as you point out, make merging fragile
There was a problem hiding this comment.
Based on Daniel's catch that env wasn't merging right, I did the cleanup that I was delaying for another CL.
IDK why the appyhosting yaml file was made to look like idiomatic java instead of idiomatic javascript. Were we afraid to use a little functional programming? Ripping all that out fixed the bug, made future code less fragile, and removed about 300LOC.
| availability: ["RUNTIME"], | ||
| }); | ||
| }; | ||
| } | ||
|
|
||
| // remove secrets to avoid confusion as they are not read anyways. |
src/apphosting/yaml.ts
Outdated
| yamlConfigToWrite.env = [...this.environmentVariables, ...this.secrets]; | ||
| yamlConfigToWrite.env = Object.entries(this.env).map(([variable, env]) => { | ||
| return { variable, ...env }; | ||
| }); | ||
|
|
||
| store(filePath, yaml.parseDocument(jsYaml.dump(yamlConfigToWrite))); | ||
| } | ||
| } | ||
|
|
||
| function parseEnv(envs: Env[]) { |
There was a problem hiding this comment.
return type is Record<string, Env> i think?
| .map(([key]) => key); | ||
| if (wereSecrets.some((key) => other.env[key]?.value)) { | ||
| throw new FirebaseError( | ||
| `Cannot convert secret to plaintext in ${other.filename ? other.filename : "apphosting yaml"}`, |
There was a problem hiding this comment.
| `Cannot convert secret to plaintext in ${other.filename ? other.filename : "apphosting yaml"}`, | |
| `Cannot convert secret to plaintext in ${other.filename ? other.filename : "apphosting.yaml"}`, |
There was a problem hiding this comment.
alternatively, other.filename ?? "apphosting.yaml"
There was a problem hiding this comment.
This shoudl only ever happen in tests where we start with .empty() instead of loading from a file. I wanted something different so that we didn't accidentally start printing invalid file names if we didn't actually know what was right.
src/emulator/apphosting/serve.ts
Outdated
| secretId = match[1]; | ||
| version = match[2] || "latest"; | ||
| } | ||
| return await secrets.accessSecretVersion(projectId, secretId, version); |
There was a problem hiding this comment.
can we try/catch this to print user friendly log when status == 401 with suggestion to use firebase apphosting:secrets:grantaccess command?
There was a problem hiding this comment.
Adding for 403. 401 should never happen, right?
| JSON.stringify([{ variable: "TEST", value: "overwritten_value" }]), | ||
| ); | ||
| describe("merge", () => { | ||
| it("merges incoming apphosting yaml config with precendence", () => { |
There was a problem hiding this comment.
| it("merges incoming apphosting yaml config with precendence", () => { | |
| it("merges incoming apphosting.yaml config with precendence", () => { |
There was a problem hiding this comment.
Technically incorrect, no? It's about merging apphosting.local.yaml over apphosting.emulators.yaml over apphosting.yaml
The emulator can now read secret references at startup. Care is taken to be fully reverse compatible so that projectId is only needed when loading a secret value from an id. Cloud Secrets Manager is only ever called if the customer tries to reference the secret.
Future changes may evolve the best practice to move customers to using a secret reference, but must steer customers (hard) away from accidentally committing a plaintext secret to their repository.