From 39eb964134f2ff13288ac117305ae226267c2e47 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 16 Mar 2016 11:32:22 +0100 Subject: [PATCH] Disable DHCP completely when no nodes are on introspection Currently we keep DHCP always open for new nodes. This is an overkill, as we always know which nodes are on introspection. It also causes problems when not all node NIC's are registered in Ironic, as these NIC's might get DHCP from our server. This change reduces probability of wrong nodes accessing our DHCP by REJECT'ing all DHCP requests when no nodes are on introspection and node_not_found_hook is not set. It does not solve the problem completely: conflicts are still possible during the introspection. Change-Id: I7a50c02023ef4364e14825cd80fa75565fac3dc8 Partial-Bug: #1557979 (cherry picked from commit 405c7de1f8671eb5da7eb5dbf996f519b70b32d9) --- ironic_inspector/firewall.py | 83 +++++++++++++----- ironic_inspector/node_cache.py | 7 ++ ironic_inspector/test/test_firewall.py | 86 +++++++++++++++++++ .../notes/disable-dhcp-c86a3a0ee2696ee0.yaml | 7 ++ 4 files changed, 162 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/disable-dhcp-c86a3a0ee2696ee0.yaml diff --git a/ironic_inspector/firewall.py b/ironic_inspector/firewall.py index 452c891e9..50b172aa4 100644 --- a/ironic_inspector/firewall.py +++ b/ironic_inspector/firewall.py @@ -11,6 +11,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import contextlib import os import subprocess @@ -31,6 +32,7 @@ INTERFACE = None LOCK = semaphore.BoundedSemaphore() BASE_COMMAND = None BLACKLIST_CACHE = None +ENABLED = True def _iptables(*args, **kwargs): @@ -100,6 +102,53 @@ def clean_up(): _clean_up(NEW_CHAIN) +def _should_enable_dhcp(): + """Check whether we should enable DHCP at all. + + We won't even open our DHCP if no nodes are on introspection and + node_not_found_hook is not set. + """ + return (node_cache.introspection_active() or + CONF.processing.node_not_found_hook) + + +@contextlib.contextmanager +def _temporary_chain(chain, main_chain): + """Context manager to operate on a temporary chain.""" + # Clean up a bit to account for possible troubles on previous run + _clean_up(chain) + _iptables('-N', chain) + + yield + + # Swap chains + _iptables('-I', 'INPUT', '-i', INTERFACE, '-p', 'udp', + '--dport', '67', '-j', chain) + _iptables('-D', 'INPUT', '-i', INTERFACE, '-p', 'udp', + '--dport', '67', '-j', main_chain, + ignore=True) + _iptables('-F', main_chain, ignore=True) + _iptables('-X', main_chain, ignore=True) + _iptables('-E', chain, main_chain) + + +def _disable_dhcp(): + """Disable DHCP completely.""" + global ENABLED + + if not ENABLED: + LOG.debug('DHCP is already disabled, not updating') + return + + LOG.debug('No nodes on introspection and node_not_found_hook is ' + 'not set - disabling DHCP') + with _temporary_chain(NEW_CHAIN, CHAIN): + # Blacklist everything + _iptables('-A', NEW_CHAIN, '-j', 'REJECT') + + ENABLED = False + + def update_filters(ironic=None): """Update firewall filter rules for introspection. @@ -118,7 +167,7 @@ def update_filters(ironic=None): :param ironic: Ironic client instance, optional. """ - global BLACKLIST_CACHE + global BLACKLIST_CACHE, ENABLED if not CONF.firewall.manage_firewall: return @@ -127,6 +176,10 @@ def update_filters(ironic=None): ironic = utils.get_client() if ironic is None else ironic with LOCK: + if not _should_enable_dhcp(): + _disable_dhcp() + return + macs_active = set(p.address for p in ironic.port.list(limit=0)) to_blacklist = macs_active - node_cache.active_macs() if BLACKLIST_CACHE is not None and to_blacklist == BLACKLIST_CACHE: @@ -138,26 +191,14 @@ def update_filters(ironic=None): # Force update on the next iteration if this attempt fails BLACKLIST_CACHE = None - # Clean up a bit to account for possible troubles on previous run - _clean_up(NEW_CHAIN) - # Operate on temporary chain - _iptables('-N', NEW_CHAIN) - # - Blacklist active macs, so that nova can boot them - for mac in to_blacklist: - _iptables('-A', NEW_CHAIN, '-m', 'mac', - '--mac-source', mac, '-j', 'DROP') - # - Whitelist everything else - _iptables('-A', NEW_CHAIN, '-j', 'ACCEPT') - - # Swap chains - _iptables('-I', 'INPUT', '-i', INTERFACE, '-p', 'udp', - '--dport', '67', '-j', NEW_CHAIN) - _iptables('-D', 'INPUT', '-i', INTERFACE, '-p', 'udp', - '--dport', '67', '-j', CHAIN, - ignore=True) - _iptables('-F', CHAIN, ignore=True) - _iptables('-X', CHAIN, ignore=True) - _iptables('-E', NEW_CHAIN, CHAIN) + with _temporary_chain(NEW_CHAIN, CHAIN): + # - Blacklist active macs, so that nova can boot them + for mac in to_blacklist: + _iptables('-A', NEW_CHAIN, '-m', 'mac', + '--mac-source', mac, '-j', 'DROP') + # - Whitelist everything else + _iptables('-A', NEW_CHAIN, '-j', 'ACCEPT') # Cache result of successful iptables update + ENABLED = True BLACKLIST_CACHE = to_blacklist diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py index 4dbeaf895..d1fc6007c 100644 --- a/ironic_inspector/node_cache.py +++ b/ironic_inspector/node_cache.py @@ -345,6 +345,13 @@ def _delete_node(uuid, session=None): session=session).filter_by(uuid=uuid).delete() +def introspection_active(): + """Check if introspection is active for at least one node.""" + # FIXME(dtantsur): is there a better way to express it? + return (db.model_query(db.Node.uuid).filter_by(finished_at=None).first() + is not None) + + def active_macs(): """List all MAC's that are on introspection right now.""" return ({x.value for x in db.model_query(db.Attribute.value). diff --git a/ironic_inspector/test/test_firewall.py b/ironic_inspector/test/test_firewall.py index 2f7aaf6e9..cbe85e067 100644 --- a/ironic_inspector/test/test_firewall.py +++ b/ironic_inspector/test/test_firewall.py @@ -94,6 +94,9 @@ class TestFirewall(test_base.NodeTest): def test_update_filters_args(self, mock_call, mock_get_client, mock_iptables): + # Pretend that we have nodes on introspection + node_cache.add_node(self.node.uuid, bmc_address='1.2.3.4') + firewall.init() update_filters_expected_args = [ @@ -244,3 +247,86 @@ class TestFirewall(test_base.NodeTest): for (args, call) in zip(update_filters_expected_args, call_args_list): self.assertEqual(args, call[0]) + + def test_update_filters_args_node_not_found_hook(self, mock_call, + mock_get_client, + mock_iptables): + # DHCP should be always opened if node_not_found hook is set + CONF.set_override('node_not_found_hook', 'enroll', 'processing') + + firewall.init() + + update_filters_expected_args = [ + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', CONF.firewall.firewall_chain), + ('-F', CONF.firewall.firewall_chain), + ('-X', CONF.firewall.firewall_chain), + ('-N', CONF.firewall.firewall_chain), + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', firewall.NEW_CHAIN), + ('-F', firewall.NEW_CHAIN), + ('-X', firewall.NEW_CHAIN), + ('-N', firewall.NEW_CHAIN), + ('-A', firewall.NEW_CHAIN, '-j', 'ACCEPT'), + ('-I', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', firewall.NEW_CHAIN), + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', CONF.firewall.firewall_chain), + ('-F', CONF.firewall.firewall_chain), + ('-X', CONF.firewall.firewall_chain), + ('-E', firewall.NEW_CHAIN, CONF.firewall.firewall_chain) + ] + + firewall.update_filters() + call_args_list = mock_iptables.call_args_list + + for (args, call) in zip(update_filters_expected_args, + call_args_list): + self.assertEqual(args, call[0]) + + def test_update_filters_args_no_introspection(self, mock_call, + mock_get_client, + mock_iptables): + firewall.init() + + update_filters_expected_args = [ + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', CONF.firewall.firewall_chain), + ('-F', CONF.firewall.firewall_chain), + ('-X', CONF.firewall.firewall_chain), + ('-N', CONF.firewall.firewall_chain), + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', firewall.NEW_CHAIN), + ('-F', firewall.NEW_CHAIN), + ('-X', firewall.NEW_CHAIN), + ('-N', firewall.NEW_CHAIN), + ('-A', firewall.NEW_CHAIN, '-j', 'REJECT'), + ('-I', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', firewall.NEW_CHAIN), + ('-D', 'INPUT', '-i', 'br-ctlplane', '-p', 'udp', '--dport', + '67', '-j', CONF.firewall.firewall_chain), + ('-F', CONF.firewall.firewall_chain), + ('-X', CONF.firewall.firewall_chain), + ('-E', firewall.NEW_CHAIN, CONF.firewall.firewall_chain) + ] + + firewall.update_filters() + call_args_list = mock_iptables.call_args_list + + for (args, call) in zip(update_filters_expected_args, + call_args_list): + self.assertEqual(args, call[0]) + + # Check caching enabled flag + + mock_iptables.reset_mock() + firewall.update_filters() + self.assertFalse(mock_iptables.called) + + # Adding a node changes it back + + node_cache.add_node(self.node.uuid, bmc_address='1.2.3.4') + mock_iptables.reset_mock() + firewall.update_filters() + + mock_iptables.assert_any_call('-A', firewall.NEW_CHAIN, '-j', 'ACCEPT') diff --git a/releasenotes/notes/disable-dhcp-c86a3a0ee2696ee0.yaml b/releasenotes/notes/disable-dhcp-c86a3a0ee2696ee0.yaml new file mode 100644 index 000000000..0f32fda51 --- /dev/null +++ b/releasenotes/notes/disable-dhcp-c86a3a0ee2696ee0.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - DHCP is now disabled completely when no nodes are on introspection and + the "node_not_found_hook" is not set. This reduces probability of serving + DHCP to wrong nodes, if their NIC is not registered in Ironic. See + https://bugs.launchpad.net/ironic-inspector/+bug/1557979 and + https://bugzilla.redhat.com/show_bug.cgi?id=1317695 for details.