Update buildAndMaybePush to use more robust method to find local digests (#9641)

* 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.

* Update with PR feedback

* Ensure container.configuration.image gets written to

Fixes an issue where the user could get into a state where the image has
been pushed but the container fails to update it's configuration. On the
next push container.configuration.image will not be set and
container.image is deleted leading to failures when trying to update the
container app.

Author: IRCody

.changeset/orange-teams-glow.md

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

packages/containers-shared/src/images.ts

@@ -10,29 +10,6 @@ import {
 	verifyDockerInstalled,
 } from "./utils";
 
-// Returns a list of docker image ids matching the provided repository:[tag]
-export async function getDockerImageDigest(
-	dockerPath: string,
-	imageTag: string
-): Promise<string> {
-	return new Promise((resolve, reject) => {
-		execFile(
-			dockerPath,
-			["images", "--digests", "--format", "{{.Digest}}", imageTag],
-			(error, stdout, stderr) => {
-				if (error) {
-					return reject(
-						new Error(
-							`Failed getting docker image digest for image: ${imageTag} with error: ${error}.`
-						)
-					);
-				}
-				return resolve(stdout.trim());
-			}
-		);
-	});
-}
-
 export async function pullImage(
 	dockerPath: string,
 	options: ContainerDevOptions

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

@@ -225,14 +225,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,
@@ -252,12 +252,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/build.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__/deploy.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 ["${getCloudflareContainerRegistry()}/test_account_id/my-container@sha256:three"]`
+							);
 						});
 
 						return child as unknown as ChildProcess;

packages/wrangler/src/cloudchamber/apply.ts

@@ -399,7 +399,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;
+		imageUpdateRequired?: boolean;
+	},
 	config: Config
 ) {
 	startSection(
@@ -476,6 +481,10 @@ export async function apply(
 		// while configuration.image is deprecated to the user, we still resolve to this for now.
 		if (!appConfigNoDefaults.configuration?.image && application) {
 			appConfigNoDefaults.configuration ??= {};
+		}
+
+		if (!args.imageUpdateRequired && application) {
+			appConfigNoDefaults.configuration ??= {};
 			appConfigNoDefaults.configuration.image = application.configuration.image;
 		}
 
@@ -844,7 +853,14 @@ 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,
+			// For the apply command we want this to default to true
+			// so that the image can be updated if the user modified it.
+			imageUpdateRequired: true,
+		},
 		config
 	);
 }

packages/wrangler/src/cloudchamber/build.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,46 @@ 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 this point
+				// because we don't want to fail on parse errors if we
+				// won't be pushing the image anyways.
+				//
+				// 	A Docker image digest is a unique, cryptographic identifier (SHA-256 hash)
+				//	representing the content of a Docker image. Unlike tags, which can be reused
+				//	or changed, a digest is immutable and ensures that the exact same image is
+				//	pulled every time. This guarantees consistency across different environments
+				//	and deployments.
+				// 	From: https://docs.docker.com/dhi/core-concepts/digests/
+				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 got ${JSON.stringify(parsedDigests)}`
+					);
+				}
+
 				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 +174,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);