Merge pull request #718 from helgi/image_secret

helgi · helgi · commit 9979523faf25 · 2016-05-18T11:09:02.000-07:00
feat(images): use imagePullSecret when pulling a private image
diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py
@@ -333,12 +333,12 @@ 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': envs,
+                'registry': release.config.registry,
                 'version': "v{}".format(release.version),
                 'replicas': replicas,
                 'app_type': scale_type,
@@ -352,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
                 )
@@ -383,14 +383,15 @@ 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': envs,
+                'registry': release.config.registry,
                 # only used if there is no previous RC
                 'replicas': replicas,
                 'version': "v{}".format(release.version),
@@ -583,6 +584,7 @@ def pod_name(size=5, chars=string.ascii_lowercase + string.digits):
             'cpu': release.config.cpu,
             'tags': release.config.tags,
             'envs': release.config.values,
+            'registry': release.config.registry,
             'version': "v{}".format(release.version),
             'build_type': release.build.type,
         }
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
@@ -40,7 +36,19 @@ def __str__(self):
 
     @property
     def image(self):
-        # return image if it is already in the registry, test host and then host + port
+        # Builder pushes to internal registry, exclude SHA based images from being returned
+        registry = self.config.registry
+        if (
+            registry.get('username', None) and
+            registry.get('password', None) and
+            # SHA means it came from a git push (builder)
+            not self.build.sha and
+            # hostname tells Builder where to push images
+            not registry.get('hostname', None)
+        ):
+            return self.build.image
+
+        # return image if it is already in a registry, test host and then host + port
         if (
             self.build.image.startswith(settings.REGISTRY_HOST) or
             self.build.image.startswith(settings.REGISTRY_URL)
@@ -95,49 +103,80 @@ def publish(self):
         if self.build.source_based:
             return
 
-        source_image = self.build.image
+        # Builder pushes to internal registry, exclude SHA based images from being returned early
+        registry = self.config.registry
+        if (
+            registry.get('username', None) and
+            registry.get('password', None) and
+            # SHA means it came from a git push (builder)
+            not self.build.sha and
+            # hostname tells Builder where to push images
+            not registry.get('hostname', None)
+        ):
+            log_event(self.app, '{} exists in the target registry. Using image for release {} of app {}'.format(self.build.image, self.version, self.app))  # noqa
+            return
+
         # return image if it is already in the registry, test host and then host + port
         if (
-            source_image.startswith(settings.REGISTRY_HOST) or
-            source_image.startswith(settings.REGISTRY_URL)
+            self.build.image.startswith(settings.REGISTRY_HOST) or
+            self.build.image.startswith(settings.REGISTRY_URL)
         ):
-            log_event(self.app, '{} already exists in the target registry. Using this image for release {} of app {}'.format(source_image, self.version, self.app))  # noqa
+            log_event(self.app, '{} exists in the target registry. Using image for release {} of app {}'.format(self.build.image, self.version, self.app))  # noqa
             return
 
         # add tag if it was not provided
+        source_image = self.build.image
         if ':' not in source_image:
             source_image = "{}:{}".format(source_image, self.build.version)
 
         # if build is source based then it was pushed into the deis registry
         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"""
@@ -665,6 +665,36 @@ def test_registry(self, mock_requests):
         response = self.client.delete(url)
         self.assertEqual(response.status_code, 405, response.data)
 
+    def test_registry_deploy(self, mock_requests):
+        """
+        Test that registry information can be applied
+        """
+        url = '/v2/apps'
+        response = self.client.post(url)
+        self.assertEqual(response.status_code, 201, response.data)
+        app_id = response.data['id']
+
+        # Set healthcheck URL to get defaults set
+        body = {'registry': json.dumps({
+            'username': 'bob',
+            'password': 's3cur3pw1'
+        })}
+        resp = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            body
+        )
+        self.assertEqual(resp.status_code, 201, response.data)
+        self.assertIn('username', resp.data['registry'])
+        self.assertIn('password', resp.data['registry'])
+        self.assertEqual(resp.data['registry']['username'], 'bob')
+        self.assertEqual(resp.data['registry']['password'], 's3cur3pw1')
+
+        # post a new build
+        url = "/v2/apps/{app_id}/builds".format(**locals())
+        body = {'image': 'autotest/example'}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
     def test_config_owner_is_requesting_user(self, mock_requests):
         """
         Ensure that setting the config value is owned by the requesting user
@@ -724,6 +754,6 @@ def test_healthchecks(self, mock_requests):
 
         # post a new build
         url = "/v2/apps/{app_id}/builds".format(**locals())
-        body = {'image': 'autotest/example'}
+        body = {'image': 'quay.io/autotest/example'}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
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

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`