From 69f95cc5bbc24c3cfb8101596022dcc956f0e300 Mon Sep 17 00:00:00 2001 From: Callum Dickinson Date: Tue, 25 Jun 2024 14:10:13 +1200 Subject: [PATCH] Allow default networks in multiple regions This adds the optional `create_in_regions` and `create_in_all_regions` parameters to the `NewDefaultNetworkAction` and the `NewProjectDefaultNetworkAction` actions. When `create_in_regions` is set, the action will ignore the default region set when creating a project, and instead create default networks and routers for each region set in the `create_in_regions` list. When `create_in_regions` is unset (or set to an empty list), the actions will default to the behaviour dictated by `create_in_all_regions`. When `create_in_all_regions` is set to `False` (the default), a default network is created in the default region only, as it always has. When it is set to `True`, it will instead create default networks in *all* active cloud regions, as discovered by Adjutant. Improve error handling in `NewDefaultNetworkAction` to give more detail in the task log when resource creation fails. Fix bugs in the fake Neutron client in the unit tests where created resources were hard-coded to be added to the `RegionOne` region. Change-Id: I0e0885dda449b22788bd4c9ae72b874d8ce661b5 --- adjutant/actions/v1/resources.py | 181 +++++++++-- .../actions/v1/tests/test_resource_actions.py | 289 ++++++++++++++++-- adjutant/common/tests/fake_clients.py | 22 +- ...ple-default-networks-5a89766d377b06d2.yaml | 13 + 4 files changed, 445 insertions(+), 60 deletions(-) create mode 100644 releasenotes/notes/multiple-default-networks-5a89766d377b06d2.yaml diff --git a/adjutant/actions/v1/resources.py b/adjutant/actions/v1/resources.py index 4e79dda..418afca 100644 --- a/adjutant/actions/v1/resources.py +++ b/adjutant/actions/v1/resources.py @@ -83,6 +83,28 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): "See 'region_defaults'.", default={}, ), + fields.ListConfig( + "create_in_regions", + help_text=( + "Set the regions in which a default network will be " + "created. When unset or empty, the default behaviour " + "depends on the value of 'create_in_all_regions'." + ), + default=[], + required=False, + ), + fields.BoolConfig( + "create_in_all_regions", + help_text=( + "When set to False (the default), if no regions are set " + "in 'create_in_regions', a default network will only be " + "created in the default region for new sign-ups. " + "When set to True, a default network will be created in " + "all regions, not just the default region." + ), + default=False, + required=False, + ), ] ) @@ -114,14 +136,34 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): self.action.save() def _create_network(self): - neutron = openstack_clients.get_neutronclient(region=self.region) + if self.config.create_in_regions: + for region in self.config.create_in_regions: + self._create_network_in_region(region) + elif self.config.create_in_all_regions: + id_manager = user_store.IdentityManager() + for region in id_manager.list_regions(): + region_id = region.id + self._create_network_in_region(region_id) + else: + self._create_network_in_region(self.region) + + def _create_network_in_region(self, region): + neutron = openstack_clients.get_neutronclient(region=region) try: - region_config = self.config.regions[self.region] + region_config = self.config.regions[region] network_config = self.config.region_defaults.overlay(region_config) except KeyError: network_config = self.config.region_defaults - if not self.get_cache("network_id"): + cache_suffix = f"_{region}" + is_default_region = region == self.region + + network_id = self.get_cache(f"network_id{cache_suffix}") + # NOTE(callumdickinson): Backwards compatibility with older tasks. + if not network_id and is_default_region: + network_id = self.get_cache("network_id") + # If the default network does not exist, create it. + if not network_id: try: network_body = { "network": { @@ -133,26 +175,39 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): network = neutron.create_network(body=network_body) except Exception as e: self.add_note( - "Error: '%s' while creating network: %s" - % (e, network_config.network_name) + ( + f"Error while creating network '{network_config.network_name}' " + f"for project '{self.project_id}' in region '{region}': {e}" + ), ) raise - self.set_cache("network_id", network["network"]["id"]) + network_id = network["network"]["id"] self.add_note( - "Network %s created for project %s" - % (network_config.network_name, self.project_id) + ( + f"Network '{network_config.network_name}' " + f"created for project '{self.project_id}' in region '{region}'" + ), ) else: self.add_note( - "Network %s already created for project %s" - % (network_config.network_name, self.project_id) + ( + f"Network '{network_config.network_name}' " + f"already created for project '{self.project_id}' " + f"in region '{region}'" + ), ) + self.set_cache(f"network_id{cache_suffix}", network_id) - if not self.get_cache("subnet_id"): + subnet_id = self.get_cache(f"subnet_id{cache_suffix}") + # NOTE(callumdickinson): Backwards compatibility with older tasks. + if not subnet_id and is_default_region: + subnet_id = self.get_cache("subnet_id") + # If the default subnet does not exist, create it. + if not subnet_id: try: subnet_body = { "subnet": { - "network_id": self.get_cache("network_id"), + "network_id": network_id, "ip_version": 4, "tenant_id": self.project_id, "dns_nameservers": network_config.dns_nameservers, @@ -161,16 +216,38 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): } subnet = neutron.create_subnet(body=subnet_body) except Exception as e: - self.add_note("Error: '%s' while creating subnet" % e) + self.add_note( + ( + "Error while creating subnet " + f"in network '{network_config.network_name}' " + f"for project '{self.project_id}' in region '{region}': {e}" + ), + ) raise - self.set_cache("subnet_id", subnet["subnet"]["id"]) - self.add_note("Subnet created for network %s" % network_config.network_name) + subnet_id = subnet["subnet"]["id"] + self.add_note( + ( + f"Subnet '{subnet_id}' created " + f"in network '{network_config.network_name}' " + f"created for project '{self.project_id}' in region '{region}'" + ), + ) else: self.add_note( - "Subnet already created for network %s" % network_config.network_name + ( + f"Subnet '{subnet_id}' already created " + f"in network '{network_config.network_name}' " + f"created for project '{self.project_id}' in region '{region}'" + ), ) + self.set_cache(f"subnet_id{cache_suffix}", subnet_id) - if not self.get_cache("router_id"): + router_id = self.get_cache(f"router_id{cache_suffix}") + # NOTE(callumdickinson): Backwards compatibility with older tasks. + if not router_id and is_default_region: + router_id = self.get_cache("router_id") + # If the default router does not exist, create it. + if not router_id: try: router_body = { "router": { @@ -185,28 +262,70 @@ class NewDefaultNetworkAction(BaseAction, ProjectMixin): router = neutron.create_router(body=router_body) except Exception as e: self.add_note( - "Error: '%s' while creating router: %s" - % (e, network_config.router_name) + ( + f"Error while creating router '{network_config.router_name}' " + f"for project '{self.project_id}' in region '{region}': {e}" + ), ) raise - self.set_cache("router_id", router["router"]["id"]) - self.add_note("Router created for project %s" % self.project_id) + router_id = router["router"]["id"] + self.add_note( + ( + f"Router '{network_config.router_name}' " + f"created for project '{self.project_id}' in region '{region}'" + ), + ) else: - self.add_note("Router already created for project %s" % self.project_id) + self.add_note( + ( + f"Router '{network_config.router_name}' " + f"already created for project '{self.project_id}' " + f"in region '{region}'" + ), + ) + self.set_cache(f"router_id{cache_suffix}", router_id) - if not self.get_cache("port_id"): + port_id = self.get_cache(f"port_id{cache_suffix}") + # NOTE(callumdickinson): Backwards compatibility with older tasks. + if not port_id and is_default_region: + port_id = self.get_cache("port_id") + # If the subnet port on the default router does not exist, create it. + if not port_id: try: - interface_body = {"subnet_id": self.get_cache("subnet_id")} - interface = neutron.add_interface_router( - self.get_cache("router_id"), body=interface_body - ) + interface_body = {"subnet_id": subnet_id} + interface = neutron.add_interface_router(router_id, body=interface_body) except Exception as e: - self.add_note("Error: '%s' while attaching interface" % e) + self.add_note( + ( + "Error while adding interface " + f"for network '{network_config.network_name}' " + f"to router '{network_config.router_name}' " + f"for project '{self.project_id}' " + f"in region '{region}': {e}" + ), + ) raise - self.set_cache("port_id", interface["port_id"]) - self.add_note("Interface added to router for subnet") + port_id = interface["port_id"] + self.add_note( + ( + f"Interface '{interface['port_id']}' " + f"for network '{network_config.network_name}' " + f"added to router '{network_config.router_name}' " + f"for project '{self.project_id}' " + f"in region '{region}'" + ), + ) else: - self.add_note("Interface added to router for project %s" % self.project_id) + self.add_note( + ( + f"Interface '{port_id}' " + f"for network '{network_config.network_name}' " + f"already added to router '{network_config.router_name}' " + f"for project '{self.project_id}' " + f"in region '{region}'" + ), + ) + self.set_cache(f"port_id{cache_suffix}", port_id) def _prepare(self): # Note: Do we need to get this from cache? it is a required setting diff --git a/adjutant/actions/v1/tests/test_resource_actions.py b/adjutant/actions/v1/tests/test_resource_actions.py index 4aa5621..84dd135 100644 --- a/adjutant/actions/v1/tests/test_resource_actions.py +++ b/adjutant/actions/v1/tests/test_resource_actions.py @@ -124,10 +124,10 @@ class ProjectSetupActionTests(AdjutantTestCase): self.assertEqual( action.action.cache, { - "network_id": "net_id_0", - "port_id": "port_id_3", - "router_id": "router_id_2", - "subnet_id": "subnet_id_1", + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", }, ) @@ -142,6 +142,93 @@ class ProjectSetupActionTests(AdjutantTestCase): len(neutron_cache["RegionOne"]["test_project_id"]["subnets"]), 1 ) + def test_network_setup_old_cache_compatibility(self): + """ + Check that a task with old-tyle cache values defined is compatible + with the new implementation. + """ + setup_neutron_cache("RegionOne", "test_project_id") + + global neutron_cache + + task = Task.objects.create( + keystone_user={"roles": ["admin"], "project_id": "test_project_id"} + ) + + project = mock.Mock() + project.id = "test_project_id" + project.name = "test_project" + project.domain = "default" + project.roles = {} + + setup_identity_cache(projects=[project]) + + data = { + "setup_network": True, + "region": "RegionOne", + "project_id": "test_project_id", + } + + action = NewDefaultNetworkAction(data, task=task, order=1) + + neutron_cache[data["region"]][project.id]["networks"]["net_id_0"] = { + "name": "somenetwork", + "tenant_id": project.id, + "admin_state_up": True, + } + neutron_cache[data["region"]][project.id]["subnets"]["subnet_id_1"] = { + "network_id": "net_id_0", + "ip_version": 4, + "tenant_id": project.id, + "dns_nameservers": ["193.168.1.2", "193.168.1.3"], + "cidr": "192.168.1.0/24", + } + neutron_cache[data["region"]][project.id]["routers"]["router_id_2"] = { + "name": "somerouter", + "external_gateway_info": { + "network_id": "3cb50f61-5bce-4c03-96e6-8e262e12bb35", + }, + "tenant_id": project.id, + "admin_state_up": True, + } + neutron_cache[data["region"]]["i"] = 4 + action.action.cache = { + "network_id": "net_id_0", + "port_id": "port_id_3", + "router_id": "router_id_2", + "subnet_id": "subnet_id_1", + } + + action.prepare() + self.assertEqual(action.valid, True) + + action.approve() + self.assertEqual(action.valid, True) + + self.assertEqual( + action.action.cache, + { + "network_id": "net_id_0", + "port_id": "port_id_3", + "router_id": "router_id_2", + "subnet_id": "subnet_id_1", + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", + }, + ) + + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["networks"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["routers"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["subnets"]), 1 + ) + def test_network_setup_no_setup(self): """ Told not to setup, should do nothing. @@ -186,6 +273,164 @@ class ProjectSetupActionTests(AdjutantTestCase): len(neutron_cache["RegionOne"]["test_project_id"]["subnets"]), 0 ) + @conf_utils.modify_conf( + CONF, + operations={ + ( + "adjutant.workflow.action_defaults.NewDefaultNetworkAction" + ".create_in_regions" + ): [ + {"operation": "override", "value": ["RegionOne", "RegionTwo"]}, + ], + }, + ) + def test_network_setup_create_in_regions(self): + """ + Check that all regions specified in 'create_in_regions' + have a default network created. + """ + setup_neutron_cache("RegionOne", "test_project_id") + setup_neutron_cache("RegionTwo", "test_project_id") + + task = Task.objects.create( + keystone_user={"roles": ["admin"], "project_id": "test_project_id"} + ) + + project = mock.Mock() + project.id = "test_project_id" + project.name = "test_project" + project.domain = "default" + project.roles = {} + + setup_identity_cache(projects=[project]) + + data = { + "setup_network": True, + "region": "RegionOne", + "project_id": "test_project_id", + } + + action = NewDefaultNetworkAction(data, task=task, order=1) + + action.prepare() + self.assertEqual(action.valid, True) + + action.approve() + self.assertEqual(action.valid, True) + + self.assertEqual( + action.action.cache, + { + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", + "network_id_RegionTwo": "net_id_0", + "port_id_RegionTwo": "port_id_3", + "router_id_RegionTwo": "router_id_2", + "subnet_id_RegionTwo": "subnet_id_1", + }, + ) + + global neutron_cache + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["networks"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["routers"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["subnets"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["networks"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["routers"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["subnets"]), 1 + ) + + @conf_utils.modify_conf( + CONF, + operations={ + ( + "adjutant.workflow.action_defaults.NewDefaultNetworkAction" + ".create_in_all_regions" + ): [ + {"operation": "override", "value": True}, + ], + }, + ) + def test_network_setup_create_in_all_regions(self): + """ + Check that all regions have a default network created + when 'create_in_all_regions' is set to True. + """ + setup_neutron_cache("RegionOne", "test_project_id") + setup_neutron_cache("RegionTwo", "test_project_id") + + task = Task.objects.create( + keystone_user={"roles": ["admin"], "project_id": "test_project_id"} + ) + + project = mock.Mock() + project.id = "test_project_id" + project.name = "test_project" + project.domain = "default" + project.roles = {} + + setup_identity_cache(projects=[project]) + + data = { + "setup_network": True, + "region": "RegionOne", + "project_id": "test_project_id", + } + + action = NewDefaultNetworkAction(data, task=task, order=1) + + action.prepare() + self.assertEqual(action.valid, True) + + action.approve() + self.assertEqual(action.valid, True) + + self.assertEqual( + action.action.cache, + { + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", + "network_id_RegionTwo": "net_id_0", + "port_id_RegionTwo": "port_id_3", + "router_id_RegionTwo": "router_id_2", + "subnet_id_RegionTwo": "subnet_id_1", + }, + ) + + global neutron_cache + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["networks"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["routers"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionOne"]["test_project_id"]["subnets"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["networks"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["routers"]), 1 + ) + self.assertEqual( + len(neutron_cache["RegionTwo"]["test_project_id"]["subnets"]), 1 + ) + def test_network_setup_fail(self): """ Should fail, but on re_approve will continue where it left off. @@ -224,7 +469,11 @@ class ProjectSetupActionTests(AdjutantTestCase): pass self.assertEqual( - action.action.cache, {"network_id": "net_id_0", "subnet_id": "subnet_id_1"} + action.action.cache, + { + "network_id_RegionOne": "net_id_0", + "subnet_id_RegionOne": "subnet_id_1", + }, ) self.assertEqual( @@ -244,10 +493,10 @@ class ProjectSetupActionTests(AdjutantTestCase): self.assertEqual( action.action.cache, { - "network_id": "net_id_0", - "port_id": "port_id_3", - "router_id": "router_id_2", - "subnet_id": "subnet_id_1", + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", }, ) @@ -297,10 +546,10 @@ class ProjectSetupActionTests(AdjutantTestCase): self.assertEqual( action.action.cache, { - "network_id": "net_id_0", - "port_id": "port_id_3", - "router_id": "router_id_2", - "subnet_id": "subnet_id_1", + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", }, ) @@ -435,7 +684,11 @@ class ProjectSetupActionTests(AdjutantTestCase): pass self.assertEqual( - action.action.cache, {"network_id": "net_id_0", "subnet_id": "subnet_id_1"} + action.action.cache, + { + "network_id_RegionOne": "net_id_0", + "subnet_id_RegionOne": "subnet_id_1", + }, ) self.assertEqual( @@ -455,10 +708,10 @@ class ProjectSetupActionTests(AdjutantTestCase): self.assertEqual( action.action.cache, { - "network_id": "net_id_0", - "port_id": "port_id_3", - "router_id": "router_id_2", - "subnet_id": "subnet_id_1", + "network_id_RegionOne": "net_id_0", + "port_id_RegionOne": "port_id_3", + "router_id_RegionOne": "router_id_2", + "subnet_id_RegionOne": "subnet_id_1", }, ) diff --git a/adjutant/common/tests/fake_clients.py b/adjutant/common/tests/fake_clients.py index c53dfbc..750ff3e 100644 --- a/adjutant/common/tests/fake_clients.py +++ b/adjutant/common/tests/fake_clients.py @@ -575,13 +575,13 @@ class FakeNeutronClient(object): project_id = body["network"]["tenant_id"] net = { "network": { - "id": "net_id_%s" % neutron_cache["RegionOne"]["i"], + "id": "net_id_%s" % neutron_cache[self.region]["i"], "body": body, } } net_id = net["network"]["id"] - neutron_cache["RegionOne"][project_id]["networks"][net_id] = net - neutron_cache["RegionOne"]["i"] += 1 + neutron_cache[self.region][project_id]["networks"][net_id] = net + neutron_cache[self.region]["i"] += 1 return net def create_subnet(self, body): @@ -589,13 +589,13 @@ class FakeNeutronClient(object): project_id = body["subnet"]["tenant_id"] subnet = { "subnet": { - "id": "subnet_id_%s" % neutron_cache["RegionOne"]["i"], + "id": "subnet_id_%s" % neutron_cache[self.region]["i"], "body": body, } } sub_id = subnet["subnet"]["id"] - neutron_cache["RegionOne"][project_id]["subnets"][sub_id] = subnet - neutron_cache["RegionOne"]["i"] += 1 + neutron_cache[self.region][project_id]["subnets"][sub_id] = subnet + neutron_cache[self.region]["i"] += 1 return subnet def create_router(self, body): @@ -603,19 +603,19 @@ class FakeNeutronClient(object): project_id = body["router"]["tenant_id"] router = { "router": { - "id": "router_id_%s" % neutron_cache["RegionOne"]["i"], + "id": "router_id_%s" % neutron_cache[self.region]["i"], "body": body, } } router_id = router["router"]["id"] - neutron_cache["RegionOne"][project_id]["routers"][router_id] = router - neutron_cache["RegionOne"]["i"] += 1 + neutron_cache[self.region][project_id]["routers"][router_id] = router + neutron_cache[self.region]["i"] += 1 return router def add_interface_router(self, router_id, body): global neutron_cache - port_id = "port_id_%s" % neutron_cache["RegionOne"]["i"] - neutron_cache["RegionOne"]["i"] += 1 + port_id = "port_id_%s" % neutron_cache[self.region]["i"] + neutron_cache[self.region]["i"] += 1 interface = { "port_id": port_id, "id": router_id, diff --git a/releasenotes/notes/multiple-default-networks-5a89766d377b06d2.yaml b/releasenotes/notes/multiple-default-networks-5a89766d377b06d2.yaml new file mode 100644 index 0000000..16cb1b3 --- /dev/null +++ b/releasenotes/notes/multiple-default-networks-5a89766d377b06d2.yaml @@ -0,0 +1,13 @@ +--- +features: + - | + The ``create_in_all_regions`` option has been added to + ``NewDefaultNetworkAction`` and ``NewProjectDefaultNetworkAction``. + When set to ``true``, default networks and routers will be created in + all enabled regions, instead of just the default region. + - | + The ``create_in_regions`` option has been added to + ``NewDefaultNetworkAction`` and ``NewProjectDefaultNetworkAction``. + This defines a list of regions to create default networks and routers in + for new sign ups, instead of the default region + (or all regions, if ``create_in_all_regions`` is ``true``).