Dramatically improve docker-compose interception UX

This moves from remapping the project name label to instead remapping
the labels that hold the config hash. That means that all docker-compose
commands (intercepted or not) are now working on the same state, with
the only difference being that each one considers containers created by
the other to need recreating. This is great! No more conflicts, no
leftover container usage, and much easier container management, with a
simpler implementation. What a win!

Author: pimterry

src/interceptors/docker/docker-commands.ts

@@ -8,6 +8,7 @@ import {
     getTerminalEnvVars,
     OVERRIDES_DIR
 } from '../terminal/terminal-env-overrides';
+import { transformComposeCreationLabels } from './docker-compose';
 
 // Used to label intercepted docker containers with the port of the proxy
 // that's currently intercepting them.
@@ -215,15 +216,7 @@ export function transformContainerCreationConfig(
             )
         ],
         Labels: {
-            ...currentConfig.Labels,
-            // If there's a docker-compose project label, append our suffix. This ensures that normal
-            // DC commands don't use intercepted containers, and vice versa.
-            ...(currentConfig.Labels?.['com.docker.compose.project']
-                ? { 'com.docker.compose.project':
-                    `${currentConfig.Labels['com.docker.compose.project']}_HTK:${proxyPort}`
-                }
-                : {}
-            ),
+            ...transformComposeCreationLabels(proxyPort, currentConfig.Labels),
             // Label the resulting container as intercepted by this specific proxy:
             [DOCKER_CONTAINER_LABEL]: String(proxyPort)
         }

src/interceptors/docker/docker-compose.ts

@@ -0,0 +1,36 @@
+const interceptionSuffix = (proxyPort: number) => `+httptoolkit:${proxyPort}`;
+
+// Take a set of labels provided by a client for a new container that we're intercepting, and remap the config
+// hashes so this can be used in future docker-compose commands automatically:
+export function transformComposeCreationLabels(proxyPort: number, labels: { [label: string]: string } | undefined) {
+    // No/null labels - we don't need to do anything
+    if (!labels) return undefined;
+
+    // Not a docker-compose container - nothing to do here
+    if (!labels['com.docker.compose.config-hash']) return labels;
+
+    return {
+        ...labels,
+        'com.docker.compose.config-hash': labels['com.docker.compose.config-hash'] + interceptionSuffix(proxyPort)
+    };
+}
+
+// Take a set of labels that we might be returning to a client, and remap the config hashes so that only
+// the intercepted containers seem to be usable (so that any non-intercepted containers are recreated, not used).
+export function transformComposeResponseLabels(proxyPort: number, labels: { [label: string]: string } | undefined) {
+    // No/null labels - we don't need to do anything
+    if (!labels) return undefined;
+
+    // Not a docker-compose container - nothing to do here
+    if (!labels['com.docker.compose.config-hash']) return labels;
+
+    const currentHash = labels['com.docker.compose.config-hash'];
+    const modifiedHash = currentHash.endsWith(interceptionSuffix(proxyPort))
+        ? currentHash.slice(0, -1 * interceptionSuffix(proxyPort).length)
+        : currentHash + '+unintercepted';
+
+    return {
+        ...labels,
+        'com.docker.compose.config-hash': modifiedHash
+    };
+}

src/interceptors/docker/docker-networking.ts

@@ -367,7 +367,7 @@ class DockerNetworkMonitor {
                 const linkStrings: string[] = container.HostConfig.Links || [];
                 const linkAliases = await Promise.all(linkStrings.map(async (link) => {
                     // Aliases are of the form:
-                    // /compose_default-service-a_1_HTK8000:/compose_linked-service-b_1_HTK8000/a
+                    // /compose_default-service-a_1:/compose_linked-service-b_1/a
                     // I.e. service-a is linked by service-b with alias 'a'.
                     const endOfContainerName = link.indexOf(':/');
                     const aliasIndex = link.lastIndexOf('/');

src/interceptors/docker/docker-proxy.ts

@@ -22,6 +22,7 @@ import {
 } from './docker-commands';
 import { injectIntoBuildStream, getBuildOutputPipeline } from './docker-build-injection';
 import { ensureDockerServicesRunning, isDockerAvailable } from './docker-interception-services';
+import { transformComposeResponseLabels } from './docker-compose';
 
 export const getDockerPipePath = (proxyPort: number, targetPlatform: NodeJS.Platform = process.platform) => {
     if (targetPlatform === 'win32') {
@@ -47,6 +48,7 @@ const BUILD_IMAGE_MATCHER = /^\/[^\/]+\/build$/;
 const EVENTS_MATCHER = /^\/[^\/]+\/events$/;
 const ATTACH_CONTAINER_MATCHER = /^\/[^\/]+\/containers\/([^\/]+)\/attach/;
 const CONTAINER_LIST_MATCHER = /^\/[^\/]+\/containers\/json/;
+const CONTAINER_INSPECT_MATCHER = /^\/[^\/]+\/containers\/[^\/]+\/json/;
 
 const DOCKER_PROXY_MAP: { [mockServerPort: number]: Promise<DestroyableServer> | undefined } = {};
 
@@ -144,41 +146,6 @@ async function createDockerProxy(proxyPort: number, httpsConfig: { certPath: str
                 }
             );
             requestBodyStream = stream.Readable.from(JSON.stringify(transformedConfig));
-
-            // If you specify an explicit name in cases that will cause conflicts (when a container already
-            // exists, or if you're using docker-compose) we try to create a separate container instead,
-            // that uses a _HTK$PORT suffix to avoid conflicts. This happens commonly, because of how
-            // docker-compose automatically generates container names.
-            const containerName = reqUrl.searchParams.get('name');
-            if (containerName) {
-                const existingContainer = await docker.getContainer(containerName).inspect()
-                    .catch<false>(() => false);
-
-                if (existingContainer && existingContainer.State.Running) {
-                    // If there's a duplicate running container, we could rename the new one, but instead we return
-                    // an error. It's likely that this will create conflicts otherwise - e.g. two running containers
-                    // using the same volumes or the same network aliases. Better to play it safe.
-                    res.statusCode = 409;
-                    res.end(JSON.stringify({
-                        "message": `Conflict. The container name ${
-                            containerName
-                        } is already in use by a running container.\n${''
-                        }HTTP Toolkit won't intercept this by default to avoid conflicts over shared resources. ${''
-                        }To create & intercept this container, either stop the existing unintercepted container, or use a different container name.`
-                    }));
-                    return;
-                } else if (existingContainer || hasDockerComposeLabels) {
-                    // If there's a naming conflict, and we can safely work around it (because the container isn't
-                    // running) then we do so.
-
-                    // For Docker-Compose, we *always* rewrite names. This ensures that subsequent Docker-Compose usage
-                    // outside intercepted usage doesn't run into naming conflicts (however, this still only applies
-                    // after checking for running containers - we never create a duplicate parallel container ourselves)
-
-                    reqUrl.searchParams.set('name', `${containerName}_HTK${proxyPort}`);
-                    req.url = reqUrl.toString();
-                }
-            }
         }
 
         // Intercept container creation (e.g. docker start):
@@ -193,70 +160,6 @@ async function createDockerProxy(proxyPort: number, httpsConfig: { certPath: str
                     "The container must be recreated here first - try `docker run <image>` instead."
                 );
             }
-
-            if (containerData.Name.endsWith(`_HTK${proxyPort}`)) {
-                // Trim initial slash and our HTK suffix:
-                const clonedContainerName = containerData.Name.slice(1, -1 * `_HTK${proxyPort}`.length);
-                const clonedContainerData = await docker.getContainer(clonedContainerName)
-                    .inspect()
-                    .catch(() => undefined);
-
-                if (clonedContainerData && clonedContainerData.State.Running) {
-                    // If you successfully intercept a docker-compose container, stop it & start the original container(s),
-                    // and then restart the already-created intercepted container, you could risk conflicts, so we warn you:
-                    res.writeHead(409).end(
-                        `Conflict: an unintercepted container with the same base name is already running.\n${''
-                        }HTTP Toolkit won't launch intercepted containers in parallel by default to avoid conflicts ${''
-                        }over shared resources. To create & intercept this container, either stop the existing ${''
-                        }unintercepted container, or use a different container name.`
-
-                    );
-                }
-            }
-        }
-
-        if (
-            reqPath.match(CONTAINER_LIST_MATCHER) ||
-            reqPath.match(EVENTS_MATCHER)
-        ) {
-            const filterString = reqUrl.searchParams.get('filters') ?? '{}';
-
-            try {
-                const filters = JSON.parse(filterString) as {
-                    // Docs say only string[] is allowed, but Docker CLI uses bool maps in "docker compose"
-                    [key: string]: string[] | { [key: string]: boolean }
-                };
-                const labelFilters = (
-                    _.isArray(filters.label)
-                        ? filters.label
-                        : Object.keys(filters.label ?? {})
-                            .filter(key => !!(filters.label as _.Dictionary<boolean>)[key])
-                );
-                const projectFilter = labelFilters.filter(l => l.startsWith("com.docker.compose.project="))[0];
-
-                if (projectFilter) {
-                    const project = projectFilter.slice(projectFilter.indexOf('=') + 1);
-
-                    // This is a request from docker-compose, looking for containers related to a
-                    // specific project. Add an extra filter so that it only finds *intercepted*
-                    // containers. This ensures it'll recreate any unintercepted containers.
-                    reqUrl.searchParams.set('filters', JSON.stringify({
-                        ...filters,
-                        label: [
-                            // Replace the project filter, to ensure that the intercepted & non-intercepted containers
-                            // are handled separately. We need to ensure that a) this request only finds intercepted
-                            // containers, and b) future non-proxied requests only find non-intercepted containers.
-                            // By excluding non-intercepted containers, we force DC to recreate, so we can then
-                            // intercept the container creation itself and inject what we need.
-                            ...labelFilters.filter((label) => label !== projectFilter),
-                            `com.docker.compose.project=${project}_HTK:${proxyPort}`
-                        ]
-                    }));
-                    req.url = reqUrl.toString();
-                }
-            } catch (e) {
-                console.log("Could not parse /containers/json filters param", e);
-            }
         }
 
         let extraDockerCommandCount: Promise<number> | undefined;
@@ -304,23 +207,73 @@ async function createDockerProxy(proxyPort: number, httpsConfig: { certPath: str
         });
 
         dockerReq.on('response', async (dockerRes) => {
+            res.on('error', (e) => {
+                console.error('Docker proxy conn error', e);
+                dockerRes.destroy();
+            });
+
+            // In any container data responses that might be used by docker-compose, we need to remap some of the
+            // content to ensure that intercepted containers are always used:
+            const isContainerInspect = reqPath.match(CONTAINER_INSPECT_MATCHER);
+            const isComposeContainerQuery = reqPath.match(CONTAINER_LIST_MATCHER) &&
+                reqUrl.searchParams.get('filters')?.includes("com.docker.compose");
+            const shouldRemapContainerData = isContainerInspect || isComposeContainerQuery;
+
+            if (shouldRemapContainerData) {
+                // We're going to mess with the body, so we need to ensure that the content
+                // length isn't going to conflict along the way:
+                delete dockerRes.headers['content-length'];
+            }
+
             res.writeHead(
                 dockerRes.statusCode!,
                 dockerRes.statusMessage,
                 dockerRes.headers
             );
-            res.on('error', (e) => {
-                console.error('Docker proxy conn error', e);
-                dockerRes.destroy();
-            });
+            res.flushHeaders(); // Required, or blocking responses (/wait) don't work!
 
             if (reqPath.match(BUILD_IMAGE_MATCHER) && dockerRes.statusCode === 200) {
+                // We transform the build output to replace the docker build interception steps with a cleaner
+                // & simpler HTTP Toolkit interception message:
                 dockerRes.pipe(getBuildOutputPipeline(await extraDockerCommandCount!)).pipe(res);
+            } else if (shouldRemapContainerData) {
+                // We need to remap container data, to hook all docker-compose behaviour:
+                const data = await new Promise<Buffer>((resolve, reject) => {
+                    const dataChunks: Buffer[] = [];
+                    dockerRes.on('data', (d) => dataChunks.push(d));
+                    dockerRes.on('end', () => resolve(Buffer.concat(dataChunks)));
+                    dockerRes.on('error', reject);
+                });
+
+                try {
+                    if (isComposeContainerQuery) {
+                        const containerQueryResponse: Dockerode.ContainerInfo[] = JSON.parse(data.toString('utf8'));
+                        const modifiedResponse = containerQueryResponse.map((container) => ({
+                            ...container,
+                            Labels: transformComposeResponseLabels(proxyPort, container.Labels)
+                        }));
+
+                        res.end(JSON.stringify(modifiedResponse));
+                    } else {
+                        const containerInspectResponse: Dockerode.ContainerInspectInfo = JSON.parse(data.toString('utf8'));
+                        const modifiedResponse = {
+                            ...containerInspectResponse,
+                            Config: {
+                                ...containerInspectResponse.Config,
+                                Labels: transformComposeResponseLabels(proxyPort, containerInspectResponse.Config?.Labels)
+                            }
+                        };
+
+                        res.end(JSON.stringify(modifiedResponse));
+                    }
+                } catch (e) {
+                    console.error("Failed to parse container data response", e);
+                    // Write the raw body back to the response - effectively just do nothing.
+                    res.end(data);
+                }
             } else {
                 dockerRes.pipe(res);
             }
-
-            res.flushHeaders(); // Required, or blocking responses (/wait) don't work!
         });
     });
 

test/interceptors/docker-terminal-interception.spec.ts

@@ -282,15 +282,15 @@ Successfully built <hash>
             .replace(/\s+\|/g, ' |'); // Strip container name variable whitespace
 
         [
-            `compose_host_1_HTK${server.port}`,
-            `compose_default-service-a_1_HTK${server.port}`,
-            `compose_default-linked-service-b_1_HTK${server.port}`,
-            `compose_multi-network-a_1_HTK${server.port}`,
-            `compose_multi-network-b_1_HTK${server.port}`
+            `host_1`,
+            `default-service-a_1`,
+            `default-linked-service-b_1`,
+            `multi-network-a_1`,
+            `multi-network-b_1`
         ].forEach((container) => {
             expect(dcOutput).to.include(`${container} | All requests ok`);
         });
-        expect(dcOutput).to.include(`compose_none_1_HTK${server.port} | Skipping`);
+        expect(dcOutput).to.include(`none_1 | Skipping`);
     });
 
     it("should clean up containers after shutdown", async () => {