fix re-deploying after container deploys fail in an incomplete state (#10253)

* use remote digest if no previous deployment

* changeset

* pr feedback

* fixup

* remove random comment from another pr...

Author: emily-shen

.changeset/whole-readers-admire.md

@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix redeploying container apps when previous deploy failed or container (but not image) was deleted.
+
+Previously this failed with `No changes detected but no previous image found` as we assumed there would be a previous deployment when an image exists in the registry.

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

@@ -7,7 +7,6 @@ import {
 	SchedulingPolicy,
 } from "@cloudflare/containers-shared";
 import { http, HttpResponse } from "msw";
-import { maybeBuildContainer } from "../../cloudchamber/deploy";
 import { clearCachedAccount } from "../../cloudchamber/locations";
 import { mockAccountV4 as mockContainersAccount } from "../cloudchamber/utils";
 import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
@@ -27,30 +26,12 @@ import { writeWranglerConfig } from "../helpers/write-wrangler-config";
 import type {
 	AccountRegistryToken,
 	Application,
-	ContainerNormalizedConfig,
 	CreateApplicationRequest,
 	ImageRegistryCredentialsConfiguration,
 } from "@cloudflare/containers-shared";
 import type { ChildProcess } from "node:child_process";
 
 vi.mock("node:child_process");
-describe("maybeBuildContainer", () => {
-	it("Should return imageUpdate: true if using an image URI", async () => {
-		const config = {
-			image_uri: "registry.cloudflare.com/some-image:uri",
-			class_name: "Test",
-		} as ContainerNormalizedConfig;
-		const result = await maybeBuildContainer(
-			config,
-			"some-tag:thing",
-			false,
-			"/usr/bin/docker"
-		);
-		expect(result.newImageLink).toEqual(
-			"registry.cloudflare.com/some-image:uri"
-		);
-	});
-});
 
 describe("wrangler deploy with containers", () => {
 	runInTempDir();

packages/wrangler/src/cloudchamber/build.ts

@@ -79,12 +79,24 @@ export function pushYargs(yargs: CommonYargsArgv) {
 		.positional("TAG", { type: "string", demandOption: true });
 }
 
+/**
+ *
+ * `{ remoteDigest: string }` implies the image already exists remotely. we will
+ * try and replace this with the image tag from the last deployment if possible.
+ * If a deployment failed between push and deploy, we can't know for certain
+ * what the tag of the last push was, so we will use the digest instead.
+ *
+ * `{ newTag: string }` implies the image was built and pushed and the deployment
+ * should be associated with a new tag.
+ */
+export type ImageRef = { remoteDigest: string } | { newTag: string };
+
 export async function buildAndMaybePush(
 	args: BuildArgs,
 	pathToDocker: string,
 	push: boolean,
 	containerConfig?: Exclude<ContainerNormalizedConfig, ImageURIConfig>
-): Promise<{ image: string; pushed: boolean }> {
+): Promise<ImageRef> {
 	try {
 		const imageTag = `${getCloudflareContainerRegistry()}/${args.tag}`;
 		const { buildCmd, dockerfile } = await constructBuildCommand(
@@ -104,7 +116,6 @@ export async function buildAndMaybePush(
 			dockerfile,
 		}).ready;
 
-		let pushed = false;
 		if (push) {
 			/**
 			 * Get `RepoDigests` and `Id`:
@@ -199,7 +210,7 @@ export async function buildAndMaybePush(
 						`Untagging built image: ${args.tag} since there was no change.`
 					);
 					await runDockerCmd(pathToDocker, ["image", "rm", imageTag]);
-					return { image: remoteDigest, pushed: false };
+					return { remoteDigest };
 				}
 			} catch (error) {
 				if (error instanceof Error) {
@@ -213,17 +224,15 @@ export async function buildAndMaybePush(
 				account.external_account_id,
 				args.tag
 			);
-
 			logger.log(
 				`Image does not exist remotely, pushing: ${namespacedImageTag}`
 			);
 			await runDockerCmd(pathToDocker, ["tag", imageTag, namespacedImageTag]);
 			await runDockerCmd(pathToDocker, ["push", namespacedImageTag]);
 			await runDockerCmd(pathToDocker, ["image", "rm", namespacedImageTag]);
-			pushed = true;
 		}
 
-		return { image: imageTag, pushed };
+		return { newTag: imageTag };
 	} catch (error) {
 		if (error instanceof Error) {
 			throw new UserError(error.message, { cause: error });

packages/wrangler/src/cloudchamber/deploy.ts

@@ -8,27 +8,23 @@ import { logger } from "../logger";
 import { fetchVersion } from "../versions/api";
 import { buildAndMaybePush } from "./build";
 import { fillOpenAPIConfiguration } from "./common";
-import type { ContainerNormalizedConfig } from "@cloudflare/containers-shared/src/types";
+import type { ImageRef } from "./build";
+import type {
+	ContainerNormalizedConfig,
+	ImageURIConfig,
+} from "@cloudflare/containers-shared/src/types";
 
-export async function maybeBuildContainer(
-	containerConfig: ContainerNormalizedConfig,
+export async function buildContainer(
+	containerConfig: Exclude<ContainerNormalizedConfig, ImageURIConfig>,
 	/** just the tag component. will be prefixed with the container name */
 	imageTag: string,
 	dryRun: boolean,
 	pathToDocker: string
-): Promise<{ newImageLink: string | undefined }> {
-	if ("image_uri" in containerConfig) {
-		return {
-			// We don't know at this point whether the image has changed
-			// but we need to make sure API checks so
-			// we always set this to the registry link.
-			newImageLink: containerConfig.image_uri,
-		};
-	}
+): Promise<ImageRef> {
 	const imageFullName = containerConfig.name + ":" + imageTag.split("-")[0];
 	logger.log("Building image", imageFullName);
 
-	const buildResult = await buildAndMaybePush(
+	return await buildAndMaybePush(
 		{
 			tag: imageFullName,
 			pathToDockerfile: containerConfig.dockerfile,
@@ -39,13 +35,6 @@ export async function maybeBuildContainer(
 		!dryRun,
 		containerConfig
 	);
-
-	if (buildResult.pushed) {
-		return { newImageLink: buildResult.image };
-	}
-	// if the image has not changed, it will not have been pushed
-	// so we don't need to update anything when we apply the container config
-	return { newImageLink: undefined };
 }
 
 export type DeployContainersArgs = {
@@ -65,13 +54,18 @@ export async function deployContainers(
 
 	const pathToDocker = getDockerPath();
 	const version = await fetchVersion(config, accountId, scriptName, versionId);
+	let imageRef: ImageRef;
 	for (const container of normalisedContainerConfig) {
-		const buildResult = await maybeBuildContainer(
-			container,
-			versionId,
-			false,
-			pathToDocker
-		);
+		if ("dockerfile" in container) {
+			imageRef = await buildContainer(
+				container,
+				versionId,
+				false,
+				pathToDocker
+			);
+		} else {
+			imageRef = { newTag: container.image_uri };
+		}
 		const targetDurableObject = version.resources.bindings.find(
 			(durableObject) =>
 				durableObject.type === "durable_object_namespace" &&
@@ -94,7 +88,7 @@ export async function deployContainers(
 
 		await apply(
 			{
-				newImageLink: buildResult.newImageLink,
+				imageRef,
 				durable_object_namespace_id: targetDurableObject.namespace_id,
 			},
 			container,

packages/wrangler/src/containers/deploy.ts

@@ -2,7 +2,6 @@
  * Note! Much of this is copied and modified from cloudchamber/apply.ts
  * However this code is only used for containers interactions, not cloudchamber ones!
  */
-import assert from "node:assert";
 import {
 	endSection,
 	log,
@@ -30,6 +29,7 @@ import {
 	sortObjectRecursive,
 	stripUndefined,
 } from "../utils/sortObjectRecursive";
+import type { ImageRef } from "../cloudchamber/build";
 import type { Config } from "../config";
 import type { ContainerApp } from "../config/environment";
 import type {
@@ -191,12 +191,7 @@ function containerConfigToCreateRequest(
 
 export async function apply(
 	args: {
-		imageUpdateRequired?: boolean;
-		/**
-		 * If the image was built and pushed, or is a registry link, we have to update the image ref and this will be defined
-		 * If it is undefined, the image has not changed, and we do not need to update the image ref
-		 */
-		newImageLink: string | undefined;
+		imageRef: ImageRef;
 		durable_object_namespace_id: string;
 	},
 	containerConfig: ContainerNormalizedConfig,
@@ -224,14 +219,16 @@ export async function apply(
 		(app) => app.name === containerConfig.name
 	);
 
+	// if there is a remote digest, it indicates that the image already exists in the managed registry
+	// so we should try and use the tag from the previous deployment if possible.
+	// however deployments that fail after push may result in no previous app but the image still existing
+	const imageRef =
+		"remoteDigest" in args.imageRef
+			? prevApp?.configuration.image ?? args.imageRef.remoteDigest
+			: args.imageRef.newTag;
 	log(dim("Container application changes\n"));
 
 	const accountId = config.account_id || (await getAccountId(config));
-	const imageRef = args.newImageLink ?? prevApp?.configuration.image;
-
-	// image ref is undefined if the image is a dockerfile and has not changed since the last deploy
-	// if image ref is undefined and there is no previous app, something weird has happened.
-	assert(imageRef, "No changes detected but no previous image found");
 
 	// let's always convert normalised container config -> CreateApplicationRequest
 	// since CreateApplicationRequest is a superset of ModifyApplicationRequestBody

packages/wrangler/src/deploy/deploy.ts

@@ -8,7 +8,7 @@ import PQueue from "p-queue";
 import { Response } from "undici";
 import { syncAssets } from "../assets";
 import { fetchListResult, fetchResult } from "../cfetch";
-import { deployContainers, maybeBuildContainer } from "../cloudchamber/deploy";
+import { buildContainer, deployContainers } from "../cloudchamber/deploy";
 import { configFileName, formatConfigSnippet } from "../config";
 import { getNormalizedContainerOptions } from "../containers/config";
 import { getBindings, provisionBindings } from "../deployment-bundle/bindings";
@@ -797,12 +797,14 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m
 		if (props.dryRun) {
 			if (normalisedContainerConfig.length) {
 				for (const container of normalisedContainerConfig) {
-					await maybeBuildContainer(
-						container,
-						workerTag ?? "worker-tag",
-						props.dryRun,
-						dockerPath
-					);
+					if ("dockerfile" in container) {
+						await buildContainer(
+							container,
+							workerTag ?? "worker-tag",
+							props.dryRun,
+							dockerPath
+						);
+					}
 				}
 			}