ref(port): when using a private registry then look for a PORT env var instead of auto discovering port

helgi · helgi · commit ab964ff72db9 · 2016-05-17T19:31:30.000-07:00
Rationale here is to not pull down the image since Kubernetes now owns the downloading and interaction with the Private Registry. Pulling an image down into the Deis Registry will strip away the benefits of the security given.

This change moves the docker port auto-discovery into the docker-client and general port handling into Release model, making the Scheduler unaware of the inherent logic. Ports are still auto-discovered on public images but when Private Registry is used the default port is 5000 and if the user wants to use a different one then they need to do config:set PORT=3000

Logic may change in the future where a user can use Private Registry but pull into the local registry, and get port auto-discovery, at the cost of security.
diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py
@@ -333,12 +333,11 @@ def _scale_pods(self, scale_types):
             # fetch application port and inject into ENV Vars as needed
             envs['PORT'] = release.get_port(routable)
 
-            image = release.image
             kwargs = {
                 'memory': release.config.memory,
                 'cpu': release.config.cpu,
                 'tags': release.config.tags,
-                'envs': release.config.values,
+                'envs': envs,
                 'registry': release.config.registry,
                 'version': "v{}".format(release.version),
                 'replicas': replicas,
@@ -353,7 +352,7 @@ def _scale_pods(self, scale_types):
                 self._scheduler.scale(
                     namespace=self.id,
                     name=self._get_job_id(scale_type),
-                    image=image,
+                    image=release.image,
                     command=command,
                     **kwargs
                 )
@@ -384,14 +383,14 @@ def deploy(self, release):
             # only web / cmd are routable
             # http://docs.deis.io/en/latest/using_deis/process-types/#web-vs-cmd-process-types
             routable = True if scale_type in ['web', 'cmd'] else False
-            # fetch application port and inject into ENV Vars as needed
+            # fetch application port and inject into ENV vars as needed
             envs['PORT'] = release.get_port(routable)
 
             deploys[scale_type] = {
                 'memory': release.config.memory,
                 'cpu': release.config.cpu,
                 'tags': release.config.tags,
-                'envs': release.config.values,
+                'envs': envs,
                 'registry': release.config.registry,
                 # only used if there is no previous RC
                 'replicas': replicas,
diff --git a/rootfs/api/models/release.py b/rootfs/api/models/release.py
@@ -3,11 +3,7 @@
 from django.conf import settings
 from django.db import models
 
-import docker
-from docker import Client
-from retrying import retry
-
-from registry import publish_release, RegistryException
+from registry import publish_release, get_port as docker_get_port, RegistryException
 from api.utils import dict_diff
 from api.models import UuidAuditedModel, log_event, DeisException
 from scheduler import KubeHTTPException
@@ -137,32 +133,50 @@ def publish(self):
         deis_registry = bool(self.build.source_based)
         publish_release(source_image, self.image, deis_registry, self.get_registry_auth())
 
-    @retry(stop_max_attempt_number=3, wait_fixed=1000)
     def get_port(self, routable=False):
         """
-        Get a port from a Docker image
+        Get application port for a given release. If pulling from private registry
+        then use default port or read from ENV var, otherwise attempt to pull from
+        the docker image
         """
-        port = None
-        # Only care about port for routable application
-        if not routable:
-            return port
-
-        if self.build.type == "buildpack":
-            msg = "Using default port 5000 for build pack image {}".format(self.image)
-            log_event(self.app, msg)
-            return 5000
-
-        # get the target repository name and tag
-        repo, tag = docker.utils.parse_repository_tag(self.image)
-
-        docker_cli = Client(version="auto")
-        docker_cli.pull(repo, tag=tag, insecure_registry=True)
-        image_info = docker_cli.inspect_image(self.image)
-        if 'ExposedPorts' not in image_info['Config']:
-            return None
+        try:
+            deis_registry = bool(self.build.source_based)
+            envs = self.config.values
+            creds = self.get_registry_auth()
+
+            port = None
+            # Only care about port for routable application
+            if not routable:
+                return port
+
+            if self.build.type == "buildpack":
+                msg = "Using default port 5000 for build pack image {}".format(self.image)
+                log_event(self.app, msg)
+                return 5000
+
+            # application has registry auth - $PORT is required
+            if creds is not None:
+                if envs.get('PORT', None) is None:
+                    log_event(self.app, 'Private registry detected but no $PORT defined. Defaulting to $PORT 5000', logging.WARNING)  # noqa
+                    return 5000
+
+                # User provided PORT
+                return envs.get('PORT')
+
+            # If the user provides PORT
+            if envs.get('PORT', None):
+                return envs.get('PORT')
+
+            # discover port from docker image
+            port = docker_get_port(self.image, deis_registry, creds)
+            if port is None:
+                msg = "Expose a port or make the app non routable by changing the process type"
+                log_event(self.app, msg, logging.ERROR)
+                raise DeisException(msg)
 
-        port = int(list(image_info['Config']['ExposedPorts'].keys())[0].split("/")[0])
-        return port
+            return port
+        except Exception as e:
+            raise DeisException(str(e)) from e
 
     def get_registry_auth(self):
         """
diff --git a/rootfs/api/tests/test_app.py b/rootfs/api/tests/test_app.py
@@ -31,7 +31,7 @@ def _mock_run(*args, **kwargs):
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class AppTest(APITestCase):
     """Tests creation of applications"""
 
diff --git a/rootfs/api/tests/test_build.py b/rootfs/api/tests/test_build.py
@@ -25,7 +25,7 @@
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class BuildTest(APITransactionTestCase):
 
     """Tests build notification from build system"""
diff --git a/rootfs/api/tests/test_config.py b/rootfs/api/tests/test_config.py
@@ -21,7 +21,7 @@
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class ConfigTest(APITransactionTestCase):
 
     """Tests setting and updating config values"""
diff --git a/rootfs/api/tests/test_hooks.py b/rootfs/api/tests/test_hooks.py
@@ -35,7 +35,7 @@
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class HookTest(APITransactionTestCase):
 
     """Tests API hooks used to trigger actions from external components"""
diff --git a/rootfs/api/tests/test_pods.py b/rootfs/api/tests/test_pods.py
@@ -24,7 +24,7 @@
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class PodTest(APITransactionTestCase):
     """Tests creation of pods on nodes"""
 
diff --git a/rootfs/api/tests/test_release.py b/rootfs/api/tests/test_release.py
@@ -23,7 +23,7 @@
 
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
-@mock.patch('api.models.release.Release.get_port', mock_port)
+@mock.patch('api.models.release.docker_get_port', mock_port)
 class ReleaseTest(APITransactionTestCase):
 
     """Tests push notification from build system"""
diff --git a/rootfs/registry/__init__.py b/rootfs/registry/__init__.py
@@ -1 +1 @@
-from .dockerclient import publish_release, RegistryException  # noqa
+from .dockerclient import publish_release, get_port, RegistryException  # noqa
diff --git a/rootfs/registry/dockerclient.py b/rootfs/registry/dockerclient.py
@@ -7,6 +7,7 @@
 from django.conf import settings
 from rest_framework.exceptions import PermissionDenied
 from simpleflock import SimpleFlock
+from retrying import retry
 
 import docker
 import docker.constants
@@ -59,6 +60,28 @@ def login(self, repository, creds=None):
 
         logger.info('Successfully logged into {} with {}'.format(repository, registry_auth['username']))  # noqa
 
+    def get_port(self, target, deis_registry=False, creds=None):
+        """
+        Get a port from a Docker image
+        """
+        # get the target repository name and tag
+        name, _ = docker.utils.parse_repository_tag(target)
+
+        # strip any "http://host.domain:port" prefix from the target repository name,
+        # since we always publish to the Deis registry
+        repo, name = auth.split_repo_name(name)
+
+        # log into pull repo
+        if not deis_registry:
+            self.login(repo, creds)
+
+        info = self.inspect_image(target, deis_registry)
+        if 'ExposedPorts' not in info['Config']:
+            return None
+
+        port = int(list(info['Config']['ExposedPorts'].keys())[0].split('/')[0])
+        return port
+
     def publish_release(self, source, target, deis_registry=False, creds=None):
         """Update a source Docker image with environment config and publish it to deis-registry."""
         # get the source repository name and tag
@@ -121,6 +144,22 @@ def tag(self, image, repo, tag):
         if not self.client.tag(image, repo, tag=tag, force=True):
             raise RegistryException('Tagging {} as {}:{} failed'.format(image, repo, tag))
 
+    @retry(stop_max_attempt_number=3, wait_fixed=1000)
+    def inspect_image(self, target, insecure_registry=True):
+        """
+        Inspect docker image to gather information from it
+
+        try thrice to find the port before raising exception as docker-py is flaky
+        """
+        # image already includes the tag, so we split it out here
+        repo, tag = docker.utils.parse_repository_tag(target)
+
+        # make sure image is pulled locally already
+        self.pull(repo, tag=tag, insecure_registry=insecure_registry)
+
+        # inspect the image
+        return self.client.inspect_image(target)
+
 
 def check_blacklist(repo):
     """Check a Docker repository name for collision with deis/* components."""
@@ -156,3 +195,7 @@ def stream_error(chunk, operation, repo, tag):
 
 def publish_release(source, target, deis_registry, creds=None):
     return DockerClient().publish_release(source, target, deis_registry, creds)
+
+
+def get_port(target, deis_registry, creds=None):
+    return DockerClient().get_port(target, deis_registry, creds)
diff --git a/rootfs/registry/tests.py b/rootfs/registry/tests.py
@@ -9,7 +9,7 @@
 
 from django.conf import settings
 from rest_framework.exceptions import PermissionDenied
-from registry import publish_release, RegistryException
+from registry import publish_release, get_port, RegistryException
 from registry.dockerclient import DockerClient
 
 
@@ -20,6 +20,28 @@ class DockerClientTest(unittest.TestCase):
     def setUp(self):
         settings.REGISTRY_HOST, settings.REGISTRY_PORT = 'localhost', 5000
 
+    def test_get_port(self, mock_client):
+        self.client = DockerClient()
+
+        # Make sure login is not called when there are no creds
+        get_port('ozzy/embryo:git-f2a8020', False)
+        self.assertFalse(self.client.client.login.called)
+
+        creds = {
+            'username': 'fake',
+            'password': 'fake',
+            'email': 'fake',
+            'registry': 'quay.io'
+        }
+
+        client = {}
+        client['Status'] = 'Login Succeeded'
+        self.client.client.login.return_value = client
+        get_port('ozzy/embryo:git-f2a8020', False, creds)
+        self.assertTrue(self.client.client.login.called)
+        self.assertTrue(self.client.client.pull.called)
+        self.assertTrue(self.client.client.inspect_image.called)
+
     def test_publish_release(self, mock_client):
         self.client = DockerClient()
 
diff --git a/rootfs/scheduler/__init__.py b/rootfs/scheduler/__init__.py
@@ -8,7 +8,6 @@
 import base64
 
 from django.conf import settings
-from docker import Client
 from docker.auth import auth as docker_auth
 from .states import PodState
 import requests
@@ -697,7 +696,7 @@ def _set_container(self, namespace, data, **kwargs):  # noqa
         if kwargs.get('healthcheck', None):
             self._healthcheck(namespace, data, kwargs.get('routable'), **kwargs['healthcheck'])
         else:
-            self._default_readiness_probe(data, kwargs.get('build_type'), kwargs.get('port', None))
+            self._default_readiness_probe(data, kwargs.get('build_type'), env.get('PORT', None))
 
     def _set_image_secret(self, data, namespace, **kwargs):
         """
@@ -927,7 +926,7 @@ def _wait_until_pods_terminate(self, namespace, labels, current, desired):
 
         logger.debug("{} pods in namespace {} are terminated".format(delta, namespace))
 
-    def _wait_until_pods_are_ready(self, namespace, container, labels, desired):
+    def _wait_until_pods_are_ready(self, namespace, container, labels, desired):  # noqa
         # If desired is 0 then there is no ready state to check on
         if desired == 0:
             return
@@ -1054,6 +1053,8 @@ def _scale_rc(self, namespace, name, desired):
 
     def _create_rc(self, namespace, name, image, command, **kwargs):  # noqa
         app_type = kwargs.get('app_type')
+        build_type = kwargs.get('build_type')
+
         container_name = namespace + '-' + app_type
         args = command.split()
         storageType = os.getenv("APP_STORAGE")
@@ -1076,7 +1077,7 @@ def _create_rc(self, namespace, name, image, command, **kwargs):  # noqa
         }
 
         # Check if it is a slug builder image.
-        if kwargs.get('build_type') == "buildpack":
+        if build_type == "buildpack":
             # only buildpack apps need access to object storage
             try:
                 self._get_secret(namespace, 'objectstorage-keyfile')
@@ -1092,17 +1093,15 @@ def _create_rc(self, namespace, name, image, command, **kwargs):  # noqa
         template = json.loads(string.Template(TEMPLATE).substitute(l))
         spec = template["spec"]["template"]["spec"]
 
-        spec = template["spec"]["template"]["spec"]
-
         # apply tags as needed to restrict pod to particular node(s)
         spec["nodeSelector"] = kwargs.get('tags', {})
 
         # Deal with container information
         container = spec["containers"][0]
         container['args'] = args
 
-        kwargs['image'] = l['image']
         # set information to the application container
+        kwargs['image'] = l['image']
         self._set_container(namespace, container, **kwargs)
         # add image to the mix
         self._set_image_secret(spec, namespace, **kwargs)

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-from .dockerclient import publish_release, RegistryException # noqa`
	`1`	`+from .dockerclient import publish_release, get_port, RegistryException # noqa`