openstack
/
neutron
Code Issues Proposed changes

ovs-fw: Fix firewall blink 

Previously, when security group was updated for given port, the firewall
removed all flows related to the port and added new rules. That
introduced a time window where there were no rules for the port.

This patch adds a new mechanism using cookie that can be described in
three states:

1) Create new openflow rules with non-default cookie that is considered
an updated cookie. All newly generated flows will be added with the next
cookie and all existing rules with default cookie are rewritten with the
default cookie.
2) Delete all rules for given port with the old default cookie. This
will leave the newly added rules in place.
3) Update the newly added flows with update cookie back to the default
cookie in order to avoid such flows being cleaned on the next restart of
ovs agent, as it fetches for stale flows.

Change-Id: I85d9e49c24ee7c91229b43cd329c42149637f254
Closes-bug: #1708731
This commit is contained in:
Jakub Libosvar 2018-02-05 17:20:09 +00:00
parent ecc60df945
commit 6f7ba76075
3 changed files with 89 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
neutron/agent/linux/openvswitch_firewall/firewall.py
View File

@ -14,6 +14,7 @@
# under the License.
import collections
import contextlib
import copy
import netaddr
@ -391,6 +392,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
"""
self.int_br = self.initialize_bridge(integration_bridge)
self._update_cookie = None
self.sg_port_map = SGPortMap()
self.sg_to_delete = set()
self._deferred = False
@ -401,6 +403,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
self.iptables_helper = iptables.Helper(self.int_br.br)
self.iptables_helper.load_driver_if_needed()
@contextlib.contextmanager
def update_cookie_context(self):
try:
self._update_cookie = self.int_br.br.request_cookie()
yield
finally:
self.int_br.br.unset_cookie(self._update_cookie)
self._update_cookie = None
def security_group_updated(self, action_type, sec_group_ids,
device_ids=None):
"""The current driver doesn't make use of this method.
@ -418,6 +429,8 @@ class OVSFirewallDriver(firewall.FirewallDriver):
create_reg_numbers(kwargs)
if isinstance(dl_type, int):
kwargs['dl_type'] = "0x{:04x}".format(dl_type)
if self._update_cookie:
kwargs['cookie'] = self._update_cookie
if self._deferred:
self.int_br.add_flow(**kwargs)
else:
@ -499,7 +512,15 @@ class OVSFirewallDriver(firewall.FirewallDriver):
if not firewall.port_sec_enabled(port):
self._initialize_egress_no_port_security(port['device'])
return
self._set_port_filters(port, old_port_expected=False)
old_of_port = self.get_ofport(port)
of_port = self.get_or_create_ofport(port)
if old_of_port:
LOG.info("Initializing port %s that was already initialized.",
port['device'])
self._update_flows_for_port(of_port, old_of_port)
else:
self._set_port_filters(of_port)
def update_port_filter(self, port):
"""Update rules for given port
@ -521,27 +542,32 @@ class OVSFirewallDriver(firewall.FirewallDriver):
self.prepare_port_filter(port)
return
try:
self._set_port_filters(port, old_port_expected=True)
# Make sure delete old allowed_address_pair MACs because
# allowed_address_pair MACs will be updated in
# self.get_or_create_ofport(port)
old_of_port = self.get_ofport(port)
of_port = self.get_or_create_ofport(port)
if old_of_port:
self._update_flows_for_port(of_port, old_of_port)
else:
self._set_port_filters(of_port)
except exceptions.OVSFWPortNotFound as not_found_error:
LOG.info("port %(port_id)s does not exist in ovsdb: %(err)s.",
{'port_id': port['device'],
'err': not_found_error})
def _set_port_filters(self, port, old_port_expected):
old_of_port = self.get_ofport(port)
# Make sure delete old allowed_address_pair MACs because
# allowed_address_pair MACs will be updated in
# self.get_or_create_ofport(port)
if old_of_port:
if not old_port_expected:
LOG.info("Initializing port %s that was already "
"initialized.", port['device'])
self.delete_all_port_flows(old_of_port)
# TODO(jlibosva): Handle firewall blink
of_port = self.get_or_create_ofport(port)
def _set_port_filters(self, of_port):
self.initialize_port_flows(of_port)
self.add_flows_from_rules(of_port)
def _update_flows_for_port(self, of_port, old_of_port):
with self.update_cookie_context():
self._set_port_filters(of_port)
self.delete_all_port_flows(old_of_port)
# Rewrite update cookie with default cookie
self._set_port_filters(of_port)
def remove_port_filter(self, port):
"""Remove port from firewall

1
neutron/tests/functional/agent/test_firewall.py
View File

@ -558,7 +558,6 @@ class FirewallTestCase(BaseFirewallTestCase):
self._apply_security_group_rules(self.FAKE_SECURITY_GROUP_ID, list())
self.tester.assert_no_established_connection(**connection)
@skip_if_firewall('openvswitch')
def test_preventing_firewall_blink(self):
direction = self.tester.INGRESS
sg_rules = [{'ethertype': 'IPv4', 'direction': 'ingress',

49
neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
View File

@ -17,12 +17,15 @@ from neutron_lib import constants
import testtools
from neutron.agent.common import ovs_lib
from neutron.agent.common import utils
from neutron.agent.linux.openvswitch_firewall import constants as ovsfw_consts
from neutron.agent.linux.openvswitch_firewall import exceptions
from neutron.agent.linux.openvswitch_firewall import firewall as ovsfw
from neutron.common import constants as n_const
from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants \
as ovs_consts
from neutron.plugins.ml2.drivers.openvswitch.agent.openflow.ovs_ofctl \
import ovs_bridge
from neutron.tests import base
TESTING_VLAN_TAG = 1
@ -715,3 +718,49 @@ class TestOVSFirewallDriver(base.BaseTestCase):
def test_remove_trusted_ports_not_managed_port(self):
"""Check that exception is not propagated outside."""
self.firewall.remove_trusted_ports(['port_id'])
class TestCookieContext(base.BaseTestCase):
def setUp(self):
super(TestCookieContext, self).setUp()
# Don't attempt to connect to ovsdb
mock.patch('neutron.agent.ovsdb.api.from_config').start()
# Don't trigger iptables -> ovsfw migration
mock.patch(
'neutron.agent.linux.openvswitch_firewall.iptables.Helper').start()
self.execute = mock.patch.object(
utils, "execute", spec=utils.execute).start()
bridge = ovs_bridge.OVSAgentBridge('foo')
mock.patch.object(
ovsfw.OVSFirewallDriver, 'initialize_bridge',
return_value=bridge.deferred(
full_ordered=True, use_bundle=True)).start()
self.firewall = ovsfw.OVSFirewallDriver(bridge)
# Remove calls from firewall initialization
self.execute.reset_mock()
def test_cookie_is_different_in_context(self):
default_cookie = self.firewall.int_br.br.default_cookie
with self.firewall.update_cookie_context():
self.firewall._add_flow(actions='drop')
update_cookie = self.firewall._update_cookie
self.firewall._add_flow(actions='drop')
expected_calls = [
mock.call(
mock.ANY,
process_input='hard_timeout=0,idle_timeout=0,priority=1,'
'cookie=%d,actions=drop' % cookie,
run_as_root=mock.ANY,
) for cookie in (update_cookie, default_cookie)
]
self.execute.assert_has_calls(expected_calls)
def test_context_cookie_is_not_left_as_used(self):
with self.firewall.update_cookie_context():
update_cookie = self.firewall._update_cookie
self.assertNotIn(
update_cookie,
self.firewall.int_br.br._reserved_cookies)