Merge pull request #2054 from balena-os/improve-sv-docker-network-setup

Convert ensureSupervisorNetwork to native Promises, remove sys interface check

docker-compose.test.yml

@@ -9,7 +9,7 @@ services:
       dockerfile: Dockerfile.template
       args:
         ARCH: ${ARCH:-amd64}
-    command: ['/wait-for-it.sh', '--', '/usr/src/app/entry.sh']
+    command: [ '/wait-for-it.sh', '--', '/usr/src/app/entry.sh' ]
     # Use bridge networking for the tests
     network_mode: 'bridge'
     networks:
@@ -30,7 +30,7 @@ services:
       - ./test/data/root:/mnt/root
       - ./test/lib/wait-for-it.sh:/wait-for-it.sh
     tmpfs:
-      - /data
+      - /data # sqlite3 database
 
   dbus:
     image: balenablocks/dbus
@@ -74,7 +74,7 @@ services:
         '--',
         'npm',
         'run',
-        'test:integration',
+        'test:integration'
       ]
     depends_on:
       - balena-supervisor

package.json

@@ -20,7 +20,7 @@
     "test:env": "ARCH=$(./build-utils/detect-arch.sh) docker-compose -f docker-compose.test.yml -f docker-compose.dev.yml up --build; npm run compose:down",
     "test:compose": "ARCH=$(./build-utils/detect-arch.sh) docker-compose -f docker-compose.yml -f docker-compose.test.yml up --build --remove-orphans --exit-code-from=sut ; npm run compose:down",
     "test": "npm run lint && npm run test:build && npm run test:unit && npm run test:legacy",
-    "compose:down": "docker-compose -f docker-compose.test.yml down",
+    "compose:down": "docker-compose -f docker-compose.test.yml down && docker volume rm $(docker volume ls -f name=balena-supervisor -q)",
     "prettify": "balena-lint -e ts -e js --fix src/ test/ typings/ build-utils/ webpack.config.js",
     "release": "tsc --project tsconfig.release.json && mv build/src/* build",
     "sync": "ts-node --files sync/sync.ts",

src/compose/app.ts

@@ -18,7 +18,7 @@ import constants = require('../lib/constants');
 
 import { getStepsFromStrategy } from './update-strategies';
 
-import { InternalInconsistencyError, NotFoundError } from '../lib/errors';
+import { InternalInconsistencyError, isNotFoundError } from '../lib/errors';
 import * as config from '../config';
 import { checkTruthy, checkString } from '../lib/validation';
 import { ServiceComposeConfig, DeviceMetadata } from './types/service';
@@ -804,8 +804,8 @@ export class App {
 					let imageInfo: ImageInspectInfo | undefined;
 					try {
 						imageInfo = await imageManager.inspectByName(svc.image);
-					} catch (e: any) {
-						if (!NotFoundError(e)) {
+					} catch (e: unknown) {
+						if (!isNotFoundError(e)) {
 							throw e;
 						}
 					}

src/compose/composition-steps.ts

@@ -276,7 +276,7 @@ export function getExecutors(app: {
 			await volumeManager.remove(step.current);
 		},
 		ensureSupervisorNetwork: async () => {
-			networkManager.ensureSupervisorNetwork();
+			await networkManager.ensureSupervisorNetwork();
 		},
 		noop: async () => {
 			/* async noop */

src/compose/images.ts

@@ -11,7 +11,7 @@ import { DeltaFetchOptions, FetchOptions, docker } from '../lib/docker-utils';
 import * as dockerUtils from '../lib/docker-utils';
 import {
 	DeltaStillProcessingError,
-	NotFoundError,
+	isNotFoundError,
 	StatusError,
 } from '../lib/errors';
 import * as LogTypes from '../lib/log-types';
@@ -236,8 +236,8 @@ export async function triggerFetch(
 		await markAsSupervised({ ...image, dockerImageId: img.Id });
 
 		success = true;
-	} catch (e: any) {
-		if (!NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
 			if (!(e instanceof ImageDownloadBackoffError)) {
 				addImageFailure(image.name);
 			}
@@ -729,8 +729,8 @@ async function removeImageIfNotNeeded(image: Image): Promise<void> {
 
 		// Mark the image as removed
 		removed = true;
-	} catch (e: any) {
-		if (NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (isNotFoundError(e)) {
 			removed = false;
 		} else {
 			throw e;

src/compose/network-manager.ts

@@ -3,10 +3,9 @@ import * as _ from 'lodash';
 
 import * as constants from '../lib/constants';
 import { docker } from '../lib/docker-utils';
-import { NotFoundError } from '../lib/errors';
+import { isNotFoundError } from '../lib/errors';
 import logTypes = require('../lib/log-types');
 import log from '../lib/supervisor-console';
-import { exists } from '../lib/fs-utils';
 
 import * as logger from '../logger';
 import { Network } from './network';
@@ -45,8 +44,8 @@ export async function create(network: Network) {
 
 		// We have a network with the same config and name
 		// already created, we can skip this
-	} catch (e: any) {
-		if (!NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
 			logger.logSystemEvent(logTypes.createNetworkError, {
 				network: { name: network.name, appUuid: network.appUuid },
 				error: e,
@@ -65,82 +64,66 @@ export async function remove(network: Network) {
 	await network.remove();
 }
 
-const supervisorIfaceSysPath = `/sys/class/net/${constants.supervisorNetworkInterface}`;
-export async function supervisorNetworkReady(): Promise<boolean> {
-	const networkExists = await exists(supervisorIfaceSysPath);
-	if (!networkExists) {
-		return false;
-	}
+const {
+	supervisorNetworkInterface: iface,
+	supervisorNetworkGateway: gateway,
+	supervisorNetworkSubnet: subnet,
+} = constants;
 
+export async function supervisorNetworkReady(): Promise<boolean> {
 	try {
 		// The inspect may fail even if the interface exist due to docker corruption
-		const network = await docker
-			.getNetwork(constants.supervisorNetworkInterface)
-			.inspect();
-		return (
-			network.Options['com.docker.network.bridge.name'] ===
-				constants.supervisorNetworkInterface &&
-			network.IPAM.Config[0].Subnet === constants.supervisorNetworkSubnet &&
-			network.IPAM.Config[0].Gateway === constants.supervisorNetworkGateway
-		);
-	} catch (e) {
+		const network = await docker.getNetwork(iface).inspect();
+		const result =
+			network.Options['com.docker.network.bridge.name'] === iface &&
+			network.IPAM.Config[0].Subnet === subnet &&
+			network.IPAM.Config[0].Gateway === gateway;
+		return result;
+	} catch (e: unknown) {
 		log.warn(
-			`Failed to read docker configuration of network ${constants.supervisorNetworkInterface}:`,
-			e,
+			`Failed to read docker configuration of network ${iface}:`,
+			(e as Error).message,
 		);
 		return false;
 	}
 }
 
-export function ensureSupervisorNetwork(): Bluebird<void> {
-	const removeIt = () => {
-		return Bluebird.resolve(
-			docker.getNetwork(constants.supervisorNetworkInterface).remove(),
-		).then(() => {
-			return docker.getNetwork(constants.supervisorNetworkInterface).inspect();
-		});
-	};
+export async function ensureSupervisorNetwork(): Promise<void> {
+	try {
+		const net = await docker.getNetwork(iface).inspect();
+		if (
+			net.Options['com.docker.network.bridge.name'] !== iface ||
+			net.IPAM.Config[0].Subnet !== subnet ||
+			net.IPAM.Config[0].Gateway !== gateway
+		) {
+			// Remove network if its configs aren't correct
+			await docker.getNetwork(iface).remove();
+			// This will throw a 404 if network has been removed completely
+			return await docker.getNetwork(iface).inspect();
+		}
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
+			return;
+		}
 
-	return Bluebird.resolve(
-		docker.getNetwork(constants.supervisorNetworkInterface).inspect(),
-	)
-		.then((net) => {
-			if (
-				net.Options['com.docker.network.bridge.name'] !==
-					constants.supervisorNetworkInterface ||
-				net.IPAM.Config[0].Subnet !== constants.supervisorNetworkSubnet ||
-				net.IPAM.Config[0].Gateway !== constants.supervisorNetworkGateway
-			) {
-				return removeIt();
-			} else {
-				return exists(supervisorIfaceSysPath).then((networkExists) => {
-					if (!networkExists) {
-						return removeIt();
-					}
-				});
-			}
-		})
-		.catch(NotFoundError, () => {
-			log.debug(`Creating ${constants.supervisorNetworkInterface} network`);
-			return Bluebird.resolve(
-				docker.createNetwork({
-					Name: constants.supervisorNetworkInterface,
-					Options: {
-						'com.docker.network.bridge.name':
-							constants.supervisorNetworkInterface,
+		log.debug(`Creating ${iface} network`);
+		await docker.createNetwork({
+			Name: iface,
+			Options: {
+				'com.docker.network.bridge.name': iface,
+			},
+			IPAM: {
+				Driver: 'default',
+				Config: [
+					{
+						Subnet: subnet,
+						Gateway: gateway,
 					},
-					IPAM: {
-						Driver: 'default',
-						Config: [
-							{
-								Subnet: constants.supervisorNetworkSubnet,
-								Gateway: constants.supervisorNetworkGateway,
-							},
-						],
-					},
-				}),
-			);
+				],
+			},
+			CheckDuplicate: true,
 		});
+	}
 }
 
 function getWithBothLabels() {

src/compose/service-manager.ts

@@ -15,7 +15,7 @@ import { PermissiveNumber } from '../config/types';
 import constants = require('../lib/constants');
 import {
 	InternalInconsistencyError,
-	NotFoundError,
+	isNotFoundError,
 	StatusCodeError,
 	isStatusError,
 } from '../lib/errors';
@@ -72,8 +72,8 @@ export const getAll = async (
 				service.status = vState.status;
 			}
 			return service;
-		} catch (e: any) {
-			if (NotFoundError(e)) {
+		} catch (e: unknown) {
+			if (isNotFoundError(e)) {
 				return null;
 			}
 			throw e;
@@ -206,8 +206,8 @@ export async function remove(service: Service) {
 
 	try {
 		await docker.getContainer(existingService.containerId).remove({ v: true });
-	} catch (e: any) {
-		if (!NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
 			logger.logSystemEvent(LogTypes.removeDeadServiceError, {
 				service,
 				error: e,
@@ -227,8 +227,8 @@ async function create(service: Service) {
 			);
 		}
 		return docker.getContainer(existing.containerId);
-	} catch (e: any) {
-		if (!NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
 			logger.logSystemEvent(LogTypes.installServiceError, {
 				service,
 				error: e,
@@ -383,8 +383,8 @@ export function listenToEvents() {
 						let service: Service | null = null;
 						try {
 							service = await getByDockerContainerId(data.id);
-						} catch (e: any) {
-							if (!NotFoundError(e)) {
+						} catch (e: unknown) {
+							if (!isNotFoundError(e)) {
 								throw e;
 							}
 						}

src/compose/volume-manager.ts

@@ -3,7 +3,7 @@ import * as Path from 'path';
 import { VolumeInspectInfo } from 'dockerode';
 
 import constants = require('../lib/constants');
-import { NotFoundError, InternalInconsistencyError } from '../lib/errors';
+import { isNotFoundError, InternalInconsistencyError } from '../lib/errors';
 import { safeRename } from '../lib/fs-utils';
 import { docker } from '../lib/docker-utils';
 import * as LogTypes from '../lib/log-types';
@@ -58,8 +58,8 @@ export async function create(volume: Volume): Promise<void> {
 		if (!volume.isEqualConfig(existing)) {
 			throw new ResourceRecreationAttemptError('volume', volume.name);
 		}
-	} catch (e: any) {
-		if (!NotFoundError(e)) {
+	} catch (e: unknown) {
+		if (!isNotFoundError(e)) {
 			logger.logSystemEvent(LogTypes.createVolumeError, {
 				volume: { name: volume.name },
 				error: e,

src/lib/constants.ts

@@ -27,9 +27,7 @@ const constants = {
 	hostOSVersionPath:
 		checkString(process.env.HOST_OS_VERSION_PATH) ||
 		`${rootMountPoint}/etc/os-release`,
-	macAddressPath:
-		checkString(process.env.MAC_ADDRESS_PATH) ||
-		`${rootMountPoint}/sys/class/net`,
+	macAddressPath: checkString(process.env.MAC_ADDRESS_PATH) || `/sys/class/net`,
 	privateAppEnvVars: [
 		'RESIN_SUPERVISOR_API_KEY',
 		'RESIN_API_KEY',

src/lib/errors.ts

@@ -22,16 +22,23 @@ export class StatusError extends Error {
 export const isStatusError = (x: unknown): x is StatusError =>
 	x != null && x instanceof Error && !isNaN((x as any).statusCode);
 
+export class NotFoundError extends Error {
+	public statusCode: number;
+	constructor() {
+		super();
+		this.statusCode = 404;
+	}
+}
+
+export const isNotFoundError = (e: unknown): e is NotFoundError =>
+	isStatusError(e) && e.statusCode === 404;
+
 interface CodedSysError extends Error {
 	code?: string;
 }
 
 export class DeviceNotFoundError extends TypedError {}
 
-export function NotFoundError(err: StatusCodeError): boolean {
-	return checkInt(err.statusCode) === 404;
-}
-
 export function ENOENT(err: CodedSysError): boolean {
 	return err.code === 'ENOENT';
 }

src/lib/legacy.ts

@@ -10,7 +10,7 @@ import * as applicationManager from '../compose/application-manager';
 import {
 	StatusError,
 	DatabaseParseError,
-	NotFoundError,
+	isNotFoundError,
 	InternalInconsistencyError,
 } from '../lib/errors';
 import * as constants from '../lib/constants';
@@ -145,12 +145,12 @@ export async function normaliseLegacyDatabase() {
 		const imageFromDocker = await docker
 			.getImage(service.image)
 			.inspect()
-			.catch((error) => {
-				if (error instanceof NotFoundError) {
+			.catch((e: unknown) => {
+				if (isNotFoundError(e)) {
 					return;
 				}
 
-				throw error;
+				throw e;
 			});
 		const imagesFromDatabase = await db
 			.models('image')

src/lib/migration.ts

@@ -9,7 +9,7 @@ const rimrafAsync = Bluebird.promisify(rimraf);
 import * as volumeManager from '../compose/volume-manager';
 import * as deviceState from '../device-state';
 import * as constants from '../lib/constants';
-import { BackupError, NotFoundError } from '../lib/errors';
+import { BackupError, isNotFoundError } from '../lib/errors';
 import { exec, pathExistsOnHost, mkdirp } from '../lib/fs-utils';
 import { log } from '../lib/supervisor-console';
 
@@ -67,11 +67,11 @@ export async function loadBackupFromMigration(
 					.then((volume) => {
 						return volume.remove();
 					})
-					.catch((error) => {
-						if (error instanceof NotFoundError) {
+					.catch((e: unknown) => {
+						if (isNotFoundError(e)) {
 							return;
 						}
-						throw error;
+						throw e;
 					});
 
 				await volumeManager.createFromPath(

test/integration/compose/application-manager.spec.ts

@@ -1,6 +1,7 @@
 import { expect } from 'chai';
 import * as sinon from 'sinon';
 import { stub } from 'sinon';
+import * as Docker from 'dockerode';
 import App from '~/src/compose/app';
 import * as applicationManager from '~/src/compose/application-manager';
 import * as imageManager from '~/src/compose/images';
@@ -172,7 +173,6 @@ describe('compose/application-manager', () => {
 	before(async () => {
 		// Stub methods that depend on external dependencies
 		stub(imageManager, 'isCleanupNeeded');
-		stub(networkManager, 'supervisorNetworkReady');
 
 		// Service.fromComposeObject gets api keys from the database
 		// which also depend on the local mode. This ensures the database
@@ -181,20 +181,28 @@ describe('compose/application-manager', () => {
 		await config.initialized();
 	});
 
-	beforeEach(() => {
+	beforeEach(async () => {
 		// Do not check for cleanup images by default
 		(imageManager.isCleanupNeeded as sinon.SinonStub).resolves(false);
-		// Do not check for network
-		// TODO: supervisorNetworkReady not only checks for a docker network, it also checks for the
-		// network interface to be created. That makes it harder to integration test with an external
-		// docker socket
-		(networkManager.supervisorNetworkReady as sinon.SinonStub).resolves(true);
+		// Set up network by default
+		await networkManager.ensureSupervisorNetwork();
 	});
 
 	after(() => {
 		// Restore stubs
 		(imageManager.isCleanupNeeded as sinon.SinonStub).restore();
-		(networkManager.supervisorNetworkReady as sinon.SinonStub).restore();
+	});
+
+	afterEach(async () => {
+		// Delete any created networks
+		const docker = new Docker();
+		const allNetworks = await docker.listNetworks();
+		await Promise.all(
+			allNetworks
+				// exclude docker default networks from the cleanup
+				.filter(({ Name }) => !['bridge', 'host', 'none'].includes(Name))
+				.map(({ Name }) => docker.getNetwork(Name).remove()),
+		);
 	});
 
 	// TODO: we don't test application manager initialization as it sets up a bunch of timers
@@ -853,8 +861,8 @@ describe('compose/application-manager', () => {
 	});
 
 	it('should infer that we need to create the supervisor network if it does not exist', async () => {
-		// stub the networkManager method to fail on finding the supervisor network
-		(networkManager.supervisorNetworkReady as sinon.SinonStub).resolves(false);
+		const docker = new Docker();
+		await docker.getNetwork('supervisor0').remove();
 
 		const targetApps = createApps(
 			{ services: [await createService()], networks: [DEFAULT_NETWORK] },
@@ -879,8 +887,8 @@ describe('compose/application-manager', () => {
 	});
 
 	it('should kill a service which depends on the supervisor network, if we need to create the network', async () => {
-		// stub the networkManager method to fail on finding the supervisor network
-		(networkManager.supervisorNetworkReady as sinon.SinonStub).resolves(false);
+		const docker = new Docker();
+		await docker.getNetwork('supervisor0').remove();
 
 		const labels = { 'io.balena.features.supervisor-api': 'true' };
 

test/integration/config.spec.ts

@@ -153,9 +153,6 @@ describe('config', () => {
 	});
 
 	it('reads and exposes MAC addresses', async () => {
-		// FIXME: this variable defaults to `/mnt/root/sys/class/net`. The supervisor runs with network_mode: host
-		// which means that it can just use the container `/sys/class/net` and the result should be the same
-		constants.macAddressPath = '/sys/class/net';
 		const macAddress = await conf.get('macAddress');
 		expect(macAddress).to.have.length.greaterThan(0);
 	});

test/legacy/10-api-binder.spec.ts

@@ -15,6 +15,7 @@ import * as TargetState from '~/src/device-state/target-state';
 import * as ApiHelper from '~/lib/api-helper';
 import supervisorVersion = require('~/lib/supervisor-version');
 import * as eventTracker from '~/src/event-tracker';
+import * as constants from '~/lib/constants';
 
 import { TypedError } from 'typed-error';
 import { DeviceNotFoundError } from '~/lib/errors';
@@ -65,6 +66,10 @@ describe('ApiBinder', () => {
 
 		spy(balenaAPI.balenaBackend!, 'registerHandler');
 		server = balenaAPI.listen(3000);
+
+		// TODO: remove when moving this suite to integration tests
+		// @ts-expect-error
+		constants.macAddressPath = './test/data/sys/class/net';
 	});
 
 	after(() => {
@@ -75,6 +80,10 @@ describe('ApiBinder', () => {
 		} catch (error) {
 			/* noop */
 		}
+
+		// TODO: remove when moving this suite to integration tests
+		// @ts-expect-error
+		constants.macAddressPath = '/sys/class/net';
 	});
 
 	// We do not support older OS versions anymore, so we only test this case

test/lib/mocked-dockerode.ts

@@ -3,15 +3,7 @@ process.env.DOCKER_HOST = 'unix:///your/dockerode/mocks/are/not/working';
 import * as dockerode from 'dockerode';
 import { Stream } from 'stream';
 import _ = require('lodash');
-import { TypedError } from 'typed-error';
-
-export class NotFoundError extends TypedError {
-	public statusCode: number;
-	constructor() {
-		super();
-		this.statusCode = 404;
-	}
-}
+import { NotFoundError } from '~/lib/errors';
 
 const overrides: Dictionary<(...args: any[]) => Resolvable<any>> = {};