Merge pull request #15682 from vespa-engine/mpolden/detect-clashing-names

Reject endpoints with clashing non-compactable IDs

4 files changed, 146 insertions, 3 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackageValidator.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackageValidator.java
index 3e72936575d..bb2d8b3c553 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackageValidator.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationPackageValidator.java
@@ -18,8 +18,10 @@ import com.yahoo.vespa.hosted.controller.deployment.DeploymentSteps;
 
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -46,6 +48,7 @@ public class ApplicationPackageValidator {
         validateSteps(applicationPackage.deploymentSpec());
         validateEndpointRegions(applicationPackage.deploymentSpec());
         validateEndpointChange(application, applicationPackage, instant);
+        validateCompactedEndpoint(applicationPackage);
         validateSecurityClientsPem(applicationPackage);
     }
 
@@ -98,6 +101,25 @@ public class ApplicationPackageValidator {
                                                                                                    instant));
     }
 
+    /** Verify that compactable endpoint parts (instance aname nd endpoint ID) do not clash */
+    private void validateCompactedEndpoint(ApplicationPackage applicationPackage) {
+        Map<List<String>, InstanceEndpoint> instanceEndpoints = new HashMap<>();
+        for (var instanceSpec : applicationPackage.deploymentSpec().instances()) {
+            for (var endpoint : instanceSpec.endpoints()) {
+                List<String> nonCompactableIds = nonCompactableIds(instanceSpec.name(), endpoint);
+                InstanceEndpoint instanceEndpoint = new InstanceEndpoint(instanceSpec.name(), endpoint.endpointId());
+                InstanceEndpoint existingEndpoint = instanceEndpoints.get(nonCompactableIds);
+                if (existingEndpoint != null) {
+                    throw new IllegalArgumentException("Endpoint with ID '" + endpoint.endpointId() + "' in instance '"
+                                                       + instanceSpec.name().value() +
+                                                       "' clashes with endpoint '" + existingEndpoint.endpointId +
+                                                       "' in instance '" + existingEndpoint.instance + "'");
+                }
+                instanceEndpoints.put(nonCompactableIds, instanceEndpoint);
+            }
+        }
+    }
+
     /** Verify changes to endpoint configuration by comparing given application package to the existing one, if any */
     private void validateEndpointChange(Application application, InstanceName instanceName, ApplicationPackage applicationPackage, Instant instant) {
         var validationId = ValidationId.globalEndpointChange;
@@ -161,4 +183,28 @@ public class ApplicationPackageValidator {
         });
     }
 
+    /** Returns a list of the non-compactable IDs of given instance and endpoint */
+    private static List<String> nonCompactableIds(InstanceName instance, Endpoint endpoint) {
+        List<String> ids = new ArrayList<>(2);
+        if (!instance.isDefault()) {
+            ids.add(instance.value());
+        }
+        if (!"default".equals(endpoint.endpointId())) {
+            ids.add(endpoint.endpointId());
+        }
+        return ids;
+    }
+
+    private static class InstanceEndpoint {
+
+        private final InstanceName instance;
+        private final String endpointId;
+
+        public InstanceEndpoint(InstanceName instance, String endpointId) {
+            this.instance = instance;
+            this.endpointId = endpointId;
+        }
+
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index a3b22ccd649..569c22d8bf6 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -977,4 +977,34 @@ public class ControllerTest {
         app1.submit().deploy();
     }
 
+    @Test
+    public void testClashingEndpointIdAndInstanceName() {
+        String deploymentXml = "<deployment version='1.0' athenz-domain='domain' athenz-service='service'>\n" +
+                               "  <instance id=\"default\">\n" +
+                               "    <prod>\n" +
+                               "      <region active=\"true\">us-west-1</region>\n" +
+                               "    </prod>\n" +
+                               "    <endpoints>\n" +
+                               "      <endpoint id=\"dev\" container-id=\"qrs\"/>\n" +
+                               "    </endpoints>\n" +
+                               "  </instance>\n" +
+                               "  <instance id=\"dev\">\n" +
+                               "    <prod>\n" +
+                               "      <region active=\"true\">us-west-1</region>\n" +
+                               "    </prod>\n" +
+                               "    <endpoints>\n" +
+                               "      <endpoint id=\"default\" container-id=\"qrs\"/>\n" +
+                               "    </endpoints>\n" +
+                               "  </instance>\n" +
+                               "</deployment>\n";
+        ApplicationPackage applicationPackage = ApplicationPackageBuilder.fromDeploymentXml(deploymentXml);
+        try {
+            tester.newDeploymentContext().submit(applicationPackage);
+            fail("Expected exception");
+        } catch (IllegalArgumentException e) {
+            assertEquals("Endpoint with ID 'default' in instance 'dev' clashes with endpoint 'dev' in instance 'default'",
+                         e.getMessage());
+        }
+    }
+
 }
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());
     }