Preserve DNS records when LB is reprovisioned

Uniqueness of a `RoutingPolicy` cannot be determined by its `canonicalHostname`,
because the latter is unique for every newly provisioned load balancer, which
means that a distinct cluster may end up with multiple routing policies.

This change ensures that only one `RoutingPolicy` can exist per
application/cluster/zone.

3 files changed, 57 insertions, 12 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
index e55bbaf6acc..80a62d94f2e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/RoutingPolicy.java
@@ -14,7 +14,8 @@ import java.util.Optional;
 import java.util.Set;
 
 /**
- * Represents the DNS routing policy for a load balancer.
+ * Represents the DNS routing policy for a load balancer. A routing policy is uniquely identified by its owner, cluster
+ * and zone.
  *
  * @author mortent
  * @author mpolden
@@ -90,13 +91,15 @@ public class RoutingPolicy {
     public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
-        RoutingPolicy policy = (RoutingPolicy) o;
-        return canonicalName.equals(policy.canonicalName);
+        RoutingPolicy that = (RoutingPolicy) o;
+        return owner.equals(that.owner) &&
+               cluster.equals(that.cluster) &&
+               zone.equals(that.zone);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(canonicalName);
+        return Objects.hash(owner, cluster, zone);
     }
 
     @Override
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
index 3dc12f422c6..603757e099d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPolicies.java
@@ -147,17 +147,11 @@ public class RoutingPolicies {
             LOGGER.log(LogLevel.WARNING, "Removing " + removalCandidates + ". Active load balancers " +
                                          activeLoadBalancers);
         }
-        removalCandidates.forEach(allPolicies::remove);
-
-        // Find CNAMEs in use
-        var cnamesUsed = allPolicies.stream()
-                .map(policy -> policy.endpointIn(controller.system()).dnsName())
-                .collect(Collectors.toSet());
 
         for (var policy : removalCandidates) {
             var dnsName = policy.endpointIn(controller.system()).dnsName();
-            if ( ! cnamesUsed.contains(dnsName))
-                controller.nameServiceForwarder().removeRecords(Record.Type.CNAME, RecordName.from(dnsName), Priority.normal);
+            controller.nameServiceForwarder().removeRecords(Record.Type.CNAME, RecordName.from(dnsName), Priority.normal);
+            allPolicies.remove(policy);
         }
         db.writeRoutingPolicies(loadBalancers.application, allPolicies);
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
index f78873bae09..1bb20296bd2 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/RoutingPoliciesTest.java
@@ -307,6 +307,47 @@ public class RoutingPoliciesTest {
         assertEquals(1, tester.policiesOf(devInstance).size());
         assertEquals(Set.of("c0.user.app1.tenant1.us-east-1.dev.vespa.oath.cloud"), tester.recordNames());
     }
+
+    @Test
+    public void reprovisioning_load_balancer_preserves_cname_record() {
+        var tester = new RoutingPoliciesTester();
+        var context = tester.newDeploymentContext("tenant1", "app1", "default");
+
+        // Initial load balancer is provisioned
+        tester.provisionLoadBalancers(1, context.instanceId(), zone1);
+        var applicationPackage = new ApplicationPackageBuilder()
+                .region(zone1.region())
+                .build();
+
+        // Application is deployed
+        context.submit(applicationPackage).deploy();
+        var expectedRecords = Set.of(
+                "c0.app1.tenant1.us-west-1.vespa.oath.cloud"
+        );
+        assertEquals(expectedRecords, tester.recordNames());
+        assertEquals(1, tester.policiesOf(context.instanceId()).size());
+
+        // Application is removed and the load balancer is deprovisioned
+        tester.controllerTester().controller().applications().deactivate(context.instanceId(), zone1);
+        tester.controllerTester().configServer().removeLoadBalancers(context.instanceId(), zone1);
+
+        // Load balancer for the same application is provisioned again, but with a different hostname
+        var newHostname = HostName.from("new-hostname");
+        var loadBalancer = new LoadBalancer("LB-0-Z-" + zone1.value(),
+                                            context.instanceId(),
+                                            ClusterSpec.Id.from("c0"),
+                                            newHostname,
+                                            LoadBalancer.State.active,
+                                            Optional.of("dns-zone-1"));
+        tester.controllerTester().configServer().addLoadBalancers(zone1, List.of(loadBalancer));
+
+        // Application redeployment preserves DNS record
+        context.submit(applicationPackage).deploy();
+        assertEquals(expectedRecords, tester.recordNames());
+        assertEquals(1, tester.policiesOf(context.instanceId()).size());
+        assertEquals("CNAME points to current load blancer", newHostname.value() + ".",
+                     tester.cnameDataOf(expectedRecords.iterator().next()).get(0));
+    }
     
     private static List<LoadBalancer> createLoadBalancers(ZoneId zone, ApplicationId application, int count) {
         List<LoadBalancer> loadBalancers = new ArrayList<>();
@@ -368,6 +409,13 @@ public class RoutingPoliciesTest {
                          .collect(Collectors.toList());
         }
 
+        private List<String> cnameDataOf(String name) {
+            return tester.controllerTester().nameService().findRecords(Record.Type.CNAME, RecordName.from(name)).stream()
+                         .map(Record::data)
+                         .map(RecordData::asString)
+                         .collect(Collectors.toList());
+        }
+
     }
 
 }