Build containers without account ID (#9765)

* Build container without account ID

This allows us to build containers in dry run mode. When we push the
container to the managed registry, we re-tag it to include the account
ID. We only ever push when not in dry run mode though, so when pushing
we can safely grab the account information.

Co-authored-by: Hasip Timurtaş <[email protected]>

* Always check for Docker when using containers

Even in dry run mode we should check that Docker is installed, because
dry run mode uses docker commands too.

---------

Co-authored-by: Greg Anders <[email protected]>

Author: hasip-timurtas

.changeset/container-dry-run-fix.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+Build container images without the user's account ID. This allows containers to be built and verified in dry run mode (where we do not necessarily have the user's account info).
+
+When we push the image to the managed registry, we first re-tag the image to include the user's account ID so that the image has the full resolved image name.

packages/containers-shared/src/images.ts

@@ -1,5 +1,8 @@
 import { buildImage } from "./build";
-import { isCloudflareRegistryLink } from "./knobs";
+import {
+	getCloudflareContainerRegistry,
+	isCloudflareRegistryLink,
+} from "./knobs";
 import { dockerLoginManagedRegistry } from "./login";
 import { ContainerDevOptions } from "./types";
 import {
@@ -61,3 +64,31 @@ export async function prepareContainerImagesForDev(
 		await checkExposedPorts(dockerPath, options);
 	}
 }
+
+/**
+ * Resolve an image name to the full unambiguous name.
+ *
+ * For now, this only converts images stored in the managed registry to contain
+ * the user's account ID in the path.
+ */
+export async function resolveImageName(
+	accountId: string,
+	image: string
+): Promise<string> {
+	let url: URL;
+	try {
+		url = new URL(`http://${image}`);
+	} catch (_) {
+		return image;
+	}
+
+	if (url.hostname !== getCloudflareContainerRegistry()) {
+		return image;
+	}
+
+	if (url.pathname.startsWith(`/${accountId}`)) {
+		return image;
+	}
+
+	return `${url.hostname}/${accountId}${url.pathname}`;
+}

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

@@ -62,7 +62,7 @@ describe("buildAndMaybePush", () => {
 			buildCmd: [
 				"build",
 				"-t",
-				`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+				`${getCloudflareContainerRegistry()}/test-app:tag`,
 				"--platform",
 				"linux/amd64",
 				"--provenance=false",
@@ -73,7 +73,7 @@ describe("buildAndMaybePush", () => {
 			dockerfile,
 		});
 		expect(dockerImageInspect).toHaveBeenCalledWith("/custom/docker/path", {
-			imageTag: `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+			imageTag: `${getCloudflareContainerRegistry()}/test-app:tag`,
 			formatString:
 				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
@@ -94,7 +94,7 @@ describe("buildAndMaybePush", () => {
 			buildCmd: [
 				"build",
 				"-t",
-				`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+				`${getCloudflareContainerRegistry()}/test-app:tag`,
 				"--platform",
 				"linux/amd64",
 				"--provenance=false",
@@ -104,14 +104,26 @@ describe("buildAndMaybePush", () => {
 			],
 			dockerfile,
 		});
-		expect(runDockerCmd).toHaveBeenCalledTimes(1);
-		expect(runDockerCmd).toHaveBeenCalledWith("docker", [
+
+		// 3 calls: docker tag + docker push + docker rm
+		expect(runDockerCmd).toHaveBeenCalledTimes(3);
+		expect(runDockerCmd).toHaveBeenNthCalledWith(1, "docker", [
+			"tag",
+			`${getCloudflareContainerRegistry()}/test-app:tag`,
+			`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+		]);
+		expect(runDockerCmd).toHaveBeenNthCalledWith(2, "docker", [
 			"push",
 			`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
 		]);
+		expect(runDockerCmd).toHaveBeenNthCalledWith(3, "docker", [
+			"image",
+			"rm",
+			`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+		]);
 		expect(dockerImageInspect).toHaveBeenCalledOnce();
 		expect(dockerImageInspect).toHaveBeenCalledWith("docker", {
-			imageTag: `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+			imageTag: `${getCloudflareContainerRegistry()}/test-app:tag`,
 			formatString:
 				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
@@ -121,7 +133,7 @@ describe("buildAndMaybePush", () => {
 	it("should be able to build image and not push if it already exists in remote", async () => {
 		vi.mocked(runDockerCmd).mockResolvedValueOnce();
 		vi.mocked(dockerImageInspect).mockResolvedValue(
-			'53387881 2 ["registry.cloudflare.com/some-account-id/test-app@sha256:three"]'
+			'53387881 2 ["registry.cloudflare.com/test-app@sha256:three"]'
 		);
 		await runWrangler(
 			"containers build ./container-context -t test-app:tag -p"
@@ -130,7 +142,7 @@ describe("buildAndMaybePush", () => {
 			buildCmd: [
 				"build",
 				"-t",
-				`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+				`${getCloudflareContainerRegistry()}/test-app:tag`,
 				"--platform",
 				"linux/amd64",
 				"--provenance=false",
@@ -154,11 +166,11 @@ describe("buildAndMaybePush", () => {
 		expect(runDockerCmd).toHaveBeenNthCalledWith(2, "docker", [
 			"image",
 			"rm",
-			`${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+			`${getCloudflareContainerRegistry()}/test-app:tag`,
 		]);
 		expect(dockerImageInspect).toHaveBeenCalledOnce();
 		expect(dockerImageInspect).toHaveBeenCalledWith("docker", {
-			imageTag: `${getCloudflareContainerRegistry()}/some-account-id/test-app:tag`,
+			imageTag: `${getCloudflareContainerRegistry()}/test-app:tag`,
 			formatString:
 				"{{ .Size }} {{ len .RootFS.Layers }} {{json .RepoDigests}}",
 		});
@@ -172,7 +184,7 @@ describe("buildAndMaybePush", () => {
 			buildCmd: [
 				"build",
 				"-t",
-				`${getCloudflareContainerRegistry()}/some-account-id/test-app`,
+				`${getCloudflareContainerRegistry()}/test-app`,
 				"--platform",
 				"linux/amd64",
 				"--provenance=false",
@@ -194,7 +206,7 @@ describe("buildAndMaybePush", () => {
 			buildCmd: [
 				"build",
 				"-t",
-				`${getCloudflareContainerRegistry()}/some-account-id/test-app`,
+				`${getCloudflareContainerRegistry()}/test-app`,
 				"--platform",
 				"linux/amd64",
 				"--provenance=false",
@@ -270,27 +282,24 @@ describe("buildAndMaybePush", () => {
 	});
 
 	describe("resolveAppDiskSize", () => {
-		const accountBase = {
-			limits: { disk_mb_per_deployment: 2000 },
-		} as CompleteAccountCustomer;
 		it("should return parsed app disk size", () => {
-			const result = resolveAppDiskSize(accountBase, {
+			const result = resolveAppDiskSize({
 				...defaultConfiguration,
 				configuration: { image: "", disk: { size: "500MB" } },
 			});
 			expect(result).toBeCloseTo(500 * 1000 * 1000, -5);
 		});
 
 		it("should return default size when disk size not set", () => {
-			const result = resolveAppDiskSize(accountBase, {
+			const result = resolveAppDiskSize({
 				...defaultConfiguration,
 				configuration: { image: "" },
 			});
 			expect(result).toBeCloseTo(2 * 1000 * 1000 * 1000, -5);
 		});
 
 		it("should return undefined if app is not passed", () => {
-			expect(resolveAppDiskSize(accountBase, undefined)).toBeUndefined();
+			expect(resolveAppDiskSize(undefined)).toBeUndefined();
 		});
 	});
 });