diff --git a/CHANGELOG.md b/CHANGELOG.md index e1caf99408c..c6841551b83 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/src/deploy/functions/release/fabricator.spec.ts b/src/deploy/functions/release/fabricator.spec.ts index 64127af797d..d9bc68bc067 100644 --- a/src/deploy/functions/release/fabricator.spec.ts +++ b/src/deploy/functions/release/fabricator.spec.ts @@ -59,6 +59,7 @@ describe("Fabricator", () => { 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")); @@ -1318,6 +1319,13 @@ describe("Fabricator", () => { }); }); + 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: {}, diff --git a/src/deploy/functions/release/fabricator.ts b/src/deploy/functions/release/fabricator.ts index ee652b22b08..36aa78c44f7 100644 --- a/src/deploy/functions/release/fabricator.ts +++ b/src/deploy/functions/release/fabricator.ts @@ -1163,6 +1163,16 @@ export class Fabricator { } async disableTaskQueue(endpoint: backend.Endpoint & backend.TaskQueueTriggered): Promise { + // 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, diff --git a/src/deploy/functions/validate.spec.ts b/src/deploy/functions/validate.spec.ts index 7fcbbebd183..57fe88269f8 100644 --- a/src/deploy/functions/validate.spec.ts +++ b/src/deploy/functions/validate.spec.ts @@ -114,6 +114,44 @@ describe("validate", () => { }); }); + describe("taskQueueFunctionNamesAreValid", () => { + const ENDPOINT_BASE: Omit = { + 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, diff --git a/src/deploy/functions/validate.ts b/src/deploy/functions/validate.ts index 999c93f43ab..f6ab8869c6c 100644 --- a/src/deploy/functions/validate.ts +++ b/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. diff --git a/src/gcp/cloudtasks.spec.ts b/src/gcp/cloudtasks.spec.ts index 9d353515b1e..dfeb9788839 100644 --- a/src/gcp/cloudtasks.spec.ts +++ b/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({ diff --git a/src/gcp/cloudtasks.ts b/src/gcp/cloudtasks.ts index 56dbd4f5553..f5a09f9be8f 100644 --- a/src/gcp/cloudtasks.ts +++ b/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,