valitydev/salt
14
0
Fork 0
mirror of https://github.com/valitydev/salt.git synced 2024-11-07 17:09:03 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix incorrect translation of docker port_bindings -> ports

Browse Source 
The logic which ensures that we expose ports which are being bound,
even when not explicitly configured, was done incorrectly. UDP ports
were being passed to the API as '1234/udp' instead of (1234, 'udp').
This results in the port not being exposed properly.

The logic has been corrected. Additionally both the "ports" input
translation function, as well as the post-processing code (where the
port numbers configured in port_bindings were being added) both
contained code to "fix" any ports which were configured using
'portnum/tcp', as these must be passed to the API simply as integers. To
reduce code duplication, this normalization is now only performed at the
very end of the post-processing function, after ports have been
translated, and any missing ports from the port_bindings have been
added.

The unit test for the port_bindings input translation code, which was
written based upon the same incorrect reading of the API docs that
resulted in the incorrect behavior, have been updated to confirm the
(now) correct behavior. The unit test for the ports input translation
code has been updated to reflect the new normalization behavior.

Finally, an integration test has been added to ensure that we properly
expose UDP ports which are added as part of the post-processing
function.
This commit is contained in:
Erik Johnson 2018-02-08 22:32:24 -06:00
parent 15a1ab4acc
commit f35d9f6fef
No known key found for this signature in database
GPG Key ID: 5E5583C437808F3F
3 changed files with 79 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
salt/utils/docker/translate/container.py
View File

@ -114,28 +114,32 @@ def _post_processing(kwargs, skip_translate, invalid):
actual_volumes.sort()
if kwargs.get('port_bindings') is not None \
and (skip_translate is True or
all(x not in skip_translate
for x in ('port_bindings', 'expose', 'ports'))):
and all(x not in skip_translate
for x in ('port_bindings', 'expose', 'ports')):
# Make sure that all ports defined in "port_bindings" are included in
# the "ports" param.
auto_ports = list(kwargs['port_bindings'])
if auto_ports:
actual_ports = []
# Sort list to make unit tests more reliable
for port in auto_ports:
if port in actual_ports:
continue
if isinstance(port, six.integer_types):
actual_ports.append((port, 'tcp'))
ports_to_bind = list(kwargs['port_bindings'])
if ports_to_bind:
ports_to_open = set(kwargs.get('ports', []))
for port_def in ports_to_bind:
if isinstance(port_def, six.integer_types):
ports_to_open.add(port_def)
else:
port, proto = port.split('/')
actual_ports.append((int(port), proto))
actual_ports.sort()
actual_ports = [
port if proto == 'tcp' else '{}/{}'.format(port, proto) for (port, proto) in actual_ports
]
kwargs.setdefault('ports', actual_ports)
port_num, proto = port_def.split('/')
ports_to_open.add((int(port_num), proto))
kwargs['ports'] = sorted(ports_to_open)
if 'ports' in kwargs \
and all(x not in skip_translate for x in ('expose', 'ports')):
# TCP ports should only be passed as the port number. Normalize the
# input so a port definition of 80/tcp becomes just 80 instead of
# (80, 'tcp').
for index, _ in enumerate(kwargs['ports']):
try:
if kwargs['ports'][index][1] == 'tcp':
kwargs['ports'][index] = ports_to_open[index][0]
except TypeError:
continue
# Functions below must match names of docker-py arguments
@ -552,13 +556,7 @@ def ports(val, **kwargs): # pylint: disable=unused-argument
raise SaltInvocationError(exc.__str__())
new_ports.update([helpers.get_port_def(x, proto)
for x in range(range_start, range_end + 1)])
ordered_new_ports = [
port if proto == 'tcp' else (port, proto) for (port, proto) in sorted(
[(new_port, 'tcp') if isinstance(new_port, six.integer_types) else new_port
for new_port in new_ports]
)
]
return ordered_new_ports
return sorted(new_ports)
def privileged(val, **kwargs): # pylint: disable=unused-argument

38
tests/integration/states/test_docker_container.py
View File

@ -444,6 +444,44 @@ class DockerContainerTestCase(ModuleCase, SaltReturnAssertsMixin):
image_info['Config']['Cmd']
)
@container_name
def test_running_with_port_bindings(self, name):
'''
This tests that the ports which are being bound are also exposed, even
when not explicitly configured. This test will create a container with
only some of the ports exposed, including some which aren't even bound.
The resulting containers exposed ports should contain all of the ports
defined in the "ports" argument, as well as each of the ports which are
being bound.
'''
# Create the container
ret = self.run_state(
'docker_container.running',
name=name,
image=self.image,
command='sleep 600',
shutdown_timeout=1,
port_bindings=[1234, '1235-1236', '2234/udp', '2235-2236/udp'],
ports=[1235, '2235/udp', 9999],
)
self.assertSaltTrueReturn(ret)
# Check the created container's port bindings and exposed ports. The
# port bindings should only contain the ports defined in the
# port_bindings argument, while the exposed ports should also contain
# the extra port (9999/tcp) which was included in the ports argument.
cinfo = self.run_function('docker.inspect_container', [name])
ports = ['1234/tcp', '1235/tcp', '1236/tcp',
'2234/udp', '2235/udp', '2236/udp']
self.assertEqual(
sorted(cinfo['HostConfig']['PortBindings']),
ports
)
self.assertEqual(
sorted(cinfo['Config']['ExposedPorts']),
ports + ['9999/tcp']
)
@container_name
def test_absent_with_stopped_container(self, name):
'''

30
tests/unit/utils/test_docker.py
View File

@ -1302,8 +1302,9 @@ class TranslateContainerInputTestCase(TranslateBase):
'3334/udp': ('10.4.5.6', 3334),
'5505/udp': ('10.7.8.9', 15505),
'5506/udp': ('10.7.8.9', 15506)},
'ports': [80, '81/udp', 3333, '3334/udp',
4505, 4506, '5505/udp', '5506/udp']}
'ports': [80, 3333, 4505, 4506,
(81, 'udp'), (3334, 'udp'),
(5505, 'udp'), (5506, 'udp')]}
)
# ip::containerPort - Bind a specific IP and an ephemeral port to a
@ -1327,8 +1328,9 @@ class TranslateContainerInputTestCase(TranslateBase):
'3334/udp': ('10.4.5.6',),
'5505/udp': ('10.7.8.9',),
'5506/udp': ('10.7.8.9',)},
'ports': [80, '81/udp', 3333, '3334/udp',
4505, 4506, '5505/udp', '5506/udp']}
'ports': [80, 3333, 4505, 4506,
(81, 'udp'), (3334, 'udp'),
(5505, 'udp'), (5506, 'udp')]}
)
# hostPort:containerPort - Bind a specific port on all of the host's
@ -1351,8 +1353,9 @@ class TranslateContainerInputTestCase(TranslateBase):
'3334/udp': 3334,
'5505/udp': 15505,
'5506/udp': 15506},
'ports': [80, '81/udp', 3333, '3334/udp',
4505, 4506, '5505/udp', '5506/udp']}
'ports': [80, 3333, 4505, 4506,
(81, 'udp'), (3334, 'udp'),
(5505, 'udp'), (5506, 'udp')]}
)
# containerPort - Bind an ephemeral port on all of the host's
@ -1372,8 +1375,9 @@ class TranslateContainerInputTestCase(TranslateBase):
'3334/udp': None,
'5505/udp': None,
'5506/udp': None},
'ports': [80, '81/udp', 3333, '3334/udp',
4505, 4506, '5505/udp', '5506/udp']},
'ports': [80, 3333, 4505, 4506,
(81, 'udp'), (3334, 'udp'),
(5505, 'udp'), (5506, 'udp')]}
)
# Test a mixture of different types of input
@ -1402,10 +1406,10 @@ class TranslateContainerInputTestCase(TranslateBase):
'19999/udp': None,
'20000/udp': None,
'20001/udp': None},
'ports': [80, '81/udp', 3333, '3334/udp',
4505, 4506, '5505/udp', '5506/udp',
9999, 10000, 10001, '19999/udp',
'20000/udp', '20001/udp']}
'ports': [80, 3333, 4505, 4506, 9999, 10000, 10001,
(81, 'udp'), (3334, 'udp'), (5505, 'udp'),
(5506, 'udp'), (19999, 'udp'),
(20000, 'udp'), (20001, 'udp')]}
)
# Error case: too many items (max 3)
@ -1510,7 +1514,7 @@ class TranslateContainerInputTestCase(TranslateBase):
self.translator,
ports=val,
),
{'ports': [1111, 2222, (3333, 'udp'), 4505, 4506]}
{'ports': [1111, 2222, 4505, 4506, (3333, 'udp')]}
)
# Error case: non-integer and non/string value