From 00a54693851a9e61bef450ff898458d0c7513c10 Mon Sep 17 00:00:00 2001 From: Torben Wetter Date: Thu, 23 Oct 2025 11:16:37 +0200 Subject: [PATCH 1/2] Fix non-interactive deployment failure when secrets exist in Secret Manager PR #9335 added a check that fails deployments in non-interactive mode when secrets are required. However, it didn't verify whether those secrets already exist in Secret Manager, causing deployments to fail even when all secrets were properly configured. This change queries Secret Manager before throwing the error to check if each required secret exists. Only truly missing secrets will cause the deployment to fail. Fixes #9368 --- CHANGELOG.md | 1 + src/deploy/functions/params.spec.ts | 95 +++++++++++++++++++++++++++++ src/deploy/functions/params.ts | 41 +++++++++---- 3 files changed, 125 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..b5fc357fb88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- Fixed non-interactive deployment failure when secrets are already configured in Secret Manager diff --git a/src/deploy/functions/params.spec.ts b/src/deploy/functions/params.spec.ts index ca7ee49b792..dcc7675091b 100644 --- a/src/deploy/functions/params.spec.ts +++ b/src/deploy/functions/params.spec.ts @@ -3,6 +3,8 @@ import * as sinon from "sinon"; import * as prompt from "../../prompt"; import * as params from "./params"; +import * as secretManager from "../../gcp/secretManager"; +import { FirebaseError } from "../../error"; const expect = chai.expect; const fakeConfig = { @@ -298,4 +300,97 @@ describe("resolveParams", () => { input.resolves("22"); await expect(params.resolveParams(paramsToResolve, fakeConfig, {})).to.eventually.be.rejected; }); + + describe("non-interactive mode with secrets", () => { + let getSecretMetadataStub: sinon.SinonStub; + + beforeEach(() => { + getSecretMetadataStub = sinon.stub(secretManager, "getSecretMetadata"); + }); + + afterEach(() => { + getSecretMetadataStub.restore(); + }); + + it("should succeed when secrets already exist in Secret Manager", async () => { + const paramsToResolve: params.Param[] = [ + { + name: "MY_SECRET", + type: "secret", + }, + ]; + getSecretMetadataStub.resolves({ + secret: { name: "MY_SECRET" }, + secretVersion: { name: "MY_SECRET/versions/1", state: "ENABLED" }, + }); + + await expect(params.resolveParams(paramsToResolve, fakeConfig, {}, { nonInteractive: true })) + .to.eventually.be.fulfilled; + }); + + it("should throw error when secrets don't exist in non-interactive mode", async () => { + const paramsToResolve: params.Param[] = [ + { + name: "MISSING_SECRET", + type: "secret", + }, + ]; + getSecretMetadataStub.resolves({}); + + await expect( + params.resolveParams(paramsToResolve, fakeConfig, {}, { nonInteractive: true }), + ).to.eventually.be.rejectedWith( + FirebaseError, + /In non-interactive mode but have no value for the following secrets: MISSING_SECRET/, + ); + }); + + it("should only report missing secrets, not existing ones in non-interactive mode", async () => { + const paramsToResolve: params.Param[] = [ + { + name: "EXISTING_SECRET", + type: "secret", + }, + { + name: "MISSING_SECRET", + type: "secret", + }, + ]; + getSecretMetadataStub.callsFake((projectId: string, secretName: string) => { + if (secretName === "EXISTING_SECRET") { + return Promise.resolve({ + secret: { name: "EXISTING_SECRET" }, + secretVersion: { name: "EXISTING_SECRET/versions/1", state: "ENABLED" }, + }); + } + return Promise.resolve({}); + }); + + try { + await params.resolveParams(paramsToResolve, fakeConfig, {}, { nonInteractive: true }); + expect.fail("Should have thrown an error"); + } catch (err: any) { + expect(err.message).to.include("MISSING_SECRET"); + expect(err.message).to.not.include("EXISTING_SECRET"); + } + }); + + it("should include format flag in error for JSON secrets", async () => { + const paramsToResolve: params.Param[] = [ + { + name: "JSON_SECRET", + type: "secret", + format: "json", + }, + ]; + getSecretMetadataStub.resolves({}); + + try { + await params.resolveParams(paramsToResolve, fakeConfig, {}, { nonInteractive: true }); + expect.fail("Should have thrown an error"); + } catch (err: any) { + expect(err.message).to.include("--format=json --data-file "); + } + }); + }); }); diff --git a/src/deploy/functions/params.ts b/src/deploy/functions/params.ts index 31a4a80c3e8..8151b53784a 100644 --- a/src/deploy/functions/params.ts +++ b/src/deploy/functions/params.ts @@ -397,18 +397,35 @@ export async function resolveParams( // Check for missing secrets in non-interactive mode if (nonInteractive && needSecret.length > 0) { - const secretNames = needSecret.map((p) => p.name).join(", "); - const commands = needSecret - .map( - (p) => - `\tfirebase functions:secrets:set ${p.name}${(p as SecretParam).format === "json" ? " --format=json --data-file " : ""}`, - ) - .join("\n"); - throw new FirebaseError( - `In non-interactive mode but have no value for the following secrets: ${secretNames}\n\n` + - "Set these secrets before deploying:\n" + - commands, - ); + // First check which secrets actually don't exist in Secret Manager + const missingSecrets: SecretParam[] = []; + for (const param of needSecret) { + const secretParam = param as SecretParam; + const metadata = await secretManager.getSecretMetadata( + firebaseConfig.projectId, + secretParam.name, + "latest", + ); + if (!metadata.secret) { + missingSecrets.push(secretParam); + } + } + + // Only throw an error if there are truly missing secrets + if (missingSecrets.length > 0) { + const secretNames = missingSecrets.map((p) => p.name).join(", "); + const commands = missingSecrets + .map( + (p) => + `\tfirebase functions:secrets:set ${p.name}${p.format === "json" ? " --format=json --data-file " : ""}`, + ) + .join("\n"); + throw new FirebaseError( + `In non-interactive mode but have no value for the following secrets: ${secretNames}\n\n` + + "Set these secrets before deploying:\n" + + commands, + ); + } } // The functions emulator will handle secrets From e86e856cef55b8b6988979b9ec12a71f1fe90095 Mon Sep 17 00:00:00 2001 From: Torben Wetter Date: Thu, 23 Oct 2025 11:48:03 +0200 Subject: [PATCH 2/2] Optimize secret existence checks to run in parallel Use Promise.all() instead of sequential await calls to check multiple secrets in parallel, improving performance when multiple secrets need to be validated. --- src/deploy/functions/params.ts | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/src/deploy/functions/params.ts b/src/deploy/functions/params.ts index 8151b53784a..133f3fbe86b 100644 --- a/src/deploy/functions/params.ts +++ b/src/deploy/functions/params.ts @@ -397,19 +397,17 @@ export async function resolveParams( // Check for missing secrets in non-interactive mode if (nonInteractive && needSecret.length > 0) { - // First check which secrets actually don't exist in Secret Manager - const missingSecrets: SecretParam[] = []; - for (const param of needSecret) { + // Check all secrets in parallel for better performance + const metadataChecks = needSecret.map((param) => { const secretParam = param as SecretParam; - const metadata = await secretManager.getSecretMetadata( - firebaseConfig.projectId, - secretParam.name, - "latest", - ); - if (!metadata.secret) { - missingSecrets.push(secretParam); - } - } + return secretManager.getSecretMetadata(firebaseConfig.projectId, secretParam.name, "latest"); + }); + const metadataResults = await Promise.all(metadataChecks); + + // Filter for secrets that don't exist + const missingSecrets: SecretParam[] = needSecret.filter((param, index) => { + return !metadataResults[index].secret; + }) as SecretParam[]; // Only throw an error if there are truly missing secrets if (missingSecrets.length > 0) {