Route53: fix deleting records which are part of a multi value record set

CHANGES.rst

@@ -99,7 +99,14 @@ Storage
   [Sanjay Santhanam - @Sanjays2402]
 
 DNS
-~~~~
+
+- [Route53] Fix ``delete_record`` failing with ``RecordDoesNotExistError`` for
+  records which are part of a multi value record set (e.g. MX). Route53 only
+  accepts a DELETE changeset which lists every value in the record set, so the
+  values of the other records in the set are now included, mirroring how
+  ``update_record`` already handles multi value records.
+  (GITHUB-1831)
+  [Sanjay Santhanam - @Sanjays2402]
 
 - [Cloudflare] Fix ``list_record_types`` advertising the unsupported ``URL``
   record type instead of ``LOC``. The ``RECORD_TYPE_MAP`` entry was keyed on

libcloud/dns/drivers/route53.py

@@ -199,7 +199,39 @@ def create_record(self, name, zone, type, data, extra=None):
             extra=extra,
         )
 
+    def _with_record_set_metadata(self, record):
+        # ``_multi_value`` / ``_other_records`` are attached by ``_to_records``,
+        # so records which did not come from ``list_records`` / ``get_record``
+        # (e.g. the ones returned by ``create_record`` or
+        # ``ex_create_multi_value_record``, or user constructed ones) carry no
+        # information about the rest of their record set. Re-fetch the record
+        # set in that case so multi value updates and deletes work regardless
+        # of how the record was obtained.
+
+        if "_multi_value" in record.extra:
+            return record
+
+        try:
+            fetched = self.list_records(zone=record.zone)
+        except Exception:
+            return record
+
+        for candidate in fetched:
+            if (
+                candidate.name == record.name
+                and candidate.type == record.type
+                and candidate.data == record.data
+            ):
+                extra = copy.deepcopy(candidate.extra)
+                extra.update({k: v for k, v in record.extra.items() if not k.startswith("_")})
+                record.extra = extra
+
+                break
+
+        return record
+
     def update_record(self, record, name=None, type=None, data=None, extra=None):
+        record = self._with_record_set_metadata(record)
         name = name or record.name
         type = type or record.type
         extra = extra or record.extra
@@ -236,14 +268,72 @@ def update_record(self, record, name=None, type=None, data=None, extra=None):
 
     def delete_record(self, record):
         try:
-            r = record
-            batch = [("DELETE", r.name, r.type, r.data, r.extra)]
-            self._post_changeset(record.zone, batch)
+            r = self._with_record_set_metadata(record)
+
+            # Multiple value records need to be handled specially - Route53
+            # only accepts a DELETE for a record set which lists every value
+            # in that set, so values for the other records need to be sent
+            # as well.
+
+            if r.extra.get("_multi_value", False) and r.extra.get("_other_records", []):
+                self._delete_multi_value_record(record=r)
+            else:
+                batch = [("DELETE", r.name, r.type, r.data, r.extra)]
+                self._post_changeset(record.zone, batch)
         except InvalidChangeBatch:
             raise RecordDoesNotExistError(value="", driver=self, record_id=r.id)
 
         return True
 
+    def _delete_multi_value_record(self, record):
+        other_records = record.extra.get("_other_records", [])
+
+        attrs = {"xmlns": NAMESPACE}
+        changeset = ET.Element("ChangeResourceRecordSetsRequest", attrs)
+        batch = ET.SubElement(changeset, "ChangeBatch")
+        changes = ET.SubElement(batch, "Changes")
+
+        change = ET.SubElement(changes, "Change")
+        ET.SubElement(change, "Action").text = "DELETE"
+
+        rrs = ET.SubElement(change, "ResourceRecordSet")
+
+        if record.name:
+            record_name = record.name + "." + record.zone.domain
+        else:
+            record_name = record.zone.domain
+
+        ET.SubElement(rrs, "Name").text = record_name
+        ET.SubElement(rrs, "Type").text = self.RECORD_TYPE_MAP[record.type]
+        ET.SubElement(rrs, "TTL").text = str(record.extra.get("ttl", "0"))
+
+        rrecs = ET.SubElement(rrs, "ResourceRecords")
+
+        rrec = ET.SubElement(rrecs, "ResourceRecord")
+        ET.SubElement(rrec, "Value").text = self._to_record_value(record.data, record.extra)
+
+        for other_record in other_records:
+            rrec = ET.SubElement(rrecs, "ResourceRecord")
+            ET.SubElement(rrec, "Value").text = self._to_record_value(
+                other_record["data"], other_record.get("extra", {})
+            )
+
+        uri = API_ROOT + "hostedzone/" + record.zone.id + "/rrset"
+        data = ET.tostring(changeset)
+        self.connection.set_context({"zone_id": record.zone.id})
+        response = self.connection.request(uri, method="POST", data=data)
+
+        return response.status == httplib.OK
+
+    def _to_record_value(self, data, extra):
+        # "priority" is parsed out of the value by _to_record, so it needs to
+        # be put back to reconstruct the value Route53 stores.
+
+        if extra and "priority" in extra:
+            return "{} {}".format(extra["priority"], data)
+
+        return data
+
     def ex_create_multi_value_record(self, name, zone, type, data, extra=None):
         """
         Create a record with multiple values with a single call.

libcloud/test/dns/test_route53.py

@@ -13,10 +13,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import re
 import sys
 import unittest
 
 from libcloud.test import MockHttp
+from libcloud.dns.base import Record
 from libcloud.dns.types import RecordType, ZoneDoesNotExistError, RecordDoesNotExistError
 from libcloud.utils.py3 import httplib
 from libcloud.test.secrets import DNS_PARAMS_ROUTE53
@@ -284,6 +286,184 @@ def test_delete_record(self):
         status = self.driver.delete_record(record=record)
         self.assertTrue(status)
 
+    def test_delete_multi_value_record(self):
+        zone = self.driver.list_zones()[0]
+        records = [r for r in self.driver.list_records(zone=zone) if r.type == RecordType.MX]
+        record = records[0]
+
+        sent = {}
+        original_request = self.driver.connection.request
+
+        def record_request(uri, *args, **kwargs):
+            if kwargs.get("method") == "POST":
+                sent["data"] = kwargs.get("data")
+
+            return original_request(uri, *args, **kwargs)
+
+        self.driver.connection.request = record_request
+        status = self.driver.delete_record(record=record)
+        self.assertTrue(status)
+
+        data = sent["data"]
+
+        if not isinstance(data, str):
+            data = data.decode("utf-8")
+
+        # Route53 only accepts a DELETE which lists every value in the record
+        # set, so all the values need to be included in the changeset.
+        values = re.findall(r"<Value>(.*?)</Value>", data)
+        self.assertEqual(
+            values,
+            [
+                "1 ASPMX.L.GOOGLE.COM.",
+                "5 ALT1.ASPMX.L.GOOGLE.COM.",
+                "5 ALT2.ASPMX.L.GOOGLE.COM.",
+                "10 ASPMX2.GOOGLEMAIL.COM.",
+                "10 ASPMX3.GOOGLEMAIL.COM.",
+            ],
+        )
+
+    def test_delete_multi_value_record_without_record_set_metadata(self):
+        # Records which did not come from list_records()/get_record() (e.g. the
+        # ones returned by create_record()) carry no _multi_value metadata, so
+        # the record set has to be re-fetched for the DELETE to be valid.
+        zone = self.driver.list_zones()[0]
+        listed = [r for r in self.driver.list_records(zone=zone) if r.type == RecordType.MX][0]
+
+        record = Record(
+            id=listed.id,
+            name=listed.name,
+            type=listed.type,
+            data=listed.data,
+            zone=zone,
+            driver=self.driver,
+            ttl=listed.extra.get("ttl"),
+            extra={"ttl": listed.extra.get("ttl"), "priority": listed.extra.get("priority")},
+        )
+
+        sent = {}
+        original_request = self.driver.connection.request
+
+        def record_request(uri, *args, **kwargs):
+            if kwargs.get("method") == "POST":
+                sent["data"] = kwargs.get("data")
+
+            return original_request(uri, *args, **kwargs)
+
+        self.driver.connection.request = record_request
+        status = self.driver.delete_record(record=record)
+        self.assertTrue(status)
+
+        data = sent["data"]
+
+        if not isinstance(data, str):
+            data = data.decode("utf-8")
+
+        values = re.findall(r"<Value>(.*?)</Value>", data)
+        self.assertEqual(
+            sorted(values),
+            sorted(
+                [
+                    "1 ASPMX.L.GOOGLE.COM.",
+                    "5 ALT1.ASPMX.L.GOOGLE.COM.",
+                    "5 ALT2.ASPMX.L.GOOGLE.COM.",
+                    "10 ASPMX2.GOOGLEMAIL.COM.",
+                    "10 ASPMX3.GOOGLEMAIL.COM.",
+                ]
+            ),
+        )
+
+    def test_delete_multi_value_record_at_zone_apex(self):
+        # A record with an empty name (zone apex) must be deleted with the
+        # zone domain as the record set name.
+        zone = self.driver.list_zones()[0]
+        record = Record(
+            id="A:@",
+            name="",
+            type=RecordType.A,
+            data="1.2.3.4",
+            zone=zone,
+            driver=self.driver,
+            extra={
+                "_multi_value": True,
+                "_other_records": [{"data": "5.6.7.8", "extra": {}}],
+            },
+        )
+
+        sent = {}
+        original_request = self.driver.connection.request
+
+        def record_request(uri, *args, **kwargs):
+            if kwargs.get("method") == "POST":
+                sent["data"] = kwargs.get("data")
+
+            return original_request(uri, *args, **kwargs)
+
+        self.driver.connection.request = record_request
+        status = self.driver.delete_record(record=record)
+        self.assertTrue(status)
+
+        data = sent["data"]
+
+        if not isinstance(data, str):
+            data = data.decode("utf-8")
+
+        self.assertIn("<Name>%s</Name>" % zone.domain, data)
+
+    def test_with_record_set_metadata_list_records_failure(self):
+        # If the record set cannot be re-fetched, the record is used as-is.
+        zone = self.driver.list_zones()[0]
+        record = Record(
+            id="A:foo",
+            name="foo",
+            type=RecordType.A,
+            data="1.2.3.4",
+            zone=zone,
+            driver=self.driver,
+            extra={},
+        )
+
+        original_list_records = self.driver.list_records
+
+        def boom(*args, **kwargs):
+            raise Exception("boom")
+
+        self.driver.list_records = boom
+
+        try:
+            result = self.driver._with_record_set_metadata(record)
+        finally:
+            self.driver.list_records = original_list_records
+
+        self.assertIs(result, record)
+
+    def test_with_record_set_metadata_no_matching_record(self):
+        # If no record in the re-fetched set matches, the record is used
+        # as-is.
+        zone = self.driver.list_zones()[0]
+        record = Record(
+            id="A:no-such-record",
+            name="no-such-record",
+            type=RecordType.A,
+            data="9.9.9.9",
+            zone=zone,
+            driver=self.driver,
+            extra={},
+        )
+
+        result = self.driver._with_record_set_metadata(record)
+
+        self.assertIs(result, record)
+        self.assertNotIn("_multi_value", result.extra)
+
+    def test_to_record_value(self):
+        self.assertEqual(self.driver._to_record_value("1.2.3.4", None), "1.2.3.4")
+        self.assertEqual(self.driver._to_record_value("1.2.3.4", {}), "1.2.3.4")
+        self.assertEqual(
+            self.driver._to_record_value("ASPMX.L.GOOGLE.COM.", {"priority": 1}),
+            "1 ASPMX.L.GOOGLE.COM.",
+        )
+
     def test_delete_record_does_not_exist(self):
         zone = self.driver.list_zones()[0]
         record = self.driver.list_records(zone=zone)[0]