From 9bd216327fda4526cd60ccf98178960242383769 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 24 Oct 2023 14:58:16 -0300 Subject: [PATCH] Expose ports from port mappings on services PR #2217 removed the expose configuration but also caused a regresion where ports set via the `ports` configuration would no longer get exposed to the host, despite portmappings being set. This fixes that issue by exposing only those ports comming from port mappings. Change-type: patch --- src/compose/ports.ts | 5 ++++ src/compose/service.ts | 11 +++++--- test/integration/state-engine.spec.ts | 9 ++++++- test/unit/compose/ports.spec.ts | 11 ++++++++ test/unit/compose/service.spec.ts | 38 +++++++++++++-------------- 5 files changed, 50 insertions(+), 24 deletions(-) diff --git a/src/compose/ports.ts b/src/compose/ports.ts index 6e75b1b0..ee62d843 100644 --- a/src/compose/ports.ts +++ b/src/compose/ports.ts @@ -15,6 +15,7 @@ export interface PortBindings { } export interface DockerPortOptions { + exposedPorts: Dictionary<{}>; portBindings: PortBindings; } @@ -48,15 +49,19 @@ export class PortMap { this.ports.externalEnd, ); + const exposedPorts: { [key: string]: {} } = {}; const portBindings: PortBindings = {}; _.zipWith(internalRange, externalRange, (internal, external) => { + exposedPorts[`${internal}/${this.ports.protocol}`] = {}; + portBindings[`${internal}/${this.ports.protocol}`] = [ { HostIp: this.ports.host, HostPort: external.toString() }, ]; }); return { + exposedPorts, portBindings, }; } diff --git a/src/compose/service.ts b/src/compose/service.ts index d501a9d8..8e0a857c 100644 --- a/src/compose/service.ts +++ b/src/compose/service.ts @@ -651,7 +651,7 @@ export class Service { containerIds: Dictionary; }): Dockerode.ContainerCreateOptions { const { binds, mounts, volumes } = this.getBindsMountsAndVolumes(); - const { portBindings } = this.generatePortBindings(); + const { exposedPorts, portBindings } = this.generateExposeAndPorts(); const tmpFs: Dictionary<''> = this.config.tmpfs.reduce( (dict, tmp) => ({ ...dict, [tmp]: '' }), @@ -689,6 +689,7 @@ export class Service { this.config.environment, ), ), + ExposedPorts: exposedPorts, Image: this.config.image, Labels: this.config.labels, NetworkingConfig: @@ -927,11 +928,13 @@ export class Service { return { binds, mounts, volumes }; } - private generatePortBindings(): DockerPortOptions { + private generateExposeAndPorts(): DockerPortOptions { + const exposed: DockerPortOptions['exposedPorts'] = {}; const ports: DockerPortOptions['portBindings'] = {}; _.each(this.config.portMaps, (pmap) => { - const { portBindings } = pmap.toDockerOpts(); + const { exposedPorts, portBindings } = pmap.toDockerOpts(); + _.merge(exposed, exposedPorts); _.mergeWith(ports, portBindings, (destVal, srcVal) => { if (destVal == null) { return srcVal; @@ -940,7 +943,7 @@ export class Service { }); }); - return { portBindings: ports }; + return { exposedPorts: exposed, portBindings: ports }; } private static extendEnvVars( diff --git a/test/integration/state-engine.spec.ts b/test/integration/state-engine.spec.ts index 4b49c5c0..5fbb77b7 100644 --- a/test/integration/state-engine.spec.ts +++ b/test/integration/state-engine.spec.ts @@ -3,6 +3,7 @@ import * as Docker from 'dockerode'; import { TargetStateV2 } from '~/lib/legacy'; import * as request from 'supertest'; import { setTimeout as delay } from 'timers/promises'; +import { exec } from '~/lib/fs-utils'; const BALENA_SUPERVISOR_ADDRESS = process.env.BALENA_SUPERVISOR_ADDRESS || 'http://balena-supervisor:48484'; @@ -97,9 +98,11 @@ describe('state engine', () => { serviceName: 'one', restart: 'unless-stopped', running: true, - command: 'sleep infinity', + command: + 'sh -c "while true; do echo -n \'Hello World!!\' | nc -lv -p 8080; done"', stop_signal: 'SIGKILL', networks: ['default'], + ports: ['8080'], labels: {}, environment: {}, }, @@ -134,6 +137,10 @@ describe('state engine', () => { { Name: '/one_11_1_deadbeef', State: 'running' }, { Name: '/two_12_1_deadbeef', State: 'running' }, ]); + + // Test that the service is running and accesssible via port 8080 + // this will throw if the server does not respond + await exec('nc -v docker 8080 -z'); }); // This test recovery from issue #1576, where a device running a service from the target release diff --git a/test/unit/compose/ports.spec.ts b/test/unit/compose/ports.spec.ts index 31df250e..02430d5c 100644 --- a/test/unit/compose/ports.spec.ts +++ b/test/unit/compose/ports.spec.ts @@ -98,6 +98,9 @@ describe('compose/ports', function () { describe('toDockerOpts', function () { it('should correctly generate docker options', () => expect(new PortMapPublic('80').toDockerOpts()).to.deep.equal({ + exposedPorts: { + '80/tcp': {}, + }, portBindings: { '80/tcp': [{ HostIp: '', HostPort: '80' }], }, @@ -105,6 +108,14 @@ describe('compose/ports', function () { it('should correctly generate docker options for a port range', () => expect(new PortMapPublic('80-85').toDockerOpts()).to.deep.equal({ + exposedPorts: { + '80/tcp': {}, + '81/tcp': {}, + '82/tcp': {}, + '83/tcp': {}, + '84/tcp': {}, + '85/tcp': {}, + }, portBindings: { '80/tcp': [{ HostIp: '', HostPort: '80' }], '81/tcp': [{ HostIp: '', HostPort: '81' }], diff --git a/test/unit/compose/service.spec.ts b/test/unit/compose/service.spec.ts index 455899b6..a55e8ee7 100644 --- a/test/unit/compose/service.spec.ts +++ b/test/unit/compose/service.spec.ts @@ -112,6 +112,7 @@ describe('compose/service: unit tests', () => { serviceId: 3, imageId: 4, composition: { + // This will be ignored expose: [1000, '243/udp'], ports: ['2344', '2345:2354', '2346:2367/udp'], }, @@ -120,6 +121,7 @@ describe('compose/service: unit tests', () => { imageInfo: { Config: { ExposedPorts: { + // This will be ignored by generateExposeAndPorts '53/tcp': {}, '53/udp': {}, '2354/tcp': {}, @@ -129,7 +131,7 @@ describe('compose/service: unit tests', () => { } as any, ); - const ports = (s as any).generatePortBindings(); + const ports = (s as any).generateExposeAndPorts(); expect(ports.portBindings).to.deep.equal({ '2344/tcp': [ { @@ -150,6 +152,13 @@ describe('compose/service: unit tests', () => { }, ], }); + // Only exposes ports coming from the `ports` + // property + expect(ports.exposedPorts).to.deep.equal({ + '2344/tcp': {}, + '2354/tcp': {}, + '2367/udp': {}, + }); }); it('correctly handles port ranges', async () => { @@ -168,7 +177,7 @@ describe('compose/service: unit tests', () => { { appName: 'test' } as any, ); - const ports = (s as any).generatePortBindings(); + const ports = (s as any).generateExposeAndPorts(); expect(ports.portBindings).to.deep.equal({ '2000/tcp': [ { @@ -195,6 +204,13 @@ describe('compose/service: unit tests', () => { }, ], }); + + expect(ports.exposedPorts).to.deep.equal({ + '2000/tcp': {}, + '2001/tcp': {}, + '2002/tcp': {}, + '2003/tcp': {}, + }); }); it('should correctly handle large port ranges', async function () { @@ -213,23 +229,7 @@ describe('compose/service: unit tests', () => { { appName: 'test' } as any, ); - expect((s as any).generatePortBindings).to.not.throw; - }); - - it('should not report implied exposed ports from portMappings', async () => { - const service = await Service.fromComposeObject( - { - appId: 123456, - serviceId: 123456, - serviceName: 'test', - composition: { - ports: ['80:80', '100:100'], - }, - }, - { appName: 'test' } as any, - ); - - expect(service.config).to.not.have.property('expose'); + expect((s as any).generateExposeAndPorts()).to.not.throw; }); it('should correctly handle spaces in volume definitions', async () => {