From 7b77e45f69d6f46338f8f278998ef66c87bef2b2 Mon Sep 17 00:00:00 2001
From: Cameron Diver <cameron@resin.io>
Date: Mon, 14 May 2018 15:14:46 +0100
Subject: [PATCH] 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        | 112 ++++++++++++++++++++++++++++++++++++
 src/compose/service.coffee  |  74 ++++++++++++++----------
 test/04-service.spec.coffee |  67 ++++++++++++++++++++-
 3 files changed, 221 insertions(+), 32 deletions(-)
 create mode 100644 src/compose/ports.ts

diff --git a/src/compose/ports.ts b/src/compose/ports.ts
new file mode 100644
index 00000000..c3aa72b8
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/src/compose/service.coffee b/src/compose/service.coffee
index 39f9d78d..d078e7df 100644
--- a/src/compose/service.coffee
+++ b/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
diff --git a/test/04-service.spec.coffee b/test/04-service.spec.coffee
index d6096397..5460f7c6 100644
--- a/test/04-service.spec.coffee
+++ b/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(