From 0fa47f635bc9a05fea2d0f9b20a6cdc5a1797855 Mon Sep 17 00:00:00 2001 From: Cameron Diver Date: Thu, 25 Apr 2019 13:18:41 +0100 Subject: [PATCH 1/2] fix: Correctly handle multiple hosts ports pointing to a container port When assigning multiple host ports to a single container port before this change, the supervisor would incorrectly take only the first host port into consideration. This change makes it so that every host port per container port is considered. Closes: #986 Change-type: patch Signed-off-by: Cameron Diver --- src/compose/ports.ts | 23 ++++++++++++----------- src/compose/service.ts | 7 ++++++- test/16-ports.spec.coffee | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/compose/ports.ts b/src/compose/ports.ts index 5fdd0e20..6ccb0241 100644 --- a/src/compose/ports.ts +++ b/src/compose/ports.ts @@ -90,24 +90,25 @@ export class PortMap { public static fromDockerOpts(portBindings: PortBindings): PortMap[] { // Create a list of portBindings, rather than the map (which we can't // order) - const portMaps = _.map(portBindings, (hostObj, internalStr) => { + const portMaps = _.flatMap(portBindings, (hostObj, internalStr) => { const match = internalStr.match(DOCKER_OPTS_PORTS_REGEX); if (match == null) { throw new Error(`Could not parse docker port output: ${internalStr}`); } const internal = parseInt(match[1], 10); - const external = parseInt(hostObj[0].HostPort, 10); - - const host = hostObj[0].HostIp; const proto = match[2] || 'tcp'; - return new PortMap({ - internalStart: internal, - internalEnd: internal, - externalStart: external, - externalEnd: external, - protocol: proto, - host, + return _.map(hostObj, ({ HostIp, HostPort }) => { + const external = parseInt(HostPort, 10); + const host = HostIp; + return new PortMap({ + internalStart: internal, + internalEnd: internal, + externalStart: external, + externalEnd: external, + protocol: proto, + host, + }); }); }); diff --git a/src/compose/service.ts b/src/compose/service.ts index a0a252c1..6ab54089 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -775,7 +775,12 @@ export class Service { _.each(this.config.portMaps, pmap => { const { exposedPorts, portBindings } = pmap.toDockerOpts(); _.merge(exposed, exposedPorts); - _.merge(ports, portBindings); + _.mergeWith(ports, portBindings, (destVal, srcVal) => { + if (destVal == null) { + return srcVal; + } + return destVal.concat(srcVal); + }); }); // We also want to merge the compose and image exposedPorts diff --git a/test/16-ports.spec.coffee b/test/16-ports.spec.coffee index f84ea56c..b9b5fd7a 100644 --- a/test/16-ports.spec.coffee +++ b/test/16-ports.spec.coffee @@ -243,6 +243,28 @@ describe 'Ports', -> }) ]) + it 'should correctly detect multiple hosts ports on an internal port', -> + expect(PortMap.fromDockerOpts({ + '100/tcp': [{ HostIp: '123', HostPort: 200 }, { HostIp: '123', HostPort: 201 }] + })).to.deep.equal([ + new PortMap({ + internalStart: 100, + internalEnd: 100, + externalStart: 200, + externalEnd: 200, + protocol: 'tcp', + host: '123' + }) + new PortMap({ + internalStart: 100, + internalEnd: 100, + externalStart: 201, + externalEnd: 201, + protocol: 'tcp', + host: '123' + }) + ]) + describe 'Running container comparison', -> it 'should not consider order when comparing current and target state', -> portBindings = require('./data/ports/not-ascending/port-bindings.json') From 9e05bc2b715d4fce9d29f7a400e9dd8749dd134a Mon Sep 17 00:00:00 2001 From: Cameron Diver Date: Thu, 25 Apr 2019 13:48:27 +0100 Subject: [PATCH 2/2] misc: Fix spurious test errors Signed-off-by: Cameron Diver --- test/05-device-state.spec.coffee | 5 ++++- test/11-api-binder.spec.coffee | 5 ++++- test/14-application-manager.spec.coffee | 5 ++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/05-device-state.spec.coffee b/test/05-device-state.spec.coffee index a87436a8..daf3f9ea 100644 --- a/test/05-device-state.spec.coffee +++ b/test/05-device-state.spec.coffee @@ -198,13 +198,16 @@ describe 'deviceState', -> prepare() @db = new DB() @config = new Config({ @db }) + @logger = { + clearOutOfDateDBLogs: -> + } eventTracker = { track: console.log } stub(Service, 'extendEnvVars').callsFake (env) -> env['ADDITIONAL_ENV_VAR'] = 'foo' return env - @deviceState = new DeviceState({ @db, @config, eventTracker }) + @deviceState = new DeviceState({ @db, @config, eventTracker, @logger }) stub(@deviceState.applications.docker, 'getNetworkGateway').returns(Promise.resolve('172.17.0.1')) stub(@deviceState.applications.images, 'inspectByName').callsFake -> Promise.try -> diff --git a/test/11-api-binder.spec.coffee b/test/11-api-binder.spec.coffee index 32e3dbe6..745874f5 100644 --- a/test/11-api-binder.spec.coffee +++ b/test/11-api-binder.spec.coffee @@ -20,7 +20,10 @@ initModels = (filename) -> track: stub().callsFake (ev, props) -> console.log(ev, props) } - @deviceState = new DeviceState({ @db, @config, @eventTracker }) + @logger = { + clearOutOfDateDBLogs: -> + } + @deviceState = new DeviceState({ @db, @config, @eventTracker, @logger }) @apiBinder = new APIBinder({ @db, @config, @eventTracker, @deviceState }) @db.init() .then => diff --git a/test/14-application-manager.spec.coffee b/test/14-application-manager.spec.coffee index f2f96f49..999d8482 100644 --- a/test/14-application-manager.spec.coffee +++ b/test/14-application-manager.spec.coffee @@ -123,7 +123,10 @@ describe 'ApplicationManager', -> eventTracker = { track: console.log } - @deviceState = new DeviceState({ @db, @config, eventTracker }) + @logger = { + clearOutOfDateDBLogs: -> + } + @deviceState = new DeviceState({ @db, @config, eventTracker, @logger }) @applications = @deviceState.applications stub(@applications.images, 'inspectByName').callsFake (imageName) -> Promise.resolve({