From 0c8b5bc8bfc2eda109c910102ad21a708530180f Mon Sep 17 00:00:00 2001
From: mmuller <mmuller@google.com>
Date: Tue, 26 Sep 2017 13:52:31 -0700
Subject: [PATCH] Build DNS changes with HashMap instead of Builder

The existing CloudDnsWriter code uses ImmutableMap.Builder to construct the
map of DNS records to update.  This has been seen to fail on alpha, presumably
in a cases where host records and domain records produce duplicate updates for
a host.

Convert the Builder to a HashMap, allowing us to safely overwrite existing
records in the case of duplicates.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170103421
---
 .../dns/writer/clouddns/CloudDnsWriter.java    | 15 ++++++++-------
 .../writer/clouddns/CloudDnsWriterTest.java    | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java b/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
index 766a5df30..3afa3ea92 100644
--- a/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
+++ b/java/google/registry/dns/writer/clouddns/CloudDnsWriter.java
@@ -46,6 +46,7 @@ import java.io.IOException;
 import java.net.Inet4Address;
 import java.net.Inet6Address;
 import java.net.InetAddress;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -82,8 +83,8 @@ public class CloudDnsWriter extends BaseDnsWriter {
   private final String projectId;
   private final String zoneName;
   private final Dns dnsConnection;
-  private final ImmutableMap.Builder<String, ImmutableSet<ResourceRecordSet>>
-      desiredRecordsBuilder = new ImmutableMap.Builder<>();
+  private final HashMap<String, ImmutableSet<ResourceRecordSet>> desiredRecords =
+      new HashMap<String, ImmutableSet<ResourceRecordSet>>();
 
   @Inject
   CloudDnsWriter(
@@ -121,7 +122,7 @@ public class CloudDnsWriter extends BaseDnsWriter {
     // desiredRecordsBuilder is populated with an empty set to indicate that all existing records
     // should be deleted.
     if (!domainResource.isPresent() || !domainResource.get().shouldPublishToDns()) {
-      desiredRecordsBuilder.put(absoluteDomainName, ImmutableSet.<ResourceRecordSet>of());
+      desiredRecords.put(absoluteDomainName, ImmutableSet.<ResourceRecordSet>of());
       return;
     }
 
@@ -171,7 +172,7 @@ public class CloudDnsWriter extends BaseDnsWriter {
       }
     }
 
-    desiredRecordsBuilder.put(absoluteDomainName, domainRecords.build());
+    desiredRecords.put(absoluteDomainName, domainRecords.build());
     logger.finefmt(
         "Will write %s records for domain %s", domainRecords.build().size(), absoluteDomainName);
   }
@@ -189,7 +190,7 @@ public class CloudDnsWriter extends BaseDnsWriter {
 
     // Return early if the host is deleted.
     if (!host.isPresent()) {
-      desiredRecordsBuilder.put(absoluteHostName, ImmutableSet.<ResourceRecordSet>of());
+      desiredRecords.put(absoluteHostName, ImmutableSet.<ResourceRecordSet>of());
       return;
     }
 
@@ -227,7 +228,7 @@ public class CloudDnsWriter extends BaseDnsWriter {
               .setRrdatas(ImmutableList.copyOf(aaaaRrData)));
     }
 
-    desiredRecordsBuilder.put(absoluteHostName, domainRecords.build());
+    desiredRecords.put(absoluteHostName, domainRecords.build());
   }
 
   /**
@@ -273,7 +274,7 @@ public class CloudDnsWriter extends BaseDnsWriter {
   @Override
   protected void commitUnchecked() {
     retrier.callWithRetry(
-        getMutateZoneCallback(desiredRecordsBuilder.build()), ZoneStateException.class);
+        getMutateZoneCallback(ImmutableMap.copyOf(desiredRecords)), ZoneStateException.class);
     logger.info("Wrote to Cloud DNS");
   }
 
diff --git a/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java b/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
index 57f1170a1..f1317bbed 100644
--- a/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
+++ b/javatests/google/registry/dns/writer/clouddns/CloudDnsWriterTest.java
@@ -471,4 +471,22 @@ public class CloudDnsWriterTest {
 
     verifyZone(ImmutableSet.<ResourceRecordSet>of());
   }
+
+  @Test
+  public void testDuplicateRecords() throws Exception {
+    // In publishing DNS records, we can end up publishing information on the same host twice
+    // (through a domain change and a host change), so this scenario needs to work.
+    persistResource(
+            fakeDomain(
+                "example.tld",
+                ImmutableSet.of(persistResource(fakeHost("0.ip4.example.tld", IPv4))),
+                0))
+        .asBuilder()
+        .addSubordinateHost("ns1.example.tld")
+        .build();
+    writer.publishDomain("example.tld");
+    writer.publishHost("0.ip4.example.tld");
+
+    verifyZone(fakeDomainRecords("example.tld", 1, 0, 0, 0));
+  }
 }