From 3243cac3fc0c7abc7fd64099b07cad2eb648a3d7 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 28 Jul 2020 11:56:35 +0200 Subject: [PATCH] Relax extra_hardware data validation by default We've seen reports that extra data sometimes contains empty lists. There is no point to discard everything in this case. This change makes ironic-inspector log a warning for any item of incorrect length by default. The old behavior can be returned via [extra_hardware]strict. Also stops discarding the received data if parsing fails: maybe this data is not for us? Change-Id: I1af55fa4ec47b345479b9e5687ad480648e502ac --- ironic_inspector/conf/__init__.py | 2 + ironic_inspector/conf/extra_hardware.py | 33 +++++++++ ironic_inspector/conf/opts.py | 3 +- ironic_inspector/plugins/extra_hardware.py | 70 +++++++++++-------- .../test/unit/test_plugins_extra_hardware.py | 70 +++++++++++++++---- .../notes/extra-check-9cf0a7d89e534ccd.yaml | 16 +++++ 6 files changed, 151 insertions(+), 43 deletions(-) create mode 100644 ironic_inspector/conf/extra_hardware.py create mode 100644 releasenotes/notes/extra-check-9cf0a7d89e534ccd.yaml diff --git a/ironic_inspector/conf/__init__.py b/ironic_inspector/conf/__init__.py index e467336df..f12431181 100644 --- a/ironic_inspector/conf/__init__.py +++ b/ironic_inspector/conf/__init__.py @@ -17,6 +17,7 @@ from ironic_inspector.conf import coordination from ironic_inspector.conf import default from ironic_inspector.conf import discovery from ironic_inspector.conf import dnsmasq_pxe_filter +from ironic_inspector.conf import extra_hardware from ironic_inspector.conf import iptables from ironic_inspector.conf import ironic from ironic_inspector.conf import pci_devices @@ -35,6 +36,7 @@ coordination.register_opts(CONF) discovery.register_opts(CONF) default.register_opts(CONF) dnsmasq_pxe_filter.register_opts(CONF) +extra_hardware.register_opts(CONF) iptables.register_opts(CONF) ironic.register_opts(CONF) pci_devices.register_opts(CONF) diff --git a/ironic_inspector/conf/extra_hardware.py b/ironic_inspector/conf/extra_hardware.py new file mode 100644 index 000000000..99931c300 --- /dev/null +++ b/ironic_inspector/conf/extra_hardware.py @@ -0,0 +1,33 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from oslo_config import cfg + +from ironic_inspector.common.i18n import _ + + +_OPTS = [ + cfg.BoolOpt('strict', + default=False, + help=_('If True, refuse to parse extra data if at least one ' + 'record is too short. Additionally, remove the ' + 'incoming "data" even if parsing failed.')), +] + + +def register_opts(conf): + conf.register_opts(_OPTS, group='extra_hardware') + + +def list_opts(): + return _OPTS diff --git a/ironic_inspector/conf/opts.py b/ironic_inspector/conf/opts.py index 449e9eb67..dcf7f31ea 100644 --- a/ironic_inspector/conf/opts.py +++ b/ironic_inspector/conf/opts.py @@ -68,7 +68,7 @@ def list_opts(): ('discovery', ironic_inspector.conf.discovery.list_opts()), ('dnsmasq_pxe_filter', ironic_inspector.conf.dnsmasq_pxe_filter.list_opts()), - ('swift', ironic_inspector.conf.swift.list_opts()), + ('extra_hardware', ironic_inspector.conf.extra_hardware.list_opts()), ('ironic', ironic_inspector.conf.ironic.list_opts()), ('iptables', ironic_inspector.conf.iptables.list_opts()), ('port_physnet', ironic_inspector.conf.port_physnet.list_opts()), @@ -76,4 +76,5 @@ def list_opts(): ('pci_devices', ironic_inspector.conf.pci_devices.list_opts()), ('pxe_filter', ironic_inspector.conf.pxe_filter.list_opts()), ('service_catalog', ironic_inspector.conf.service_catalog.list_opts()), + ('swift', ironic_inspector.conf.swift.list_opts()), ] diff --git a/ironic_inspector/plugins/extra_hardware.py b/ironic_inspector/plugins/extra_hardware.py index b73650f6c..d9930d618 100644 --- a/ironic_inspector/plugins/extra_hardware.py +++ b/ironic_inspector/plugins/extra_hardware.py @@ -18,11 +18,14 @@ string in a Swift object. The object is named 'extra_hardware-' and is stored in the 'inspector' container. """ +from oslo_config import cfg + from ironic_inspector.plugins import base from ironic_inspector import utils LOG = utils.getProcessingLogger(__name__) EDEPLOY_ITEM_SIZE = 4 +CONF = cfg.CONF class ExtraHardwareHook(base.ProcessingHook): @@ -42,43 +45,54 @@ class ExtraHardwareHook(base.ProcessingHook): 'the ramdisk', node_info=node_info, data=introspection_data) return + data = introspection_data['data'] + if not self._is_edeploy_data(data): + LOG.warning('Extra hardware data was not in a recognised ' + 'format (eDeploy), and will not be forwarded to ' + 'introspection rules', node_info=node_info, + data=introspection_data) + if CONF.extra_hardware.strict: + LOG.debug('Deleting \"data\" key from introspection data as ' + 'it is malformed and strict mode is on.', + node_info=node_info, data=introspection_data) + del introspection_data['data'] + return # NOTE(sambetts) If data is edeploy format, convert to dicts for rules # processing, store converted data in introspection_data['extra']. # Delete introspection_data['data'], it is assumed unusable # by rules. - if self._is_edeploy_data(data): - LOG.debug('Extra hardware data is in eDeploy format, ' - 'converting to usable format', - node_info=node_info, data=introspection_data) - introspection_data['extra'] = self._convert_edeploy_data(data) - else: - LOG.warning('Extra hardware data was not in a recognised ' - 'format (eDeploy), and will not be forwarded to ' - 'introspection rules', node_info=node_info, - data=introspection_data) + + converted = {} + for item in data: + if not item: + continue + + try: + converted_0 = converted.setdefault(item[0], {}) + converted_1 = converted_0.setdefault(item[1], {}) + + try: + item[3] = int(item[3]) + except (ValueError, TypeError): + pass + + converted_1[item[2]] = item[3] + except IndexError: + LOG.warning('Ignoring invalid extra data item %s', item, + node_info=node_info, data=introspection_data) + + introspection_data['extra'] = converted LOG.debug('Deleting \"data\" key from introspection data as it is ' - 'assumed unusable by introspection rules. Raw data is ' - 'stored in swift', + 'assumed unusable by introspection rules.', node_info=node_info, data=introspection_data) del introspection_data['data'] def _is_edeploy_data(self, data): - return all(isinstance(item, list) and len(item) == EDEPLOY_ITEM_SIZE - for item in data) - - def _convert_edeploy_data(self, data): - converted = {} - for item in data: - converted_0 = converted.setdefault(item[0], {}) - converted_1 = converted_0.setdefault(item[1], {}) - - try: - item[3] = int(item[3]) - except (ValueError, TypeError): - pass - - converted_1[item[2]] = item[3] - return converted + return isinstance(data, list) and all( + isinstance(item, list) + and (not CONF.extra_hardware.strict + or len(item) == EDEPLOY_ITEM_SIZE) + for item in data) diff --git a/ironic_inspector/test/unit/test_plugins_extra_hardware.py b/ironic_inspector/test/unit/test_plugins_extra_hardware.py index 73faa2069..18268e36f 100644 --- a/ironic_inspector/test/unit/test_plugins_extra_hardware.py +++ b/ironic_inspector/test/unit/test_plugins_extra_hardware.py @@ -11,14 +11,22 @@ # See the License for the specific language governing permissions and # limitations under the License. +from unittest import mock + +from oslo_config import cfg + from ironic_inspector.plugins import extra_hardware from ironic_inspector.test import base as test_base +CONF = cfg.CONF + + +@mock.patch.object(extra_hardware.LOG, 'warning', autospec=True) class TestExtraHardware(test_base.NodeTest): hook = extra_hardware.ExtraHardwareHook() - def test_data_recieved(self): + def test_data_recieved(self, mock_warn): introspection_data = { 'data': [['memory', 'total', 'size', '4294967296'], ['cpu', 'physical', 'number', '1'], @@ -43,29 +51,63 @@ class TestExtraHardware(test_base.NodeTest): } self.assertEqual(expected, introspection_data['extra']) + self.assertFalse(mock_warn.called) - def test_data_not_in_edeploy_format(self): + def test_data_recieved_with_errors(self, mock_warn): + introspection_data = { + 'data': [['memory', 'total', 'size', '4294967296'], + [], + ['cpu', 'physical', 'number', '1'], + ['cpu', 'physical', 'WUT'], + ['cpu', 'logical', 'number', '1']]} + self.hook.before_processing(introspection_data) + self.hook.before_update(introspection_data, self.node_info) + + expected = { + 'memory': { + 'total': { + 'size': 4294967296 + } + }, + 'cpu': { + 'physical': { + 'number': 1 + }, + 'logical': { + 'number': 1 + }, + } + } + + self.assertEqual(expected, introspection_data['extra']) + # An empty list is not a warning, a bad record is. + self.assertEqual(1, mock_warn.call_count) + + def test_data_not_in_edeploy_format(self, mock_warn): introspection_data = { 'data': [['memory', 'total', 'size', '4294967296'], ['cpu', 'physical', 'number', '1'], {'interface': 'eth1'}]} self.hook.before_processing(introspection_data) self.hook.before_update(introspection_data, self.node_info) + self.assertNotIn('extra', introspection_data) + self.assertIn('data', introspection_data) + self.assertTrue(mock_warn.called) + def test_data_not_in_edeploy_format_strict_mode(self, mock_warn): + CONF.set_override('strict', True, group='extra_hardware') + introspection_data = { + 'data': [['memory', 'total', 'size', '4294967296'], + ['cpu', 'physical', 'WUT']] + } + self.hook.before_processing(introspection_data) + self.hook.before_update(introspection_data, self.node_info) + self.assertNotIn('extra', introspection_data) self.assertNotIn('data', introspection_data) + self.assertTrue(mock_warn.called) - def test_no_data_recieved(self): + def test_no_data_recieved(self, mock_warn): introspection_data = {'cats': 'meow'} self.hook.before_processing(introspection_data) self.hook.before_update(introspection_data, self.node_info) - - def test__convert_edeploy_data(self): - introspection_data = [['Sheldon', 'J.', 'Plankton', '123'], - ['Larry', 'the', 'Lobster', None], - ['Eugene', 'H.', 'Krabs', 'The cashier']] - - data = self.hook._convert_edeploy_data(introspection_data) - expected_data = {'Sheldon': {'J.': {'Plankton': 123}}, - 'Larry': {'the': {'Lobster': None}}, - 'Eugene': {'H.': {'Krabs': 'The cashier'}}} - self.assertEqual(expected_data, data) + self.assertTrue(mock_warn.called) diff --git a/releasenotes/notes/extra-check-9cf0a7d89e534ccd.yaml b/releasenotes/notes/extra-check-9cf0a7d89e534ccd.yaml new file mode 100644 index 000000000..1cb5557cf --- /dev/null +++ b/releasenotes/notes/extra-check-9cf0a7d89e534ccd.yaml @@ -0,0 +1,16 @@ +--- +fixes: + - | + The ``extra_hardware`` processing hook no longer refuses to parse extra + data if some records are empty or have unexpected length. These records + are now discared. + + The previous behavior can be returned by setting the new option + ``[extra_hardware]strict`` to ``True``. + - | + The ``extra_hardware`` processing hook no longer removes the incoming + ``data`` object if it has unexpected data format, assuming that this + object is used for something else. + + The previous behavior can be returned by setting the new option + ``[extra_hardware]strict`` to ``True``.