[l2][unittest] Fix: RemoveSelfIP.revert is missing critical info

When RemoveSelfIP.revert restores a deleted SelfIP on the device, it
relies on store['existing_selfips'] for the needed information.
store['existing_selfips'] is a list of port objects, retrieved via
GetExistingSelfIPsForVLAN. However, these port objects contain only
the port ID. But when RemoveSelfIP is rolled back, it needs to know
more than the port ID, e.g. the subnet, which is retrieved from the
port's first fixed IP. Since the port only has an ID, the SelfIP
cannot be restored on the BigIP.

This commit fixes GetExistingSelfIPsForVLAN to return a list of the
SelfIP dictionaries retrieved from the device, instead of a list of
port objects. This way, all the necessary information to restore a
SelfIP on the device is present.

For ease of use GetExistingSelfIPsForVLAN adds another key to the
SelfIP dictionaries: port_id. This way we only have to extract it from
the SelfIP name once.

This commit also fixes the corresponding flow/task tests to construct
store['existing_selfips'] correctly.

octavia_f5/controller/worker/flows/f5_flows.py

@@ -31,8 +31,9 @@ def make_ensure_l2_flow(self, selfips: [network_models.Port], store: dict) -> fl
 
         # make SelfIP creation subflow
         ensure_selfips_subflow = unordered_flow.Flow('ensure-selfips-subflow')
-        for selfip in selfips:
-            ensure_selfip_task = f5_tasks.EnsureSelfIP(name=f"ensure-selfip-{selfip.id}", inject={'port': selfip})
+        for selfip_port in selfips:
+            ensure_selfip_task = f5_tasks.EnsureSelfIP(
+                name=f"ensure-selfip-{selfip_port.id}", inject={'port': selfip_port})
             ensure_selfips_subflow.add(ensure_selfip_task)
 
         # create subnet routes for all subnets that don't have a SelfIP
@@ -76,7 +77,8 @@ def make_remove_l2_flow(self, store: dict) -> flow.Flow:
         # remove SelfIPs
         remove_selfips_subflow = unordered_flow.Flow('remove-selfips-subflow')
         for selfip in existing_selfips:
-            remove_selfip_task = f5_tasks.RemoveSelfIP(name=f"remove-selfip-{selfip.id}", inject={'port': selfip})
+            remove_selfip_task = f5_tasks.RemoveSelfIP(name=f"remove-selfip-{selfip['port_id']}",
+                                                       inject={'selfip': selfip})
             remove_selfips_subflow.add(remove_selfip_task)
 
         # remove other L2 objects
@@ -122,7 +124,7 @@ def make_remove_selfips_and_subnet_routes_flow(self, needed_selfips, subnets_tha
                                                    store: dict) -> flow.Flow:
         """ Remove unneeded SelfIPs and subnet routes of a specific network
 
-        :param needed_selfips: SelfIPs that must exist
+        :param needed_selfips: Ports for SelfIPs that must exist
         :param subnets_that_need_routes: Subnets for which subnet routes must exist
         """
         host = store['bigip'].hostname
@@ -145,14 +147,14 @@ def make_remove_selfips_and_subnet_routes_flow(self, needed_selfips, subnets_tha
             remove_subnet_routes_subflow.add(remove_subnet_route_task)
 
         # remove SelfIPs that are existing but not needed
-        selfips_to_remove = [port for port in existing_selfips if port.id not in [p.id for p in needed_selfips]]
-        LOG.debug(f"{host}: SelfIPs to remove for network {network.id}: {[p.id for p in selfips_to_remove]}")
+        selfips_to_remove = [sip for sip in existing_selfips if sip['port_id'] not in [p.id for p in needed_selfips]]
+        LOG.debug(f"{host}: SelfIPs to remove for network {network.id}: {[sip['port_id'] for sip in selfips_to_remove]}")
 
         # make SelfIPs removal subflow
         remove_selfips_subflow = unordered_flow.Flow('remove-selfips-subflow')
-        for selfip_port in selfips_to_remove:
-            remove_selfip = f5_tasks.RemoveSelfIP(name=f"remove-selfip-{selfip_port.id}",
-                                                  inject={'port': selfip_port})
+        for selfip in selfips_to_remove:
+            remove_selfip = f5_tasks.RemoveSelfIP(name=f"remove-selfip-{selfip['port_id']}",
+                                                  inject={'selfip': selfip})
             remove_selfips_subflow.add(remove_selfip)
 
         # make and return flow
@@ -174,13 +176,15 @@ def make_ensure_selfips_and_subnet_routes_flow(self, needed_selfips, subnets_tha
         preexisting_subnet_routes = store['existing_subnet_routes']
 
         # find SelfIPs that are expected but not existing
-        selfips_to_create = [port for port in needed_selfips if port.id not in [p.id for p in preexisting_selfips]]
+        selfips_to_create = [port for port in needed_selfips
+                             if port.id not in [sip['port_id'] for sip in preexisting_selfips]]
         LOG.debug(f"{host}: SelfIPs to add for network {network.id}: {[p.id for p in selfips_to_create]}")
 
         # make SelfIP creation subflow
         ensure_selfips_subflow = unordered_flow.Flow('ensure-selfips-subflow')
-        for selfip in selfips_to_create:
-            ensure_selfip_task = f5_tasks.EnsureSelfIP(name=f"ensure-selfip-{selfip.id}", inject={'port': selfip})
+        for selfip_port in selfips_to_create:
+            ensure_selfip_task = f5_tasks.EnsureSelfIP(
+                name=f"ensure-selfip-{selfip_port.id}", inject={'port': selfip_port})
             ensure_selfips_subflow.add(ensure_selfip_task)
 
         # find subnet routes for subnets that need them but don't have any yet
@@ -191,7 +195,8 @@ def make_ensure_selfips_and_subnet_routes_flow(self, needed_selfips, subnets_tha
         ]
         subnets_to_create_routes_for = [s for s in subnets_that_need_routes
                                         if s not in subnets_of_preexisting_subnet_routes]
-        LOG.debug(f"{host}: Subnet of network {network.id} for which routes will be created: {subnets_to_create_routes_for}")
+        LOG.debug(f"{host}: Subnet of network {network.id} for which routes will be created: "
+                  f"{subnets_to_create_routes_for}")
 
         # make subnet route creation subflow
         ensure_subnet_routes_subflow = unordered_flow.Flow('ensure-subnet-routes-subflow')

octavia_f5/controller/worker/tasks/f5_tasks.py

@@ -222,7 +222,7 @@ def revert(self, port: network_models.Port,
         selfip_name = f"port-{port.id}"
 
         # don't remove the SelfIP if it existed before this task was executed
-        if port.id in [p.id for p in existing_selfips]:
+        if port.id in [p['port_id'] for p in existing_selfips]:
             LOG.warning("Reverting EnsureSelfIP: Not deleting SelfIP, since it existed before the task was run: "
                         f"{selfip_name}")
             return
@@ -239,21 +239,24 @@ def revert(self, port: network_models.Port,
 class GetExistingSelfIPsForVLAN(task.Task):
     default_provides = 'existing_selfips'
 
-    @staticmethod
-    def _remove_port_prefix(name: str):
-        return name[len('port-'):]
-
     @decorators.RaisesIControlRestError()
     def execute(self, bigip: bigip_restclient.BigIPRestClient,
                 network: f5_network_models.Network):
-        vlan = f"/Common/vlan-{network.vlan_id}"
+
+        # get items
         device_response = bigip.get(path='/mgmt/tm/net/self?$select=vlan,name')
         device_response.raise_for_status()
         items = device_response.json().get('items', [])
-        return [network_models.Port(id=self._remove_port_prefix(item['name']))
-                for item in items
-                if item['vlan'] == vlan
-                and item['name'].startswith('port-')]
+
+        # filter for VLAN
+        vlan = f"/Common/vlan-{network.vlan_id}"
+        items = [i for i in items if i['vlan'] == vlan and i['name'].startswith('port-')]
+
+        # we have to get the port ID oftentimes, so inject it for ease of use
+        for i in items:
+            i['port_id'] = i['name'][len('port-'):]
+
+        return items
 
 
 class GetExistingSubnetRoutesForNetwork(task.Task):
@@ -451,39 +454,28 @@ def revert(self, bigip: bigip_restclient.BigIPRestClient,
 class RemoveSelfIP(task.Task):
 
     @decorators.RaisesIControlRestError()
-    def execute(self, bigip: bigip_restclient.BigIPRestClient,
-                port: network_models.Port):
-        res = bigip.delete(path=f"/mgmt/tm/net/self/port-{port.id}")
+    def execute(self, bigip: bigip_restclient.BigIPRestClient, selfip: dict):
+        res = bigip.delete(path=f"/mgmt/tm/net/self/port-{selfip['port_id']}")
 
         if res.status_code == 404:
-            LOG.warning(f"SelfIP port-{port.id} was already removed")
+            LOG.warning(f"SelfIP port-{selfip['port_id']} was already removed")
         else:
             res.raise_for_status()
 
     @decorators.RaisesIControlRestError()
-    def revert(self, port: network_models.Port,
-               bigip: bigip_restclient.BigIPRestClient,
-               existing_selfips: [network_models.Port],
-               network, *args, **kwargs):
+    def revert(self, bigip: bigip_restclient.BigIPRestClient,
+               selfip: dict, existing_selfips: [dict], *args, **kwargs):
 
         # don't restore SelfIP if it didn't exist before this task was executed
-        if port.id not in [p.id for p in existing_selfips]:
+        if selfip['name'] not in [sip['name'] for sip in existing_selfips]:
             LOG.warning("Reverting RemoveSelfIP: Not restoring SelfIP since it didn't exist before the task "
-                        f"was run: port-{port.id}")
+                        f"was run: {selfip['name']}")
             return
 
-        # payload
-        network_driver = driver_utils.get_network_driver()
-        name = f"port-{port.id}"
-        vlan = f"/Common/vlan-{network.vlan_id}"
-        subnet = network_driver.get_subnet(port.fixed_ips[0].subnet_id)
-        subnet_cidr = IPNetwork(subnet.cidr)
-        address = f"{port.fixed_ips[0].ip_address}%{network.vlan_id}/{subnet_cidr.prefixlen}"
-        selfip = {'name': name, 'vlan': vlan, 'address': address}
-
         # restore SelfIP
-        LOG.warning(f"Reverting RemoveSelfIP: Restoring SelfIP: port-{port.id}")
-        res = bigip.post(path=f"/mgmt/tm/net/self/", json=selfip)
+        payload = {'name': selfip['name'], 'vlan': selfip['vlan'], 'address': selfip['address']}
+        LOG.warning(f"Reverting RemoveSelfIP: Restoring SelfIP: {selfip['name']}")
+        res = bigip.post(path=f"/mgmt/tm/net/self/", json=payload)
         res.raise_for_status()
 
 

octavia_f5/tests/unit/controller/worker/flows/test_f5_flows.py

@@ -292,7 +292,7 @@ def test_remove_selfip(self):
 
         store = {'network': mock_network,
                  'bigip': mock_bigip,
-                 'existing_selfips': [selfip_port],
+                 'existing_selfips': [{'name': f"port-{selfip_port.id}", 'port_id': selfip_port.id}],
                  'existing_subnet_routes': []}
         needed_selfips = []
         subnets_that_need_routes = []
@@ -371,7 +371,7 @@ def test_replace_selfip_with_subnet_route(self, mock_get_subnet):
 
         store = {'network': mock_network,
                  'bigip': mock_bigip,
-                 'existing_selfips': [selfip_port],
+                 'existing_selfips': [{'name': f"port-{selfip_port.id}", 'port_id': selfip_port.id}],
                  'existing_subnet_routes': []}
         needed_selfips = []
         subnets_that_need_routes = [mock_subnet_id]

octavia_f5/tests/unit/controller/worker/tasks/test_f5_tasks.py

@@ -372,8 +372,6 @@ def test_revert_RemoveSelfIP(self, mock_get_subnet):
                        'provider:segmentation_id': 1234}]
         )
 
-        selfip_fixed_ip = network_models.FixedIP(
-            ip_address='1.2.3.2', subnet_id=mock_subnet_id)
         # SelfIPs to remove come from the GetExistingSelfIPsForVLAN task and thus only contain ID, nothing else
         selfip_port = network_models.Port(id='test-selfip-port-id')
         selfip_name = f"port-{selfip_port.id}"
@@ -384,10 +382,16 @@ class TestException(Exception):
         mock_bigip = mock.Mock(spec=as3restclient.AS3RestClient)
         mock_bigip.delete.side_effect = TestException(
             "Test exception to trigger rollback of EnsureSelfIP")
+        selfip_port_dict = {
+            'name': f"port-{selfip_port.id}",
+            'port_id': selfip_port.id,
+            'address': "1.2.3.2%1234/24",
+            'vlan': '/Common/vlan-1234',
+        }
         store = {
             'bigip': mock_bigip,
-            'existing_selfips': [selfip_port],
-            'port': selfip_port,
+            'existing_selfips': [selfip_port_dict],
+            'selfip': selfip_port_dict,
             'network': mock_network,
         }