refactor containers config handling (#9961)

* 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

* validate and normalise app name, constraints and observability too in config normalisation function

* bring back dev specific options type

* resolve image path in readConfig (main validation func)

* fix custom instance limit units and add test

* PR feedback

* rebase fixups

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,14 @@
 import { spawn } from "child_process";
 import { readFileSync } from "fs";
-import path from "path";
-import type { BuildArgs, ContainerDevOptions, Logger } from "./types";
+import type {
+	BuildArgs,
+	ContainerDevOptions,
+	ImageURIConfig,
+	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 +29,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 +93,15 @@ export function dockerBuild(
 
 export async function buildImage(
 	dockerPath: string,
-	options: ContainerDevOptions,
-	configPath: string | undefined
+	options: Exclude<ContainerDevOptions, ImageURIConfig>
 ) {
-	// 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: options.image_tag,
+		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

@@ -4,30 +4,33 @@ import {
 	isCloudflareRegistryLink,
 } from "./knobs";
 import { dockerLoginManagedRegistry } from "./login";
-import { type ContainerDevOptions } from "./types";
 import {
 	checkExposedPorts,
-	isDockerfile,
 	runDockerCmd,
 	verifyDockerInstalled,
 } from "./utils";
+import type { ContainerDevOptions, DockerfileConfig } from "./types";
 
 export async function pullImage(
 	dockerPath: string,
-	options: ContainerDevOptions
+	options: Exclude<ContainerDevOptions, DockerfileConfig>
 ): Promise<{ abort: () => void; ready: Promise<void> }> {
 	await dockerLoginManagedRegistry(dockerPath);
 	const pull = runDockerCmd(dockerPath, [
 		"pull",
-		options.image,
+		options.image_uri,
 		// 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.image_uri,
+				options.image_tag,
+			]);
 		}
 	});
 
@@ -49,9 +52,8 @@ export async function pullImage(
  * such as checking if the Docker CLI is installed, and if the container images
  * expose any ports.
  */
-export async function prepareContainerImagesForDev(options: {
+export async function prepareContainerImagesForDev(args: {
 	dockerPath: string;
-	configPath?: string;
 	containerOptions: ContainerDevOptions[];
 	onContainerImagePreparationStart: (args: {
 		containerOptions: ContainerDevOptions;
@@ -63,11 +65,10 @@ export async function prepareContainerImagesForDev(options: {
 }) {
 	const {
 		dockerPath,
-		configPath,
 		containerOptions,
 		onContainerImagePreparationStart,
 		onContainerImagePreparationEnd,
-	} = options;
+	} = args;
 	let aborted = false;
 	if (process.platform === "win32") {
 		throw new Error(
@@ -76,8 +77,8 @@ 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);
+		if ("dockerfile" in options) {
+			const build = await buildImage(dockerPath, options);
 			onContainerImagePreparationStart({
 				containerOptions: options,
 				abort: () => {
@@ -90,9 +91,9 @@ export async function prepareContainerImagesForDev(options: {
 				containerOptions: options,
 			});
 		} else {
-			if (!isCloudflareRegistryLink(options.image)) {
+			if (!isCloudflareRegistryLink(options.image_uri)) {
 				throw new Error(
-					`Image "${options.image}" is a registry link but does not point to the Cloudflare container registry.\n` +
+					`Image "${options.image_uri}" 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`
 				);
 			}

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,60 @@ export type BuildArgs = {
 	setNetworkToHost?: boolean;
 };
 
+export type ContainerNormalizedConfig = SharedContainerConfig &
+	(ImageURIConfig | DockerfileConfig);
+export type DockerfileConfig = {
+	/** 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 ImageURIConfig = {
+	image_uri: 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;
+	/** 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;
+	};
+	observability: { logs_enabled: boolean };
+} & InstanceTypeOrLimits;
+
 /** 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;
+	image_tag: string;
 	/** container's DO class name */
 	class_name: string;
-	imageBuildContext?: string;
-	/** build time args */
-	args?: Record<string, string>;
-};
+} & (DockerfileConfig | ImageURIConfig);

packages/containers-shared/src/types.ts

@@ -3,7 +3,7 @@ import { randomUUID } from "crypto";
 import { existsSync, statSync } from "fs";
 import path from "path";
 import { dockerImageInspect } from "./inspect";
-import { type ContainerDevOptions } from "./types";
+import type { ContainerDevOptions } from "./types";
 import type { StdioOptions } from "child_process";
 
 /** helper for simple docker command call that don't require any io handling */
@@ -57,7 +57,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 +96,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 +176,7 @@ export const cleanupContainers = async (
 			["inherit", "pipe", "pipe"]
 		);
 		return true;
-	} catch (error) {
+	} catch {
 		return false;
 	}
 };
@@ -233,7 +233,7 @@ export async function checkExposedPorts(
 	options: ContainerDevOptions
 ) {
 	const output = await dockerImageInspect(dockerPath, {
-		imageTag: options.imageTag,
+		imageTag: options.image_tag,
 		formatString: "{{ len .Config.ExposedPorts }}",
 	});
 	if (output === "0") {

packages/containers-shared/src/utils.ts

@@ -1,6 +1,7 @@
 import { mkdirSync, writeFileSync } from "fs";
 import { checkExposedPorts, isDockerfile } from "./../src/utils";
 import { runInTempDir } from "./helpers/run-in-tmp-dir";
+import type { ContainerDevOptions } from "../src/types";
 
 describe("isDockerfile", () => {
 	const dockerfile =
@@ -71,6 +72,10 @@ vi.mock("../src/inspect", async (importOriginal) => {
 	};
 });
 
+const containerConfig = {
+	dockerfile: "",
+	class_name: "MyContainer",
+} as ContainerDevOptions;
 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)
 		).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(`
+		await expect(checkExposedPorts("docker", containerConfig)).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"]