Find and remove duplicate networks

We have seen a few times devices with duplicated network names for some
reason. While we don't know the cause the networks get duplicates, this
can be disruptive for updates as trying to create a container referencing a duplicate
network results in a 400 error from the engine.

This commit finds and removes duplicate networks via the state engine,
this means that even if somehow a container could be referencing a
network that has been duplicated later somehow, this will remove the
container first.

While thies doesn't solve the problem of duplicate networks being
created in the first place, it will fix the state of the system to
correct the inconsistency.

Change-type: minor
Closes: #590

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/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: [