Skip to content

Add automation to catch potential issues with networks, volumes, bind mounts, and ports #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { z } from 'zod/v4';
import { InspectContainersItem, InspectContainersItemMount, InspectContainersItemNetwork, PortBinding } from '../../contracts/ContainerClient';
import { InspectContainersItem, InspectContainersItemBindMount, InspectContainersItemMount, InspectContainersItemNetwork, InspectContainersItemVolumeMount, PortBinding } from '../../contracts/ContainerClient';
import { dayjs } from '../../utils/dayjs';
import { parseDockerLikeImageName } from '../../utils/parseDockerLikeImageName';
import { toArray } from '../../utils/toArray';
Expand Down Expand Up @@ -95,7 +95,7 @@ export function normalizeDockerInspectContainerRecord(container: DockerInspectCo
gateway: dockerNetwork.Gateway || undefined,
ipAddress: dockerNetwork.IPAddress || undefined,
macAddress: dockerNetwork.MacAddress || undefined,
};
} satisfies InspectContainersItemNetwork;
});

// Parse the exposed ports for the container and normalize to a PortBinding record
Expand All @@ -110,7 +110,7 @@ export function normalizeDockerInspectContainerRecord(container: DockerInspectCo
: protocol.toLowerCase() === 'udp'
? 'udp'
: undefined,
};
} satisfies PortBinding;
});

// Parse the volume and bind mounts associated with the given runtime and normalize to
Expand All @@ -123,16 +123,15 @@ export function normalizeDockerInspectContainerRecord(container: DockerInspectCo
source: mount.Source,
destination: mount.Destination,
readOnly: !mount.RW,
}];
} satisfies InspectContainersItemBindMount];
case 'volume':
return [...curMounts, {
type: 'volume',
name: mount.Name,
source: mount.Source,
source: mount.Name,
Copy link
Contributor Author

@bwateratmsft bwateratmsft Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a longstanding bug. The description of source on InspectContainersItemVolumeMount is The source of the volume mount (volume name), we should have been using mount.Name, not mount.Source (which is an obscure path deep in the Docker data dirs and not human-friendly).

The same bug existed for the Podman client.

We would have caught this mistake ages ago if we'd used satisfies here, so I added it to prevent possible future mistakes.

destination: mount.Destination,
driver: mount.Driver,
readOnly: !mount.RW,
}];
} satisfies InspectContainersItemVolumeMount];
}

}, new Array<InspectContainersItemMount>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import { z } from 'zod/v4';
import { InspectContainersItem, InspectContainersItemMount, InspectContainersItemNetwork, PortBinding } from '../../contracts/ContainerClient';
import { InspectContainersItem, InspectContainersItemBindMount, InspectContainersItemMount, InspectContainersItemNetwork, InspectContainersItemVolumeMount, PortBinding } from '../../contracts/ContainerClient';
import { dayjs } from '../../utils/dayjs';
import { parseDockerLikeImageName } from '../../utils/parseDockerLikeImageName';
import { toArray } from '../../utils/toArray';
Expand Down Expand Up @@ -94,7 +94,7 @@ export function normalizePodmanInspectContainerRecord(container: PodmanInspectCo
gateway: dockerNetwork.Gateway || undefined,
ipAddress: dockerNetwork.IPAddress || undefined,
macAddress: dockerNetwork.MacAddress || undefined,
};
} satisfies InspectContainersItemNetwork;
});

// Parse the exposed ports for the container and normalize to a PortBinding record
Expand All @@ -109,7 +109,7 @@ export function normalizePodmanInspectContainerRecord(container: PodmanInspectCo
: protocol.toLowerCase() === 'udp'
? 'udp'
: undefined,
};
} satisfies PortBinding;
});

// Parse the volume and bind mounts associated with the given runtime and normalize to
Expand All @@ -122,16 +122,15 @@ export function normalizePodmanInspectContainerRecord(container: PodmanInspectCo
source: mount.Source,
destination: mount.Destination,
readOnly: !mount.RW,
}];
} satisfies InspectContainersItemBindMount];
case 'volume':
return [...curMounts, {
type: 'volume',
name: mount.Name,
source: mount.Source,
source: mount.Name,
destination: mount.Destination,
driver: mount.Driver,
readOnly: !mount.RW,
}];
} satisfies InspectContainersItemVolumeMount];
}

}, new Array<InspectContainersItemMount>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,25 @@ describe('(integration) ContainersClientE2E', function () {
describe('Containers', function () {
const imageToTest = 'alpine:latest';
const testContainerName = 'test-container-e2e';
const testContainerNetworkName = 'test-networkForContainer-e2e';
const testContainerVolumeName = 'test-volumeForContainer-e2e';
let testContainerBindMountSource: string;
let testContainerId: string;

before('Containers', async function () {
testContainerBindMountSource = __dirname;

// If running in WSL, convert the bind mount source path to WSL format
if (runInWsl) {
testContainerBindMountSource = wslifyPath(testContainerBindMountSource);
}

// Pull a small image for testing
await defaultRunner.getCommandRunner()(
client.pullImage({ imageRef: imageToTest })
);

// Try removing the container if it exists so we don't get a name conflict
// Try removing the container if it exists so we don't get a name/port conflict
try {
await defaultRunner.getCommandRunner()(
client.removeContainers({ containers: [testContainerName], force: true })
Expand All @@ -298,17 +308,52 @@ describe('(integration) ContainersClientE2E', function () {
// Ignore error if the container doesn't exist
}

// Try removing the network if it exists so we don't get a name conflict
try {
await defaultRunner.getCommandRunner()(
client.removeNetworks({ networks: [testContainerNetworkName] })
);
} catch (error) {
// Ignore error if the network doesn't exist
}

// Try removing the volume if it exists so we don't get a name conflict
try {
await defaultRunner.getCommandRunner()(
client.removeVolumes({ volumes: [testContainerVolumeName] })
);
} catch (error) {
// Ignore error if the volume doesn't exist
}

// Create a network for the container
await defaultRunner.getCommandRunner()(
client.createNetwork({ name: testContainerNetworkName })
);

// Create a volume for the container
await defaultRunner.getCommandRunner()(
client.createVolume({ name: testContainerVolumeName })
);

// Create a container that will stay running
// For fun we'll add a network, a bind mount, a volume, and some ports to it and verify those in both
// it('ListContainersCommand') and it('InspectContainersCommand')
testContainerId = await defaultRunner.getCommandRunner()(
client.runContainer({
imageRef: imageToTest,
detached: true,
name: testContainerName,
network: testContainerNetworkName,
mounts: [
{ type: 'bind', source: testContainerBindMountSource, destination: '/data1', readOnly: true },
{ type: 'volume', source: testContainerVolumeName, destination: '/data2', readOnly: false }
],
ports: [{ hostPort: 8080, containerPort: 80 }],
exposePorts: [3000], // Uses the `--expose` flag to expose a port without binding it
publishAllPorts: true, // Which will then get bound to a random port on the host, due to this flag
Comment on lines +352 to +354
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importantly, this is currently broken in Docker Desktop 4.42.x, unless you disable IPv6. See microsoft/vscode-containers#155 (comment).

})
) as string;

expect(testContainerId).to.be.a('string');
expect(await validateContainerExists(client, defaultRunner, { containerId: testContainerId })).to.be.ok;
Comment on lines -309 to -311
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just moved down into it('RunContainerCommand')

});

after('Containers', async function () {
Expand All @@ -320,19 +365,52 @@ describe('(integration) ContainersClientE2E', function () {
client.removeContainers({ containers: [testContainerId], force: true })
);
}

// Try removing the network if it exists so we don't get a name conflict
try {
await defaultRunner.getCommandRunner()(
client.removeNetworks({ networks: [testContainerNetworkName] })
);
} catch (error) {
// Ignore error if the network doesn't exist
}

// Try removing the volume if it exists so we don't get a name conflict
try {
await defaultRunner.getCommandRunner()(
client.removeVolumes({ volumes: [testContainerVolumeName] })
);
} catch (error) {
// Ignore error if the volume doesn't exist
}
});

it('RunContainerCommand', async function () {
// This is already fully tested in the before('Containers') hook and the it('StopContainersCommand') test
// Ensure the container was created
expect(testContainerId).to.be.a('string');
expect(await validateContainerExists(client, defaultRunner, { containerId: testContainerId })).to.be.ok;
});

it('ListContainersCommand', async function () {
const container = await validateContainerExists(client, defaultRunner, { containerId: testContainerId }) as ListContainersItem;
expect(container).to.be.ok;

// Validate some important properties
expect(container.name).to.equal(testContainerName);
expect(container.state).to.be.a('string');
expect(container.image).to.be.an('object');
expect(container.createdAt).to.be.instanceOf(Date);

// Validate the network
expect(container.networks).to.be.an('array');
expect(container.networks).to.include(testContainerNetworkName);

// Validate the ports
expect(container.ports).to.be.an('array');
expect(container.ports.some(p => p.hostPort === 8080 && p.containerPort === 80)).to.be.true;
expect(container.ports.some(p => p.containerPort === 3000 && !!p.hostPort && p.hostPort > 0 && p.hostPort < 65536)).to.be.true; // Exposed port with random binding

// Volumes and bind mounts do not show up in ListContainersCommand, so we won't validate those here
});

it('InspectContainersCommand', async function () {
Expand All @@ -345,16 +423,34 @@ describe('(integration) ContainersClientE2E', function () {
expect(containers.length).to.equal(1);

const container = containers[0];

// Validate some important properties
expect(container.id).to.equal(testContainerId);
expect(container.name).to.include(testContainerName);
expect(container.image).to.be.an('object');
expect(container.environmentVariables).to.be.an('object');
expect(container.ports).to.be.an('array');
expect(container.labels).to.be.an('object');
expect(container.entrypoint).to.be.an('array');
expect(container.command).to.be.an('array');
expect(container.createdAt).to.be.instanceOf(Date);
expect(container.raw).to.be.a('string');

// Validate the network
expect(container.networks).to.be.an('array');
expect(container.networks.some(n => n.name === testContainerNetworkName)).to.be.true;

// Validate the bind mount
expect(container.mounts).to.be.an('array');
expect(container.mounts.some(m => m.type === 'bind' && m.source === testContainerBindMountSource && m.destination === '/data1' && m.readOnly === true)).to.be.true;

// Validate the volume
expect(container.mounts).to.be.an('array');
expect(container.mounts.some(m => m.type === 'volume' && m.source === testContainerVolumeName && m.destination === '/data2' && m.readOnly === false)).to.be.true;

// Validate the ports
expect(container.ports).to.be.an('array');
expect(container.ports.some(p => p.hostPort === 8080 && p.containerPort === 80)).to.be.true;
expect(container.ports.some(p => p.containerPort === 3000 && !!p.hostPort && p.hostPort > 0 && p.hostPort < 65536)).to.be.true; // Exposed port with random binding
});

it('ExecContainerCommand', async function () {
Expand Down