From 8fea6838634b106f883a72d203c2745654d809bc Mon Sep 17 00:00:00 2001 From: Erik Olof Gunnar Andersson Date: Wed, 5 Jan 2022 21:35:14 -0800 Subject: Fix duplicate zone when creating ptr records This fixes a race-condition when creating multiple PTR records under the same zone. There is a brief window when creating two identical zones can cause an error. This adds a fallback that should prevent the error. I also added a threaded test that caught multiple additional bugs in this code. - Wrong find_recordset used caused the wrong exception to be thrown. - The transaction workflow would break error handling. Change-Id: Ia1194ab838c52d5d91cb1d26c4556c73b4f3a745 (cherry picked from commit 0c7d218ba103e8260322e40f76a49a8c92556bfe) --- designate/central/service.py | 40 +++++++++++++++++----------- designate/tests/__init__.py | 6 ++++- designate/tests/test_central/test_service.py | 28 +++++++++++++++++-- 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/designate/central/service.py b/designate/central/service.py index fbe880c2..a7dd7de4 100644 --- a/designate/central/service.py +++ b/designate/central/service.py @@ -2247,19 +2247,7 @@ class Service(service.RPCService): 'zonename': zone_name }) - email = cfg.CONF['service:central'].managed_resource_email - tenant_id = cfg.CONF['service:central'].managed_resource_tenant_id - - zone_values = { - 'type': 'PRIMARY', - 'name': zone_name, - 'email': email, - 'tenant_id': tenant_id - } - - zone = self.create_zone( - elevated_context, objects.Zone(**zone_values) - ) + zone = self._create_ptr_zone(elevated_context, zone_name) record_name = self.network_api.address_name(fip['address']) recordset_values = { @@ -2287,6 +2275,27 @@ class Service(service.RPCService): context, fip, record, zone=zone, recordset=recordset ) + def _create_ptr_zone(self, elevated_context, zone_name): + zone_values = { + 'type': 'PRIMARY', + 'name': zone_name, + 'email': cfg.CONF['service:central'].managed_resource_email, + 'tenant_id': cfg.CONF['service:central'].managed_resource_tenant_id + } + try: + zone = self.create_zone( + elevated_context, objects.Zone(**zone_values) + ) + except exceptions.DuplicateZone: + # NOTE(eandersson): This code is prone to race conditions, and + # it does not hurt to try to handle this if it + # fails. + zone = self.storage.find_zone( + elevated_context, {'name': zone_name} + ) + + return zone + def _unset_floatingip_reverse(self, context, region, floatingip_id): """ Unset the FloatingIP PTR record based on the @@ -2367,6 +2376,7 @@ class Service(service.RPCService): fips.append(fip_ptr) return fips + @transaction def _delete_ptr_record(self, context, record): try: recordset = self.get_recordset( @@ -2393,10 +2403,11 @@ class Service(service.RPCService): except exceptions.RecordSetNotFound: pass + @transaction def _replace_or_create_ptr_recordset(self, context, record, zone_id, name, type, ttl=None): try: - recordset = self.find_recordset(context, { + recordset = self.storage.find_recordset(context, { 'zone_id': zone_id, 'name': name, 'type': type, @@ -2422,7 +2433,6 @@ class Service(service.RPCService): return recordset @rpc.expected_exceptions() - @transaction def update_floatingip(self, context, region, floatingip_id, values): """ We strictly see if values['ptrdname'] is str or None and set / unset diff --git a/designate/tests/__init__.py b/designate/tests/__init__.py index 24c7beaa..b560adcd 100644 --- a/designate/tests/__init__.py +++ b/designate/tests/__init__.py @@ -212,7 +212,11 @@ class TestCase(base.BaseTestCase): ptr_fixtures = [ {'ptrdname': 'srv1.example.com.'}, - {'ptrdname': 'srv1.example.net.'} + {'ptrdname': 'srv1.example.net.'}, + {'ptrdname': 'srv2.example.com.'}, + {'ptrdname': 'srv3.example.com.'}, + {'ptrdname': 'srv4.example.com.'}, + {'ptrdname': 'srv5.example.com.'}, ] blacklist_fixtures = [{ diff --git a/designate/tests/test_central/test_service.py b/designate/tests/test_central/test_service.py index 8307f348..abc98aaf 100644 --- a/designate/tests/test_central/test_service.py +++ b/designate/tests/test_central/test_service.py @@ -15,10 +15,12 @@ # License for the specific language governing permissions and limitations # under the License. -import datetime +from collections import namedtuple +from concurrent import futures import copy +import datetime +import futurist import random -from collections import namedtuple from unittest import mock import testtools @@ -2586,6 +2588,28 @@ class CentralServiceTest(CentralTestCase): self.assertIsNone(fip_ptr['description']) self.assertIsNotNone(fip_ptr['ttl']) + def test_set_floatingip_multiple_requests(self): + context = self.get_context() + + def update_floatingip(fixture): + fip = self.network_api.fake.allocate_floatingip(context.project_id) + return self.central_service.update_floatingip( + context, fip['region'], fip['id'], fixture + ) + + with futurist.GreenThreadPoolExecutor() as executor: + results = [] + for fixture in [0, 2, 3, 4, 5]: + results.append(executor.submit( + update_floatingip, fixture=self.get_ptr_fixture(fixture) + )) + for future in futures.as_completed(results): + self.assertTrue(future.result()) + + fips = self.central_service.list_floatingips(context) + + self.assertEqual(5, len(fips)) + def test_set_floatingip_no_managed_resource_tenant_id(self): context = self.get_context(project_id='a') -- cgit v1.2.1