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 <alex@zor.io>

* Fix if expression to be more python-idiomatic

Co-authored-by: alexzorin <alex@zor.io>

* Sort AUTHORS.md

* Simplify tests

Make rrs_mock modeling simpler and refactor

* Revert "Simplify tests"

This reverts commit 9de9623ba7.

* 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 b3aa35bcf1.

* Add name in AUTHORS.md

Co-authored-by: alexzorin <alex@zor.io>

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)

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):

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)
 

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.