refactor containers config handling

Unfortunately I had to squash this to fix merge conflicts, so this commit contains:

* add config normalisation function

* refactor dev to use normalised config

* refactor containers deploy to use normalised config

* consolidate tests into containers/deploy.test.ts

* fix up loop in build command and config handling in ensureDiskSize

* stray fixups for cloudchamber apply

Author: emily-shen

packages/containers-shared/.eslintrc.js

@@ -0,0 +1,4 @@
+module.exports = {
+	root: true,
+	extends: ["@cloudflare/eslint-config-worker"],
+};

packages/containers-shared/.eslintrc.json

@@ -1,12 +1,9 @@
 import { spawn } from "child_process";
 import { readFileSync } from "fs";
-import path from "path";
-import type { BuildArgs, ContainerDevOptions, Logger } from "./types";
+import type { BuildArgs, DockerfileConfig, Logger } from "./types";
 
 export async function constructBuildCommand(
 	options: BuildArgs,
-	/** wrangler config path. used to resolve relative dockerfile path */
-	configPath: string | undefined,
 	logger?: Logger
 ) {
 	const platform = options.platform ?? "linux/amd64";
@@ -27,16 +24,11 @@ export async function constructBuildCommand(
 	if (options.setNetworkToHost) {
 		buildCmd.push("--network", "host");
 	}
-	const baseDir = configPath ? path.dirname(configPath) : process.cwd();
-	const absDockerfilePath = path.resolve(baseDir, options.pathToDockerfile);
-	const dockerfile = readFileSync(absDockerfilePath, "utf-8");
 
-	const absBuildContext = options.buildContext
-		? path.resolve(baseDir, options.buildContext)
-		: path.dirname(absDockerfilePath);
+	const dockerfile = readFileSync(options.pathToDockerfile, "utf-8");
 	// pipe in the dockerfile
 	buildCmd.push("-f", "-");
-	buildCmd.push(absBuildContext);
+	buildCmd.push(options.buildContext);
 	logger?.debug(`Building image with command: ${buildCmd.join(" ")}`);
 	return { buildCmd, dockerfile };
 }
@@ -96,20 +88,16 @@ export function dockerBuild(
 
 export async function buildImage(
 	dockerPath: string,
-	options: ContainerDevOptions,
-	configPath: string | undefined
+	options: DockerfileConfig,
+	imageTag: string
 ) {
-	// just let the tag default to latest
-	const { buildCmd, dockerfile } = await constructBuildCommand(
-		{
-			tag: options.imageTag,
-			pathToDockerfile: options.image,
-			buildContext: options.imageBuildContext,
-			args: options.args,
-			platform: "linux/amd64",
-		},
-		configPath
-	);
+	const { buildCmd, dockerfile } = await constructBuildCommand({
+		tag: imageTag,
+		pathToDockerfile: options.dockerfile,
+		buildContext: options.image_build_context,
+		args: options.image_vars,
+		platform: "linux/amd64",
+	});
 
 	return dockerBuild(dockerPath, { buildCmd, dockerfile });
 }

packages/containers-shared/src/build.ts

@@ -1,33 +1,34 @@
 import { buildImage } from "./build";
 import {
 	getCloudflareContainerRegistry,
+	getDevContainerImageName,
 	isCloudflareRegistryLink,
 } from "./knobs";
 import { dockerLoginManagedRegistry } from "./login";
-import { type ContainerDevOptions } from "./types";
 import {
 	checkExposedPorts,
-	isDockerfile,
 	runDockerCmd,
 	verifyDockerInstalled,
 } from "./utils";
+import type { ContainerNormalisedConfig, RegistryLinkConfig } from "./types";
 
 export async function pullImage(
 	dockerPath: string,
-	options: ContainerDevOptions
+	options: RegistryLinkConfig,
+	tag: string
 ): Promise<{ abort: () => void; ready: Promise<void> }> {
 	await dockerLoginManagedRegistry(dockerPath);
 	const pull = runDockerCmd(dockerPath, [
 		"pull",
-		options.image,
+		options.registry_link,
 		// All containers running on our platform need to be built for amd64 architecture, but by default docker pull seems to look for an image matching the host system, so we need to specify this here
 		"--platform",
 		"linux/amd64",
 	]);
 	const ready = pull.ready.then(async ({ aborted }: { aborted: boolean }) => {
 		if (!aborted) {
 			// re-tag image with the expected dev-formatted image tag for consistency
-			await runDockerCmd(dockerPath, ["tag", options.image, options.imageTag]);
+			await runDockerCmd(dockerPath, ["tag", options.registry_link, tag]);
 		}
 	});
 
@@ -51,19 +52,17 @@ export async function pullImage(
  */
 export async function prepareContainerImagesForDev(options: {
 	dockerPath: string;
-	configPath?: string;
-	containerOptions: ContainerDevOptions[];
+	containerOptions: ContainerNormalisedConfig[];
 	onContainerImagePreparationStart: (args: {
-		containerOptions: ContainerDevOptions;
+		containerOptions: ContainerNormalisedConfig;
 		abort: () => void;
 	}) => void;
 	onContainerImagePreparationEnd: (args: {
-		containerOptions: ContainerDevOptions;
+		containerOptions: ContainerNormalisedConfig[];
 	}) => void;
 }) {
 	const {
 		dockerPath,
-		configPath,
 		containerOptions,
 		onContainerImagePreparationStart,
 		onContainerImagePreparationEnd,
@@ -76,8 +75,9 @@ export async function prepareContainerImagesForDev(options: {
 	}
 	await verifyDockerInstalled(dockerPath);
 	for (const options of containerOptions) {
-		if (isDockerfile(options.image, configPath)) {
-			const build = await buildImage(dockerPath, options, configPath);
+		const tag = getDevContainerImageName(options.class_name, containerBuildId);
+		if ("dockerfile" in options) {
+			const build = await buildImage(dockerPath, options, tag);
 			onContainerImagePreparationStart({
 				containerOptions: options,
 				abort: () => {
@@ -90,13 +90,13 @@ export async function prepareContainerImagesForDev(options: {
 				containerOptions: options,
 			});
 		} else {
-			if (!isCloudflareRegistryLink(options.image)) {
+			if (!isCloudflareRegistryLink(options.registry_link)) {
 				throw new Error(
-					`Image "${options.image}" is a registry link but does not point to the Cloudflare container registry.\n` +
+					`Image "${options.registry_link}" is a registry link but does not point to the Cloudflare container registry.\n` +
 						`To use an existing image from another repository, see https://developers.cloudflare.com/containers/image-management/#using-existing-images`
 				);
 			}
-			const pull = await pullImage(dockerPath, options);
+			const pull = await pullImage(dockerPath, options, tag);
 			onContainerImagePreparationStart({
 				containerOptions: options,
 				abort: () => {
@@ -110,7 +110,7 @@ export async function prepareContainerImagesForDev(options: {
 			});
 		}
 		if (!aborted) {
-			await checkExposedPorts(dockerPath, options);
+			await checkExposedPorts(dockerPath, options, tag);
 		}
 	}
 }

packages/containers-shared/src/images.ts

@@ -1,3 +1,5 @@
+import type { InstanceType, SchedulingPolicy } from "./client";
+
 export interface Logger {
 	debug: (message: string) => void;
 	log: (message: string) => void;
@@ -11,7 +13,7 @@ export type BuildArgs = {
 	tag: string;
 	pathToDockerfile: string;
 	/** image_build_context or args.PATH. if not provided, defaults to the dockerfile directory */
-	buildContext?: string;
+	buildContext: string;
 	/** any env vars that should be passed in at build time */
 	args?: Record<string, string>;
 	/** platform to build for. defaults to linux/amd64 */
@@ -20,15 +22,50 @@ export type BuildArgs = {
 	setNetworkToHost?: boolean;
 };
 
-/** build/pull agnostic container options */
-export type ContainerDevOptions = {
-	/** may be dockerfile or registry link */
-	image: string;
-	/** formatted as cloudflare-dev/workername-DOclassname:build-id */
-	imageTag: string;
+export type ContainerNormalisedConfig = RegistryLinkConfig | DockerfileConfig;
+export type DockerfileConfig = SharedContainerConfig & {
+	/**  absolute path, resolved relative to the wrangler config file */
+	dockerfile: string;
+	/** absolute path, resolved relative to the wrangler config file. defaults to the directory of the dockerfile */
+	image_build_context: string;
+	image_vars?: Record<string, string>;
+};
+export type RegistryLinkConfig = SharedContainerConfig & {
+	registry_link: string;
+};
+
+export type InstanceTypeOrLimits =
+	| {
+			/** if undefined in config, defaults to instance_type */
+			/** disk size is defined in config in mb but normalised here to bytes */
+			disk_bytes?: number;
+			vcpu?: number;
+			memory_mib?: number;
+	  }
+	| {
+			/** if undefined in config, defaults to "dev" */
+			instance_type: InstanceType;
+	  };
+
+/**
+ * Shared container config that is used regardless of whether the image is from a dockerfile or a registry link.
+ */
+export type SharedContainerConfig = {
+	/** if undefined in config, defaults to worker_name[-envName]-class_name. */
+	name: string;
 	/** container's DO class name */
 	class_name: string;
-	imageBuildContext?: string;
-	/** build time args */
-	args?: Record<string, string>;
-};
+	/** if undefined in config, defaults to 0 */
+	max_instances: number;
+	/** if undefined in config, defaults to "default" */
+	scheduling_policy: SchedulingPolicy;
+	/** if undefined in config, defaults to 25 */
+	rollout_step_percentage: number;
+	/** if undefined in config, defaults to "full_auto" */
+	rollout_kind: "full_auto" | "full_manual" | "none";
+	constraints?: {
+		regions?: string[];
+		cities?: string[];
+		tier?: number;
+	};
+} & InstanceTypeOrLimits;

packages/containers-shared/src/types.ts

@@ -1,9 +1,8 @@
 import { execFile, spawn } from "child_process";
-import { randomUUID } from "crypto";
 import { existsSync, statSync } from "fs";
 import path from "path";
 import { dockerImageInspect } from "./inspect";
-import { type ContainerDevOptions } from "./types";
+import type { ContainerNormalisedConfig } from "./types";
 import type { StdioOptions } from "child_process";
 
 /** helper for simple docker command call that don't require any io handling */
@@ -57,7 +56,7 @@ export const runDockerCmd = (
 			}
 		},
 		ready,
-		then: async (resolve, reject) => ready.then(resolve).catch(reject),
+		then: async (onResolve, onReject) => ready.then(onResolve).catch(onReject),
 	};
 };
 
@@ -96,8 +95,8 @@ export const verifyDockerInstalled = async (
 	}
 };
 
-export function isDir(path: string) {
-	const stats = statSync(path);
+export function isDir(inputPath: string) {
+	const stats = statSync(inputPath);
 	return stats.isDirectory();
 }
 
@@ -176,7 +175,7 @@ export const cleanupContainers = async (
 			["inherit", "pipe", "pipe"]
 		);
 		return true;
-	} catch (error) {
+	} catch {
 		return false;
 	}
 };
@@ -230,10 +229,11 @@ export const getContainerIdsFromImage = async (
  */
 export async function checkExposedPorts(
 	dockerPath: string,
-	options: ContainerDevOptions
+	options: ContainerNormalisedConfig,
+	imageTag: string
 ) {
 	const output = await dockerImageInspect(dockerPath, {
-		imageTag: options.imageTag,
+		imageTag: imageTag,
 		formatString: "{{ len .Config.ExposedPorts }}",
 	});
 	if (output === "0") {

packages/containers-shared/src/utils.ts

@@ -1,4 +1,5 @@
 import { mkdirSync, writeFileSync } from "fs";
+import { ContainerNormalisedConfig } from "../src/types";
 import { checkExposedPorts, isDockerfile } from "./../src/utils";
 import { runInTempDir } from "./helpers/run-in-tmp-dir";
 
@@ -71,6 +72,10 @@ vi.mock("../src/inspect", async (importOriginal) => {
 	};
 });
 
+const containerConfig = {
+	dockerfile: "",
+	class_name: "MyContainer",
+} as ContainerNormalisedConfig;
 describe("checkExposedPorts", () => {
 	beforeEach(() => {
 		docketImageInspectResult = "1";
@@ -79,23 +84,14 @@ describe("checkExposedPorts", () => {
 	it("should not error when some ports are exported", async () => {
 		docketImageInspectResult = "1";
 		await expect(
-			checkExposedPorts("./container-context/Dockerfile", {
-				image: "",
-				imageTag: "",
-				class_name: "MyContainer",
-			})
+			checkExposedPorts("docker", containerConfig, "image:tag")
 		).resolves.toBeUndefined();
 	});
 
 	it("should error, with an appropriate message when no ports are exported", async () => {
 		docketImageInspectResult = "0";
-		expect(
-			checkExposedPorts("./container-context/Dockerfile", {
-				image: "",
-				imageTag: "",
-				class_name: "MyContainer",
-			})
-		).rejects.toThrowErrorMatchingInlineSnapshot(`
+		expect(checkExposedPorts("docker", containerConfig, "image:tag")).rejects
+			.toThrowErrorMatchingInlineSnapshot(`
 				[Error: The container "MyContainer" does not expose any ports. In your Dockerfile, please expose any ports you intend to connect to.
 				For additional information please see: https://developers.cloudflare.com/containers/local-dev/#exposing-ports.
 				]

packages/containers-shared/tests/utils.test.ts

@@ -7,8 +7,8 @@
 		"allowSyntheticDefaultImports": true,
 		"experimentalDecorators": true,
 		"skipLibCheck": true,
-		"outDir": "./dist",
-		"types": ["node"]
+		"types": ["node"],
+		"noUncheckedIndexedAccess": true
 	},
 	"include": ["src/**/*", "index.ts"],
 	"exclude": ["dist", "node_modules"]