openstack/deb-ironic-inspector
Code Issues Proposed changes

Disable DHCP completely when no nodes are on introspection

Browse Source 
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
This commit is contained in:
Dmitry Tantsur 2016-03-16 11:32:22 +01:00
parent 5fa5b4a8ec
commit 405c7de1f8
4 changed files with 162 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

83
ironic_inspector/firewall.py
View File

@ -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):
@ -101,6 +103,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.
@ -119,7 +168,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
@ -128,6 +177,10 @@ def update_filters(ironic=None):
ironic = ir_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:
@ -139,26 +192,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

7
ironic_inspector/node_cache.py
View File

@ -417,6 +417,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).

86
ironic_inspector/test/test_firewall.py
View File

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

7
releasenotes/notes/disable-dhcp-c86a3a0ee2696ee0.yaml Normal file
View File

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