From d34c99b764fc03679522c6fc9ed5d74896c4e93d Mon Sep 17 00:00:00 2001 From: Benjamin Ludwig Date: Tue, 2 Jul 2024 15:51:49 +0200 Subject: [PATCH] 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. --- .../controller/worker/flows/f5_flows.py | 16 +++++++-------- .../controller/worker/tasks/f5_tasks.py | 20 ++++++++----------- .../controller/worker/tasks/test_f5_tasks.py | 10 ++++++---- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/octavia_f5/controller/worker/flows/f5_flows.py b/octavia_f5/controller/worker/flows/f5_flows.py index c093eb1..0a4101b 100644 --- a/octavia_f5/controller/worker/flows/f5_flows.py +++ b/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 diff --git a/octavia_f5/controller/worker/tasks/f5_tasks.py b/octavia_f5/controller/worker/tasks/f5_tasks.py index 0ac1dff..48ec198 100644 --- a/octavia_f5/controller/worker/tasks/f5_tasks.py +++ b/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,8 +428,9 @@ 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]: @@ -436,18 +438,12 @@ def revert(self, bigip: bigip_restclient.BigIPRestClient, 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() diff --git a/octavia_f5/tests/unit/controller/worker/tasks/test_f5_tasks.py b/octavia_f5/tests/unit/controller/worker/tasks/test_f5_tasks.py index b4b3a1b..ba0efec 100644 --- a/octavia_f5/tests/unit/controller/worker/tasks/test_f5_tasks.py +++ b/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()