From e54a1a52ec246e199d2a9731a390111ae3ba61da Mon Sep 17 00:00:00 2001
From: Dan Smith <dansmith@redhat.com>
Date: Tue, 9 Feb 2021 11:55:46 -0800
Subject: [PATCH] Allow plugins to mutate image extra_properties

This adds a set_image_extra_properties() method to the action wrapper.
It specifically disallows setting os_glance_* properties, for two
reasons. First, several (such as the task lock and the store lists)
need special handling to be atomic. Second, setting os_glance_*
properties from image metadata injection is almost definitely
destined for failure, now or later, as it would muck with our
internals. As discussed during the work to formally reserve that
namespace from the API, we also need to make sure operators do not
set these keys during injection.

This makes us drop any such keys, with appropriate logging. The
next patch will make the metadata injection task use this and is
the point at which we will actually change that behavior.

Change-Id: I0574ee3daf08d59b4547e353c921451e756e09f6
---
 glance/async_/flows/api_image_import.py       | 19 +++++++++
 .../async_/flows/test_api_image_import.py     | 41 +++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/glance/async_/flows/api_image_import.py b/glance/async_/flows/api_image_import.py
index a7ca79d4bf..ea79f99ebd 100644
--- a/glance/async_/flows/api_image_import.py
+++ b/glance/async_/flows/api_image_import.py
@@ -29,6 +29,7 @@ from taskflow.patterns import linear_flow as lf
 from taskflow import retry
 from taskflow import task
 
+from glance.api import common as api_common
 import glance.async_.flows._internal_plugins as internal_plugins
 import glance.async_.flows.plugins as import_plugins
 from glance.common import exception
@@ -301,6 +302,24 @@ class _ImportActions(object):
                 raise AttributeError('Setting %s is not allowed' % attr)
             setattr(self._image, attr, value)
 
+    def set_image_extra_properties(self, properties):
+        """Merge values into image extra_properties.
+
+        This allows a plugin to set additional properties on the image,
+        as long as those are outside the reserved namespace. Any keys
+        in the internal namespace will be dropped (and logged).
+
+        :param properties: A dict of properties to be merged in
+        """
+        for key, value in properties.items():
+            if key.startswith(api_common.GLANCE_RESERVED_NS):
+                LOG.warning(('Dropping %(key)s=%(val)s during metadata '
+                             'injection for %(image)s'),
+                            {'key': key, 'val': value,
+                             'image': self.image_id})
+            else:
+                self._image.extra_properties[key] = value
+
     def remove_location_for_store(self, backend):
         """Remove a location from an image given a backend store.
 
diff --git a/glance/tests/unit/async_/flows/test_api_image_import.py b/glance/tests/unit/async_/flows/test_api_image_import.py
index 48ed12a17c..027e680d5f 100644
--- a/glance/tests/unit/async_/flows/test_api_image_import.py
+++ b/glance/tests/unit/async_/flows/test_api_image_import.py
@@ -689,6 +689,47 @@ class TestImportActionWrapper(test_utils.BaseTestCase):
             self.assertRaises(AttributeError,
                               action.set_image_attribute, id='foo')
 
+    @mock.patch.object(import_flow, 'LOG')
+    def test_set_image_extra_properties(self, mock_log):
+        mock_repo = mock.MagicMock()
+        mock_image = mock_repo.get.return_value
+        mock_image.image_id = IMAGE_ID1
+        mock_image.extra_properties = {'os_glance_import_task': TASK_ID1}
+        mock_image.status = 'bar'
+        wrapper = import_flow.ImportActionWrapper(mock_repo, IMAGE_ID1,
+                                                  TASK_ID1)
+        # One banned property
+        with wrapper as action:
+            action.set_image_extra_properties({'os_glance_foo': 'bar'})
+        self.assertEqual({'os_glance_import_task': TASK_ID1},
+                         mock_image.extra_properties)
+
+        mock_log.warning.assert_called()
+        mock_log.warning.reset_mock()
+
+        # Two banned properties
+        with wrapper as action:
+            action.set_image_extra_properties({'os_glance_foo': 'bar',
+                                               'os_glance_baz': 'bat'})
+        self.assertEqual({'os_glance_import_task': TASK_ID1},
+                         mock_image.extra_properties)
+
+        mock_log.warning.assert_called()
+        mock_log.warning.reset_mock()
+
+        # One banned and one allowed property
+        with wrapper as action:
+            action.set_image_extra_properties({'foo': 'bar',
+                                               'os_glance_foo': 'baz'})
+        self.assertEqual({'foo': 'bar',
+                          'os_glance_import_task': TASK_ID1},
+                         mock_image.extra_properties)
+
+        mock_log.warning.assert_called_once_with(
+            'Dropping %(key)s=%(val)s during metadata injection for %(image)s',
+            {'key': 'os_glance_foo', 'val': 'baz',
+             'image': IMAGE_ID1})
+
     def test_drop_lock_for_task(self):
         mock_repo = mock.MagicMock()
         mock_repo.get.return_value.extra_properties = {