Show warning instead of exception for invalid network config

A previous PR (#1656) fixed validation for network ipam config,
checking that both network and subnet are defined for each ipam config entry
(as described in the docker documentation).

After that PR, the validations throws an exception if the network target state is incorrect,
but this turns out to be the wrong approach, because that exception is also triggered
when querying target state.

This isn't a problem in normal operation, but it is in local mode, because local
mode queries the old target state before sending a new one. Since the query fails,
the CLI can never push the new target state.

This PR replaces the exception with a warning on the logs, since a
misconfigured network won't cause any engine failures, it will just
prevent containers to communicate through the provided network.

A future improvement should move this validation to an earlier point in the process,
so the target state can get rejected before it even gets to a point it
can be used.

Relates-to: #1693
Change-type: patch
This commit is contained in:
Felipe Lalanne 2021-05-04 15:11:07 -04:00
parent 6fc91965fb
commit 5197a1330d
2 changed files with 51 additions and 38 deletions

View File

@ -5,14 +5,12 @@ import * as dockerode from 'dockerode';
import { docker } from '../lib/docker-utils'; import { docker } from '../lib/docker-utils';
import logTypes = require('../lib/log-types'); import logTypes = require('../lib/log-types');
import * as logger from '../logger'; import * as logger from '../logger';
import log from '../lib/supervisor-console';
import * as ComposeUtils from './utils'; import * as ComposeUtils from './utils';
import { ComposeNetworkConfig, NetworkConfig } from './types/network'; import { ComposeNetworkConfig, NetworkConfig } from './types/network';
import { import { InvalidNetworkNameError } from './errors';
InvalidNetworkConfigurationError,
InvalidNetworkNameError,
} from './errors';
export class Network { export class Network {
public appId: number; public appId: number;
@ -199,13 +197,17 @@ export class Network {
}, },
): void { ): void {
// Check if every ipam config entry has both a subnet and a gateway // Check if every ipam config entry has both a subnet and a gateway
_.each(_.get(config, 'ipam.config', []), ({ subnet, gateway }) => { if (
if (!subnet || !gateway) { _.some(
throw new InvalidNetworkConfigurationError( _.get(config, 'ipam.config', []),
'Network IPAM config entries must have both a subnet and gateway', ({ subnet, gateway }) => !subnet || !gateway,
); )
} ) {
}); log.warn(
'Network IPAM config entries must have both a subnet and gateway defined.' +
' Your network might not work properly otherwise.',
);
}
} }
public static generateDockerName(appId: number, name: string) { public static generateDockerName(appId: number, name: string) {

View File

@ -1,8 +1,11 @@
import { expect } from 'chai'; import { expect } from 'chai';
import * as sinon from 'sinon';
import { Network } from '../../../src/compose/network'; import { Network } from '../../../src/compose/network';
import { NetworkInspectInfo } from 'dockerode'; import { NetworkInspectInfo } from 'dockerode';
import { log } from '../../../src/lib/supervisor-console';
describe('compose/network', () => { describe('compose/network', () => {
describe('creating a network from a compose object', () => { describe('creating a network from a compose object', () => {
it('creates a default network configuration if no config is given', () => { it('creates a default network configuration if no config is given', () => {
@ -77,38 +80,46 @@ describe('compose/network', () => {
}); });
}); });
it('rejects IPAM configuration without both gateway and subnet', () => { it('warns about IPAM configuration without both gateway and subnet', () => {
expect(() => const logSpy = sinon.spy(log, 'warn');
Network.fromComposeObject('default', 12345, {
ipam: { Network.fromComposeObject('default', 12345, {
driver: 'default', ipam: {
config: [ driver: 'default',
{ config: [
subnet: '172.20.0.0/16', {
}, subnet: '172.20.0.0/16',
], },
options: {}, ],
}, options: {},
}), },
).to.throw( });
expect(logSpy).to.be.called.calledOnce;
expect(logSpy).to.be.called.calledWithMatch(
'Network IPAM config entries must have both a subnet and gateway', 'Network IPAM config entries must have both a subnet and gateway',
); );
expect(() => logSpy.resetHistory();
Network.fromComposeObject('default', 12345, {
ipam: { Network.fromComposeObject('default', 12345, {
driver: 'default', ipam: {
config: [ driver: 'default',
{ config: [
gateway: '172.20.0.1', {
}, gateway: '172.20.0.1',
], },
options: {}, ],
}, options: {},
}), },
).to.throw( });
expect(logSpy).to.be.called.calledOnce;
expect(logSpy).to.be.called.calledWithMatch(
'Network IPAM config entries must have both a subnet and gateway', 'Network IPAM config entries must have both a subnet and gateway',
); );
logSpy.restore();
}); });
it('parses values from a compose object', () => { it('parses values from a compose object', () => {