Merge ~chris.macnaughton/ubuntu/+source/neutron:bug/1881157 into ~ubuntu-server-dev/ubuntu/+source/neutron:stable/queens

[Chris MacNaughton]

[Corey Bryant (corey.bryant)]

Seems fine on a quick glance. We'll need to get point releases uploaded for > queens before we can upload this one. Let's hold off on merging until then in case something else comes up ahead of this.

[Corey Bryant (corey.bryant)]

Diff with stable/queens patch looks good. Looks like there was a quilt refresh to adapt to the current state of the package branch. Approving for now but not merging yet.

[Corey Bryant (corey.bryant)]

review: Approve

[Chris MacNaughton (chris.macnaughton)]

Agreed - ideally we'll get that neutron point release out soon which will give us S/T/U more easily!

Unmerged commits

0489eea... by Chris MacNaughton on 2020-09-22

d/p/lp1881157-remote-sg-is-left-behind.patch: Backport fix for security group IDs being left behind (LP: #1881157).

diff --git a/debian/changelog b/debian/changelog
index 75f60a5..70e65db 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+neutron (2:12.1.1-0ubuntu3) UNRELEASED; urgency=medium
+
+  * d/p/lp1881157-remote-sg-is-left-behind.patch: Backport fix for security
+    group IDs being left behind (LP: #1881157). 
+
+ -- Chris MacNaughton <[e-mail]>  Tue, 22 Sep 2020 11:16:27 +0000
+
 neutron (2:12.1.1-0ubuntu2) bionic; urgency=medium
 
   * d/p/Ensure-fip-ip-rules-deleted-when-fip-removed.patch
diff --git a/debian/patches/lp1881157-remote-sg-is-left-behind.patch b/debian/patches/lp1881157-remote-sg-is-left-behind.patch
 new file mode 100644
index 0000000..fda14bc
--- /dev/null
+++ b/debian/patches/lp1881157-remote-sg-is-left-behind.patch
@@ -0,0 +1,395 @@
+From d0b696f076a94830b040ffcb2dec28e2eabe2e04 Mon Sep 17 00:00:00 2001
+From: Rodolfo Alonso Hernandez <[e-mail]>
+Date: Tue, 02 Jun 2020 17:09:07 +0000
+Subject: [PATCH] [OVS][FW] Remote SG IDs left behind when a SG is removed
+
+When any port in the OVS agent is using a security groups (SG) and
+this SG is removed, is marked to be deleted. This deletion process
+is done in [1].
+
+The SG deletion process consists on removing any reference of this SG
+from the firewall and the SG port map. The firewall removes this SG in
+[2].
+
+The information of a SG is stored in:
+* ConjIPFlowManager.conj_id_map = ConjIdMap(). This class stores the
+  conjunction IDS (conj_ids) in a dictionary using the following keys:
+
+    ConjIdMap.id_map[(sg_id, remote_sg_id, direction, ethertype,
+      conj_ids)] = conj_id_XXX
+
+* ConjIPFlowManager.conj_ids is a nested dictionary, built in the
+  following way:
+
+    self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id] = \
+      set([conj_id_1, conj_id_2, ...])
+
+This patch stores all conjuntion IDs generated and assigned to the
+tuple (sg_id, remote_sg_id, direction, ethertype). When a SG is
+removed, the deletion method will look for this SG in the new storage
+variable created, ConjIdMap.id_map_group, and will mark all the
+conjuntion IDs related to be removed. That will cleanup those rules
+left in the OVS matching:
+  action=conjunction(conj_id, 1/2)
+
+[1]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L731
+[2]https://github.com/openstack/neutron/blob/118930f03d31f157f8c7a9e6c57122ecea8982b9/neutron/agent/linux/openvswitch_firewall/firewall.py#L399
+
+Conflicts:
+    neutron/agent/linux/openvswitch_firewall/firewall.py
+    neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
+
+Change-Id: I63e446a30cf10e7bcd34a6f0d6ba1711301efcbe
+Related-Bug: #1881157
+(cherry picked from commit 0eebd002ccda66dc6d9f9e5a254815109225e299)
+(cherry picked from commit ed22f7a2ff19a874bc8521f84cb4fd1c7483a23f)
+(cherry picked from commit 6615f248e25a361d988db1795a3486a58f4768cb)
+---
+
+diff --git a/neutron/agent/linux/openvswitch_firewall/firewall.py b/neutron/agent/linux/openvswitch_firewall/firewall.py
+index 3570032..b8522b2 100644
+--- a/neutron/agent/linux/openvswitch_firewall/firewall.py
++++ b/neutron/agent/linux/openvswitch_firewall/firewall.py
+@@ -268,6 +268,10 @@
+ 
+     def __init__(self):
+         self.id_map = collections.defaultdict(self._conj_id_factory)
++        # Stores the set of conjuntion IDs used for each unique tuple
++        # (sg_id, remote_sg_id, direction, ethertype). Each tuple can have up
++        # to 8 conjuntion IDs (see ConjIPFlowManager.add()).
++        self.id_map_group = collections.defaultdict(set)
+         self.id_free = collections.deque()
+         self.max_id = 0
+ 
+@@ -298,12 +302,21 @@
+         return a list of (remote_sg_id, conj_id), which are no longer
+         in use.
+         """
+-        result = []
++        result = set([])
+         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))
++                result.add((k[1], conj_id))
+                 self.id_free.append(conj_id)
++
++        # If the remote_sg_id is removed, the tuple (sg_id, remote_sg_id,
++        # direction, ethertype) no longer exists; the conjunction IDs assigned
++        # to this tuple should be removed too.
++        for k in list(self.id_map_group.keys()):
++            if sg_id in k[0:2]:
++                conj_id_groups = self.id_map_group.pop(k, [])
++                for conj_id in conj_id_groups:
++                    result.add((k[1], conj_id))
+ 
+         return result
+ 
+@@ -346,12 +359,22 @@
+         return addr_to_conj
+ 
+     def _update_flows_for_vlan_subr(self, direction, ethertype, vlan_tag,
+-                                    flow_state, addr_to_conj):
++                                    flow_state, addr_to_conj,
++                                    conj_id_to_remove):
+         """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)
++        conj_id_to_remove = conj_id_to_remove or []
++        # Delete any current flow related to any deleted IP address, before
++        # creating the flows for the current IPs.
++        self.driver.delete_flows_for_flow_state(
++            flow_state, addr_to_conj, direction, ethertype, vlan_tag)
++        for conj_id_set in conj_id_to_remove:
++            # Remove any remaining flow with remote SG ID conj_id_to_remove
++            for current_ip, conj_ids in flow_state.items():
++                conj_ids_to_remove = conj_id_set & set(conj_ids)
++                self.driver.delete_flow_for_ip(
++                    current_ip, direction, ethertype, vlan_tag,
++                    conj_ids_to_remove)
++
+         for addr, conj_ids in addr_to_conj.items():
+             conj_ids.sort()
+             if flow_state.get(addr) == conj_ids:
+@@ -360,7 +383,7 @@
+                     addr, direction, ethertype, vlan_tag, conj_ids):
+                 self.driver._add_flow(**flow)
+ 
+-    def update_flows_for_vlan(self, vlan_tag):
++    def update_flows_for_vlan(self, vlan_tag, conj_id_to_remove=None):
+         """Install action=conjunction(conj_id, 1/2) flows,
+         which depend on IP addresses of remote_group_id.
+         """
+@@ -372,7 +395,7 @@
+                 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)
++                addr_to_conj, conj_id_to_remove)
+             self.flow_state[vlan_tag][(direction, ethertype)] = addr_to_conj
+ 
+     def add(self, vlan_tag, sg_id, remote_sg_id, direction, ethertype,
+@@ -393,32 +416,43 @@
+                 collections.defaultdict(set))
+         self.conj_ids[vlan_tag][(direction, ethertype)][remote_sg_id].add(
+             conj_id)
++
++        conj_id_tuple = (sg_id, remote_sg_id, direction, ethertype)
++        self.conj_id_map.id_map_group[conj_id_tuple].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
++        Free all conj_ids associated with the sg_id removed and clean up
+         obsolete entries from the self.conj_ids map.  Unlike the add
+         method, it also updates flows.
++        If a SG is removed, both sg_id and remote_sg_id should be removed from
++        the "vlan_conj_id_map".
+         """
+-        id_list = self.conj_id_map.delete_sg(sg_id)
++        id_set = self.conj_id_map.delete_sg(sg_id)
+         unused_dict = collections.defaultdict(set)
+-        for remote_sg_id, conj_id in id_list:
++        for remote_sg_id, conj_id in id_set:
+             unused_dict[remote_sg_id].add(conj_id)
+ 
+         for vlan_tag, vlan_conj_id_map in self.conj_ids.items():
+             update = False
++            conj_id_to_remove = []
+             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):
++                        if remote_sg_id == sg_id:
++                            conj_id_to_remove.append(
++                                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)
++                self.update_flows_for_vlan(vlan_tag,
++                                           conj_id_to_remove=conj_id_to_remove)
+ 
+ 
+ class OVSFirewallDriver(firewall.FirewallDriver):
+@@ -499,7 +533,8 @@
+ 
+     def _delete_flows(self, **kwargs):
+         create_reg_numbers(kwargs)
+-        if self._deferred:
++        deferred = kwargs.pop('deferred', self._deferred)
++        if deferred:
+             self.int_br.delete_flows(**kwargs)
+         else:
+             self.int_br.br.delete_flows(**kwargs)
+@@ -1416,20 +1451,31 @@
+                                  in_port=port.ofport)
+         self._delete_flows(reg_port=port.ofport)
+ 
+-    def delete_flows_for_ip_addresses(
+-            self, ip_addresses, direction, ethertype, vlan_tag):
++    def delete_flows_for_flow_state(
++            self, flow_state, addr_to_conj, direction, ethertype, vlan_tag):
++        # Remove rules for deleted IPs and action=conjunction(conj_id, 1/2)
++        removed_ips = set(flow_state.keys()) - set(addr_to_conj.keys())
++        for removed_ip in removed_ips:
++            conj_ids = flow_state[removed_ip]
++            self.delete_flow_for_ip(removed_ip, direction, ethertype, vlan_tag,
++                                    conj_ids)
++
+         if not cfg.CONF.AGENT.explicitly_egress_direct:
+             return
+ 
+-        for ip_addr in ip_addresses:
++        for ip_addr in removed_ips:
+             # 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)
++            self.delete_flow_for_ip(ip_addr, direction, ethertype, vlan_tag,
++                                    [0])
++
++    def delete_flow_for_ip(self, ip_address, direction, ethertype,
++                           vlan_tag, conj_ids):
++        for flow in rules.create_flows_for_ip_address(
++                ip_address, direction, ethertype, vlan_tag, conj_ids):
++            # 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 flow['actions']
++            del flow['priority']
++            self._delete_flows(deferred=False, **flow)
+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 6aeb7a4..c1aed92 100644
+--- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
++++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_firewall.py
+@@ -282,23 +282,50 @@
+                           constants.IPv6)
+ 
+     def test_delete_sg(self):
+-        test_data = [('sg1', 'sg1'), ('sg1', 'sg2')]
++        test_data = [
++            # conj_id: 8
++            ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 0),
++            # conj_id: 10
++            ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 1),
++            # conj_id: 12
++            ('sg1', 'sg1', constants.INGRESS_DIRECTION, constants.IPv6, 2),
++            # conj_id: 16
++            ('sg2', 'sg1', constants.EGRESS_DIRECTION, constants.IPv6, 0),
++            # conj_id: 24
++            ('sg1', 'sg3', constants.INGRESS_DIRECTION, constants.IPv6, 0),
++            # conj_id: 36 (and 32 without priority offset, stored in id_map)
++            ('sg3', 'sg4', constants.INGRESS_DIRECTION, constants.IPv4, 2),
++            # conj_id: 40
++            ('sg5', 'sg4', constants.EGRESS_DIRECTION, constants.IPv4, 0),
++        ]
+ 
+         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))
++        conj_id_segment = set([])  # see ConjIPFlowManager.get_conj_id
++        # This is similar to ConjIPFlowManager.add method
++        for sg_id, rsg_id, direction, ip_version, prio_offset in test_data:
++            conj_id_tuple = (sg_id, rsg_id, direction, ip_version)
++            conj_id = self.conj_id_map.get_conj_id(*conj_id_tuple)
++            conj_id_segment.add(conj_id)
++            conj_id_plus_prio = conj_id + prio_offset * 2
++            self.conj_id_map.id_map_group[conj_id_tuple].add(conj_id_plus_prio)
++            ids.append(conj_id_plus_prio)
+ 
+         result = self.conj_id_map.delete_sg('sg1')
+-        self.assertIn(('sg1', ids[0]), result)
+-        self.assertIn(('sg2', ids[1]), result)
+-        self.assertFalse(self.conj_id_map.id_map)
++        self.assertEqual(
++            {('sg3', 24), ('sg1', 12), ('sg1', 16), ('sg1', 8), ('sg1', 10)},
++            result)
++        result = self.conj_id_map.delete_sg('sg3')
++        self.assertEqual({('sg4', 32), ('sg4', 36)}, result)
++        result = self.conj_id_map.delete_sg('sg4')
++        self.assertEqual({('sg4', 40)}, result)
++        self.assertEqual({}, self.conj_id_map.id_map)
++        self.assertEqual({}, self.conj_id_map.id_map_group)
+ 
+-        reallocated = self.conj_id_map.get_conj_id(
+-            'sg-foo', 'sg-foo', firewall.INGRESS_DIRECTION,
+-            constants.IPv6)
+-        self.assertIn(reallocated, ids)
++        reallocated = set([])
++        for sg_id, rsg_id, direction, ip_version, _ in test_data:
++            conj_id_tuple = (sg_id, rsg_id, direction, ip_version)
++            reallocated.add(self.conj_id_map.get_conj_id(*conj_id_tuple))
++        self.assertEqual(reallocated, conj_id_segment)
+ 
+ 
+ class TestConjIPFlowManager(base.BaseTestCase):
+@@ -363,7 +390,7 @@
+                        dl_type=2048, nw_src='10.22.3.4/32', priority=73,
+                        reg_net=self.vlan_tag, table=82)])
+ 
+-    def test_sg_removed(self):
++    def _sg_removed(self, sg_name):
+         with mock.patch.object(self.manager.conj_id_map,
+                                'get_conj_id') as get_id_mock, \
+              mock.patch.object(self.manager.conj_id_map,
+@@ -376,11 +403,26 @@
+                 firewall.INGRESS_DIRECTION, constants.IPv4)] = {
+                     '10.22.3.4': [self.conj_id]}
+ 
+-            self.manager.sg_removed('sg')
++            self.manager.sg_removed(sg_name)
++
++    def test_sg_removed(self):
++        self._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)
++        self.driver.delete_flows_for_flow_state.assert_called_once_with(
++            {'10.22.3.4': [self.conj_id]}, {},
++            constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag)
++        self.driver.delete_flow_for_ip.assert_not_called()
++
++    def test_remote_sg_removed(self):
++        self._sg_removed('remote_id')
++        self.driver._add_flow.assert_not_called()
++        self.driver.delete_flows_for_flow_state.assert_called_once_with(
++            {'10.22.3.4': [self.conj_id]}, {},
++            constants.INGRESS_DIRECTION, constants.IPv4, self.vlan_tag)
++        # "conj_id_to_remove" is populated with the remote_sg conj_id assigned,
++        # "_update_flows_for_vlan_subr" will call "delete_flow_for_ip".
++        self.driver.delete_flow_for_ip.assert_called_once_with(
++            '10.22.3.4', 'ingress', 'IPv4', 100, {self.conj_id})
+ 
+ 
+ class FakeOVSPort(object):
+@@ -401,7 +443,6 @@
+         self.fake_ovs_port = FakeOVSPort('port', 1, '00:00:00:00:00:00')
+         self.mock_bridge.br.get_vif_port_by_id.return_value = \
+             self.fake_ovs_port
+-        cfg.CONF.set_override('explicitly_egress_direct', True, 'AGENT')
+ 
+     def _prepare_security_group(self):
+         security_group_rules = [
+@@ -925,6 +966,43 @@
+         """Check that exception is not propagated outside."""
+         self.firewall.remove_trusted_ports(['port_id'])
+ 
++    def _test_delete_flows_for_flow_state(self, addr_to_conj,
++                                          explicitly_egress_direct=True):
++        direction = 'one_direction'
++        ethertype = 'ethertype'
++        vlan_tag = 'taaag'
++        with mock.patch.object(self.firewall, 'delete_flow_for_ip') as \
++                mock_delete_flow_for_ip:
++            flow_state = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
++            cfg.CONF.set_override('explicitly_egress_direct',
++                                  explicitly_egress_direct, 'AGENT')
++            self.firewall.delete_flows_for_flow_state(
++                flow_state, addr_to_conj, direction, ethertype, vlan_tag)
++        calls = []
++        for removed_ip in set(flow_state.keys()) - set(addr_to_conj.keys()):
++            calls.append(mock.call(removed_ip, direction, ethertype, vlan_tag,
++                                   flow_state[removed_ip]))
++            if explicitly_egress_direct:
++                calls.append(mock.call(removed_ip, direction, ethertype,
++                                       vlan_tag, [0]))
++        mock_delete_flow_for_ip.assert_has_calls(calls)
++
++    def test_delete_flows_for_flow_state_no_removed_ips_exp_egress(self):
++        addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
++        self._test_delete_flows_for_flow_state(addr_to_conj)
++
++    def test_delete_flows_for_flow_state_no_removed_ips_no_exp_egress(self):
++        addr_to_conj = {'addr1': {8, 16, 24}, 'addr2': {32, 40}}
++        self._test_delete_flows_for_flow_state(addr_to_conj, False)
++
++    def test_delete_flows_for_flow_state_removed_ips_exp_egress(self):
++        addr_to_conj = {'addr2': {32, 40}}
++        self._test_delete_flows_for_flow_state(addr_to_conj)
++
++    def test_delete_flows_for_flow_state_removed_ips_no_exp_egress(self):
++        addr_to_conj = {'addr1': {8, 16, 24}}
++        self._test_delete_flows_for_flow_state(addr_to_conj, False)
++
+ 
+ class TestCookieContext(base.BaseTestCase):
+     def setUp(self):
diff --git a/debian/patches/series b/debian/patches/series
index 96882c3..e65079e 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,3 +1,4 @@
 skip-iptest.patch
 flake8-legacy.patch
 Ensure-fip-ip-rules-deleted-when-fip-removed.patch
+lp1881157-remote-sg-is-left-behind.patch