From cd07aae84fcf51ae0fcfd6841e61aeef43197e51 Mon Sep 17 00:00:00 2001 From: nicktrn <55853254+nicktrn@users.noreply.github.com> Date: Wed, 3 Sep 2025 18:40:54 +0100 Subject: [PATCH 1/3] fix(webapp): prevent duplicate preview env image tags --- .../app/v3/getDeploymentImageRef.server.ts | 12 +- .../services/initializeDeployment.server.ts | 2 +- .../webapp/test/getDeploymentImageRef.test.ts | 168 ++++++++++++------ 3 files changed, 122 insertions(+), 60 deletions(-) diff --git a/apps/webapp/app/v3/getDeploymentImageRef.server.ts b/apps/webapp/app/v3/getDeploymentImageRef.server.ts index e01f340a26..2d88deb9de 100644 --- a/apps/webapp/app/v3/getDeploymentImageRef.server.ts +++ b/apps/webapp/app/v3/getDeploymentImageRef.server.ts @@ -11,6 +11,10 @@ import { STSClient, AssumeRoleCommand } from "@aws-sdk/client-sts"; import { tryCatch } from "@trigger.dev/core"; import { logger } from "~/services/logger.server"; import { type RegistryConfig } from "./registryConfig.server"; +import type { EnvironmentType } from "@trigger.dev/core/v3"; +import { customAlphabet } from "nanoid"; + +const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); // Optional configuration for cross-account access export type AssumeRoleConfig = { @@ -101,19 +105,21 @@ export async function getDeploymentImageRef({ registry, projectRef, nextVersion, - environmentSlug, + environmentType, }: { registry: RegistryConfig; projectRef: string; nextVersion: string; - environmentSlug: string; + environmentType: EnvironmentType; }): Promise<{ imageRef: string; isEcr: boolean; repoCreated: boolean; }> { const repositoryName = `${registry.namespace}/${projectRef}`; - const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${environmentSlug}`; + const envType = environmentType.toLowerCase(); + const randomSuffix = nanoid(); + const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${envType}.${randomSuffix}`; if (!isEcrRegistry(registry.host)) { return { diff --git a/apps/webapp/app/v3/services/initializeDeployment.server.ts b/apps/webapp/app/v3/services/initializeDeployment.server.ts index cdd174ed92..ad354e1c15 100644 --- a/apps/webapp/app/v3/services/initializeDeployment.server.ts +++ b/apps/webapp/app/v3/services/initializeDeployment.server.ts @@ -78,7 +78,7 @@ export class InitializeDeploymentService extends BaseService { registry: registryConfig, projectRef: environment.project.externalRef, nextVersion, - environmentSlug: environment.slug, + environmentType: environment.type, }) ); diff --git a/apps/webapp/test/getDeploymentImageRef.test.ts b/apps/webapp/test/getDeploymentImageRef.test.ts index 2810c14435..2d566bcb49 100644 --- a/apps/webapp/test/getDeploymentImageRef.test.ts +++ b/apps/webapp/test/getDeploymentImageRef.test.ts @@ -8,7 +8,9 @@ import { } from "../app/v3/getDeploymentImageRef.server"; import { DeleteRepositoryCommand } from "@aws-sdk/client-ecr"; -describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", () => { +const escapeHostForRegex = (host: string) => host.replace(/\./g, "\\."); + +describe("getDeploymentImageRef", () => { const testHost = process.env.DEPLOY_REGISTRY_HOST || "123456789012.dkr.ecr.us-east-1.amazonaws.com"; const testNamespace = process.env.DEPLOY_REGISTRY_NAMESPACE || "test-namespace"; @@ -25,7 +27,7 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", // Clean up test repository after tests afterAll(async () => { - if (process.env.KEEP_TEST_REPO === "1") { + if (process.env.KEEP_TEST_REPO === "1" || process.env.RUN_ECR_TESTS !== "1") { return; } @@ -57,7 +59,7 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", it("should return the correct image ref for non-ECR registry", async () => { const imageRef = await getDeploymentImageRef({ registry: { - host: "registry.digitalocean.com", + host: "registry.example.com", namespace: testNamespace, username: "test-user", password: "test-pass", @@ -67,17 +69,78 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", }, projectRef: testProjectRef, nextVersion: "20250630.1", - environmentSlug: "test", + environmentType: "DEVELOPMENT", }); - expect(imageRef.imageRef).toBe( - `registry.digitalocean.com/${testNamespace}/${testProjectRef}:20250630.1.test` + // Check the image ref structure and that it contains expected parts + expect(imageRef.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + "registry.example.com" + )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.[a-z0-9]{8}$` + ) ); expect(imageRef.isEcr).toBe(false); }); - it("should create ECR repository and return correct image ref", async () => { - const imageRef1 = await getDeploymentImageRef({ + it.skipIf(process.env.RUN_ECR_TESTS !== "1")( + "should create ECR repository and return correct image ref", + async () => { + const imageRef1 = await getDeploymentImageRef({ + registry: { + host: testHost, + namespace: testNamespace, + username: "test-user", + password: "test-pass", + ecrTags: registryTags, + ecrAssumeRoleArn: roleArn, + ecrAssumeRoleExternalId: externalId, + }, + projectRef: testProjectRef2, + nextVersion: "20250630.1", + environmentType: "DEVELOPMENT", + }); + + expect(imageRef1.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + testHost + )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.[a-z0-9]{8}$` + ) + ); + expect(imageRef1.isEcr).toBe(true); + expect(imageRef1.repoCreated).toBe(true); + + const imageRef2 = await getDeploymentImageRef({ + registry: { + host: testHost, + namespace: testNamespace, + username: "test-user", + password: "test-pass", + ecrTags: registryTags, + ecrAssumeRoleArn: roleArn, + ecrAssumeRoleExternalId: externalId, + }, + projectRef: testProjectRef2, + nextVersion: "20250630.2", + environmentType: "DEVELOPMENT", + }); + + expect(imageRef2.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + testHost + )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.[a-z0-9]{8}$` + ) + ); + expect(imageRef2.isEcr).toBe(true); + expect(imageRef2.repoCreated).toBe(false); + } + ); + + it.skipIf(process.env.RUN_ECR_TESTS !== "1")("should reuse existing ECR repository", async () => { + // This should use the repository created in the previous test + const imageRef = await getDeploymentImageRef({ registry: { host: testHost, namespace: testNamespace, @@ -87,20 +150,29 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", ecrAssumeRoleArn: roleArn, ecrAssumeRoleExternalId: externalId, }, - projectRef: testProjectRef2, - nextVersion: "20250630.1", - environmentSlug: "test", + projectRef: testProjectRef, + nextVersion: "20250630.2", + environmentType: "PRODUCTION", }); - expect(imageRef1.imageRef).toBe( - `${testHost}/${testNamespace}/${testProjectRef2}:20250630.1.test` + expect(imageRef.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + testHost + )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.[a-z0-9]{8}$` + ) ); - expect(imageRef1.isEcr).toBe(true); - expect(imageRef1.repoCreated).toBe(true); + expect(imageRef.isEcr).toBe(true); + }); - const imageRef2 = await getDeploymentImageRef({ + it("should generate unique image tags for different deployments with same environment type", async () => { + // Simulates the scenario where multiple deployments happen to the same environment type + const sameEnvironmentType = "PREVIEW"; + const sameVersion = "20250630.1"; + + const firstImageRef = await getDeploymentImageRef({ registry: { - host: testHost, + host: "registry.example.com", namespace: testNamespace, username: "test-user", password: "test-pass", @@ -108,23 +180,14 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", ecrAssumeRoleArn: roleArn, ecrAssumeRoleExternalId: externalId, }, - projectRef: testProjectRef2, - nextVersion: "20250630.2", - environmentSlug: "test", + projectRef: testProjectRef, + nextVersion: sameVersion, + environmentType: sameEnvironmentType, }); - expect(imageRef2.imageRef).toBe( - `${testHost}/${testNamespace}/${testProjectRef2}:20250630.2.test` - ); - expect(imageRef2.isEcr).toBe(true); - expect(imageRef2.repoCreated).toBe(false); - }); - - it("should reuse existing ECR repository", async () => { - // This should use the repository created in the previous test - const imageRef = await getDeploymentImageRef({ + const secondImageRef = await getDeploymentImageRef({ registry: { - host: testHost, + host: "registry.example.com", namespace: testNamespace, username: "test-user", password: "test-pass", @@ -133,37 +196,30 @@ describe.skipIf(process.env.RUN_REGISTRY_TESTS !== "1")("getDeploymentImageRef", ecrAssumeRoleExternalId: externalId, }, projectRef: testProjectRef, - nextVersion: "20250630.2", - environmentSlug: "prod", + nextVersion: sameVersion, + environmentType: sameEnvironmentType, }); - expect(imageRef.imageRef).toBe( - `${testHost}/${testNamespace}/${testProjectRef}:20250630.2.prod` + // Even with the same environment type and version, the image refs should be different due to random suffix + expect(firstImageRef.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + "registry.example.com" + )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$` + ) ); - expect(imageRef.isEcr).toBe(true); - }); - - it("should throw error for invalid ECR host", async () => { - await expect( - getDeploymentImageRef({ - registry: { - host: "invalid.ecr.amazonaws.com", - namespace: testNamespace, - username: "test-user", - password: "test-pass", - ecrTags: registryTags, - ecrAssumeRoleArn: roleArn, - ecrAssumeRoleExternalId: externalId, - }, - projectRef: testProjectRef, - nextVersion: "20250630.1", - environmentSlug: "test", - }) - ).rejects.toThrow("Invalid ECR registry host: invalid.ecr.amazonaws.com"); + expect(secondImageRef.imageRef).toMatch( + new RegExp( + `^${escapeHostForRegex( + "registry.example.com" + )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$` + ) + ); + expect(firstImageRef.imageRef).not.toBe(secondImageRef.imageRef); }); }); -describe.skipIf(process.env.RUN_REGISTRY_AUTH_TESTS !== "1")("getEcrAuthToken", () => { +describe.skipIf(process.env.RUN_ECR_TESTS !== "1")("getEcrAuthToken", () => { const testHost = process.env.DEPLOY_REGISTRY_HOST || "123456789012.dkr.ecr.us-east-1.amazonaws.com"; From 0a176767e26e70f396b906b32175017a80743015 Mon Sep 17 00:00:00 2001 From: nicktrn <55853254+nicktrn@users.noreply.github.com> Date: Wed, 3 Sep 2025 23:15:26 +0100 Subject: [PATCH 2/3] use deploy shortcode instead of new nanoid --- .../app/v3/getDeploymentImageRef.server.ts | 8 +++----- .../v3/services/initializeDeployment.server.ts | 5 ++++- apps/webapp/test/getDeploymentImageRef.test.ts | 18 ++++++++++++------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/apps/webapp/app/v3/getDeploymentImageRef.server.ts b/apps/webapp/app/v3/getDeploymentImageRef.server.ts index 2d88deb9de..a456791e97 100644 --- a/apps/webapp/app/v3/getDeploymentImageRef.server.ts +++ b/apps/webapp/app/v3/getDeploymentImageRef.server.ts @@ -12,9 +12,6 @@ import { tryCatch } from "@trigger.dev/core"; import { logger } from "~/services/logger.server"; import { type RegistryConfig } from "./registryConfig.server"; import type { EnvironmentType } from "@trigger.dev/core/v3"; -import { customAlphabet } from "nanoid"; - -const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); // Optional configuration for cross-account access export type AssumeRoleConfig = { @@ -106,11 +103,13 @@ export async function getDeploymentImageRef({ projectRef, nextVersion, environmentType, + deploymentShortCode, }: { registry: RegistryConfig; projectRef: string; nextVersion: string; environmentType: EnvironmentType; + deploymentShortCode: string; }): Promise<{ imageRef: string; isEcr: boolean; @@ -118,8 +117,7 @@ export async function getDeploymentImageRef({ }> { const repositoryName = `${registry.namespace}/${projectRef}`; const envType = environmentType.toLowerCase(); - const randomSuffix = nanoid(); - const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${envType}.${randomSuffix}`; + const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${envType}.${deploymentShortCode}`; if (!isEcrRegistry(registry.host)) { return { diff --git a/apps/webapp/app/v3/services/initializeDeployment.server.ts b/apps/webapp/app/v3/services/initializeDeployment.server.ts index ad354e1c15..234cfa1270 100644 --- a/apps/webapp/app/v3/services/initializeDeployment.server.ts +++ b/apps/webapp/app/v3/services/initializeDeployment.server.ts @@ -73,12 +73,15 @@ export class InitializeDeploymentService extends BaseService { const isV4Deployment = payload.type === "MANAGED"; const registryConfig = getRegistryConfig(isV4Deployment); + const deploymentShortCode = nanoid(8); + const [imageRefError, imageRefResult] = await tryCatch( getDeploymentImageRef({ registry: registryConfig, projectRef: environment.project.externalRef, nextVersion, environmentType: environment.type, + deploymentShortCode, }) ); @@ -111,7 +114,7 @@ export class InitializeDeploymentService extends BaseService { data: { friendlyId: generateFriendlyId("deployment"), contentHash: payload.contentHash, - shortCode: nanoid(8), + shortCode: deploymentShortCode, version: nextVersion, status: "BUILDING", environmentId: environment.id, diff --git a/apps/webapp/test/getDeploymentImageRef.test.ts b/apps/webapp/test/getDeploymentImageRef.test.ts index 2d566bcb49..f7306ae3f2 100644 --- a/apps/webapp/test/getDeploymentImageRef.test.ts +++ b/apps/webapp/test/getDeploymentImageRef.test.ts @@ -70,6 +70,7 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef, nextVersion: "20250630.1", environmentType: "DEVELOPMENT", + deploymentShortCode: "test1234", }); // Check the image ref structure and that it contains expected parts @@ -77,7 +78,7 @@ describe("getDeploymentImageRef", () => { new RegExp( `^${escapeHostForRegex( "registry.example.com" - )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.test1234$` ) ); expect(imageRef.isEcr).toBe(false); @@ -99,13 +100,14 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef2, nextVersion: "20250630.1", environmentType: "DEVELOPMENT", + deploymentShortCode: "test1234", }); expect(imageRef1.imageRef).toMatch( new RegExp( `^${escapeHostForRegex( testHost - )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.test1234$` ) ); expect(imageRef1.isEcr).toBe(true); @@ -124,13 +126,14 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef2, nextVersion: "20250630.2", environmentType: "DEVELOPMENT", + deploymentShortCode: "test1234", }); expect(imageRef2.imageRef).toMatch( new RegExp( `^${escapeHostForRegex( testHost - )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.test1234$` ) ); expect(imageRef2.isEcr).toBe(true); @@ -153,13 +156,14 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef, nextVersion: "20250630.2", environmentType: "PRODUCTION", + deploymentShortCode: "test1234", }); expect(imageRef.imageRef).toMatch( new RegExp( `^${escapeHostForRegex( testHost - )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.test1234$` ) ); expect(imageRef.isEcr).toBe(true); @@ -183,6 +187,7 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef, nextVersion: sameVersion, environmentType: sameEnvironmentType, + deploymentShortCode: "test1234", }); const secondImageRef = await getDeploymentImageRef({ @@ -198,6 +203,7 @@ describe("getDeploymentImageRef", () => { projectRef: testProjectRef, nextVersion: sameVersion, environmentType: sameEnvironmentType, + deploymentShortCode: "test4321", }); // Even with the same environment type and version, the image refs should be different due to random suffix @@ -205,14 +211,14 @@ describe("getDeploymentImageRef", () => { new RegExp( `^${escapeHostForRegex( "registry.example.com" - )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test1234$` ) ); expect(secondImageRef.imageRef).toMatch( new RegExp( `^${escapeHostForRegex( "registry.example.com" - )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$` + )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test4321$` ) ); expect(firstImageRef.imageRef).not.toBe(secondImageRef.imageRef); From a721b30afb0532ae70f8033371ef5189b838b1cd Mon Sep 17 00:00:00 2001 From: nicktrn <55853254+nicktrn@users.noreply.github.com> Date: Wed, 3 Sep 2025 23:19:10 +0100 Subject: [PATCH 3/3] replace regexps in tests --- .../webapp/test/getDeploymentImageRef.test.ts | 55 +++++-------------- 1 file changed, 14 insertions(+), 41 deletions(-) diff --git a/apps/webapp/test/getDeploymentImageRef.test.ts b/apps/webapp/test/getDeploymentImageRef.test.ts index f7306ae3f2..a05236596f 100644 --- a/apps/webapp/test/getDeploymentImageRef.test.ts +++ b/apps/webapp/test/getDeploymentImageRef.test.ts @@ -8,8 +8,6 @@ import { } from "../app/v3/getDeploymentImageRef.server"; import { DeleteRepositoryCommand } from "@aws-sdk/client-ecr"; -const escapeHostForRegex = (host: string) => host.replace(/\./g, "\\."); - describe("getDeploymentImageRef", () => { const testHost = process.env.DEPLOY_REGISTRY_HOST || "123456789012.dkr.ecr.us-east-1.amazonaws.com"; @@ -74,12 +72,8 @@ describe("getDeploymentImageRef", () => { }); // Check the image ref structure and that it contains expected parts - expect(imageRef.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - "registry.example.com" - )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.test1234$` - ) + expect(imageRef.imageRef).toBe( + `registry.example.com/${testNamespace}/${testProjectRef}:20250630.1.development.test1234` ); expect(imageRef.isEcr).toBe(false); }); @@ -103,12 +97,8 @@ describe("getDeploymentImageRef", () => { deploymentShortCode: "test1234", }); - expect(imageRef1.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - testHost - )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.test1234$` - ) + expect(imageRef1.imageRef).toBe( + `${testHost}/${testNamespace}/${testProjectRef2}:20250630.1.development.test1234` ); expect(imageRef1.isEcr).toBe(true); expect(imageRef1.repoCreated).toBe(true); @@ -129,12 +119,8 @@ describe("getDeploymentImageRef", () => { deploymentShortCode: "test1234", }); - expect(imageRef2.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - testHost - )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.test1234$` - ) + expect(imageRef2.imageRef).toBe( + `${testHost}/${testNamespace}/${testProjectRef2}:20250630.2.development.test1234` ); expect(imageRef2.isEcr).toBe(true); expect(imageRef2.repoCreated).toBe(false); @@ -159,12 +145,8 @@ describe("getDeploymentImageRef", () => { deploymentShortCode: "test1234", }); - expect(imageRef.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - testHost - )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.test1234$` - ) + expect(imageRef.imageRef).toBe( + `${testHost}/${testNamespace}/${testProjectRef}:20250630.2.production.test1234` ); expect(imageRef.isEcr).toBe(true); }); @@ -206,20 +188,12 @@ describe("getDeploymentImageRef", () => { deploymentShortCode: "test4321", }); - // Even with the same environment type and version, the image refs should be different due to random suffix - expect(firstImageRef.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - "registry.example.com" - )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test1234$` - ) + // Even with the same environment type and version, the image refs should be different due to deployment short codes + expect(firstImageRef.imageRef).toBe( + `registry.example.com/${testNamespace}/${testProjectRef}:${sameVersion}.preview.test1234` ); - expect(secondImageRef.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex( - "registry.example.com" - )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test4321$` - ) + expect(secondImageRef.imageRef).toBe( + `registry.example.com/${testNamespace}/${testProjectRef}:${sameVersion}.preview.test4321` ); expect(firstImageRef.imageRef).not.toBe(secondImageRef.imageRef); }); @@ -250,8 +224,7 @@ describe.skipIf(process.env.RUN_ECR_TESTS !== "1")("getEcrAuthToken", () => { expect(auth.password.length).toBeGreaterThan(0); // Verify the token format (should be a base64-encoded string) - const base64Regex = /^[A-Za-z0-9+/=]+$/; - expect(base64Regex.test(auth.password)).toBe(true); + expect(auth.password).toMatch(/^[A-Za-z0-9+/=]+$/); }); it("should throw error for invalid region", async () => {