Use HostConfig.Mounts to bind paths into container

When using the binds api the docker engine will auto-create the host
path as a directory if it doesn't exist. This can lead to unintended
side-effects when the source is a file (like when bind-mounting the
docker socket), as seen in the linked issue.

This also handles tmpfs and named volumes previously set without Mounts,
we take Config.Volumes and HostConfig.Tmpfs and
deduplicate over the full set.

To ensure containers managed by this Supervisor or newer run with the correct
HostConfig.Mounts config and not HostConfig.Binds, Bind configs are ignored
in favor of Mounts when comparing configs to ensure containers get
recreated as necessary. As a result of potential recreates which will happen
when self-serve upgrading to this Supervisor on a running device, this is
a semver major change.

Closes: #1231
Change-type: major
Signed-off-by: Robert Günzler <robertg@balena.io>
Co-authored-by: Christina Wang <christina@balena.io>

src/compose/service-manager.ts

@@ -20,6 +20,8 @@ import {
 } from '../lib/errors';
 import * as LogTypes from '../lib/log-types';
 import { checkInt, isValidDeviceName } from '../lib/validation';
+import { getPathOnHost, mkdirp } from '../lib/fs-utils';
+import { BASE_LOCK_DIR } from '../lib/update-lock';
 import { Service, ServiceStatus } from './service';
 import { serviceNetworksToDockerNetworks } from './utils';
 
@@ -273,6 +275,13 @@ async function create(service: Service) {
 		logger.logSystemEvent(LogTypes.installService, { service });
 		reportNewStatus(mockContainerId, service, 'Installing');
 
+		// Create directories on host for update lock binds, since Supervisor uses the
+		// Docker Mounts API which means an engine error is thrown if bind doesn't exist on host.
+		const lockDirPath = getPathOnHost(
+			`${BASE_LOCK_DIR}/${service.appId}/${service.serviceName}`,
+		);
+		await mkdirp(lockDirPath);
+
 		const container = await docker.createContainer(conf);
 		service.containerId = container.id;
 

src/compose/service.ts

@@ -528,10 +528,29 @@ export class Service {
 		}
 		expose = _.uniq(expose);
 
-		const tmpfs: string[] = [];
-		_.each((container.HostConfig as any).Tmpfs, (_v, key) => {
-			tmpfs.push(key);
-		});
+		const tmpfs: string[] = _.uniq(
+			([] as string[]).concat(
+				Object.keys(container.HostConfig?.Tmpfs || {}),
+				// also consider mount api
+				(container.HostConfig?.Mounts || [])
+					.filter(({ Type }) => Type === 'tmpfs')
+					.map(({ Target }) => Target),
+			),
+		);
+
+		// bind and volume mounts
+		const mounts: string[] = _.uniq(
+			([] as string[]).concat(
+				Object.keys(container.Config?.Volumes || {}),
+				// also consider mount api
+				(container.HostConfig?.Mounts || [])
+					.filter(({ Type }) => Type === 'bind' || Type === 'volume')
+					.map(
+						(mount) =>
+							`${mount.Source}:${mount.Target}${mount.ReadOnly ? ':ro' : ''}`,
+					),
+			),
+		);
 
 		// We cannot use || for this value, as the empty string is a
 		// valid restart policy but will equate to null in an OR
@@ -558,10 +577,7 @@ export class Service {
 			hostname,
 			command: container.Config.Cmd || '',
 			entrypoint: container.Config.Entrypoint || '',
-			volumes: _.concat(
-				container.HostConfig.Binds || [],
-				_.keys(container.Config.Volumes || {}),
-			),
+			volumes: mounts,
 			image: container.Config.Image,
 			environment: Service.omitDeviceNameVars(
 				conversions.envArrayToObject(container.Config.Env || []),
@@ -667,7 +683,7 @@ export class Service {
 		deviceName: string;
 		containerIds: Dictionary<string>;
 	}): Dockerode.ContainerCreateOptions {
-		const { binds, volumes } = this.getBindsAndVolumes();
+		const { mounts, volumes } = this.getMountsAndVolumes();
 		const { exposedPorts, portBindings } = this.generateExposeAndPorts();
 
 		const tmpFs: Dictionary<''> = {};
@@ -723,7 +739,6 @@ export class Service {
 			HostConfig: {
 				CapAdd: this.config.capAdd,
 				CapDrop: this.config.capDrop,
-				Binds: binds,
 				CgroupParent: this.config.cgroupParent,
 				Devices: this.config.devices,
 				DeviceRequests: this.config.deviceRequests,
@@ -751,6 +766,7 @@ export class Service {
 				CpusetCpus: this.config.cpuset,
 				Memory: this.config.memLimit,
 				MemoryReservation: this.config.memReservation,
+				Mounts: mounts,
 				OomKillDisable: this.config.oomKillDisable,
 				OomScoreAdj: this.config.oomScoreAdj,
 				Privileged: this.config.privileged,
@@ -923,21 +939,37 @@ export class Service {
 		);
 	}
 
-	private getBindsAndVolumes(): {
-		binds: string[];
+	private getMountsAndVolumes(): {
+		mounts: Dockerode.MountSettings[];
 		volumes: { [volName: string]: {} };
 	} {
-		const binds: string[] = [];
+		const mounts: Dockerode.MountSettings[] = [];
 		const volumes: { [volName: string]: {} } = {};
-		_.each(this.config.volumes, (volume) => {
+		for (const volume of this.config.volumes) {
 			if (_.includes(volume, ':')) {
-				binds.push(volume);
+				const [source, target, mode] = volume.split(':');
+				const mount: Dockerode.MountSettings = {
+					Type: 'bind',
+					Source: source,
+					Target: target,
+					ReadOnly: mode === 'ro' ? true : false,
+				};
+				if (!path.isAbsolute(source)) {
+					mount.Type = 'volume';
+				}
+				mounts.push(mount);
 			} else {
 				volumes[volume] = {};
 			}
-		});
+		}
+		for (const tmpfs of this.config.tmpfs) {
+			mounts.push({
+				Type: 'tmpfs',
+				Target: tmpfs,
+			} as Dockerode.MountSettings);
+		}
 
-		return { binds, volumes };
+		return { mounts, volumes };
 	}
 
 	private generateExposeAndPorts(): DockerPortOptions {

test/41-device-api-v1.spec.ts

@@ -31,6 +31,8 @@ import * as targetStateCache from '../src/device-state/target-state-cache';
 import blink = require('../src/lib/blink');
 import constants = require('../src/lib/constants');
 import * as deviceAPI from '../src/device-api/common';
+import * as lockfile from '../src/lib/lockfile';
+import * as fsUtils from '../src/lib/fs-utils';
 
 import { UpdatesLockedError } from '../src/lib/errors';
 import { SchemaTypeKey } from '../src/config/schema-type';
@@ -109,6 +111,17 @@ describe('SupervisorAPI [V1 Endpoints]', () => {
 		// Stub logs for all API methods
 		loggerStub = stub(logger, 'attach');
 		loggerStub.resolves();
+
+		// Stub lockfile calls to ensure nothing leaks through
+		// TODO: remove this after redoing API tests
+		stub(lockfile, 'lock').resolves();
+
+		// Stub fs-utils methods that /v1/restart and /v1/reboot
+		// use, otherwise reboot breadcrumb and lockfile directories
+		// get written to in test environment
+		// TODO: remove this after redoing API tests
+		stub(fsUtils, 'touch').resolves();
+		stub(fsUtils, 'mkdirp').resolves();
 	});
 
 	after(async () => {
@@ -125,6 +138,9 @@ describe('SupervisorAPI [V1 Endpoints]', () => {
 		await mockedAPI.cleanUp();
 		targetStateCacheMock.restore();
 		loggerStub.restore();
+		(lockfile.lock as SinonStub).restore();
+		(fsUtils.touch as SinonStub).restore();
+		(fsUtils.mkdirp as SinonStub).restore();
 	});
 
 	describe('POST /v1/restart', () => {

test/data/docker-states/entrypoint/inspect.json

@@ -33,9 +33,20 @@
 	"AppArmorProfile": "",
 	"ExecIDs": null,
 	"HostConfig": {
-		"Binds": [
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/resin",
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/balena"
+		"Binds": null,
+		"Mounts": [
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/resin",
+				"ReadOnly": false
+			},
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/balena",
+				"ReadOnly": false
+			}
 		],
 		"ContainerIDFile": "",
 		"LogConfig": {

test/data/docker-states/network-mode-service/inspect.json

@@ -34,9 +34,20 @@
 	"AppArmorProfile": "",
 	"ExecIDs": null,
 	"HostConfig": {
-		"Binds": [
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/resin",
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/balena"
+		"Binds": null,
+		"Mounts": [
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/resin",
+				"ReadOnly": false
+			},
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/balena",
+				"ReadOnly": false
+			}
 		],
 		"ContainerIDFile": "",
 		"LogConfig": {

test/data/docker-states/simple/inspect.json

@@ -34,9 +34,20 @@
 	"AppArmorProfile": "",
 	"ExecIDs": null,
 	"HostConfig": {
-		"Binds": [
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/resin",
-			"/tmp/balena-supervisor/services/1011165/main:/tmp/balena"
+		"Binds": null,
+		"Mounts": [
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/resin",
+				"ReadOnly": false
+			},
+			{
+				"Type": "bind",
+				"Source": "/tmp/balena-supervisor/services/1011165/main",
+				"Target": "/tmp/balena",
+				"ReadOnly": false
+			}
 		],
 		"ContainerIDFile": "",
 		"LogConfig": {

test/lib/mockerode.ts

@@ -250,7 +250,7 @@ export function createContainer(container: PartialContainerInspectInfo) {
 	const fakeContainer = createFake(dockerode.Container.prototype);
 
 	return {
-		...fakeContainer, // by default all methods fail unless overriden
+		...fakeContainer, // by default all methods fail unless overridden
 		id,
 		inspectInfo,
 		info,

test/src/compose/service.spec.ts

@@ -485,6 +485,142 @@ describe('compose/service', () => {
 		});
 	});
 
+	describe('Configuring service volumes', () => {
+		it('should add bind mounts using the mounts API', async () => {
+			const s = await Service.fromComposeObject(
+				{
+					appId: 123456,
+					serviceId: 123456,
+					serviceName: 'test',
+					composition: {
+						volumes: ['myvolume:/myvolume'],
+						tmpfs: ['/var/tmp'],
+					},
+				},
+				{ appName: 'test' } as any,
+			);
+
+			// inject bind mounts (as feature labels would)
+			s.config.volumes.push('/sys:/sys:ro');
+			s.config.volumes.push(
+				`${constants.dockerSocket}:${constants.containerDockerSocket}`,
+			);
+
+			const c = s.toDockerContainer({ deviceName: 'foo' } as any);
+
+			expect(c)
+				.to.have.property('HostConfig')
+				.that.has.property('Mounts')
+				.that.deep.includes.members([
+					{
+						Type: 'volume',
+						Source: Volume.generateDockerName(123456, 'myvolume'),
+						Target: '/myvolume',
+						ReadOnly: false,
+					},
+					{
+						Type: 'bind',
+						Source: '/tmp/balena-supervisor/services/123456/test',
+						Target: '/tmp/resin',
+						ReadOnly: false,
+					},
+					{
+						Type: 'bind',
+						Source: '/tmp/balena-supervisor/services/123456/test',
+						Target: '/tmp/balena',
+						ReadOnly: false,
+					},
+					{
+						Type: 'bind',
+						Source: '/sys',
+						Target: '/sys',
+						ReadOnly: true,
+					},
+					{
+						Type: 'bind',
+						Source: constants.dockerSocket,
+						Target: constants.containerDockerSocket,
+						ReadOnly: false,
+					},
+					{
+						Type: 'tmpfs',
+						Target: '/var/tmp',
+					},
+				]);
+		});
+
+		it('should obtain the service volume config from docker configuration', () => {
+			const mockContainer = createContainer({
+				Id: 'deadbeef',
+				Name: 'main_431889_572579',
+				HostConfig: {
+					// Volumes with Bind configs are ignored since
+					// the Supervisor uses explicit Mounts for volumes
+					Binds: ['ignoredvolume:/ignoredvolume'],
+					Tmpfs: {
+						'/var/tmp1': '',
+					},
+					Mounts: [
+						{
+							Type: 'volume',
+							Source: 'testvolume',
+							Target: '/testvolume',
+							ReadOnly: false,
+						},
+						{
+							Type: 'bind',
+							Source: '/proc',
+							Target: '/proc',
+							ReadOnly: true,
+						},
+						{
+							Type: 'bind',
+							Source: '/sys',
+							Target: '/sys',
+							ReadOnly: true,
+						},
+						{
+							Type: 'volume',
+							Source: 'anothervolume',
+							Target: '/anothervolume',
+							ReadOnly: false,
+						},
+						{
+							Type: 'tmpfs',
+							Source: '',
+							Target: '/var/tmp2',
+						},
+					],
+				},
+				Config: {
+					Volumes: {
+						'/var/lib/volume': {},
+					},
+					Labels: {
+						'io.resin.app-id': '1011165',
+						'io.resin.architecture': 'armv7hf',
+						'io.resin.service-id': '43697',
+						'io.resin.service-name': 'main',
+						'io.resin.supervised': 'true',
+					},
+				},
+			});
+			const s = Service.fromDockerContainer(mockContainer.inspectInfo);
+			expect(s.config)
+				.to.have.property('volumes')
+				.that.deep.equals([
+					'/var/lib/volume',
+					'testvolume:/testvolume',
+					'/proc:/proc:ro',
+					'/sys:/sys:ro',
+					'anothervolume:/anothervolume',
+				]);
+			expect(s.config)
+				.to.have.property('tmpfs')
+				.that.deep.equals(['/var/tmp1', '/var/tmp2']);
+		});
+	});
+
 	describe('Comparing services', () => {
 		describe('Comparing array parameters', () => {
 			it('Should correctly compare ordered array parameters', async () => {
@@ -593,6 +729,75 @@ describe('compose/service', () => {
 				expect(svc1.isEqualConfig(svc2, {})).to.be.true;
 			});
 		});
+
+		describe('Comparing volume bind mounts for services', () => {
+			it('should distinguish between volumes using HostConfig.Mounts and HostConfig.Binds', async () => {
+				const ctnWithBinds = await createContainer({
+					Id: 'deadfeet',
+					Name: 'main_1111_2222_abcd',
+					HostConfig: {
+						Binds: [
+							'myvolume:/myvolume',
+							'/tmp/balena-supervisor/services/1234567/main:/tmp/resin',
+							'/tmp/balena-supervisor/services/1234567/main:/tmp/balena',
+						],
+					},
+					Config: {
+						Labels: {
+							'io.resin.app-id': '1234567',
+							'io.resin.architecture': 'amd64',
+							'io.resin.service-id': '43697',
+							'io.resin.service-name': 'main',
+							'io.resin.supervised': 'true',
+						},
+					},
+				});
+				const svcWithBinds = await Service.fromDockerContainer(
+					ctnWithBinds.inspectInfo,
+				);
+
+				const ctnWithMounts = await createContainer({
+					Id: 'deadfeet',
+					Name: 'main_1111_2222_abcd',
+					HostConfig: {
+						Mounts: [
+							{
+								Type: 'volume',
+								Source: 'myvolume',
+								Target: '/myvolume',
+								ReadOnly: false,
+							},
+							{
+								Type: 'bind',
+								Source: '/tmp/balena-supervisor/services/1234567/main',
+								Target: '/tmp/resin',
+								ReadOnly: false,
+							},
+							{
+								Type: 'bind',
+								Source: '/tmp/balena-supervisor/services/1234567/main',
+								Target: '/tmp/balena',
+								ReadOnly: false,
+							},
+						],
+					},
+					Config: {
+						Labels: {
+							'io.resin.app-id': '1234567',
+							'io.resin.architecture': 'amd64',
+							'io.resin.service-id': '43697',
+							'io.resin.service-name': 'main',
+							'io.resin.supervised': 'true',
+						},
+					},
+				});
+				const svcWithMounts = await Service.fromDockerContainer(
+					ctnWithMounts.inspectInfo,
+				);
+
+				expect(svcWithBinds.isEqualConfig(svcWithMounts, {})).to.be.false;
+			});
+		});
 	});
 
 	describe('Feature labels', () => {