fix(docker): bubble up docker errors from the internal docker client

Fixes #556

Author: helgi

rootfs/api/models/release.py

@@ -2,8 +2,9 @@
 
 from django.conf import settings
 from django.db import models
+from rest_framework.serializers import ValidationError
 
-from registry import publish_release
+from registry import publish_release, RegistryException
 from api.utils import dict_diff
 
 from api.models import UuidAuditedModel, log_event
@@ -77,6 +78,10 @@ def new(self, user, config, build, summary=None, source_version='latest'):
             # If we cannot publish this app, just log and carry on
             log_event(self.app, e)
             pass
+        except RegistryException as e:
+            log_event(self.app, e)
+            # Uses ValidationError to get return of 400 up in views
+            raise ValidationError({'detail': str(e)})
 
         return release
 
@@ -99,8 +104,8 @@ def publish(self, source_version='latest'):
             source_image = "{}:{}".format(source_image, source_tag)
 
         # If the build has a SHA, assume it's from deis-builder and in the deis-registry already
-        deis_registry = bool(self.build.sha)
         if not self.build.dockerfile and not self.build.sha:
+            deis_registry = bool(self.build.sha)
             publish_release(source_image, self.image, deis_registry)
 
     def previous(self):

rootfs/registry/__init__.py

@@ -1 +1 @@
-from .dockerclient import publish_release  # noqa
+from .dockerclient import publish_release, RegistryException  # noqa

rootfs/registry/dockerclient.py

@@ -7,12 +7,18 @@
 from django.conf import settings
 from rest_framework.exceptions import PermissionDenied
 from simpleflock import SimpleFlock
+
 import docker
 import docker.constants
+from docker.errors import APIError
 
 logger = logging.getLogger(__name__)
 
 
+class RegistryException(Exception):
+    pass
+
+
 class DockerClient(object):
     """Use the Docker API to pull, tag, build, and push images to deis-registry."""
 
@@ -40,35 +46,41 @@ def publish_release(self, source, target, deis_registry):
             repo = "{}/{}".format(self.registry, src_name)
         else:
             repo = src_name
-        self.pull(repo, src_tag)
 
-        # tag the image locally without the repository URL
-        image = "{}:{}".format(src_name, src_tag)
-        self.tag(image, "{}/{}".format(self.registry, name), tag=tag)
+        try:
+            self.pull(repo, src_tag)
+
+            # tag the image locally without the repository URL
+            image = "{}:{}".format(src_name, src_tag)
+            self.tag(image, "{}/{}".format(self.registry, name), tag=tag)
 
-        # push the image to deis-registry
-        self.push("{}/{}".format(self.registry, name), tag)
+            # push the image to deis-registry
+            self.push("{}/{}".format(self.registry, name), tag)
+        except APIError as e:
+            raise RegistryException(str(e))
 
     def pull(self, repo, tag):
         """Pull a Docker image into the local storage graph."""
         check_blacklist(repo)
         logger.info("Pulling Docker image {}:{}".format(repo, tag))
         with SimpleFlock(self.FLOCKFILE, timeout=1200):
-            stream = self.client.pull(repo, tag=tag, stream=True, insecure_registry=True)
-            log_output(stream)
+            stream = self.client.pull(repo, tag=tag, stream=True,
+                                      decode=True, insecure_registry=True)
+            log_output(stream, 'pull', repo, tag)
 
     def push(self, repo, tag):
         """Push a local Docker image to a registry."""
         logger.info("Pushing Docker image {}:{}".format(repo, tag))
-        stream = self.client.push(repo, tag=tag, stream=True, insecure_registry=True)
-        log_output(stream)
+        stream = self.client.push(repo, tag=tag, stream=True, decode=True,
+                                  insecure_registry=True)
+        log_output(stream, 'push', repo, tag)
 
     def tag(self, image, repo, tag):
         """Tag a local Docker image with a new name and tag."""
         check_blacklist(repo)
         logger.info("Tagging Docker image {} as {}:{}".format(image, repo, tag))
         if not self.client.tag(image, repo, tag=tag, force=True):
-            raise docker.errors.DockerException("tagging failed")
+            raise RegistryException('Tagging {} as {}:{} failed'.format(image, repo, tag))
 
 
 def check_blacklist(repo):
@@ -81,13 +93,26 @@ def check_blacklist(repo):
         raise PermissionDenied("Repository name {} is not allowed".format(repo))
 
 
-def log_output(stream):
-    """Log a stream at DEBUG level, and raise DockerException if it contains "error"."""
+def log_output(stream, operation, repo, tag):
+    """Log a stream at DEBUG level, and raise RegistryException if it contains an error"""
     for chunk in stream:
-        logger.debug(chunk)
         # error handling requires looking at the response body
-        if b'"error"' in chunk.lower():
-            raise docker.errors.DockerException(chunk)
+        if 'error' in chunk:
+            stream_error(chunk, operation, repo, tag)
+
+
+def stream_error(chunk, operation, repo, tag):
+    """Translate docker stream errors into a more digestable format"""
+    # not all errors provide the code
+    if 'code' in chunk['errorDetail']:
+        # permission denied on the repo
+        if chunk['errorDetail']['code'] == 403:
+            message = 'Permission Denied attempting to {} image {}:{}'.format(operation, repo, tag)
+    else:
+        # grab the generic error and strip the useless Error: portion
+        message = chunk['error'].replace('Error: ', '')
+
+    raise RegistryException(message)
 
 
 def strip_prefix(name):
@@ -97,5 +122,4 @@ def strip_prefix(name):
 
 
 def publish_release(source, target, deis_registry):
-    client = DockerClient()
-    return client.publish_release(source, target, deis_registry)
+    return DockerClient().publish_release(source, target, deis_registry)

rootfs/registry/tests.py

@@ -33,7 +33,8 @@ def test_publish_release(self, mock_client):
         self.client.publish_release('ozzy/embryo:git-f2a8020', 'quay.io/ozzy/embryo:v4', True)
         docker_push = self.client.client.push
         docker_push.assert_called_with(
-            'localhost:5000/ozzy/embryo', tag='v4', insecure_registry=True, stream=True)
+            'localhost:5000/ozzy/embryo', tag='v4', insecure_registry=True,
+            decode=True, stream=True)
         # Test that blacklisted image names can't be published
         with self.assertRaises(PermissionDenied):
             self.client.publish_release(
@@ -47,7 +48,7 @@ def test_pull(self, mock_client):
         self.client.pull('alpine', '3.2')
         docker_pull = self.client.client.pull
         docker_pull.assert_called_once_with(
-            'alpine', tag='3.2', insecure_registry=True, stream=True)
+            'alpine', tag='3.2', insecure_registry=True, decode=True, stream=True)
         # Test that blacklisted image names can't be pulled
         with self.assertRaises(PermissionDenied):
             self.client.pull('deis/controller', 'v1.11.1')
@@ -59,7 +60,7 @@ def test_push(self, mock_client):
         self.client.push('ozzy/embryo', 'v4')
         docker_push = self.client.client.push
         docker_push.assert_called_once_with(
-            'ozzy/embryo', tag='v4', insecure_registry=True, stream=True)
+            'ozzy/embryo', tag='v4', insecure_registry=True, decode=True, stream=True)
 
     def test_tag(self, mock_client):
         self.client = DockerClient()