Merge pull request #2130 from balena-os/duplicate-networks

Find and remove duplicate networks

src/compose/app.ts

@@ -35,8 +35,8 @@ export interface AppConstructOpts {
 	isHost?: boolean;
 
 	services: Service[];
-	volumes: Dictionary<Volume>;
-	networks: Dictionary<Network>;
+	volumes: Volume[];
+	networks: Network[];
 }
 
 export interface UpdateState {
@@ -62,8 +62,8 @@ export class App {
 	// Services are stored as an array, as at any one time we could have more than one
 	// service for a single service ID running (for example handover)
 	public services: Service[];
-	public networks: Dictionary<Network>;
-	public volumes: Dictionary<Volume>;
+	public networks: Network[];
+	public volumes: Volume[];
 
 	public constructor(opts: AppConstructOpts, public isTargetState: boolean) {
 		this.appId = opts.appId;
@@ -76,21 +76,26 @@ export class App {
 		this.networks = opts.networks;
 		this.isHost = !!opts.isHost;
 
-		if (this.networks.default == null && isTargetState) {
+		if (
+			this.networks.find((n) => n.name === 'default') == null &&
+			isTargetState
+		) {
 			const allHostNetworking = this.services.every(
 				(svc) => svc.config.networkMode === 'host',
 			);
 			// We always want a default network
-			this.networks.default = Network.fromComposeObject(
-				'default',
-				opts.appId,
-				// app uuid always exists on the target state
-				opts.appUuid!,
-				// We don't want the default bridge to have actual addresses at all if services
-				// aren't using it, to minimize chances of host-Docker address conflicts.
-				// If config_only is specified, the network is created and available in Docker
-				// by name and config only, and no actual networking setup occurs.
-				{ config_only: allHostNetworking },
+			this.networks.push(
+				Network.fromComposeObject(
+					'default',
+					opts.appId,
+					// app uuid always exists on the target state
+					opts.appUuid!,
+					// We don't want the default bridge to have actual addresses at all if services
+					// aren't using it, to minimize chances of host-Docker address conflicts.
+					// If config_only is specified, the network is created and available in Docker
+					// by name and config only, and no actual networking setup occurs.
+					{ config_only: allHostNetworking },
+				),
 			);
 		}
 	}
@@ -231,33 +236,63 @@ export class App {
 		}
 	}
 
-	private compareComponents<T extends { isEqualConfig(target: T): boolean }>(
-		current: Dictionary<T>,
-		target: Dictionary<T>,
+	private compareComponents<
+		T extends { name: string; isEqualConfig(target: T): boolean },
+	>(
+		current: T[],
+		target: T[],
 		// Should this function issue remove steps? (we don't want to for volumes)
 		generateRemoves: boolean,
 	): Array<ChangingPair<T>> {
-		const currentNames = _.keys(current);
-		const targetNames = _.keys(target);
-
 		const outputs: Array<{ current?: T; target?: T }> = [];
+		const toBeUpdated: string[] = [];
+
+		// Find those components that change between the current and target state
+		// those will have to be removed first and added later
+		target.forEach((tgt) => {
+			const curr = current.find(
+				(item) => item.name === tgt.name && !item.isEqualConfig(tgt),
+			);
+			if (curr) {
+				outputs.push({ current: curr, target: tgt });
+				toBeUpdated.push(curr.name);
+			}
+		});
 
 		if (generateRemoves) {
-			for (const name of _.difference(currentNames, targetNames)) {
-				outputs.push({ current: current[name] });
-			}
-		}
-		for (const name of _.difference(targetNames, currentNames)) {
-			outputs.push({ target: target[name] });
+			const toBeRemoved: string[] = [];
+			// Find those components that are not part of the target state
+			current.forEach((curr) => {
+				if (!target.find((tgt) => tgt.name === curr.name)) {
+					outputs.push({ current: curr });
+					toBeRemoved.push(curr.name);
+				}
+			});
+
+			// Find duplicates in the current state and remove them
+			current.forEach((item, index) => {
+				const hasDuplicate =
+					current.findIndex((it) => it.name === item.name) !== index;
+
+				if (
+					hasDuplicate &&
+					// Skip components that are being updated as those will need to be removed anyway
+					!toBeUpdated.includes(item.name) &&
+					// Avoid adding the component again if it has already been marked for removal
+					!toBeRemoved.includes(item.name)
+				) {
+					outputs.push({ current: item });
+					toBeRemoved.push(item.name);
+				}
+			});
 		}
 
-		const toBeUpdated = _.filter(
-			_.intersection(targetNames, currentNames),
-			(name) => !current[name].isEqualConfig(target[name]),
-		);
-		for (const name of toBeUpdated) {
-			outputs.push({ current: current[name], target: target[name] });
-		}
+		// Find newly created components
+		target.forEach((tgt) => {
+			if (!current.find((curr) => tgt.name === curr.name)) {
+				outputs.push({ target: tgt });
+			}
+		});
 
 		return outputs;
 	}
@@ -370,7 +405,6 @@ export class App {
 		/**
 		 * Checks if Supervisor should keep the state loop alive while waiting on a service to stop
 		 * @param serviceCurrent
-		 * @param serviceTarget
 		 */
 		const shouldWaitForStop = (serviceCurrent: Service) => {
 			return (
@@ -738,22 +772,20 @@ export class App {
 	public static async fromTargetState(
 		app: targetStateCache.DatabaseApp,
 	): Promise<App> {
-		const volumes = _.mapValues(JSON.parse(app.volumes) ?? {}, (conf, name) => {
-			if (conf == null) {
-				conf = {};
-			}
+		const jsonVolumes = JSON.parse(app.volumes) ?? {};
+		const volumes = Object.keys(jsonVolumes).map((name) => {
+			const conf = jsonVolumes[name];
 			if (conf.labels == null) {
 				conf.labels = {};
 			}
 			return Volume.fromComposeObject(name, app.appId, app.uuid, conf);
 		});
 
-		const networks = _.mapValues(
-			JSON.parse(app.networks) ?? {},
-			(conf, name) => {
-				return Network.fromComposeObject(name, app.appId, app.uuid, conf ?? {});
-			},
-		);
+		const jsonNetworks = JSON.parse(app.networks) ?? {};
+		const networks = Object.keys(jsonNetworks).map((name) => {
+			const conf = jsonNetworks[name];
+			return Network.fromComposeObject(name, app.appId, app.uuid, conf ?? {});
+		});
 
 		const [opts, supervisorApiHost, hostPathExists, hostname] =
 			await Promise.all([

src/compose/application-manager.ts

@@ -242,8 +242,8 @@ export async function inferNextSteps(
 					{
 						appId,
 						services: [],
-						volumes: {},
-						networks: {},
+						volumes: [],
+						networks: [],
 					},
 					false,
 				);
@@ -352,8 +352,8 @@ export async function getCurrentApps(): Promise<InstancedAppState> {
 					appUuid: uuid,
 					commit,
 					services: componentGroups[appId].services,
-					networks: _.keyBy(componentGroups[appId].networks, 'name'),
-					volumes: _.keyBy(componentGroups[appId].volumes, 'name'),
+					networks: componentGroups[appId].networks,
+					volumes: componentGroups[appId].volumes,
 				},
 				false,
 			);

src/compose/network-manager.ts

@@ -11,10 +11,10 @@ import * as logger from '../logger';
 import { Network } from './network';
 import { ResourceRecreationAttemptError } from './errors';
 
-export function getAll(): Bluebird<Network[]> {
-	return getWithBothLabels().map((network: { Name: string }) => {
+export async function getAll(): Promise<Network[]> {
+	return getWithBothLabels().map(async (network: { Id: string }) => {
 		return docker
-			.getNetwork(network.Name)
+			.getNetwork(network.Id)
 			.inspect()
 			.then((net) => {
 				return Network.fromDockerNetwork(net);

src/compose/network.ts

@@ -205,8 +205,12 @@ export class Network {
 			network: { name: this.name, appUuid: this.appUuid },
 		});
 
-		// Find the network
-		const [networkName] = (await docker.listNetworks())
+		// Find the networks with the same name. While theoretically
+		// the network name is unique, because moby is not great with concurrency
+		// it's possible to have multiple networks with the same name
+		// https://github.com/moby/moby/issues/20648
+		// For this reason we need to delete them all
+		const networkIds = (await docker.listNetworks())
 			.filter((network) => {
 				try {
 					const { appId, appUuid, name } = Network.deconstructDockerName(
@@ -220,14 +224,16 @@ export class Network {
 					return false;
 				}
 			})
-			.map((network) => network.Name);
+			.map((network) => network.Id);
 
-		if (!networkName) {
+		if (networkIds.length === 0) {
 			return;
 		}
 
 		try {
-			await docker.getNetwork(networkName).remove();
+			await Promise.all(
+				networkIds.map((networkId) => docker.getNetwork(networkId).remove()),
+			);
 		} catch (error) {
 			logger.logSystemEvent(logTypes.removeNetworkError, {
 				network: { name: this.name, appUuid: this.appUuid },

src/device-api/actions.ts

@@ -234,7 +234,7 @@ export const doPurge = async (appId: number, force: boolean = false) => {
 		const clonedState = safeStateClone(currentState);
 		// Set services & volumes as empty to be applied as intermediate state
 		app.services = [];
-		app.volumes = {};
+		app.volumes = [];
 
 		applicationManager.setIsApplyingIntermediate(true);
 

test/integration/device-state/db-format.spec.ts

@@ -8,9 +8,7 @@ import * as dbFormat from '~/src/device-state/db-format';
 import { TargetApps } from '~/src/types/state';
 
 function getDefaultNetwork(appId: number) {
-	return {
-		default: Network.fromComposeObject('default', appId, 'deadbeef', {}),
-	};
+	return [Network.fromComposeObject('default', appId, 'deadbeef', {})];
 }
 
 describe('device-state/db-format', () => {
@@ -119,7 +117,7 @@ describe('device-state/db-format', () => {
 		expect(app).to.have.property('appName').that.equals('test-app');
 		expect(app).to.have.property('source').that.equals(apiEndpoint);
 		expect(app).to.have.property('services').that.has.lengthOf(1);
-		expect(app).to.have.property('volumes').that.deep.equals({});
+		expect(app).to.have.property('volumes').that.deep.equals([]);
 		expect(app)
 			.to.have.property('networks')
 			.that.deep.equals(getDefaultNetwork(1));
@@ -194,7 +192,7 @@ describe('device-state/db-format', () => {
 		expect(app).to.have.property('appName').that.equals('test-app');
 		expect(app).to.have.property('source').that.equals(apiEndpoint);
 		expect(app).to.have.property('services').that.has.lengthOf(1);
-		expect(app).to.have.property('volumes').that.deep.equals({});
+		expect(app).to.have.property('volumes').that.deep.equals([]);
 		expect(app)
 			.to.have.property('networks')
 			.that.deep.equals(getDefaultNetwork(1));
@@ -303,7 +301,7 @@ describe('device-state/db-format', () => {
 		expect(newapp).to.have.property('appName').that.equals('test-app');
 		expect(newapp).to.have.property('source').that.equals('local');
 		expect(newapp).to.have.property('services').that.has.lengthOf(1);
-		expect(newapp).to.have.property('volumes').that.deep.equals({});
+		expect(newapp).to.have.property('volumes').that.deep.equals([]);
 		expect(newapp)
 			.to.have.property('networks')
 			.that.deep.equals(getDefaultNetwork(1));

test/lib/state-helper.ts

@@ -109,14 +109,8 @@ export function createApps(
 			{
 				appId,
 				services: servicesByAppId[appId] ?? [],
-				networks: (networksByAppId[appId] ?? []).reduce(
-					(nets, n) => ({ ...nets, [n.name]: n }),
-					{},
-				),
-				volumes: (volumesByAppId[appId] ?? []).reduce(
-					(vols, v) => ({ ...vols, [v.name]: v }),
-					{},
-				),
+				networks: networksByAppId[appId] ?? [],
+				volumes: volumesByAppId[appId] ?? [],
 			},
 			target,
 		);

test/unit/compose/app.spec.ts

@@ -30,11 +30,8 @@ function createApp({
 			appId,
 			appUuid,
 			services,
-			networks: networks.reduce(
-				(res, net) => ({ ...res, [net.name]: net }),
-				{},
-			),
-			volumes: volumes.reduce((res, vol) => ({ ...res, [vol.name]: vol }), {}),
+			networks,
+			volumes,
 		},
 		isTarget,
 	);
@@ -425,7 +422,6 @@ describe('compose/app', () => {
 				networks: [
 					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
 				],
-				isTarget: true,
 			});
 			const target = createApp({ networks: [], isTarget: true });
 
@@ -438,6 +434,115 @@ describe('compose/app', () => {
 			});
 		});
 
+		it('should correctly remove default duplicate networks', () => {
+			const current = createApp({
+				networks: [defaultNetwork, defaultNetwork],
+			});
+			const target = createApp({
+				networks: [],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+
+			const [removeNetworkStep] = expectSteps('removeNetwork', steps);
+
+			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				name: 'default',
+			});
+		});
+
+		it('should correctly remove duplicate networks', () => {
+			const current = createApp({
+				networks: [
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+			});
+			const target = createApp({
+				networks: [
+					// The target is a single network
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+
+			const [removeNetworkStep] = expectSteps('removeNetwork', steps);
+
+			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				name: 'test-network',
+			});
+		});
+
+		it('should ignore the duplicates if there are changes already', () => {
+			const current = createApp({
+				networks: [
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+			});
+			const target = createApp({
+				networks: [
+					// The target is a single network
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {
+						config_only: true,
+					}),
+				],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+			const [removeNetworkStep] = expectSteps('removeNetwork', steps);
+
+			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				name: 'test-network',
+			});
+		});
+
+		// This should never happen because there can never be a service that is refencing
+		// a network that has a duplicate
+		it('should generate service kill steps if there are duplicate networks', async () => {
+			const current = createApp({
+				appUuid: 'deadbeef',
+				networks: [
+					defaultNetwork,
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+				services: [
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						composition: { networks: ['test-network'] },
+					}),
+				],
+			});
+			const target = createApp({
+				networks: [
+					Network.fromComposeObject('test-network', 1, 'deadbeef', {}),
+				],
+				services: [
+					await createService({
+						appId: 1,
+						appUuid: 'deadbeef',
+						composition: { networks: ['test-network'] },
+					}),
+				],
+				isTarget: true,
+			});
+
+			const steps = current.nextStepsForAppUpdate(defaultContext, target);
+
+			const [removeNetworkStep] = expectSteps('kill', steps);
+
+			expect(removeNetworkStep).to.have.property('current').that.deep.includes({
+				serviceName: 'test',
+			});
+		});
+
 		it('should correctly infer more than one network removal step', () => {
 			const current = createApp({
 				networks: [