diff --git a/CHANGELOG.md b/CHANGELOG.md index 94c870566b5..2450b4cb0f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,3 +7,4 @@ - Added web app support for Crashlytics MCP tools and prompts. - Added support for forwarding custom HTTP headers (`Mcp-Param-*`) to remote MCP tools when defined in tool parameter input schemas (`x-mcp-header`), per [SEP-2243](https://modelcontextprotocol.io/seps/2243-http-standardization). - Improved function parameter prompting clarity for multi-codebase deploys (#10897) +- Fixed a bug where deploying functions from a codebase with no extensions still required the Cloud Billing API to be enabled. (#7584) diff --git a/src/deploy/extensions/deploy.spec.ts b/src/deploy/extensions/deploy.spec.ts new file mode 100644 index 00000000000..d291451d60e --- /dev/null +++ b/src/deploy/extensions/deploy.spec.ts @@ -0,0 +1,78 @@ +import { expect } from "chai"; +import * as sinon from "sinon"; + +import { deploy } from "./deploy"; +import { Context, Payload } from "./args"; +import * as cloudbilling from "../../gcp/cloudbilling"; +import * as provisioningHelper from "../../extensions/provisioningHelper"; + +describe("Extensions deploy", () => { + let checkBillingEnabledStub: sinon.SinonStub; + let bulkCheckProductsProvisionedStub: sinon.SinonStub; + + const options: any = { nonInteractive: true, project: "test-project" }; + const instance = (instanceId: string) => ({ instanceId, params: {}, systemParams: {} }) as any; + + beforeEach(() => { + checkBillingEnabledStub = sinon.stub(cloudbilling, "checkBillingEnabled").resolves(true); + bulkCheckProductsProvisionedStub = sinon + .stub(provisioningHelper, "bulkCheckProductsProvisioned") + .resolves(); + }); + + afterEach(() => { + sinon.restore(); + }); + + it("should do nothing when there is nothing to deploy", async () => { + // A functions deploy for a codebase that declares no extensions reaches this + // stage with an empty payload, and must not require the Cloud Billing API. + await deploy({} as Context, options, {} as Payload); + + expect(checkBillingEnabledStub.called).to.be.false; + expect(bulkCheckProductsProvisionedStub.called).to.be.false; + }); + + it("should do nothing when the payload only has empty instance lists", async () => { + const payload: Payload = { + instancesToCreate: [], + instancesToUpdate: [], + instancesToConfigure: [], + instancesToDelete: [], + }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.false; + expect(bulkCheckProductsProvisionedStub.called).to.be.false; + }); + + it("should deploy normally when there is an instance to create", async () => { + const payload: Payload = { instancesToCreate: [instance("new-instance")] }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.calledWith("test-project")).to.be.true; + // Asserted so that widening the empty-payload guard above cannot silently turn + // a real deploy into a no-op. + expect(bulkCheckProductsProvisionedStub.called).to.be.true; + }); + + it("should not skip a delete-only deploy", async () => { + const payload: Payload = { instancesToDelete: [instance("doomed")] }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.true; + expect(bulkCheckProductsProvisionedStub.called).to.be.true; + }); + + it("should deploy normally when only configuring an instance", async () => { + const payload: Payload = { instancesToConfigure: [instance("existing")] }; + + await deploy({} as Context, options, payload); + + expect(checkBillingEnabledStub.called).to.be.true; + expect(bulkCheckProductsProvisionedStub.called).to.be.true; + }); +}); diff --git a/src/deploy/extensions/deploy.ts b/src/deploy/extensions/deploy.ts index 1cfdee1dbfc..88d1ddaea42 100644 --- a/src/deploy/extensions/deploy.ts +++ b/src/deploy/extensions/deploy.ts @@ -16,15 +16,33 @@ import { checkBilling } from "./validate"; * @param payload The deploy payload */ export async function deploy(context: Context, options: Options, payload: Payload): Promise { + const instancesToCreate = payload.instancesToCreate ?? []; + const instancesToUpdate = payload.instancesToUpdate ?? []; + const instancesToConfigure = payload.instancesToConfigure ?? []; + const instancesToDelete = payload.instancesToDelete ?? []; + + // Nothing to do. A functions deploy reaches this stage even when the codebase + // declares no extensions, since every codebase reports an extensions record and + // it is empty in that case. Without this guard such a deploy would go on to + // require the Cloud Billing API below. + if ( + !instancesToCreate.length && + !instancesToUpdate.length && + !instancesToConfigure.length && + !instancesToDelete.length + ) { + return; + } + const projectId = needProjectId(options); // First, check that billing is enabled await checkBilling(projectId, options.nonInteractive); // Then, check that required products are provisioned. await bulkCheckProductsProvisioned(projectId, [ - ...(payload.instancesToCreate ?? []), - ...(payload.instancesToUpdate ?? []), - ...(payload.instancesToConfigure ?? []), + ...instancesToCreate, + ...instancesToUpdate, + ...instancesToConfigure, ]); if (context.have) { @@ -43,17 +61,17 @@ export async function deploy(context: Context, options: Options, payload: Payloa // Validate all creates, updates and configures. // Skip validating local extensions, since doing so requires us to create a new source. // No need to validate deletes. - for (const create of payload.instancesToCreate?.filter((i) => !!i.ref) ?? []) { + for (const create of instancesToCreate.filter((i) => !!i.ref)) { const task = tasks.createExtensionInstanceTask(projectId, create, /* validateOnly=*/ true); void validationQueue.run(task); } - for (const update of payload.instancesToUpdate?.filter((i) => !!i.ref) ?? []) { + for (const update of instancesToUpdate.filter((i) => !!i.ref)) { const task = tasks.updateExtensionInstanceTask(projectId, update, /* validateOnly=*/ true); void validationQueue.run(task); } - for (const configure of payload.instancesToConfigure?.filter((i) => !!i.ref) ?? []) { + for (const configure of instancesToConfigure.filter((i) => !!i.ref)) { const task = tasks.configureExtensionInstanceTask( projectId, configure, diff --git a/src/deploy/extensions/prepare.spec.ts b/src/deploy/extensions/prepare.spec.ts index f8f6e8dc2a3..c89940c67b6 100644 --- a/src/deploy/extensions/prepare.spec.ts +++ b/src/deploy/extensions/prepare.spec.ts @@ -52,6 +52,36 @@ describe("Extensions prepare", () => { await expect(prepareDynamicExtensions(context, options, payload, builds)).to.not.be.rejected; }); + it("should report no plan if the extensions API is down", async () => { + haveDynamicStub.rejects(new Error("Extensions API is having an outage")); + + const options: any = { + config: { + src: { functions: { source: "functions" } }, + }, + }; + + const prepared = await prepareDynamicExtensions({}, options, {}, {}); + + expect(prepared).to.be.false; + }); + + it("should report no plan when nothing is defined and nothing exists", async () => { + // functions/prepare uses this to tell "no extensions" apart from a real plan, + // so that the deploy and release stages are skipped entirely. + const payload: Payload = {}; + const options: any = { + config: { + src: { functions: { source: "functions" } }, + }, + }; + + const prepared = await prepareDynamicExtensions({}, options, payload, {}); + + expect(prepared).to.be.false; + expect(payload).to.deep.equal({}); + }); + it("should proceed normally if extensions API is healthy", async () => { haveDynamicStub.resolves([ { @@ -90,5 +120,44 @@ describe("Extensions prepare", () => { v2apistub.restore(); tosStub.restore(); }); + + it("should report a plan when an existing instance needs deleting", async () => { + haveDynamicStub.resolves([ + { + instanceId: "test-extension", + ref: { publisherId: "test", extensionId: "test", version: "0.1.0" }, + params: {}, + systemParams: {}, + labels: { codebase: "default" }, + }, + ]); + + const payload: Payload = {}; + const options: any = { + config: { + get: () => [], + src: { functions: { source: "functions" } }, + }, + rc: { getEtags: () => [] }, + dryRun: true, + }; + + const wantDynamicStub: sinon.SinonStub = sinon.stub(planner, "wantDynamic").resolves([]); + const v2apistub: sinon.SinonStub = sinon + .stub(v2FunctionHelper, "ensureNecessaryV2ApisAndRoles") + .resolves(); + const tosStub: sinon.SinonStub = sinon + .stub(tos, "getAppDeveloperTOSStatus") + .resolves({ lastAcceptedVersion: "1.0.0" } as any); + + const prepared = await prepareDynamicExtensions({}, options, payload, {}); + + expect(prepared).to.be.true; + expect(payload.instancesToDelete).to.have.length(1); + + wantDynamicStub.restore(); + v2apistub.restore(); + tosStub.restore(); + }); }); }); diff --git a/src/deploy/extensions/prepare.ts b/src/deploy/extensions/prepare.ts index 72a42cf1976..c83d9abbcc5 100644 --- a/src/deploy/extensions/prepare.ts +++ b/src/deploy/extensions/prepare.ts @@ -161,13 +161,15 @@ async function prepareHelper( * @param options The prepare options * @param payload The prepare payload * @param builds firebase functions builds + * @return Whether a deployment plan was prepared. False means the context and + * payload were left untouched, so there is nothing for the caller to deploy. */ export async function prepareDynamicExtensions( context: Context, options: DeployOptions, payload: Payload, builds: Record, -): Promise { +): Promise { const functionsConfig = normalizeAndValidate(options.config.src.functions); const filters = getEndpointFilters(options, functionsConfig); const extensions = extractExtensionsFromBuilds(builds, filters); @@ -189,12 +191,12 @@ export async function prepareDynamicExtensions( "Failed to fetch the list of extensions. Assuming for now that there are no existing extensions. " + "If you are trying to install an extension through Firebase Functions this may fail later.", ); - return; + return false; } if (Object.keys(extensions).length === 0 && haveExtensions.length === 0) { // Nothing defined, and nothing to delete - return; + return false; } const dynamicWant = await planner.wantDynamic({ @@ -203,14 +205,8 @@ export async function prepareDynamicExtensions( extensions, }); - return prepareHelper( - context, - options, - payload, - dynamicWant, - haveExtensions, - true /* isDynamic */, - ); + await prepareHelper(context, options, payload, dynamicWant, haveExtensions, true /* isDynamic */); + return true; } /** diff --git a/src/deploy/functions/prepare.ts b/src/deploy/functions/prepare.ts index 7e13b1094e2..14a4f93fe34 100644 --- a/src/deploy/functions/prepare.ts +++ b/src/deploy/functions/prepare.ts @@ -278,9 +278,13 @@ export async function prepare( if (Object.values(wantBuilds).some((b) => b.extensions)) { const extContext: ExtContext = {}; const extPayload: ExtPayload = {}; - await prepareDynamicExtensions(extContext, options, extPayload, wantBuilds); - context.extensions = extContext; - payload.extensions = extPayload; + // Every codebase reports an extensions record, empty when it declares none, so + // reaching here does not mean there is anything to deploy. Only hand the + // extensions stages a plan when one was actually prepared. + if (await prepareDynamicExtensions(extContext, options, extPayload, wantBuilds)) { + context.extensions = extContext; + payload.extensions = extPayload; + } } // == Phase 2. Resolve build to backend.