Merge "Disable DHCP completely when no nodes are on introspection"
This commit is contained in:
commit
d99e0c72e8
|
@ -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
|
||||
|
|
|
@ -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).
|
||||
|
|
|
@ -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')
|
||||
|
|
|
@ -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.
|
Loading…
Reference in New Issue