Merge pull request #103 from Cryptophobia/master

fix(controller): revert release.check_image_access for now

Author: Cryptophobia

rootfs/api/models/app.py

@@ -523,12 +523,8 @@ def deploy(self, release, force_deploy=False, rollback_on_failure=True):  # noqa
             # create the application config in k8s (secret in this case) for all deploy objects
             self.set_application_config(release)
             # only buildpack apps need access to object storage
-            # only docker apps need check access to the image, so users can't exploit the k8s
-            # image cache to gain access to other users' images
             if release.build.type == 'buildpack':
                 self.create_object_store_secret()
-            else:
-                release.check_image_access()
 
             # gather all proc types to be deployed
             tasks = [

rootfs/api/models/release.py

@@ -3,7 +3,7 @@
 from django.conf import settings
 from django.db import models
 
-from registry import publish_release, get_port as docker_get_port, check_access as docker_check_access, RegistryException # noqa
+from registry import publish_release, get_port as docker_get_port, RegistryException
 from api.utils import dict_diff
 from api.models import UuidAuditedModel
 from api.exceptions import DeisException, AlreadyExists
@@ -136,13 +136,6 @@ def publish(self):
         deis_registry = bool(self.build.source_based)
         publish_release(source_image, self.image, deis_registry, self.get_registry_auth())
 
-    def check_image_access(self):
-        try:
-            creds = self.get_registry_auth()
-            docker_check_access(self.image, creds)
-        except Exception as e:
-            raise DeisException(str(e)) from e
-
     def get_port(self):
         """
         Get application port for a given release. If pulling from private registry

rootfs/api/tests/test_app.py

@@ -35,7 +35,6 @@ 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.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class AppTest(DeisTestCase):
     """Tests creation of applications"""
 

rootfs/api/tests/test_build.py

@@ -23,7 +23,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class BuildTest(DeisTransactionTestCase):
 
     """Tests build notification from build system"""
@@ -592,44 +591,6 @@ def test_build_image_in_registry_with_auth(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 201, response.data)
 
-    def test_build_image_no_registry_password(self, mock_requests):
-        """build with image from private registry, but no password given"""
-        app_id = self.create_app()
-
-        # post an image as a build
-        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
-            mock_check_access.side_effect = Exception('no no no')  # let the image access fail
-            url = "/v2/apps/{app_id}/builds".format(**locals())
-            image = 'autotest/example'
-            response = self.client.post(url, {'image': image})
-            self.assertEqual(response.status_code, 400, response.data)
-
-    def test_build_image_wrong_registry_password(self, mock_requests):
-        """build with image from private registry, but wrong password given"""
-        app_id = self.create_app()
-
-        # post an image as a build using registry hostname
-        url = "/v2/apps/{app_id}/builds".format(**locals())
-        image = 'autotest/example'
-        response = self.client.post(url, {'image': image})
-        self.assertEqual(response.status_code, 201, response.data)
-
-        # add the required PORT information
-        url = '/v2/apps/{app_id}/config'.format(**locals())
-        body = {'values': json.dumps({'PORT': '80'})}
-        response = self.client.post(url, body)
-        self.assertEqual(response.status_code, 201, response.data)
-
-        # set some registry information
-        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
-            mock_check_access.side_effect = Exception('no no no')  # let the image access fail
-            url = '/v2/apps/{app_id}/config'.format(**locals())
-            body = {'registry': json.dumps({'username': 'bob', 'password': 'zoomzoom'})}
-            response = self.client.post(url, body)
-            self.assertEqual(response.status_code, 400, response.data)
-            mock_check_access.assert_called_with(
-                image, {'username': 'bob', 'password': 'zoomzoom', 'email': 'autotest@deis.io'})
-
     def test_build_image_in_registry_with_auth_no_port(self, mock_requests):
         """add authentication to the build but with no PORT config"""
         app_id = self.create_app()

rootfs/api/tests/test_config.py

@@ -21,7 +21,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class ConfigTest(DeisTransactionTestCase):
     """Tests setting and updating config values"""
 

rootfs/api/tests/test_healthchecks.py

@@ -13,7 +13,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class TestHealthchecks(DeisTransactionTestCase):
     """Tests setting and updating config values"""
 

rootfs/api/tests/test_hooks.py

@@ -50,7 +50,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class HookTest(DeisTransactionTestCase):
 
     """Tests API hooks used to trigger actions from external components"""

rootfs/api/tests/test_pods.py

@@ -21,7 +21,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class PodTest(DeisTransactionTestCase):
     """Tests creation of pods on nodes"""
 

rootfs/api/tests/test_registry.py

@@ -1,6 +1,5 @@
 import json
 import requests_mock
-from unittest import mock
 
 from django.core.cache import cache
 from django.contrib.auth.models import User
@@ -137,11 +136,7 @@ def test_registry_deploy(self, mock_requests):
         self.assertEqual(response.data['registry']['password'], 's3cur3pw1')
 
         # post a new build
-        with mock.patch('api.models.release.docker_check_access') as mock_check_access:
-            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)
-            mock_check_access.assert_called_with(
-                'autotest/example',
-                {'password': 's3cur3pw1', 'username': 'bob', 'email': 'autotest@deis.io'})
+        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)

rootfs/api/tests/test_release.py

@@ -21,7 +21,6 @@
 @requests_mock.Mocker(real_http=True, adapter=adapter)
 @mock.patch('api.models.release.publish_release', lambda *args: None)
 @mock.patch('api.models.release.docker_get_port', mock_port)
-@mock.patch('api.models.release.docker_check_access', lambda *args: None)
 class ReleaseTest(DeisTransactionTestCase):
 
     """Tests push notification from build system"""

rootfs/registry/__init__.py

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

rootfs/registry/dockerclient.py

@@ -154,17 +154,6 @@ def inspect_image(self, target):
         # inspect the image
         return self.client.inspect_image(target)
 
-    def check_access(self, target, creds=None):
-        """
-        Check access to the docker image, with the given creds (if any).
-        Due to the k8s docker image cache, we can't rely on k8s to do this
-        check - see https://github.com/teamhephy/workflow/issues/78
-        """
-        name, _ = docker.utils.parse_repository_tag(target)
-        self.login(name, creds)
-        self.inspect_image(target)
-        # no exception == success
-
 
 def check_blacklist(repo):
     """Check a Docker repository name for collision with deis/* components."""
@@ -208,7 +197,3 @@ def publish_release(source, target, deis_registry, creds=None):
 
 def get_port(target, deis_registry, creds=None):
     return DockerClient().get_port(target, deis_registry, creds)
-
-
-def check_access(target, creds=None):
-    return DockerClient().check_access(target, creds)