Many improvements to docker network and container states

Much Improved Support for Docker Networking
===========================================

The `docker_network.present` state has undergone a full rewrite, which
includes the following improvements:

Full API Support for Network Management
---------------------------------------

The improvements made to input handling in the
`docker_container.running` state for 2017.7.0 have now been expanded to
docker_network.present`.  This brings with it full support for all
tunable configuration arguments.

Custom Subnets
--------------

Custom subnets can now be configured. Both IPv4 and mixed IPv4/IPv6
networks are supported.

Network Configuration in :py:func:`docker_container.running` States
-------------------------------------------------------------------

It is now possible to configure static IPv4/IPv6 addresses, as well as
links and labels.

Improved Handling of Images from Custom Registries
==================================================

Rather than attempting to parse the tag from the passed image name, Salt
will now resolve that tag down to an image ID and use that ID instead.

Due to this change, there are some backward-incompatible changes to
image management. See below for a full list of these changes.

Backward-incompatible Changes to Docker Image Management
--------------------------------------------------------

Passing image names to the following functions must now be done using separate
`repository` and `tag` arguments:

- `docker.build`
- `docker.commit`
- `docker.import`
- `docker.load`
- `docker.tag`
- `docker.sls_build`

Additionally, the `tag` argument must now be explicitly passed to the
`docker_image.present` state, unless the image is being pulled from a
docker registry.

doc/ref/configuration/minion.rst

@@ -1292,8 +1292,42 @@ The password used for HTTP proxy access.
 
     proxy_password: obolus
 
+.. conf_minion:: docker.compare_container_networks
+
+``docker.compare_container_networks``
+-------------------------------------
+
+.. versionadded:: Oxygen
+
+Default: ``{'static': ['Aliases', 'Links', 'IPAMConfig'], 'automatic': ['IPAddress', 'Gateway', 'GlobalIPv6Address', 'IPv6Gateway']}``
+
+Specifies which keys are examined by
+:py:func:`docker.compare_container_networks
+<salt.modules.dockermod.compare_container_networks>`.
+
+.. note::
+    This should not need to be modified unless new features added to Docker
+    result in new keys added to the network configuration which must be
+    compared to determine if two containers have different network configs.
+    This config option exists solely as a way to allow users to continue using
+    Salt to manage their containers after an API change, without waiting for a
+    new Salt release to catch up to the changes in the Docker API.
+
+.. code-block:: yaml
+
+    docker.compare_container_networks:
+      static:
+        - Aliases
+        - Links
+        - IPAMConfig
+      automatic:
+        - IPAddress
+        - Gateway
+        - GlobalIPv6Address
+        - IPv6Gateway
+
 Minion Execution Module Management
-========================
+==================================
 
 .. conf_minion:: disable_modules
 
@@ -1303,7 +1337,7 @@ Minion Execution Module Management
 Default: ``[]`` (all execution modules are enabled by default)
 
 The event may occur in which the administrator desires that a minion should not
-be able to execute a certain module. 
+be able to execute a certain module.
 
 However, the ``sys`` module is built into the minion and cannot be disabled.
 

doc/topics/releases/oxygen.rst

@@ -4,6 +4,80 @@
 Salt Release Notes - Codename Oxygen
 ====================================
 
+Lots of Docker Improvements
+---------------------------
+
+Much Improved Support for Docker Networking
+===========================================
+
+The :py:func:`docker_network.present <salt.states.docker_network.present>`
+state has undergone a full rewrite, which includes the following improvements:
+
+Full API Support for Network Management
+---------------------------------------
+
+The improvements made to input handling in the
+:py:func:`docker_container.running <salt.states.docker_container.running>`
+state for 2017.7.0 have now been expanded to :py:func:`docker_network.present
+<salt.states.docker_network.present>`. This brings with it full support for all
+tunable configuration arguments.
+
+Custom Subnets
+--------------
+
+Custom subnets can now be configured. Both IPv4 and mixed IPv4/IPv6 networks
+are supported. See :ref:`here <salt-states-docker-network-present-ipam>` for
+more information.
+
+Network Configuration in :py:func:`docker_container.running` States
+-------------------------------------------------------------------
+
+A long-requested feature has finally been added! It is now possible to
+configure static IPv4/IPv6 addresses, as well as links and labels. See
+:ref:`here <salt-states-docker-container-network-management>` for more
+informaion.
+
+.. note::
+    While the ``containers`` argument to :py:func:`docker_network.present`
+    will continue to be supported, it will no longer be the recommended way of
+    ensuring that a container is attached to a network.
+
+Improved Handling of Images from Custom Registries
+==================================================
+
+Rather than attempting to parse the tag from the passed image name, Salt will
+now resolve that tag down to an image ID and use that ID instead.
+
+.. important::
+    Due to this change, there are some backward-incompatible changes to image
+    management. See below for a full list of these changes.
+
+Backward-incompatible Changes to Docker Image Management
+********************************************************
+
+Passing image names to the following functions must now be done using separate
+``repository`` and ``tag`` arguments:
+
+- :py:func:`docker.build <salt.modules.dockermod.build>`
+- :py:func:`docker.commit <salt.modules.dockermod.commit>`
+- :py:func:`docker.import <salt.modules.dockermod.import_>`
+- :py:func:`docker.load <salt.modules.dockermod.load>`
+- :py:func:`docker.tag <salt.modules.dockermod.tag_>`
+- :py:func:`docker.sls_build <salt.modules.dockermod.sls_build>`
+
+Additionally, the ``tag`` argument must now be explicitly passed to the
+:py:func:`docker_image.present <salt.states.docker_image.present>` state,
+unless the image is being pulled from a docker registry.
+
+`start` Argument Added to :py:func:`docker.create <salt.modules.dockermod.create>` Function
+-------------------------------------------------------------------------------------------
+
+This allows for ``docker run``-like functionality. For example:
+
+.. code-block:: bash
+
+    salt myminion docker.create image=foo/bar:baz command=/path/to/command start=True
+
 Comparison Operators in Package Installation
 --------------------------------------------
 

salt/config/__init__.py

@@ -1153,6 +1153,9 @@ VALID_OPTS = {
     # part of the extra_minion_data param
     # Subconfig entries can be specified by using the ':' notation (e.g. key:subkey)
     'pass_to_ext_pillars': (six.string_types, list),
+
+    # Used by salt.modules.dockermod.compare_container_networks to specify which keys are compared
+    'docker.compare_container_networks': dict,
 }
 
 # default configurations
@@ -1432,6 +1435,11 @@ DEFAULT_MINION_OPTS = {
     'extmod_whitelist': {},
     'extmod_blacklist': {},
     'minion_sign_messages': False,
+    'docker.compare_container_networks': {
+        'static': ['Aliases', 'Links', 'IPAMConfig'],
+        'automatic': ['IPAddress', 'Gateway',
+                      'GlobalIPv6Address', 'IPv6Gateway'],
+    },
 }
 
 DEFAULT_MASTER_OPTS = {

salt/states/docker_container.py

@@ -52,6 +52,7 @@ import logging
 from salt.exceptions import CommandExecutionError
 import copy
 import salt.utils.args
+import salt.utils.data
 import salt.utils.docker
 from salt.ext import six
 
@@ -94,6 +95,8 @@ def _check_diff(changes):
     https://github.com/saltstack/salt/pull/39996#issuecomment-288025200
     '''
     for conf_dict in changes:
+        if conf_dict == 'Networks':
+            continue
         for item in changes[conf_dict]:
             if changes[conf_dict][item]['new'] is None:
                 old = changes[conf_dict][item]['old']
@@ -109,6 +112,16 @@ def _check_diff(changes):
     return False
 
 
+def _get_networks(data):
+    '''
+    Pass either a container name/ID or the result of inspecting the container,
+    and get back the container's networks.
+    '''
+    if not isinstance(data, dict):
+        data = __salt__['docker.inspect_container'](data)
+    return data.get('NetworkSettings', {}).get('Networks', {})
+
+
 def running(name,
             image=None,
             skip_translate=None,
@@ -117,8 +130,9 @@ def running(name,
             force=False,
             watch_action='force',
             start=True,
-            shutdown_timeout=salt.utils.docker.SHUTDOWN_TIMEOUT,
+            shutdown_timeout=None,
             client_timeout=salt.utils.docker.CLIENT_TIMEOUT,
+            networks=None,
             **kwargs):
     '''
     Ensure that a container with a specific configuration is present and
@@ -128,12 +142,9 @@ def running(name,
         Name of the container
 
     image
-        Image to use for the container. Image names can be specified either
-        using ``repo:tag`` notation, or just the repo name (in which case a tag
-        of ``latest`` is assumed).
+        Image to use for the container
 
         .. note::
-
             This state will pull the image if it is not present. However, if
             the image needs to be built from a Dockerfile or loaded from a
             saved image, or if you would like to use requisites to trigger a
@@ -182,7 +193,7 @@ def running(name,
               docker_container.running:
                 - image: 7.3.1611
                 - skip_translate: port_bindings
-                - port_bindings: {8080: [('10.2.9.10', 80)], '4193/udp', 9314}
+                - port_bindings: {8080: [('10.2.9.10', 80)], '4193/udp': 9314}
 
         See the following links for more information:
 
@@ -191,7 +202,7 @@ def running(name,
 
     .. _docker-py: https://pypi.python.org/pypi/docker-py
     .. _`docker-py Low-level API`: http://docker-py.readthedocs.io/en/stable/api.html#docker.api.container.ContainerApiMixin.create_container
-    .. _`Docker Engine API`: https://docs.docker.com/engine/api/v1.26/#operation/ContainerCreate
+    .. _`Docker Engine API`: https://docs.docker.com/engine/api/v1.33/#operation/ContainerCreate
 
     ignore_collisions : False
         Since many of docker-py_'s arguments differ in name from their CLI
@@ -247,12 +258,12 @@ def running(name,
         instances such as this, the container only needs to be started the
         first time.
 
-    shutdown_timeout : 10
+    shutdown_timeout
         If the container needs to be replaced, the container will be stopped
-        using :py:func:`docker.stop <salt.modules.dockermod.stop>`. The value
-        of this parameter will be passed to :py:func:`docker.stop
-        <salt.modules.dockermod.stop>` as the ``timeout`` value, telling Docker
-        how long to wait for a graceful shutdown before killing the container.
+        using :py:func:`docker.stop <salt.modules.dockermod.stop>`. If a
+        ``shutdown_timout`` is not set, and the container was created using
+        ``stop_timeout``, that timeout will be used. If neither of these values
+        were set, then a timeout of 10 seconds will be used.
 
         .. versionchanged:: 2017.7.0
             This option was renamed from ``stop_timeout`` to
@@ -266,6 +277,44 @@ def running(name,
         .. note::
             This is only used if Salt needs to pull the requested image.
 
+    .. _salt-states-docker-container-network-management:
+
+    **NETWORK MANAGEMENT**
+
+    .. versionadded:: Oxygen
+
+    The ``networks`` argument can be used to ensure that a container is
+    attached to one or more networks. Optionally, arguments can be passed to
+    the networks. In the example below, ``net1`` is being configured with
+    arguments, while ``net2`` is being configured *without* arguments:
+
+    .. code-block:: yaml
+
+        foo:
+          docker_container.running:
+            - image: myuser/myimage:foo
+            - networks:
+              - net1:
+                - aliases:
+                  - bar
+                  - baz
+                - ipv4_address: 10.0.20.50
+              - net2
+            - require:
+              - docker_network: net1
+              - docker_network: net2
+
+    The supported arguments are the ones from the docker-py's
+    `connect_container_to_network`_ function (other than ``container`` and
+    ``net_id``).
+
+    .. important::
+        Unlike with the arguments described in the **CONTAINER CONFIGURATION
+        PARAMETERS** section below, these network configuration parameters are
+        not translated at all.  Consult the `connect_container_to_network`_
+        documentation for the correct type/format of data to pass.
+
+    .. _`connect_container_to_network`: https://docker-py.readthedocs.io/en/stable/api.html#docker.api.network.NetworkApiMixin.connect_container_to_network
 
     **CONTAINER CONFIGURATION PARAMETERS**
 
@@ -865,69 +914,36 @@ def running(name,
 
     labels
         Add metadata to the container. Labels can be set both with and without
-        values:
-
-        **WITH VALUES**
-
-        The below three examples are equivalent:
+        values, and labels with values can be passed either as ``key=value`` or
+        ``key: value `` pairs. For example, while the below would be very
+        confusing to read, it is technically valid, and demonstrates the
+        different ways in which labels can be passed:
 
         .. code-block:: yaml
 
-            foo:
-              docker_container.running:
-                - image: bar/baz:latest
-                - labels: label1=value1,label2=value2
-
-        .. code-block:: yaml
-
-            foo:
-              docker_container.running:
-                - image: bar/baz:latest
+            mynet:
+              docker_network.present:
                 - labels:
-                  - label1=value1
-                  - label2=value2
+                  - foo
+                  - bar=baz
+                  - hello: world
+
+        The labels can also simply be passed as a YAML dictionary, though this
+        can be error-prone due to some :ref:`idiosyncrasies
+        <yaml-idiosyncrasies>` with how PyYAML loads nested data structures:
 
         .. code-block:: yaml
 
             foo:
-              docker_container.running:
-                - image: bar/baz:latest
+              docker_network.present:
                 - labels:
-                  - label1: value1
-                  - label2: value2
+                    foo: ''
+                    bar: baz
+                    hello: world
 
-        The labels can also simply be passed as a dictionary, though this can
-        be error-prone due to some :ref:`idiosyncrasies <yaml-idiosyncrasies>`
-        with how PyYAML loads nested data structures:
-
-        .. code-block:: yaml
-
-            foo:
-              docker_container.running:
-                - image: bar/baz:latest
-                - labels:
-                    label1: value1
-                    label2: value2
-
-        **WITHOUT VALUES**
-
-        The below two examples are equivalent:
-
-        .. code-block:: yaml
-
-            foo:
-              docker_container.running:
-                - image: bar/baz:latest
-                - labels: label1,label2
-
-        .. code-block:: yaml
-
-            foo:
-              docker_container.running:
-                - image: bar/baz:latest
-                - labels:
-                    - label1
-                    - label2
+        .. versionchanged:: Oxygen
+            Methods for specifying labels can now be mixed. Earlier releases
+            required either labels with or without values.
 
     links
         Link this container to another. Links can be specified as a list of
@@ -1543,105 +1559,137 @@ def running(name,
     '''
     ret = {'name': name,
            'changes': {},
-           'result': False,
+           'result': True,
            'comment': ''}
+
     if image is None:
+        ret['result'] = False
         ret['comment'] = 'The \'image\' argument is required'
         return ret
     elif not isinstance(image, six.string_types):
         image = str(image)
 
+    networks = salt.utils.args.split_input(networks or [])
+    if not networks:
+        networks = {}
+    else:
+        # We don't want to recurse the repack, as the values of the kwargs
+        # being passed when connecting to the network will not be dictlists.
+        networks = salt.utils.data.repack_dictlist(networks)
+        if not networks:
+            ret['result'] = False
+            ret['comment'] = 'Invalid network configuration (see documentation)'
+            return ret
+        for net_name, net_conf in six.iteritems(networks):
+            if net_conf is None:
+                networks[net_name] = {}
+            else:
+                networks[net_name] = salt.utils.data.repack_dictlist(net_conf)
+                if not networks[net_name]:
+                    ret['result'] = False
+                    ret['comment'] = (
+                        'Invalid configuration for network \'{0}\' '
+                        '(see documentation)'.format(net_name)
+                    )
+                    return ret
+                for key in ('links', 'aliases'):
+                    try:
+                        networks[net_name][key] = salt.utils.args.split_input(
+                            networks[net_name][key]
+                        )
+                    except KeyError:
+                        continue
+
+        # Iterate over the networks again now, looking for
+        # incorrectly-formatted arguments
+        errors = []
+        for net_name, net_conf in six.iteritems(networks):
+            if net_conf is not None:
+                for key, val in six.iteritems(net_conf):
+                    if val is None:
+                        errors.append(
+                            'Config option \'{0}\' for network \'{1}\' is '
+                            'missing a value'.format(key, net_name)
+                        )
+        if errors:
+            ret['result'] = False
+            return _format_comments(ret, errors)
+
     comments = []
 
+    if networks:
+        try:
+            all_networks = [
+                x['Name'] for x in __salt__['docker.networks']()
+                if 'Name' in x
+            ]
+        except CommandExecutionError as exc:
+            ret.setdefault('warnings', []).append(
+                'Failed to get list of networks: {0}.'.format(exc)
+            )
+        else:
+            missing_networks = [
+                x for x in sorted(networks) if x not in all_networks]
+            if missing_networks:
+                ret['result'] = False
+                comments.append(
+                    'The following networks are not present: {0}'.format(
+                        ', '.join(missing_networks)
+                    )
+                )
+                return _format_comments(ret, comments)
+
     # Pop off the send_signal argument passed by the watch requisite
     send_signal = kwargs.pop('send_signal', False)
 
-    # Resolve the full name of the tag
-    expanded_tag = ':'.join(salt.utils.docker.get_repo_tag(image))
-    # Get all tags on the minion
-    all_tags = __salt__['docker.list_tags']()
+    image_id = __salt__['docker.resolve_image_id'](image)
 
-    resolved_id = None
-    if expanded_tag in all_tags:
-        image = expanded_tag
-    else:
-        # The passed image may have been an ID. Try to resolve the image ID by
-        # using docker.inspect_image.
-        try:
-            resolved_id = __salt__['docker.inspect_image'](image)['Id']
-        except CommandExecutionError:
-            # No matching image ID
-            pass
-        except KeyError:
-            ret.setdefault('warnings', []).append(
-                'No \'Id\' key in image inspect API return, this may be due '
-                'to a change in docker-py.'
-            )
-
-        if resolved_id is not None:
-            image = resolved_id
-        elif not __opts__['test']:
-            # Since we did not resolve the passed image to one of the image IDs
-            # on the minion, assume that the passed image is a tag that is not
-            # present on the minion (and must therefore be pulled).
+    if image_id is False:
+        if not __opts__['test']:
+            # Image not pulled locally, so try pulling it
             try:
                 pull_result = __salt__['docker.pull'](
                     image,
                     client_timeout=client_timeout,
                 )
             except Exception as exc:
-                ret['comment'] = 'Failed to pull {0}: {1}'.format(image, exc)
-                return ret
+                ret['result'] = False
+                comments.append('Failed to pull {0}: {1}'.format(image, exc))
+                return _format_comments(ret, comments)
             else:
                 ret['changes']['image'] = pull_result
+                # Try resolving again now that we've pulled
+                image_id = __salt__['docker.resolve_image_id'](image)
+                if image_id is False:
+                    # Shouldn't happen unless the pull failed
+                    ret['result'] = False
+                    comments.append(
+                        'Image \'{0}\' not present despite a docker pull '
+                        'raising no errors'.format(image)
+                    )
+                    return _format_comments(ret, comments)
 
-    if resolved_id is not None:
-        # No need to repeat the inspect
-        image_id = resolved_id
-    else:
-        try:
-            image_id = __salt__['docker.inspect_image'](image)['Id']
-        except CommandExecutionError:
-            if not __opts__['test']:
-                # Shouldn't happen unless the pull failed
-                ret['comment'] = (
-                    'Image \'{0}\' not present despite a docker pull raising '
-                    'no errors'.format(image)
-                )
-                return ret
-            image_id = None
-
-    if name in __salt__['docker.list_containers'](all=True):
-        try:
-            try:
-                current_image_id = \
-                    __salt__['docker.inspect_container'](name)['Image']
-            except KeyError:
-                ret['comment'] = (
-                    'Unable to detect current image for container \'{0}\'. '
-                    'This might be due to a change in the Docker API.'
-                    .format(name)
-                )
-                return ret
-        except CommandExecutionError as exc:
-            ret['comment'] = ('Error occurred checking for existence of '
-                              'container \'{0}\': {1}'.format(name, exc))
-            return ret
-    else:
+    try:
+        current_image_id = __salt__['docker.inspect_container'](name)['Image']
+    except CommandExecutionError:
         current_image_id = None
+    except KeyError:
+        ret['result'] = False
+        comments.append(
+            'Unable to detect current image for container \'{0}\'. '
+            'This might be due to a change in the Docker API.'.format(name)
+        )
+        return _format_comments(ret, comments)
 
     # Shorthand to make the below code more understandable
     exists = current_image_id is not None
 
-    if exists:
-        pre_state = __salt__['docker.state'](name)
-    else:
-        pre_state = None
+    pre_state = __salt__['docker.state'](name) if exists else None
 
-    # If True, we're definitely going to be using the temp container as the new
-    # container (because we're forcing the change, or because the image IDs
-    # differ). If False, we'll need to perform a comparison between it and the
-    # new container.
+    # If skip_comparison is True, we're definitely going to be using the temp
+    # container as the new container (because we're forcing the change, or
+    # because the image IDs differ). If False, we'll need to perform a
+    # comparison between it and the new container.
     skip_comparison = force or not exists or current_image_id != image_id
 
     if skip_comparison and __opts__['test']:
@@ -1658,10 +1706,11 @@ def running(name,
         )
         return _format_comments(ret, comments)
 
-    # Create temp container
+    # Create temp container (or just create the named container if the
+    # container does not already exist)
     try:
         temp_container = __salt__['docker.create'](
-            image,
+            image_id,
             name=name if not exists else None,
             skip_translate=skip_translate,
             ignore_collisions=ignore_collisions,
@@ -1670,6 +1719,7 @@ def running(name,
             **kwargs)
         temp_container_name = temp_container['Name']
     except KeyError as exc:
+        ret['result'] = False
         comments.append(
             'Key \'{0}\' missing from API response, this may be due to a '
             'change in the Docker Remote API. Please report this on the '
@@ -1678,6 +1728,7 @@ def running(name,
         )
         return _format_comments(ret, comments)
     except Exception as exc:
+        ret['result'] = False
         msg = exc.__str__()
         if isinstance(exc, CommandExecutionError) \
                 and isinstance(exc.info, dict) and 'invalid' in exc.info:
@@ -1691,8 +1742,11 @@ def running(name,
         return _format_comments(ret, comments)
 
     def _replace(orig, new):
+        rm_kwargs = {'stop': True}
+        if shutdown_timeout is not None:
+            rm_kwargs['timeout'] = shutdown_timeout
         ret['changes'].setdefault('container_id', {})['removed'] = \
-            __salt__['docker.rm'](name, stop=True)
+            __salt__['docker.rm'](name, **rm_kwargs)
         try:
             result = __salt__['docker.rename'](new, orig)
         except CommandExecutionError as exc:
@@ -1704,22 +1758,46 @@ def running(name,
             comments.append('Failed to replace container \'{0}\'')
         return result
 
+    def _delete_temp_container():
+        log.debug('Removing temp container \'%s\'', temp_container_name)
+        __salt__['docker.rm'](temp_container_name)
+
     # If we're not skipping the comparison, then the assumption is that
     # temp_container will be discarded, unless the comparison reveals
     # differences, in which case we'll set cleanup_temp = False to prevent it
     # from being cleaned.
     cleanup_temp = not skip_comparison
     try:
+        pre_net_connect = __salt__['docker.inspect_container'](
+            name if exists else temp_container_name)
+        for net_name, net_conf in six.iteritems(networks):
+            try:
+                __salt__['docker.connect_container_to_network'](
+                    temp_container_name,
+                    net_name,
+                    **net_conf)
+            except CommandExecutionError as exc:
+                # Shouldn't happen, stopped docker containers can be
+                # attached to networks even if the static IP lies outside
+                # of the network's subnet. An exception will be raised once
+                # you try to start the container, however.
+                ret['result'] = False
+                comments.append(exc.__str__())
+                return _format_comments(ret, comments)
+        post_net_connect = __salt__['docker.inspect_container'](
+            temp_container_name)
+
+        net_changes = __salt__['docker.compare_container_networks'](
+            pre_net_connect, post_net_connect)
+
         if not skip_comparison:
-            changes = __salt__['docker.compare_container'](
+            container_changes = __salt__['docker.compare_containers'](
                 name,
                 temp_container_name,
                 ignore='Hostname',
             )
-
-            if changes:
-                ret['changes']['container'] = changes
-                if _check_diff(changes):
+            if container_changes:
+                if _check_diff(container_changes):
                     ret.setdefault('warnings', []).append(
                         'The detected changes may be due to incorrect '
                         'handling of arguments in earlier Salt releases. If '
@@ -1730,6 +1808,8 @@ def running(name,
                         )
                     )
 
+                changes_ptr = ret['changes'].setdefault('container', {})
+                changes_ptr.update(container_changes)
                 if __opts__['test']:
                     ret['result'] = None
                     comments.append(
@@ -1738,41 +1818,53 @@ def running(name,
                             'created' if not exists else 'replaced'
                         )
                     )
-                    return _format_comments(ret, comments)
                 else:
                     # We don't want to clean the temp container, we'll be
                     # replacing the existing one with it.
                     cleanup_temp = False
                     # Replace the container
                     if not _replace(name, temp_container_name):
+                        ret['result'] = False
                         return _format_comments(ret, comments)
                     ret['changes'].setdefault('container_id', {})['added'] = \
                         temp_container['Id']
-
             else:
                 # No changes between existing container and temp container.
                 # First check if a requisite is asking to send a signal to the
                 # existing container.
                 if send_signal:
-                    try:
-                        __salt__['docker.signal'](name, signal=watch_action)
-                    except CommandExecutionError as exc:
+                    if __opts__['test']:
                         comments.append(
-                            'Failed to signal container: {0}'.format(exc)
+                            'Signal {0} would be sent to container'.format(
+                                watch_action
+                            )
                         )
-                        return _format_comments(ret, comments)
                     else:
-                        ret['changes']['signal'] = watch_action
-                        comments.append(
-                            'Sent signal {0} to container'.format(watch_action)
-                        )
-                elif ret['changes']:
+                        try:
+                            __salt__['docker.signal'](name, signal=watch_action)
+                        except CommandExecutionError as exc:
+                            ret['result'] = False
+                            comments.append(
+                                'Failed to signal container: {0}'.format(exc)
+                            )
+                            return _format_comments(ret, comments)
+                        else:
+                            ret['changes']['signal'] = watch_action
+                            comments.append(
+                                'Sent signal {0} to container'.format(watch_action)
+                            )
+                elif container_changes:
                     if not comments:
                         log.warning(
                             'docker_container.running: detected changes without '
                             'a specific comment for container \'%s\'', name
                         )
-                        comments.append('Container \'{0}\' changed.'.format(name))
+                        comments.append(
+                            'Container \'{0}\'{1} updated.'.format(
+                                name,
+                                ' would be' if __opts__['test'] else ''
+                            )
+                        )
                 else:
                     # Container was not replaced, no differences between the
                     # existing container and the temp container were detected,
@@ -1781,14 +1873,89 @@ def running(name,
                         'Container \'{0}\' is already configured as specified'
                         .format(name)
                     )
+
+        if net_changes:
+            ret['changes'].setdefault('container', {})['Networks'] = net_changes
+            if __opts__['test']:
+                ret['result'] = None
+                comments.append('Network configuration would be updated')
+            elif cleanup_temp:
+                # We only need to make network changes if the container
+                # isn't being replaced, since we would already have
+                # attached all the networks for purposes of comparison.
+                network_failure = False
+                for net_name in sorted(net_changes):
+                    errors = []
+                    disconnected = connected = False
+                    try:
+                        if name in __salt__['docker.connected'](net_name):
+                            __salt__['docker.disconnect_container_from_network'](
+                                name,
+                                net_name)
+                            disconnected = True
+                    except CommandExecutionError as exc:
+                        errors.append(exc.__str__())
+
+                    if net_name in networks:
+                        try:
+                            __salt__['docker.connect_container_to_network'](
+                                name,
+                                net_name,
+                                **networks[net_name])
+                            connected = True
+                        except CommandExecutionError as exc:
+                            errors.append(exc.__str__())
+                            if disconnected:
+                                # We succeeded in disconnecting but failed
+                                # to reconnect. This can happen if the
+                                # network's subnet has changed and we try
+                                # to reconnect with the same IP address
+                                # from the old subnet.
+                                for item in list(net_changes[net_name]):
+                                    if net_changes[net_name][item]['old'] is None:
+                                        # Since they'd both be None, just
+                                        # delete this key from the changes
+                                        del net_changes[net_name][item]
+                                    else:
+                                        net_changes[net_name][item]['new'] = None
+
+                    if errors:
+                        comments.extend(errors)
+                        network_failure = True
+
+                    ret['changes'].setdefault(
+                        'container', {}).setdefault(
+                            'Networks', {})[net_name] = net_changes[net_name]
+
+                    if disconnected and connected:
+                        comments.append(
+                            'Reconnected to network \'{0}\' with updated '
+                            'configuration'.format(net_name)
+                        )
+                    elif disconnected:
+                        comments.append(
+                            'Disconnected from network \'{0}\''.format(
+                                net_name
+                            )
+                        )
+                    elif connected:
+                        comments.append(
+                            'Connected to network \'{0}\''.format(net_name)
+                        )
+
+                if network_failure:
+                    ret['result'] = False
+                    return _format_comments(ret, comments)
     finally:
         if cleanup_temp:
-            log.debug('Removing temp container \'%s\'', temp_container_name)
-            __salt__['docker.rm'](temp_container_name)
+            _delete_temp_container()
 
     if skip_comparison:
-        if exists:
+        if not exists:
+            comments.append('Created container \'{0}\''.format(name))
+        else:
             if not _replace(name, temp_container):
+                ret['result'] = False
                 return _format_comments(ret, comments)
         ret['changes'].setdefault('container_id', {})['added'] = \
             temp_container['Id']
@@ -1804,17 +1971,49 @@ def running(name,
         if __opts__['test']:
             ret['result'] = None
             comments.append('Container would be started')
-            return _format_comments(ret, comments)
-        try:
-            post_state = __salt__['docker.start'](name)['state']['new']
-        except Exception as exc:
-            comments.append(
-                'Failed to start container \'{0}\': \'{1}\''.format(name, exc)
-            )
-            return _format_comments(ret, comments)
+        else:
+            try:
+                post_state = __salt__['docker.start'](name)['state']['new']
+            except Exception as exc:
+                ret['result'] = False
+                comments.append(
+                    'Failed to start container \'{0}\': \'{1}\''.format(name, exc)
+                )
+                return _format_comments(ret, comments)
     else:
         post_state = __salt__['docker.state'](name)
 
+    if not __opts__['test'] and post_state == 'running':
+        # Now that we're certain the container is running, check each modified
+        # network to see if the network went from static (or disconnected) to
+        # automatic IP configuration. If so, grab the automatically-assigned
+        # IPs and munge the changes dict to include them. Note that this can
+        # only be done after the container is started bceause automatic IPs are
+        # assigned at runtime.
+        contextkey = '.'.join((name, 'docker_container.running'))
+
+        def _get_nets():
+            if contextkey not in __context__:
+                new_container_info = \
+                    __salt__['docker.inspect_container'](name)
+                __context__[contextkey] = new_container_info.get(
+                    'NetworkSettings', {}).get('Networks', {})
+            return __context__[contextkey]
+        autoip_keys = __opts__['docker.compare_container_networks'].get('automatic', [])
+        for net_name, net_changes in six.iteritems(
+                ret['changes'].get('container', {}).get('Networks', {})):
+            if 'IPConfiguration' in net_changes \
+                    and net_changes['IPConfiguration']['new'] == 'automatic':
+                for key in autoip_keys:
+                    val = _get_nets().get(net_name, {}).get(key)
+                    if val:
+                        net_changes[key] = {'old': None, 'new': val}
+                        try:
+                            net_changes.pop('IPConfiguration')
+                        except KeyError:
+                            pass
+        __context__.pop(contextkey, None)
+
     if pre_state != post_state:
         ret['changes']['state'] = {'old': pre_state, 'new': post_state}
         if pre_state is not None:
@@ -1829,18 +2028,18 @@ def running(name,
         ret['changes']['image'] = {'old': current_image_id, 'new': image_id}
 
     if post_state != 'running' and start:
+        ret['result'] = False
         comments.append('Container is not running')
-    else:
-        ret['result'] = True
 
     return _format_comments(ret, comments)
 
 
 def stopped(name=None,
             containers=None,
-            shutdown_timeout=salt.utils.docker.SHUTDOWN_TIMEOUT,
+            shutdown_timeout=None,
             unpause=False,
-            error_on_absent=True):
+            error_on_absent=True,
+            **kwargs):
     '''
     Ensure that a container (or containers) is stopped
 
@@ -1873,9 +2072,12 @@ def stopped(name=None,
         all specified containers in a single run, rather than executing the
         state separately on each image (as it would in the first example).
 
-    shutdown_timeout : 10
+    shutdown_timeout
         Timeout for graceful shutdown of the container. If this timeout is
-        exceeded, the container will be killed.
+        exceeded, the container will be killed. If this value is not passed,
+        then the container's configured ``stop_timeout`` will be observed. If
+        ``stop_timeout`` was also unset on the container, then a timeout of 10
+        seconds will be used.
 
     unpause : False
         Set to ``True`` to unpause any paused containers before stopping. If
@@ -1961,7 +2163,10 @@ def stopped(name=None,
 
     stop_errors = []
     for target in to_stop:
-        changes = __salt__['docker.stop'](target, unpause=unpause)
+        stop_kwargs = {'unpause': unpause}
+        if shutdown_timeout:
+            stop_kwargs['timeout'] = shutdown_timeout
+        changes = __salt__['docker.stop'](target, **stop_kwargs)
         if changes['result'] is True:
             ret['changes'][target] = changes
         else:

salt/states/docker_image.py

@@ -42,6 +42,8 @@ import logging
 import salt.utils.docker
 import salt.utils.args
 from salt.ext.six.moves import zip
+from salt.ext import six
+from salt.exceptions import CommandExecutionError
 
 # Enable proper logging
 log = logging.getLogger(__name__)  # pylint: disable=invalid-name
@@ -61,6 +63,7 @@ def __virtual__():
 
 
 def present(name,
+            tag=None,
             build=None,
             load=None,
             force=False,
@@ -72,45 +75,56 @@ def present(name,
             saltenv='base',
             **kwargs):
     '''
-    Ensure that an image is present. The image can either be pulled from a
-    Docker registry, built from a Dockerfile, or loaded from a saved image.
-    Image names can be specified either using ``repo:tag`` notation, or just
-    the repo name (in which case a tag of ``latest`` is assumed).
-    Repo identifier is mandatory, we don't assume the default repository
-    is docker hub.
+    .. versionchanged:: Oxygen
+        The ``tag`` argument has been added. It is now required unless pulling
+        from a registry.
 
-    If neither of the ``build`` or ``load`` arguments are used, then Salt will
-    pull from the :ref:`configured registries <docker-authentication>`. If the
-    specified image already exists, it will not be pulled unless ``force`` is
-    set to ``True``. Here is an example of a state that will pull an image from
-    the Docker Hub:
+    Ensure that an image is present. The image can either be pulled from a
+    Docker registry, built from a Dockerfile, loaded from a saved image, or
+    built by running SLS files against a base image.
+
+    If none of the ``build``, ``load``, or ``sls`` arguments are used, then Salt
+    will pull from the :ref:`configured registries <docker-authentication>`. If
+    the specified image already exists, it will not be pulled unless ``force``
+    is set to ``True``. Here is an example of a state that will pull an image
+    from the Docker Hub:
 
     .. code-block:: yaml
 
-        myuser/myimage:mytag:
-          docker_image.present
+        myuser/myimage:
+          docker_image.present:
+            - tag: mytag
+
+    tag
+        Tag name for the image. Required when using ``build``, ``load``, or
+        ``sls`` to create the image, but optional if pulling from a repository.
+
+        .. versionadded:: Oxygen
 
     build
         Path to directory on the Minion containing a Dockerfile
 
         .. code-block:: yaml
 
-            myuser/myimage:mytag:
+            myuser/myimage:
               docker_image.present:
                 - build: /home/myuser/docker/myimage
+                - tag: mytag
 
-
-            myuser/myimage:mytag:
+            myuser/myimage:
               docker_image.present:
                 - build: /home/myuser/docker/myimage
+                - tag: mytag
                 - dockerfile: Dockerfile.alternative
 
-        .. versionadded:: 2016.11.0
-
         The image will be built using :py:func:`docker.build
         <salt.modules.dockermod.build>` and the specified image name and tag
         will be applied to it.
 
+        .. versionadded:: 2016.11.0
+        .. versionchanged: Oxygen
+            The ``tag`` must be manually specified using the ``tag`` argument.
+
     load
         Loads a tar archive created with :py:func:`docker.load
         <salt.modules.dockermod.load>` (or the ``docker load`` Docker CLI
@@ -118,9 +132,13 @@ def present(name,
 
         .. code-block:: yaml
 
-            myuser/myimage:mytag:
+            myuser/myimage:
               docker_image.present:
                 - load: salt://path/to/image.tar
+                - tag: mytag
+
+        .. versionchanged: Oxygen
+            The ``tag`` must be manually specified using the ``tag`` argument.
 
     force : False
         Set this parameter to ``True`` to force Salt to pull/build/load the
@@ -143,8 +161,9 @@ def present(name,
 
         .. code-block:: yaml
 
-            myuser/myimage:mytag:
+            myuser/myimage:
               docker_image.present:
+                - tag: latest
                 - sls:
                     - webapp1
                     - webapp2
@@ -152,6 +171,8 @@ def present(name,
                 - saltenv: base
 
         .. versionadded: 2017.7.0
+        .. versionchanged: Oxygen
+            The ``tag`` must be manually specified using the ``tag`` argument.
 
     base
         Base image with which to start :py:func:`docker.sls_build
@@ -170,29 +191,48 @@ def present(name,
            'result': False,
            'comment': ''}
 
-    if build is not None and load is not None:
-        ret['comment'] = 'Only one of \'build\' or \'load\' is permitted.'
+    if not isinstance(name, six.string_types):
+        name = str(name)
+
+    # At most one of the args that result in an image being built can be used
+    num_build_args = len([x for x in (build, load, sls) if x is not None])
+    if num_build_args > 1:
+        ret['comment'] = \
+            'Only one of \'build\', \'load\', or \'sls\' is permitted.'
         return ret
-
-    image = ':'.join(salt.utils.docker.get_repo_tag(name))
-    resolved_tag = __salt__['docker.resolve_tag'](image)
-
-    if resolved_tag is False:
-        # Specified image is not present
-        image_info = None
+    elif num_build_args == 1:
+        # If building, we need the tag to be specified
+        if not tag:
+            ret['comment'] = (
+                'The \'tag\' argument is required if any one of \'build\', '
+                '\'load\', or \'sls\' is used.'
+            )
+            return ret
+        if not isinstance(tag, six.string_types):
+            tag = str(tag)
+        full_image = ':'.join((name, tag))
     else:
+        if tag:
+            name = '{0}:{1}'.format(name, tag)
+        full_image = name
+
+    try:
+        image_info = __salt__['docker.inspect_image'](full_image)
+    except CommandExecutionError as exc:
+        msg = exc.__str__()
+        if '404' in msg:
+            # Image not present
+            image_info = None
+        else:
+            ret['comment'] = msg
+            return ret
+
+    if image_info is not None:
         # Specified image is present
         if not force:
             ret['result'] = True
-            ret['comment'] = 'Image \'{0}\' already present'.format(name)
+            ret['comment'] = 'Image {0} already present'.format(full_image)
             return ret
-        else:
-            try:
-                image_info = __salt__['docker.inspect_image'](name)
-            except Exception as exc:
-                ret['comment'] = \
-                    'Unable to get info for image \'{0}\': {1}'.format(name, exc)
-                return ret
 
     if build or sls:
         action = 'built'
@@ -203,12 +243,12 @@ def present(name,
 
     if __opts__['test']:
         ret['result'] = None
-        if (resolved_tag is not False and force) or resolved_tag is False:
-            ret['comment'] = 'Image \'{0}\' will be {1}'.format(name, action)
+        if (image_info is not None and force) or image_info is None:
+            ret['comment'] = 'Image {0} will be {1}'.format(full_image, action)
             return ret
 
     if build:
-        # get the functions default value and args
+        # Get the functions default value and args
         argspec = salt.utils.args.get_function_argspec(__salt__['docker.build'])
         # Map any if existing args from kwargs into the build_args dictionary
         build_args = dict(list(zip(argspec.args, argspec.defaults)))
@@ -218,30 +258,30 @@ def present(name,
         try:
             # map values passed from the state to the build args
             build_args['path'] = build
-            build_args['image'] = image
+            build_args['image'] = full_image
             build_args['dockerfile'] = dockerfile
             image_update = __salt__['docker.build'](**build_args)
         except Exception as exc:
             ret['comment'] = (
-                'Encountered error building {0} as {1}: {2}'
-                .format(build, image, exc)
+                'Encountered error building {0} as {1}: {2}'.format(
+                    build, full_image, exc
+                )
             )
             return ret
         if image_info is None or image_update['Id'] != image_info['Id'][:12]:
             ret['changes'] = image_update
 
     elif sls:
-        if isinstance(sls, list):
-            sls = ','.join(sls)
         try:
-            image_update = __salt__['docker.sls_build'](name=image,
+            image_update = __salt__['docker.sls_build'](repository=name,
+                                                        tag=tag,
                                                         base=base,
                                                         mods=sls,
                                                         saltenv=saltenv)
         except Exception as exc:
             ret['comment'] = (
-                'Encountered error using sls {0} for building {1}: {2}'
-                .format(sls, image, exc)
+                'Encountered error using SLS {0} for building {1}: {2}'
+                .format(sls, full_image, exc)
             )
             return ret
         if image_info is None or image_update['Id'] != image_info['Id'][:12]:
@@ -249,11 +289,13 @@ def present(name,
 
     elif load:
         try:
-            image_update = __salt__['docker.load'](path=load, image=image)
+            image_update = __salt__['docker.load'](path=load,
+                                                   repository=name,
+                                                   tag=tag)
         except Exception as exc:
             ret['comment'] = (
                 'Encountered error loading {0} as {1}: {2}'
-                .format(load, image, exc)
+                .format(load, full_image, exc)
             )
             return ret
         if image_info is None or image_update.get('Layers', []):
@@ -262,13 +304,13 @@ def present(name,
     else:
         try:
             image_update = __salt__['docker.pull'](
-                image,
+                name,
                 insecure_registry=insecure_registry,
                 client_timeout=client_timeout
             )
         except Exception as exc:
             ret['comment'] = \
-                'Encountered error pulling {0}: {1}'.format(image, exc)
+                'Encountered error pulling {0}: {1}'.format(full_image, exc)
             return ret
         if (image_info is not None and image_info['Id'][:12] == image_update
                 .get('Layers', {})
@@ -280,18 +322,28 @@ def present(name,
             # Only add to the changes dict if layers were pulled
             ret['changes'] = image_update
 
-    ret['result'] = bool(__salt__['docker.resolve_tag'](image))
+    try:
+        __salt__['docker.inspect_image'](full_image)
+        error = False
+    except CommandExecutionError:
+        msg = exc.__str__()
+        if '404' not in msg:
+            error = 'Failed to inspect image \'{0}\' after it was {1}: {2}'.format(
+                full_image, action, msg
+            )
 
-    if not ret['result']:
-        # This shouldn't happen, failure to pull should be caught above
-        ret['comment'] = 'Image \'{0}\' could not be {1}'.format(name, action)
-    elif not ret['changes']:
-        ret['comment'] = (
-            'Image \'{0}\' was {1}, but there were no changes'
-            .format(name, action)
-        )
+    if error:
+        ret['comment'] = error
     else:
-        ret['comment'] = 'Image \'{0}\' was {1}'.format(name, action)
+        ret['result'] = True
+        if not ret['changes']:
+            ret['comment'] = (
+                'Image \'{0}\' was {1}, but there were no changes'.format(
+                    name, action
+                )
+            )
+        else:
+            ret['comment'] = 'Image \'{0}\' was {1}'.format(full_image, action)
     return ret
 
 
@@ -362,19 +414,16 @@ def absent(name=None, images=None, force=False):
     elif name:
         targets = [name]
 
-    pre_tags = __salt__['docker.list_tags']()
     to_delete = []
     for target in targets:
-        resolved_tag = __salt__['docker.resolve_tag'](target, tags=pre_tags)
+        resolved_tag = __salt__['docker.resolve_tag'](target)
         if resolved_tag is not False:
             to_delete.append(resolved_tag)
-    log.debug('targets = {0}'.format(targets))
-    log.debug('to_delete = {0}'.format(to_delete))
 
     if not to_delete:
         ret['result'] = True
         if len(targets) == 1:
-            ret['comment'] = 'Image \'{0}\' is not present'.format(name)
+            ret['comment'] = 'Image {0} is not present'.format(name)
         else:
             ret['comment'] = 'All specified images are not present'
         return ret
@@ -382,11 +431,13 @@ def absent(name=None, images=None, force=False):
     if __opts__['test']:
         ret['result'] = None
         if len(to_delete) == 1:
-            ret['comment'] = ('Image \'{0}\' will be removed'
-                              .format(to_delete[0]))
+            ret['comment'] = 'Image {0} will be removed'.format(to_delete[0])
         else:
-            ret['comment'] = ('The following images will be removed: {0}'
-                              .format(', '.join(to_delete)))
+            ret['comment'] = (
+                'The following images will be removed: {0}'.format(
+                    ', '.join(to_delete)
+                )
+            )
         return ret
 
     result = __salt__['docker.rmi'](*to_delete, force=force)
@@ -397,8 +448,9 @@ def absent(name=None, images=None, force=False):
         if [x for x in to_delete if x not in post_tags]:
             ret['changes'] = result
             ret['comment'] = (
-                'The following image(s) failed to be removed: {0}'
-                .format(', '.join(failed))
+                'The following image(s) failed to be removed: {0}'.format(
+                    ', '.join(failed)
+                )
             )
         else:
             ret['comment'] = 'None of the specified images were removed'
@@ -410,11 +462,12 @@ def absent(name=None, images=None, force=False):
     else:
         ret['changes'] = result
         if len(to_delete) == 1:
-            ret['comment'] = 'Image \'{0}\' was removed'.format(to_delete[0])
+            ret['comment'] = 'Image {0} was removed'.format(to_delete[0])
         else:
             ret['comment'] = (
-                'The following images were removed: {0}'
-                .format(', '.join(to_delete))
+                'The following images were removed: {0}'.format(
+                    ', '.join(to_delete)
+                )
             )
         ret['result'] = True
 
@@ -430,5 +483,5 @@ def mod_watch(name, sfun=None, **kwargs):
     return {'name': name,
             'changes': {},
             'result': False,
-            'comment': ('watch requisite is not'
-                        ' implemented for {0}'.format(sfun))}
+            'comment': 'watch requisite is not implemented for '
+                       '{0}'.format(sfun)}

salt/utils/docker/__init__.py

@@ -15,6 +15,7 @@ import os
 import salt.utils.args
 import salt.utils.data
 import salt.utils.docker.translate
+from salt.utils.docker.translate.helpers import split as _split
 from salt.exceptions import CommandExecutionError, SaltInvocationError
 from salt.utils.args import get_function_argspec as _argspec
 
@@ -40,29 +41,6 @@ except ImportError:
     pass
 
 NOTSET = object()
-ALIASES = {
-    'cmd': 'command',
-    'cpuset': 'cpuset_cpus',
-    'dns_option': 'dns_opt',
-    'env': 'environment',
-    'expose': 'ports',
-    'interactive': 'stdin_open',
-    'ipc': 'ipc_mode',
-    'label': 'labels',
-    'memory': 'mem_limit',
-    'memory_swap': 'memswap_limit',
-    'publish': 'port_bindings',
-    'publish_all': 'publish_all_ports',
-    'restart': 'restart_policy',
-    'rm': 'auto_remove',
-    'sysctl': 'sysctls',
-    'security_opts': 'security_opt',
-    'ulimit': 'ulimits',
-    'user_ns_mode': 'userns_mode',
-    'volume': 'volumes',
-    'workdir': 'working_dir',
-}
-ALIASES_REVMAP = dict([(y, x) for x, y in six.iteritems(ALIASES)])
 
 # Default timeout as of docker-py 1.0.0
 CLIENT_TIMEOUT = 60
@@ -72,13 +50,10 @@ SHUTDOWN_TIMEOUT = 10
 log = logging.getLogger(__name__)
 
 
-def _split(item, sep=',', maxsplit=-1):
-    return [x.strip() for x in item.split(sep, maxsplit)]
-
-
 def get_client_args():
     if not HAS_DOCKER_PY:
         raise CommandExecutionError('docker Python module not imported')
+
     try:
         create_args = _argspec(docker.APIClient.create_container).args
     except AttributeError:
@@ -100,6 +75,28 @@ def get_client_args():
                 'Could not get create_host_config argspec'
             )
 
+    try:
+        func_ref = docker.api.network.NetworkApiMixin.create_network
+        if six.PY2:
+            try:
+                # create_network is decorated, so we have to dig into the closure
+                # created by functools.wraps
+                network_config_args = \
+                    _argspec(func_ref.__func__.__closure__[0].cell_contents).args
+            except (AttributeError, IndexError):
+                # functools.wraps changed (unlikely), bail out
+                network_config_args = []
+        else:
+            try:
+                # functools.wraps makes things a little easier in Python 3
+                network_config_args = _argspec(func_ref.__wrapped__).args
+            except AttributeError:
+                # functools.wraps changed (unlikely), bail out
+                network_config_args = []
+    except AttributeError:
+        # Function moved, bail out
+        network_config_args = []
+
     try:
         endpoint_config_args = \
             _argspec(docker.types.EndpointConfig.__init__).args
@@ -116,14 +113,25 @@ def get_client_args():
                     'Could not get create_endpoint_config argspec'
                 )
 
-    for arglist in (create_args, host_config_args, endpoint_config_args):
+    try:
+        ipam_args = _argspec(docker.types.IPAMPool.__init__).args
+    except AttributeError:
         try:
-            # The API version is passed automagically by the API code that
-            # imports these classes/functions and is not an arg that we will be
-            # passing, so remove it if present.
-            arglist.remove('version')
-        except ValueError:
-            pass
+            ipam_args = _argspec(docker.utils.create_ipam_pool).args
+        except AttributeError:
+            raise CommandExecutionError('Could not get ipam args')
+
+    for arglist in (create_args, host_config_args, network_config_args,
+                    endpoint_config_args, ipam_args):
+        # The API version is passed automagically by the API code that imports
+        # these classes/functions and is not an arg that we will be passing, so
+        # remove it if present. Similarly, don't include "self" if it shows up
+        # in the arglist.
+        for argname in ('version', 'self'):
+            try:
+                arglist.remove(argname)
+            except ValueError:
+                pass
 
     # Remove any args in host or networking config from the main config dict.
     # This keeps us from accidentally allowing args that docker-py has moved
@@ -138,51 +146,32 @@ def get_client_args():
 
     return {'create_container': create_args,
             'host_config': host_config_args,
-            'networking_config': endpoint_config_args}
+            'endpoint_config': endpoint_config_args,
+            'network_config': network_config_args,
+            'ipam_config': ipam_args}
 
 
-def get_repo_tag(image, default_tag='latest'):
+def translate_input(translator,
+                    skip_translate=None,
+                    ignore_collisions=False,
+                    validate_ip_addrs=True,
+                    **kwargs):
     '''
-    Resolves the docker repo:tag notation and returns repo name and tag
-    '''
-    if not isinstance(image, six.string_types):
-        image = str(image)
-    try:
-        r_name, r_tag = image.rsplit(':', 1)
-    except ValueError:
-        r_name = image
-        r_tag = default_tag
-    if not r_tag:
-        # Would happen if some wiseguy requests a tag ending in a colon
-        # (e.g. 'somerepo:')
-        log.warning(
-            'Assuming tag \'%s\' for repo \'%s\'', default_tag, image
-        )
-        r_tag = default_tag
-    elif '/' in r_tag:
-        # Public registry notation with no tag specified
-        # (e.g. foo.bar.com:5000/imagename)
-        return image, default_tag
-    return r_name, r_tag
-
-
-def translate_input(**kwargs):
-    '''
-    Translate CLI/SLS input into the format the API expects. A
-    ``skip_translate`` kwarg can be passed to control which arguments are
-    translated. It can be either a comma-separated list or an iterable
-    containing strings (e.g. a list or tuple), and members of that tuple will
-    have their translation skipped. Optionally, skip_translate can be set to
-    True to skip *all* translation.
+    Translate CLI/SLS input into the format the API expects. The ``translator``
+    argument must be a module containing translation functions, within
+    salt.utils.docker.translate. A ``skip_translate`` kwarg can be passed to
+    control which arguments are translated. It can be either a comma-separated
+    list or an iterable containing strings (e.g. a list or tuple), and members
+    of that tuple will have their translation skipped. Optionally,
+    skip_translate can be set to True to skip *all* translation.
     '''
     kwargs = salt.utils.args.clean_kwargs(**kwargs)
     invalid = {}
     collisions = []
 
-    skip_translate = kwargs.pop('skip_translate', None)
     if skip_translate is True:
         # Skip all translation
-        return kwargs, invalid, collisions
+        return kwargs
     else:
         if not skip_translate:
             skip_translate = ()
@@ -195,120 +184,148 @@ def translate_input(**kwargs):
                 log.error('skip_translate is not an iterable, ignoring')
                 skip_translate = ()
 
-    validate_ip_addrs = kwargs.pop('validate_ip_addrs', True)
+    try:
+        # Using list(kwargs) here because if there are any invalid arguments we
+        # will be popping them from the kwargs.
+        for key in list(kwargs):
+            real_key = translator.ALIASES.get(key, key)
+            if real_key in skip_translate:
+                continue
 
-    # Using list(kwargs) here because if there are any invalid arguments we
-    # will be popping them from the kwargs.
-    for key in list(kwargs):
-        real_key = ALIASES.get(key, key)
-        if real_key in skip_translate:
-            continue
+            # ipam_pools is designed to be passed as a list of actual
+            # dictionaries, but if each of the dictionaries passed has a single
+            # element, it will be incorrectly repacked.
+            if key != 'ipam_pools' and salt.utils.data.is_dictlist(kwargs[key]):
+                kwargs[key] = salt.utils.data.repack_dictlist(kwargs[key])
 
-        if salt.utils.data.is_dictlist(kwargs[key]):
-            kwargs[key] = salt.utils.data.repack_dictlist(kwargs[key])
+            try:
+                kwargs[key] = getattr(translator, real_key)(
+                    kwargs[key],
+                    validate_ip_addrs=validate_ip_addrs,
+                    skip_translate=skip_translate)
+            except AttributeError:
+                log.debug('No translation function for argument \'%s\'', key)
+                continue
+            except SaltInvocationError as exc:
+                kwargs.pop(key)
+                invalid[key] = exc.strerror
 
         try:
-            func = getattr(salt.utils.docker.translate, real_key)
-            kwargs[key] = func(kwargs[key], validate_ip_addrs=validate_ip_addrs)
+            translator._merge_keys(kwargs)
         except AttributeError:
-            log.debug('No translation function for argument \'%s\'', key)
-            continue
-        except SaltInvocationError as exc:
-            kwargs.pop(key)
-            invalid[key] = exc.strerror
+            pass
 
-    log_driver = kwargs.pop('log_driver', NOTSET)
-    log_opt = kwargs.pop('log_opt', NOTSET)
-    if 'log_config' not in kwargs:
-        # The log_config is a mixture of the CLI options --log-driver and
-        # --log-opt (which we support in Salt as log_driver and log_opt,
-        # respectively), but it must be submitted to the host config in the
-        # format {'Type': log_driver, 'Config': log_opt}. So, we need to
-        # construct this argument to be passed to the API from those two
-        # arguments.
-        if log_driver is not NOTSET or log_opt is not NOTSET:
-            kwargs['log_config'] = {
-                'Type': log_driver if log_driver is not NOTSET else 'none',
-                'Config': log_opt if log_opt is not NOTSET else {}
-            }
-
-    # Convert CLI versions of commands to their API counterparts
-    for key in ALIASES:
-        if key in kwargs:
-            new_key = ALIASES[key]
-            value = kwargs.pop(key)
-            if new_key in kwargs:
-                collisions.append(new_key)
-            else:
-                kwargs[new_key] = value
-
-    # Don't allow conflicting options to be set
-    if kwargs.get('port_bindings') is not None \
-            and kwargs.get('publish_all_ports'):
-        kwargs.pop('port_bindings')
-        invalid['port_bindings'] = 'Cannot be used when publish_all_ports=True'
-    if kwargs.get('hostname') is not None \
-            and kwargs.get('network_mode') == 'host':
-        kwargs.pop('hostname')
-        invalid['hostname'] = 'Cannot be used when network_mode=True'
-
-    # Make sure volumes and ports are defined to match the binds and port_bindings
-    if kwargs.get('binds') is not None \
-            and (skip_translate is True or
-                 all(x not in skip_translate
-                     for x in ('binds', 'volume', 'volumes'))):
-        # Make sure that all volumes defined in "binds" are included in the
-        # "volumes" param.
-        auto_volumes = []
-        if isinstance(kwargs['binds'], dict):
-            for val in six.itervalues(kwargs['binds']):
-                try:
-                    if 'bind' in val:
-                        auto_volumes.append(val['bind'])
-                except TypeError:
-                    continue
-        else:
-            if isinstance(kwargs['binds'], list):
-                auto_volume_defs = kwargs['binds']
-            else:
-                try:
-                    auto_volume_defs = _split(kwargs['binds'])
-                except AttributeError:
-                    auto_volume_defs = []
-            for val in auto_volume_defs:
-                try:
-                    auto_volumes.append(_split(val, ':')[1])
-                except IndexError:
-                    continue
-        if auto_volumes:
-            actual_volumes = kwargs.setdefault('volumes', [])
-            actual_volumes.extend([x for x in auto_volumes
-                                   if x not in actual_volumes])
-            # Sort list to make unit tests more reliable
-            actual_volumes.sort()
-
-    if kwargs.get('port_bindings') is not None \
-            and (skip_translate is True or
-                 all(x not in skip_translate
-                     for x in ('port_bindings', 'expose', 'ports'))):
-        # Make sure that all ports defined in "port_bindings" are included in
-        # the "ports" param.
-        auto_ports = list(kwargs['port_bindings'])
-        if auto_ports:
-            actual_ports = []
-            # Sort list to make unit tests more reliable
-            for port in auto_ports:
-                if port in actual_ports:
-                    continue
-                if isinstance(port, six.integer_types):
-                    actual_ports.append((port, 'tcp'))
+        # Convert CLI versions of commands to their docker-py counterparts
+        for key in translator.ALIASES:
+            if key in kwargs:
+                new_key = translator.ALIASES[key]
+                value = kwargs.pop(key)
+                if new_key in kwargs:
+                    collisions.append(new_key)
                 else:
-                    port, proto = port.split('/')
-                    actual_ports.append((int(port), proto))
-            actual_ports.sort()
-            actual_ports = [
-                port if proto == 'tcp' else '{}/{}'.format(port, proto) for (port, proto) in actual_ports
-            ]
-            kwargs.setdefault('ports', actual_ports)
+                    kwargs[new_key] = value
 
-    return kwargs, invalid, sorted(collisions)
+        try:
+            translator._post_processing(kwargs, skip_translate, invalid)
+        except AttributeError:
+            pass
+
+    except Exception as exc:
+        error_message = exc.__str__()
+        log.error(
+            'Error translating input: \'%s\'', error_message, exc_info=True)
+    else:
+        error_message = None
+
+    error_data = {}
+    if error_message is not None:
+        error_data['error_message'] = error_message
+    if invalid:
+        error_data['invalid'] = invalid
+    if collisions and not ignore_collisions:
+        for item in collisions:
+            error_data.setdefault('collisions', []).append(
+                '\'{0}\' is an alias for \'{1}\', they cannot both be used'
+                .format(translator.ALIASES_REVMAP[item], item)
+            )
+    if error_data:
+        raise CommandExecutionError(
+            'Failed to translate input', info=error_data)
+
+    return kwargs
+
+
+def create_ipam_config(*pools, **kwargs):
+    '''
+    Builds an IP address management (IPAM) config dictionary
+    '''
+    kwargs = salt.utils.args.clean_kwargs(**kwargs)
+
+    try:
+        # docker-py 2.0 and newer
+        pool_args = salt.utils.args.get_function_argspec(
+            docker.types.IPAMPool.__init__).args
+        create_pool = docker.types.IPAMPool
+        create_config = docker.types.IPAMConfig
+    except AttributeError:
+        # docker-py < 2.0
+        pool_args = salt.utils.args.get_function_argspec(
+            docker.utils.create_ipam_pool).args
+        create_pool = docker.utils.create_ipam_pool
+        create_config = docker.utils.create_ipam_config
+
+    for primary_key, alias_key in (('driver', 'ipam_driver'),
+                                   ('options', 'ipam_opts')):
+
+        if alias_key in kwargs:
+            alias_val = kwargs.pop(alias_key)
+            if primary_key in kwargs:
+                log.warning(
+                    'docker.create_ipam_config: Both \'%s\' and \'%s\' '
+                    'passed. Ignoring \'%s\'',
+                    alias_key, primary_key, alias_key
+                )
+            else:
+                kwargs[primary_key] = alias_val
+
+    if salt.utils.data.is_dictlist(kwargs.get('options')):
+        kwargs['options'] = salt.utils.data.repack_dictlist(kwargs['options'])
+
+    # Get all of the IPAM pool args that were passed as individual kwargs
+    # instead of in the *pools tuple
+    pool_kwargs = {}
+    for key in list(kwargs):
+        if key in pool_args:
+            pool_kwargs[key] = kwargs.pop(key)
+
+    pool_configs = []
+    if pool_kwargs:
+        pool_configs.append(create_pool(**pool_kwargs))
+    pool_configs.extend([create_pool(**pool) for pool in pools])
+
+    if pool_configs:
+        # Sanity check the IPAM pools. docker-py's type/function for creating
+        # an IPAM pool will allow you to create a pool with a gateway, IP
+        # range, or map of aux addresses, even when no subnet is passed.
+        # However, attempting to use this IPAM pool when creating the network
+        # will cause the Docker Engine to throw an error.
+        if any('Subnet' not in pool for pool in pool_configs):
+            raise SaltInvocationError('A subnet is required in each IPAM pool')
+        else:
+            kwargs['pool_configs'] = pool_configs
+
+    ret = create_config(**kwargs)
+    pool_dicts = ret.get('Config')
+    if pool_dicts:
+        # When you inspect a network with custom IPAM configuration, only
+        # arguments which were explictly passed are reflected. By contrast,
+        # docker-py will include keys for arguments which were not passed in
+        # but set the value to None. Thus, for ease of comparison, the below
+        # loop will remove all keys with a value of None from the generated
+        # pool configs.
+        for idx, _ in enumerate(pool_dicts):
+            for key in list(pool_dicts[idx]):
+                if pool_dicts[idx][key] is None:
+                    del pool_dicts[idx][key]
+
+    return ret

salt/utils/docker/translate/__init__.py

@@ -0,0 +1 @@
+# -*- coding: utf-8 -*-

salt/utils/docker/translate/container.py

@@ -1,278 +1,146 @@
 # -*- coding: utf-8 -*-
 '''
-Functions to translate input in the docker CLI format to the format desired by
-by the API.
+Functions to translate input for container creation
 '''
-
 # Import Python libs
 from __future__ import absolute_import
-import logging
 import os
 
 # Import Salt libs
-import salt.utils.network
 from salt.exceptions import SaltInvocationError
 
 # Import 3rd-party libs
 from salt.ext import six
-from salt.ext.six.moves import range, zip  # pylint: disable=import-error,redefined-builtin
+from salt.ext.six.moves import range  # pylint: disable=import-error,redefined-builtin
 
-log = logging.getLogger(__name__)
-NOTSET = object()
+# Import helpers
+from . import helpers
+
+ALIASES = {
+    'cmd': 'command',
+    'cpuset': 'cpuset_cpus',
+    'dns_option': 'dns_opt',
+    'env': 'environment',
+    'expose': 'ports',
+    'interactive': 'stdin_open',
+    'ipc': 'ipc_mode',
+    'label': 'labels',
+    'memory': 'mem_limit',
+    'memory_swap': 'memswap_limit',
+    'publish': 'port_bindings',
+    'publish_all': 'publish_all_ports',
+    'restart': 'restart_policy',
+    'rm': 'auto_remove',
+    'sysctl': 'sysctls',
+    'security_opts': 'security_opt',
+    'ulimit': 'ulimits',
+    'user_ns_mode': 'userns_mode',
+    'volume': 'volumes',
+    'workdir': 'working_dir',
+}
+ALIASES_REVMAP = dict([(y, x) for x, y in six.iteritems(ALIASES)])
 
 
-def _split(item, sep=',', maxsplit=-1):
-    return [x.strip() for x in item.split(sep, maxsplit)]
-
-
-def _get_port_def(port_num, proto='tcp'):
+def _merge_keys(kwargs):
     '''
-    Given a port number and protocol, returns the port definition expected by
-    docker-py. For TCP ports this is simply an integer, for UDP ports this is
-    (port_num, 'udp').
-
-    port_num can also be a string in the format 'port_num/udp'. If so, the
-    "proto" argument will be ignored. The reason we need to be able to pass in
-    the protocol separately is because this function is sometimes invoked on
-    data derived from a port range (e.g. '2222-2223/udp'). In these cases the
-    protocol has already been stripped off and the port range resolved into the
-    start and end of the range, and get_port_def() is invoked once for each
-    port number in that range. So, rather than munge udp ports back into
-    strings before passing them to this function, the function will see if it
-    has a string and use the protocol from it if present.
-
-    This function does not catch the TypeError or ValueError which would be
-    raised if the port number is non-numeric. This function either needs to be
-    run on known good input, or should be run within a try/except that catches
-    these two exceptions.
+    The log_config is a mixture of the CLI options --log-driver and --log-opt
+    (which we support in Salt as log_driver and log_opt, respectively), but it
+    must be submitted to the host config in the format {'Type': log_driver,
+    'Config': log_opt}. So, we need to construct this argument to be passed to
+    the API from those two arguments.
     '''
-    try:
-        port_num, _, port_num_proto = port_num.partition('/')
-    except AttributeError:
-        pass
-    else:
-        if port_num_proto:
-            proto = port_num_proto
-    try:
-        if proto.lower() == 'udp':
-            return int(port_num), 'udp'
-    except AttributeError:
-        pass
-    return int(port_num)
+    log_driver = kwargs.pop('log_driver', helpers.NOTSET)
+    log_opt = kwargs.pop('log_opt', helpers.NOTSET)
+    if 'log_config' not in kwargs:
+        if log_driver is not helpers.NOTSET \
+                or log_opt is not helpers.NOTSET:
+            kwargs['log_config'] = {
+                'Type': log_driver
+                    if log_driver is not helpers.NOTSET
+                    else 'none',
+                'Config': log_opt
+                    if log_opt is not helpers.NOTSET
+                    else {}
+            }
 
 
-def _get_port_range(port_def):
+def _post_processing(kwargs, skip_translate, invalid):
     '''
-    Given a port number or range, return a start and end to that range. Port
-    ranges are defined as a string containing two numbers separated by a dash
-    (e.g. '4505-4506').
-
-    A ValueError will be raised if bad input is provided.
+    Additional container-specific post-translation processing
     '''
-    if isinstance(port_def, six.integer_types):
-        # Single integer, start/end of range is the same
-        return port_def, port_def
-    try:
-        comps = [int(x) for x in _split(port_def, '-')]
-        if len(comps) == 1:
-            range_start = range_end = comps[0]
+    # Don't allow conflicting options to be set
+    if kwargs.get('port_bindings') is not None \
+            and kwargs.get('publish_all_ports'):
+        kwargs.pop('port_bindings')
+        invalid['port_bindings'] = 'Cannot be used when publish_all_ports=True'
+    if kwargs.get('hostname') is not None \
+            and kwargs.get('network_mode') == 'host':
+        kwargs.pop('hostname')
+        invalid['hostname'] = 'Cannot be used when network_mode=True'
+
+    # Make sure volumes and ports are defined to match the binds and port_bindings
+    if kwargs.get('binds') is not None \
+            and (skip_translate is True or
+                 all(x not in skip_translate
+                     for x in ('binds', 'volume', 'volumes'))):
+        # Make sure that all volumes defined in "binds" are included in the
+        # "volumes" param.
+        auto_volumes = []
+        if isinstance(kwargs['binds'], dict):
+            for val in six.itervalues(kwargs['binds']):
+                try:
+                    if 'bind' in val:
+                        auto_volumes.append(val['bind'])
+                except TypeError:
+                    continue
         else:
-            range_start, range_end = comps
-        if range_start > range_end:
-            raise ValueError('start > end')
-    except (TypeError, ValueError) as exc:
-        if exc.__str__() == 'start > end':
-            msg = (
-                'Start of port range ({0}) cannot be greater than end of '
-                'port range ({1})'.format(range_start, range_end)
-            )
-        else:
-            msg = '\'{0}\' is non-numeric or an invalid port range'.format(
-                port_def
-            )
-        raise ValueError(msg)
-    else:
-        return range_start, range_end
+            if isinstance(kwargs['binds'], list):
+                auto_volume_defs = kwargs['binds']
+            else:
+                try:
+                    auto_volume_defs = helpers.split(kwargs['binds'])
+                except AttributeError:
+                    auto_volume_defs = []
+            for val in auto_volume_defs:
+                try:
+                    auto_volumes.append(helpers.split(val, ':')[1])
+                except IndexError:
+                    continue
+        if auto_volumes:
+            actual_volumes = kwargs.setdefault('volumes', [])
+            actual_volumes.extend([x for x in auto_volumes
+                                   if x not in actual_volumes])
+            # Sort list to make unit tests more reliable
+            actual_volumes.sort()
+
+    if kwargs.get('port_bindings') is not None \
+            and (skip_translate is True or
+                 all(x not in skip_translate
+                     for x in ('port_bindings', 'expose', 'ports'))):
+        # Make sure that all ports defined in "port_bindings" are included in
+        # the "ports" param.
+        auto_ports = list(kwargs['port_bindings'])
+        if auto_ports:
+            actual_ports = []
+            # Sort list to make unit tests more reliable
+            for port in auto_ports:
+                if port in actual_ports:
+                    continue
+                if isinstance(port, six.integer_types):
+                    actual_ports.append((port, 'tcp'))
+                else:
+                    port, proto = port.split('/')
+                    actual_ports.append((int(port), proto))
+            actual_ports.sort()
+            actual_ports = [
+                port if proto == 'tcp' else '{}/{}'.format(port, proto) for (port, proto) in actual_ports
+            ]
+            kwargs.setdefault('ports', actual_ports)
 
 
-def _map_vals(val, *names, **extra_opts):
-    '''
-    Many arguments come in as a list of VAL1:VAL2 pairs, but map to a list
-    of dicts in the format {NAME1: VAL1, NAME2: VAL2}. This function
-    provides common code to handle these instances.
-    '''
-    fill = extra_opts.pop('fill', NOTSET)
-    expected_num_elements = len(names)
-    val = _translate_stringlist(val)
-    for idx, item in enumerate(val):
-        if not isinstance(item, dict):
-            elements = [x.strip() for x in item.split(':')]
-            num_elements = len(elements)
-            if num_elements < expected_num_elements:
-                if fill is NOTSET:
-                    raise SaltInvocationError(
-                        '\'{0}\' contains {1} value(s) (expected {2})'.format(
-                            item, num_elements, expected_num_elements
-                        )
-                    )
-                elements.extend([fill] * (expected_num_elements - num_elements))
-            elif num_elements > expected_num_elements:
-                raise SaltInvocationError(
-                    '\'{0}\' contains {1} value(s) (expected {2})'.format(
-                        item,
-                        num_elements,
-                        expected_num_elements if fill is NOTSET
-                            else 'up to {0}'.format(expected_num_elements)
-                    )
-                )
-            val[idx] = dict(zip(names, elements))
-    return val
-
-
-def _validate_ip(val):
-    try:
-        if not salt.utils.network.is_ip(val):
-            raise SaltInvocationError(
-                '\'{0}\' is not a valid IP address'.format(val)
-            )
-    except RuntimeError:
-        pass
-
-
-# Helpers to perform common translation actions
-def _translate_str(val):
-    return str(val) if not isinstance(val, six.string_types) else val
-
-
-def _translate_int(val):
-    if not isinstance(val, six.integer_types):
-        try:
-            val = int(val)
-        except (TypeError, ValueError):
-            raise SaltInvocationError('\'{0}\' is not an integer'.format(val))
-    return val
-
-
-def _translate_bool(val):
-    return bool(val) if not isinstance(val, bool) else val
-
-
-def _translate_dict(val):
-    '''
-    Not really translating, just raising an exception if it's not a dict
-    '''
-    if not isinstance(val, dict):
-        raise SaltInvocationError('\'{0}\' is not a dictionary'.format(val))
-    return val
-
-
-def _translate_command(val):
-    '''
-    Input should either be a single string, or a list of strings. This is used
-    for the two args that deal with commands ("command" and "entrypoint").
-    '''
-    if isinstance(val, six.string_types):
-        return val
-    elif isinstance(val, list):
-        for idx in range(len(val)):
-            if not isinstance(val[idx], six.string_types):
-                val[idx] = str(val[idx])
-    else:
-        # Make sure we have a string
-        val = str(val)
-    return val
-
-
-def _translate_bytes(val):
-    '''
-    These values can be expressed as an integer number of bytes, or a string
-    expression (i.e. 100mb, 1gb, etc.).
-    '''
-    try:
-        val = int(val)
-    except (TypeError, ValueError):
-        if not isinstance(val, six.string_types):
-            val = str(val)
-    return val
-
-
-def _translate_stringlist(val):
-    '''
-    On the CLI, these are passed as multiple instances of a given CLI option.
-    In Salt, we accept these as a comma-delimited list but the API expects a
-    Python list. This function accepts input and returns it back as a Python
-    list of strings. If the input is a string which is a comma-separated list
-    of items, split that string and return it.
-    '''
-    if not isinstance(val, list):
-        try:
-            val = _split(val)
-        except AttributeError:
-            val = _split(str(val))
-    for idx in range(len(val)):
-        if not isinstance(val[idx], six.string_types):
-            val[idx] = str(val[idx])
-    return val
-
-
-def _translate_device_rates(val, numeric_rate=True):
-    '''
-    CLI input is a list of PATH:RATE pairs, but the API expects a list of
-    dictionaries in the format [{'Path': path, 'Rate': rate}]
-    '''
-    val = _map_vals(val, 'Path', 'Rate')
-    for idx in range(len(val)):
-        try:
-            is_abs = os.path.isabs(val[idx]['Path'])
-        except AttributeError:
-            is_abs = False
-        if not is_abs:
-            raise SaltInvocationError(
-                'Path \'{Path}\' is not absolute'.format(**val[idx])
-            )
-
-        # Attempt to convert to an integer. Will fail if rate was specified as
-        # a shorthand (e.g. 1mb), this is OK as we will check to make sure the
-        # value is an integer below if that is what is required.
-        try:
-            val[idx]['Rate'] = int(val[idx]['Rate'])
-        except (TypeError, ValueError):
-            pass
-
-        if numeric_rate:
-            try:
-                val[idx]['Rate'] = int(val[idx]['Rate'])
-            except ValueError:
-                raise SaltInvocationError(
-                    'Rate \'{Rate}\' for path \'{Path}\' is '
-                    'non-numeric'.format(**val[idx])
-                )
-    return val
-
-
-def _translate_key_val(val, delimiter='='):
-    '''
-    CLI input is a list of key/val pairs, but the API expects a dictionary in
-    the format {key: val}
-    '''
-    if isinstance(val, dict):
-        return val
-    val = _translate_stringlist(val)
-    new_val = {}
-    for item in val:
-        try:
-            lvalue, rvalue = _split(item, delimiter, 1)
-        except (AttributeError, TypeError, ValueError):
-            raise SaltInvocationError(
-                '\'{0}\' is not a key{1}value pair'.format(item, delimiter)
-            )
-        new_val[lvalue] = rvalue
-    return new_val
-
-
-# Functions below must match names of API arguments
+# Functions below must match names of docker-py arguments
 def auto_remove(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def binds(val, **kwargs):  # pylint: disable=unused-argument
@@ -284,7 +152,7 @@ def binds(val, **kwargs):  # pylint: disable=unused-argument
     if not isinstance(val, dict):
         if not isinstance(val, list):
             try:
-                val = _split(val)
+                val = helpers.split(val)
             except AttributeError:
                 raise SaltInvocationError(
                     '\'{0}\' is not a dictionary or list of bind '
@@ -294,7 +162,7 @@ def binds(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def blkio_weight(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def blkio_weight_device(val, **kwargs):  # pylint: disable=unused-argument
@@ -302,7 +170,7 @@ def blkio_weight_device(val, **kwargs):  # pylint: disable=unused-argument
     CLI input is a list of PATH:WEIGHT pairs, but the API expects a list of
     dictionaries in the format [{'Path': path, 'Weight': weight}]
     '''
-    val = _map_vals(val, 'Path', 'Weight')
+    val = helpers.map_vals(val, 'Path', 'Weight')
     for idx in range(len(val)):
         try:
             val[idx]['Weight'] = int(val[idx]['Weight'])
@@ -315,205 +183,179 @@ def blkio_weight_device(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def cap_add(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def cap_drop(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def command(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_command(val)
+    return helpers.translate_command(val)
 
 
 def cpuset_cpus(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def cpuset_mems(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def cpu_group(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def cpu_period(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def cpu_shares(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def detach(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def device_read_bps(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_device_rates(val, numeric_rate=False)
+    return helpers.translate_device_rates(val, numeric_rate=False)
 
 
 def device_read_iops(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_device_rates(val, numeric_rate=True)
+    return helpers.translate_device_rates(val, numeric_rate=True)
 
 
 def device_write_bps(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_device_rates(val, numeric_rate=False)
+    return helpers.translate_device_rates(val, numeric_rate=False)
 
 
 def device_write_iops(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_device_rates(val, numeric_rate=True)
+    return helpers.translate_device_rates(val, numeric_rate=True)
 
 
 def devices(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def dns_opt(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def dns_search(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def dns(val, **kwargs):
-    val = _translate_stringlist(val)
+    val = helpers.translate_stringlist(val)
     if kwargs.get('validate_ip_addrs', True):
         for item in val:
-            _validate_ip(item)
+            helpers.validate_ip(item)
     return val
 
 
 def domainname(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def entrypoint(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_command(val)
+    return helpers.translate_command(val)
 
 
 def environment(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter='=')
+    return helpers.translate_key_val(val, delimiter='=')
 
 
 def extra_hosts(val, **kwargs):
-    val = _translate_key_val(val, delimiter=':')
+    val = helpers.translate_key_val(val, delimiter=':')
     if kwargs.get('validate_ip_addrs', True):
         for key in val:
-            _validate_ip(val[key])
+            helpers.validate_ip(val[key])
     return val
 
 
 def group_add(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def host_config(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_dict(val)
+    return helpers.translate_dict(val)
 
 
 def hostname(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def ipc_mode(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def isolation(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def labels(val, **kwargs):  # pylint: disable=unused-argument
-    '''
-    Can either be a list of label names, or a list of name=value pairs. The API
-    can accept either a list of label names or a dictionary mapping names to
-    values, so the value we translate will be different depending on the input.
-    '''
-    if not isinstance(val, dict):
-        val = _translate_stringlist(val)
-        try:
-            has_mappings = all('=' in x for x in val)
-        except TypeError:
-            has_mappings = False
-
-        if has_mappings:
-            # The try/except above where has_mappings was defined has
-            # already confirmed that all elements are strings, and that
-            # all contain an equal sign. So we do not need to enclose
-            # the split in another try/except.
-            val = dict([_split(x, '=', 1) for x in val])
-        else:
-            # Stringify any non-string values
-            for idx in range(len(val)):
-                if '=' in val[idx]:
-                    raise SaltInvocationError(
-                        'Mix of labels with and without values'
-                    )
-            return val
-    return val
+    return helpers.translate_labels(val)
 
 
 def links(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter=':')
+    return helpers.translate_key_val(val, delimiter=':')
 
 
 def log_driver(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def log_opt(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter='=')
+    return helpers.translate_key_val(val, delimiter='=')
 
 
 def lxc_conf(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter='=')
+    return helpers.translate_key_val(val, delimiter='=')
 
 
 def mac_address(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def mem_limit(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bytes(val)
+    return helpers.translate_bytes(val)
 
 
 def mem_swappiness(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def memswap_limit(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bytes(val)
+    return helpers.translate_bytes(val)
 
 
 def name(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def network_disabled(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def network_mode(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def oom_kill_disable(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def oom_score_adj(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def pid_mode(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def pids_limit(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def port_bindings(val, **kwargs):
@@ -531,9 +373,9 @@ def port_bindings(val, **kwargs):
     if not isinstance(val, dict):
         if not isinstance(val, list):
             try:
-                val = _split(val)
+                val = helpers.split(val)
             except AttributeError:
-                val = _split(str(val))
+                val = helpers.split(str(val))
 
         for idx in range(len(val)):
             if not isinstance(val[idx], six.string_types):
@@ -544,7 +386,7 @@ def port_bindings(val, **kwargs):
 
         bindings = {}
         for binding in val:
-            bind_parts = _split(binding, ':')
+            bind_parts = helpers.split(binding, ':')
             num_bind_parts = len(bind_parts)
             if num_bind_parts == 1:
                 # Single port or port range being passed through (no
@@ -556,7 +398,7 @@ def port_bindings(val, **kwargs):
                     )
                 container_port, _, proto = container_port.partition('/')
                 try:
-                    start, end = _get_port_range(container_port)
+                    start, end = helpers.get_port_range(container_port)
                 except ValueError as exc:
                     # Using __str__() to avoid deprecation warning for using
                     # the message attribute of the ValueError.
@@ -578,8 +420,10 @@ def port_bindings(val, **kwargs):
                     )
                 container_port, _, proto = bind_parts[1].partition('/')
                 try:
-                    cport_start, cport_end = _get_port_range(container_port)
-                    hport_start, hport_end = _get_port_range(bind_parts[0])
+                    cport_start, cport_end = \
+                        helpers.get_port_range(container_port)
+                    hport_start, hport_end = \
+                        helpers.get_port_range(bind_parts[0])
                 except ValueError as exc:
                     # Using __str__() to avoid deprecation warning for
                     # using the message attribute of the ValueError.
@@ -600,10 +444,11 @@ def port_bindings(val, **kwargs):
             elif num_bind_parts == 3:
                 host_ip, host_port = bind_parts[0:2]
                 if validate_ip_addrs:
-                    _validate_ip(host_ip)
+                    helpers.validate_ip(host_ip)
                 container_port, _, proto = bind_parts[2].partition('/')
                 try:
-                    cport_start, cport_end = _get_port_range(container_port)
+                    cport_start, cport_end = \
+                        helpers.get_port_range(container_port)
                 except ValueError as exc:
                     # Using __str__() to avoid deprecation warning for
                     # using the message attribute of the ValueError.
@@ -613,7 +458,8 @@ def port_bindings(val, **kwargs):
                     hport_list = [None] * len(cport_list)
                 else:
                     try:
-                        hport_start, hport_end = _get_port_range(host_port)
+                        hport_start, hport_end = \
+                            helpers.get_port_range(host_port)
                     except ValueError as exc:
                         # Using __str__() to avoid deprecation warning for
                         # using the message attribute of the ValueError.
@@ -678,7 +524,7 @@ def ports(val, **kwargs):  # pylint: disable=unused-argument
     '''
     if not isinstance(val, list):
         try:
-            val = _split(val)
+            val = helpers.split(val)
         except AttributeError:
             if isinstance(val, six.integer_types):
                 val = [val]
@@ -698,12 +544,13 @@ def ports(val, **kwargs):  # pylint: disable=unused-argument
                 '\'{0}\' is not a valid port definition'.format(item)
             )
         try:
-            range_start, range_end = _get_port_range(item)
+            range_start, range_end = \
+                helpers.get_port_range(item)
         except ValueError as exc:
             # Using __str__() to avoid deprecation warning for using
             # the "message" attribute of the ValueError.
             raise SaltInvocationError(exc.__str__())
-        new_ports.update([_get_port_def(x, proto)
+        new_ports.update([helpers.get_port_def(x, proto)
                           for x in range(range_start, range_end + 1)])
     ordered_new_ports = [
         port if proto == 'tcp' else (port, proto) for (port, proto) in sorted(
@@ -715,15 +562,15 @@ def ports(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def privileged(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def publish_all_ports(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def read_only(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def restart_policy(val, **kwargs):  # pylint: disable=unused-argument
@@ -733,8 +580,12 @@ def restart_policy(val, **kwargs):  # pylint: disable=unused-argument
     to make sure the mapped result uses '0' for the count if this optional
     value was omitted.
     '''
-    val = _map_vals(val, 'Name', 'MaximumRetryCount', fill='0')
-    # _map_vals() converts the input into a list of dicts, but the API
+    val = helpers.map_vals(
+        val,
+        'Name',
+        'MaximumRetryCount',
+        fill='0')
+    # map_vals() converts the input into a list of dicts, but the API
     # wants just a dict, so extract the value from the single-element
     # list. If there was more than one element in the list, then
     # invalid input was passed (i.e. a comma-separated list, when what
@@ -754,48 +605,49 @@ def restart_policy(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def security_opt(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def shm_size(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bytes(val)
+    return helpers.translate_bytes(val)
 
 
 def stdin_open(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def stop_signal(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def stop_timeout(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_int(val)
+    return helpers.translate_int(val)
 
 
 def storage_opt(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter='=')
+    return helpers.translate_key_val(val, delimiter='=')
 
 
 def sysctls(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_key_val(val, delimiter='=')
+    return helpers.translate_key_val(val, delimiter='=')
 
 
 def tmpfs(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_dict(val)
+    return helpers.translate_dict(val)
 
 
 def tty(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_bool(val)
+    return helpers.translate_bool(val)
 
 
 def ulimits(val, **kwargs):  # pylint: disable=unused-argument
-    val = _translate_stringlist(val)
+    val = helpers.translate_stringlist(val)
     for idx in range(len(val)):
         if not isinstance(val[idx], dict):
             try:
-                ulimit_name, limits = _split(val[idx], '=', 1)
-                comps = _split(limits, ':', 1)
+                ulimit_name, limits = \
+                    helpers.split(val[idx], '=', 1)
+                comps = helpers.split(limits, ':', 1)
             except (AttributeError, ValueError):
                 raise SaltInvocationError(
                     'Ulimit definition \'{0}\' is not in the format '
@@ -839,18 +691,18 @@ def user(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def userns_mode(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def volume_driver(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_str(val)
+    return helpers.translate_str(val)
 
 
 def volumes(val, **kwargs):  # pylint: disable=unused-argument
     '''
     Should be a list of absolute paths
     '''
-    val = _translate_stringlist(val)
+    val = helpers.translate_stringlist(val)
     for item in val:
         if not os.path.isabs(item):
             raise SaltInvocationError(
@@ -860,7 +712,7 @@ def volumes(val, **kwargs):  # pylint: disable=unused-argument
 
 
 def volumes_from(val, **kwargs):  # pylint: disable=unused-argument
-    return _translate_stringlist(val)
+    return helpers.translate_stringlist(val)
 
 
 def working_dir(val, **kwargs):  # pylint: disable=unused-argument

salt/utils/docker/translate/helpers.py

@@ -0,0 +1,308 @@
+# -*- coding: utf-8 -*-
+'''
+Functions to translate input in the docker CLI format to the format desired by
+by the API.
+'''
+# Import Python libs
+from __future__ import absolute_import
+import os
+
+# Import Salt libs
+import salt.utils.data
+import salt.utils.network
+from salt.exceptions import SaltInvocationError
+
+# Import 3rd-party libs
+from salt.ext import six
+from salt.ext.six.moves import range, zip  # pylint: disable=import-error,redefined-builtin
+
+NOTSET = object()
+
+
+def split(item, sep=',', maxsplit=-1):
+    return [x.strip() for x in item.split(sep, maxsplit)]
+
+
+def get_port_def(port_num, proto='tcp'):
+    '''
+    Given a port number and protocol, returns the port definition expected by
+    docker-py. For TCP ports this is simply an integer, for UDP ports this is
+    (port_num, 'udp').
+
+    port_num can also be a string in the format 'port_num/udp'. If so, the
+    "proto" argument will be ignored. The reason we need to be able to pass in
+    the protocol separately is because this function is sometimes invoked on
+    data derived from a port range (e.g. '2222-2223/udp'). In these cases the
+    protocol has already been stripped off and the port range resolved into the
+    start and end of the range, and get_port_def() is invoked once for each
+    port number in that range. So, rather than munge udp ports back into
+    strings before passing them to this function, the function will see if it
+    has a string and use the protocol from it if present.
+
+    This function does not catch the TypeError or ValueError which would be
+    raised if the port number is non-numeric. This function either needs to be
+    run on known good input, or should be run within a try/except that catches
+    these two exceptions.
+    '''
+    try:
+        port_num, _, port_num_proto = port_num.partition('/')
+    except AttributeError:
+        pass
+    else:
+        if port_num_proto:
+            proto = port_num_proto
+    try:
+        if proto.lower() == 'udp':
+            return int(port_num), 'udp'
+    except AttributeError:
+        pass
+    return int(port_num)
+
+
+def get_port_range(port_def):
+    '''
+    Given a port number or range, return a start and end to that range. Port
+    ranges are defined as a string containing two numbers separated by a dash
+    (e.g. '4505-4506').
+
+    A ValueError will be raised if bad input is provided.
+    '''
+    if isinstance(port_def, six.integer_types):
+        # Single integer, start/end of range is the same
+        return port_def, port_def
+    try:
+        comps = [int(x) for x in split(port_def, '-')]
+        if len(comps) == 1:
+            range_start = range_end = comps[0]
+        else:
+            range_start, range_end = comps
+        if range_start > range_end:
+            raise ValueError('start > end')
+    except (TypeError, ValueError) as exc:
+        if exc.__str__() == 'start > end':
+            msg = (
+                'Start of port range ({0}) cannot be greater than end of '
+                'port range ({1})'.format(range_start, range_end)
+            )
+        else:
+            msg = '\'{0}\' is non-numeric or an invalid port range'.format(
+                port_def
+            )
+        raise ValueError(msg)
+    else:
+        return range_start, range_end
+
+
+def map_vals(val, *names, **extra_opts):
+    '''
+    Many arguments come in as a list of VAL1:VAL2 pairs, but map to a list
+    of dicts in the format {NAME1: VAL1, NAME2: VAL2}. This function
+    provides common code to handle these instances.
+    '''
+    fill = extra_opts.pop('fill', NOTSET)
+    expected_num_elements = len(names)
+    val = translate_stringlist(val)
+    for idx, item in enumerate(val):
+        if not isinstance(item, dict):
+            elements = [x.strip() for x in item.split(':')]
+            num_elements = len(elements)
+            if num_elements < expected_num_elements:
+                if fill is NOTSET:
+                    raise SaltInvocationError(
+                        '\'{0}\' contains {1} value(s) (expected {2})'.format(
+                            item, num_elements, expected_num_elements
+                        )
+                    )
+                elements.extend([fill] * (expected_num_elements - num_elements))
+            elif num_elements > expected_num_elements:
+                raise SaltInvocationError(
+                    '\'{0}\' contains {1} value(s) (expected {2})'.format(
+                        item,
+                        num_elements,
+                        expected_num_elements if fill is NOTSET
+                            else 'up to {0}'.format(expected_num_elements)
+                    )
+                )
+            val[idx] = dict(zip(names, elements))
+    return val
+
+
+def validate_ip(val):
+    try:
+        if not salt.utils.network.is_ip(val):
+            raise SaltInvocationError(
+                '\'{0}\' is not a valid IP address'.format(val)
+            )
+    except RuntimeError:
+        pass
+
+
+def validate_subnet(val):
+    try:
+        if not salt.utils.network.is_subnet(val):
+            raise SaltInvocationError(
+                '\'{0}\' is not a valid subnet'.format(val)
+            )
+    except RuntimeError:
+        pass
+
+
+def translate_str(val):
+    return str(val) if not isinstance(val, six.string_types) else val
+
+
+def translate_int(val):
+    if not isinstance(val, six.integer_types):
+        try:
+            val = int(val)
+        except (TypeError, ValueError):
+            raise SaltInvocationError('\'{0}\' is not an integer'.format(val))
+    return val
+
+
+def translate_bool(val):
+    return bool(val) if not isinstance(val, bool) else val
+
+
+def translate_dict(val):
+    '''
+    Not really translating, just raising an exception if it's not a dict
+    '''
+    if not isinstance(val, dict):
+        raise SaltInvocationError('\'{0}\' is not a dictionary'.format(val))
+    return val
+
+
+def translate_command(val):
+    '''
+    Input should either be a single string, or a list of strings. This is used
+    for the two args that deal with commands ("command" and "entrypoint").
+    '''
+    if isinstance(val, six.string_types):
+        return val
+    elif isinstance(val, list):
+        for idx in range(len(val)):
+            if not isinstance(val[idx], six.string_types):
+                val[idx] = str(val[idx])
+    else:
+        # Make sure we have a string
+        val = str(val)
+    return val
+
+
+def translate_bytes(val):
+    '''
+    These values can be expressed as an integer number of bytes, or a string
+    expression (i.e. 100mb, 1gb, etc.).
+    '''
+    try:
+        val = int(val)
+    except (TypeError, ValueError):
+        if not isinstance(val, six.string_types):
+            val = str(val)
+    return val
+
+
+def translate_stringlist(val):
+    '''
+    On the CLI, these are passed as multiple instances of a given CLI option.
+    In Salt, we accept these as a comma-delimited list but the API expects a
+    Python list. This function accepts input and returns it back as a Python
+    list of strings. If the input is a string which is a comma-separated list
+    of items, split that string and return it.
+    '''
+    if not isinstance(val, list):
+        try:
+            val = split(val)
+        except AttributeError:
+            val = split(str(val))
+    for idx in range(len(val)):
+        if not isinstance(val[idx], six.string_types):
+            val[idx] = str(val[idx])
+    return val
+
+
+def translate_device_rates(val, numeric_rate=True):
+    '''
+    CLI input is a list of PATH:RATE pairs, but the API expects a list of
+    dictionaries in the format [{'Path': path, 'Rate': rate}]
+    '''
+    val = map_vals(val, 'Path', 'Rate')
+    for idx in range(len(val)):
+        try:
+            is_abs = os.path.isabs(val[idx]['Path'])
+        except AttributeError:
+            is_abs = False
+        if not is_abs:
+            raise SaltInvocationError(
+                'Path \'{Path}\' is not absolute'.format(**val[idx])
+            )
+
+        # Attempt to convert to an integer. Will fail if rate was specified as
+        # a shorthand (e.g. 1mb), this is OK as we will check to make sure the
+        # value is an integer below if that is what is required.
+        try:
+            val[idx]['Rate'] = int(val[idx]['Rate'])
+        except (TypeError, ValueError):
+            pass
+
+        if numeric_rate:
+            try:
+                val[idx]['Rate'] = int(val[idx]['Rate'])
+            except ValueError:
+                raise SaltInvocationError(
+                    'Rate \'{Rate}\' for path \'{Path}\' is '
+                    'non-numeric'.format(**val[idx])
+                )
+    return val
+
+
+def translate_key_val(val, delimiter='='):
+    '''
+    CLI input is a list of key/val pairs, but the API expects a dictionary in
+    the format {key: val}
+    '''
+    if isinstance(val, dict):
+        return val
+    val = translate_stringlist(val)
+    new_val = {}
+    for item in val:
+        try:
+            lvalue, rvalue = split(item, delimiter, 1)
+        except (AttributeError, TypeError, ValueError):
+            raise SaltInvocationError(
+                '\'{0}\' is not a key{1}value pair'.format(item, delimiter)
+            )
+        new_val[lvalue] = rvalue
+    return new_val
+
+
+def translate_labels(val):
+    '''
+    Can either be a list of label names, or a list of name=value pairs. The API
+    can accept either a list of label names or a dictionary mapping names to
+    values, so the value we translate will be different depending on the input.
+    '''
+    if not isinstance(val, dict):
+        if not isinstance(val, list):
+            val = split(val)
+        new_val = {}
+        for item in val:
+            if isinstance(item, dict):
+                if len(item) != 1:
+                    raise SaltInvocationError('Invalid label(s)')
+                key = next(iter(item))
+                val = item[key]
+            else:
+                try:
+                    key, val = split(item, '=', 1)
+                except ValueError:
+                    key = item
+                    val = ''
+            if not isinstance(key, six.string_types):
+                key = str(key)
+            if not isinstance(val, six.string_types):
+                val = str(val)
+            new_val[key] = val
+        val = new_val
+    return val

salt/utils/docker/translate/network.py

@@ -0,0 +1,142 @@
+# -*- coding: utf-8 -*-
+'''
+Functions to translate input for network creation
+'''
+# Import Python libs
+from __future__ import absolute_import
+
+# Import Salt libs
+from salt.exceptions import SaltInvocationError
+
+# Import 3rd-party libs
+from salt.ext import six
+
+# Import helpers
+from . import helpers
+
+ALIASES = {
+    'driver_opt': 'options',
+    'driver_opts': 'options',
+    'ipv6': 'enable_ipv6',
+}
+IPAM_ALIASES = {
+    'ip_range': 'iprange',
+    'aux_address': 'aux_addresses',
+}
+# ALIASES is a superset of IPAM_ALIASES
+ALIASES.update(IPAM_ALIASES)
+ALIASES_REVMAP = dict([(y, x) for x, y in six.iteritems(ALIASES)])
+
+DEFAULTS = {'check_duplicate': True}
+
+
+def _post_processing(kwargs, skip_translate, invalid):  # pylint: disable=unused-argument
+    '''
+    Additional network-specific post-translation processing
+    '''
+    # If any defaults were not expicitly passed, add them
+    for item in DEFAULTS:
+        if item not in kwargs:
+            kwargs[item] = DEFAULTS[item]
+
+
+# Functions below must match names of docker-py arguments
+def driver(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_str(val)
+
+
+def options(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_key_val(val, delimiter='=')
+
+
+def ipam(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_dict(val)
+
+
+def check_duplicate(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_bool(val)
+
+
+def internal(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_bool(val)
+
+
+def labels(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_labels(val)
+
+
+def enable_ipv6(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_bool(val)
+
+
+def attachable(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_bool(val)
+
+
+def ingress(val, **kwargs):  # pylint: disable=unused-argument
+    return helpers.translate_bool(val)
+
+
+# IPAM args
+def ipam_driver(val, **kwargs):  # pylint: disable=unused-argument
+    return driver(val, **kwargs)
+
+
+def ipam_opts(val, **kwargs):  # pylint: disable=unused-argument
+    return options(val, **kwargs)
+
+
+def ipam_pools(val, **kwargs):  # pylint: disable=unused-argument
+    if not hasattr(val, '__iter__') \
+            or not all(isinstance(x, dict) for x in val):
+        # Can't do a simple dictlist check because each dict may have more than
+        # one element.
+        raise SaltInvocationError('ipam_pools must be a list of dictionaries')
+    skip_translate = kwargs.get('skip_translate', ())
+    if not (skip_translate is True or 'ipam_pools' in skip_translate):
+        _globals = globals()
+        for ipam_dict in val:
+            for key in list(ipam_dict):
+                if skip_translate is not True and key in skip_translate:
+                    continue
+                if key in IPAM_ALIASES:
+                    # Make sure we resolve aliases, since this wouldn't have
+                    # been done within the individual IPAM dicts
+                    ipam_dict[IPAM_ALIASES[key]] = ipam_dict.pop(key)
+                    key = IPAM_ALIASES[key]
+                if key in _globals:
+                    ipam_dict[key] = _globals[key](ipam_dict[key])
+    return val
+
+
+def subnet(val, **kwargs):  # pylint: disable=unused-argument
+    validate_ip_addrs = kwargs.get('validate_ip_addrs', True)
+    val = helpers.translate_str(val)
+    if validate_ip_addrs:
+        helpers.validate_subnet(val)
+    return val
+
+
+def iprange(val, **kwargs):  # pylint: disable=unused-argument
+    validate_ip_addrs = kwargs.get('validate_ip_addrs', True)
+    val = helpers.translate_str(val)
+    if validate_ip_addrs:
+        helpers.validate_subnet(val)
+    return val
+
+
+def gateway(val, **kwargs):  # pylint: disable=unused-argument
+    validate_ip_addrs = kwargs.get('validate_ip_addrs', True)
+    val = helpers.translate_str(val)
+    if validate_ip_addrs:
+        helpers.validate_ip(val)
+    return val
+
+
+def aux_addresses(val, **kwargs):  # pylint: disable=unused-argument
+    validate_ip_addrs = kwargs.get('validate_ip_addrs', True)
+    val = helpers.translate_key_val(val, delimiter='=')
+    if validate_ip_addrs:
+        for address in six.itervalues(val):
+            helpers.validate_ip(address)
+    return val

salt/utils/network.py

@@ -263,6 +263,33 @@ def is_ipv6(ip):
         return False
 
 
+def is_subnet(cidr):
+    '''
+    Returns a bool telling if the passed string is an IPv4 or IPv6 subnet
+    '''
+    return is_ipv4_subnet(cidr) or is_ipv6_subnet(cidr)
+
+
+def is_ipv4_subnet(cidr):
+    '''
+    Returns a bool telling if the passed string is an IPv4 subnet
+    '''
+    try:
+        return '/' in cidr and bool(ipaddress.IPv4Network(cidr))
+    except Exception:
+        return False
+
+
+def is_ipv6_subnet(cidr):
+    '''
+    Returns a bool telling if the passed string is an IPv6 subnet
+    '''
+    try:
+        return '/' in cidr and bool(ipaddress.IPv6Network(cidr))
+    except Exception:
+        return False
+
+
 @jinja_filter('is_ip')
 def is_ip_filter(ip, options=None):
     '''

tests/integration/states/test_docker_network.py

@@ -0,0 +1,500 @@
+# -*- coding: utf-8 -*-
+'''
+Integration tests for the docker_network states
+'''
+# Import Python Libs
+from __future__ import absolute_import
+import errno
+import functools
+import logging
+import os
+import subprocess
+import tempfile
+
+# Import Salt Testing Libs
+from tests.support.unit import skipIf
+from tests.support.case import ModuleCase
+from tests.support.docker import with_network, random_name
+from tests.support.paths import FILES, TMP
+from tests.support.helpers import destructiveTest
+from tests.support.mixins import SaltReturnAssertsMixin
+
+# Import Salt Libs
+import salt.utils.files
+import salt.utils.network
+import salt.utils.path
+from salt.exceptions import CommandExecutionError
+
+log = logging.getLogger(__name__)
+
+
+IMAGE_NAME = random_name(prefix='salt_busybox_')
+IPV6_ENABLED = bool(salt.utils.network.ip_addrs6(include_loopback=True))
+
+
+def network_name(func):
+    '''
+    Generate a randomized name for a network and clean it up afterward
+    '''
+    @functools.wraps(func)
+    def wrapper(self, *args, **kwargs):
+        name = random_name(prefix='salt_net_')
+        try:
+            return func(self, name, *args, **kwargs)
+        finally:
+            self.run_function(
+                'docker.disconnect_all_containers_from_network', [name])
+            try:
+                self.run_function('docker.remove_network', [name])
+            except CommandExecutionError as exc:
+                if 'No such network' not in exc.__str__():
+                    raise
+    return wrapper
+
+
+def container_name(func):
+    '''
+    Generate a randomized name for a container and clean it up afterward
+    '''
+    def build_image():
+        # Create temp dir
+        image_build_rootdir = tempfile.mkdtemp(dir=TMP)
+        script_path = \
+            os.path.join(FILES, 'file/base/mkimage-busybox-static')
+        cmd = [script_path, image_build_rootdir, IMAGE_NAME]
+        log.debug('Running \'%s\' to build busybox image', ' '.join(cmd))
+        process = subprocess.Popen(
+            cmd,
+            close_fds=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT)
+        output = process.communicate()[0]
+        log.debug('Output from mkimge-busybox-static:\n%s', output)
+
+        if process.returncode != 0:
+            raise Exception('Failed to build image')
+
+        try:
+            salt.utils.files.rm_rf(image_build_rootdir)
+        except OSError as exc:
+            if exc.errno != errno.ENOENT:
+                raise
+
+    @functools.wraps(func)
+    def wrapper(self, *args, **kwargs):
+        try:
+            self.run_function('docker.inspect_image', [IMAGE_NAME])
+        except CommandExecutionError:
+            pass
+        else:
+            build_image()
+
+        name = random_name(prefix='salt_test_')
+        self.run_function(
+            'docker.create',
+            name=name,
+            image=IMAGE_NAME,
+            command='sleep 600',
+            start=True)
+        try:
+            return func(self, name, *args, **kwargs)
+        finally:
+            try:
+                self.run_function('docker.rm', [name], force=True)
+            except CommandExecutionError as exc:
+                if 'No such container' not in exc.__str__():
+                    raise
+    return wrapper
+
+
+@destructiveTest
+@skipIf(not salt.utils.path.which('dockerd'), 'Docker not installed')
+class DockerNetworkTestCase(ModuleCase, SaltReturnAssertsMixin):
+    '''
+    Test docker_network states
+    '''
+    @classmethod
+    def tearDownClass(cls):
+        '''
+        Remove test image if present. Note that this will run a docker rmi even
+        if no test which required the image was run.
+        '''
+        cmd = ['docker', 'rmi', '--force', IMAGE_NAME]
+        log.debug('Running \'%s\' to destroy busybox image', ' '.join(cmd))
+        process = subprocess.Popen(
+            cmd,
+            close_fds=True,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT)
+        output = process.communicate()[0]
+        log.debug('Output from %s:\n%s', ' '.join(cmd), output)
+
+        if process.returncode != 0 and 'No such image' not in output:
+            raise Exception('Failed to destroy image')
+
+    def run_state(self, function, **kwargs):
+        ret = super(DockerNetworkTestCase, self).run_state(function, **kwargs)
+        log.debug('ret = %s', ret)
+        return ret
+
+    @with_network(create=False)
+    def test_absent(self, net):
+        self.assertSaltTrueReturn(
+            self.run_state('docker_network.present', name=net.name))
+        ret = self.run_state('docker_network.absent', name=net.name)
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        self.assertEqual(ret['changes'], {'removed': True})
+        self.assertEqual(
+            ret['comment'],
+            'Removed network \'{0}\''.format(net.name)
+        )
+
+    @container_name
+    @with_network(create=False)
+    def test_absent_with_disconnected_container(self, net, container_name):
+        self.assertSaltTrueReturn(
+            self.run_state('docker_network.present',
+                name=net.name,
+                containers=[container_name])
+        )
+        ret = self.run_state('docker_network.absent', name=net.name)
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        self.assertEqual(
+            ret['changes'],
+            {
+                'removed': True,
+                'disconnected': [container_name],
+            }
+        )
+        self.assertEqual(
+            ret['comment'],
+            'Removed network \'{0}\''.format(net.name)
+        )
+
+    @with_network(create=False)
+    def test_absent_when_not_present(self, net):
+        ret = self.run_state('docker_network.absent', name=net.name)
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+        self.assertEqual(ret['changes'], {})
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' already absent'.format(net.name)
+        )
+
+    @with_network(create=False)
+    def test_present(self, net):
+        ret = self.run_state('docker_network.present', name=net.name)
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        # Make sure the state return is what we expect
+        self.assertEqual(ret['changes'], {'created': True})
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' created'.format(net.name)
+        )
+
+        # Now check to see that the network actually exists. If it doesn't,
+        # this next function call will raise an exception.
+        self.run_function('docker.inspect_network', [net.name])
+
+    @container_name
+    @with_network(create=False)
+    def test_present_with_containers(self, net, container_name):
+        ret = self.run_state(
+            'docker_network.present',
+            name=net.name,
+            containers=[container_name])
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        self.assertEqual(
+            ret['changes'],
+            {
+                'created': True,
+                'connected': [container_name],
+            }
+        )
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' created'.format(net.name)
+        )
+
+        # Now check to see that the network actually exists. If it doesn't,
+        # this next function call will raise an exception.
+        self.run_function('docker.inspect_network', [net.name])
+
+    def _test_present_reconnect(self, net, container_name, reconnect=True):
+        ret = self.run_state(
+            'docker_network.present',
+            name=net.name,
+            driver='bridge')
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        self.assertEqual(ret['changes'], {'created': True})
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' created'.format(net.name)
+        )
+
+        # Connect the container
+        self.run_function(
+            'docker.connect_container_to_network',
+            [container_name, net.name]
+        )
+
+        # Change the driver to force the network to be replaced
+        ret = self.run_state(
+            'docker_network.present',
+            name=net.name,
+            driver='macvlan',
+            reconnect=reconnect)
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        self.assertEqual(
+            ret['changes'],
+            {
+                'recreated': True,
+                'reconnected' if reconnect else 'disconnected': [container_name],
+                net.name: {
+                    'Driver': {
+                        'old': 'bridge',
+                        'new': 'macvlan',
+                    }
+                }
+            }
+        )
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' was replaced with updated config'.format(net.name)
+        )
+
+    @container_name
+    @with_network(create=False)
+    def test_present_with_reconnect(self, net, container_name):
+        '''
+        Test reconnecting with containers not passed to state
+        '''
+        self._test_present_reconnect(net, container_name, reconnect=True)
+
+    @container_name
+    @with_network(create=False)
+    def test_present_with_no_reconnect(self, net, container_name):
+        '''
+        Test reconnecting with containers not passed to state
+        '''
+        self._test_present_reconnect(net, container_name, reconnect=False)
+
+    @with_network()
+    def test_present_internal(self, net):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net.name,
+                internal=True,
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net.name])
+        self.assertIs(net_info['Internal'], True)
+
+    @with_network()
+    def test_present_labels(self, net):
+        # Test a mix of different ways of specifying labels
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net.name,
+                labels=[
+                    'foo',
+                    'bar=baz',
+                    {'hello': 'world'},
+                ],
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net.name])
+        self.assertEqual(
+            net_info['Labels'],
+            {'foo': '',
+             'bar': 'baz',
+             'hello': 'world'},
+        )
+
+    @with_network(subnet='fe3f:2180:26:1::/123')
+    @with_network(subnet='10.247.197.96/27')
+    @skipIf(not IPV6_ENABLED, 'IPv6 not enabled')
+    def test_present_enable_ipv6(self, net1, net2):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net1.name,
+                enable_ipv6=True,
+                ipam_pools=[
+                    {'subnet': net1.subnet},
+                    {'subnet': net2.subnet},
+                ],
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net1.name])
+        self.assertIs(net_info['EnableIPv6'], True)
+
+    @with_network()
+    def test_present_attachable(self, net):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net.name,
+                attachable=True,
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net.name])
+        self.assertIs(net_info['Attachable'], True)
+
+    @skipIf(True, 'Skip until we can set up docker swarm testing')
+    @with_network()
+    def test_present_scope(self, net):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net.name,
+                scope='global',
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net.name])
+        self.assertIs(net_info['Scope'], 'global')
+
+    @skipIf(True, 'Skip until we can set up docker swarm testing')
+    @with_network()
+    def test_present_ingress(self, net):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net.name,
+                ingress=True,
+            )
+        )
+        net_info = self.run_function('docker.inspect_network', [net.name])
+        self.assertIs(net_info['Ingress'], True)
+
+    @with_network(subnet='10.247.197.128/27')
+    @with_network(subnet='10.247.197.96/27')
+    def test_present_with_custom_ipv4(self, net1, net2):
+        # First run will test passing the IPAM arguments individually
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=net1.name,
+                subnet=net1.subnet,
+                gateway=net1.gateway,
+            )
+        )
+        # Second run will pass them in the ipam_pools argument
+        ret = self.run_state(
+            'docker_network.present',
+            name=net1.name,  # We want to keep the same network name
+            ipam_pools=[
+                {'subnet': net2.subnet,
+                 'gateway': net2.gateway},
+            ],
+        )
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        # Docker requires there to be IPv4, even when only an IPv6 subnet was
+        # provided. So, there will be both an IPv4 and IPv6 pool in the
+        # configuration.
+        expected = {
+            'recreated': True,
+            net1.name: {
+                'IPAM': {
+                    'Config': {
+                        'old': [
+                            {'Subnet': net1.subnet,
+                             'Gateway': net1.gateway},
+                        ],
+                        'new': [
+                            {'Subnet': net2.subnet,
+                             'Gateway': net2.gateway},
+                        ],
+                    }
+                }
+            }
+        }
+        self.assertEqual(ret['changes'], expected)
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' was replaced with updated config'.format(
+                net1.name
+            )
+        )
+
+    @with_network(subnet='fe3f:2180:26:1::20/123')
+    @with_network(subnet='fe3f:2180:26:1::/123')
+    @with_network(subnet='10.247.197.96/27')
+    @skipIf(not IPV6_ENABLED, 'IPv6 not enabled')
+    def test_present_with_custom_ipv6(self, ipv4_net, ipv6_net1, ipv6_net2):
+        self.assertSaltTrueReturn(
+            self.run_state(
+                'docker_network.present',
+                name=ipv4_net.name,
+                enable_ipv6=True,
+                ipam_pools=[
+                    {'subnet': ipv4_net.subnet,
+                     'gateway': ipv4_net.gateway},
+                    {'subnet': ipv6_net1.subnet,
+                     'gateway': ipv6_net1.gateway}
+                ],
+            )
+        )
+
+        ret = self.run_state(
+            'docker_network.present',
+            name=ipv4_net.name,  # We want to keep the same network name
+            enable_ipv6=True,
+            ipam_pools=[
+                {'subnet': ipv4_net.subnet,
+                 'gateway': ipv4_net.gateway},
+                {'subnet': ipv6_net2.subnet,
+                 'gateway': ipv6_net2.gateway}
+            ],
+        )
+        self.assertSaltTrueReturn(ret)
+        ret = ret[next(iter(ret))]
+
+        # Docker requires there to be IPv4, even when only an IPv6 subnet was
+        # provided. So, there will be both an IPv4 and IPv6 pool in the
+        # configuration.
+        expected = {
+            'recreated': True,
+            ipv4_net.name: {
+                'IPAM': {
+                    'Config': {
+                        'old': [
+                            {'Subnet': ipv4_net.subnet,
+                             'Gateway': ipv4_net.gateway},
+                            {'Subnet': ipv6_net1.subnet,
+                             'Gateway': ipv6_net1.gateway}
+                        ],
+                        'new': [
+                            {'Subnet': ipv4_net.subnet,
+                             'Gateway': ipv4_net.gateway},
+                            {'Subnet': ipv6_net2.subnet,
+                             'Gateway': ipv6_net2.gateway}
+                        ],
+                    }
+                }
+            }
+        }
+        self.assertEqual(ret['changes'], expected)
+        self.assertEqual(
+            ret['comment'],
+            'Network \'{0}\' was replaced with updated config'.format(
+                ipv4_net.name
+            )
+        )

tests/support/docker.py

@@ -0,0 +1,109 @@
+# -*- coding: utf-8 -*-
+'''
+Common code used in Docker integration tests
+'''
+# Import Python libs
+from __future__ import absolute_import
+import functools
+import random
+import string
+
+# Import Salt libs
+from salt.exceptions import CommandExecutionError
+
+# Import 3rd-party libs
+from salt._compat import ipaddress
+from salt.ext.six.moves import range  # pylint: disable=import-error,redefined-builtin
+
+
+def random_name(prefix=''):
+    ret = prefix
+    for _ in range(8):
+        ret += random.choice(string.ascii_lowercase)
+    return ret
+
+
+class Network(object):
+    def __init__(self, name, **kwargs):
+        self.kwargs = kwargs
+        self.name = name
+        try:
+            self.net = ipaddress.ip_network(self.kwargs['subnet'])
+            self._rand_indexes = random.sample(
+                range(2, self.net.num_addresses - 1),
+                self.net.num_addresses - 3)
+            self.ip_arg = 'ipv{0}_address'.format(self.net.version)
+        except KeyError:
+            # No explicit subnet passed
+            self.net = self.ip_arg = None
+
+    def __getitem__(self, index):
+        try:
+            return self.net[self._rand_indexes[index]].compressed
+        except (TypeError, AttributeError):
+            raise ValueError(
+                'Indexing not supported for networks without a custom subnet')
+
+    def arg_map(self, arg_name):
+        return {'ipv4_address': 'IPv4Address',
+                'ipv6_address': 'IPv6Address',
+                'links': 'Links',
+                'aliases': 'Aliases'}[arg_name]
+
+    @property
+    def subnet(self):
+        try:
+            return self.net.compressed
+        except AttributeError:
+            return None
+
+    @property
+    def gateway(self):
+        try:
+            return self.kwargs['gateway']
+        except KeyError:
+            try:
+                return self.net[1].compressed
+            except (AttributeError, IndexError):
+                return None
+
+
+class with_network(object):
+    '''
+    Generate a network for the test. Information about the network will be
+    passed to the wrapped function.
+    '''
+    def __init__(self, **kwargs):
+        self.create = kwargs.pop('create', False)
+        self.network = Network(random_name(prefix='salt_net_'), **kwargs)
+        if self.network.net is not None:
+            if 'enable_ipv6' not in kwargs:
+                kwargs['enable_ipv6'] = self.network.net.version == 6
+        self.kwargs = kwargs
+
+    def __call__(self, func):
+        self.func = func
+        return functools.wraps(func)(
+            lambda testcase, *args, **kwargs: self.wrap(testcase, *args, **kwargs)  # pylint: disable=W0108
+        )
+
+    def wrap(self, testcase, *args, **kwargs):
+        if self.create:
+            testcase.run_function(
+                'docker.create_network',
+                [self.network.name],
+                **self.kwargs)
+        try:
+            return self.func(testcase, self.network, *args, **kwargs)
+        finally:
+            try:
+                testcase.run_function(
+                    'docker.disconnect_all_containers_from_network',
+                    [self.network.name])
+            except CommandExecutionError as exc:
+                if '404' not in exc.__str__():
+                    raise
+            else:
+                testcase.run_function(
+                    'docker.remove_network',
+                    [self.network.name])

tests/unit/modules/test_dockermod.py

@@ -130,7 +130,7 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
                                        ('kill', ()),
                                        ('pause', ()),
                                        ('signal_', ('KILL',)),
-                                       ('start', ()),
+                                       ('start_', ()),
                                        ('stop', ()),
                                        ('unpause', ()),
                                        ('_run', ('command',)),
@@ -164,8 +164,12 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
         host_config = {}
         client = Mock()
         client.api_version = '1.21'
+        client.networks = Mock(return_value=[
+            {'Name': 'foo',
+             'Id': '01234',
+             'Containers': {}}
+        ])
         get_client_mock = MagicMock(return_value=client)
-
         with patch.dict(docker_mod.__dict__,
                         {'__salt__': __salt__}):
             with patch.object(docker_mod, '_get_client', get_client_mock):
@@ -207,7 +211,6 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
                                                               'Subnet': '192.168.0.0/24'
                                                           }],
                                                           'Driver': 'default',
-                                                          'Options': {}
                                                       },
                                                       check_duplicate=True)
 
@@ -276,7 +279,7 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
                 with patch.object(docker_mod, '_get_client', get_client_mock):
                     docker_mod.connect_container_to_network('container', 'foo')
         client.connect_container_to_network.assert_called_once_with(
-            'container', 'foo', None)
+            'container', 'foo')
 
     @skipIf(docker_version < (1, 5, 0),
             'docker module must be installed to run this test or is too old. >=1.5.0')
@@ -546,16 +549,13 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
             })
 
         ret = None
-        with patch.object(docker_mod, 'start', docker_start_mock):
-            with patch.object(docker_mod, 'create', docker_create_mock):
-                with patch.object(docker_mod, 'stop', docker_stop_mock):
-                    with patch.object(docker_mod, 'commit', docker_commit_mock):
-                        with patch.object(docker_mod, 'sls', docker_sls_mock):
-                            with patch.object(docker_mod, 'rm_', docker_rm_mock):
-                                ret = docker_mod.sls_build(
-                                    'foo',
-                                    mods='foo',
-                                )
+        with patch.object(docker_mod, 'start_', docker_start_mock), \
+                patch.object(docker_mod, 'create', docker_create_mock), \
+                patch.object(docker_mod, 'stop', docker_stop_mock), \
+                patch.object(docker_mod, 'commit', docker_commit_mock), \
+                patch.object(docker_mod, 'sls', docker_sls_mock), \
+                patch.object(docker_mod, 'rm_', docker_rm_mock):
+            ret = docker_mod.sls_build('foo', mods='foo')
         docker_create_mock.assert_called_once_with(
             cmd='sleep infinity',
             image='opensuse/python', interactive=True, tty=True)
@@ -563,7 +563,7 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
         docker_sls_mock.assert_called_once_with('ID', 'foo', 'base')
         docker_stop_mock.assert_called_once_with('ID')
         docker_rm_mock.assert_called_once_with('ID')
-        docker_commit_mock.assert_called_once_with('ID', 'foo')
+        docker_commit_mock.assert_called_once_with('ID', 'foo', tag='latest')
         self.assertEqual(
             {'Id': 'ID2', 'Image': 'foo', 'Time_Elapsed': 42}, ret)
 
@@ -604,16 +604,12 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
             })
 
         ret = None
-        with patch.object(docker_mod, 'start', docker_start_mock):
-            with patch.object(docker_mod, 'create', docker_create_mock):
-                with patch.object(docker_mod, 'stop', docker_stop_mock):
-                    with patch.object(docker_mod, 'rm_', docker_rm_mock):
-                        with patch.object(docker_mod, 'sls', docker_sls_mock):
-                            ret = docker_mod.sls_build(
-                                'foo',
-                                mods='foo',
-                                dryrun=True
-                            )
+        with patch.object(docker_mod, 'start_', docker_start_mock), \
+                patch.object(docker_mod, 'create', docker_create_mock), \
+                patch.object(docker_mod, 'stop', docker_stop_mock), \
+                patch.object(docker_mod, 'rm_', docker_rm_mock), \
+                patch.object(docker_mod, 'sls', docker_sls_mock):
+            ret = docker_mod.sls_build('foo', mods='foo', dryrun=True)
         docker_create_mock.assert_called_once_with(
             cmd='sleep infinity',
             image='opensuse/python', interactive=True, tty=True)
@@ -669,19 +665,15 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
         context = {'docker.exec_driver': 'docker-exec'}
         salt_dunder = {'config.option': docker_config_mock}
 
-        with patch.object(docker_mod, 'run_all', docker_run_all_mock):
-            with patch.object(docker_mod, 'copy_to', docker_copy_to_mock):
-                with patch.object(docker_mod, '_get_client', get_client_mock):
-                    with patch.dict(docker_mod.__opts__, {'cachedir': '/tmp'}):
-                        with patch.dict(docker_mod.__salt__, salt_dunder):
-                            with patch.dict(docker_mod.__context__, context):
-                                # call twice to verify tmp path later
-                                for i in range(2):
-                                    ret = docker_mod.call(
-                                        'ID',
-                                        'test.arg',
-                                        1, 2,
-                                        arg1='val1')
+        with patch.object(docker_mod, 'run_all', docker_run_all_mock), \
+                patch.object(docker_mod, 'copy_to', docker_copy_to_mock), \
+                patch.object(docker_mod, '_get_client', get_client_mock), \
+                patch.dict(docker_mod.__opts__, {'cachedir': '/tmp'}), \
+                patch.dict(docker_mod.__salt__, salt_dunder), \
+                patch.dict(docker_mod.__context__, context):
+            # call twice to verify tmp path later
+            for i in range(2):
+                ret = docker_mod.call('ID', 'test.arg', 1, 2, arg1='val1')
 
         # Check that the directory is different each time
         # [ call(name, [args]), ...
@@ -759,21 +751,28 @@ class DockerTestCase(TestCase, LoaderModuleMockMixin):
 
     def test_resolve_tag(self):
         '''
-        Test the resolve_tag function
+        Test the resolve_tag function. It runs docker.insect_image on the image
+        name passed and then looks for the RepoTags key in the result
         '''
-        with_prefix = 'docker.io/foo:latest'
-        no_prefix = 'bar:latest'
-        with patch.object(docker_mod,
-                          'list_tags',
-                          MagicMock(return_value=[with_prefix])):
-            self.assertEqual(docker_mod.resolve_tag('foo'), with_prefix)
-            self.assertEqual(docker_mod.resolve_tag('foo:latest'), with_prefix)
-            self.assertEqual(docker_mod.resolve_tag(with_prefix), with_prefix)
-            self.assertEqual(docker_mod.resolve_tag('foo:bar'), False)
+        id_ = 'sha256:6ad733544a6317992a6fac4eb19fe1df577d4dec7529efec28a5bd0edad0fd30'
+        tags = ['foo:latest', 'foo:bar']
+        mock_tagged = MagicMock(return_value={'Id': id_, 'RepoTags': tags})
+        mock_untagged = MagicMock(return_value={'Id': id_, 'RepoTags': []})
+        mock_unexpected = MagicMock(return_value={'Id': id_})
+        mock_not_found = MagicMock(side_effect=CommandExecutionError())
 
-        with patch.object(docker_mod,
-                          'list_tags',
-                          MagicMock(return_value=[no_prefix])):
-            self.assertEqual(docker_mod.resolve_tag('bar'), no_prefix)
-            self.assertEqual(docker_mod.resolve_tag(no_prefix), no_prefix)
-            self.assertEqual(docker_mod.resolve_tag('bar:baz'), False)
+        with patch.object(docker_mod, 'inspect_image', mock_tagged):
+            self.assertEqual(docker_mod.resolve_tag('foo'), tags[0])
+            self.assertEqual(docker_mod.resolve_tag('foo', all=True), tags)
+
+        with patch.object(docker_mod, 'inspect_image', mock_untagged):
+            self.assertEqual(docker_mod.resolve_tag('foo'), id_)
+            self.assertEqual(docker_mod.resolve_tag('foo', all=True), [id_])
+
+        with patch.object(docker_mod, 'inspect_image', mock_unexpected):
+            self.assertEqual(docker_mod.resolve_tag('foo'), id_)
+            self.assertEqual(docker_mod.resolve_tag('foo', all=True), [id_])
+
+        with patch.object(docker_mod, 'inspect_image', mock_not_found):
+            self.assertIs(docker_mod.resolve_tag('foo'), False)
+            self.assertIs(docker_mod.resolve_tag('foo', all=True), False)

tests/unit/states/test_docker_network.py

@@ -1,216 +0,0 @@
-# -*- coding: utf-8 -*-
-'''
-Unit tests for the docker state
-'''
-
-# Import Python Libs
-from __future__ import absolute_import
-
-# Import Salt Testing Libs
-from tests.support.mixins import LoaderModuleMockMixin
-from tests.support.unit import skipIf, TestCase
-from tests.support.mock import (
-    Mock,
-    NO_MOCK,
-    NO_MOCK_REASON,
-    patch
-)
-
-# Import Salt Libs
-import salt.modules.dockermod as docker_mod
-import salt.states.docker_network as docker_state
-
-
-@skipIf(NO_MOCK, NO_MOCK_REASON)
-class DockerNetworkTestCase(TestCase, LoaderModuleMockMixin):
-    '''
-    Test docker_network states
-    '''
-    def setup_loader_modules(self):
-        return {
-            docker_mod: {
-                '__context__': {'docker.docker_version': ''}
-            },
-            docker_state: {
-                '__opts__': {'test': False}
-            }
-        }
-
-    def test_present(self):
-        '''
-        Test docker_network.present
-        '''
-        docker_create_network = Mock(return_value='created')
-        docker_connect_container_to_network = Mock(return_value='connected')
-        docker_inspect_container = Mock(return_value={'Id': 'abcd', 'Name': 'container_bar'})
-        # Get docker.networks to return a network with a name which is a superset of the name of
-        # the network which is to be created, despite this network existing we should still expect
-        # that the new network will be created.
-        # Regression test for #41982.
-        docker_networks = Mock(return_value=[{
-            'Name': 'network_foobar',
-            'Containers': {'container': {}}
-        }])
-        __salt__ = {'docker.create_network': docker_create_network,
-                    'docker.inspect_container': docker_inspect_container,
-                    'docker.connect_container_to_network': docker_connect_container_to_network,
-                    'docker.networks': docker_networks,
-                    }
-        with patch.dict(docker_state.__dict__,
-                        {'__salt__': __salt__}):
-            ret = docker_state.present(
-                'network_foo',
-                containers=['container'],
-                gateway='192.168.0.1',
-                ip_range='192.168.0.128/25',
-                subnet='192.168.0.0/24'
-                )
-        docker_create_network.assert_called_with('network_foo',
-                                                 driver=None,
-                                                 driver_opts=None,
-                                                 gateway='192.168.0.1',
-                                                 ip_range='192.168.0.128/25',
-                                                 subnet='192.168.0.0/24')
-        docker_connect_container_to_network.assert_called_with('abcd',
-                                                                 'network_foo')
-        self.assertEqual(ret, {'name': 'network_foo',
-                               'comment': '',
-                               'changes': {'connected': ['container_bar'],
-                                           'created': 'created'},
-                               'result': True})
-
-    def test_present_with_change(self):
-        '''
-        Test docker_network.present when the specified network has properties differing from the already present network
-        '''
-        network_details = {
-            'Id': 'abcd',
-            'Name': 'network_foo',
-            'Driver': 'macvlan',
-            'Containers': {
-                'abcd': {}
-            },
-            'Options': {
-                'parent': 'eth0'
-            },
-            'IPAM': {
-                'Config': [
-                    {
-                        'Subnet': '192.168.0.0/24',
-                        'Gateway': '192.168.0.1'
-                    }
-                ]
-            }
-        }
-        docker_networks = Mock(return_value=[network_details])
-        network_details['Containers'] = {'abcd': {'Id': 'abcd', 'Name': 'container_bar'}}
-        docker_inspect_network = Mock(return_value=network_details)
-        docker_inspect_container = Mock(return_value={'Id': 'abcd', 'Name': 'container_bar'})
-        docker_disconnect_container_from_network = Mock(return_value='disconnected')
-        docker_remove_network = Mock(return_value='removed')
-        docker_create_network = Mock(return_value='created')
-        docker_connect_container_to_network = Mock(return_value='connected')
-
-        __salt__ = {'docker.networks': docker_networks,
-                    'docker.inspect_network': docker_inspect_network,
-                    'docker.inspect_container': docker_inspect_container,
-                    'docker.disconnect_container_from_network': docker_disconnect_container_from_network,
-                    'docker.remove_network': docker_remove_network,
-                    'docker.create_network': docker_create_network,
-                    'docker.connect_container_to_network': docker_connect_container_to_network,
-                    }
-        with patch.dict(docker_state.__dict__,
-                        {'__salt__': __salt__}):
-            ret = docker_state.present(
-                'network_foo',
-                driver='macvlan',
-                gateway='192.168.1.1',
-                subnet='192.168.1.0/24',
-                driver_opts={'parent': 'eth1'},
-                containers=['abcd']
-            )
-
-        docker_disconnect_container_from_network.assert_called_with('abcd', 'network_foo')
-        docker_remove_network.assert_called_with('network_foo')
-        docker_create_network.assert_called_with('network_foo',
-                                                 driver='macvlan',
-                                                 driver_opts={'parent': 'eth1'},
-                                                 gateway='192.168.1.1',
-                                                 ip_range=None,
-                                                 subnet='192.168.1.0/24')
-        docker_connect_container_to_network.assert_called_with('abcd', 'network_foo')
-
-        self.assertEqual(ret, {'name': 'network_foo',
-                               'comment': 'Network \'network_foo\' was replaced with updated config',
-                               'changes': {
-                                   'updated': {'network_foo': {
-                                       'old': {
-                                           'driver_opts': {'parent': 'eth0'},
-                                           'gateway': '192.168.0.1',
-                                           'subnet': '192.168.0.0/24'
-                                       },
-                                       'new': {
-                                           'driver_opts': {'parent': 'eth1'},
-                                           'gateway': '192.168.1.1',
-                                           'subnet': '192.168.1.0/24'
-                                       }
-                                   }},
-                                   'reconnected': ['container_bar']
-                               },
-                               'result': True})
-
-    def test_absent(self):
-        '''
-        Test docker_network.absent
-        '''
-        docker_remove_network = Mock(return_value='removed')
-        docker_disconnect_container_from_network = Mock(return_value='disconnected')
-        docker_networks = Mock(return_value=[{
-            'Name': 'network_foo',
-            'Containers': {'container': {}}
-        }])
-        __salt__ = {
-            'docker.remove_network': docker_remove_network,
-            'docker.disconnect_container_from_network': docker_disconnect_container_from_network,
-            'docker.networks': docker_networks,
-        }
-        with patch.dict(docker_state.__dict__,
-                        {'__salt__': __salt__}):
-            ret = docker_state.absent('network_foo')
-        docker_disconnect_container_from_network.assert_called_with('container',
-                                                                      'network_foo')
-        docker_remove_network.assert_called_with('network_foo')
-        self.assertEqual(ret, {'name': 'network_foo',
-                               'comment': '',
-                               'changes': {'disconnected': 'disconnected',
-                                           'removed': 'removed'},
-                               'result': True})
-
-    def test_absent_with_matching_network(self):
-        '''
-        Test docker_network.absent when the specified network does not exist,
-        but another network with a name which is a superset of the specified
-        name does exist.  In this case we expect there to be no attempt to remove
-        any network.
-        Regression test for #41982.
-        '''
-        docker_remove_network = Mock(return_value='removed')
-        docker_disconnect_container_from_network = Mock(return_value='disconnected')
-        docker_networks = Mock(return_value=[{
-            'Name': 'network_foobar',
-            'Containers': {'container': {}}
-        }])
-        __salt__ = {
-            'docker.remove_network': docker_remove_network,
-            'docker.disconnect_container_from_network': docker_disconnect_container_from_network,
-            'docker.networks': docker_networks,
-        }
-        with patch.dict(docker_state.__dict__,
-                        {'__salt__': __salt__}):
-            ret = docker_state.absent('network_foo')
-        docker_disconnect_container_from_network.assert_not_called()
-        docker_remove_network.assert_not_called()
-        self.assertEqual(ret, {'name': 'network_foo',
-                               'comment': 'Network \'network_foo\' already absent',
-                               'changes': {},
-                               'result': True})

tests/unit/utils/test_network.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 # Import Python libs
 from __future__ import absolute_import
+import logging
 import socket
 
 # Import Salt Testing libs
@@ -11,6 +12,8 @@ from tests.support.mock import NO_MOCK, NO_MOCK_REASON, patch, MagicMock
 # Import salt libs
 import salt.utils.network as network
 
+log = logging.getLogger(__name__)
+
 LINUX = '''\
 eth0      Link encap:Ethernet  HWaddr e0:3f:49:85:6a:af
           inet addr:10.10.10.56  Bcast:10.10.10.255  Mask:255.255.252.0
@@ -96,6 +99,11 @@ USER     COMMAND    PID   FD PROTO  LOCAL ADDRESS    FOREIGN ADDRESS
 salt-master python2.781106 35 tcp4  127.0.0.1:61115  127.0.0.1:4506
 '''
 
+IPV4_SUBNETS = {True: ('10.10.0.0/24',),
+                False: ('10.10.0.0', '10.10.0.0/33', 'FOO', 9, '0.9.800.1000/24')}
+IPV6_SUBNETS = {True: ('::1/128',),
+                False: ('::1', '::1/129', 'FOO', 9, 'aj01::feac/64')}
+
 
 @skipIf(NO_MOCK, NO_MOCK_REASON)
 class NetworkTestCase(TestCase):
@@ -160,6 +168,31 @@ class NetworkTestCase(TestCase):
         self.assertFalse(network.is_ipv6('10.0.1.2'))
         self.assertFalse(network.is_ipv6('2001.0db8.85a3.0000.0000.8a2e.0370.7334'))
 
+    def test_is_subnet(self):
+        for subnet_data in (IPV4_SUBNETS, IPV6_SUBNETS):
+            for item in subnet_data[True]:
+                log.debug('Testing that %s is a valid subnet', item)
+                self.assertTrue(network.is_subnet(item))
+            for item in subnet_data[False]:
+                log.debug('Testing that %s is not a valid subnet', item)
+                self.assertFalse(network.is_subnet(item))
+
+    def test_is_ipv4_subnet(self):
+        for item in IPV4_SUBNETS[True]:
+            log.debug('Testing that %s is a valid subnet', item)
+            self.assertTrue(network.is_ipv4_subnet(item))
+        for item in IPV4_SUBNETS[False]:
+            log.debug('Testing that %s is not a valid subnet', item)
+            self.assertFalse(network.is_ipv4_subnet(item))
+
+    def test_is_ipv6_subnet(self):
+        for item in IPV6_SUBNETS[True]:
+            log.debug('Testing that %s is a valid subnet', item)
+            self.assertTrue(network.is_ipv6_subnet(item))
+        for item in IPV6_SUBNETS[False]:
+            log.debug('Testing that %s is not a valid subnet', item)
+            self.assertFalse(network.is_ipv6_subnet(item))
+
     def test_cidr_to_ipv4_netmask(self):
         self.assertEqual(network.cidr_to_ipv4_netmask(24), '255.255.255.0')
         self.assertEqual(network.cidr_to_ipv4_netmask(21), '255.255.248.0')