Improve containers exposed port validation logic (#9879)

* update containers error message presented when no port is exported by the image

* fix: enable Dockerfile exposed port validation on linux as well

* update e2e

Author: dario-piotrowicz

.changeset/rotten-peaches-end.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/containers-shared": patch
+---
+
+fix: enable Dockerfile exposed port validation on linux as well

.changeset/shiny-snails-bake.md

@@ -0,0 +1,5 @@
+---
+"@cloudflare/containers-shared": patch
+---
+
+update error message presented when no port is exported by the image

packages/containers-shared/src/utils.ts

@@ -166,7 +166,7 @@ const getContainerIdsFromImage = async (
 
 /**
  * While all ports are exposed in prod, a limitation of local dev with docker is that
- * non-linux users will have to manually expose ports in their Dockerfile.
+ * users will have to manually expose ports in their Dockerfile.
  * We want to fail early and clearly if a user tries to develop with a container
  * that has no ports exposed and is definitely not accessible.
  *
@@ -180,10 +180,10 @@ export async function checkExposedPorts(
 		imageTag: options.imageTag,
 		formatString: "{{ len .Config.ExposedPorts }}",
 	});
-	if (output === "0" && process.platform !== "linux") {
+	if (output === "0") {
 		throw new Error(
-			`The container "${options.class_name}" does not expose any ports.\n` +
-				"To develop containers locally on non-Linux platforms, you must expose any ports that you call with `getTCPPort()` in your Dockerfile."
+			`The container "${options.class_name}" does not expose any ports. In your Dockerfile, please expose any ports you intend to connect to.\n` +
+				"For additional information please see: https://developers.cloudflare.com/containers/local-dev/#exposing-ports.\n"
 		);
 	}
 }

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

@@ -1,5 +1,5 @@
-import { mkdirSync, rmSync, writeFileSync } from "fs";
-import { isDockerfile } from "./../src/utils";
+import { mkdirSync, writeFileSync } from "fs";
+import { checkExposedPorts, isDockerfile } from "./../src/utils";
 import { runInTempDir } from "./helpers/run-in-tmp-dir";
 
 describe("isDockerfile", () => {
@@ -60,3 +60,45 @@ describe("isDockerfile", () => {
 			`);
 	});
 });
+
+let docketImageInspectResult = "0";
+
+vi.mock("../src/inspect", async (importOriginal) => {
+	const mod: object = await importOriginal();
+	return {
+		...mod,
+		dockerImageInspect: () => docketImageInspectResult,
+	};
+});
+
+describe("checkExposedPorts", () => {
+	beforeEach(() => {
+		docketImageInspectResult = "1";
+	});
+
+	it("should not error when some ports are exported", async () => {
+		docketImageInspectResult = "1";
+		await expect(
+			checkExposedPorts("./container-context/Dockerfile", {
+				image: "",
+				imageTag: "",
+				class_name: "MyContainer",
+			})
+		).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(`
+				[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/wrangler/e2e/containers.dev.test.ts

@@ -263,43 +263,21 @@ describe
 			);
 		});
 
-		describe("make sure ports are exposed if necessary", () => {
-			beforeEach(async () => {
-				await helper.seed({
-					Dockerfile: dedent`
-					FROM alpine:latest
-					CMD ["echo", "hello world"]
-					`,
-				});
-				if (source === "pull") {
-					await helper.run(
-						`wrangler containers build . -t ${workerName}:tmp-e2e -p`
-					);
-				}
+		it("errors if no ports are exposed", async () => {
+			await helper.seed({
+				Dockerfile: dedent`
+							FROM alpine:latest
+							CMD ["echo", "hello world"]
+							`,
 			});
-			// this will never run in CI
-			it.skipIf(process.platform === "linux")(
-				"errors in windows/macos if no ports are exposed",
-				async () => {
-					const worker = helper.runLongLived("wrangler dev");
-					expect(await worker.exitCode).toBe(1);
-					expect(await worker.output).toContain("does not expose any ports");
-				}
-			);
-
-			it.skipIf(process.platform !== "linux")(
-				"doesn't error in linux if no ports are exposed",
-				async () => {
-					const worker = helper.runLongLived("wrangler dev");
-					await worker.readUntil(/Preparing container/);
-					if (source === "pull") {
-						await worker.readUntil(/Status/);
-					} else {
-						await worker.readUntil(/DONE/);
-					}
-					await worker.readUntil(/Container image\(s\) ready/);
-				}
-			);
+			if (source === "pull") {
+				await helper.run(
+					`wrangler containers build . -t ${workerName}:tmp-e2e -p`
+				);
+			}
+			const worker = helper.runLongLived("wrangler dev");
+			expect(await worker.exitCode).toBe(1);
+			expect(await worker.output).toContain("does not expose any ports");
 		});
 
 		it("errors if docker is not installed", async () => {