Add better error messages for invalid conf molds

Change-Id: Id2701ae2cf04b336dd74f4f9795dda5eea049db0
(cherry picked from commit b3f6da3df4f832a614f334b78d7dc2da8c718480)

4 files changed, 119 insertions, 13 deletions

diff --git a/ironic/common/molds.py b/ironic/common/molds.py
index 9c6a439bd..234fcc6e3 100644
--- a/ironic/common/molds.py
+++ b/ironic/common/molds.py
@@ -81,7 +81,17 @@ def get_configuration(task, url):
     auth_header = _get_auth_header(task)
     response = _request(url, auth_header)
     if response.status_code == requests.codes.ok:
-        return response.json()
+        if not response.content:
+            raise exception.IronicException(_(
+                "Configuration mold for node %(node_uuid)s at %(url)s is "
+                "empty") % {'node_uuid': task.node.uuid, 'url': url})
+        try:
+            return response.json()
+        except json.decoder.JSONDecodeError as jde:
+            raise exception.IronicException(_(
+                "Configuration mold for node %(node_uuid)s at %(url)s has "
+                "invalid JSON: %(error)s)")
+                % {'node_uuid': task.node.uuid, 'url': url, 'error': jde})
 
     response.raise_for_status()
 
diff --git a/ironic/drivers/modules/drac/management.py b/ironic/drivers/modules/drac/management.py
index 006357a08..3bacde962 100644
--- a/ironic/drivers/modules/drac/management.py
+++ b/ironic/drivers/modules/drac/management.py
@@ -25,6 +25,8 @@ import time
 
 from futurist import periodics
 from ironic_lib import metrics_utils
+import jsonschema
+from jsonschema import exceptions as json_schema_exc
 from oslo_log import log as logging
 from oslo_utils import importutils
 
@@ -93,6 +95,23 @@ _CLEAR_JOB_IDS = 'JID_CLEARALL'
 # Clean steps constant
 _CLEAR_JOBS_CLEAN_STEPS = ['clear_job_queue', 'known_good_state']
 
+_CONF_MOLD_SCHEMA = {
+    'type': 'object',
+    'properties': {
+        'oem': {
+            'type': 'object',
+            'properties': {
+                'interface': {'const': 'idrac-redfish'},
+                'data': {'type': 'object', 'minProperties': 1}
+            },
+            'required': ['interface', 'data']
+        }
+
+    },
+    'required': ['oem'],
+    'additionalProperties': False
+}
+
 
 def _get_boot_device(node, drac_boot_devices=None):
     client = drac_common.get_drac_client(node)
@@ -186,6 +205,19 @@ def _flexibly_program_boot_order(device, drac_boot_mode):
     return bios_settings
 
 
+def _validate_conf_mold(data):
+    """Validates iDRAC configuration mold JSON schema
+
+    :param data: dictionary of configuration mold data
+    :raises InvalidParameterValue: If configuration mold validation fails
+    """
+    try:
+        jsonschema.validate(data, _CONF_MOLD_SCHEMA)
+    except json_schema_exc.ValidationError as e:
+        raise exception.InvalidParameterValue(
+            _("Invalid configuration mold: %(error)s") % {'error': e})
+
+
 def set_boot_device(node, device, persistent=False):
     """Set the boot device for a node.
 
@@ -407,15 +439,7 @@ class DracRedfishManagement(redfish_management.RedfishManagement):
                        {'node': task.node.uuid,
                         'configuration_name': import_configuration_location}))
 
-        interface = configuration["oem"]["interface"]
-        if interface != "idrac-redfish":
-            raise exception.DracOperationError(
-                error=(_("Invalid configuration for node %(node)s "
-                         "in %(configuration_name)s. Supports only "
-                         "idrac-redfish, but found %(interface)s") %
-                       {'node': task.node.uuid,
-                        'configuration_name': import_configuration_location,
-                        'interface': interface}))
+        _validate_conf_mold(configuration)
 
         task_monitor = drac_utils.execute_oem_manager_method(
             task, 'import system configuration',
diff --git a/ironic/tests/unit/common/test_molds.py b/ironic/tests/unit/common/test_molds.py
index 53c0ad0ac..bd2c37e47 100644
--- a/ironic/tests/unit/common/test_molds.py
+++ b/ironic/tests/unit/common/test_molds.py
@@ -12,6 +12,7 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import json
 from unittest import mock
 
 from oslo_config import cfg
@@ -166,6 +167,7 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
         cfg.CONF.molds.storage = 'swift'
         response = mock.MagicMock()
         response.status_code = 200
+        response.content = "{'key': 'value'}"
         response.json.return_value = {'key': 'value'}
         mock_get.return_value = response
         url = 'https://example.com/file1'
@@ -199,6 +201,7 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
         cfg.CONF.molds.password = 'password'
         response = mock.MagicMock()
         response.status_code = 200
+        response.content = "{'key': 'value'}"
         response.json.return_value = {'key': 'value'}
         mock_get.return_value = response
         url = 'https://example.com/file2'
@@ -217,6 +220,7 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
         cfg.CONF.molds.password = None
         response = mock.MagicMock()
         response.status_code = 200
+        response.content = "{'key': 'value'}"
         response.json.return_value = {'key': 'value'}
         mock_get.return_value = response
         url = 'https://example.com/file2'
@@ -289,3 +293,31 @@ class ConfigurationMoldTestCase(db_base.DbTestCase):
                 'https://example.com/file2',
                 headers={'Authorization': 'Basic dXNlcjpwYXNzd29yZA=='})
             self.assertEqual(mock_get.call_count, 2)
+
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_configuration_empty(self, mock_get):
+        cfg.CONF.molds.storage = 'http'
+        response = mock.MagicMock()
+        response.status_code = 200
+        response.content = ''
+        mock_get.return_value = response
+        url = 'https://example.com/file2'
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.assertRaises(exception.IronicException,
+                              molds.get_configuration, task, url)
+
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_configuration_invalid_json(self, mock_get):
+        cfg.CONF.molds.storage = 'http'
+        response = mock.MagicMock()
+        response.status_code = 200
+        response.content = 'not json'
+        response.json.side_effect = json.decoder.JSONDecodeError(
+            'Expecting value', 'not json', 0)
+        mock_get.return_value = response
+        url = 'https://example.com/file2'
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            self.assertRaises(exception.IronicException,
+                              molds.get_configuration, task, url)
diff --git a/ironic/tests/unit/drivers/modules/drac/test_management.py b/ironic/tests/unit/drivers/modules/drac/test_management.py
index a115d56d8..ebc958424 100644
--- a/ironic/tests/unit/drivers/modules/drac/test_management.py
+++ b/ironic/tests/unit/drivers/modules/drac/test_management.py
@@ -909,8 +909,8 @@ class DracRedfishManagementTestCase(test_utils.BaseDracTest):
 
     @mock.patch.object(molds, 'get_configuration', autospec=True)
     @mock.patch.object(redfish_utils, 'get_system', autospec=True)
-    def test_import_configuration_incorrect_interface(self, mock_get_system,
-                                                      mock_get_configuration):
+    def test_import_configuration_incorrect_schema(self, mock_get_system,
+                                                   mock_get_configuration):
         task = mock.Mock(node=self.node, context=self.context)
         fake_manager_oem1 = mock.Mock()
         fake_manager1 = mock.Mock()
@@ -920,7 +920,7 @@ class DracRedfishManagementTestCase(test_utils.BaseDracTest):
             '{"oem": {"interface": "idrac-wsman", '
             '"data": {"prop1": "value1", "prop2": 2}}}')
 
-        self.assertRaises(exception.DracOperationError,
+        self.assertRaises(exception.InvalidParameterValue,
                           self.management.import_configuration, task, 'edge')
 
     @mock.patch.object(deploy_utils, 'get_async_step_return_state',
@@ -1360,3 +1360,43 @@ class DracRedfishManagementTestCase(test_utils.BaseDracTest):
             mock_manager_oem.job_service.delete_jobs.assert_called_once_with(
                 job_ids=['JID_CLEARALL'])
             mock_manager_oem.reset_idrac.assert_called_once_with()
+
+    def test__validate_conf_mold(self):
+        drac_mgmt._validate_conf_mold({'oem': {'interface': 'idrac-redfish',
+                                       'data': {'SystemConfiguration': {}}}})
+
+    def test__validate_conf_mold_oem_missing(self):
+        self.assertRaisesRegex(
+            exception.InvalidParameterValue,
+            "'oem' is a required property",
+            drac_mgmt._validate_conf_mold,
+            {'bios': {'reset': False}})
+
+    def test__validate_conf_mold_interface_missing(self):
+        self.assertRaisesRegex(
+            exception.InvalidParameterValue,
+            "'interface' is a required property",
+            drac_mgmt._validate_conf_mold,
+            {'oem': {'data': {'SystemConfiguration': {}}}})
+
+    def test__validate_conf_mold_interface_not_supported(self):
+        self.assertRaisesRegex(
+            exception.InvalidParameterValue,
+            "'idrac-redfish' was expected",
+            drac_mgmt._validate_conf_mold,
+            {'oem': {'interface': 'idrac-wsman',
+             'data': {'SystemConfiguration': {}}}})
+
+    def test__validate_conf_mold_data_missing(self):
+        self.assertRaisesRegex(
+            exception.InvalidParameterValue,
+            "'data' is a required property",
+            drac_mgmt._validate_conf_mold,
+            {'oem': {'interface': 'idrac-redfish'}})
+
+    def test__validate_conf_mold_data_empty(self):
+        self.assertRaisesRegex(
+            exception.InvalidParameterValue,
+            "does not have enough properties",
+            drac_mgmt._validate_conf_mold,
+            {'oem': {'interface': 'idrac-redfish', 'data': {}}})