ovs-fw: Remove iptables rules on hybrid ports
ovs-firewall now scans ports on its bridge and stores those that have
prefix 'qvo', which means such ports use hybrid plugging. Because
ovs-agent makes a full-sync when it's started, all ports that reside on
the node are passed to firewall driver to refresh firewall, a new helper
was added.
In case the initial scan noticed hybrid plugged, an iptables firewall
driver is instantiated and each port is passed down to helper that
removes iptables rules for given port.
Once all ports are processed, a mark is added to ovsdb to avoid cleaning
iptables in the future. That means next time ovs-agent is started
iptables firewall will not be instantiated.
NOTE: Fullstack tests are a great candidate to cover the migration but
I'll leave it as TODO after we stabilize fullstack tests.
Closes-bug: #1721895
Change-Id: I662c310133a089bf29b734c539e57a8cff923074
This commit is contained in:
Jakub Libosvar
parent
779a8d31e7
commit
9d74de162a
|
|
@ -23,6 +23,7 @@ from oslo_utils import netutils
|
|||
from neutron.agent import firewall
|
||||
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 iptables
|
||||
from neutron.agent.linux.openvswitch_firewall import rules
|
||||
from neutron.common import constants
|
||||
from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants \
|
||||
|
|
@ -386,6 +387,9 @@ class OVSFirewallDriver(firewall.FirewallDriver):
|
|||
self._drop_all_unmatched_flows()
|
||||
self.conj_ip_manager = ConjIPFlowManager(self)
|
||||
|
||||
self.iptables_helper = iptables.Helper(self.int_br.br)
|
||||
self.iptables_helper.load_driver_if_needed()
|
||||
|
||||
def security_group_updated(self, action_type, sec_group_ids,
|
||||
device_ids=None):
|
||||
"""The current driver doesn't make use of this method.
|
||||
|
|
@ -470,6 +474,7 @@ class OVSFirewallDriver(firewall.FirewallDriver):
|
|||
return port['device'] in self.sg_port_map.ports
|
||||
|
||||
def prepare_port_filter(self, port):
|
||||
self.iptables_helper.cleanup_port(port)
|
||||
if not firewall.port_sec_enabled(port):
|
||||
self._initialize_egress_no_port_security(port['device'])
|
||||
return
|
||||
|
|
|
|||
|
|
@ -0,0 +1,96 @@
|
|||
# Copyright 2017 Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# 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 neutron.common import constants as n_const
|
||||
|
||||
|
||||
def get_device_port_name(port_id):
|
||||
return ('qvo' + port_id)[:n_const.LINUX_DEV_LEN]
|
||||
|
||||
|
||||
def get_iptables_driver_instance():
|
||||
"""Load hybrid iptables firewall driver."""
|
||||
from neutron.agent.linux import iptables_firewall
|
||||
|
||||
class HybridIptablesHelper(
|
||||
iptables_firewall.OVSHybridIptablesFirewallDriver):
|
||||
"""Don't remove conntrack when removing iptables rules."""
|
||||
def _remove_conntrack_entries_from_port_deleted(port):
|
||||
pass
|
||||
|
||||
return HybridIptablesHelper()
|
||||
|
||||
|
||||
class Helper(object):
|
||||
"""Helper to avoid loading firewall driver.
|
||||
|
||||
The main purpose is to avoid loading iptables driver for cases where no
|
||||
ports have hybrid plugging on given node.
|
||||
|
||||
The helper stores metadata for iptables cleanup into br-int ovsdb Bridge
|
||||
table. Specifically it checks for other_config['iptables_cleaned'] boolean
|
||||
value.
|
||||
"""
|
||||
HYBRID_PORT_PREFIX = 'qvo'
|
||||
CLEANED_METADATA = 'iptables_cleaned'
|
||||
|
||||
def __init__(self, int_br):
|
||||
self.int_br = int_br
|
||||
self.hybrid_ports = None
|
||||
self.iptables_driver = None
|
||||
|
||||
def load_driver_if_needed(self):
|
||||
self.hybrid_ports = self.get_hybrid_ports()
|
||||
if self.hybrid_ports and self.has_not_been_cleaned:
|
||||
self.iptables_driver = get_iptables_driver_instance()
|
||||
|
||||
def get_hybrid_ports(self):
|
||||
"""Return True if there is a port with hybrid plugging."""
|
||||
return {
|
||||
port_name for port_name in self.int_br.get_port_name_list()
|
||||
if port_name.startswith(self.HYBRID_PORT_PREFIX)}
|
||||
|
||||
def cleanup_port(self, port):
|
||||
if not self.iptables_driver:
|
||||
return
|
||||
device_name = get_device_port_name(port['device'])
|
||||
try:
|
||||
self.hybrid_ports.remove(device_name)
|
||||
except KeyError:
|
||||
# It's not a hybrid plugged port
|
||||
return
|
||||
|
||||
# TODO(jlibosva): Optimize, add port to firewall without installing
|
||||
# iptables rules and then call remove from firewall
|
||||
self.iptables_driver.prepare_port_filter(port)
|
||||
self.iptables_driver.remove_port_filter(port)
|
||||
if not self.hybrid_ports:
|
||||
self.mark_as_cleaned()
|
||||
# Let GC remove iptables driver
|
||||
self.iptables_driver = None
|
||||
|
||||
@property
|
||||
def has_not_been_cleaned(self):
|
||||
other_config = self.int_br.db_get_val(
|
||||
'Bridge', self.int_br.br_name, 'other_config')
|
||||
return other_config.get(self.CLEANED_METADATA, '').lower() != 'true'
|
||||
|
||||
def mark_as_cleaned(self):
|
||||
# TODO(jlibosva): Make it a single transaction
|
||||
other_config = self.int_br.db_get_val(
|
||||
'Bridge', self.int_br.br_name, 'other_config')
|
||||
other_config[self.CLEANED_METADATA] = 'true'
|
||||
self.int_br.set_db_attribute(
|
||||
'Bridge', self.int_br.br_name, 'other_config', other_config)
|
||||
|
|
@ -0,0 +1,102 @@
|
|||
# Copyright 2017 Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# 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 neutron_lib import constants
|
||||
from oslo_utils import uuidutils
|
||||
|
||||
from neutron.agent import firewall
|
||||
from neutron.agent.linux import iptables_firewall
|
||||
import neutron.agent.linux.openvswitch_firewall.firewall as ovs_fw_mod
|
||||
import neutron.agent.linux.openvswitch_firewall.iptables as iptables_helper
|
||||
from neutron.tests.common import conn_testers
|
||||
from neutron.tests.common import net_helpers
|
||||
from neutron.tests.functional.agent import test_firewall
|
||||
from neutron.tests.functional import base
|
||||
|
||||
|
||||
class TestHelper(base.BaseSudoTestCase):
|
||||
def setUp(self):
|
||||
super(TestHelper, self).setUp()
|
||||
self.bridge = self.useFixture(net_helpers.OVSBridgeFixture()).bridge
|
||||
self.namespace = self.useFixture(net_helpers.NamespaceFixture()).name
|
||||
self.iptables_firewall = (
|
||||
iptables_firewall.OVSHybridIptablesFirewallDriver(self.namespace))
|
||||
|
||||
def add_sg_rules(self, port, security_group_rules):
|
||||
"""Add security group rules to given port.
|
||||
|
||||
Method creates a security group for isolated firewall use. Adds passed
|
||||
rules to it and calls to prepare_port_filter() to the firewall driver.
|
||||
Method returns port description.
|
||||
"""
|
||||
sg_id = uuidutils.generate_uuid()
|
||||
self.iptables_firewall.update_security_group_rules(
|
||||
sg_id, security_group_rules)
|
||||
description = {
|
||||
'admin_state_up': True,
|
||||
'device': port.port_id,
|
||||
'device_owner': test_firewall.DEVICE_OWNER_COMPUTE,
|
||||
'fixed_ips': ['192.168.0.1'],
|
||||
'mac_address': port.port.link.address,
|
||||
'port_security_enabled': True,
|
||||
'security_groups': [sg_id],
|
||||
'status': 'ACTIVE',
|
||||
'network_id': uuidutils.generate_uuid()}
|
||||
|
||||
self.iptables_firewall.prepare_port_filter(description)
|
||||
|
||||
return description
|
||||
|
||||
def _set_vlan_tag_on_port(self, port, tag):
|
||||
qvo_dev_name = iptables_helper.get_device_port_name(port.port_id)
|
||||
conn_testers.OVSBaseConnectionTester.set_tag(
|
||||
qvo_dev_name, self.bridge, tag)
|
||||
|
||||
def _prepare_port_and_description(self, security_group_rules):
|
||||
hybrid_port = self.useFixture(
|
||||
net_helpers.OVSPortFixture(
|
||||
self.bridge, self.namespace, hybrid_plug=True))
|
||||
self._set_vlan_tag_on_port(hybrid_port, 1)
|
||||
description = self.add_sg_rules(hybrid_port, security_group_rules)
|
||||
|
||||
return hybrid_port, description
|
||||
|
||||
def _check_no_iptables_rules_for_port(self, port):
|
||||
tap_name = self.iptables_firewall._get_device_name(
|
||||
{'device': port.port_id})
|
||||
iptables_rules = (
|
||||
self.iptables_firewall.iptables.get_rules_for_table('filter'))
|
||||
for line in iptables_rules:
|
||||
if tap_name in line:
|
||||
raise Exception("port %s still has iptables rules in %s" % (
|
||||
tap_name, line))
|
||||
|
||||
def test_migration(self):
|
||||
sg_rules = [{'ethertype': constants.IPv4,
|
||||
'direction': firewall.INGRESS_DIRECTION,
|
||||
'protocol': constants.PROTO_NAME_ICMP},
|
||||
{'ethertype': constants.IPv4,
|
||||
'direction': firewall.EGRESS_DIRECTION}]
|
||||
port, desc = self._prepare_port_and_description(sg_rules)
|
||||
ovs_firewall = ovs_fw_mod.OVSFirewallDriver(self.bridge)
|
||||
# Check that iptables driver was set and replace it with the one that
|
||||
# has access to namespace
|
||||
if isinstance(
|
||||
ovs_firewall.iptables_helper.iptables_driver,
|
||||
iptables_firewall.OVSHybridIptablesFirewallDriver):
|
||||
ovs_firewall.iptables_helper.iptables_driver = (
|
||||
self.iptables_firewall)
|
||||
ovs_firewall.prepare_port_filter(desc)
|
||||
self._check_no_iptables_rules_for_port(port)
|
||||
|
|
@ -0,0 +1,102 @@
|
|||
# Copyright 2017 Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# 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.
|
||||
|
||||
import mock
|
||||
|
||||
from neutron.agent.linux import iptables_firewall
|
||||
from neutron.agent.linux.openvswitch_firewall import iptables
|
||||
from neutron.tests import base
|
||||
|
||||
|
||||
class TestHelper(base.BaseTestCase):
|
||||
def setUp(self):
|
||||
super(TestHelper, self).setUp()
|
||||
self.helper = iptables.Helper(mock.Mock())
|
||||
mock.patch.object(iptables_firewall, 'cfg').start()
|
||||
mock.patch('neutron.agent.linux.ip_conntrack.get_conntrack').start()
|
||||
|
||||
def test_get_hybrid_ports(self):
|
||||
present_ports = ['tap1234', 'qvo-1234', 'tap9876', 'qvo-fghfhfh']
|
||||
self.helper.int_br.get_port_name_list.return_value = present_ports
|
||||
expected_hybrid_ports = ['qvo-1234', 'qvo-fghfhfh']
|
||||
observed = self.helper.get_hybrid_ports()
|
||||
self.assertItemsEqual(expected_hybrid_ports, observed)
|
||||
|
||||
def test_has_not_been_cleaned_no_value(self):
|
||||
other_config = {'foo': 'bar'}
|
||||
self.helper.int_br.db_get_val.return_value = other_config
|
||||
self.assertTrue(self.helper.has_not_been_cleaned)
|
||||
|
||||
def test_has_not_been_cleaned_true(self):
|
||||
other_config = {'foo': 'bar', iptables.Helper.CLEANED_METADATA: 'true'}
|
||||
self.helper.int_br.db_get_val.return_value = other_config
|
||||
self.assertFalse(self.helper.has_not_been_cleaned)
|
||||
|
||||
def test_has_not_been_cleaned_false(self):
|
||||
other_config = {'foo': 'bar',
|
||||
iptables.Helper.CLEANED_METADATA: 'false'}
|
||||
self.helper.int_br.db_get_val.return_value = other_config
|
||||
self.assertTrue(self.helper.has_not_been_cleaned)
|
||||
|
||||
def test_load_driver_if_needed_no_hybrid_ports(self):
|
||||
self.helper.int_br.get_port_name_list.return_value = [
|
||||
'tap1234', 'tap9876']
|
||||
self.helper.load_driver_if_needed()
|
||||
self.assertIsNone(self.helper.iptables_driver)
|
||||
|
||||
def test_load_driver_if_needed_hybrid_ports_cleaned(self):
|
||||
"""If was cleaned, driver shouldn't be loaded."""
|
||||
self.helper.int_br.get_port_name_list.return_value = [
|
||||
'tap1234', 'qvo-1234', 'tap9876', 'qvo-fghfhfh']
|
||||
self.helper.int_br.db_get_val.return_value = {
|
||||
'foo': 'bar', iptables.Helper.CLEANED_METADATA: 'true'}
|
||||
self.helper.load_driver_if_needed()
|
||||
self.assertIsNone(self.helper.iptables_driver)
|
||||
|
||||
def test_load_driver_if_needed_hybrid_ports_not_cleaned(self):
|
||||
"""If hasn't been cleaned, driver should be loaded."""
|
||||
self.helper.int_br.get_port_name_list.return_value = [
|
||||
'tap1234', 'qvo-1234', 'tap9876', 'qvo-fghfhfh']
|
||||
self.helper.int_br.db_get_val.return_value = {'foo': 'bar'}
|
||||
self.helper.load_driver_if_needed()
|
||||
self.assertIsNotNone(self.helper.iptables_driver)
|
||||
|
||||
def test_get_iptables_driver_instance_has_correct_instance(self):
|
||||
instance = iptables.get_iptables_driver_instance()
|
||||
self.assertIsInstance(
|
||||
instance,
|
||||
iptables_firewall.OVSHybridIptablesFirewallDriver)
|
||||
|
||||
def test_cleanup_port_last_port_marks_cleaned(self):
|
||||
self.helper.iptables_driver = mock.Mock()
|
||||
self.helper.hybrid_ports = {'qvoport'}
|
||||
with mock.patch.object(self.helper, 'mark_as_cleaned') as mock_mark:
|
||||
self.helper.cleanup_port({'device': 'port'})
|
||||
self.assertIsNone(self.helper.iptables_driver)
|
||||
self.assertTrue(mock_mark.called)
|
||||
|
||||
def test_cleanup_port_existing_ports(self):
|
||||
self.helper.iptables_driver = mock.Mock()
|
||||
self.helper.hybrid_ports = {'qvoport', 'qvoanother'}
|
||||
with mock.patch.object(self.helper, 'mark_as_cleaned') as mock_mark:
|
||||
self.helper.cleanup_port({'device': 'port'})
|
||||
self.assertIsNotNone(self.helper.iptables_driver)
|
||||
self.assertFalse(mock_mark.called)
|
||||
|
||||
def test_cleanup_port_unknown(self):
|
||||
self.helper.iptables_driver = mock.Mock()
|
||||
self.helper.hybrid_ports = {'qvoanother'}
|
||||
self.helper.cleanup_port({'device': 'port'})
|
||||
self.assertFalse(self.helper.iptables_driver.remove_port_filter.called)
|
||||
Loading…
Reference in New Issue