Repository: praekeltfoundation/marathon-acme

Issue:
Support Marathon 1.5 networking API
Marathon 1.5 isn't out yet but changes things up a lot:
https://github.com/mesosphere/marathon/blob/master/docs/docs/networking.md

Discussion:

Patch:
diff --git a/.travis.yml b/.travis.yml
index 2046992..c0f46b6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -7,7 +7,7 @@ branches:
   only:
     - develop
     - master
-    - /\d+\.\d+(\.\d+)?/
+    - /^\d+\.\d+(\.\d+)?$/
 
 language: python
 matrix:
diff --git a/marathon_acme/marathon_util.py b/marathon_acme/marathon_util.py
index 771939d..36b257c 100644
--- a/marathon_acme/marathon_util.py
+++ b/marathon_acme/marathon_util.py
@@ -2,46 +2,101 @@ def get_number_of_app_ports(app):
     """
     Get the number of ports for the given app JSON. This roughly follows the
     logic in marathon-lb for finding app IPs/ports, although we are only
-    interested in the quantity of ports an app has:
-    https://github.com/mesosphere/marathon-lb/blob/v1.7.0/utils.py#L278-L350
+    interested in the quantity of ports an app should have and don't consider
+    the specific IPs/ports of individual tasks:
+    https://github.com/mesosphere/marathon-lb/blob/v1.10.3/utils.py#L393-L415
 
     :param app: The app JSON from the Marathon API.
     :return: The number of ports for the app.
     """
-    if _is_ip_per_task(app):
-        if _is_user_network(app):
-            return len(app['container']['docker']['portMappings'])
-        else:
-            return len(app['ipAddress']['discovery']['ports'])
+    mode = _get_networking_mode(app)
+    ports_list = None
+    if mode == 'host':
+        ports_list = _get_port_definitions(app)
+    elif mode == 'container/bridge':
+        ports_list = _get_port_definitions(app)
+        if ports_list is None:
+            ports_list = _get_container_port_mappings(app)
+    elif mode == 'container':
+        ports_list = _get_ip_address_discovery_ports(app)
+        # Marathon 1.5+: the ipAddress field is missing -> ports_list is None
+        # Marathon <1.5: the ipAddress field can be present, but ports_list can
+        # still be empty while the container port mapping is not :-/
+        if not ports_list:
+            ports_list = _get_container_port_mappings(app)
     else:
-        # Prefer the 'portDefinitions' field added in Marathon 1.0.0 but fall
-        # back to the deprecated 'ports' array if that's not present.
-        if 'portDefinitions' in app:
-            return len(app['portDefinitions'])
-        else:
-            return len(app['ports'])
+        raise RuntimeError(
+            "Unknown Marathon networking mode '{}'".format(mode))
 
+    return len(ports_list)
 
-def _is_ip_per_task(app):
+
+def _get_networking_mode(app):
     """
-    Return whether the application is using IP-per-task.
-    :param app: The application to check.
-    :return: True if using IP per task, False otherwise.
+    Get the Marathon networking mode for the app.
     """
-    return app.get('ipAddress') is not None
+    # Marathon 1.5+: there is a `networks` field
+    networks = app.get('networks')
+    if networks:
+        # Modes cannot be mixed, so assigning the last mode is fine
+        return networks[-1].get('mode', 'container')
+
+    # Older Marathon: determine equivalent network mode
+    container = app.get('container')
+    if container is not None and 'docker' in container:
+        docker_network = container['docker'].get('network')
+        if docker_network == 'USER':
+            return 'container'
+        elif docker_network == 'BRIDGE':
+            return 'container/bridge'
+
+    return 'container' if _is_legacy_ip_per_task(app) else 'host'
 
 
-def _is_user_network(app):
+def _get_container_port_mappings(app):
     """
-    Returns True if container network mode is set to USER
-    :param app: The application to check.
-    :return: True if using USER network, False otherwise.
+    Get the ``portMappings`` field for the app container.
     """
-    container = app.get('container')
-    if container is None:
-        return False
+    container = app['container']
+
+    # Marathon 1.5+: container.portMappings field
+    port_mappings = container.get('portMappings')
+
+    # Older Marathon: container.docker.portMappings field
+    if port_mappings is None and 'docker' in container:
+        port_mappings = container['docker'].get('portMappings')
 
-    if container['type'] != 'DOCKER':
-        return False
+    return port_mappings
 
-    return container['docker']['network'] == 'USER'
+
+def _get_port_definitions(app):
+    """
+    Get the ``portDefinitions`` field for the app if present.
+    """
+    if 'portDefinitions' in app:
+        return app['portDefinitions']
+
+    # In the worst case try use the old `ports` array
+    # Only useful on very old Marathons
+    if 'ports' in app:
+        return app['ports']
+
+    return None
+
+
+def _get_ip_address_discovery_ports(app):
+    """
+    Get the ports from the ``ipAddress`` field for the app if present.
+    """
+    if not _is_legacy_ip_per_task(app):
+        return None
+    return app['ipAddress']['discovery']['ports']
+
+
+def _is_legacy_ip_per_task(app):
+    """
+    Return whether the application is using IP-per-task on Marathon < 1.5.
+    :param app: The application to check.
+    :return: True if using IP per task, False otherwise.
+    """
+    return app.get('ipAddress') is not None


Test patch:
diff --git a/marathon_acme/tests/test_marathon_util.py b/marathon_acme/tests/test_marathon_util.py
index e793424..9eb7c2f 100644
--- a/marathon_acme/tests/test_marathon_util.py
+++ b/marathon_acme/tests/test_marathon_util.py
@@ -1,4 +1,5 @@
 import pytest
+from testtools import ExpectedException
 from testtools.assertions import assert_that
 from testtools.matchers import Equals
 
@@ -53,6 +54,19 @@ TEST_APP = {
     'deployments': [],
 }
 
+CONTAINER_HOST_NETWORKING = {
+    'type': 'DOCKER',
+    'volumes': [],
+    'docker': {
+        'image': 'praekeltfoundation/marathon-lb:1.6.0',
+        'network': 'HOST',
+        'portMappings': [],
+        'privileged': True,
+        'parameters': [],
+        'forcePullImage': False
+    }
+}
+
 CONTAINER_USER_NETWORKING = {
     'type': 'DOCKER',
     'volumes': [],
@@ -115,6 +129,77 @@ CONTAINER_MESOS = {
     ],
 }
 
+# https://github.com/mesosphere/marathon/blob/v1.5.1/docs/docs/networking.md#host-mode
+NETWORKS_CONTAINER_HOST_MARATHON15 = [{'mode': 'host'}]
+CONTAINER_MESOS_HOST_NETWORKING_MARATHON15 = {
+    'type': 'MESOS',
+    'docker': {
+        'image': 'my-image:1.0'
+    },
+}
+
+# https://github.com/mesosphere/marathon/blob/v1.5.1/docs/docs/networking.md#specifying-ports-1
+NETWORKS_CONTAINER_BRIDGE_MARATHON15 = [{'mode': 'container/bridge'}]
+CONTAINER_BRIDGE_NETWORKING_MARATHON15 = {
+    'type': 'DOCKER',
+    'docker': {
+      'forcePullImage': True,
+      'image': 'praekeltfoundation/mc2:release-3.11.2',
+      'parameters': [
+        {
+          'key': 'add-host',
+          'value': 'servicehost:172.17.0.1'
+        }
+      ],
+      'privileged': False
+    },
+    'volumes': [],
+    'portMappings': [
+      {
+        'containerPort': 80,
+        'hostPort': 0,
+        'labels': {},
+        'protocol': 'tcp',
+        'servicePort': 10005
+      }
+    ]
+}
+CONTAINER_MESOS_BRIDGE_NETWORKING_MARATHON15 = {
+    'type': 'MESOS',
+    'docker': {
+        'image': 'my-image:1.0'
+    },
+    'portMappings': [
+        {'containerPort': 80, 'hostPort': 0, 'name': 'http'},
+        {'containerPort': 443, 'hostPort': 0, 'name': 'https'},
+        {'containerPort': 4000, 'hostPort': 0, 'name': 'mon'}
+    ]
+}
+
+# https://github.com/mesosphere/marathon/blob/v1.5.1/docs/docs/networking.md#enabling-container-mode
+NETWORKS_CONTAINER_USER_MARATHON15 = [{'mode': 'container', 'name': 'dcos'}]
+CONTAINER_USER_NETWORKING_MARATHON15 = {
+    'type': 'DOCKER',
+    'docker': {
+      'forcePullImage': False,
+      'image': 'python:3-alpine',
+      'parameters': [],
+      'privileged': False
+    },
+    'volumes': [],
+    'portMappings': [
+      {
+        'containerPort': 8080,
+        'labels': {
+          'VIP_0': '/foovu1:8080'
+        },
+        'name': 'foovu1http',
+        'protocol': 'tcp',
+        'servicePort': 10004
+      }
+    ],
+}
+
 IP_ADDRESS_NO_PORTS = {
     'groups': [],
     'labels': {},
@@ -151,6 +236,17 @@ def test_app():
 
 
 class TestGetNumberOfAppPortsFunc(object):
+    def test_host_networking(self, test_app):
+        """
+        When the app uses Docker containers with HOST networking, the ports
+        should be counted from the 'portDefinitions' field.
+        """
+        test_app['container'] = CONTAINER_HOST_NETWORKING
+        test_app['portDefinitions'] = PORT_DEFINITIONS_ONE_PORT
+
+        num_ports = get_number_of_app_ports(test_app)
+        assert_that(num_ports, Equals(1))
+
     def test_user_networking(self, test_app):
         """
         When the app uses a Docker container with USER networking, it will have
@@ -207,3 +303,64 @@ class TestGetNumberOfAppPortsFunc(object):
 
         num_ports = get_number_of_app_ports(test_app)
         assert_that(num_ports, Equals(2))
+
+    def test_host_networking_mesos_marathon15(self, test_app):
+        """
+        For Marathon 1.5+, when the app uses Mesos containers with host
+        networking, the ports should be counted from the 'portDefinitions'
+        field.
+        """
+        test_app['container'] = CONTAINER_MESOS_HOST_NETWORKING_MARATHON15
+        test_app['networks'] = NETWORKS_CONTAINER_HOST_MARATHON15
+        test_app['portDefinitions'] = PORT_DEFINITIONS_ONE_PORT
+
+        num_ports = get_number_of_app_ports(test_app)
+        assert_that(num_ports, Equals(1))
+
+    def test_bridge_networking_marathon15(self, test_app):
+        """
+        For Marathon 1.5+, when the app uses Docker containers with
+        'container/bridge' networking, the ports should be counted from the
+        ``container.portMappings`` field.
+        """
+        test_app['container'] = CONTAINER_BRIDGE_NETWORKING_MARATHON15
+        test_app['networks'] = NETWORKS_CONTAINER_BRIDGE_MARATHON15
+
+        num_ports = get_number_of_app_ports(test_app)
+        assert_that(num_ports, Equals(1))
+
+    def test_bridge_networking_mesos_marathon15(self, test_app):
+        """
+        For Marathon 1.5+, when the app uses Mesos containers with
+        'container/bridge' networking, the ports should be counted from the
+        ``container.portMappings`` field.
+        """
+        test_app['container'] = CONTAINER_MESOS_BRIDGE_NETWORKING_MARATHON15
+        test_app['networks'] = NETWORKS_CONTAINER_BRIDGE_MARATHON15
+
+        num_ports = get_number_of_app_ports(test_app)
+        assert_that(num_ports, Equals(3))
+
+    def test_user_networking_marathon15(self, test_app):
+        """
+        For Marathon 1.5+, when the app uses Docker containers with 'container'
+        networking, the ports should be counted from the
+        ``container.portMappings`` field.
+        """
+        test_app['container'] = CONTAINER_USER_NETWORKING_MARATHON15
+        test_app['networks'] = NETWORKS_CONTAINER_USER_MARATHON15
+
+        num_ports = get_number_of_app_ports(test_app)
+        assert_that(num_ports, Equals(1))
+
+    def test_unknown_networking_mode(self, test_app):
+        """
+        When an app is defined with an unknown networking mode, an error is
+        raised.
+        """
+        test_app['networks'] = [{'mode': 'container/iptables'}]
+
+        with ExpectedException(
+            RuntimeError,
+                r"Unknown Marathon networking mode 'container/iptables'"):
+            get_number_of_app_ports(test_app)