From 192bc5f1a8781111b6b0c2211f4421fafacb27c7 Mon Sep 17 00:00:00 2001 From: IWAMOTO Toshihiro Date: Fri, 24 Jun 2016 17:20:36 +0900 Subject: [PATCH] Use conjunction for security group rules with remote_group_id Prior to this commit, the number of flows can be prohibitively large in some cases. Closes-bug: #1628819 Change-Id: I194e7f40db840d29af317ddc2e342a1409000151 --- doc/source/devref/openvswitch_firewall.rst | 52 +++- .../linux/openvswitch_firewall/firewall.py | 272 ++++++++++++++++-- .../agent/linux/openvswitch_firewall/rules.py | 112 ++++++-- .../openvswitch_firewall/test_firewall.py | 130 ++++++++- .../linux/openvswitch_firewall/test_rules.py | 83 +++++- 5 files changed, 577 insertions(+), 72 deletions(-) diff --git a/doc/source/devref/openvswitch_firewall.rst b/doc/source/devref/openvswitch_firewall.rst index 11a4b184f9e..abcdd74e8fe 100644 --- a/doc/source/devref/openvswitch_firewall.rst +++ b/doc/source/devref/openvswitch_firewall.rst @@ -80,11 +80,39 @@ ingress or egress filtering table, depending on its direction. The reason the rules are based on security group rules in separate tables is to make it easy to detect these rules during removal. -The firewall driver method ``create_rules_generator_for_port`` creates a -generator that builds a single security group rule either from rules belonging -to a given group, or rules allowing connections to remote groups. Every rule is -then expanded into several OpenFlow rules by the method -``create_flows_from_rule_and_port``. +Security group rules are treated differently for those without a +remote group ID and those with a remote group ID. A security group +rule without a remote group ID is expanded into several OpenFlow rules +by the method ``create_flows_from_rule_and_port``. A security group +rule with a remote group ID is expressed by three sets of flows. The +first two are conjunctive flows which will be described in the next +section. The third set matches on the conjunction IDs and does accept +actions. + + +Uses of conjunctive flows +------------------------- + +With a security group rule with a remote group ID, flows that match on +nw_src for remote_group_id addresses and match on dl_dst for port MAC +addresses are needed (for ingress rules; likewise for egress +rules). Without conjunction, this results in O(n*m) flows where n and +m are number of ports in the remote group ID and the port security group, +respectively. + +A conj_id is allocated for each (remote_group_id, security_group_id, +direction, ethertype) tuple. The class ``ConjIdMap`` handles the +mapping. The same conj_id is shared between security group rules if +multiple rules belong to the same tuple above. + +Conjunctive flows consist of 2 dimensions. Flows that belong to the +dimension 1 of 2 are generated by the method +``create_flows_for_ip_address`` and are in charge of IP address based +filtering specified by their remote group IDs. Flows that belong to +the dimension 2 of 2 are generated by the method +``create_flows_from_rule_and_port`` and modified by the method +``substitute_conjunction_actions``, which represents the portion of +the rule other than its remote group ID. Rules example with explanation: @@ -278,12 +306,18 @@ port. Not tracked packets are sent to obtain conntrack information. Similarly to ``table 72``, ``table 82`` accepts established and related connections. In this case we allow all icmp traffic coming from ``security group 1`` which is in this case only ``port 1`` with ip address -``192.168.0.1``. +``192.168.0.1``. The first two rules match on the ip address, and the +next two rules match on the icmp protocol and the destination mac address. +These four rules define conjunction flows. :: - table=82, priority=70,ct_state=+est-rel-rpl,icmp,reg5=0x2,dl_dst=fa:16:3e:24:57:c7,nw_src=192.168.0.1 actions=strip_vlan,output:2 - table=82, priority=70,ct_state=+new-est,icmp,reg5=0x2,dl_dst=fa:16:3e:24:57:c7,nw_src=192.168.0.1 actions=ct(commit,zone=NXM_NX_REG6[0..15]),strip_vlan,output:2 + table=82, priority=70,ct_state=+est-rel-rpl,ip,reg6=0xfff,nw_src=192.168.0.1 actions=conjunction(2147352552,1/2) + table=82, priority=70,ct_state=+new-est,ip,reg6=0xfff,nw_src=192.168.0.1 actions=conjunction(2147352553,1/2) + table=82, priority=70,ct_state=+est-rel-rpl,icmp,reg5=0x2,dl_dst=fa:16:3e:24:57:c7 actions=conjunction(2147352552,2/2) + table=82, priority=70,ct_state=+new-est,icmp,reg5=0x2,dl_dst=fa:16:3e:24:57:c7 actions=conjunction(2147352553,2/2) + table=82, priority=70,conj_id=2147352552,ct_state=+est-rel-rpl,ip,reg5=0x2,dl_dst=fa:16:3e:24:57:c7 actions=strip_vlan,output:2 + table=82, priority=70,conj_id=2147352553,ct_state=+new-est,ip,reg5=0x2,dl_dst=fa:16:3e:24:57:c7 actions=ct(commit,zone=NXM_NX_REG6[0..15]),strip_vlan,output:2 table=82, priority=50,ct_state=+inv+trk actions=drop The mechanism for dropping connections that are not allowed anymore is the @@ -315,8 +349,6 @@ Future work ----------- - Create fullstack tests with tunneling enabled - - Conjunctions in Openflow rules can be created to decrease the number of - rules needed for remote security groups - During the update of firewall rules, we can use bundles to make the changes atomic diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py index c0a0b59ab42..26553691ecb 100644 --- a/neutron/agent/linux/openvswitch_firewall/firewall.py +++ b/neutron/agent/linux/openvswitch_firewall/firewall.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + import netaddr from neutron_lib import constants as lib_const from oslo_log import log as logging @@ -78,6 +80,7 @@ class SecurityGroup(object): self.raw_rules = [] self.remote_rules = [] self.members = {} + self.members_on_flows = {} self.ports = set() def update_rules(self, rules): @@ -87,11 +90,8 @@ class SecurityGroup(object): self.remote_rules = [rule for rule in rules if 'remote_group_id' in rule] - def get_ethertype_filtered_addresses(self, ethertype, - exclude_addresses=None): - exclude_addresses = set(exclude_addresses) or set() - group_addresses = set(self.members.get(ethertype, [])) - return list(group_addresses - exclude_addresses) + def get_ethertype_filtered_addresses(self, ethertype): + return self.members.get(ethertype, []) class OFPort(object): @@ -140,6 +140,9 @@ class SGPortMap(object): self.ports = {} self.sec_groups = {} + def get_sg(self, sg_id): + return self.sec_groups.get(sg_id, None) + def get_or_create_sg(self, sg_id): try: sec_group = self.sec_groups[sg_id] @@ -148,6 +151,9 @@ class SGPortMap(object): self.sec_groups[sg_id] = sec_group return sec_group + def delete_sg(self, sg_id): + del self.sec_groups[sg_id] + def create_port(self, port, port_dict): self.ports[port.id] = port self.update_port(port, port_dict) @@ -176,6 +182,162 @@ class SGPortMap(object): sec_group.members = members +class ConjIdMap(object): + """Handle conjuction ID allocations and deallocations.""" + + def __init__(self): + self.id_map = collections.defaultdict(self._conj_id_factory) + self.id_free = collections.deque() + self.max_id = 0 + + def _conj_id_factory(self): + # If there is any freed ID, use one. + if self.id_free: + return self.id_free.popleft() + # Allocate new one. It must be an even number. + self.max_id += 2 + return self.max_id + + def get_conj_id(self, sg_id, remote_sg_id, direction, ethertype): + """Return a conjunction ID specified by the arguments. + Allocate one if necessary. The returned ID is always an even + number, allowing the caller to use 2 IDs for each combination. + """ + if direction not in [firewall.EGRESS_DIRECTION, + firewall.INGRESS_DIRECTION]: + raise ValueError("Invalid direction '%s'" % direction) + if ethertype not in [lib_const.IPv4, lib_const.IPv6]: + raise ValueError("Invalid ethertype '%s'" % ethertype) + + return self.id_map[(sg_id, remote_sg_id, direction, ethertype)] + + def delete_sg(self, sg_id): + """Free all conj_ids associated with the sg_id and + return a list of (remote_sg_id, conj_id), which are no longer + in use. + """ + result = [] + for k in list(self.id_map.keys()): + if sg_id in k[0:2]: + conj_id = self.id_map.pop(k) + result.append((k[1], conj_id)) + self.id_free.append(conj_id) + + return result + + +class ConjIPFlowManager(object): + """Manage conj_id allocation and remote securitygroups derived + conjunction flows. + + Flows managed by this class is of form: + + nw_src=10.2.3.4,reg_net=0xf00 actions=conjunction(123,1/2) + + These flows are managed per network and are usually per remote_group_id, + but flows from different remote_group need to be merged on shared networks, + where the complexity arises and this manager is needed. + + """ + + def __init__(self, driver): + self.conj_id_map = ConjIdMap() + self.driver = driver + # The following two are dict of dicts and are indexed like: + # self.x[vlan_tag][(direction, ethertype)] + self.conj_ids = collections.defaultdict(dict) + self.flow_state = collections.defaultdict( + lambda: collections.defaultdict(dict)) + + def _build_addr_conj_id_map(self, ethertype, sg_conj_id_map): + """Build a map of addr -> list of conj_ids.""" + addr_to_conj = collections.defaultdict(list) + for remote_id, conj_id_set in sg_conj_id_map.items(): + remote_group = self.driver.sg_port_map.get_sg(remote_id) + if not remote_group: + LOG.debug('No member for SG %s', remote_id) + continue + for addr in remote_group.get_ethertype_filtered_addresses( + ethertype): + addr_to_conj[addr].extend(conj_id_set) + + return addr_to_conj + + def _update_flows_for_vlan_subr(self, direction, ethertype, vlan_tag, + flow_state, addr_to_conj): + """Do the actual flow updates for given direction and ethertype.""" + current_ips = set(flow_state.keys()) + self.driver.delete_flows_for_ip_addresses( + current_ips - set(addr_to_conj.keys()), + direction, ethertype, vlan_tag) + for addr, conj_ids in addr_to_conj.items(): + conj_ids.sort() + if flow_state.get(addr) == conj_ids: + continue + for flow in rules.create_flows_for_ip_address( + addr, direction, ethertype, vlan_tag, conj_ids): + self.driver._add_flow(**flow) + + def update_flows_for_vlan(self, vlan_tag): + """Install action=conjunction(conj_id, 1/2) flows, + which depend on IP addresses of remote_group_id. + """ + for (direction, ethertype), sg_conj_id_map in ( + self.conj_ids[vlan_tag].items()): + # TODO(toshii): optimize when remote_groups have + # no address overlaps. + addr_to_conj = self._build_addr_conj_id_map( + ethertype, sg_conj_id_map) + self._update_flows_for_vlan_subr(direction, ethertype, vlan_tag, + self.flow_state[vlan_tag][(direction, ethertype)], + addr_to_conj) + self.flow_state[vlan_tag][(direction, ethertype)] = addr_to_conj + + def add(self, vlan_tag, sg_id, remote_sg_id, direction, ethertype): + """Get conj_id specified by the arguments + and notify the manager that + (remote_sg_id, direction, ethertype, conj_id) flows need to be + populated on the vlan_tag network. + + A caller must call update_flows_for_vlan to have the change in effect. + + """ + conj_id = self.conj_id_map.get_conj_id( + sg_id, remote_sg_id, direction, ethertype) + + if (direction, ethertype) not in self.conj_ids[vlan_tag]: + self.conj_ids[vlan_tag][(direction, ethertype)] = ( + collections.defaultdict(set)) + self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id].add( + conj_id) + return conj_id + + def sg_removed(self, sg_id): + """Handle SG removal events. + + Free all conj_ids associated with the sg_id and clean up + obsolete entries from the self.conj_ids map. Unlike the add + method, it also updates flows. + """ + id_list = self.conj_id_map.delete_sg(sg_id) + unused_dict = collections.defaultdict(set) + for remote_sg_id, conj_id in id_list: + unused_dict[remote_sg_id].add(conj_id) + + for vlan_tag, vlan_conj_id_map in self.conj_ids.items(): + update = False + for sg_conj_id_map in vlan_conj_id_map.values(): + for remote_sg_id, unused in unused_dict.items(): + if (remote_sg_id in sg_conj_id_map and + sg_conj_id_map[remote_sg_id] & unused): + sg_conj_id_map[remote_sg_id] -= unused + if not sg_conj_id_map[remote_sg_id]: + del sg_conj_id_map[remote_sg_id] + update = True + if update: + self.update_flows_for_vlan(vlan_tag) + + class OVSFirewallDriver(firewall.FirewallDriver): REQUIRED_PROTOCOLS = [ ovs_consts.OPENFLOW10, @@ -196,8 +358,10 @@ class OVSFirewallDriver(firewall.FirewallDriver): """ self.int_br = self.initialize_bridge(integration_bridge) self.sg_port_map = SGPortMap() + self.sg_to_delete = set() self._deferred = False self._drop_all_unmatched_flows() + self.conj_ip_manager = ConjIPFlowManager(self) def security_group_updated(self, action_type, sec_group_ids, device_ids=None): @@ -208,14 +372,8 @@ class OVSFirewallDriver(firewall.FirewallDriver): """ def _accept_flow(self, **flow): - flow['ct_state'] = ovsfw_consts.OF_STATE_ESTABLISHED_NOT_REPLY - self._add_flow(**flow) - flow['ct_state'] = ovsfw_consts.OF_STATE_NEW_NOT_ESTABLISHED - if flow['table'] == ovs_consts.RULES_INGRESS_TABLE: - flow['actions'] = ( - 'ct(commit,zone=NXM_NX_REG{:d}[0..15]),{:s}'.format( - ovsfw_consts.REG_NET, flow['actions'])) - self._add_flow(**flow) + for f in rules.create_accept_flows(flow): + self._add_flow(**f) def _add_flow(self, **kwargs): dl_type = kwargs.get('dl_type') @@ -319,18 +477,47 @@ class OVSFirewallDriver(firewall.FirewallDriver): of_port = self.get_ofport(port) self.delete_all_port_flows(of_port) self.sg_port_map.remove_port(of_port) + for sec_group in of_port.sec_groups: + self._schedule_sg_deletion_maybe(sec_group.id) def update_security_group_rules(self, sg_id, rules): self.sg_port_map.update_rules(sg_id, rules) def update_security_group_members(self, sg_id, member_ips): self.sg_port_map.update_members(sg_id, member_ips) + if not member_ips: + self._schedule_sg_deletion_maybe(sg_id) + + def _schedule_sg_deletion_maybe(self, sg_id): + """Schedule possible deletion of the given SG. + + This function must be called when the number of ports + associated to sg_id drops to zero, as it isn't possible + to know SG deletions from agents due to RPC API design. + """ + sec_group = self.sg_port_map.get_or_create_sg(sg_id) + if not sec_group.members or not sec_group.ports: + self.sg_to_delete.add(sg_id) + + def _cleanup_stale_sg(self): + sg_to_delete = self.sg_to_delete + self.sg_to_delete = set() + + for sg_id in sg_to_delete: + sec_group = self.sg_port_map.get_sg(sg_id) + if sec_group.members and sec_group.ports: + # sec_group is still in use + continue + + self.conj_ip_manager.sg_removed(sg_id) + self.sg_port_map.delete_sg(sg_id) def filter_defer_apply_on(self): self._deferred = True def filter_defer_apply_off(self): if self._deferred: + self._cleanup_stale_sg() self.int_br.apply_flows() self._deferred = False @@ -672,29 +859,57 @@ class OVSFirewallDriver(firewall.FirewallDriver): ovsfw_consts.CT_MARK_INVALID) ) + def _add_non_ip_conj_flows(self, port): + """Install conjunction flows that don't depend on IP address of remote + groups, which consist of actions=conjunction(conj_id, 2/2) flows and + actions=accept flows. + + The remaining part is done by ConjIPFlowManager. + """ + for sec_group_id, rule in ( + self._create_remote_rules_generator_for_port(port)): + direction = rule['direction'] + ethertype = rule['ethertype'] + + conj_id = self.conj_ip_manager.add(port.vlan_tag, sec_group_id, + rule['remote_group_id'], + direction, ethertype) + + flows = rules.create_flows_from_rule_and_port(rule, port) + for flow in rules.substitute_conjunction_actions( + flows, 2, [conj_id]): + self._add_flow(**flow) + + # Install actions=accept flows. + for flow in rules.create_conj_flows( + port, conj_id, direction, ethertype): + self._add_flow(**flow) + def add_flows_from_rules(self, port): self._initialize_tracked_ingress(port) self._initialize_tracked_egress(port) LOG.debug('Creating flow rules for port %s that is port %d in OVS', port.id, port.ofport) - rules_generator = self.create_rules_generator_for_port(port) - for rule in rules_generator: + for rule in self._create_rules_generator_for_port(port): flows = rules.create_flows_from_rule_and_port(rule, port) LOG.debug("RULGEN: Rules generated for flow %s are %s", rule, flows) for flow in flows: self._accept_flow(**flow) - def create_rules_generator_for_port(self, port): + self._add_non_ip_conj_flows(port) + + self.conj_ip_manager.update_flows_for_vlan(port.vlan_tag) + + def _create_rules_generator_for_port(self, port): for sec_group in port.sec_groups: for rule in sec_group.raw_rules: yield rule + + def _create_remote_rules_generator_for_port(self, port): + for sec_group in port.sec_groups: for rule in sec_group.remote_rules: - remote_group = self.sg_port_map.sec_groups[ - rule['remote_group_id']] - for ip_addr in remote_group.get_ethertype_filtered_addresses( - rule['ethertype'], port.fixed_ips): - yield rules.create_rule_for_ip_address(ip_addr, rule) + yield sec_group.id, rule def delete_all_port_flows(self, port): """Delete all flows for given port""" @@ -704,3 +919,18 @@ class OVSFirewallDriver(firewall.FirewallDriver): self._delete_flows(reg_port=port.ofport) self._delete_flows(table=ovs_consts.ACCEPT_OR_INGRESS_TABLE, dl_dst=port.mac) + + def delete_flows_for_ip_addresses( + self, ip_addresses, direction, ethertype, vlan_tag): + for ip_addr in ip_addresses: + # Generate deletion template with bogus conj_id. + flows = rules.create_flows_for_ip_address( + ip_addr, direction, ethertype, vlan_tag, [0]) + for f in flows: + # The following del statements are partly for + # complying the OpenFlow spec. It forbids the use of + # these field in non-strict delete flow messages, and + # the actions field is bogus anyway. + del f['actions'] + del f['priority'] + self._delete_flows(**f) diff --git a/neutron/agent/linux/openvswitch_firewall/rules.py b/neutron/agent/linux/openvswitch_firewall/rules.py index a7a7803b769..8c64f613371 100644 --- a/neutron/agent/linux/openvswitch_firewall/rules.py +++ b/neutron/agent/linux/openvswitch_firewall/rules.py @@ -25,6 +25,17 @@ from neutron.plugins.ml2.drivers.openvswitch.agent.common import constants \ LOG = logging.getLogger(__name__) +CT_STATES = [ + ovsfw_consts.OF_STATE_ESTABLISHED_NOT_REPLY, + ovsfw_consts.OF_STATE_NEW_NOT_ESTABLISHED] + +FLOW_FIELD_FOR_IPVER_AND_DIRECTION = { + (n_consts.IP_VERSION_4, firewall.EGRESS_DIRECTION): 'nw_dst', + (n_consts.IP_VERSION_6, firewall.EGRESS_DIRECTION): 'ipv6_dst', + (n_consts.IP_VERSION_4, firewall.INGRESS_DIRECTION): 'nw_src', + (n_consts.IP_VERSION_6, firewall.INGRESS_DIRECTION): 'ipv6_src', +} + FORBIDDEN_PREFIXES = (n_consts.IPv4_ANY, n_consts.IPv6_ANY) @@ -48,24 +59,22 @@ def create_flows_from_rule_and_port(rule, port): } if is_valid_prefix(dst_ip_prefix): - if utils.get_ip_version(dst_ip_prefix) == n_consts.IP_VERSION_4: - flow_template["nw_dst"] = dst_ip_prefix - elif utils.get_ip_version(dst_ip_prefix) == n_consts.IP_VERSION_6: - flow_template["ipv6_dst"] = dst_ip_prefix + flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[( + utils.get_ip_version(dst_ip_prefix), firewall.EGRESS_DIRECTION)] + ] = dst_ip_prefix if is_valid_prefix(src_ip_prefix): - if utils.get_ip_version(src_ip_prefix) == n_consts.IP_VERSION_4: - flow_template["nw_src"] = src_ip_prefix - elif utils.get_ip_version(src_ip_prefix) == n_consts.IP_VERSION_6: - flow_template["ipv6_src"] = src_ip_prefix + flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[( + utils.get_ip_version(src_ip_prefix), firewall.INGRESS_DIRECTION)] + ] = src_ip_prefix flows = create_protocol_flows(direction, flow_template, port, rule) return flows -def create_protocol_flows(direction, flow_template, port, rule): - flow_template = flow_template.copy() +def populate_flow_common(direction, flow_template, port): + """Initialize common flow fields.""" if direction == firewall.INGRESS_DIRECTION: flow_template['table'] = ovs_consts.RULES_INGRESS_TABLE flow_template['dl_dst'] = port.mac @@ -77,6 +86,13 @@ def create_protocol_flows(direction, flow_template, port, rule): # should be applied flow_template['actions'] = 'resubmit(,{:d})'.format( ovs_consts.ACCEPT_OR_INGRESS_TABLE) + return flow_template + + +def create_protocol_flows(direction, flow_template, port, rule): + flow_template = populate_flow_common(direction, + flow_template.copy(), + port) protocol = rule.get('protocol') if protocol: if (rule.get('ethertype') == n_consts.IPv6 and @@ -128,12 +144,72 @@ def create_port_range_flows(flow_template, rule): return flows -def create_rule_for_ip_address(ip_address, rule): - new_rule = rule.copy() - del new_rule['remote_group_id'] - direction = rule['direction'] +def create_flows_for_ip_address(ip_address, direction, ethertype, + vlan_tag, conj_ids): + """Create flows from a rule and an ip_address derived from + remote_group_id + """ + ip_prefix = str(netaddr.IPNetwork(ip_address).cidr) - new_rule[firewall.DIRECTION_IP_PREFIX[direction]] = ip_prefix - LOG.debug('RULGEN: From rule %s with IP %s created new rule %s', - rule, ip_address, new_rule) - return new_rule + + flow_template = { + 'priority': 70, + 'dl_type': ovsfw_consts.ethertype_to_dl_type_map[ethertype], + 'reg_net': vlan_tag, # needed for project separation + } + + ip_ver = utils.get_ip_version(ip_prefix) + + if direction == firewall.EGRESS_DIRECTION: + flow_template['table'] = ovs_consts.RULES_EGRESS_TABLE + elif direction == firewall.INGRESS_DIRECTION: + flow_template['table'] = ovs_consts.RULES_INGRESS_TABLE + + flow_template[FLOW_FIELD_FOR_IPVER_AND_DIRECTION[( + ip_ver, direction)]] = ip_prefix + + return substitute_conjunction_actions([flow_template], 1, conj_ids) + + +def create_accept_flows(flow): + flow['ct_state'] = CT_STATES[0] + result = [flow.copy()] + flow['ct_state'] = CT_STATES[1] + if flow['table'] == ovs_consts.RULES_INGRESS_TABLE: + flow['actions'] = ( + 'ct(commit,zone=NXM_NX_REG{:d}[0..15]),{:s}'.format( + ovsfw_consts.REG_NET, flow['actions'])) + result.append(flow) + return result + + +def substitute_conjunction_actions(flows, dimension, conj_ids): + result = [] + for flow in flows: + for i in range(2): + new_flow = flow.copy() + new_flow['ct_state'] = CT_STATES[i] + new_flow['actions'] = ','.join( + ["conjunction(%d,%d/2)" % (s + i, dimension) + for s in conj_ids]) + result.append(new_flow) + + return result + + +def create_conj_flows(port, conj_id, direction, ethertype): + """Generate "accept" flows for a given conjunction ID.""" + flow_template = { + 'priority': 70, + 'conj_id': conj_id, + 'dl_type': ovsfw_consts.ethertype_to_dl_type_map[ethertype], + # This reg_port matching is for delete_all_port_flows. + # The matching is redundant as it has been done by + # conjunction(...,2/2) flows and flows can be summarized + # without this. + 'reg_port': port.ofport, + } + flow_template = populate_flow_common(direction, flow_template, port) + flows = create_accept_flows(flow_template) + flows[1]['conj_id'] += 1 + return flows diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py index cab353b6530..d03fa797816 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py @@ -67,16 +67,11 @@ class TestSecurityGroup(base.BaseTestCase): self.assertEqual(expected_raw_rules, self.sg.raw_rules) self.assertEqual(expected_remote_rules, self.sg.remote_rules) - def get_ethertype_filtered_addresses(self): + def test_get_ethertype_filtered_addresses(self): addresses = self.sg.get_ethertype_filtered_addresses('type') expected_addresses = [1, 2, 3, 4] self.assertEqual(expected_addresses, addresses) - def get_ethertype_filtered_addresses_with_excluded_addresses(self): - addresses = self.sg.get_ethertype_filtered_addresses('type', [2, 3]) - expected_addresses = [1, 4] - self.assertEqual(expected_addresses, addresses) - class TestOFPort(base.BaseTestCase): def setUp(self): @@ -226,6 +221,95 @@ class TestSGPortMap(base.BaseTestCase): self.map.update_members(1, []) +class TestConjIdMap(base.BaseTestCase): + def setUp(self): + super(TestConjIdMap, self).setUp() + self.conj_id_map = ovsfw.ConjIdMap() + + def test_get_conj_id(self): + allocated = [] + for direction in [firewall.EGRESS_DIRECTION, + firewall.INGRESS_DIRECTION]: + id_ = self.conj_id_map.get_conj_id( + 'sg', 'remote', direction, constants.IPv4) + allocated.append(id_) + self.assertEqual(len(set(allocated)), 2) + self.assertEqual(len(self.conj_id_map.id_map), 2) + self.assertEqual(self.conj_id_map.get_conj_id( + 'sg', 'remote', firewall.EGRESS_DIRECTION, constants.IPv4), + allocated[0]) + + def test_get_conj_id_invalid(self): + self.assertRaises(ValueError, self.conj_id_map.get_conj_id, + 'sg', 'remote', 'invalid-direction', + constants.IPv6) + + def test_delete_sg(self): + test_data = [('sg1', 'sg1'), ('sg1', 'sg2')] + + ids = [] + for sg_id, remote_sg_id in test_data: + ids.append(self.conj_id_map.get_conj_id( + sg_id, remote_sg_id, + firewall.INGRESS_DIRECTION, constants.IPv6)) + + result = self.conj_id_map.delete_sg('sg1') + self.assertIn(('sg1', ids[0]), result) + self.assertIn(('sg2', ids[1]), result) + self.assertEqual(len(self.conj_id_map.id_map), 0) + + reallocated = self.conj_id_map.get_conj_id( + 'sg-foo', 'sg-foo', firewall.INGRESS_DIRECTION, + constants.IPv6) + self.assertIn(reallocated, ids) + + +class TestConjIPFlowManager(base.BaseTestCase): + def setUp(self): + super(TestConjIPFlowManager, self).setUp() + self.driver = mock.Mock() + self.manager = ovsfw.ConjIPFlowManager(self.driver) + self.vlan_tag = 100 + self.conj_id = 10 + + def test_update_flows_for_vlan(self): + remote_group = self.driver.sg_port_map.get_sg.return_value + remote_group.get_ethertype_filtered_addresses.return_value = [ + '10.22.3.4'] + with mock.patch.object(self.manager.conj_id_map, + 'get_conj_id') as get_conj_id_mock: + get_conj_id_mock.return_value = self.conj_id + self.manager.add(self.vlan_tag, 'sg', 'remote_id', + firewall.INGRESS_DIRECTION, constants.IPv4) + self.manager.update_flows_for_vlan(self.vlan_tag) + self.assertEqual(self.driver._add_flow.call_args_list, + [mock.call(actions='conjunction(10,1/2)', ct_state='+est-rel-rpl', + dl_type=2048, nw_src='10.22.3.4/32', priority=70, + reg_net=self.vlan_tag, table=82), + mock.call(actions='conjunction(11,1/2)', ct_state='+new-est', + dl_type=2048, nw_src='10.22.3.4/32', priority=70, + reg_net=self.vlan_tag, table=82)]) + + def test_sg_removed(self): + with mock.patch.object(self.manager.conj_id_map, + 'get_conj_id') as get_id_mock, \ + mock.patch.object(self.manager.conj_id_map, + 'delete_sg') as delete_sg_mock: + get_id_mock.return_value = self.conj_id + delete_sg_mock.return_value = [('remote_id', self.conj_id)] + self.manager.add(self.vlan_tag, 'sg', 'remote_id', + firewall.INGRESS_DIRECTION, constants.IPv4) + self.manager.flow_state[self.vlan_tag][( + firewall.INGRESS_DIRECTION, constants.IPv4)] = { + '10.22.3.4': [self.conj_id]} + + self.manager.sg_removed('sg') + self.driver._add_flow.assert_not_called() + self.driver.delete_flows_for_ip_addresses.assert_called_once_with( + {'10.22.3.4'}, firewall.INGRESS_DIRECTION, constants.IPv4, + self.vlan_tag) + + class FakeOVSPort(object): def __init__(self, name, port, mac): self.port_name = name @@ -256,6 +340,10 @@ class TestOVSFirewallDriver(base.BaseTestCase): security_group_rules = [ {'ethertype': constants.IPv4, 'protocol': constants.PROTO_NAME_UDP, + 'direction': firewall.EGRESS_DIRECTION}, + {'ethertype': constants.IPv6, + 'protocol': constants.PROTO_NAME_TCP, + 'remote_group_id': 2, 'direction': firewall.EGRESS_DIRECTION}] self.firewall.update_security_group_rules(2, security_group_rules) @@ -432,8 +520,9 @@ class TestOVSFirewallDriver(base.BaseTestCase): self.firewall.update_port_filter(port_dict) self.assertTrue(self.mock_bridge.br.delete_flows.called) - add_calls = self.mock_bridge.br.add_flow.call_args_list - filter_rule = mock.call( + conj_id = self.firewall.conj_ip_manager.conj_id_map.get_conj_id( + 2, 2, firewall.EGRESS_DIRECTION, constants.IPv6) + filter_rules = [mock.call( actions='resubmit(,{:d})'.format( ovs_consts.ACCEPT_OR_INGRESS_TABLE), dl_src=self.port_mac, @@ -442,8 +531,17 @@ class TestOVSFirewallDriver(base.BaseTestCase): priority=70, ct_state=ovsfw_consts.OF_STATE_NEW_NOT_ESTABLISHED, reg5=self.port_ofport, - table=ovs_consts.RULES_EGRESS_TABLE) - self.assertIn(filter_rule, add_calls) + table=ovs_consts.RULES_EGRESS_TABLE), + mock.call( + actions='conjunction({:d},2/2)'.format(conj_id), + ct_state=ovsfw_consts.OF_STATE_ESTABLISHED_NOT_REPLY, + dl_src=mock.ANY, + dl_type=mock.ANY, + nw_proto=6, + priority=70, reg5=self.port_ofport, + table=ovs_consts.RULES_EGRESS_TABLE)] + self.mock_bridge.br.add_flow.assert_has_calls( + filter_rules, any_order=True) def test_update_port_filter_create_new_port_if_not_present(self): port_dict = {'device': 'port-id', @@ -470,6 +568,7 @@ class TestOVSFirewallDriver(base.BaseTestCase): self.firewall.prepare_port_filter(port_dict) self.firewall.remove_port_filter(port_dict) self.assertTrue(self.mock_bridge.br.delete_flows.called) + self.assertIn(1, self.firewall.sg_to_delete) def test_remove_port_filter_port_security_disabled(self): port_dict = {'device': 'port-id', @@ -492,3 +591,14 @@ class TestOVSFirewallDriver(base.BaseTestCase): """Just make sure it doesn't crash""" new_members = {constants.IPv4: [1, 2, 3, 4]} self.firewall.update_security_group_members(2, new_members) + + def test__cleanup_stale_sg(self): + self._prepare_security_group() + self.firewall.sg_to_delete = {1} + with mock.patch.object(self.firewall.conj_ip_manager, + 'sg_removed') as sg_removed_mock,\ + mock.patch.object(self.firewall.sg_port_map, + 'delete_sg') as delete_sg_mock: + self.firewall._cleanup_stale_sg() + sg_removed_mock.assert_called_once_with(1) + delete_sg_mock.assert_called_once_with(1) diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py index 637b0a1e127..fd40e8825ae 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_rules.py @@ -16,6 +16,7 @@ import mock from neutron_lib import constants from neutron.agent import firewall +from neutron.agent.linux.openvswitch_firewall import constants as ovsfw_consts from neutron.agent.linux.openvswitch_firewall import firewall as ovsfw from neutron.agent.linux.openvswitch_firewall import rules from neutron.common import constants as n_const @@ -307,18 +308,74 @@ class TestCreatePortRangeFlows(base.BaseTestCase): self._test_create_port_range_flows_helper(expected_flows, rule) -class TestCreateRuleForIpAddress(base.BaseTestCase): - def test_create_rule_for_ip_address(self): - sg_rule = { - 'remote_group_id': 'remote_id', - 'direction': firewall.INGRESS_DIRECTION, - 'some_settings': 'foo', +class TestCreateFlowsForIpAddress(base.BaseTestCase): + def _generate_conjuncion_actions(self, conj_ids, offset): + return ','.join( + ["conjunction(%d,1/2)" % (c + offset) + for c in conj_ids]) + + def test_create_flows_for_ip_address_egress(self): + expected_template = { + 'table': ovs_consts.RULES_EGRESS_TABLE, + 'priority': 70, + 'dl_type': n_const.ETHERTYPE_IP, + 'reg_net': 0x123, + 'nw_dst': '192.168.0.1/32' } - expected_rule = { - 'direction': firewall.INGRESS_DIRECTION, - 'source_ip_prefix': '192.168.0.1/32', - 'some_settings': 'foo', + + conj_ids = [12, 20] + flows = rules.create_flows_for_ip_address( + '192.168.0.1', firewall.EGRESS_DIRECTION, constants.IPv4, + 0x123, conj_ids) + + self.assertEqual(2, len(flows)) + self.assertEqual(ovsfw_consts.OF_STATE_ESTABLISHED_NOT_REPLY, + flows[0]['ct_state']) + self.assertEqual(ovsfw_consts.OF_STATE_NEW_NOT_ESTABLISHED, + flows[1]['ct_state']) + self.assertEqual(self._generate_conjuncion_actions(conj_ids, 0), + flows[0]['actions']) + self.assertEqual(self._generate_conjuncion_actions(conj_ids, 1), + flows[1]['actions']) + for f in flows: + del f['actions'] + del f['ct_state'] + self.assertEqual(expected_template, f) + + +class TestCreateConjFlows(base.BaseTestCase): + def test_create_conj_flows(self): + ovs_port = mock.Mock(ofport=1, vif_mac='00:00:00:00:00:00') + port_dict = {'device': 'port_id'} + port = ovsfw.OFPort( + port_dict, ovs_port, vlan_tag=TESTING_VLAN_TAG) + + conj_id = 1234 + expected_template = { + 'table': ovs_consts.RULES_INGRESS_TABLE, + 'dl_dst': port.mac, + 'dl_type': n_const.ETHERTYPE_IPV6, + 'priority': 70, + 'conj_id': conj_id, + 'reg_port': port.ofport } - translated_rule = rules.create_rule_for_ip_address( - '192.168.0.1', sg_rule) - self.assertEqual(expected_rule, translated_rule) + + flows = rules.create_conj_flows(port, conj_id, + firewall.INGRESS_DIRECTION, + constants.IPv6) + + self.assertEqual(ovsfw_consts.OF_STATE_ESTABLISHED_NOT_REPLY, + flows[0]['ct_state']) + self.assertEqual(ovsfw_consts.OF_STATE_NEW_NOT_ESTABLISHED, + flows[1]['ct_state']) + self.assertEqual("strip_vlan,output:{:d}".format(port.ofport), + flows[0]['actions']) + self.assertEqual("ct(commit,zone=NXM_NX_REG{:d}[0..15]),{:s}".format( + ovsfw_consts.REG_NET, flows[0]['actions']), + flows[1]['actions']) + + for f in flows: + del f['actions'] + del f['ct_state'] + self.assertEqual(expected_template, f) + expected_template['conj_id'] += 1