From da0a79f384aa7df48fe47e7770535cd047a27e53 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 13 Mar 2025 11:03:09 +0100 Subject: [PATCH 1/6] Update packageName to delete artifacts during function cleanup --- src/deploy/functions/containerCleaner.spec.ts | 71 ++++++++++++++----- src/deploy/functions/containerCleaner.ts | 26 ++++--- 2 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/deploy/functions/containerCleaner.spec.ts b/src/deploy/functions/containerCleaner.spec.ts index a5d4b3c79ce..ea24a92543f 100644 --- a/src/deploy/functions/containerCleaner.spec.ts +++ b/src/deploy/functions/containerCleaner.spec.ts @@ -190,6 +190,56 @@ describe("ArtifactRegistryCleaner", () => { sinon.verifyAndRestore(); }); + describe("packagePath", () => { + it("encodes project IDs with dashes", () => { + const func = { + id: "function", + region: "region", + project: "my-cool-project", + }; + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/my-cool-project/locations/region/repositories/gcf-artifacts/packages/my--cool--project__region__function", + ); + }); + + it("encodes project IDs with underscores", () => { + const func = { + id: "function", + region: "region", + project: "my_cool_project", + }; + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/my_cool_project/locations/region/repositories/gcf-artifacts/packages/my__cool__project__region__function", + ); + }); + + it("encodes regions with dashes", () => { + const func = { + id: "function", + region: "us-central1", + project: "project", + }; + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/project/locations/us-central1/repositories/gcf-artifacts/packages/project__us--central1__function", + ); + }); + + it("encodes function IDs with capital letters", () => { + const func = { + id: "Strange-Casing_cases", + region: "region", + project: "project", + }; + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/project/locations/region/repositories/gcf-artifacts/packages/project__region__s-strange--_casing__cases", + ); + }); + }); + it("deletes artifacts", async () => { const cleaner = new containerCleaner.ArtifactRegistryCleaner(); const func = { @@ -202,7 +252,7 @@ describe("ArtifactRegistryCleaner", () => { await cleaner.cleanupFunction(func); expect(ar.deletePackage).to.have.been.calledWith( - "projects/project/locations/region/repositories/gcf-artifacts/packages/function", + "projects/project/locations/region/repositories/gcf-artifacts/packages/project__region__function", ); expect(poll.pollOperation).to.have.been.called; }); @@ -219,24 +269,7 @@ describe("ArtifactRegistryCleaner", () => { await cleaner.cleanupFunction(func); expect(ar.deletePackage).to.have.been.calledWith( - "projects/project/locations/region/repositories/gcf-artifacts/packages/function", - ); - expect(poll.pollOperation).to.not.have.been.called; - }); - - it("encodeds to avoid upper-case letters", async () => { - const cleaner = new containerCleaner.ArtifactRegistryCleaner(); - const func = { - id: "Strange-Casing_cases", - region: "region", - project: "project", - }; - - ar.deletePackage.returns(Promise.resolve({ name: "op", done: true })); - - await cleaner.cleanupFunction(func); - expect(ar.deletePackage).to.have.been.calledWith( - "projects/project/locations/region/repositories/gcf-artifacts/packages/s-strange--_casing__cases", + "projects/project/locations/region/repositories/gcf-artifacts/packages/project__region__function", ); expect(poll.pollOperation).to.not.have.been.called; }); diff --git a/src/deploy/functions/containerCleaner.ts b/src/deploy/functions/containerCleaner.ts index 7ab2207d931..179fb31347e 100644 --- a/src/deploy/functions/containerCleaner.ts +++ b/src/deploy/functions/containerCleaner.ts @@ -101,17 +101,25 @@ export async function cleanupBuildImages( // requests through a ThrottlerQueue. export class ArtifactRegistryCleaner { static packagePath(func: backend.TargetIds): string { - // GCFv1 names can include upper-case letters, but docker images cannot. - // to fix this, the artifact registry path for these images uses a custom encoding scheme. - // * Underscores are doubled - // * Dashes are doubled + // GCF V2 artifact names follow a specific encoding scheme: + // Each part (project, region, functionId) is encoded separately with these rules: + // * Underscores are doubled ("_" -> "__") + // * Dashes are doubled ("-" -> "--") // * A leading capital letter is replaced with // * Other capital letters are replaced with - const encodedId = func.id - .replace(/_/g, "__") - .replace(/-/g, "--") - .replace(/^[A-Z]/, (first) => `${first.toLowerCase()}-${first.toLowerCase()}`) - .replace(/[A-Z]/g, (upper) => `_${upper.toLowerCase()}`); + // Then the parts are joined with double underscores + const encodePart = (part: string): string => { + return part + .replace(/_/g, "__") + .replace(/-/g, "--") + .replace(/^[A-Z]/, (first) => `${first.toLowerCase()}-${first.toLowerCase()}`) + .replace(/[A-Z]/g, (upper) => `_${upper.toLowerCase()}`); + }; + + const encodedId = [encodePart(func.project), encodePart(func.region), encodePart(func.id)].join( + "__", + ); + return `projects/${func.project}/locations/${func.region}/repositories/gcf-artifacts/packages/${encodedId}`; } From a99ade2810ef1a3da10ff8b7fc7685b1f621932f Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 13 Mar 2025 11:08:56 +0100 Subject: [PATCH 2/6] Add example to packagePath --- src/deploy/functions/containerCleaner.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/deploy/functions/containerCleaner.ts b/src/deploy/functions/containerCleaner.ts index 179fb31347e..d88f95fbc52 100644 --- a/src/deploy/functions/containerCleaner.ts +++ b/src/deploy/functions/containerCleaner.ts @@ -108,6 +108,11 @@ export class ArtifactRegistryCleaner { // * A leading capital letter is replaced with // * Other capital letters are replaced with // Then the parts are joined with double underscores + // Example: + // - project "my-cool-project" -> "my--cool--project" + // - region "us-central1" -> "us--central1" + // - functionId "myFunction" -> "my_function" + // Final result: "my--cool--project__us--central1__my_function" const encodePart = (part: string): string => { return part .replace(/_/g, "__") From 65a8cff089a0f3ddecd69a1135eee289292ed4ba Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 13 Mar 2025 11:33:13 +0100 Subject: [PATCH 3/6] Add artifact name schema --- src/deploy/functions/containerCleaner.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/deploy/functions/containerCleaner.ts b/src/deploy/functions/containerCleaner.ts index d88f95fbc52..38da3f6ca80 100644 --- a/src/deploy/functions/containerCleaner.ts +++ b/src/deploy/functions/containerCleaner.ts @@ -101,8 +101,10 @@ export async function cleanupBuildImages( // requests through a ThrottlerQueue. export class ArtifactRegistryCleaner { static packagePath(func: backend.TargetIds): string { - // GCF V2 artifact names follow a specific encoding scheme: - // Each part (project, region, functionId) is encoded separately with these rules: + // GCF V2 artifact names follow this schema: + // {encoded_project}__{encoded_region}__{encoded_function} + // + // Each part is encoded separately with these rules: // * Underscores are doubled ("_" -> "__") // * Dashes are doubled ("-" -> "--") // * A leading capital letter is replaced with From d21284448390dd8904cbc2d27590ee5ce73493e0 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 13 Mar 2025 12:08:30 +0100 Subject: [PATCH 4/6] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..4983a676240 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- - Fix GCF V2 artifact cleanup by correctly encoding artifact names to match GCF V2's format (#8318) From c2553bea7faf29b40d3d83acc3643d84625e9be8 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 14 Mar 2025 15:31:10 +0100 Subject: [PATCH 5/6] Update packageName to handle v1 and v2 deployments --- src/deploy/functions/containerCleaner.spec.ts | 112 +++++++++++++----- src/deploy/functions/containerCleaner.ts | 73 +++++++----- 2 files changed, 128 insertions(+), 57 deletions(-) diff --git a/src/deploy/functions/containerCleaner.spec.ts b/src/deploy/functions/containerCleaner.spec.ts index ea24a92543f..a8ca56a3417 100644 --- a/src/deploy/functions/containerCleaner.spec.ts +++ b/src/deploy/functions/containerCleaner.spec.ts @@ -9,18 +9,31 @@ import * as docker from "../../gcp/docker"; import * as poller from "../../operation-poller"; import * as utils from "../../utils"; +function createTestEndpoint(overrides: Partial = {}): backend.Endpoint { + return { + id: "function", + region: "us-central1", + project: "project", + platform: "gcfv2", + entryPoint: "function", + runtime: "nodejs16", + httpsTrigger: {}, + ...overrides, + }; +} + describe("CleanupBuildImages", () => { - let gcr: sinon.SinonStubbedInstance; + let gcr: containerCleaner.ContainerRegistryCleaner; let ar: sinon.SinonStubbedInstance; let logLabeledWarning: sinon.SinonStub; - const TARGET: backend.TargetIds = { - project: "project", - region: "us-central1", - id: "id", - }; + let dockerHelper: sinon.SinonStubbedInstance; + const TARGET = createTestEndpoint(); beforeEach(() => { - gcr = sinon.createStubInstance(containerCleaner.ContainerRegistryCleaner); + dockerHelper = sinon.createStubInstance(containerCleaner.DockerHelper); + gcr = new containerCleaner.ContainerRegistryCleaner(); + // Inject the stubbed DockerHelper + (gcr as any).helpers = { us: dockerHelper }; ar = sinon.createStubInstance(containerCleaner.ArtifactRegistryCleaner); logLabeledWarning = sinon.stub(utils, "logLabeledWarning"); }); @@ -30,13 +43,24 @@ describe("CleanupBuildImages", () => { }); it("uses GCR and AR", async () => { - await containerCleaner.cleanupBuildImages([TARGET], [], { gcr, ar } as any); - expect(gcr.cleanupFunction).to.have.been.called; + dockerHelper.ls.withArgs("project/gcf/us-central1").resolves({ + children: ["uuid"], + digests: [], + tags: [], + }); + dockerHelper.ls.withArgs("project/gcf/us-central1/uuid").resolves({ + children: [], + digests: ["sha256:func-hash"], + tags: ["function_version-1"], + }); + + await containerCleaner.cleanupBuildImages([TARGET], [], { gcr, ar }); + expect(dockerHelper.rm).to.have.been.calledWith("project/gcf/us-central1/uuid"); }); it("reports failed domains from AR", async () => { ar.cleanupFunction.rejects(new Error("uh oh")); - await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar } as any); + await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar }); expect(logLabeledWarning).to.have.been.calledWithMatch( "functions", new RegExp( @@ -46,8 +70,8 @@ describe("CleanupBuildImages", () => { }); it("reports failed domains from GCR", async () => { - gcr.cleanupFunction.rejects(new Error("uh oh")); - await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar } as any); + dockerHelper.ls.withArgs("project/gcf/us-central1").rejects(new Error("uh oh")); + await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar }); expect(logLabeledWarning).to.have.been.calledWithMatch( "functions", new RegExp("https://console.cloud.google.com/gcr/images/project/us/gcf"), @@ -191,48 +215,78 @@ describe("ArtifactRegistryCleaner", () => { }); describe("packagePath", () => { - it("encodes project IDs with dashes", () => { - const func = { + it("uses V1 encoding for gcfv1 functions", () => { + const func = createTestEndpoint({ + id: "helloWorldV1", + region: "us-central1", + project: "my-project", + platform: "gcfv1", + }); + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/my-project/locations/us-central1/repositories/gcf-artifacts/packages/hello_world_v1", + ); + }); + + it("uses V2 encoding for gcfv2 functions", () => { + const func = createTestEndpoint({ + id: "helloWorldV2", + region: "us-central1", + project: "my-project", + platform: "gcfv2", + }); + + expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( + "projects/my-project/locations/us-central1/repositories/gcf-artifacts/packages/my--project__us--central1__hello_world_v2", + ); + }); + + it("encodes V2 project IDs with dashes", () => { + const func = createTestEndpoint({ id: "function", region: "region", project: "my-cool-project", - }; + platform: "gcfv2", + }); expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( "projects/my-cool-project/locations/region/repositories/gcf-artifacts/packages/my--cool--project__region__function", ); }); - it("encodes project IDs with underscores", () => { - const func = { + it("encodes V2 project IDs with underscores", () => { + const func = createTestEndpoint({ id: "function", region: "region", project: "my_cool_project", - }; + platform: "gcfv2", + }); expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( "projects/my_cool_project/locations/region/repositories/gcf-artifacts/packages/my__cool__project__region__function", ); }); - it("encodes regions with dashes", () => { - const func = { + it("encodes V2 regions with dashes", () => { + const func = createTestEndpoint({ id: "function", region: "us-central1", project: "project", - }; + platform: "gcfv2", + }); expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( "projects/project/locations/us-central1/repositories/gcf-artifacts/packages/project__us--central1__function", ); }); - it("encodes function IDs with capital letters", () => { - const func = { + it("encodes V2 function IDs with capital letters", () => { + const func = createTestEndpoint({ id: "Strange-Casing_cases", region: "region", project: "project", - }; + platform: "gcfv2", + }); expect(containerCleaner.ArtifactRegistryCleaner.packagePath(func)).to.equal( "projects/project/locations/region/repositories/gcf-artifacts/packages/project__region__s-strange--_casing__cases", @@ -242,11 +296,12 @@ describe("ArtifactRegistryCleaner", () => { it("deletes artifacts", async () => { const cleaner = new containerCleaner.ArtifactRegistryCleaner(); - const func = { + const func = createTestEndpoint({ id: "function", region: "region", project: "project", - }; + platform: "gcfv2", + }); ar.deletePackage.returns(Promise.resolve({ name: "op" } as any)); @@ -259,11 +314,12 @@ describe("ArtifactRegistryCleaner", () => { it("bypasses poller if the operation is completed", async () => { const cleaner = new containerCleaner.ArtifactRegistryCleaner(); - const func = { + const func = createTestEndpoint({ id: "function", region: "region", project: "project", - }; + platform: "gcfv2", + }); ar.deletePackage.returns(Promise.resolve({ name: "op", done: true })); diff --git a/src/deploy/functions/containerCleaner.ts b/src/deploy/functions/containerCleaner.ts index 38da3f6ca80..27045acbb9d 100644 --- a/src/deploy/functions/containerCleaner.ts +++ b/src/deploy/functions/containerCleaner.ts @@ -39,8 +39,8 @@ async function retry(func: () => Promise): Promise { } export async function cleanupBuildImages( - haveFunctions: backend.TargetIds[], - deletedFunctions: backend.TargetIds[], + haveFunctions: backend.Endpoint[], + deletedFunctions: backend.Endpoint[], cleaners: { gcr?: ContainerRegistryCleaner; ar?: ArtifactRegistryCleaner } = {}, ): Promise { utils.logBullet(clc.bold(clc.cyan("functions: ")) + "cleaning up build files..."); @@ -100,32 +100,11 @@ export async function cleanupBuildImages( // than the raw Docker API. If there are reports of any quota issues we may have to run these // requests through a ThrottlerQueue. export class ArtifactRegistryCleaner { - static packagePath(func: backend.TargetIds): string { - // GCF V2 artifact names follow this schema: - // {encoded_project}__{encoded_region}__{encoded_function} - // - // Each part is encoded separately with these rules: - // * Underscores are doubled ("_" -> "__") - // * Dashes are doubled ("-" -> "--") - // * A leading capital letter is replaced with - // * Other capital letters are replaced with - // Then the parts are joined with double underscores - // Example: - // - project "my-cool-project" -> "my--cool--project" - // - region "us-central1" -> "us--central1" - // - functionId "myFunction" -> "my_function" - // Final result: "my--cool--project__us--central1__my_function" - const encodePart = (part: string): string => { - return part - .replace(/_/g, "__") - .replace(/-/g, "--") - .replace(/^[A-Z]/, (first) => `${first.toLowerCase()}-${first.toLowerCase()}`) - .replace(/[A-Z]/g, (upper) => `_${upper.toLowerCase()}`); - }; - - const encodedId = [encodePart(func.project), encodePart(func.region), encodePart(func.id)].join( - "__", - ); + static packagePath(func: backend.Endpoint): string { + const encodedId = + func.platform === "gcfv2" + ? ArtifactRegistryCleaner.encodePackageNameV2(func) + : ArtifactRegistryCleaner.encodePackageNameV1(func); return `projects/${func.project}/locations/${func.region}/repositories/gcf-artifacts/packages/${encodedId}`; } @@ -136,6 +115,42 @@ export class ArtifactRegistryCleaner { masterTimeout: 5 * 60 * 1_000, }; + private static encodePart(part: string): string { + return part + .replace(/_/g, "__") + .replace(/-/g, "--") + .replace(/^[A-Z]/, (first) => `${first.toLowerCase()}-${first.toLowerCase()}`) + .replace(/[A-Z]/g, (upper) => `_${upper.toLowerCase()}`); + } + + // GCF V1: Simple underscore concatenation + // Example: "helloWorldV1" -> "hello_world_v1" + private static encodePackageNameV1(func: backend.TargetIds): string { + return ArtifactRegistryCleaner.encodePart(func.id); + } + + // GCF V2 artifact names follow this schema: + // {encoded_project}__{encoded_region}__{encoded_function} + // + // Each part is encoded separately with these rules: + // * Underscores are doubled ("_" -> "__") + // * Dashes are doubled ("-" -> "--") + // * A leading capital letter is replaced with + // * Other capital letters are replaced with + // Then the parts are joined with double underscores + // Example: + // - project "my-cool-project" -> "my--cool--project" + // - region "us-central1" -> "us--central1" + // - functionId "myFunction" -> "my_function" + // Final result: "my--cool--project__us--central1__my_function" + private static encodePackageNameV2(func: backend.TargetIds): string { + return [ + ArtifactRegistryCleaner.encodePart(func.project), + ArtifactRegistryCleaner.encodePart(func.region), + ArtifactRegistryCleaner.encodePart(func.id), + ].join("__"); + } + // GCFv1 for AR has the following directory structure // Hostname: -docker.pkg.dev // Directory structure: @@ -145,7 +160,7 @@ export class ArtifactRegistryCleaner { // We leave the cache directory alone because it only costs // a few MB and improves performance. We only delete the cache if // the function was deleted in its entirety. - async cleanupFunction(func: backend.TargetIds): Promise { + async cleanupFunction(func: backend.Endpoint): Promise { let op: artifactregistry.Operation; try { op = await artifactregistry.deletePackage(ArtifactRegistryCleaner.packagePath(func)); From 8051045c9f78c115748f85bb056e78fd45d10874 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 14 Mar 2025 15:37:38 +0100 Subject: [PATCH 6/6] Update tests --- src/deploy/functions/containerCleaner.spec.ts | 33 +++++-------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/src/deploy/functions/containerCleaner.spec.ts b/src/deploy/functions/containerCleaner.spec.ts index a8ca56a3417..043847a8256 100644 --- a/src/deploy/functions/containerCleaner.spec.ts +++ b/src/deploy/functions/containerCleaner.spec.ts @@ -11,7 +11,7 @@ import * as utils from "../../utils"; function createTestEndpoint(overrides: Partial = {}): backend.Endpoint { return { - id: "function", + id: "id", region: "us-central1", project: "project", platform: "gcfv2", @@ -23,17 +23,13 @@ function createTestEndpoint(overrides: Partial = {}): backend. } describe("CleanupBuildImages", () => { - let gcr: containerCleaner.ContainerRegistryCleaner; + let gcr: sinon.SinonStubbedInstance; let ar: sinon.SinonStubbedInstance; let logLabeledWarning: sinon.SinonStub; - let dockerHelper: sinon.SinonStubbedInstance; - const TARGET = createTestEndpoint(); + const TARGET: backend.Endpoint = createTestEndpoint(); beforeEach(() => { - dockerHelper = sinon.createStubInstance(containerCleaner.DockerHelper); - gcr = new containerCleaner.ContainerRegistryCleaner(); - // Inject the stubbed DockerHelper - (gcr as any).helpers = { us: dockerHelper }; + gcr = sinon.createStubInstance(containerCleaner.ContainerRegistryCleaner); ar = sinon.createStubInstance(containerCleaner.ArtifactRegistryCleaner); logLabeledWarning = sinon.stub(utils, "logLabeledWarning"); }); @@ -43,24 +39,13 @@ describe("CleanupBuildImages", () => { }); it("uses GCR and AR", async () => { - dockerHelper.ls.withArgs("project/gcf/us-central1").resolves({ - children: ["uuid"], - digests: [], - tags: [], - }); - dockerHelper.ls.withArgs("project/gcf/us-central1/uuid").resolves({ - children: [], - digests: ["sha256:func-hash"], - tags: ["function_version-1"], - }); - - await containerCleaner.cleanupBuildImages([TARGET], [], { gcr, ar }); - expect(dockerHelper.rm).to.have.been.calledWith("project/gcf/us-central1/uuid"); + await containerCleaner.cleanupBuildImages([TARGET], [], { gcr, ar } as any); + expect(gcr.cleanupFunction).to.have.been.called; }); it("reports failed domains from AR", async () => { ar.cleanupFunction.rejects(new Error("uh oh")); - await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar }); + await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar } as any); expect(logLabeledWarning).to.have.been.calledWithMatch( "functions", new RegExp( @@ -70,8 +55,8 @@ describe("CleanupBuildImages", () => { }); it("reports failed domains from GCR", async () => { - dockerHelper.ls.withArgs("project/gcf/us-central1").rejects(new Error("uh oh")); - await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar }); + gcr.cleanupFunction.rejects(new Error("uh oh")); + await containerCleaner.cleanupBuildImages([], [TARGET], { gcr, ar } as any); expect(logLabeledWarning).to.have.been.calledWithMatch( "functions", new RegExp("https://console.cloud.google.com/gcr/images/project/us/gcf"),