From d714ccec0537c6dd4176f50ee707f0c784c0d5a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lorenzo=20Fundar=C3=B3?= Date: Thu, 17 Dec 2020 11:22:12 +0100 Subject: [PATCH] Fix fetch of existing records from Google DNS (#8521) * Fix fetch of existing records from Google DNS There has been many complaints regarding `certbot_dns_google` plugin failing with: * HTTP 412 - Precondition not met * HTTP 409 - Conflict See #6036. This PR fixes that situation. The bug lies on how we fetch the TXT records from google. For large amount of records the Google API paginates the result but we ignore the subsequent pages and assume that if the record is not in the first response then it doesn't exist. This leads to either HTTP 409, or HTTP 412 or both. In this PR we leverage the use of filters on the API to get exactly the records we are looking for. Apart from fixing the problem stated above, it has the extra benefit of making the process faster by reducing the amount of API calls and it doesn't require us to handle any pagination logic * Explain changes on CHANGELOG * Edit AUTHORS.md * make execute static * Update certbot/CHANGELOG.md Being more specific for which plugin this fix bug is meant for. Co-authored-by: alexzorin * Fix if expression to be more python-idiomatic Co-authored-by: alexzorin * Sort AUTHORS.md * Simplify tests Make rrs_mock modeling simpler and refactor * Revert "Simplify tests" This reverts commit 9de9623ba7466bf76a7d9075d4eba6980cbe0b62. * Reimplement conditional mock We still want to use a conditional mock by make it more simple to understand by using MagicMock. * Revert "Sort AUTHORS.md" This reverts commit b3aa35bcf16f393b2e08ca22278d4c0cfe6c7282. * Add name in AUTHORS.md Co-authored-by: alexzorin --- AUTHORS.md | 1 + .../_internal/dns_google.py | 9 +++---- certbot-dns-google/tests/dns_google_test.py | 27 +++++++++++++------ certbot/CHANGELOG.md | 1 + 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/AUTHORS.md b/AUTHORS.md index f76c323a5..ff5c61613 100644 --- a/AUTHORS.md +++ b/AUTHORS.md @@ -149,6 +149,7 @@ Authors * [Lior Sabag](https://github.com/liorsbg) * [Lipis](https://github.com/lipis) * [lord63](https://github.com/lord63) +* [Lorenzo Fundaró](https://github.com/lfundaro) * [Luca Beltrame](https://github.com/lbeltrame) * [Luca Ebach](https://github.com/lucebac) * [Luca Olivetti](https://github.com/olivluca) diff --git a/certbot-dns-google/certbot_dns_google/_internal/dns_google.py b/certbot-dns-google/certbot_dns_google/_internal/dns_google.py index 1bd3468da..cd4b2d2d5 100644 --- a/certbot-dns-google/certbot_dns_google/_internal/dns_google.py +++ b/certbot-dns-google/certbot_dns_google/_internal/dns_google.py @@ -240,9 +240,10 @@ class _GoogleClient(object): """ rrs_request = self.dns.resourceRecordSets() - request = rrs_request.list(managedZone=zone_id, project=self.project_id) # Add dot as the API returns absolute domains record_name += "." + request = rrs_request.list(project=self.project_id, managedZone=zone_id, name=record_name, + type="TXT") try: response = request.execute() except googleapiclient_errors.Error: @@ -250,10 +251,8 @@ class _GoogleClient(object): "requesting a wildcard certificate, this might not work.") logger.debug("Error was:", exc_info=True) else: - if response: - for rr in response["rrsets"]: - if rr["name"] == record_name and rr["type"] == "TXT": - return rr["rrdatas"] + if response and response["rrsets"]: + return response["rrsets"][0]["rrdatas"] return None def _find_managed_zone_id(self, domain): diff --git a/certbot-dns-google/tests/dns_google_test.py b/certbot-dns-google/tests/dns_google_test.py index 40002f143..bcb6bb80f 100644 --- a/certbot-dns-google/tests/dns_google_test.py +++ b/certbot-dns-google/tests/dns_google_test.py @@ -70,7 +70,7 @@ class GoogleClientTest(unittest.TestCase): zone = "ZONE_ID" change = "an-id" - def _setUp_client_with_mock(self, zone_request_side_effect): + def _setUp_client_with_mock(self, zone_request_side_effect, rrs_list_side_effect=None): from certbot_dns_google._internal.dns_google import _GoogleClient pwd = os.path.dirname(__file__) @@ -86,9 +86,16 @@ class GoogleClientTest(unittest.TestCase): mock_mz.list.return_value.execute.side_effect = zone_request_side_effect mock_rrs = mock.MagicMock() - rrsets = {"rrsets": [{"name": "_acme-challenge.example.org.", "type": "TXT", + def rrs_list(project=None, managedZone=None, name=None, type=None): + response = {"rrsets": []} + if name == "_acme-challenge.example.org.": + response = {"rrsets": [{"name": "_acme-challenge.example.org.", "type": "TXT", "rrdatas": ["\"example-txt-contents\""]}]} - mock_rrs.list.return_value.execute.return_value = rrsets + mock_return = mock.MagicMock() + mock_return.execute.return_value = response + mock_return.execute.side_effect = rrs_list_side_effect + return mock_return + mock_rrs.list.side_effect = rrs_list mock_changes = mock.MagicMock() client.dns.managedZones = mock.MagicMock(return_value=mock_mz) @@ -287,12 +294,19 @@ class GoogleClientTest(unittest.TestCase): @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') @mock.patch('certbot_dns_google._internal.dns_google.open', mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) - def test_get_existing(self, unused_credential_mock): + def test_get_existing_found(self, unused_credential_mock): client, unused_changes = self._setUp_client_with_mock( [{'managedZones': [{'id': self.zone}]}]) # Record name mocked in setUp found = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") self.assertEqual(found, ["\"example-txt-contents\""]) + + @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name') + @mock.patch('certbot_dns_google._internal.dns_google.open', + mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) + def test_get_existing_not_found(self, unused_credential_mock): + client, unused_changes = self._setUp_client_with_mock( + [{'managedZones': [{'id': self.zone}]}]) not_found = client.get_existing_txt_rrset(self.zone, "nonexistent.tld") self.assertEqual(not_found, None) @@ -301,10 +315,7 @@ class GoogleClientTest(unittest.TestCase): mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True) def test_get_existing_fallback(self, unused_credential_mock): client, unused_changes = self._setUp_client_with_mock( - [{'managedZones': [{'id': self.zone}]}]) - mock_execute = client.dns.resourceRecordSets.return_value.list.return_value.execute - mock_execute.side_effect = API_ERROR - + [{'managedZones': [{'id': self.zone}]}], API_ERROR) rrset = client.get_existing_txt_rrset(self.zone, "_acme-challenge.example.org") self.assertFalse(rrset) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index e4f4eda51..8bfee52be 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -22,6 +22,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * The Certbot snap no longer loads packages installed via `pip install --user`. This was unintended and DNS plugins should be installed via `snap` instead. +* `certbot-dns-google` would sometimes crash with HTTP 409/412 errors when used with very large zones (#6036) More details about these changes can be found on our GitHub repo.