Reject load balancers with clashing non-compactable IDs

2 files changed, 70 insertions, 3 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
index 206aa077027..0b5a04ca42c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
@@ -2,10 +2,12 @@
 package com.yahoo.vespa.hosted.provision.provisioning;
 
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.ApplicationTransaction;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostName;
 import com.yahoo.config.provision.NodeType;
+import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.exception.LoadBalancerServiceException;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.flags.BooleanFlag;
@@ -82,7 +84,8 @@ public class LoadBalancerProvisioner {
         try (var lock = db.lock(application)) {
             ClusterSpec.Id clusterId = effectiveId(cluster);
             List<Node> nodes = nodesOf(clusterId, application);
-            provision(application, clusterId, nodes, false);
+            LoadBalancerId loadBalancerId = requireNonClashing(new LoadBalancerId(application, clusterId));
+            provision(loadBalancerId, nodes, false);
         }
     }
 
@@ -149,9 +152,30 @@ public class LoadBalancerProvisioner {
         return canForwardTo;
     }
 
+    /** Find all load balancer IDs owned by given tenant and application */
+    private List<LoadBalancerId> findLoadBalancers(TenantName tenant, ApplicationName application) {
+        return db.readLoadBalancerIds().stream()
+                 .filter(id -> id.application().tenant().equals(tenant) &&
+                               id.application().application().equals(application))
+                 .collect(Collectors.toUnmodifiableList());
+    }
+
+    /** Require that load balancer IDs do not clash. This prevents name clashing when compacting endpoint DNS names */
+    private LoadBalancerId requireNonClashing(LoadBalancerId loadBalancerId) {
+        List<LoadBalancerId> loadBalancerIds = findLoadBalancers(loadBalancerId.application().tenant(),
+                                                                 loadBalancerId.application().application());
+        List<String> nonCompactableIds = withoutCompactableIds(loadBalancerId);
+        for (var id : loadBalancerIds) {
+            if (id.equals(loadBalancerId)) continue;
+            if (nonCompactableIds.equals(withoutCompactableIds(id))) {
+                throw new IllegalArgumentException(loadBalancerId + " clashes with " + id);
+            }
+        }
+        return loadBalancerId;
+    }
+
     /** Idempotently provision a load balancer for given application and cluster */
-    private void provision(ApplicationId application, ClusterSpec.Id clusterId, List<Node> nodes, boolean activate) {
-        var id = new LoadBalancerId(application, clusterId);
+    private void provision(LoadBalancerId id, List<Node> nodes, boolean activate) {
         var now = nodeRepository.clock().instant();
         var loadBalancer = db.readLoadBalancer(id);
         if (loadBalancer.isEmpty() && activate) return; // Nothing to activate as this load balancer was never prepared
@@ -171,6 +195,10 @@ public class LoadBalancerProvisioner {
         db.writeLoadBalancer(newLoadBalancer);
     }
 
+    private void provision(ApplicationId application, ClusterSpec.Id clusterId, List<Node> nodes, boolean activate) {
+        provision(new LoadBalancerId(application, clusterId), nodes, activate);
+    }
+
     private LoadBalancerInstance provisionInstance(LoadBalancerId id, List<Node> nodes, boolean force) {
         var reals = new LinkedHashSet<Real>();
         for (var node : nodes) {
@@ -206,6 +234,18 @@ public class LoadBalancerProvisioner {
         return nodes.stream().collect(Collectors.groupingBy(node -> effectiveId(node.allocation().get().membership().cluster())));
     }
 
+    /** Returns a list of the non-compactable IDs of given load balancer */
+    private static List<String> withoutCompactableIds(LoadBalancerId id) {
+        List<String> ids = new ArrayList<>(2);
+        if (!"default".equals(id.cluster().value())) {
+            ids.add(id.cluster().value());
+        }
+        if (!id.application().instance().isDefault()) {
+            ids.add(id.application().instance().value());
+        }
+        return ids;
+    }
+
     /** Find IP addresses reachable by the load balancer service */
     private Set<String> reachableIpAddresses(Node node) {
         Set<String> reachable = new LinkedHashSet<>(node.ipConfig().primary());
@@ -225,4 +265,5 @@ public class LoadBalancerProvisioner {
         return cluster.combinedId().orElse(cluster.id());
     }
 
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
index c51ef7250e2..7a636a030ec 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
@@ -33,6 +33,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author mpolden
@@ -236,6 +237,31 @@ public class LoadBalancerProvisionerTest {
         assertEquals(cluster, lbs.get().get(0).id().cluster());
     }
 
+    @Test
+    public void reject_load_balancers_with_clashing_names() {
+        ApplicationId instance1 = ApplicationId.from("t1", "a1", "default");
+        ApplicationId instance2 = ApplicationId.from("t1", "a1", "dev");
+        ApplicationId instance3 = ApplicationId.from("t1", "a1", "qrs");
+        ClusterSpec.Id devCluster = ClusterSpec.Id.from("dev");
+        ClusterSpec.Id defaultCluster = ClusterSpec.Id.from("default");
+
+        // instance1 is deployed
+        tester.activate(instance1, prepare(instance1, clusterRequest(ClusterSpec.Type.container, devCluster)));
+
+        // instance2 clashes because cluster name matches instance1
+        try {
+            prepare(instance2, clusterRequest(ClusterSpec.Type.container, defaultCluster));
+            fail("Expected exception");
+        } catch (IllegalArgumentException ignored) {
+        }
+
+        // instance2 changes cluster name and does not clash
+        tester.activate(instance2, prepare(instance2, clusterRequest(ClusterSpec.Type.container, ClusterSpec.Id.from("qrs"))));
+
+        // instance3 clashes because instance name matches instance2 cluster
+        tester.activate(instance3, prepare(instance3, clusterRequest(ClusterSpec.Type.container, defaultCluster)));
+    }
+
     private void dirtyNodesOf(ApplicationId application) {
         tester.nodeRepository().setDirty(tester.nodeRepository().getNodes(application), Agent.system, this.getClass().getSimpleName());
     }