Fix RemoveSubnetRoute: Always take subnet route as dictionary

RemoveSubnetRoute needs the subnet route as a dictionary (so that
revert can work), but it was supplied as a name. Fix RemoveSubnetRoute
to take a dictionary and fix usages of RemoveSubnetRoute accordingly.

More details:

GetExistingSubnetRoutesForNetwork retrieves the existing subnet
routes, returning them as list of dictionaries, taken from the device
response. This list is passed to the RemoveSubnetRoute task for it to
be able to correctly rollback. RemoveSubnetRoute is called in two
places - make_remove_selfips_and_subnet_routes_flow (for removing
UNNEEDED subnet routes) and make_remove_l2_flow (for removing ALL
subnet routes). The make_remove_l2_flow function passes the
above-mentioned list of existing subnet routes to RemoveSubnetRoute
verbatim, but make_remove_selfips_and_subnet_routes_flow erroneously
extracts the names first (via a list comprehension: [r['name'] for r
in existing_subnet_routes ...]).

- This bug was introduced in 7c0153e (which added
  GetExistingSubnetRoutesForNetwork).

- In 4f78720 I had found the erroneous call in
  make_remove_selfips_and_subnet_routes_flow and adjusted the
  RemoveSubnetRoute task to take a name instead of a dictionary, but did
  not adjust the usage in make_remove_l2_flow, thus "inverting" the
  bug. Also, I forgot to adjust the RemoveSubnetRoute.revert method,
  meaning that the execute method now worked with the call in
  make_remove_selfips_and_subnet_routes_flow but not
  make_remove_l2_flow, and the other way round for the revert method.

- In b16dbc8 I adjusted the RemoveSubnetRoute.revert method as
  well. Now both the execute and the revert methods work with the call
  in make_remove_selfips_and_subnet_routes_flow but not
  make_remove_l2_flow.

This commit fixes this error.

- Adjust RemoveSubnetRoute.execute and RemoveSubnetRoute.revert to
  take a subnet route instead of just its name. This makes calling the
  task easier, since we can fix the erroneous list comprehension (see
  above). It also makes rollback easier because the revert method now
  has all the info it needs and doesn't need to infer anything anymore.

- make_remove_l2_flow: Supply subnet_route instead of subnet_route_name

- make_remove_selfips_and_subnet_routes_flow: Supply subnet_route
  instead of subnet_route_name and - most importantly - fix list
  comprehension (to not extract the name).

- test_revert_RemoveSubnetRoute: Supply subnet_route instead of
  subnet_route_name and check that during rollback the subnet route is
  recreated with its original attributes.

octavia_f5/controller/worker/flows/f5_flows.py

@@ -69,9 +69,9 @@ def make_remove_l2_flow(self, store: dict) -> flow.Flow:
 
         # remove subnet routes
         remove_subnet_routes_subflow = unordered_flow.Flow('remove-subnet-routes-subflow')
-        for subnet_route_name in existing_subnet_routes:
-            remove_subnet_route_task = f5_tasks.RemoveSubnetRoute(name=f"remove-subnet-route-{subnet_route_name}",
-                                                                  inject={'subnet_route_name': subnet_route_name})
+        for subnet_route in existing_subnet_routes:
+            remove_subnet_route_task = f5_tasks.RemoveSubnetRoute(name=f"remove-subnet-route-{subnet_route['name']}",
+                                                                  inject={'subnet_route': subnet_route})
             remove_subnet_routes_subflow.add(remove_subnet_route_task)
 
         # remove SelfIPs
@@ -134,16 +134,16 @@ def make_remove_selfips_and_subnet_routes_flow(self, needed_selfips, subnets_tha
 
         # remove subnet routes that are existing but don't belong to one of the subnets that need routes
         subnet_route_network_part = f5_tasks.get_subnet_route_name(network.id, '')
-        subnet_routes_to_remove = [r['name'] for r in existing_subnet_routes
+        subnet_routes_to_remove = [r for r in existing_subnet_routes
                                    if r['name'].startswith(subnet_route_network_part)
                                    and r['name'][len(subnet_route_network_part):] not in subnets_that_need_routes]
-        LOG.debug(f"{host}: Subnet routes to remove for network {network.id} (subnet IDs): {subnet_routes_to_remove}")
+        LOG.debug(f"{host}: Subnet routes to remove for network {network.id} (subnet IDs): {[r['name'] for r in subnet_routes_to_remove]}")
 
         # make subnet routes removal subflow
         remove_subnet_routes_subflow = unordered_flow.Flow('remove-subnet-routes-subflow')
-        for subnet_route_name in subnet_routes_to_remove:
-            remove_subnet_route_task = f5_tasks.RemoveSubnetRoute(name=f"remove-subnet-route-{subnet_route_name}",
-                                                                  inject={'subnet_route_name': subnet_route_name})
+        for subnet_route in subnet_routes_to_remove:
+            remove_subnet_route_task = f5_tasks.RemoveSubnetRoute(name=f"remove-subnet-route-{subnet_route['name']}",
+                                                                  inject={'subnet_route': subnet_route})
             remove_subnet_routes_subflow.add(remove_subnet_route_task)
 
         # remove SelfIPs that are existing but not needed

octavia_f5/controller/worker/tasks/f5_tasks.py

@@ -417,7 +417,8 @@ class RemoveSubnetRoute(task.Task):
 
     @decorators.RaisesIControlRestError()
     def execute(self, bigip: bigip_restclient.BigIPRestClient,
-                subnet_route_name):
+                subnet_route):
+        subnet_route_name = subnet_route['name']
         res = bigip.delete(path=f"/mgmt/tm/net/route/~Common~{subnet_route_name}")
 
         if res.status_code == 404:
@@ -427,27 +428,22 @@ def execute(self, bigip: bigip_restclient.BigIPRestClient,
 
     @decorators.RaisesIControlRestError()
     def revert(self, bigip: bigip_restclient.BigIPRestClient,
-               subnet_route_name, existing_subnet_routes, network,
+               subnet_route, existing_subnet_routes, network,
                *args, **kwargs):
+        subnet_route_name = subnet_route['name']
 
         # don't restore subnet route if it didn't exist before this task was executed
         if subnet_route_name not in [r['name'] for r in existing_subnet_routes]:
             LOG.warning("Reverting RemoveSubnetRoute: Not restoring subnet route since it didn't exist before the task "
                         f"was run: {subnet_route_name}")
             return
 
-        # payload
-        network_driver = driver_utils.get_network_driver()
-        subnet_id = subnet_route_name.split('_')[-1]
-        subnet = network_driver.get_subnet(subnet_id)
-        subnet_cidr = IPNetwork(subnet.cidr)
-        vlan = f"/Common/vlan-{network.vlan_id}"
-        net = f"{subnet_cidr.ip}%{network.vlan_id}/{subnet_cidr.prefixlen}"
-        subnet_route = {'name': subnet_route_name, 'tmInterface': vlan, 'network': net}
-
         # restore subnet route
+        payload = {'name': subnet_route_name,
+                   'tmInterface': subnet_route['tmInterface'],
+                   'network': subnet_route['network']}
         LOG.warning(f"Reverting RemoveSubnetRoute: Restoring subnet route: {subnet_route_name}")
-        res = bigip.post(path='/mgmt/tm/net/route', json=subnet_route)
+        res = bigip.post(path='/mgmt/tm/net/route', json=payload)
         res.raise_for_status()
 
 

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

@@ -437,7 +437,9 @@ def test_revert_RemoveSubnetRoute(self, mock_get_subnet):
         )
 
         subnet_route_name = f'net_{mock_network_id}_sub_{mock_subnet_id}'
-        subnet_route = {'name': subnet_route_name}
+        subnet_route = {'name': subnet_route_name,
+                        'tmInterface': 'original_subnet_route_tmInterface',
+                        'network': 'original_subnet_route_network'}
 
         # Revert before SelfIP deletion, so that we can check that post is called unconditionally
         class TestException(Exception):
@@ -447,7 +449,7 @@ class TestException(Exception):
             "Test exception to trigger rollback of EnsureSubnetRoute")
         store = {
             'bigip': mock_bigip,
-            'subnet_route_name': subnet_route_name,
+            'subnet_route': subnet_route,
             'existing_subnet_routes': [subnet_route],
             'network': mock_network,
         }
@@ -459,8 +461,8 @@ class TestException(Exception):
             path=f"/mgmt/tm/net/route",
             json={
                 'name': subnet_route_name,
-                'tmInterface': "/Common/vlan-1234",
-                'network': "2.3.4.0%1234/24",
+                'tmInterface': "original_subnet_route_tmInterface",
+                'network': "original_subnet_route_network",
             },
         )
         mock_bigip.get.assert_not_called()