fix: prune secret versions labelled firebase-managed=functions

CHANGELOG.md

@@ -1,3 +1,4 @@
+- Fixed `functions:secrets:set` and `functions:secrets:prune` not destroying unused versions of secrets created with v13.6.1 or later. (#11066)
 - [fixed] Improve error message when billing is not enabled
 - Fixed a crash in `setEnqueuer` when deploying Cloud Tasks functions whose IAM policy has no bindings (#11184).
 - Updated dependencies to address security vulnerabilities, including `protobufjs`, `tar`, `@grpc/grpc-js`, `express`, `undici`, `hono`, `tmp`, and `form-data`.

src/commands/functions-secrets-prune.ts

@@ -7,11 +7,10 @@
 import { Options } from "../options";
 import { needProjectId, needProjectNumber } from "../projectUtils";
 import { requirePermissions } from "../requirePermissions";
-import { isFirebaseManaged } from "../deploymentTool";
 import { logBullet, logSuccess } from "../utils";
 import { confirm } from "../prompt";
-import { destroySecretVersion } from "../gcp/secretManager";
 import { requireAuth } from "../requireAuth";
+import { FirebaseError } from "../error";
 
 export const command = new Command("functions:secrets:prune")
   .withForce("destroy unused secrets without prompt")
@@ -30,10 +29,11 @@
 
     logBullet("Loading secrets...");
 
-    const haveBackend = await backend.existingBackend({ projectId } as args.Context);
-    const haveEndpoints = backend
-      .allEndpoints(haveBackend)
-      .filter((e) => isFirebaseManaged(e.labels || []));
+    const context = { projectId } as args.Context;
+    const haveBackend = await backend.existingBackend(context);
+    backend.assertAllRegionsReachable(context);
+    // Every function counts as a consumer, whatever tool deployed it.
+    const haveEndpoints = backend.allEndpoints(haveBackend);
 
     const pruned = await secrets.pruneSecrets({ projectNumber, projectId }, haveEndpoints);
 
@@ -48,14 +48,17 @@
         pruned.map((sv) => `${sv.secret}@${sv.version}`).join("\n\t"),
     );
 
-    const confirmed =
-      options.destroy ||
-      (await confirm({
-        message: `Do you want to destroy unused secret versions?`,
-        default: true,
-        force: options.force,
-        nonInteractive: options.nonInteractive,
-      }));
+    if (options.nonInteractive && !options.force) {
+      throw new FirebaseError(
+        "Refusing to destroy secret versions in non-interactive mode. Pass --force to destroy them without confirmation.",
+      );
+    }
+    const confirmed = await confirm({
+      message: `Do you want to destroy unused secret versions?`,
+      default: true,
+      force: options.force,
+      nonInteractive: options.nonInteractive,
+    });
     if (!confirmed) {
       logBullet(
         "Run the following commands to destroy each unused secret version:\n\t" +
@@ -65,6 +68,18 @@
       );
       return;
     }
-    await Promise.all(pruned.map((sv) => destroySecretVersion(projectId, sv.secret, sv.version)));
+    const { destroyed, erred } = await secrets.destroySecretVersions(pruned);
+    if (destroyed.length) {
+      logBullet(
+        "Destroyed secret versions:\n\t" +
+          destroyed.map((sv) => `${sv.secret}@${sv.version}`).join("\n\t"),
+      );
+    }
+    if (erred.length) {
+      throw new FirebaseError(
+        `Failed to destroy ${erred.length} secret versions:\n\t` +
+          erred.map((e) => e.message).join("\n\t"),
+      );
+    }
     logSuccess("Destroyed all unused secrets!");
   });

src/commands/functions-secrets-set.ts

@@ -12,7 +12,6 @@ import { logBullet, logSuccess, logWarning, readSecretValue } from "../utils";
 import { needProjectId, needProjectNumber } from "../projectUtils";
 import {
   addVersion,
-  destroySecretVersion,
   toSecretVersionResourceName,
   isFunctionsManaged,
   ensureApi,
@@ -31,8 +30,11 @@ export const command = new Command("functions:secrets:set <KEY>")
   .before(requirePermissions, [
     "secretmanager.secrets.create",
     "secretmanager.secrets.get",
+    "secretmanager.secrets.list",
     "secretmanager.secrets.update",
     "secretmanager.versions.add",
+    "secretmanager.versions.destroy",
+    "secretmanager.versions.list",
   ])
   .option(
     "--data-file <dataFile>",
@@ -91,7 +93,8 @@ export const command = new Command("functions:secrets:set <KEY>")
       return;
     }
 
-    let haveBackend = await backend.existingBackend({ projectId } as args.Context);
+    const context = { projectId } as args.Context;
+    let haveBackend = await backend.existingBackend(context);
     const endpointsToUpdate = backend
       .allEndpoints(haveBackend)
       .filter((e) => secrets.inUse({ projectId, projectNumber }, secret, e));
@@ -105,10 +108,19 @@ export const command = new Command("functions:secrets:set <KEY>")
         endpointsToUpdate.map((e) => `${e.id}(${e.region})`).join("\n\t"),
     );
 
+    // Only the versions these functions are moving off are candidates for destruction.
+    // Anything else unused is left to functions:secrets:prune, which lists what it will remove.
+    const staleVersions = new Set(
+      secrets
+        .of(endpointsToUpdate)
+        .filter((sev) => sev.secret === secret.name && sev.version && sev.version !== "latest")
+        .map((sev) => sev.version),
+    );
+
     const redeploy = options.nonInteractive
       ? false
       : await confirm({
-          message: `Do you want to re-deploy the functions and destroy the stale version of secret ${secret.name}?`,
+          message: `Do you want to re-deploy the functions and destroy the stale versions of secret ${secret.name}?`,
           default: true,
           force: options.force,
         });
@@ -133,7 +145,8 @@ export const command = new Command("functions:secrets:set <KEY>")
     await Promise.all(updateOps);
 
     // Double check that old secrets versions are unused.
-    haveBackend = await backend.existingBackend({ projectId } as args.Context, true);
+    haveBackend = await backend.existingBackend(context, true);
+    backend.assertAllRegionsReachable(context);
     const staleEndpoints = backend.allEndpoints(
       backend.matchingBackend(haveBackend, (e) => {
         const pInfo = { projectId, projectNumber };
@@ -151,16 +164,22 @@ export const command = new Command("functions:secrets:set <KEY>")
       );
     }
 
-    // Remove stale secret versions;
     const secretsToPrune = (
       await secrets.pruneSecrets({ projectId, projectNumber }, backend.allEndpoints(haveBackend))
-    ).filter((sv) => sv.key === key);
+    ).filter((sv) => sv.key === key && staleVersions.has(sv.version));
+    if (secretsToPrune.length === 0) {
+      return;
+    }
     logBullet(
       `Removing secret versions: ${secretsToPrune
         .map((sv) => sv.key + "[" + sv.version + "]")
         .join(", ")}`,
     );
-    await Promise.all(
-      secretsToPrune.map((sv) => destroySecretVersion(projectId, sv.secret, sv.version)),
-    );
+    const { erred } = await secrets.destroySecretVersions(secretsToPrune);
+    if (erred.length) {
+      throw new FirebaseError(
+        `Failed to destroy ${erred.length} secret versions:\n\t` +
+          erred.map((e) => e.message).join("\n\t"),
+      );
+    }
   });

src/deploy/functions/backend.spec.ts

@@ -538,6 +538,30 @@ describe("Backend", () => {
         expect(logLabeledWarning).to.have.been.called;
       });
     });
+
+    describe("assertAllRegionsReachable", () => {
+      it("does nothing when every region was reachable", async () => {
+        listAllFunctions.resolves({ functions: [], unreachable: [] });
+        listAllFunctionsV2.resolves({ functions: [], unreachable: [] });
+        const context = newContext();
+        await backend.existingBackend(context);
+
+        expect(() => backend.assertAllRegionsReachable(context)).to.not.throw();
+      });
+
+      it("throws naming the unreachable regions", async () => {
+        listAllFunctions.resolves({ functions: [], unreachable: ["us-central1"] });
+        listAllFunctionsV2.resolves({ functions: [], unreachable: ["europe-west1"] });
+        const context = newContext();
+        await backend.existingBackend(context);
+
+        expect(() => backend.assertAllRegionsReachable(context))
+          .to.throw(FirebaseError)
+          .with.property("message")
+          .that.includes("us-central1")
+          .and.includes("europe-west1");
+      });
+    });
   });
 
   describe("compareFunctions", () => {

src/deploy/functions/backend.ts

@@ -195,7 +195,7 @@
   return allMemoryOptions.includes(mem as MemoryOptions);
 }
 
 export function isValidEgressSetting(egress: unknown): egress is VpcEgressSettings {
   return egress === "PRIVATE_RANGES_ONLY" || egress === "ALL_TRAFFIC";
 }
 
@@ -465,7 +465,7 @@
 }
 
 /**
 
  * A helper utility to create an empty backend.
  * Tests that verify the behavior of one possible resource in a Backend can use
  * this method to avoid compiler errors when new fields are added to Backend.
@@ -683,12 +683,33 @@
         existingBackend.endpoints[endpoint.region][endpoint.id] = endpoint;
       }
     }
   } catch (err: any) {
     logger.debug(`Error loading Cloud Run services: ${err.message}`);
     unreachableRegions.run = ["unknown"];
   }
 }
 
+/**
+ * Throws if any region was unreachable when the existing backend was loaded. For callers that
+ * act on the absence of a function, such as destroying secret versions nothing appears to use,
+ * where a missing region must not read as "not in use".
+ * @param context A context object from the Command library, after existingBackend has run.
+ */
+export function assertAllRegionsReachable(context: Context): void {
+  const unreachable = [
+    ...(context.unreachableRegions?.gcfV1 || []),
+    ...(context.unreachableRegions?.gcfV2 || []),
+    ...(context.unreachableRegions?.run || []),
+  ];
+  if (unreachable.length) {
+    throw new FirebaseError(
+      "The following Cloud Functions regions are currently unreachable:\n\t" +
+        unreachable.join("\n\t") +
+        "\nFunctions in those regions could not be checked. Please try again in a few minutes.",
+    );
+  }
+}
+
 /**
  * A helper function that guards against unavailable regions affecting a backend deployment.
  * If the desired backend uses a region that is unavailable, a FirebaseError is thrown.
@@ -870,7 +891,7 @@
     logger.info(
       `Function name ${httpsFunc.id} is too long to have a deterministic Cloud Run URI. Printing the non-deterministic URI instead.`,
     );
     return httpsFunc.uri!;
   }
   return `https://${serviceName}-${projectNumber}.${httpsFunc.region}.run.app`;
 }

src/functions/secrets.spec.ts

@@ -227,10 +227,11 @@
     let listSecretVersionsStub: sinon.SinonStub;
     let getSecretVersionStub: sinon.SinonStub;
 
+    // Secrets created before v13.6.1 carry firebase-managed=true.
     const secret1: secretManager.Secret = {
       projectId: "project",
       name: "MY_SECRET1",
-      labels: {},
+      labels: { [secretManager.FIREBASE_MANAGED]: "true" },
       replication: {},
     };
     const secretVersion11: secretManager.SecretVersion = {
@@ -247,9 +248,35 @@
     const secret2: secretManager.Secret = {
       projectId: "project",
       name: "MY_SECRET2",
+      labels: { [secretManager.FIREBASE_MANAGED]: "functions" },
+      replication: {},
+    };
+
+    const unmanagedSecret: secretManager.Secret = {
+      projectId: "project",
+      name: "UNMANAGED",
       labels: {},
       replication: {},
     };
+
+    const apphostingSecret: secretManager.Secret = {
+      projectId: "project",
+      name: "APPHOSTING_SECRET",
+      labels: { [secretManager.FIREBASE_MANAGED]: "apphosting" },
+      replication: {},
+    };
+
+    const unmanagedVersion: secretManager.SecretVersion = {
+      secret: unmanagedSecret,
+      versionId: "1",
+      createTime: "2024-03-28T19:43:26",
+    };
+
+    const apphostingVersion: secretManager.SecretVersion = {
+      secret: apphostingSecret,
+      versionId: "1",
+      createTime: "2024-03-28T19:43:26",
+    };
     const secretVersion21: secretManager.SecretVersion = {
       secret: secret2,
       versionId: "1",
@@ -289,6 +316,43 @@
       ).to.eventually.deep.equal([]);
     });
 
+    it("finds secrets managed under either label value and ignores the rest", async () => {
+      listSecretsStub.resolves([secret1, secret2, unmanagedSecret, apphostingSecret]);
+      // Keyed by name rather than call order so an unmanaged secret slipping through
+      // shows up as an extra pruned version instead of an exhausted stub.
+      listSecretVersionsStub.withArgs("project", secret1.name).resolves([secretVersion11]);
+      listSecretVersionsStub.withArgs("project", secret2.name).resolves([secretVersion21]);
+      listSecretVersionsStub.withArgs("project", unmanagedSecret.name).resolves([unmanagedVersion]);
+      listSecretVersionsStub
+        .withArgs("project", apphostingSecret.name)
+        .resolves([apphostingVersion]);
+
+      const pruned = await secrets.pruneSecrets(
+        { projectId: "project", projectNumber: "12345" },
+        [],
+      );
+
+      expect(pruned).to.have.deep.members([secretVersion11, secretVersion21].map(toSecretEnvVar));
+      expect(pruned).to.have.length(2);
+      // Deep-equal the full argument list: calledWith would still pass if a stale
+      // label filter were being sent as a second argument.
+      expect(listSecretsStub.firstCall.args).to.deep.equal(["project"]);
+      expect(listSecretVersionsStub).to.have.callCount(2);
+    });
+
+    it("only considers enabled versions", async () => {
+      listSecretsStub.resolves([secret2]);
+      listSecretVersionsStub.resolves([secretVersion21]);
+
+      await secrets.pruneSecrets({ projectId: "project", projectNumber: "12345" }, []);
+
+      expect(listSecretVersionsStub.firstCall.args).to.deep.equal([
+        "project",
+        secret2.name,
+        "state: ENABLED",
+      ]);
+    });
+
     it("returns all secrets given no endpoints", async () => {
       listSecretsStub.resolves([secret1, secret2]);
       listSecretVersionsStub.onFirstCall().resolves([secretVersion11, secretVersion12]);
@@ -436,6 +500,50 @@
     });
   });
 
+  describe("destroySecretVersions", () => {
+    let destroySecretVersionStub: sinon.SinonStub;
+
+    const version1: secrets.SecretForPruning = {
+      projectId: "project",
+      key: "MY_SECRET",
+      secret: "MY_SECRET",
+      version: "1",
+    };
+    const version2: secrets.SecretForPruning = { ...version1, version: "2" };
+
+    beforeEach(() => {
+      destroySecretVersionStub = sinon
+        .stub(secretManager, "destroySecretVersion")
+        .rejects("Unexpected call");
+    });
+
+    afterEach(() => {
+      destroySecretVersionStub.restore();
+    });
+
+    it("destroys every version and reports them", async () => {
+      destroySecretVersionStub.resolves();
+
+      await expect(secrets.destroySecretVersions([version1, version2])).to.eventually.deep.equal({
+        destroyed: [version1, version2],
+        erred: [],
+      });
+      expect(destroySecretVersionStub).to.have.been.calledWithExactly("project", "MY_SECRET", "1");
+      expect(destroySecretVersionStub).to.have.been.calledWithExactly("project", "MY_SECRET", "2");
+    });
+
+    it("keeps destroying after a failure and reports both outcomes", async () => {
+      destroySecretVersionStub.withArgs("project", "MY_SECRET", "1").rejects({ message: "boom" });
+      destroySecretVersionStub.withArgs("project", "MY_SECRET", "2").resolves();
+
+      await expect(secrets.destroySecretVersions([version1, version2])).to.eventually.deep.equal({
+        destroyed: [version2],
+        erred: [{ message: "boom" }],
+      });
+      expect(destroySecretVersionStub).to.have.callCount(2);
+    });
+  });
+
   describe("pruneAndDestroySecrets", () => {
     let pruneSecretsStub: sinon.SinonStub;
     let destroySecretVersionStub: sinon.SinonStub;
@@ -556,7 +664,7 @@
       };
       const fn: Omit<gcf.CloudFunction, gcf.OutputOnlyFields> = {
         name: `projects/${endpoint.project}/locations/${endpoint.region}/functions/${endpoint.id}`,
         runtime: endpoint.runtime!,
         entryPoint: endpoint.entryPoint,
         secretEnvironmentVariables: [{ ...sev, version: "2" }],
       };