Allow Google DNS plugin to write multiple TXT record values (#5652)

* Allow Google DNS plugin to write multiple TXT record values in same resourcerecord

* Atomic updates

* Split rrsets request

certbot-dns-google/certbot_dns_google/dns_google.py

@@ -107,6 +107,15 @@ class _GoogleClient(object):
 
         zone_id = self._find_managed_zone_id(domain)
 
+        record_contents = self.get_existing_txt_rrset(zone_id, record_name)
+        add_records = record_contents[:]
+
+        if "\""+record_content+"\"" in record_contents:
+            # The process was interrupted previously and validation token exists
+            return
+
+        add_records.append(record_content)
+
         data = {
             "kind": "dns#change",
             "additions": [
@@ -114,12 +123,24 @@ class _GoogleClient(object):
                     "kind": "dns#resourceRecordSet",
                     "type": "TXT",
                     "name": record_name + ".",
-                    "rrdatas": [record_content, ],
+                    "rrdatas": add_records,
                     "ttl": record_ttl,
                 },
             ],
         }
 
+        if record_contents:
+            # We need to remove old records in the same request
+            data["deletions"] = [
+                {
+                    "kind": "dns#resourceRecordSet",
+                    "type": "TXT",
+                    "name": record_name + ".",
+                    "rrdatas": record_contents,
+                    "ttl": record_ttl,
+                },
+            ]
+
         changes = self.dns.changes()  # changes | pylint: disable=no-member
 
         try:
@@ -154,6 +175,8 @@ class _GoogleClient(object):
             logger.warn('Error finding zone. Skipping cleanup.')
             return
 
+        record_contents = self.get_existing_txt_rrset(zone_id, record_name)
+
         data = {
             "kind": "dns#change",
             "deletions": [
@@ -161,12 +184,26 @@ class _GoogleClient(object):
                     "kind": "dns#resourceRecordSet",
                     "type": "TXT",
                     "name": record_name + ".",
-                    "rrdatas": [record_content, ],
+                    "rrdatas": record_contents,
                     "ttl": record_ttl,
                 },
             ],
         }
 
+        # Remove the record being deleted from the list
+        readd_contents = [r for r in record_contents if r != "\"" + record_content + "\""]
+        if readd_contents:
+            # We need to remove old records in the same request
+            data["additions"] = [
+                {
+                    "kind": "dns#resourceRecordSet",
+                    "type": "TXT",
+                    "name": record_name + ".",
+                    "rrdatas": readd_contents,
+                    "ttl": record_ttl,
+                },
+            ]
+
         changes = self.dns.changes()  # changes | pylint: disable=no-member
 
         try:
@@ -175,6 +212,28 @@ class _GoogleClient(object):
         except googleapiclient_errors.Error as e:
             logger.warn('Encountered error deleting TXT record: %s', e)
 
+    def get_existing_txt_rrset(self, zone_id, record_name):
+        """
+        Get existing TXT records from the RRset for the record name.
+
+        :param str zone_id: The ID of the managed zone.
+        :param str record_name: The record name (typically beginning with '_acme-challenge.').
+
+        :returns: List of TXT record values
+        :rtype: `list` of `string`
+
+        """
+        rrs_request = self.dns.resourceRecordSets()  # pylint: disable=no-member
+        request = rrs_request.list(managedZone=zone_id, project=self.project_id)
+        response = request.execute()
+        # Add dot as the API returns absolute domains
+        record_name += "."
+        if response:
+            for rr in response["rrsets"]:
+                if rr["name"] == record_name and rr["type"] == "TXT":
+                    return rr["rrdatas"]
+        return []
+
     def _find_managed_zone_id(self, domain):
         """
         Find the managed zone for a given domain.

certbot-dns-google/certbot_dns_google/dns_google_test.py

@@ -74,10 +74,15 @@ class GoogleClientTest(unittest.TestCase):
         mock_mz = mock.MagicMock()
         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",
+                              "rrdatas": ["\"example-txt-contents\""]}]}
+        mock_rrs.list.return_value.execute.return_value = rrsets
         mock_changes = mock.MagicMock()
 
         client.dns.managedZones = mock.MagicMock(return_value=mock_mz)
         client.dns.changes = mock.MagicMock(return_value=mock_changes)
+        client.dns.resourceRecordSets = mock.MagicMock(return_value=mock_rrs)
 
         return client, mock_changes
 
@@ -137,6 +142,30 @@ class GoogleClientTest(unittest.TestCase):
                                             managedZone=self.zone,
                                             project=PROJECT_ID)
 
+    @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name')
+    @mock.patch('certbot_dns_google.dns_google.open',
+                mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True)
+    def test_add_txt_record_delete_old(self, unused_credential_mock):
+        client, changes = self._setUp_client_with_mock(
+            [{'managedZones': [{'id': self.zone}]}])
+        mock_get_rrs = "certbot_dns_google.dns_google._GoogleClient.get_existing_txt_rrset"
+        with mock.patch(mock_get_rrs) as mock_rrs:
+            mock_rrs.return_value = ["sample-txt-contents"]
+            client.add_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl)
+            self.assertTrue(changes.create.called)
+            self.assertTrue("sample-txt-contents" in
+                changes.create.call_args_list[0][1]["body"]["deletions"][0]["rrdatas"])
+
+    @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name')
+    @mock.patch('certbot_dns_google.dns_google.open',
+                mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True)
+    def test_add_txt_record_noop(self, unused_credential_mock):
+        client, changes = self._setUp_client_with_mock(
+            [{'managedZones': [{'id': self.zone}]}])
+        client.add_txt_record(DOMAIN, "_acme-challenge.example.org",
+                              "example-txt-contents", self.record_ttl)
+        self.assertFalse(changes.create.called)
+
     @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name')
     @mock.patch('certbot_dns_google.dns_google.open',
                 mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True)
@@ -172,7 +201,12 @@ class GoogleClientTest(unittest.TestCase):
     def test_del_txt_record(self, unused_credential_mock):
         client, changes = self._setUp_client_with_mock([{'managedZones': [{'id': self.zone}]}])
 
-        client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl)
+        mock_get_rrs = "certbot_dns_google.dns_google._GoogleClient.get_existing_txt_rrset"
+        with mock.patch(mock_get_rrs) as mock_rrs:
+            mock_rrs.return_value = ["\"sample-txt-contents\"",
+                                     "\"example-txt-contents\""]
+            client.del_txt_record(DOMAIN, "_acme-challenge.example.org",
+                                "example-txt-contents", self.record_ttl)
 
         expected_body = {
             "kind": "dns#change",
@@ -180,8 +214,17 @@ class GoogleClientTest(unittest.TestCase):
                 {
                     "kind": "dns#resourceRecordSet",
                     "type": "TXT",
-                    "name": self.record_name + ".",
-                    "rrdatas": [self.record_content, ],
+                    "name": "_acme-challenge.example.org.",
+                    "rrdatas": ["\"sample-txt-contents\"", "\"example-txt-contents\""],
+                    "ttl": self.record_ttl,
+                },
+            ],
+            "additions": [
+                {
+                    "kind": "dns#resourceRecordSet",
+                    "type": "TXT",
+                    "name": "_acme-challenge.example.org.",
+                    "rrdatas": ["\"sample-txt-contents\"", ],
                     "ttl": self.record_ttl,
                 },
             ],
@@ -217,6 +260,18 @@ class GoogleClientTest(unittest.TestCase):
 
         client.del_txt_record(DOMAIN, self.record_name, self.record_content, self.record_ttl)
 
+    @mock.patch('oauth2client.service_account.ServiceAccountCredentials.from_json_keyfile_name')
+    @mock.patch('certbot_dns_google.dns_google.open',
+                mock.mock_open(read_data='{"project_id": "' + PROJECT_ID + '"}'), create=True)
+    def test_get_existing(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.assertEquals(found, ["\"example-txt-contents\""])
+        not_found = client.get_existing_txt_rrset(self.zone, "nonexistent.tld")
+        self.assertEquals(not_found, [])
+
     def test_get_project_id(self):
         from certbot_dns_google.dns_google import _GoogleClient