Raise bad request early if image metadata is invalid

Image data is currently uploaded into the backend store even if it is a bad
request. This patch add value validation in the deserializer so we don't set bad
data. It also does a full validation of the image metadata to make sure
everything that is needed for an image upload is set correctly before the upload
can occur.

There were currently some relevant tests that had incorrect names causing them
to not run. These have been fixed and modified.

Fixes bug 1035982

Change-Id: I6f3ef83ba7b3de3cb885fad9737fef1a03d9222c

glance/api/v1/images.py

@@ -57,6 +57,9 @@
 LOG = logging.getLogger(__name__)
 SUPPORTED_PARAMS = glance.api.v1.SUPPORTED_PARAMS
 SUPPORTED_FILTERS = glance.api.v1.SUPPORTED_FILTERS
+CONTAINER_FORMATS = ['ami', 'ari', 'aki', 'bare', 'ovf']
+DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi',
+               'iso']
 
 
 # Defined at module level due to _is_opt_registered
@@ -67,6 +70,43 @@
 CONF.register_opt(default_store_opt)
 
 
+def validate_image_meta(req, values):
+
+    name = values.get('name')
+    disk_format = values.get('disk_format')
+    container_format = values.get('container_format')
+
+    if 'disk_format' in values:
+        if not disk_format in DISK_FORMATS:
+            msg = "Invalid disk format '%s' for image." % disk_format
+            raise HTTPBadRequest(explanation=msg, request=req)
+
+    if 'container_format' in values:
+        if not container_format in CONTAINER_FORMATS:
+            msg = "Invalid container format '%s' for image." % container_format
+            raise HTTPBadRequest(explanation=msg, request=req)
+
+    if name and len(name) > 255:
+        msg = _('Image name too long: %d') % len(name)
+        raise HTTPBadRequest(explanation=msg, request=req)
+
+    amazon_formats = ('aki', 'ari', 'ami')
+
+    if disk_format in amazon_formats or container_format in amazon_formats:
+        if disk_format is None:
+            values['disk_format'] = container_format
+        elif container_format is None:
+            values['container_format'] = disk_format
+        elif container_format != disk_format:
+            msg = ("Invalid mix of disk and container formats. "
+                   "When setting a disk or container format to "
+                   "one of 'aki', 'ari', or 'ami', the container "
+                   "and disk formats must match.")
+            raise HTTPBadRequest(explanation=msg, request=req)
+
+    return values
+
+
 class Controller(controller.BaseController):
     """
     WSGI controller for images resource in Glance v1 API
@@ -566,6 +606,8 @@ def _get_size(self, context, image_meta, location):
 
     def _handle_source(self, req, image_id, image_meta, image_data):
         if image_data:
+            image_meta = self._validate_image_for_activation(req, image_id,
+                                                        image_meta)
             image_meta = self._upload_and_activate(req, image_meta)
         elif self._copy_from(req):
             msg = _('Triggering asynchronous copy from external source')
@@ -574,9 +616,23 @@ def _handle_source(self, req, image_id, image_meta, image_data):
         else:
             location = image_meta.get('location')
             if location:
+                self._validate_image_for_activation(req, image_id, image_meta)
                 image_meta = self._activate(req, image_id, location)
         return image_meta
 
+    def _validate_image_for_activation(self, req, id, values):
+        """Ensures that all required image metadata values are valid."""
+        image = self.get_image_meta_or_404(req, id)
+        if not 'disk_format' in values:
+            values['disk_format'] = image['disk_format']
+        if not 'container_format' in values:
+            values['container_format'] = image['container_format']
+        if not 'name' in values:
+            values['name'] = image['name']
+
+        values = validate_image_meta(req, values)
+        return values
+
     @utils.mutating
     def create(self, req, image_meta, image_data):
         """
@@ -851,6 +907,7 @@ def _deserialize(self, request):
             raise HTTPBadRequest(explanation=msg, request=request)
 
         image_meta = result['image_meta']
+        image_meta = validate_image_meta(request, image_meta)
         if request.content_length:
             image_size = request.content_length
         elif 'size' in image_meta:

glance/db/sqlalchemy/api.py

@@ -46,9 +46,6 @@
 LOG = os_logging.getLogger(__name__)
 
 
-CONTAINER_FORMATS = ['ami', 'ari', 'aki', 'bare', 'ovf']
-DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi',
-               'iso']
 STATUSES = ['active', 'saving', 'queued', 'killed', 'pending_delete',
             'deleted']
 
@@ -535,8 +532,6 @@ def validate_image(values):
     :param values: Mapping of image metadata to check
     """
     status = values.get('status')
-    disk_format = values.get('disk_format')
-    container_format = values.get('container_format')
 
     status = values.get('status', None)
     if not status:
@@ -547,53 +542,6 @@ def validate_image(values):
         msg = "Invalid image status '%s' for image." % status
         raise exception.Invalid(msg)
 
-    def _amazon_format(disk, container):
-        amazon_formats = ('aki', 'ari', 'ami')
-        return ((disk in amazon_formats and
-                 (container in CONTAINER_FORMATS or container is None)) or
-                (container in amazon_formats and
-                 (disk in DISK_FORMATS or disk is None)))
-
-    def _only_one_of(a, b):
-        return (a and b is None) or (b and a is None)
-
-    if _amazon_format(disk_format, container_format):
-        if _only_one_of(container_format, disk_format):
-            container_format = (container_format if disk_format is None
-                                else disk_format)
-            values['container_format'] = container_format
-            disk_format = container_format
-            values['disk_format'] = disk_format
-        elif container_format != disk_format:
-            msg = ("Invalid mix of disk and container formats. "
-                   "When setting a disk or container format to "
-                   "one of 'aki', 'ari', or 'ami', the container "
-                   "and disk formats must match.")
-            raise exception.Invalid(msg)
-
-    def _required_format_absent(format, formats):
-        activating = status == 'active'
-        unrecognized = format not in formats
-        # We don't mind having format = None when we're just registering
-        # an image, but if the image is being activated, make sure that the
-        # format is valid. Conversely if the format happens to be set on
-        # registration, it must be one of the recognized formats.
-        return ((activating and (not format or unrecognized))
-                or (not activating and format and unrecognized))
-
-    if _required_format_absent(disk_format, DISK_FORMATS):
-        msg = "Invalid disk format '%s' for image." % disk_format
-        raise exception.Invalid(msg)
-
-    if _required_format_absent(container_format, CONTAINER_FORMATS):
-        msg = "Invalid container format '%s' for image." % container_format
-        raise exception.Invalid(msg)
-
-    name = values.get('name')
-    if name and len(name) > 255:
-        msg = _('Image name too long: %d') % len(name)
-        raise exception.Invalid(msg)
-
     return values
 
 

glance/tests/functional/v1/test_api.py

@@ -530,8 +530,11 @@ def test_traceback_not_consumed(self):
             test_data_file.flush()
         path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
         http = httplib2.Http()
+        headers = minimal_headers('Image1')
+        headers['Content-Type'] = 'not octet-stream'
         response, content = http.request(path, 'POST',
-                            body=test_data_file.name)
+                            body=test_data_file.name,
+                            headers=headers)
         self.assertEqual(response.status, 400)
         expected = "Content-Type must be application/octet-stream"
         self.assertTrue(expected in content,
@@ -1229,7 +1232,7 @@ def test_unsupported_default_store(self):
                           expected_exitcode=255,
                           **self.__dict__.copy())
 
-    def _do_test_post_image_content_missing_format(self, format):
+    def _do_test_post_image_content_bad_format(self, format):
         """
         We test that missing container/disk format fails with 400 "Bad Request"
 
@@ -1238,11 +1241,19 @@ def _do_test_post_image_content_missing_format(self, format):
         self.cleanup()
         self.start_servers(**self.__dict__.copy())
 
+        # Verify no public images
+        path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 200)
+        images = json.loads(content)['images']
+        self.assertEqual(len(images), 0)
+
         path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
 
         # POST /images without given format being specified
         headers = minimal_headers('Image1')
-        del headers['X-Image-Meta-' + format]
+        headers['X-Image-Meta-' + format] = 'bad_value'
         with tempfile.NamedTemporaryFile() as test_data_file:
             test_data_file.write("XXX")
             test_data_file.flush()
@@ -1252,19 +1263,28 @@ def _do_test_post_image_content_missing_format(self, format):
                             body=test_data_file.name)
         self.assertEqual(response.status, 400)
         type = format.replace('_format', '')
-        expected = "Details: Invalid %s format 'None' for image" % type
+        expected = "Invalid %s format 'bad_value' for image" % type
         self.assertTrue(expected in content,
                         "Could not find '%s' in '%s'" % (expected, content))
 
+        # make sure the image was not created
+        # Verify no public images
+        path = "http://%s:%d/v1/images" % ("127.0.0.1", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 200)
+        images = json.loads(content)['images']
+        self.assertEqual(len(images), 0)
+
         self.stop_servers()
 
     @skip_if_disabled
-    def _do_test_post_image_content_missing_diskformat(self):
-        self._do_test_post_image_content_missing_format('container_format')
+    def test_post_image_content_bad_container_format(self):
+        self._do_test_post_image_content_bad_format('container_format')
 
     @skip_if_disabled
-    def _do_test_post_image_content_missing_disk_format(self):
-        self._do_test_post_image_content_missing_format('disk_format')
+    def test_post_image_content_bad_disk_format(self):
+        self._do_test_post_image_content_bad_format('disk_format')
 
     def _do_test_put_image_content_missing_format(self, format):
         """
@@ -1288,6 +1308,7 @@ def _do_test_put_image_content_missing_format(self, format):
         self.assertEqual(response.status, 201)
         data = json.loads(content)
         image_id = data['image']['id']
+        print data
 
         # PUT image content images without given format being specified
         path = ("http://%s:%d/v1/images/%s" %
@@ -1303,18 +1324,18 @@ def _do_test_put_image_content_missing_format(self, format):
                             body=test_data_file.name)
         self.assertEqual(response.status, 400)
         type = format.replace('_format', '')
-        expected = "Details: Invalid %s format 'None' for image" % type
+        expected = "Invalid %s format 'None' for image" % type
         self.assertTrue(expected in content,
                         "Could not find '%s' in '%s'" % (expected, content))
 
         self.stop_servers()
 
     @skip_if_disabled
-    def _do_test_put_image_content_missing_diskformat(self):
+    def test_put_image_content_bad_container_format(self):
         self._do_test_put_image_content_missing_format('container_format')
 
     @skip_if_disabled
-    def _do_test_put_image_content_missing_disk_format(self):
+    def test_put_image_content_bad_disk_format(self):
         self._do_test_put_image_content_missing_format('disk_format')
 
     @skip_if_disabled

glance/tests/unit/test_clients.py

@@ -1058,57 +1058,6 @@ def test_add_image_with_bad_status(self):
                           self.client.add_image,
                           fixture)
 
-    def test_add_image_with_acceptably_long_name(self):
-        """Tests adding image with acceptably long name"""
-        name = 'x' * 255
-        fixture = {'name': name,
-                   'is_public': True,
-                   'disk_format': 'vmdk',
-                   'container_format': 'ovf',
-                   'size': 19,
-                   'location': "file:///tmp/glance-tests/2",
-                  }
-
-        new_image = self.client.add_image(fixture)
-
-        data = self.client.get_image(new_image['id'])
-        self.assertEquals(name, data['name'])
-
-    def test_add_image_with_excessively_long_name(self):
-        """Tests adding image with excessively long name"""
-        name = 'x' * 256
-        fixture = {'name': name,
-                   'is_public': True,
-                   'disk_format': 'vmdk',
-                   'container_format': 'ovf',
-                   'size': 19,
-                   'location': "file:///tmp/glance-tests/2",
-                  }
-
-        self.assertRaises(exception.Invalid,
-                          self.client.add_image,
-                          fixture)
-
-    def test_update_image_with_acceptably_long_name(self):
-        """Tests updating image with acceptably long name"""
-        name = 'x' * 255
-        fixture = {'name': name}
-
-        self.assertTrue(self.client.update_image(UUID2, fixture))
-
-        data = self.client.get_image(UUID2)
-        self.assertEquals(name, data['name'])
-
-    def test_update_image_with_excessively_long_name(self):
-        """Tests updating image with excessively long name"""
-        name = 'x' * 256
-        fixture = {'name': name}
-
-        self.assertRaises(exception.Invalid,
-                          self.client.update_image,
-                          UUID2,
-                          fixture)
-
     def test_update_image(self):
         """Tests that the /images PUT registry API updates the image"""
         fixture = {'name': 'fake public image #2',