diff --git a/.server-changes/README.md b/.server-changes/README.md index 2b0eeade36..1ecfe3ee23 100644 --- a/.server-changes/README.md +++ b/.server-changes/README.md @@ -42,10 +42,16 @@ The body text (below the frontmatter) is a one-line description of the change. K These entries are public-facing - they ship verbatim in user-visible release notes. A few rules to keep them clean: +- **Write for the user, not the reviewer.** Lead with what the user notices or has to do. If a reader who doesn't know the codebase can't tell what changed for them, rewrite it. - **One sentence is usually enough.** The body is the bullet in the changelog. If you need a paragraph, you're probably describing the implementation rather than the change. - **Describe behavior, not implementation.** Skip internal scopes, middleware names, library specifics, framework internals. Users care about what's different for them, not how it's wired. - **Never name internal tools or infra.** Observability stacks, internal services, infra components, monitoring backends, CI surfaces, AWS specifics - none of these belong in user-facing notes. +Before / after: + +- ❌ _"The image verification step now parses the manifest's layer media types and returns a new result the finalizer rejects."_ (describes the wiring; a user can't act on it) +- ✅ _"Deploying with an outdated CLI could produce an image that fails to start on every run. These deploys are now stopped before going live, with a message asking you to upgrade the CLI and re-deploy."_ (what the user sees and does) + ## Lifecycle 1. Engineer adds a `.server-changes/` file in their PR diff --git a/.server-changes/reject-unpullable-deploy-image.md b/.server-changes/reject-unpullable-deploy-image.md new file mode 100644 index 0000000000..51cb837009 --- /dev/null +++ b/.server-changes/reject-unpullable-deploy-image.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Deploying with an outdated CLI could produce an image that fails to start on every run. These deploys are now stopped before going live, with a message asking you to upgrade the CLI and re-deploy. diff --git a/AGENTS.md b/AGENTS.md index a51faebbcd..536ec0188d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,6 +104,8 @@ pnpm run changeset:add When modifying only server components (`apps/webapp/`, `apps/supervisor/`, etc.) with no package changes, add a `.server-changes/` file instead. See `.server-changes/README.md` for format and documentation. +**Write the description for users, not maintainers.** Both changesets and `.server-changes/` notes ship verbatim in user-visible release notes. Lead with what changed *for the user* - one plain sentence describing behavior, not implementation, and never naming internal tools or infra. The full writing guidance in `.server-changes/README.md` applies to changesets too. + ## Dependency Pinning Zod is pinned to a single version across the entire monorepo (currently `3.25.76`). When adding zod to a new or existing package, use the **exact same version** as the rest of the repo - never a different version or a range. Mismatched zod versions cause runtime type incompatibilities (e.g., schemas from one package can't be used as body validators in another). diff --git a/apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts b/apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts index 464dd8aabe..acc8cbb537 100644 --- a/apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts +++ b/apps/webapp/app/v3/services/finalizeDeploymentV2.server.ts @@ -186,6 +186,12 @@ export class FinalizeDeploymentV2Service extends BaseService { ); } + if (result === "nonconformant") { + throw new ServiceValidationError( + "Deployment image is not runnable: it contains zstd-compressed layers inside a Docker (v2s2) manifest, which the container runtime cannot pull. This typically comes from an outdated CLI version. Please upgrade to the latest trigger.dev CLI and re-deploy." + ); + } + // Fail closed: if we can't confirm the image is present, don't promote a version // that might not start. Set DEPLOY_IMAGE_VERIFICATION_ENABLED=0 for out-of-band pushes. if (result === "unknown") { diff --git a/apps/webapp/app/v3/services/verifyDeploymentImage.server.ts b/apps/webapp/app/v3/services/verifyDeploymentImage.server.ts index 7062f89d88..858196380a 100644 --- a/apps/webapp/app/v3/services/verifyDeploymentImage.server.ts +++ b/apps/webapp/app/v3/services/verifyDeploymentImage.server.ts @@ -5,6 +5,7 @@ import { } from "@aws-sdk/client-ecr"; import { tryCatch } from "@trigger.dev/core"; import pRetry, { AbortError } from "p-retry"; +import { z } from "zod"; import { logger } from "~/services/logger.server"; import { type AssumeRoleConfig, @@ -16,7 +17,85 @@ import { type RegistryConfig } from "../registryConfig.server"; const SHA256_DIGEST = /^sha256:[a-f0-9]{64}$/; -export type ImageLookupResult = "found" | "missing" | "unknown"; +export type ImageLookupResult = "found" | "missing" | "unknown" | "nonconformant"; + +// A zstd layer carried in a Docker (v2s2) manifest rather than an OCI manifest is +// unpullable by cri-o/containerd/podman. OCI zstd (...tar+zstd) is fine - only this +// Docker media type is rejected. An outdated CLI that predates OCI-media-type output +// can emit it when reusing zstd layers from a prior build or the registry cache. +const UNPULLABLE_LAYER_MEDIA_TYPE = "application/vnd.docker.image.rootfs.diff.tar.zstd"; + +// Nested indexes are exotic (index -> per-platform image manifests is depth 1); cap the +// walk so a pathological manifest can't fan out unbounded. +const MAX_MANIFEST_DEPTH = 3; + +// Lenient: we only read what we need. An image manifest carries layers[]; a manifest list +// / OCI index carries manifests[] (per-platform child pointers). Both optional so either +// shape parses cleanly. +const ManifestSchema = z.object({ + layers: z.array(z.object({ mediaType: z.string().optional() })).optional(), + manifests: z.array(z.object({ digest: z.string() })).optional(), +}); + +// Inspect one raw manifest: does it directly carry an unpullable layer, and (if it's an +// index) which child manifests should be walked. Fails open to empty on anything we can't +// parse, so an unreadable manifest never blocks a deploy. +export function inspectManifest(rawManifest: string | undefined): { + hasUnpullableLayer: boolean; + childDigests: string[]; +} { + const empty = { hasUnpullableLayer: false, childDigests: [] }; + + if (!rawManifest) { + return empty; + } + + let json: unknown; + try { + json = JSON.parse(rawManifest); + } catch { + return empty; + } + + const parsed = ManifestSchema.safeParse(json); + if (!parsed.success) { + return empty; + } + + return { + hasUnpullableLayer: + parsed.data.layers?.some((layer) => layer.mediaType === UNPULLABLE_LAYER_MEDIA_TYPE) ?? false, + childDigests: parsed.data.manifests?.map((child) => child.digest) ?? [], + }; +} + +// Walk a manifest and, for a multi-arch index, its child manifests (fetched by digest). +// Returns true if any layer anywhere in the tree uses the unpullable media type. A child +// that can't be fetched (undefined) is treated as conformant - fail open. +export async function treeHasUnpullableLayer( + rawManifest: string | undefined, + fetchManifest: (digest: string) => Promise, + depth = 0 +): Promise { + const { hasUnpullableLayer, childDigests } = inspectManifest(rawManifest); + + if (hasUnpullableLayer) { + return true; + } + + if (depth >= MAX_MANIFEST_DEPTH) { + return false; + } + + for (const digest of childDigests) { + const childManifest = await fetchManifest(digest); + if (await treeHasUnpullableLayer(childManifest, fetchManifest, depth + 1)) { + return true; + } + } + + return false; +} /** * Split a stored ECR image reference into repository + tag. @@ -86,8 +165,10 @@ const sendBatchGetImage: BatchGetImageSender = async ({ imageIds, }) => { const ecr = await createEcrClient({ region, assumeRole }); - // No acceptedMediaTypes: only the single-manifest types are valid enum values, and - // we only care whether the image exists, not its manifest format. + // Intentionally no acceptedMediaTypes: ECR returns the manifest as stored - which we + // rely on for the layer-media-type check - and omitting it avoids a multi-arch index + // being reported as a failure (i.e. misread as missing). BatchGetImage populates + // imageManifest by default when the image exists; the check fails open if it's absent. return ecr.send(new BatchGetImageCommand({ repositoryName, registryId, imageIds })); }; @@ -183,5 +264,46 @@ export async function ecrImageExists( return "unknown"; } - return interpretBatchGetImageResponse(response); + const result = interpretBatchGetImageResponse(response); + + if (result !== "found") { + return result; + } + + // Image exists - now confirm the runtime can actually pull it. Follow index children by + // digest so multi-arch deploys are covered, not just single image manifests. + const fetchManifest = async (digest: string): Promise => { + const [fetchError, childResponse] = await tryCatch( + _send({ + region, + assumeRole, + registryId: accountId, + repositoryName: parsed.repositoryName, + imageIds: [{ imageDigest: digest }], + }) + ); + + if (fetchError) { + logger.warn("Could not fetch child manifest for conformance check", { + imageReference, + digest, + error: fetchError.message, + }); + return undefined; // fail open on this child + } + + return childResponse.images?.[0]?.imageManifest; + }; + + const topManifest = response.images?.[0]?.imageManifest; + + if (await treeHasUnpullableLayer(topManifest, fetchManifest)) { + logger.error("Deployment image has a runtime-incompatible layer media type", { + imageReference, + repositoryName: parsed.repositoryName, + }); + return "nonconformant"; + } + + return "found"; } diff --git a/apps/webapp/test/verifyDeploymentImage.test.ts b/apps/webapp/test/verifyDeploymentImage.test.ts index 43508141cf..a4ead7e44a 100644 --- a/apps/webapp/test/verifyDeploymentImage.test.ts +++ b/apps/webapp/test/verifyDeploymentImage.test.ts @@ -2,14 +2,37 @@ import { RepositoryNotFoundException } from "@aws-sdk/client-ecr"; import { describe, expect, it } from "vitest"; import { ecrImageExists, + inspectManifest, interpretBatchGetImageResponse, parseEcrImageReference, + treeHasUnpullableLayer, } from "~/v3/services/verifyDeploymentImage.server"; import { type RegistryConfig } from "~/v3/registryConfig.server"; const ECR_HOST = "123456789012.dkr.ecr.us-east-1.amazonaws.com"; const ecrConfig: RegistryConfig = { host: ECR_HOST, namespace: "deployments-test" }; +const DIGEST_A = `sha256:${"a".repeat(64)}`; +const DIGEST_B = `sha256:${"b".repeat(64)}`; +const ZSTD_DOCKER = "application/vnd.docker.image.rootfs.diff.tar.zstd"; + +const imageManifest = (layerMediaTypes: string[]) => + JSON.stringify({ + schemaVersion: 2, + mediaType: "application/vnd.docker.distribution.manifest.v2+json", + layers: layerMediaTypes.map((mediaType) => ({ mediaType, digest: DIGEST_A })), + }); + +const indexManifest = (childDigests: string[]) => + JSON.stringify({ + schemaVersion: 2, + mediaType: "application/vnd.oci.image.index.v1+json", + manifests: childDigests.map((digest) => ({ + digest, + mediaType: "application/vnd.oci.image.manifest.v1+json", + })), + }); + describe("parseEcrImageReference", () => { it("splits repository and tag for a ref under the configured host", () => { const ref = `${ECR_HOST}/deployments-test/proj_abc:20240101.1.prod.a1b2c3d4`; @@ -61,6 +84,73 @@ describe("interpretBatchGetImageResponse", () => { }); }); +describe("inspectManifest", () => { + it("flags an unpullable zstd layer in an image manifest", () => { + const result = inspectManifest( + imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip", ZSTD_DOCKER]) + ); + expect(result.hasUnpullableLayer).toBe(true); + expect(result.childDigests).toEqual([]); + }); + + it("passes OCI zstd and gzip layers (runtime-supported media types)", () => { + const result = inspectManifest( + imageManifest([ + "application/vnd.oci.image.layer.v1.tar+gzip", + "application/vnd.oci.image.layer.v1.tar+zstd", + ]) + ); + expect(result.hasUnpullableLayer).toBe(false); + }); + + it("returns child digests for an index and does not flag it directly", () => { + const result = inspectManifest(indexManifest([DIGEST_A, DIGEST_B])); + expect(result.hasUnpullableLayer).toBe(false); + expect(result.childDigests).toEqual([DIGEST_A, DIGEST_B]); + }); + + it("fails open (empty) when the manifest is absent or unparseable", () => { + expect(inspectManifest(undefined)).toEqual({ hasUnpullableLayer: false, childDigests: [] }); + expect(inspectManifest("not json")).toEqual({ hasUnpullableLayer: false, childDigests: [] }); + }); +}); + +describe("treeHasUnpullableLayer", () => { + const neverFetch = async () => undefined; + + it("detects an unpullable layer in a flat image manifest", async () => { + expect(await treeHasUnpullableLayer(imageManifest([ZSTD_DOCKER]), neverFetch)).toBe(true); + }); + + it("follows an index and detects an unpullable layer in a child", async () => { + const children: Record = { + [DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+gzip"]), + [DIGEST_B]: imageManifest([ZSTD_DOCKER]), + }; + const result = await treeHasUnpullableLayer( + indexManifest([DIGEST_A, DIGEST_B]), + async (digest) => children[digest] + ); + expect(result).toBe(true); + }); + + it("returns false for an index whose children are all conformant", async () => { + const children: Record = { + [DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+zstd"]), + [DIGEST_B]: imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip"]), + }; + const result = await treeHasUnpullableLayer( + indexManifest([DIGEST_A, DIGEST_B]), + async (digest) => children[digest] + ); + expect(result).toBe(false); + }); + + it("fails open when a child manifest can't be fetched", async () => { + expect(await treeHasUnpullableLayer(indexManifest([DIGEST_A]), neverFetch)).toBe(false); + }); +}); + describe("ecrImageExists", () => { it("returns unknown for a non-ECR registry without calling the registry", async () => { let called = false; @@ -174,4 +264,54 @@ describe("ecrImageExists", () => { ); expect(seen.imageIds).toEqual([{ imageTag: "v1.prod.a1b2c3d4" }]); }); + + // Resolve a manifest by tag or digest against a fixture map, mimicking BatchGetImage. + const sendFrom = + (byRef: Record) => + async (input: any): Promise => { + const id = input.imageIds[0]; + const manifest = byRef[id.imageDigest ?? id.imageTag]; + return manifest ? { images: [{ imageManifest: manifest }] } : { images: [{}] }; + }; + + it("returns nonconformant for a single-arch image with an unpullable zstd layer", async () => { + const result = await ecrImageExists( + { + imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`, + registryConfig: ecrConfig, + }, + sendFrom({ "v1.prod.a1b2c3d4": imageManifest([ZSTD_DOCKER]) }) + ); + expect(result).toBe("nonconformant"); + }); + + it("returns nonconformant when a multi-arch index has an unpullable child", async () => { + const result = await ecrImageExists( + { + imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`, + registryConfig: ecrConfig, + }, + sendFrom({ + "v1.prod.a1b2c3d4": indexManifest([DIGEST_A, DIGEST_B]), + [DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+gzip"]), + [DIGEST_B]: imageManifest([ZSTD_DOCKER]), + }) + ); + expect(result).toBe("nonconformant"); + }); + + it("returns found when a multi-arch index's children are all conformant", async () => { + const result = await ecrImageExists( + { + imageReference: `${ECR_HOST}/deployments-test/proj_abc:v1.prod.a1b2c3d4`, + registryConfig: ecrConfig, + }, + sendFrom({ + "v1.prod.a1b2c3d4": indexManifest([DIGEST_A, DIGEST_B]), + [DIGEST_A]: imageManifest(["application/vnd.oci.image.layer.v1.tar+zstd"]), + [DIGEST_B]: imageManifest(["application/vnd.docker.image.rootfs.diff.tar.gzip"]), + }) + ); + expect(result).toBe("found"); + }); });