Fix incorrect removal of IP rules in the amphora

When plugging a member, an IP rule was deleted from the amphora. It
triggered connectivity issue when a client on subnet1 sends a request to
the VIP (on subnet2) while subnet1 is also plugged to the amp as a
member subnet.
The IP rules should be updated only when plugging/updating the VIP
interface. The JSON files that store the configuration of the interfaces
now contain the type of the interface ('vip', 'lo' or 'member') and the
rules update step is skipped when the type is not 'vip'.

Closes-Bug: #2016475

Change-Id: I3fe9bb584c8522482e9d7289ae8ea1aab205dd4d

octavia/amphorae/backends/agent/api_server/osutils.py

@@ -56,6 +56,7 @@ class BaseOS(object):
     def write_interface_file(self, interface, ip_address, prefixlen):
         interface = interface_file.InterfaceFile(
             name=interface,
+            if_type=consts.LO,
             addresses=[{
                 "address": ip_address,
                 "prefixlen": prefixlen

octavia/amphorae/backends/utils/interface.py

@@ -197,6 +197,8 @@ class InterfaceController(object):
 
             self._addresses_up(interface, ipr, idx)
             self._routes_up(interface, ipr, idx)
+            # only the vip port updates the rules
+            if interface.if_type == consts.VIP:
                 self._rules_up(interface, ipr, idx)
 
             self._scripts_up(interface, current_state)
@@ -378,6 +380,8 @@ class InterfaceController(object):
             current_state = link.get(consts.STATE)
 
             if current_state == consts.IFACE_UP:
+                # only the vip port updates the rules
+                if interface.if_type == consts.VIP:
                     for rule in interface.rules:
                         rule[consts.FAMILY] = self._family(rule[consts.SRC])
                         LOG.debug("%s: Deleting rule %s", interface.name, rule)

octavia/amphorae/backends/utils/interface_file.py

@@ -25,9 +25,11 @@ CONF = cfg.CONF
 
 
 class InterfaceFile(object):
-    def __init__(self, name, mtu=None, addresses=None,
+    def __init__(self, name, if_type,
+                 mtu=None, addresses=None,
                  routes=None, rules=None, scripts=None):
         self.name = name
+        self.if_type = if_type
         self.mtu = mtu
         self.addresses = addresses or []
         self.routes = routes or []
@@ -92,6 +94,7 @@ class InterfaceFile(object):
                                flags, mode), 'w') as fp:
             interface = {
                 consts.NAME: self.name,
+                consts.IF_TYPE: self.if_type,
                 consts.ADDRESSES: self.addresses,
                 consts.ROUTES: self.routes,
                 consts.RULES: self.rules,
@@ -105,7 +108,7 @@ class InterfaceFile(object):
 class VIPInterfaceFile(InterfaceFile):
     def __init__(self, name, mtu, vips, vrrp_info, fixed_ips, topology):
 
-        super().__init__(name, mtu=mtu)
+        super().__init__(name, if_type=consts.VIP, mtu=mtu)
 
         has_ipv4 = any(vip['ip_version'] == 4 for vip in vips)
         has_ipv6 = any(vip['ip_version'] == 6 for vip in vips)
@@ -236,7 +239,7 @@ class VIPInterfaceFile(InterfaceFile):
 
 class PortInterfaceFile(InterfaceFile):
     def __init__(self, name, mtu, fixed_ips):
-        super().__init__(name, mtu=mtu)
+        super().__init__(name, if_type=consts.BACKEND, mtu=mtu)
 
         if fixed_ips:
             ip_versions = set()

octavia/common/constants.py

@@ -911,6 +911,9 @@ AMPHORA_SUPPORTED_ALPN_PROTOCOLS = [lib_consts.ALPN_PROTOCOL_HTTP_2,
                                     lib_consts.ALPN_PROTOCOL_HTTP_1_0]
 
 # Amphora interface fields
+IF_TYPE = 'if_type'
+BACKEND = 'backend'
+LO = 'lo'
 MTU = 'mtu'
 ADDRESSES = 'addresses'
 ROUTES = 'routes'

octavia/tests/unit/amphorae/backends/agent/api_server/test_osutils.py

@@ -106,7 +106,7 @@ class TestOSUtils(base.TestCase):
                                                  '192.0.2.2', 16)
 
         mock_interface_file.assert_called_once_with(
-            name='eth1',
+            name='eth1', if_type="lo",
             addresses=[{"address": "192.0.2.2", "prefixlen": 16}])
         mock_interface.write.assert_called_once()
 

octavia/tests/unit/amphorae/backends/utils/test_interface.py

@@ -74,6 +74,7 @@ class TestInterface(base.TestCase):
                    '],\n'
                    '"mtu": 1450,\n'
                    '"name": "eth1",\n'
+                   '"if_type": "mytype",\n'
                    '"routes": [\n'
                    '{"dst": "0.0.0.0/0",\n'
                    '"gateway": "10.0.0.1"},\n'
@@ -107,6 +108,7 @@ class TestInterface(base.TestCase):
 
         expected_dict = {
             consts.NAME: "eth1",
+            consts.IF_TYPE: "mytype",
             consts.MTU: 1450,
             consts.ADDRESSES: [{
                 consts.ADDRESS: "10.0.0.2",
@@ -331,6 +333,7 @@ class TestInterface(base.TestCase):
                 mock_link, mock_addr, mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.ADDRESS: '1.2.3.4',
@@ -445,6 +448,78 @@ class TestInterface(base.TestCase):
             mock.call(["post-up", "eth1"])
         ])
 
+    @mock.patch('pyroute2.IPRoute.rule')
+    @mock.patch('pyroute2.IPRoute.route')
+    @mock.patch('pyroute2.IPRoute.addr')
+    @mock.patch('pyroute2.IPRoute.link')
+    @mock.patch('pyroute2.IPRoute.get_links')
+    @mock.patch('pyroute2.IPRoute.link_lookup')
+    @mock.patch('pyroute2.IPRoute.get_rules')
+    @mock.patch('subprocess.check_output')
+    def test_up_backend(self, mock_check_output, mock_get_rules,
+                        mock_link_lookup, mock_get_links, mock_link, mock_addr,
+                        mock_route, mock_rule):
+        iface = interface_file.InterfaceFile(
+            name="eth1",
+            if_type="backend",
+            mtu=1450,
+            addresses=[{
+                consts.ADDRESS: '1.2.3.4',
+                consts.PREFIXLEN: 24
+            }],
+            routes=[],
+            rules=[],
+            scripts={
+                consts.IFACE_UP: [{
+                    consts.COMMAND: "post-up eth1"
+                }],
+                consts.IFACE_DOWN: [{
+                    consts.COMMAND: "post-down eth1"
+                }],
+            })
+
+        idx = mock.MagicMock()
+        mock_link_lookup.return_value = [idx]
+
+        mock_get_links.return_value = [{
+            consts.STATE: consts.IFACE_DOWN
+        }]
+        mock_get_rules.return_value = [{
+            'src_len': 32,
+            'attrs': {
+                'FRA_SRC': '1.1.1.1',
+                'FRA_TABLE': 20,
+                'FRA_PROTOCOL': 0
+            }
+        }]
+
+        controller = interface.InterfaceController()
+        controller.up(iface)
+
+        mock_link.assert_called_once_with(
+            controller.SET,
+            index=idx,
+            state=consts.IFACE_UP,
+            mtu=1450)
+
+        mock_addr.assert_has_calls([
+            mock.call(controller.ADD,
+                      index=idx,
+                      address='1.2.3.4',
+                      prefixlen=24,
+                      family=socket.AF_INET),
+        ])
+
+        mock_route.assert_called_once_with(
+            'dump', family=mock.ANY, match=mock.ANY)
+
+        # for 'backend' iface, we don't update the rules
+        mock_rule.assert_not_called()
+
+        mock_check_output.assert_has_calls([
+            mock.call(["post-up", "eth1"])
+        ])
+
     @mock.patch('pyroute2.IPRoute.rule')
     @mock.patch('pyroute2.IPRoute.route')
     @mock.patch('pyroute2.IPRoute.addr')
@@ -463,6 +538,7 @@ class TestInterface(base.TestCase):
                        mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.ADDRESS: '1.2.3.4',
@@ -658,6 +734,7 @@ class TestInterface(base.TestCase):
                      mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.DHCP: True,
@@ -722,6 +799,7 @@ class TestInterface(base.TestCase):
                   mock_link, mock_addr, mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.ADDRESS: '1.2.3.4',
@@ -848,6 +926,7 @@ class TestInterface(base.TestCase):
                               mock_addr, mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.ADDRESS: '1.2.3.4',
@@ -997,6 +1076,7 @@ class TestInterface(base.TestCase):
                                mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.ADDRESS: '1.2.3.4',
@@ -1069,6 +1149,7 @@ class TestInterface(base.TestCase):
                        mock_addr, mock_route, mock_rule):
         iface = interface_file.InterfaceFile(
             name="eth1",
+            if_type="vip",
             mtu=1450,
             addresses=[{
                 consts.DHCP: True,

octavia/tests/unit/amphorae/backends/utils/test_interface_file.py

@@ -691,6 +691,7 @@ class TestInterfaceFile(base.TestCase):
                    '"prefixlen": 26}\n'
                    '],\n'
                    '"mtu": 1450,\n'
+                   '"if_type": "mytype",\n'
                    '"name": "eth1",\n'
                    '"routes": [\n'
                    '{"dst": "0.0.0.0/0",\n'
@@ -717,6 +718,7 @@ class TestInterfaceFile(base.TestCase):
 
         expected_dict = {
             consts.NAME: "eth1",
+            consts.IF_TYPE: "mytype",
             consts.MTU: 1450,
             consts.ADDRESSES: [{
                 consts.ADDRESS: "10.0.0.181",

octavia/tests/unit/cmd/test_interface.py

@@ -23,8 +23,8 @@ class TestInterfaceCMD(base.TestCase):
     def setUp(self):
         super().setUp()
 
-        self.interface1 = interface_file.InterfaceFile("eth1")
+        self.interface1 = interface_file.InterfaceFile("eth1", if_type="type1")
-        self.interface2 = interface_file.InterfaceFile("eth2")
+        self.interface2 = interface_file.InterfaceFile("eth2", if_type="type2")
 
     def test_interfaces_find(self):
         controller = mock.Mock()

releasenotes/notes/fix-ip-rules-in-amphora-b74b7b616752c13b.yaml

@@ -0,0 +1,11 @@
+---
+fixes:
+  - |
+    Fixed a bug that could have made the VIP port unreachable because of the
+    removal of some IP rules in the Amphora. It could have been triggered only
+    when sending a request from a subnet that is not the VIP subnet but that is
+    plugged as a member subnet.
+upgrade:
+  - |
+    A patch that fixes an issue making the VIP port unreachable because of
+    missing IP rules requires an update of the Amphora image.