image create/update: fix handling of custom properties

When passing custom properties that are not strings to image-create,
image-create-via-import and image-update, we get the following error:

---8<------------------------------------------------------------------
$ glance image-create --name myimg --property min_ram=123
Unable to set 'min_ram' to '123'. Reason: '123' is not of type 'integer'

Failed validating 'type' in schema['properties']['min_ram']:
    {'description': 'Amount of ram (in MB) required to boot image.',
     'type': 'integer'}

On instance['min_ram']:
    '123'
---8<------------------------------------------------------------------

Because we get custom properties through the command line, they all
default to being strings, which means warlock cannot properly validate
their types.

Furthermore, warlock does not check the readOnly attribute of the
jsonschema, which means it will not complain if we try to set the value
of a read-only property, thus making one useless API call and taking
time to give feedback to the user:

---8<------------------------------------------------------------------
$ time glance image-create --name myimg --property updated_at=foo
HTTP 403 Forbidden: Attribute &#x27;updated_at&#x27; is read-only.

real    0m1.191s
user    0m0.503s
sys     0m0.063s
---8<------------------------------------------------------------------

This patch makes sure that:
- integer properties are cast from string to integer before they have to
  be validated by Warlock;
- we exit early if a read-only property is given on the command line;
- we do not repeat ourselves by factorising this code in
  _populate_properties.

Change-Id: I1cd417262ed66925300ecfe7252d2e1ff9e45f46
Closes-Bug: #1578596

glanceclient/tests/unit/v2/test_images.py

@@ -710,6 +710,7 @@ schema_fixtures = {
                             'required': ['url', 'metadata'],
                         }
                     },
+                    'min_ram': {'type': 'integer'},
                     'color': {'type': 'string', 'is_base': False},
                     'tags': {'type': 'array'},
                 },
@@ -998,6 +999,14 @@ class TestController(testtools.TestCase):
         self.assertEqual('3a4560a1-e585-443e-9b39-553b46ec92d1', image.id)
         self.assertEqual('image-1', image.name)
 
+    def test_create_image_bad_type(self):
+        properties = {
+            'name': 'image-1',
+            'min_ram': '42',  # This should be an integer, not a string
+        }
+        with testtools.ExpectedException(TypeError):
+            self.controller.create(**properties)
+
     def test_create_bad_additionalProperty_type(self):
         properties = {
             'name': 'image-1',

glanceclient/tests/unit/v2/test_shell_v2.py

@@ -54,7 +54,13 @@ def schema_args(schema_getter, omit=None):
                 'description': 'Format of the disk'},
             'location': {'type': 'string'},
             'locations': {'type': 'string'},
-            'copy_from': {'type': 'string'}}}
+            'copy_from': {'type': 'string'},
+            'virtual_size': {
+                'readOnly': True,
+                'type': ['null', 'integer'],
+            },
+        }
+    }
     return original_schema_args(my_schema_getter, omit)
 
 
@@ -3491,3 +3497,26 @@ class ShellV2Test(testtools.TestCase):
             mock_exit.assert_called_once_with(
                 'Direct server endpoint needs to be provided. Do '
                 'not use loadbalanced or catalog endpoints.')
+
+    def test_populate_properties(self):
+        fields = {
+            'property': [
+                'container_format=bare',
+                'disk_format=qcow2',
+                'min_ram=42',
+            ]
+        }
+        test_shell._populate_properties(fields)
+        self.assertEqual(fields['container_format'], 'bare')
+        self.assertEqual(fields['disk_format'], 'qcow2')
+        self.assertEqual(fields['min_ram'], 42)
+
+    def test_populate_properties_read_only(self):
+        fields = {
+            'property': [
+                'container_format=bare',
+                'disk_format=qcow2',
+                'virtual_size=1337',
+            ]
+        }
+        self.assertRaises(SystemExit, test_shell._populate_properties, fields)

glanceclient/v2/shell.py

@@ -49,6 +49,57 @@ def get_image_schema():
     return IMAGE_SCHEMA
 
 
+def _populate_properties(fields):
+    """Add one entry to "fields" per property defined in the "property" key
+
+    The "fields" parameter is modified in-place:
+    - the "property" field is removed;
+    - one entry per property is added.
+
+    If a read-only property is encountered, the whole program exits.
+
+    Example: if this function is called with a "fields" variable that has the
+    following value:
+
+        {
+            'property': [
+                'min_disk=1',
+                'min_ram=42',
+            ]
+        }
+
+    Then, after this function has exited, "fields" will have the following
+    value:
+
+        {
+            'min_disk': 1,
+            'min_ram': 42,
+        }
+    """
+    schema = get_image_schema()
+    schema_properties = schema.get('properties', {})
+    for datum in fields.pop('property', []):
+        key, value = datum.split('=', 1)
+        # Warlock does not check whether a property is marked as 'readOnly' in
+        # the schema, so we do it 'manually' here.
+        if schema_properties.get(key, {}).get('readOnly', False):
+            utils.exit(f'Property "{key}" is read-only')
+
+        # We want to pass the properties to warlock so that it validates their
+        # types, but all of our properties are strings, since that is what we
+        # got from argparse. Let us try to cast the value to the expected type.
+        type_ = schema_properties.get(key, {}).get('type', 'string')
+        try:
+            if type_ == 'integer':
+                value = int(value)
+        except ValueError:
+            # The value given by the user does not have the expected type. Let
+            # warlock fail to validate it, since the error message will be
+            # helpful to the user.
+            pass
+        fields[key] = value
+
+
 @utils.schema_args(get_image_schema, omit=['locations', 'os_hidden'])
 # NOTE(rosmaita): to make this option more intuitive for end users, we
 # do not use the Glance image property name 'os_hidden' here.  This means
@@ -82,10 +133,7 @@ def do_image_create(gc, args):
                           schema.is_core_property(x[0])),
                          _args))
 
-    raw_properties = fields.pop('property', [])
-    for datum in raw_properties:
-        key, value = datum.split('=', 1)
-        fields[key] = value
+    _populate_properties(fields)
 
     backend = args.store
 
@@ -199,10 +247,7 @@ def do_image_create_via_import(gc, args):
                           schema.is_core_property(x[0])),
                          _args))
 
-    raw_properties = fields.pop('property', [])
-    for datum in raw_properties:
-        key, value = datum.split('=', 1)
-        fields[key] = value
+    _populate_properties(fields)
 
     file_name = fields.pop('file', None)
     using_stdin = hasattr(sys.stdin, 'isatty') and not sys.stdin.isatty()
@@ -369,10 +414,7 @@ def do_image_update(gc, args):
                           schema.is_core_property(x[0])),
                          _args))
 
-    raw_properties = fields.pop('property', [])
-    for datum in raw_properties:
-        key, value = datum.split('=', 1)
-        fields[key] = value
+    _populate_properties(fields)
 
     remove_properties = fields.pop('remove_property', None)