Update buildAndMaybePush to use more robust method to find local digests

Older versions of docker sometimes will report the digest as "<none>"
which would break with the previous implementation. This implementation
should correctly grab the values.

Author: IRCody

.changeset/orange-teams-glow.md

@@ -0,0 +1,6 @@
+---
+"@cloudflare/containers-shared": patch
+"wrangler": patch
+---
+
+Update container builds to use a more robust method for detecting if the currently built image already exists.

packages/containers-shared/index.ts

@@ -6,4 +6,3 @@ export * from "./src/utils";
 export * from "./src/types";
 export * from "./src/inspect";
 export * from "./src/registry";
-export * from "./src/images";

packages/containers-shared/src/images.ts

@@ -240,14 +240,14 @@ describe("cloudchamber apply", () => {
 			{
 				id: "abc",
 				name: "my-container-app",
-				max_instances: 3,
+				max_instances: 4,
 				instances: 3,
 				created_at: new Date().toString(),
 				account_id: "1",
 				version: 1,
 				scheduling_policy: SchedulingPolicy.DEFAULT,
 				configuration: {
-					image: "./Dockerfile2",
+					image: "./Dockerfile",
 				},
 				constraints: {
 					tier: 1,
@@ -267,12 +267,11 @@ describe("cloudchamber apply", () => {
 			│
 			├ EDIT my-container-app
 			│
-			│   [containers.configuration]
-			│ - image = \\"./Dockerfile2\\"
-			│ + image = \\"./Dockerfile\\"
-			│
-			│   [containers.constraints]
-			│   ...
+			│   [[containers]]
+			│   instances = 0
+			│ - max_instances = 4
+			│ + max_instances = 3
+			│   name = \\"my-container-app\\"
 			│
 			├ NEW my-container-app-2
 			│

packages/wrangler/src/__tests__/cloudchamber/apply.test.ts

@@ -4,7 +4,6 @@ import {
 	dockerImageInspect,
 	dockerLoginManagedRegistry,
 	getCloudflareContainerRegistry,
-	getDockerImageDigest,
 	runDockerCmd,
 } from "@cloudflare/containers-shared";
 import { ensureDiskLimits } from "../../cloudchamber/build";
@@ -31,7 +30,6 @@ vi.mock("@cloudflare/containers-shared", async (importOriginal) => {
 		runDockerCmd: vi.fn(),
 		dockerBuild: vi.fn(),
 		dockerImageInspect: vi.fn(),
-		getDockerImageDigest: vi.fn(),
 	});
 });
 
@@ -46,8 +44,7 @@ describe("buildAndMaybePush", () => {
 
 	beforeEach(() => {
 		vi.clearAllMocks();
-		vi.mocked(dockerImageInspect).mockResolvedValue("53387881 2");
-		vi.mocked(getDockerImageDigest).mockRejectedValue("failed");
+		vi.mocked(dockerImageInspect).mockResolvedValue("53387881 2 []");
 		mkdirSync("./container-context");
 
 		writeFileSync("./container-context/Dockerfile", dockerfile);
@@ -77,7 +74,8 @@ describe("buildAndMaybePush", () => {
 		});
 		expect(dockerImageInspect).toHaveBeenCalledWith("/custom/docker/path", {
 			imageTag: `${getCloudflareContainerRegistry()}/test_account_id/test-app:tag`,
-			formatString: "{{ .Size }} {{ len .RootFS.Layers }}",
+			formatString:
+				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
 		expect(runDockerCmd).toHaveBeenCalledWith("/custom/docker/path", [
 			"push",
@@ -114,14 +112,17 @@ describe("buildAndMaybePush", () => {
 		expect(dockerImageInspect).toHaveBeenCalledOnce();
 		expect(dockerImageInspect).toHaveBeenCalledWith("docker", {
 			imageTag: `${getCloudflareContainerRegistry()}/test_account_id/test-app:tag`,
-			formatString: "{{ .Size }} {{ len .RootFS.Layers }}",
+			formatString:
+				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
 		expect(dockerLoginManagedRegistry).toHaveBeenCalledOnce();
 	});
 
 	it("should be able to build image and not push if it already exists in remote", async () => {
-		vi.mocked(getDockerImageDigest).mockResolvedValue("three");
 		vi.mocked(runDockerCmd).mockResolvedValueOnce();
+		vi.mocked(dockerImageInspect).mockResolvedValue(
+			'53387881 2 ["registry.cloudflare.com/test_account_id/test-app@sha256:three"]'
+		);
 		await runWrangler(
 			"containers build ./container-context -t test-app:tag -p"
 		);
@@ -146,7 +147,7 @@ describe("buildAndMaybePush", () => {
 			[
 				"manifest",
 				"inspect",
-				`${getCloudflareContainerRegistry()}/test_account_id/test-app@three`,
+				`${getCloudflareContainerRegistry()}/test_account_id/test-app@sha256:three`,
 			],
 			"ignore"
 		);
@@ -158,7 +159,8 @@ describe("buildAndMaybePush", () => {
 		expect(dockerImageInspect).toHaveBeenCalledOnce();
 		expect(dockerImageInspect).toHaveBeenCalledWith("docker", {
 			imageTag: `${getCloudflareContainerRegistry()}/test_account_id/test-app:tag`,
-			formatString: "{{ .Size }} {{ len .RootFS.Layers }}",
+			formatString:
+				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
 		expect(dockerLoginManagedRegistry).toHaveBeenCalledOnce();
 	});

packages/wrangler/src/__tests__/cloudchamber/build.test.ts

@@ -1,5 +1,5 @@
 import { Buffer } from "node:buffer";
-import { execFile, spawn, spawnSync } from "node:child_process";
+import { spawn, spawnSync } from "node:child_process";
 import { randomFillSync } from "node:crypto";
 import * as fs from "node:fs";
 import * as path from "node:path";
@@ -8711,28 +8711,6 @@ addEventListener('fetch', event => {});`
 						},
 					} as unknown as ChildProcess;
 				}
-				vi.mocked(execFile)
-					// docker images first call
-					.mockImplementationOnce((cmd, args, callback) => {
-						expect(cmd).toBe("/usr/bin/docker");
-						expect(args).toEqual([
-							"images",
-							"--digests",
-							"--format",
-							"{{.Digest}}",
-							getCloudflareContainerRegistry() +
-								"/test_account_id/my-container:Galaxy",
-						]);
-						if (callback) {
-							const back = callback as (
-								error: Error | null,
-								stdout: string,
-								stderr: string
-							) => void;
-							back(null, "three\n", "");
-						}
-						return {} as ChildProcess;
-					});
 
 				vi.mocked(spawn)
 					// 1. docker build
@@ -8783,7 +8761,7 @@ addEventListener('fetch', event => {});`
 							"inspect",
 							`${getCloudflareContainerRegistry()}/test_account_id/my-container:Galaxy`,
 							"--format",
-							"{{ .Size }} {{ len .RootFS.Layers }}",
+							"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 						]);
 
 						const stdout = new PassThrough();
@@ -8803,7 +8781,10 @@ addEventListener('fetch', event => {});`
 
 						// Simulate docker output
 						setImmediate(() => {
-							stdout.emit("data", "123456 4\n");
+							stdout.emit(
+								"data",
+								`123456 4 ["registry.cloudflare.com/test_account_id/test-app@sha256:three"]\n`
+							);
 						});
 
 						return child as unknown as ChildProcess;

packages/wrangler/src/__tests__/deploy.test.ts

@@ -314,7 +314,12 @@ function sortObjectRecursive<T = Record<string | number, unknown>>(
 }
 
 export async function apply(
-	args: { skipDefaults: boolean | undefined; json: boolean; env?: string },
+	args: {
+		skipDefaults: boolean | undefined;
+		json: boolean;
+		env?: string;
+		pushed?: boolean;
+	},
 	config: Config
 ) {
 	startSection(
@@ -378,7 +383,7 @@ export async function apply(
 
 	for (const appConfigNoDefaults of config.containers) {
 		const application = applicationByNames[appConfigNoDefaults.name];
-		if (!appConfigNoDefaults.configuration.image && application) {
+		if (!args.pushed && application) {
 			appConfigNoDefaults.configuration.image = application.configuration.image;
 		}
 		const appConfig = containerAppToCreateApplication(
@@ -742,7 +747,12 @@ export async function applyCommand(
 	config: Config
 ) {
 	return apply(
-		{ skipDefaults: args.skipDefaults, env: args.env, json: args.json },
+		{
+			skipDefaults: args.skipDefaults,
+			env: args.env,
+			json: args.json,
+			pushed: false,
+		},
 		config
 	);
 }

packages/wrangler/src/cloudchamber/apply.ts

@@ -6,7 +6,6 @@ import {
 	dockerImageInspect,
 	dockerLoginManagedRegistry,
 	getCloudflareRegistryWithAccountNamespace,
-	getDockerImageDigest,
 	isDir,
 	runDockerCmd,
 } from "@cloudflare/containers-shared";
@@ -79,7 +78,7 @@ export async function buildAndMaybePush(
 	pathToDocker: string,
 	push: boolean,
 	containerConfig?: ContainerApp
-): Promise<string> {
+): Promise<{ image: string; pushed: boolean }> {
 	try {
 		// account is also used to check limits below, so it is better to just pull the entire
 		// account information here
@@ -110,10 +109,11 @@ export async function buildAndMaybePush(
 		// account's disk size limits
 		const inspectOutput = await dockerImageInspect(pathToDocker, {
 			imageTag,
-			formatString: "{{ .Size }} {{ len .RootFS.Layers }}",
+			formatString:
+				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
 
-		const [sizeStr, layerStr] = inspectOutput.split(" ");
+		const [sizeStr, layerStr, repoDigests] = inspectOutput.split(" ");
 		const size = parseInt(sizeStr, 10);
 		const layers = parseInt(layerStr, 10);
 
@@ -126,18 +126,38 @@ export async function buildAndMaybePush(
 			account: account,
 			containerApp: containerConfig,
 		});
-
+		let pushed = false;
 		if (push) {
 			await dockerLoginManagedRegistry(pathToDocker);
 			try {
+				// we don't try to parse repoDigests until here because we
+				// don't want to fail on parse errors if we aren't pushing anyways
+				// as it's only relevant when pushing.
+				const parsedDigests = JSON.parse(repoDigests);
+				if (!Array.isArray(parsedDigests)) {
+					// If it's not the format we expect, fall back to pushing
+					// since it's annoying but safe.
+					throw new Error(
+						"Expected RepoDigests from docker inspect to be an array but it wasn't"
+					);
+				}
+
 				const repositoryOnly = imageTag.split(":")[0];
 				// if this succeeds it means this image already exists remotely
 				// if it fails it means it doesn't exist remotely and should be pushed.
-				const localDigest = await getDockerImageDigest(pathToDocker, imageTag);
-				const digest = repositoryOnly + "@" + localDigest;
+				const digests = parsedDigests.filter(
+					(d): d is string =>
+						typeof d === "string" && d.split("@")[0] === repositoryOnly
+				);
+				if (digests.length !== 1) {
+					throw new Error(
+						`Expected there to only be 1 valid digests for this repository: ${repositoryOnly} but there were ${digests.length}`
+					);
+				}
+
 				await runDockerCmd(
 					pathToDocker,
-					["manifest", "inspect", digest],
+					["manifest", "inspect", digests[0]],
 					"ignore"
 				);
 
@@ -146,14 +166,20 @@ export async function buildAndMaybePush(
 					`Untagging built image: ${imageTag} since there was no change.`
 				);
 				await runDockerCmd(pathToDocker, ["image", "rm", imageTag]);
-				return "";
+				return { image: digests[0], pushed: false };
 			} catch (error) {
 				logger.log(`Image does not exist remotely, pushing: ${imageTag}`);
+				if (error instanceof Error) {
+					logger.debug(
+						`Checking for local image ${imageTag} failed with error: ${error.message}`
+					);
+				}
 
 				await runDockerCmd(pathToDocker, ["push", imageTag]);
+				pushed = true;
 			}
 		}
-		return imageTag;
+		return { image: imageTag, pushed: pushed };
 	} catch (error) {
 		if (error instanceof Error) {
 			throw new UserError(error.message);

packages/wrangler/src/cloudchamber/build.ts

@@ -17,14 +17,16 @@ export async function maybeBuildContainer(
 	imageTag: string,
 	dryRun: boolean,
 	pathToDocker: string
-) {
+): Promise<{ image: string; pushed: boolean }> {
 	try {
 		if (
 			!isDockerfile(
 				containerConfig.image ?? containerConfig.configuration.image
 			)
 		) {
-			return containerConfig.image ?? containerConfig.configuration.image;
+			const image =
+				containerConfig.image ?? containerConfig.configuration.image;
+			return { image: image, pushed: false };
 		}
 	} catch (err) {
 		if (err instanceof Error) {
@@ -36,13 +38,13 @@ export async function maybeBuildContainer(
 
 	const options = getBuildArguments(containerConfig, imageTag);
 	logger.log("Building image", options.tag);
-	const tag = await buildAndMaybePush(
+	const buildResult = await buildAndMaybePush(
 		options,
 		pathToDocker,
 		!dryRun,
 		containerConfig
 	);
-	return tag;
+	return buildResult;
 }
 
 export type DeployContainersArgs = {
@@ -105,17 +107,19 @@ export async function deployContainers(
 			],
 		};
 
-		const image = await maybeBuildContainer(
+		const buildResult = await maybeBuildContainer(
 			container,
 			versionId,
 			dryRun,
 			pathToDocker
 		);
+		container.configuration.image = buildResult.image;
+		container.image = buildResult.image;
 
-		container.configuration.image = image;
-		container.image = image;
-
-		await apply({ skipDefaults: false, json: true, env }, configuration);
+		await apply(
+			{ skipDefaults: false, json: true, env, pushed: buildResult.pushed },
+			configuration
+		);
 	}
 }