openstack/ironic-inspector
Code Issues Proposed changes

Relax extra_hardware data validation by default

Browse Source 
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
This commit is contained in:
Dmitry Tantsur 2020-07-28 11:56:35 +02:00
parent 9ecb6e33eb
commit 3243cac3fc
6 changed files with 151 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
ironic_inspector/conf/__init__.py
View File

@ -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)

33
ironic_inspector/conf/extra_hardware.py Normal file
View File

@ -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

3
ironic_inspector/conf/opts.py
View File

@ -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()),
]

58
ironic_inspector/plugins/extra_hardware.py
View File

@ -18,11 +18,14 @@ string in a Swift object. The object is named 'extra_hardware-<node uuid>' 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,36 +45,31 @@ 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)
LOG.debug('Deleting \"data\" key from introspection data as it is '
'assumed unusable by introspection rules. Raw data is '
'stored in swift',
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:
if not item:
continue
try:
converted_0 = converted.setdefault(item[0], {})
converted_1 = converted_0.setdefault(item[1], {})
@ -81,4 +79,20 @@ class ExtraHardwareHook(base.ProcessingHook):
pass
converted_1[item[2]] = item[3]
return converted
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.',
node_info=node_info, data=introspection_data)
del introspection_data['data']
def _is_edeploy_data(self, data):
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)

70
ironic_inspector/test/unit/test_plugins_extra_hardware.py
View File

@ -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)

16
releasenotes/notes/extra-check-9cf0a7d89e534ccd.yaml Normal file
View File

@ -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``.