Skip to content

Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aanand merged 3 commits into from
Sep 4, 2014
Merged

Additional validation for container volumes and ports. #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8157f08
Fix the return value of get_tty_width() it should return an int.
dnephin Aug 26, 2014
07fa169
Resolves #260, #301 and #449
dnephin Aug 26, 2014
24044fa
Some additional validation for container ports, and a couple extra te…
dnephin Aug 26, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/yml.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.

@dnephindnephinAug 24, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.

@nathanleclairenathanleclaireAug 27, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


```
volumes:
- /var/lib/mysql
- cache/:/tmp/cache
- ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down
2 changes: 1 addition & 1 deletion fig/cli/formatter.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
if len(tty_size) != 2:
return 80
_, width = tty_size
return width
return int(width)


class Formatter(object):
Expand Down
104 changes: 55 additions & 49 deletions fig/service.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
return self.start_container(container, **options)

def start_container(self, container=None, intermediate_container=None, **override_options):
if container is None:
container = self.create_container(**override_options)

options = self.options.copy()
options.update(override_options)

port_bindings = {}
container = container or self.create_container(**override_options)
options = dict(self.options, **override_options)
ports = dict(split_port(port) for port in options.get('ports') or [])

if options.get('ports', None) is not None:
for port in options['ports']:
internal_port, external_port = split_port(port)
port_bindings[internal_port] = external_port

volume_bindings = {}

if options.get('volumes', None) is not None:
for volume in options['volumes']:
if ':' in volume:
external_dir, internal_dir = volume.split(':')
volume_bindings[os.path.abspath(external_dir)] = {
'bind': internal_dir,
'ro': False,
}
volume_bindings = dict(
build_volume_binding(parse_volume_spec(volume))
for volume in options.get('volumes') or []
if ':' in volume)

privileged = options.get('privileged', False)
net = options.get('net', 'bridge')
dns = options.get('dns', None)

container.start(
links=self._get_links(link_to_self=override_options.get('one_off', False)),
port_bindings=port_bindings,
links=self._get_links(link_to_self=options.get('one_off', False)),
port_bindings=ports,
binds=volume_bindings,
volumes_from=self._get_volumes_from(intermediate_container),
privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
def start_or_create_containers(self):
containers = self.containers(stopped=True)

if len(containers) == 0:
if not containers:
log.info("Creating %s..." % self.next_container_name())
new_container = self.create_container()
return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
container_options['ports'] = ports

if 'volumes' in container_options:
container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
container_options['volumes'] = dict(
(parse_volume_spec(v).internal, {})
for v in container_options['volumes'])

if 'environment' in container_options:
if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
return name[1:]


def split_volume(v):
"""
If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
If v is of the format INTERNAL, returns (None, INTERNAL).
"""
if ':' in v:
return v.split(':', 1)
else:
return (None, v)
def parse_volume_spec(volume_config):
parts = volume_config.split(':')
if len(parts) > 3:
raise ConfigError("Volume %s has incorrect format, should be "
"external:internal[:mode]" % volume_config)

if len(parts) == 1:
return VolumeSpec(None, parts[0], 'rw')

if len(parts) == 2:
parts.append('rw')

external, internal, mode = parts
if mode not in ('rw', 'ro'):
raise ConfigError("Volume %s has invalid mode (%s), should be "
"one of: rw, ro." % (volume_config, mode))

return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
external = os.path.expanduser(volume_spec.external)
return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
if ':' in external_port:
external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
if external_ip:
if external_port:
external_port = (external_ip, external_port)
else:
external_port = (external_ip,)
return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
raise ConfigError('Invalid port "%s", should be '
'[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
internal_port, = parts
return internal_port, None
if len(parts) == 2:
external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?

@dnephindnephinAug 26, 2014

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.


def split_env(env):
Expand Down
71 changes: 68 additions & 3 deletions tests/unit/service_test.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
ConfigError,
split_port,
parse_volume_spec,
build_volume_binding,
)


class ServiceTest(unittest.TestCase):
def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
Service(name='foo', ports=['8000'])

def test_split_port(self):
def test_split_port_with_host_ip(self):
internal_port, external_port = split_port("127.0.0.1:1000:2000")
self.assertEqual(internal_port, "2000")
self.assertEqual(external_port, ("127.0.0.1", "1000"))

def test_split_port_with_protocol(self):
internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
self.assertEqual(internal_port, "2000/udp")
self.assertEqual(external_port, ("127.0.0.1", "1000"))

def test_split_port_with_host_ip_no_port(self):
internal_port, external_port = split_port("127.0.0.1::2000")
self.assertEqual(internal_port, "2000")
self.assertEqual(external_port, ("127.0.0.1",))
self.assertEqual(external_port, ("127.0.0.1", None))

def test_split_port_with_host_port(self):
internal_port, external_port = split_port("1000:2000")
self.assertEqual(internal_port, "2000")
self.assertEqual(external_port, "1000")

def test_split_port_no_host_port(self):
internal_port, external_port = split_port("2000")
self.assertEqual(internal_port, "2000")
self.assertEqual(external_port, None)

def test_split_port_invalid(self):
with self.assertRaises(ConfigError):
split_port("0.0.0.0:1000:2000:tcp")

def test_split_domainname_none(self):
service = Service('foo',
hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
opts = service._get_container_create_options({})
self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

def test_parse_volume_spec_only_one_path(self):
spec = parse_volume_spec('/the/volume')
self.assertEqual(spec, (None, '/the/volume', 'rw'))

def test_parse_volume_spec_internal_and_external(self):
spec = parse_volume_spec('external:interval')
self.assertEqual(spec, ('external', 'interval', 'rw'))

def test_parse_volume_spec_with_mode(self):
spec = parse_volume_spec('external:interval:ro')
self.assertEqual(spec, ('external', 'interval', 'ro'))

def test_parse_volume_spec_too_many_parts(self):
with self.assertRaises(ConfigError):
parse_volume_spec('one:two:three:four')

def test_parse_volume_bad_mode(self):
with self.assertRaises(ConfigError):
parse_volume_spec('one:two:notrw')

def test_build_volume_binding(self):
binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
self.assertEqual(
binding,
('/outside', dict(bind='/inside', ro=False)))

@mock.patch.dict(os.environ)
def test_build_volume_binding_with_environ(self):
os.environ['VOLUME_PATH'] = '/opt'
binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

@mock.patch.dict(os.environ)
def test_building_volume_binding_with_home(self):
os.environ['HOME'] = '/home/user'
binding = build_volume_binding(parse_volume_spec('~:/home/user'))
self.assertEqual(
binding,
('/home/user', dict(bind='/home/user', ro=False)))
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Additional validation for container volumes and ports. by dnephin · Pull Request #442 · docker/compose · GitHub
Skip to content
Additional validation for container volumes and ports.#442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

aanand
merged 3 commits into

from

Sep 4, 2014
 Merged


Additional validation for container volumes and ports.
#442
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8157f08

Fix the return value of get_tty_width() it should return an int.


dnephin Aug 26, 2014
07fa169

Resolves #260, #301 and #449


dnephin Aug 26, 2014
24044fa

Some additional validation for container ports, and a couple extra te…


dnephin Aug 26, 2014

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


8 changes: 6 additions & 2 deletions
docs/yml.md

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -74,14 +74,18 @@ expose:

### volumes

Mount paths as volumes, optionally specifying a path on the host machine (`HOST:CONTAINER`).
Mount paths as volumes, optionally specifying a path on the host machine
(`HOST:CONTAINER`), or an access mode (`HOST:CONTAINER:ro`).

Note: Mapping local volumes is currently unsupported on boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx) if want to map local volumes.
Note for fig on OSX: Mapping local volumes is currently unsupported on
boot2docker. We recommend you use [docker-osx](https://github.com/noplay/docker-osx)
if want to map local volumes on OSX.
@dnephindnephinAug 24, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen multiple people confused about this so I tried to clarify that this is only an issue on OSX, not linux.
@nathanleclairenathanleclaireAug 27, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

```
volumes:
 - /var/lib/mysql
 - cache/:/tmp/cache
 - ~/configs:/etc/configs/:ro
```

### volumes_from
Expand Down

2 changes: 1 addition & 1 deletion
fig/cli/formatter.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@ def get_tty_width():
 if len(tty_size) != 2:
 return 80
 _, width = tty_size
 return width
 return int(width)


class Formatter(object):
Expand Down

104 changes: 55 additions & 49 deletions
fig/service.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
from __future__ import unicode_literals
from __future__ import absolute_import
from collections import namedtuple
from .packages.docker.errors import APIError
import logging
import re
Expand DownExpand Up@@ -39,6 +40,9 @@ class ConfigError(ValueError):
 pass


VolumeSpec = namedtuple('VolumeSpec', 'external internal mode')


class Service(object):
 def __init__(self, name, client=None, project='default', links=None, volumes_from=None, **options):
 if not re.match('^%s+$' % VALID_NAME_CHARS, name):
Expand DownExpand Up@@ -214,37 +218,22 @@ def start_container_if_stopped(self, container, **options):
 return self.start_container(container, **options)

 def start_container(self, container=None, intermediate_container=None, **override_options):
 if container is None:
 container = self.create_container(**override_options)

 options = self.options.copy()
 options.update(override_options)

 port_bindings = {}
 container = container or self.create_container(**override_options)
 options = dict(self.options, **override_options)
 ports = dict(split_port(port) for port in options.get('ports') or [])

 if options.get('ports', None) is not None:
 for port in options['ports']:
 internal_port, external_port = split_port(port)
 port_bindings[internal_port] = external_port

 volume_bindings = {}

 if options.get('volumes', None) is not None:
 for volume in options['volumes']:
 if ':' in volume:
 external_dir, internal_dir = volume.split(':')
 volume_bindings[os.path.abspath(external_dir)] = {
 'bind': internal_dir,
 'ro': False,
 }
 volume_bindings = dict(
 build_volume_binding(parse_volume_spec(volume))
 for volume in options.get('volumes') or []
 if ':' in volume)

 privileged = options.get('privileged', False)
 net = options.get('net', 'bridge')
 dns = options.get('dns', None)

 container.start(
 links=self._get_links(link_to_self=override_options.get('one_off', False)),
 port_bindings=port_bindings,
 links=self._get_links(link_to_self=options.get('one_off', False)),
 port_bindings=ports,
 binds=volume_bindings,
 volumes_from=self._get_volumes_from(intermediate_container),
 privileged=privileged,
Expand All@@ -256,7 +245,7 @@ def start_container(self, container=None, intermediate_container=None, **overrid
 def start_or_create_containers(self):
 containers = self.containers(stopped=True)

 if len(containers) == 0:
 if not containers:
 log.info("Creating %s..." % self.next_container_name())
 new_container = self.create_container()
 return [self.start_container(new_container)]
Expand DownExpand Up@@ -338,7 +327,9 @@ def _get_container_create_options(self, override_options, one_off=False):
 container_options['ports'] = ports

 if 'volumes' in container_options:
 container_options['volumes'] = dict((split_volume(v)[1], {}) for v in container_options['volumes'])
 container_options['volumes'] = dict(
 (parse_volume_spec(v).internal, {})
 for v in container_options['volumes'])

 if 'environment' in container_options:
 if isinstance(container_options['environment'], list):
Expand DownExpand Up@@ -433,32 +424,47 @@ def get_container_name(container):
 return name[1:]


def split_volume(v):
 """
 If v is of the format EXTERNAL:INTERNAL, returns (EXTERNAL, INTERNAL).
 If v is of the format INTERNAL, returns (None, INTERNAL).
 """
 if ':' in v:
 return v.split(':', 1)
 else:
 return (None, v)
def parse_volume_spec(volume_config):
 parts = volume_config.split(':')
 if len(parts) > 3:
 raise ConfigError("Volume %s has incorrect format, should be "
 "external:internal[:mode]" % volume_config)

 if len(parts) == 1:
 return VolumeSpec(None, parts[0], 'rw')

 if len(parts) == 2:
 parts.append('rw')

 external, internal, mode = parts
 if mode not in ('rw', 'ro'):
 raise ConfigError("Volume %s has invalid mode (%s), should be "
 "one of: rw, ro." % (volume_config, mode))

 return VolumeSpec(external, internal, mode)


def build_volume_binding(volume_spec):
 internal = {'bind': volume_spec.internal, 'ro': volume_spec.mode == 'ro'}
 external = os.path.expanduser(volume_spec.external)
 return os.path.abspath(os.path.expandvars(external)), internal


def split_port(port):
port = str(port)
external_ip = None
if ':' in port:
external_port, internal_port = port.rsplit(':', 1)
 if ':' in external_port:
 external_ip, external_port = external_port.split(':', 1)
else:
external_port, internal_port = (None, port)
 if external_ip:
if external_port:
 external_port = (external_ip, external_port)
 else:
external_port = (external_ip,)
 return internal_port, external_port
parts = str(port).split(':')
if not 1 <= len(parts) <= 3:
 raise ConfigError('Invalid port "%s", should be '
 '[[remote_ip:]remote_port:]port[/protocol]' % port)

if len(parts) == 1:
 internal_port, = parts
return internal_port, None
 if len(parts) == 2:
 external_port, internal_port = parts
return internal_port, external_port

external_ip, external_port, internal_port = parts
 return internal_port, (external_ip, external_port or None)

@aanandaanandAug 26, 2014

Copy link


Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list-fiddling stuff is really confusing. Could we bring back the local variables, so I can see what's being returned in each case?
@dnephindnephinAug 26, 2014

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm happy to make some changes here. I think in order to validate the string properly, we still need to make it a list, but additional local variables I can add.

def split_env(env):
Expand Down

71 changes: 68 additions & 3 deletions
tests/unit/service_test.py

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
from __future__ import unicode_literals
from __future__ import absolute_import
import os

from .. import unittest
import mock

from fig import Service
from fig.service import ConfigError, split_port
from fig.service import (
 ConfigError,
 split_port,
 parse_volume_spec,
 build_volume_binding,
)


class ServiceTest(unittest.TestCase):
 def test_name_validations(self):
Expand All@@ -28,23 +38,35 @@ def test_config_validation(self):
 self.assertRaises(ConfigError, lambda: Service(name='foo', port=['8000']))
 Service(name='foo', ports=['8000'])

 def test_split_port(self):
 def test_split_port_with_host_ip(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_protocol(self):
 internal_port, external_port = split_port("127.0.0.1:1000:2000/udp")
 self.assertEqual(internal_port, "2000/udp")
 self.assertEqual(external_port, ("127.0.0.1", "1000"))

 def test_split_port_with_host_ip_no_port(self):
 internal_port, external_port = split_port("127.0.0.1::2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, ("127.0.0.1",))
 self.assertEqual(external_port, ("127.0.0.1", None))

 def test_split_port_with_host_port(self):
 internal_port, external_port = split_port("1000:2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, "1000")

 def test_split_port_no_host_port(self):
 internal_port, external_port = split_port("2000")
 self.assertEqual(internal_port, "2000")
 self.assertEqual(external_port, None)

 def test_split_port_invalid(self):
 with self.assertRaises(ConfigError):
 split_port("0.0.0.0:1000:2000:tcp")

 def test_split_domainname_none(self):
 service = Service('foo',
 hostname = 'name',
Expand DownExpand Up@@ -82,3 +104,46 @@ def test_split_domainname_weird(self):
 opts = service._get_container_create_options({})
 self.assertEqual(opts['hostname'], 'name.sub', 'hostname')
 self.assertEqual(opts['domainname'], 'domain.tld', 'domainname')


class ServiceVolumesTest(unittest.TestCase):

 def test_parse_volume_spec_only_one_path(self):
 spec = parse_volume_spec('/the/volume')
 self.assertEqual(spec, (None, '/the/volume', 'rw'))

 def test_parse_volume_spec_internal_and_external(self):
 spec = parse_volume_spec('external:interval')
 self.assertEqual(spec, ('external', 'interval', 'rw'))

 def test_parse_volume_spec_with_mode(self):
 spec = parse_volume_spec('external:interval:ro')
 self.assertEqual(spec, ('external', 'interval', 'ro'))

 def test_parse_volume_spec_too_many_parts(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:three:four')

 def test_parse_volume_bad_mode(self):
 with self.assertRaises(ConfigError):
 parse_volume_spec('one:two:notrw')

 def test_build_volume_binding(self):
 binding = build_volume_binding(parse_volume_spec('/outside:/inside'))
 self.assertEqual(
 binding,
 ('/outside', dict(bind='/inside', ro=False)))

 @mock.patch.dict(os.environ)
 def test_build_volume_binding_with_environ(self):
 os.environ['VOLUME_PATH'] = '/opt'
 binding = build_volume_binding(parse_volume_spec('${VOLUME_PATH}:/opt'))
 self.assertEqual(binding, ('/opt', dict(bind='/opt', ro=False)))

 @mock.patch.dict(os.environ)
 def test_building_volume_binding_with_home(self):
 os.environ['HOME'] = '/home/user'
 binding = build_volume_binding(parse_volume_spec('~:/home/user'))
 self.assertEqual(
 binding,
 ('/home/user', dict(bind='/home/user', ro=False)))