Store port ranges as ranges, to reduce memory usage

Before this change, port ranges were iterated and stored as an object
per port mapping. Now the port ranges are stored as ranges until they
need to be converted to objects. The need to convert to objects still
exists as this is the format which the docker remote API expects, but
hopefully this should alleviate bugs like #644 by making the memory more
shorter-lived.

Also added more tests.

Change-type: patch
Closes: #644
Signed-of-by: Cameron Diver <cameron@resin.io>

src/compose/ports.ts

@@ -0,0 +1,112 @@
+import * as _ from 'lodash';
+import TypedError = require('typed-error');
+
+// Adapted from https://github.com/docker/docker-py/blob/master/docker/utils/ports.py#L3
+const PORTS_REGEX =
+	/^(?:(?:([a-fA-F\d.:]+):)?([\d]*)(?:-([\d]+))?:)?([\d]+)(?:-([\d]+))?(?:\/(udp|tcp))?$/;
+
+export class InvalidPortDefinition extends TypedError { }
+
+export interface PortBindings {
+	[key: string]: Array<{ HostIp: string, HostPort: string }>;
+}
+
+export interface DockerPortOptions {
+	exposedPorts: Map<string, {}>;
+	portBindings: PortBindings;
+}
+
+export class PortMap {
+
+	private internalStart: number;
+	private internalEnd: number;
+	private externalStart: number;
+	private externalEnd: number;
+	private protocol: string;
+	private host: string;
+
+	public constructor(portStr: string) {
+		this.parsePortString(portStr);
+	}
+
+	public toDockerOpts(): DockerPortOptions {
+		const internalRange = this.generatePortRange(this.internalStart, this.internalEnd);
+		const externalRange = this.generatePortRange(this.externalStart, this.externalEnd);
+
+		const exposed: { [key: string]: {} } = {};
+		const portBindings: PortBindings = {};
+
+		_.zipWith(internalRange, externalRange, (internal, external) => {
+			exposed[`${internal}/${this.protocol}`] = {};
+
+			portBindings[`${internal}/${this.protocol}`] = [
+				{ HostIp: this.host, HostPort: external!.toString() },
+			];
+		});
+
+		return {
+			exposedPorts: exposed,
+			portBindings,
+		};
+	}
+
+	private parsePortString(portStr: string): void {
+		const match = portStr.match(PORTS_REGEX);
+		if (match == null) {
+			throw new InvalidPortDefinition(`Could not parse port definition: ${portStr}`);
+		}
+
+		let [
+			,
+			host = '',
+			external,
+			externalEnd,
+			internal,
+			internalEnd,
+			protocol = 'tcp',
+		] = match;
+
+		if (external == null) {
+			external = internal;
+		}
+
+		if (internalEnd == null) {
+			internalEnd = internal;
+		}
+
+		if (externalEnd == null) {
+			if (internal === internalEnd) {
+				// This is a special case to handle a:b
+				externalEnd = external;
+			} else {
+				// and this handles a-b
+				externalEnd = internalEnd;
+			}
+		}
+
+		this.internalStart = parseInt(internal, 10);
+		this.internalEnd = parseInt(internalEnd, 10);
+		this.externalStart = parseInt(external, 10);
+		this.externalEnd = parseInt(externalEnd, 10);
+		this.host = host;
+		this.protocol = protocol;
+
+		// Ensure we have the same range
+		if (this.internalEnd - this.internalStart !== this.externalEnd - this.externalStart) {
+			throw new InvalidPortDefinition(
+				`Range for internal and external ports does not match: ${portStr}`,
+			);
+		}
+	}
+
+	private generatePortRange(start: number, end: number): number[] {
+		if (start > end) {
+			throw new Error('Incorrect port range! The end port cannot be larger than the start port!');
+		}
+		if (start === end) {
+			return [ start ];
+		}
+
+		return _.range(start, end + 1);
+	}
+}

src/compose/service.coffee

@@ -7,12 +7,10 @@ constants = require '../lib/constants'
 conversions =  require '../lib/conversions'
 parseCommand = require('shell-quote').parse
 Duration = require 'duration-js'
+{ PortMap } = require './ports'
 
 validRestartPolicies = [ 'no', 'always', 'on-failure', 'unless-stopped' ]
 
-# Adapted from https://github.com/docker/docker-py/blob/master/docker/utils/ports.py#L3
-PORTS_REGEX = /^(?:(?:([a-fA-F\d.:]+):)?([\d]*)(?:-([\d]+))?:)?([\d]+)(?:-([\d]+))?(?:\/(udp|tcp))?$/
-
 parseMemoryNumber = (numAsString, defaultVal) ->
 	m = numAsString?.toString().match(/^([0-9]+)([bkmg]?)b?$/i)
 	if !m? and defaultVal?
@@ -273,7 +271,7 @@ module.exports = class Service
 			@extendLabels(opts.imageInfo)
 			@extendAndSanitiseVolumes(opts.imageInfo)
 			@extendAndSanitiseExposedPorts(opts.imageInfo)
-			{ @exposedPorts, @portBindings } = @getPortsAndPortBindings()
+			@portMappings = @getPortsAndPortBindings()
 			@devices = formatDevices(@devices)
 			@addFeaturesFromLabels(opts)
 			if @dns?
@@ -537,27 +535,26 @@ module.exports = class Service
 
 	# TODO: map ports for any of the possible formats "container:host/protocol", port ranges, etc.
 	getPortsAndPortBindings: =>
-		exposedPorts = {}
+		portMaps = _.map @ports, (p) -> new PortMap(p)
+		return PortMap.normalisePortMaps(portMaps)
+
+	generatePortBindings: =>
 		portBindings = {}
-		if @ports?
-			for port in @ports
-				m = port.match(PORTS_REGEX)
-				if m? # Ignore invalid port mappings
-					[ _unused, host = '', external, externalEnd, internal, internalEnd, protocol = 'tcp' ] = m
-					external ?= internal
-					internalEnd ?= internal
-					externalEnd ?= external
-					externalRange = _.map([external..externalEnd], String)
-					internalRange = _.map([internal..internalEnd], String)
-					if externalRange.length == internalRange.length # Ignore invalid port mappings
-						for hostPort, ind in externalRange
-							containerPort = internalRange[ind]
-							exposedPorts["#{containerPort}/#{protocol}"] = {}
-							portBindings["#{containerPort}/#{protocol}"] = [ { HostIp: host, HostPort: hostPort } ]
+		exposedPorts = {}
+		for portMap in @portMappings
+			ports = portMap.toDockerOpts()
+			_.merge(portBindings, ports.portBindings)
+			_.merge(exposedPorts, ports.exposedPorts)
+
+		# Any additonal exposed ports
 		if @expose?
 			for port in @expose
 				exposedPorts[port] = {}
-		return { exposedPorts, portBindings }
+
+		return {
+			portBindings,
+			exposedPorts
+		}
 
 	getBindsAndVolumes: =>
 		binds = []
@@ -578,6 +575,12 @@ module.exports = class Service
 		networkMode = @networkMode
 		if _.startsWith(networkMode, 'service:')
 			networkMode = "container:#{_.replace(networkMode, 'service:', '')}_#{@imageId}_#{@releaseId}"
+
+		# Generate port options
+		maps = @generatePortBindings()
+		portBindings = maps.portBindings
+		exposedPorts = maps.exposedPorts
+
 		conf = {
 			name: "#{@serviceName}_#{@imageId}_#{@releaseId}"
 			Image: @image
@@ -586,7 +589,7 @@ module.exports = class Service
 			Tty: true
 			Volumes: volumes
 			Env: _.map @environment, (v, k) -> k + '=' + v
-			ExposedPorts: @exposedPorts
+			ExposedPorts: exposedPorts
 			Labels: @labels
 			Domainname: @domainname
 			User: @user
@@ -597,7 +600,7 @@ module.exports = class Service
 				ShmSize: @shmSize
 				Privileged: @privileged
 				NetworkMode: networkMode
-				PortBindings: @portBindings
+				PortBindings: portBindings
 				Binds: binds
 				CapAdd: @capAdd
 				CapDrop: @capDrop
@@ -661,6 +664,10 @@ module.exports = class Service
 		_.isEmpty(_.xor(_.keys(@networks), _.keys(otherService.networks)))
 
 	isSameContainer: (otherService) =>
+		# We need computed fields to be present to compare two services
+		@portMappings ?= @getPortsAndPortBindings()
+		otherService.portMappings ?= otherService.getPortsAndPortBindings()
+
 		propertiesToCompare = [
 			'image'
 			'command'
@@ -669,8 +676,7 @@ module.exports = class Service
 			'privileged'
 			'restartPolicy'
 			'labels'
-			'portBindings'
-			'exposedPorts'
+			'portMappings'
 			'shmSize'
 			'cpuShares'
 			'cpuQuota'
@@ -710,11 +716,19 @@ module.exports = class Service
 			'groupAdd'
 			'securityOpt'
 		]
-		return _.isEqual(_.pick(this, propertiesToCompare), _.pick(otherService, propertiesToCompare)) and
-			_.isEqual(_.omit(@environment, [ 'RESIN_DEVICE_NAME_AT_INIT' ]), _.omit(otherService.environment, [ 'RESIN_DEVICE_NAME_AT_INIT' ])) and
-			@hasSameNetworks(otherService) and
-			_.every arraysToCompare, (property) =>
-				_.isEmpty(_.xorWith(this[property], otherService[property], _.isEqual))
+
+		equalProps = _.isEqual(_.pick(this, propertiesToCompare), _.pick(otherService, propertiesToCompare))
+		equalEnv = _.isEqual(
+			_.omit(@environment, [ 'RESIN_DEVICE_NAME_AT_INIT ']),
+			_.omit(otherService.environment, [ 'RESIN_DEVICE_NAME_AT_INIT '])
+		)
+		equalNetworks = @hasSameNetworks(otherService)
+		equalArrays = _.every arraysToCompare, (property) =>
+			_.isEmpty(_.xorWith(this[property], otherService[property], _.isEqual))
+
+		equal = equalProps and equalEnv and equalNetworks and equalArrays
+
+		return equal
 
 	isEqualExceptForRunningState: (otherService) =>
 		return @isSameContainer(otherService) and

test/04-service.spec.coffee

@@ -81,7 +81,8 @@ describe 'compose/service.cofee', ->
 				}
 			}
 		})
-		expect(s.portBindings).to.deep.equal({
+		ports = s.generatePortBindings()
+		expect(ports.portBindings).to.deep.equal({
 			'2344/tcp': [{
 				HostIp: '',
 				HostPort: '2344'
@@ -95,7 +96,7 @@ describe 'compose/service.cofee', ->
 				HostPort: '2346'
 			}]
 		})
-		expect(s.exposedPorts).to.deep.equal({
+		expect(ports.exposedPorts).to.deep.equal({
 			'1000/tcp': {}
 			'243/udp': {}
 			'2344/tcp': {}
@@ -105,6 +106,68 @@ describe 'compose/service.cofee', ->
 			'53/udp': {}
 		})
 
+	it 'correctly handles port ranges', ->
+		s = new Service({
+			appId: '1234'
+			serviceName: 'foo'
+			releaseId: 2
+			serviceId: 3
+			imageId: 4
+			expose: [
+				1000,
+				'243/udp'
+			],
+			ports: [
+				'1000-1003:2000-2003'
+			]
+		})
+
+		ports = s.generatePortBindings()
+		expect(ports.portBindings).to.deep.equal({
+			'2000/tcp': [
+				HostIp: ''
+				HostPort: '1000'
+			],
+			'2001/tcp': [
+				HostIp: ''
+				HostPort: '1001'
+			],
+			'2002/tcp': [
+				HostIp: ''
+				HostPort: '1002'
+			],
+			'2003/tcp': [
+				HostIp: ''
+				HostPort: '1003'
+			]
+		})
+
+		expect(ports.exposedPorts).to.deep.equal({
+			'1000/tcp': {}
+			'2000/tcp': {}
+			'2001/tcp': {}
+			'2002/tcp': {}
+			'2003/tcp': {}
+			'243/udp': {}
+		})
+
+	it 'should correctly handle large port ranges', ->
+		@timeout(60000)
+		s = new Service({
+			appId: '1234'
+			serviceName: 'foo'
+			releaseId: 2
+			serviceId: 3
+			imageId: 4
+			ports: [
+				'5-65536:5-65536/tcp'
+				'5-65536:5-65536/udp'
+			]
+		})
+
+		expect(s.generatePortBindings()).to.not.throw
+
+
 	describe 'parseMemoryNumber()', ->
 		makeComposeServiceWithLimit = (memLimit) ->
 			new Service(