From d66d9058dace7274443b4613ef4ac89b601c5ef9 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Fri, 15 Aug 2025 13:16:18 -0700 Subject: [PATCH 1/5] BYO bucket for v2 functions uploads with runfunctions enabled --- src/deploy/functions/deploy.spec.ts | 103 ++++++++++++++++++++++++++++ src/deploy/functions/deploy.ts | 76 ++++++++++++++++---- src/gcp/cloudfunctionsv2.ts | 2 +- src/gcp/storage.ts | 47 ++++++++++--- 4 files changed, 204 insertions(+), 24 deletions(-) diff --git a/src/deploy/functions/deploy.spec.ts b/src/deploy/functions/deploy.spec.ts index db042b4063d..4d6a585a21a 100644 --- a/src/deploy/functions/deploy.spec.ts +++ b/src/deploy/functions/deploy.spec.ts @@ -1,8 +1,12 @@ import { expect } from "chai"; +import * as sinon from "sinon"; import * as args from "./args"; import * as backend from "./backend"; import * as deploy from "./deploy"; +import * as gcs from "../../gcp/storage"; +import * as gcfv2 from "../../gcp/cloudfunctionsv2"; +import * as experiments from "../../experiments"; describe("deploy", () => { const ENDPOINT_BASE: Omit = { @@ -137,4 +141,103 @@ describe("deploy", () => { expect(result).to.be.false; }); }); + + describe("uploadSourceV2", () => { + let gcsUploadStub: sinon.SinonStub; + let gcsUpsertBucketStub: sinon.SinonStub; + let gcfv2GenerateUploadUrlStub: sinon.SinonStub; + let createReadStreamStub: sinon.SinonStub; + let experimentEnabled: boolean; + + const SOURCE: args.Source = { + functionsSourceV2: "source.zip", + functionsSourceV2Hash: "source-hash", + }; + + before(() => { + experimentEnabled = experiments.isEnabled("runfunctions"); + }); + after(() => experiments.setEnabled("runfunctions", experimentEnabled)); + + beforeEach(() => { + gcsUploadStub = sinon.stub(gcs, "upload").resolves({ generation: "1" }); + gcsUpsertBucketStub = sinon.stub(gcs, "upsertBucket").resolves(); + gcfv2GenerateUploadUrlStub = sinon.stub(gcfv2, "generateUploadUrl").resolves({ + uploadUrl: "https://storage.googleapis.com/upload/url", + storageSource: { + bucket: "gcf-sources-123-us-central1", + object: "source-hash.zip", + }, + }); + createReadStreamStub = sinon.stub(deploy, "createReadStream").returns("stream" as any); + }); + + afterEach(() => { + sinon.restore(); + }); + + describe("with runfunctions experiment enabled", () => { + before(() => experiments.setEnabled("runfunctions", true)); + + it("should call gcs.upsertBucket and gcs.upload for gcfv2 functions", async () => { + const wantBackend = backend.of({ ...ENDPOINT, platform: "gcfv2" }); + + await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend); + + expect(gcsUpsertBucketStub).to.be.calledOnceWith("project", { + name: "firebase-functions-src-123456", + location: "region", + lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + }); + expect(createReadStreamStub).to.be.calledOnceWith("source.zip"); + expect(gcsUploadStub).to.be.calledOnceWith( + { file: "source.zip", stream: "stream" }, + "firebase-functions-src-123456/source-hash.zip", + undefined, + true, + ); + expect(gcfv2GenerateUploadUrlStub).not.to.be.called; + }); + + it("should call gcs.upsertBucket and gcs.upload for run functions", async () => { + const wantBackend = backend.of({ ...ENDPOINT, platform: "run" }); + + await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend); + + expect(gcsUpsertBucketStub).to.be.calledOnceWith("project", { + name: "firebase-functions-src-123456", + location: "region", + lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + }); + expect(createReadStreamStub).to.be.calledOnceWith("source.zip"); + expect(gcsUploadStub).to.be.calledOnceWith( + { file: "source.zip", stream: "stream" }, + "firebase-functions-src-123456/source-hash.zip", + undefined, + true, + ); + expect(gcfv2GenerateUploadUrlStub).not.to.be.called; + }); + }); + + context("with runfunctions experiment disabled", () => { + before(() => experiments.setEnabled("runfunctions", false)); + + it("should call gcfv2.generateUploadUrl and gcs.upload", async () => { + const wantBackend = backend.of({ ...ENDPOINT, platform: "gcfv2" }); + + await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend); + + expect(gcfv2GenerateUploadUrlStub).to.be.calledOnceWith("project", "region"); + expect(createReadStreamStub).to.be.calledOnceWith("source.zip"); + expect(gcsUploadStub).to.be.calledOnceWith( + { file: "source.zip", stream: "stream" }, + "https://storage.googleapis.com/upload/url", + undefined, + true, + ); + expect(gcsUpsertBucketStub).not.to.be.called; + }); + }); + }); }); diff --git a/src/deploy/functions/deploy.ts b/src/deploy/functions/deploy.ts index 3ecfe091bf0..2a6cbad30a3 100644 --- a/src/deploy/functions/deploy.ts +++ b/src/deploy/functions/deploy.ts @@ -11,8 +11,10 @@ import * as gcs from "../../gcp/storage"; import * as gcf from "../../gcp/cloudfunctions"; import * as gcfv2 from "../../gcp/cloudfunctionsv2"; import * as backend from "./backend"; +import * as experiments from "../../experiments"; import { findEndpoint } from "./backend"; import { deploy as extDeploy } from "../extensions"; +import { getProjectNumber } from "../../getProjectNumber"; setGracefulCleanup(); @@ -48,33 +50,80 @@ async function uploadSourceV1( return uploadUrl; } -async function uploadSourceV2( +// Trampoline to allow tests to mock out createStream. +export function createReadStream(filePath: string): NodeJS.ReadableStream { + return fs.createReadStream(filePath); +} + +export async function uploadSourceV2( projectId: string, + projectNumber: string, source: args.Source, wantBackend: backend.Backend, ): Promise { - const v2Endpoints = backend.allEndpoints(wantBackend).filter((e) => e.platform === "gcfv2"); + const v2Endpoints = backend + .allEndpoints(wantBackend) + .filter((e) => e.platform === "gcfv2" || e.platform === "run"); if (v2Endpoints.length === 0) { return; } + // N.B. Should we upload to multiple regions? For now, just pick the first one. + // Uploading to multiple regions might slow upload and cost the user money if they + // pay their ISP for bandwidth, but having a bucket per region would avoid cross-region + // fees from GCP. const region = v2Endpoints[0].region; // Just pick a region to upload the source. - const res = await gcfv2.generateUploadUrl(projectId, region); const uploadOpts = { file: source.functionsSourceV2!, - stream: fs.createReadStream(source.functionsSourceV2!), + stream: (exports as { createReadStream: typeof createReadStream }).createReadStream( + source.functionsSourceV2!, + ), }; - if (process.env.GOOGLE_CLOUD_QUOTA_PROJECT) { - logLabeledWarning( - "functions", - "GOOGLE_CLOUD_QUOTA_PROJECT is not usable when uploading source for Cloud Functions.", - ); + + // Legacy behavior: use the GCF API + if (!experiments.isEnabled("runfunctions")) { + if (process.env.GOOGLE_CLOUD_QUOTA_PROJECT) { + logLabeledWarning( + "functions", + "GOOGLE_CLOUD_QUOTA_PROJECT is not usable when uploading source for Cloud Functions.", + ); + } + const res = await gcfv2.generateUploadUrl(projectId, region); + await gcs.upload(uploadOpts, res.uploadUrl, undefined, true /* ignoreQuotaProject */); + return res.storageSource; } - await gcs.upload(uploadOpts, res.uploadUrl, undefined, true /* ignoreQuotaProject */); - return res.storageSource; + + // New behavior: BYO bucket since we're moving away from the GCF API. + // Using "func-" as the prefix to balance clarity and avoid name lenght limits. + // TODO: can wire project number through to ensure we fit in the 63 char limit. + const bucketName = `firebase-functions-src-${projectNumber}`; + const bucketOptions: gcs.CreateBucketRequest = { + name: bucketName, + location: region, + lifecycle: { + rule: [ + { + action: { type: "Delete" }, + condition: { age: 1 }, // Delete objects after 1 day + }, + ], + }, + }; + await gcs.upsertBucket(projectId, bucketOptions); + await gcs.upload( + uploadOpts, + `${bucketName}/${source.functionsSourceV2Hash}.zip`, + undefined, + true /* ignoreQuotaProject */, + ); + return { + bucket: bucketName, + object: source.functionsSourceV2Hash + ".zip", + }; } async function uploadCodebase( context: args.Context, + projectNumber: string, codebase: string, wantBackend: backend.Backend, ): Promise { @@ -86,7 +135,7 @@ async function uploadCodebase( const uploads: Promise[] = []; try { uploads.push(uploadSourceV1(context.projectId, source, wantBackend)); - uploads.push(uploadSourceV2(context.projectId, source, wantBackend)); + uploads.push(uploadSourceV2(context.projectId, projectNumber, source, wantBackend)); const [sourceUrl, storage] = await Promise.all(uploads); if (sourceUrl) { @@ -132,7 +181,8 @@ export async function deploy( if (shouldUploadBeSkipped(context, wantBackend, haveBackend)) { continue; } - uploads.push(uploadCodebase(context, codebase, wantBackend)); + const projectNumber = options.projectNumber || (await getProjectNumber(context.projectId)); + uploads.push(uploadCodebase(context, projectNumber, codebase, wantBackend)); } await Promise.all(uploads); } diff --git a/src/gcp/cloudfunctionsv2.ts b/src/gcp/cloudfunctionsv2.ts index a1d13ddff16..2faf69aa71a 100644 --- a/src/gcp/cloudfunctionsv2.ts +++ b/src/gcp/cloudfunctionsv2.ts @@ -60,7 +60,7 @@ export interface BuildConfig { export interface StorageSource { bucket: string; object: string; - generation: number; + generation?: number; } export interface RepoSource { diff --git a/src/gcp/storage.ts b/src/gcp/storage.ts index f00aa3df0ba..cd38019d900 100644 --- a/src/gcp/storage.ts +++ b/src/gcp/storage.ts @@ -142,7 +142,7 @@ interface GetDefaultBucketResponse { }; } -interface CreateBucketRequest { +export interface CreateBucketRequest { name: string; location: string; lifecycle: { @@ -150,7 +150,7 @@ interface CreateBucketRequest { }; } -interface LifecycleRule { +export interface LifecycleRule { action: { type: string; }; @@ -223,9 +223,10 @@ export async function upload( uploadUrl: string, extraHeaders?: Record, ignoreQuotaProject?: boolean, -): Promise { - const url = new URL(uploadUrl); - const localAPIClient = new Client({ urlPrefix: url.origin, auth: false }); +): Promise<{ generation: string | null }> { + const url = new URL(uploadUrl, storageOrigin()); + const signedUrl = url.searchParams.has("GoogleAccessId"); + const localAPIClient = new Client({ urlPrefix: url.origin, auth: !signedUrl }); const res = await localAPIClient.request({ method: "PUT", path: url.pathname, @@ -329,17 +330,24 @@ export async function getBucket(bucketName: string): Promise { export async function createBucket( projectId: string, req: CreateBucketRequest, + projectPrivate?: boolean, ): Promise { + const queryParams: Record = { + project: projectId, + }; + // TODO: This should probably be always on, but we need to audit the other cases of this method to + // make sure we don't break anything. + if (projectPrivate) { + queryParams["predefinedAcl"] = "projectPrivate"; + queryParams["predefinedDefaultObjectAcl"] = "projectPrivate"; + } + try { const localAPIClient = new Client({ urlPrefix: storageOrigin() }); const result = await localAPIClient.post( `/storage/v1/b`, req, - { - queryParams: { - project: projectId, - }, - }, + { queryParams }, ); return result.body; } catch (err: any) { @@ -350,6 +358,25 @@ export async function createBucket( } } +/** + * Creates a storage bucket on GCP if it does not already exist. + * @param {string} projectId + * @param {CreateBucketRequest} req + */ +export async function upsertBucket(projectId: string, req: CreateBucketRequest): Promise { + try { + await getBucket(req.name); + return; + } catch (err: any) { + // If the bucket doesn't exist, create it. + if (err.original?.status === 404) { + await createBucket(projectId, req, true /* projectPrivate */); + return; + } + throw err; + } +} + /** * Gets the list of storage buckets associated with a specific project from GCP. * Ref: https://cloud.google.com/storage/docs/json_api/v1/buckets/list From 5662b5d3153ac8d4744be58c8c5cb183b52d33c0 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Fri, 15 Aug 2025 13:24:09 -0700 Subject: [PATCH 2/5] update comments --- src/deploy/functions/deploy.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/deploy/functions/deploy.ts b/src/deploy/functions/deploy.ts index 2a6cbad30a3..9072f8d1f44 100644 --- a/src/deploy/functions/deploy.ts +++ b/src/deploy/functions/deploy.ts @@ -93,8 +93,7 @@ export async function uploadSourceV2( } // New behavior: BYO bucket since we're moving away from the GCF API. - // Using "func-" as the prefix to balance clarity and avoid name lenght limits. - // TODO: can wire project number through to ensure we fit in the 63 char limit. + // Using project number to ensure we don't exceed the bucket name length limit (in addition to PII controversy). const bucketName = `firebase-functions-src-${projectNumber}`; const bucketOptions: gcs.CreateBucketRequest = { name: bucketName, From 5b627297e3c704707ec686327653f262437c02be Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Mon, 18 Aug 2025 15:26:30 -0700 Subject: [PATCH 3/5] Reconcile FAH & CF3 upsert bucket. Fix Map usage in FAH --- src/deploy/apphosting/args.ts | 6 +- src/deploy/apphosting/deploy.spec.ts | 62 ++++++------ src/deploy/apphosting/deploy.ts | 127 +++++++++++-------------- src/deploy/apphosting/prepare.spec.ts | 23 +++-- src/deploy/apphosting/prepare.ts | 14 +-- src/deploy/apphosting/release.spec.ts | 26 +++-- src/deploy/apphosting/release.ts | 59 ++++++------ src/deploy/functions/deploy.spec.ts | 28 ++++-- src/deploy/functions/deploy.ts | 28 +++--- src/gcp/storage.spec.ts | 132 ++++++++++++++++++++++++++ src/gcp/storage.ts | 44 +++++++-- 11 files changed, 350 insertions(+), 199 deletions(-) create mode 100644 src/gcp/storage.spec.ts diff --git a/src/deploy/apphosting/args.ts b/src/deploy/apphosting/args.ts index 7c94697e120..e18e2d16e59 100644 --- a/src/deploy/apphosting/args.ts +++ b/src/deploy/apphosting/args.ts @@ -1,7 +1,7 @@ import { AppHostingSingle } from "../../firebaseConfig"; export interface Context { - backendConfigs: Map; - backendLocations: Map; - backendStorageUris: Map; + backendConfigs: Record; + backendLocations: Record; + backendStorageUris: Record; } diff --git a/src/deploy/apphosting/deploy.spec.ts b/src/deploy/apphosting/deploy.spec.ts index 0fafc735494..24da9869caa 100644 --- a/src/deploy/apphosting/deploy.spec.ts +++ b/src/deploy/apphosting/deploy.spec.ts @@ -1,8 +1,6 @@ import { expect } from "chai"; import * as sinon from "sinon"; import { Config } from "../../config"; -import { FirebaseError } from "../../error"; -import { AppHostingSingle } from "../../firebaseConfig"; import * as gcs from "../../gcp/storage"; import { RC } from "../../rc"; import { Context } from "./args"; @@ -26,24 +24,20 @@ const BASE_OPTS = { function initializeContext(): Context { return { - backendConfigs: new Map([ - [ - "foo", - { - backendId: "foo", - rootDir: "/", - ignore: [], - }, - ], - ]), - backendLocations: new Map([["foo", "us-central1"]]), - backendStorageUris: new Map(), + backendConfigs: { + foo: { + backendId: "foo", + rootDir: "/", + ignore: [], + }, + }, + backendLocations: { foo: "us-central1" }, + backendStorageUris: {}, }; } describe("apphosting", () => { - let getBucketStub: sinon.SinonStub; - let createBucketStub: sinon.SinonStub; + let upsertBucketStub: sinon.SinonStub; let uploadObjectStub: sinon.SinonStub; let createArchiveStub: sinon.SinonStub; let createReadStreamStub: sinon.SinonStub; @@ -53,8 +47,7 @@ describe("apphosting", () => { getProjectNumberStub = sinon .stub(getProjectNumber, "getProjectNumber") .throws("Unexpected getProjectNumber call"); - getBucketStub = sinon.stub(gcs, "getBucket").throws("Unexpected getBucket call"); - createBucketStub = sinon.stub(gcs, "createBucket").throws("Unexpected createBucket call"); + upsertBucketStub = sinon.stub(gcs, "upsertBucket").throws("Unexpected upsertBucket call"); uploadObjectStub = sinon.stub(gcs, "uploadObject").throws("Unexpected uploadObject call"); createArchiveStub = sinon.stub(util, "createArchive").throws("Unexpected createArchive call"); createReadStreamStub = sinon @@ -80,15 +73,10 @@ describe("apphosting", () => { }), }; - it("creates regional GCS bucket if one doesn't exist yet", async () => { + it("upserts regional GCS bucket", async () => { const context = initializeContext(); getProjectNumberStub.resolves("000000000000"); - getBucketStub.onFirstCall().rejects( - new FirebaseError("error", { - original: new FirebaseError("original error", { status: 404 }), - }), - ); - createBucketStub.resolves(); + upsertBucketStub.resolves(); createArchiveStub.resolves({ projectSourcePath: "my-project/", zippedSourcePath: "path/to/foo-1234.zip", @@ -101,14 +89,30 @@ describe("apphosting", () => { await deploy(context, opts); - expect(createBucketStub).to.be.calledOnce; + expect(upsertBucketStub).to.be.calledWith({ + product: "apphosting", + createMessage: + "Creating Cloud Storage bucket in us-central1 to store App Hosting source code uploads at firebaseapphosting-sources-000000000000-us-central1...", + projectId: "my-project", + req: { + name: "firebaseapphosting-sources-000000000000-us-central1", + location: "us-central1", + lifecycle: { + rule: [ + { + action: { type: "Delete" }, + condition: { age: 30 }, + }, + ], + }, + }, + }); }); it("correctly creates and sets storage URIs", async () => { const context = initializeContext(); getProjectNumberStub.resolves("000000000000"); - getBucketStub.resolves(); - createBucketStub.resolves(); + upsertBucketStub.resolves(); createArchiveStub.resolves({ projectSourcePath: "my-project/", zippedSourcePath: "path/to/foo-1234.zip", @@ -121,7 +125,7 @@ describe("apphosting", () => { await deploy(context, opts); - expect(context.backendStorageUris.get("foo")).to.equal( + expect(context.backendStorageUris["foo"]).to.equal( "gs://firebaseapphosting-sources-000000000000-us-central1/foo-1234.zip", ); }); diff --git a/src/deploy/apphosting/deploy.ts b/src/deploy/apphosting/deploy.ts index b25b8a9b560..ac3b3710ef3 100644 --- a/src/deploy/apphosting/deploy.ts +++ b/src/deploy/apphosting/deploy.ts @@ -1,11 +1,11 @@ import * as fs from "fs"; import * as path from "path"; -import { FirebaseError, getErrStatus } from "../../error"; +import { FirebaseError } from "../../error"; import * as gcs from "../../gcp/storage"; import { getProjectNumber } from "../../getProjectNumber"; import { Options } from "../../options"; import { needProjectId } from "../../projectUtils"; -import { logLabeledBullet, logLabeledWarning } from "../../utils"; +import { logLabeledBullet } from "../../utils"; import { Context } from "./args"; import { createArchive } from "./util"; @@ -14,7 +14,7 @@ import { createArchive } from "./util"; * build and deployment. Creates storage buckets if necessary. */ export default async function (context: Context, options: Options): Promise { - if (context.backendConfigs.size === 0) { + if (Object.entries(context.backendConfigs).length === 0) { return; } const projectId = needProjectId(options); @@ -24,77 +24,58 @@ export default async function (context: Context, options: Options): Promise { + const bucketName = `firebaseapphosting-sources-${options.projectNumber}-${loc.toLowerCase()}`; + await gcs.upsertBucket({ + product: "apphosting", + createMessage: `Creating Cloud Storage bucket in ${loc} to store App Hosting source code uploads at ${bucketName}...`, + projectId, + req: { + name: bucketName, + location: loc, + lifecycle: { + rule: [ + { + action: { + type: "Delete", }, - ], - }, - }); - } catch (err) { - if (getErrStatus((err as FirebaseError).original) === 403) { - logLabeledWarning( - "apphosting", - "Failed to create Cloud Storage bucket because user does not have sufficient permissions. " + - "See https://cloud.google.com/storage/docs/access-control/iam-roles for more details on " + - "IAM roles that are able to create a Cloud Storage bucket, and ask your project administrator " + - "to grant you one of those roles.", - ); - throw (err as FirebaseError).original; - } - } - } else { - throw err; - } - } - } + condition: { + age: 30, + }, + }, + ], + }, + }, + }); + }), + ); - for (const cfg of context.backendConfigs.values()) { - const { projectSourcePath, zippedSourcePath } = await createArchive(cfg, options.projectRoot); - const backendLocation = context.backendLocations.get(cfg.backendId); - if (!backendLocation) { - throw new FirebaseError( - `Failed to find location for backend ${cfg.backendId}. Please contact support with the contents of your firebase-debug.log to report your issue.`, + await Promise.all( + Object.values(context.backendConfigs).map(async (cfg) => { + const { projectSourcePath, zippedSourcePath } = await createArchive(cfg, options.projectRoot); + const backendLocation = context.backendLocations[cfg.backendId]; + if (!backendLocation) { + throw new FirebaseError( + `Failed to find location for backend ${cfg.backendId}. Please contact support with the contents of your firebase-debug.log to report your issue.`, + ); + } + logLabeledBullet( + "apphosting", + `Uploading source code at ${projectSourcePath} for backend ${cfg.backendId}...`, ); - } - logLabeledBullet( - "apphosting", - `Uploading source code at ${projectSourcePath} for backend ${cfg.backendId}...`, - ); - const { bucket, object } = await gcs.uploadObject( - { - file: zippedSourcePath, - stream: fs.createReadStream(zippedSourcePath), - }, - `firebaseapphosting-sources-${options.projectNumber}-${backendLocation.toLowerCase()}`, - ); - logLabeledBullet("apphosting", `Source code uploaded at gs://${bucket}/${object}`); - context.backendStorageUris.set( - cfg.backendId, - `gs://firebaseapphosting-sources-${options.projectNumber}-${backendLocation.toLowerCase()}/${path.basename(zippedSourcePath)}`, - ); - } + const { bucket, object } = await gcs.uploadObject( + { + file: zippedSourcePath, + stream: fs.createReadStream(zippedSourcePath), + }, + `firebaseapphosting-sources-${options.projectNumber}-${backendLocation.toLowerCase()}`, + ); + logLabeledBullet("apphosting", `Source code uploaded at gs://${bucket}/${object}`); + context.backendStorageUris[cfg.backendId] = + `gs://firebaseapphosting-sources-${options.projectNumber}-${backendLocation.toLowerCase()}/${path.basename(zippedSourcePath)}`; + }), + ); } diff --git a/src/deploy/apphosting/prepare.spec.ts b/src/deploy/apphosting/prepare.spec.ts index 369f1f88640..d2750d0aa85 100644 --- a/src/deploy/apphosting/prepare.spec.ts +++ b/src/deploy/apphosting/prepare.spec.ts @@ -3,7 +3,6 @@ import * as sinon from "sinon"; import * as backend from "../../apphosting/backend"; import { Config } from "../../config"; import * as apiEnabled from "../../ensureApiEnabled"; -import { AppHostingSingle } from "../../firebaseConfig"; import * as apphosting from "../../gcp/apphosting"; import * as devconnect from "../../gcp/devConnect"; import * as prompt from "../../prompt"; @@ -26,9 +25,9 @@ const BASE_OPTS = { function initializeContext(): Context { return { - backendConfigs: new Map(), - backendLocations: new Map(), - backendStorageUris: new Map(), + backendConfigs: {}, + backendLocations: {}, + backendStorageUris: {}, }; } @@ -86,8 +85,8 @@ describe("apphosting", () => { await prepare(context, opts); - expect(context.backendLocations.get("foo")).to.equal("us-central1"); - expect(context.backendConfigs.get("foo")).to.deep.equal({ + expect(context.backendLocations["foo"]).to.equal("us-central1"); + expect(context.backendConfigs["foo"]).to.deep.equal({ backendId: "foo", rootDir: "/", ignore: [], @@ -106,8 +105,8 @@ describe("apphosting", () => { await prepare(context, opts); expect(doSetupSourceDeployStub).to.be.calledWith("my-project", "foo"); - expect(context.backendLocations.get("foo")).to.equal("us-central1"); - expect(context.backendConfigs.get("foo")).to.deep.equal({ + expect(context.backendLocations["foo"]).to.equal("us-central1"); + expect(context.backendConfigs["foo"]).to.deep.equal({ backendId: "foo", rootDir: "/", ignore: [], @@ -140,8 +139,8 @@ describe("apphosting", () => { await prepare(context, optsWithAlwaysDeploy); - expect(context.backendLocations.get("foo")).to.equal(undefined); - expect(context.backendConfigs.get("foo")).to.deep.equal(undefined); + expect(context.backendLocations["foo"]).to.be.undefined; + expect(context.backendConfigs["foo"]).to.be.undefined; }); it("prompts user if codebase is already connected and alwaysDeployFromSource is undefined", async () => { @@ -164,8 +163,8 @@ describe("apphosting", () => { await prepare(context, opts); - expect(context.backendLocations.get("foo")).to.equal("us-central1"); - expect(context.backendConfigs.get("foo")).to.deep.equal({ + expect(context.backendLocations["foo"]).to.equal("us-central1"); + expect(context.backendConfigs["foo"]).to.deep.equal({ backendId: "foo", rootDir: "/", ignore: [], diff --git a/src/deploy/apphosting/prepare.ts b/src/deploy/apphosting/prepare.ts index 01c8886049b..ccf01a58bfb 100644 --- a/src/deploy/apphosting/prepare.ts +++ b/src/deploy/apphosting/prepare.ts @@ -23,9 +23,9 @@ export default async function (context: Context, options: Options): Promise(); - context.backendLocations = new Map(); - context.backendStorageUris = new Map(); + context.backendConfigs = {}; + context.backendLocations = {}; + context.backendStorageUris = {}; const configs = getBackendConfigs(options); const { backends } = await listBackends(projectId, "-"); @@ -101,8 +101,8 @@ export default async function (context: Context, options: Options): Promise 0) { @@ -131,8 +131,8 @@ export default async function (context: Context, options: Options): Promise([ - [ - "foo", - { - backendId: "foo", - rootDir: "/", - ignore: [], - }, - ], - ]), - backendLocations: new Map([["foo", "us-central1"]]), - backendStorageUris: new Map([ - ["foo", "gs://firebaseapphosting-sources-us-central1/foo-1234.zip"], - ]), + backendConfigs: { + foo: { + backendId: "foo", + rootDir: "/", + ignore: [], + }, + }, + backendLocations: { foo: "us-central1" }, + backendStorageUris: { + foo: "gs://firebaseapphosting-sources-us-central1/foo-1234.zip", + }, }; } diff --git a/src/deploy/apphosting/release.ts b/src/deploy/apphosting/release.ts index 4a5e3f62dbf..22efa7be40c 100644 --- a/src/deploy/apphosting/release.ts +++ b/src/deploy/apphosting/release.ts @@ -16,47 +16,46 @@ import { Context } from "./args"; * Orchestrates rollouts for the backends targeted for deployment. */ export default async function (context: Context, options: Options): Promise { - if (context.backendConfigs.size === 0) { + const backendIds = Object.keys(context.backendConfigs); + + const missingBackends = backendIds.filter( + (id) => !context.backendLocations[id] || !context.backendStorageUris[id], + ); + if (missingBackends.length > 0) { + logLabeledWarning( + "apphosting", + `Failed to find metadata for backend(s) ${backendIds.join(", ")}. Please contact support with the contents of your firebase-debug.log to report your issue.`, + ); + backendIds.filter((id) => !missingBackends.includes(id)); + } + + if (backendIds.length === 0) { return; } + const projectId = needProjectId(options); - const rollouts = []; - const backendIds = []; - for (const backendId of context.backendConfigs.keys()) { - const config = context.backendConfigs.get(backendId); - const location = context.backendLocations.get(backendId); - const storageUri = context.backendStorageUris.get(backendId); - if (!config || !location || !storageUri) { - logLabeledWarning( - "apphosting", - `Failed to find metadata for backend ${backendId}. Please contact support with the contents of your firebase-debug.log to report your issue.`, - ); - continue; - } - backendIds.push(backendId); - rollouts.push( - orchestrateRollout({ - projectId, - location, - backendId, - buildInput: { - source: { - archive: { - userStorageUri: storageUri, - rootDirectory: config.rootDir, - }, + const rollouts = backendIds.map((backendId) => + orchestrateRollout({ + projectId, + backendId, + location: context.backendLocations[backendId], + buildInput: { + source: { + archive: { + userStorageUri: context.backendStorageUris[backendId], + rootDirectory: context.backendConfigs[backendId].rootDir, }, }, - }), - ); - } + }, + }), + ); logLabeledBullet( "apphosting", `You may also track the rollout(s) at:\n\t${consoleOrigin()}/project/${projectId}/apphosting`, ); const rolloutsSpinner = ora( - `Starting rollout(s) for backend(s) ${Array.from(context.backendConfigs.keys()).join(", ")}; this may take a few minutes. It's safe to exit now.\n`, + `Starting rollout(s) for backend(s) ${backendIds.join(", ")}; this may take a few minutes. It's safe to exit now.\n`, ).start(); const results = await Promise.allSettled(rollouts); for (let i = 0; i < results.length; i++) { diff --git a/src/deploy/functions/deploy.spec.ts b/src/deploy/functions/deploy.spec.ts index 4d6a585a21a..3777d4cb37f 100644 --- a/src/deploy/functions/deploy.spec.ts +++ b/src/deploy/functions/deploy.spec.ts @@ -153,6 +153,8 @@ describe("deploy", () => { functionsSourceV2: "source.zip", functionsSourceV2Hash: "source-hash", }; + const CREATE_MESSAGE = + "Creating Cloud Storage bucket in region to store Functions source code uploads at firebase-functions-src-123456..."; before(() => { experimentEnabled = experiments.isEnabled("runfunctions"); @@ -184,10 +186,15 @@ describe("deploy", () => { await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend); - expect(gcsUpsertBucketStub).to.be.calledOnceWith("project", { - name: "firebase-functions-src-123456", - location: "region", - lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + expect(gcsUpsertBucketStub).to.be.calledOnceWith({ + product: "functions", + projectId: "project", + createMessage: CREATE_MESSAGE, + req: { + name: "firebase-functions-src-123456", + location: "region", + lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + }, }); expect(createReadStreamStub).to.be.calledOnceWith("source.zip"); expect(gcsUploadStub).to.be.calledOnceWith( @@ -204,10 +211,15 @@ describe("deploy", () => { await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend); - expect(gcsUpsertBucketStub).to.be.calledOnceWith("project", { - name: "firebase-functions-src-123456", - location: "region", - lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + expect(gcsUpsertBucketStub).to.be.calledOnceWith({ + product: "functions", + projectId: "project", + createMessage: CREATE_MESSAGE, + req: { + name: "firebase-functions-src-123456", + location: "region", + lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] }, + }, }); expect(createReadStreamStub).to.be.calledOnceWith("source.zip"); expect(gcsUploadStub).to.be.calledOnceWith( diff --git a/src/deploy/functions/deploy.ts b/src/deploy/functions/deploy.ts index 9072f8d1f44..9196c689fe6 100644 --- a/src/deploy/functions/deploy.ts +++ b/src/deploy/functions/deploy.ts @@ -95,19 +95,23 @@ export async function uploadSourceV2( // New behavior: BYO bucket since we're moving away from the GCF API. // Using project number to ensure we don't exceed the bucket name length limit (in addition to PII controversy). const bucketName = `firebase-functions-src-${projectNumber}`; - const bucketOptions: gcs.CreateBucketRequest = { - name: bucketName, - location: region, - lifecycle: { - rule: [ - { - action: { type: "Delete" }, - condition: { age: 1 }, // Delete objects after 1 day - }, - ], + await gcs.upsertBucket({ + product: "functions", + projectId, + createMessage: `Creating Cloud Storage bucket in ${region} to store Functions source code uploads at ${bucketName}...`, + req: { + name: bucketName, + location: region, + lifecycle: { + rule: [ + { + action: { type: "Delete" }, + condition: { age: 1 }, // Delete objects after 1 day + }, + ], + }, }, - }; - await gcs.upsertBucket(projectId, bucketOptions); + }); await gcs.upload( uploadOpts, `${bucketName}/${source.functionsSourceV2Hash}.zip`, diff --git a/src/gcp/storage.spec.ts b/src/gcp/storage.spec.ts new file mode 100644 index 00000000000..47e4a83fbfb --- /dev/null +++ b/src/gcp/storage.spec.ts @@ -0,0 +1,132 @@ +import { expect } from "chai"; +import * as sinon from "sinon"; + +import * as storage from "./storage"; +import * as utils from "../utils"; +import { FirebaseError } from "../error"; + +describe("storage", () => { + describe("upsertBucket", () => { + let getBucketStub: sinon.SinonStub; + let createBucketStub: sinon.SinonStub; + let logLabeledBulletStub: sinon.SinonStub; + let logLabeledWarningStub: sinon.SinonStub; + + beforeEach(() => { + getBucketStub = sinon.stub(storage, "getBucket"); + createBucketStub = sinon.stub(storage, "createBucket"); + logLabeledBulletStub = sinon.stub(utils, "logLabeledBullet"); + logLabeledWarningStub = sinon.stub(utils, "logLabeledWarning"); + }); + + afterEach(() => { + sinon.restore(); + }); + + it("should not call createBucket if the bucket already exists", async () => { + getBucketStub.resolves(); + + await storage.upsertBucket({ + product: "test", + createMessage: "Creating bucket", + projectId: "test-project", + req: { name: "test-bucket", location: "us-central1", lifecycle: { rule: [] } }, + }); + + expect(getBucketStub).to.be.calledOnceWith("test-bucket"); + expect(createBucketStub).to.not.be.called; + expect(logLabeledBulletStub).to.not.be.called; + }); + + it("should call createBucket if the bucket does not exist (404)", async () => { + const error = new FirebaseError("Not found", { original: { status: 404 } as any }); + getBucketStub.rejects(error); + createBucketStub.resolves(); + + await storage.upsertBucket({ + product: "test", + createMessage: "Creating bucket", + projectId: "test-project", + req: { name: "test-bucket", location: "us-central1", lifecycle: { rule: [] } }, + }); + + expect(getBucketStub).to.be.calledOnceWith("test-bucket"); + expect(createBucketStub).to.be.calledOnceWith( + "test-project", + { + name: "test-bucket", + location: "us-central1", + lifecycle: { rule: [] }, + }, + true, + ); + expect(logLabeledBulletStub).to.be.calledOnceWith("test", "Creating bucket"); + }); + + it("should call createBucket if the bucket does not exist (403)", async () => { + const error = new FirebaseError("Unauthenticated", { original: { status: 403 } as any }); + getBucketStub.rejects(error); + createBucketStub.resolves(); + + await storage.upsertBucket({ + product: "test", + createMessage: "Creating bucket", + projectId: "test-project", + req: { name: "test-bucket", location: "us-central1", lifecycle: { rule: [] } }, + }); + + expect(getBucketStub).to.be.calledOnceWith("test-bucket"); + expect(createBucketStub).to.be.calledOnceWith( + "test-project", + { + name: "test-bucket", + location: "us-central1", + lifecycle: { rule: [] }, + }, + true, + ); + expect(logLabeledBulletStub).to.be.calledOnceWith("test", "Creating bucket"); + }); + + it("should explain IAM errors", async () => { + const notFound = new FirebaseError("Bucket not found", { original: { status: 404 } as any }); + const permissionDenied = new FirebaseError("Permission denied", { + original: { status: 403 } as any, + }); + getBucketStub.rejects(notFound); + createBucketStub.rejects(permissionDenied); + + await expect( + storage.upsertBucket({ + product: "test", + createMessage: "Creating bucket", + projectId: "test-project", + req: { name: "test-bucket", location: "us-central1", lifecycle: { rule: [] } }, + }), + ).to.be.rejected; + + expect(logLabeledWarningStub).to.be.calledWithMatch( + "test", + /Failed to create Cloud Storage bucket because user does not have sufficient permissions/, + ); + }); + + it("should forward unexpected errors", async () => { + const error = new FirebaseError("Unexpected error", { original: { status: 500 } as any }); + getBucketStub.rejects(error); + + await expect( + storage.upsertBucket({ + product: "test", + createMessage: "Creating bucket", + projectId: "test-project", + req: { name: "test-bucket", location: "us-central1", lifecycle: { rule: [] } }, + }), + ).to.be.rejectedWith("Unexpected error"); + + expect(getBucketStub).to.be.calledOnceWith("test-bucket"); + expect(createBucketStub).to.not.be.called; + expect(logLabeledBulletStub).to.not.be.called; + }); + }); +}); diff --git a/src/gcp/storage.ts b/src/gcp/storage.ts index cd38019d900..73fd510dae0 100644 --- a/src/gcp/storage.ts +++ b/src/gcp/storage.ts @@ -4,9 +4,10 @@ import * as clc from "colorette"; import { firebaseStorageOrigin, storageOrigin } from "../api"; import { Client } from "../apiv2"; -import { FirebaseError } from "../error"; +import { FirebaseError, getErrStatus } from "../error"; import { logger } from "../logger"; import { ensure } from "../ensureApiEnabled"; +import * as utils from "../utils"; /** Bucket Interface */ interface BucketResponse { @@ -360,18 +361,41 @@ export async function createBucket( /** * Creates a storage bucket on GCP if it does not already exist. - * @param {string} projectId - * @param {CreateBucketRequest} req */ -export async function upsertBucket(projectId: string, req: CreateBucketRequest): Promise { +export async function upsertBucket(opts: { + product: string; + createMessage: string; + projectId: string; + req: CreateBucketRequest; +}): Promise { try { - await getBucket(req.name); + await (exports as { getBucket: typeof getBucket }).getBucket(opts.req.name); return; - } catch (err: any) { - // If the bucket doesn't exist, create it. - if (err.original?.status === 404) { - await createBucket(projectId, req, true /* projectPrivate */); - return; + } catch (err) { + const errStatus = getErrStatus((err as FirebaseError).original); + // Unfortunately, requests for a non-existent bucket from the GCS API sometimes return 403 responses as well as 404s. + // We must attempt to create a new bucket on both 403s and 404s. + if (errStatus !== 403 && errStatus !== 404) { + throw err; + } + } + + utils.logLabeledBullet(opts.product, opts.createMessage); + try { + await (exports as { createBucket: typeof createBucket }).createBucket( + opts.projectId, + opts.req, + true /* projectPrivate */, + ); + } catch (err) { + if (getErrStatus((err as FirebaseError).original) === 403) { + utils.logLabeledWarning( + opts.product, + "Failed to create Cloud Storage bucket because user does not have sufficient permissions. " + + "See https://cloud.google.com/storage/docs/access-control/iam-roles for more details on " + + "IAM roles that are able to create a Cloud Storage bucket, and ask your project administrator " + + "to grant you one of those roles.", + ); } throw err; } From 12031db4d2c36899d12a7e5091a334fe55c05d53 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Wed, 20 Aug 2025 15:36:30 -0700 Subject: [PATCH 4/5] PR feedback --- src/deploy/apphosting/deploy.ts | 2 -- src/deploy/apphosting/release.ts | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/deploy/apphosting/deploy.ts b/src/deploy/apphosting/deploy.ts index ac3b3710ef3..de26ae3c2b4 100644 --- a/src/deploy/apphosting/deploy.ts +++ b/src/deploy/apphosting/deploy.ts @@ -24,8 +24,6 @@ export default async function (context: Context, options: Options): Promise { const bucketName = `firebaseapphosting-sources-${options.projectNumber}-${loc.toLowerCase()}`; diff --git a/src/deploy/apphosting/release.ts b/src/deploy/apphosting/release.ts index 22efa7be40c..3c2dce12da9 100644 --- a/src/deploy/apphosting/release.ts +++ b/src/deploy/apphosting/release.ts @@ -16,7 +16,7 @@ import { Context } from "./args"; * Orchestrates rollouts for the backends targeted for deployment. */ export default async function (context: Context, options: Options): Promise { - const backendIds = Object.keys(context.backendConfigs); + let backendIds = Object.keys(context.backendConfigs); const missingBackends = backendIds.filter( (id) => !context.backendLocations[id] || !context.backendStorageUris[id], @@ -26,7 +26,7 @@ export default async function (context: Context, options: Options): Promise !missingBackends.includes(id)); + backendIds = backendIds.filter((id) => !missingBackends.includes(id)); } if (backendIds.length === 0) { From d2879612f969e5573c13a506e7c2dde846f70523 Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Tue, 26 Aug 2025 12:05:42 -0700 Subject: [PATCH 5/5] PR feedback --- src/deploy/apphosting/deploy.ts | 5 +++-- src/gcp/storage.ts | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/deploy/apphosting/deploy.ts b/src/deploy/apphosting/deploy.ts index de26ae3c2b4..b89a1dd6880 100644 --- a/src/deploy/apphosting/deploy.ts +++ b/src/deploy/apphosting/deploy.ts @@ -64,16 +64,17 @@ export default async function (context: Context, options: Options): Promise { const url = new URL(uploadUrl, storageOrigin()); - const signedUrl = url.searchParams.has("GoogleAccessId"); - const localAPIClient = new Client({ urlPrefix: url.origin, auth: !signedUrl }); + const isSignedUrl = url.searchParams.has("GoogleAccessId"); + const localAPIClient = new Client({ urlPrefix: url.origin, auth: !isSignedUrl }); const res = await localAPIClient.request({ method: "PUT", path: url.pathname,