fix!: add validation for names of task queue funcs

CHANGELOG.md

@@ -1 +1,2 @@
+- Deploying a task queue function whose name is not a legal Cloud Tasks queue ID (for example one containing an underscore) now fails validation instead of silently leaving the function without a queue, and such functions can now be deleted (#10834).
 - [Fixed] Report the GCFv2-to-GCFv1 downgrade error during validation instead of a misleading CPU error (#5461).

src/deploy/functions/release/fabricator.spec.ts

@@ -59,6 +59,7 @@
     scheduler.jobFromEndpoint.restore();
     tasks.queueFromEndpoint.restore();
     tasks.queueNameForEndpoint.restore();
+    tasks.isValidQueueId.restore();
     runv2.serviceFromEndpoint.restore();
     gcf.createFunction.rejects(new Error("unexpected gcf.createFunction"));
     gcf.updateFunction.rejects(new Error("unexpected gcf.updateFunction"));
@@ -452,7 +453,7 @@
     it("handles topics that already exist", async () => {
       pubsub.createTopic.callsFake(() => {
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -527,7 +528,7 @@
       eventarc.createChannel.callsFake(({ name }) => {
         expect(name).to.equal("channel");
         const err = new Error("Already exists");
         (err as any).status = 409;
         return Promise.reject(err);
       });
       gcfv2.createFunction.resolves({ name: "op", done: false });
@@ -593,7 +594,7 @@
       eventarc.getChannel.resolves(undefined);
       eventarc.createChannel.callsFake(() => {
         const err = new Error("🤷‍♂️");
         (err as any).status = 400;
         return Promise.reject(err);
       });
 
@@ -1318,6 +1319,13 @@
       });
     });
 
+    it("skips ids that cannot be queue ids", async () => {
+      const ep = endpoint({ taskQueueTrigger: {} }, { id: "dummy_function" }) as backend.Endpoint &
+        backend.TaskQueueTriggered;
+      await fab.disableTaskQueue(ep);
+      expect(tasks.updateQueue).to.not.have.been.called;
+    });
+
     it("wraps errors", async () => {
       const ep = endpoint({
         taskQueueTrigger: {},
@@ -1907,7 +1915,7 @@
       expect(deleteEndpoint).to.not.have.been.called;
 
       // Resolve the create operation
       resolveCreate!();
 
       await applyPlanPromise;
 
@@ -1954,7 +1962,7 @@
 
   describe("createRunFunction", () => {
     it("creates a Cloud Run service with correct configuration", async () => {
       runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
       run.setInvokerCreate.resolves();
 
       const ep = endpoint(
@@ -1995,7 +2003,7 @@
     });
 
     it("always sets callable triggers to public on creation", async () => {
       runv2.createService.resolves({ uri: "https://service", name: "service" } as any);
       run.setInvokerCreate.resolves();
 
       const ep = endpoint(

src/deploy/functions/release/fabricator.ts

@@ -1163,6 +1163,16 @@ export class Fabricator {
   }
 
   async disableTaskQueue(endpoint: backend.Endpoint & backend.TaskQueueTriggered): Promise<void> {
+    // The queue name is derived from the function id, so a function whose id is not a legal
+    // queue ID cannot have a queue to disable. Older CLI versions let such functions deploy,
+    // and Cloud Tasks rejects the name outright, which would otherwise block their deletion.
+    if (!cloudtasks.isValidQueueId(endpoint.id)) {
+      logger.debug(
+        `Skipping disable of task queue for ${endpoint.id}: not a legal Cloud Tasks queue ID, ` +
+          `so no queue can exist.`,
+      );
+      return;
+    }
     const update = {
       name: cloudtasks.queueNameForEndpoint(endpoint),
       state: "DISABLED" as cloudtasks.State,

src/deploy/functions/validate.spec.ts

@@ -114,6 +114,44 @@ describe("validate", () => {
     });
   });
 
+  describe("taskQueueFunctionNamesAreValid", () => {
+    const ENDPOINT_BASE: Omit<backend.Endpoint, "httpsTrigger"> = {
+      platform: "gcfv2",
+      id: "id",
+      region: "us-east1",
+      project: "project",
+      entryPoint: "id",
+      runtime: "nodejs16",
+    };
+
+    it("should not throw on hyphenated task queue function names", () => {
+      const endpoints: backend.Endpoint[] = [
+        { ...ENDPOINT_BASE, id: "my-task-function", taskQueueTrigger: {} },
+      ];
+      expect(() => {
+        validate.taskQueueFunctionNamesAreValid(endpoints);
+      }).to.not.throw();
+    });
+
+    it("should throw on underscores in task queue function names", () => {
+      const endpoints: backend.Endpoint[] = [
+        { ...ENDPOINT_BASE, id: "dummy_function", taskQueueTrigger: {} },
+      ];
+      expect(() => {
+        validate.taskQueueFunctionNamesAreValid(endpoints);
+      }).to.throw(FirebaseError, /dummy_function/);
+    });
+
+    it("should ignore underscores in non-task-queue function names", () => {
+      const endpoints: backend.Endpoint[] = [
+        { ...ENDPOINT_BASE, id: "dummy_function", httpsTrigger: {} },
+      ];
+      expect(() => {
+        validate.taskQueueFunctionNamesAreValid(endpoints);
+      }).to.not.throw();
+    });
+  });
+
   describe("endpointsAreValid", () => {
     const ENDPOINT_BASE: backend.Endpoint = {
       platform: "gcfv2",
@@ -135,6 +173,18 @@ describe("validate", () => {
       expect(() => validate.endpointsAreValid(backend.of(ep))).to.throw(/GCF gen 1/);
     });
 
+    it("rejects task queue function names that are not legal queue ids", () => {
+      const ep: backend.Endpoint = {
+        ...ENDPOINT_BASE,
+        id: "dummy_function",
+        taskQueueTrigger: {},
+      };
+      expect(() => validate.endpointsAreValid(backend.of(ep))).to.throw(
+        FirebaseError,
+        /dummy_function/,
+      );
+    });
+
     it("Disallows concurrency for low-CPU gen 2", () => {
       const ep: backend.Endpoint = {
         ...ENDPOINT_BASE,

src/deploy/functions/validate.ts

@@ -3,6 +3,7 @@ import * as clc from "colorette";
 
 import { FirebaseError } from "../../error";
 import { getSecretVersion, SecretVersion } from "../../gcp/secretManager";
+import * as cloudtasks from "../../gcp/cloudtasks";
 import { logger } from "../../logger";
 import {
   EndpointFilter,
@@ -96,6 +97,7 @@ export function endpointsAreValid(
   validateLifecycleHooks(wantBackend, existingBackend);
   const endpoints = backend.allEndpoints(wantBackend);
   functionIdsAreValid(endpoints);
+  taskQueueFunctionNamesAreValid(endpoints);
   validateTimeoutConfig(endpoints);
   for (const ep of endpoints) {
     validateScheduledTimeout(ep);
@@ -358,6 +360,27 @@ export function functionIdsAreValid(functions: { id: string; platform: string }[
   }
 }
 
+/**
+ * Validate that task queue function names conform to Cloud Tasks queue ID naming rules.
+ * Unlike Cloud Functions, Cloud Tasks queue IDs (which we derive from the function name)
+ * cannot contain underscores. See
+ * https://cloud.google.com/tasks/docs/reference/rest/v2/projects.locations.queues#Queue
+ * @throws { FirebaseError } Task queue function names must be valid Cloud Tasks queue IDs.
+ */
+export function taskQueueFunctionNamesAreValid(endpoints: backend.Endpoint[]): void {
+  const invalidIds = endpoints
+    .filter(backend.isTaskQueueTriggered)
+    .filter((ep) => !cloudtasks.isValidQueueId(ep.id));
+  if (invalidIds.length !== 0) {
+    const msg =
+      `Task queue function name(s) ${invalidIds.map((ep) => ep.id).join(", ")} cannot be used as ` +
+      `Cloud Tasks queue IDs, so their queues were never created. Rename each function to use ` +
+      `only letters, numbers, and hyphens. Python function names cannot contain hyphens, so use ` +
+      `a name with no separator at all.`;
+    throw new FirebaseError(msg);
+  }
+}
+
 /**
  * Validate secret environment variables setting, if any.
  * A bad secret configuration can lead to a significant delay in function deploys.

src/gcp/cloudtasks.spec.ts

@@ -21,6 +21,7 @@ describe("CloudTasks", () => {
   beforeEach(() => {
     ct = sinon.stub(cloudtasks);
     ct.queueNameForEndpoint.restore();
+    ct.isValidQueueId.restore();
     ct.queueFromEndpoint.restore();
     ct.triggerFromQueue.restore();
     ct.setEnqueuer.restore();
@@ -31,6 +32,26 @@ describe("CloudTasks", () => {
     sinon.verifyAndRestore();
   });
 
+  describe("isValidQueueId", () => {
+    it("accepts letters, numbers and hyphens", () => {
+      expect(cloudtasks.isValidQueueId("my-queue-2")).to.be.true;
+    });
+
+    it("rejects underscores", () => {
+      expect(cloudtasks.isValidQueueId("dummy_function")).to.be.false;
+    });
+
+    it("accepts uppercase", () => {
+      expect(cloudtasks.isValidQueueId("MyQueue")).to.be.true;
+    });
+
+    it("rejects ids that are empty or too long", () => {
+      expect(cloudtasks.isValidQueueId("")).to.be.false;
+      expect(cloudtasks.isValidQueueId("a".repeat(100))).to.be.true;
+      expect(cloudtasks.isValidQueueId("a".repeat(101))).to.be.false;
+    });
+  });
+
   describe("queueFromEndpoint", () => {
     it("handles minimal endpoints", () => {
       expect(cloudtasks.queueFromEndpoint(ENDPOINT)).to.deep.equal({

src/gcp/cloudtasks.ts

@@ -217,6 +217,15 @@ export async function setEnqueuer(
   }
 }
 
+/**
+ * Whether a string is a legal Cloud Tasks queue ID.
+ * Notably narrower than a Cloud Functions function name, which also permits underscores.
+ * https://cloud.google.com/tasks/docs/reference/rest/v2/projects.locations.queues#Queue
+ */
+export function isValidQueueId(id: string): boolean {
+  return /^[a-zA-Z0-9-]{1,100}$/.test(id);
+}
+
 /** The name of the Task Queue we will use for this endpoint. */
 export function queueNameForEndpoint(
   endpoint: backend.Endpoint & backend.TaskQueueTriggered,