Verify container cluster names in hsoted vespa

5 files changed, 101 insertions, 21 deletions

diff --git a/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java b/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
index 95d97bc9e87..756646beddb 100644
--- a/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
+++ b/config-model/src/main/java/com/yahoo/config/model/ConfigModelRepo.java
@@ -80,7 +80,7 @@ public class ConfigModelRepo implements ConfigModelRepoAdder, Serializable, Iter
                                  ConfigModelRegistry configModelRegistry) throws IOException {
         Element userServicesElement = getServicesFromApp(deployState.getApplicationPackage());
         readConfigModels(root, userServicesElement, deployState, vespaModel, configModelRegistry);
-        builder.postProc(deployState.getDeployLogger(), root, this);
+        builder.postProc(deployState, root, this);
     }
 
     private Element getServicesFromApp(ApplicationPackage applicationPackage) throws IOException {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/VespaModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/VespaModelBuilder.java
index 2cf32f1e8ff..421e3a2902c 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/VespaModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/VespaModelBuilder.java
@@ -22,6 +22,6 @@ public abstract class VespaModelBuilder {
      * @param producerRoot the root producer.
      * @param configModelRepo a {@link com.yahoo.config.model.ConfigModelRepo instance}
      */
-    public abstract void postProc(DeployLogger deployLogger, AbstractConfigProducer producerRoot, ConfigModelRepo configModelRepo);
+    public abstract void postProc(DeployState deployState, AbstractConfigProducer producerRoot, ConfigModelRepo configModelRepo);
 
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
index cb3c43074fc..a5ff77dc1eb 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/VespaDomBuilder.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.builder.xml.dom;
 
+import ai.vespa.validation.Validation;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.ApplicationConfigProducerRoot;
 import com.yahoo.config.model.ConfigModelRepo;
@@ -22,8 +23,15 @@ import com.yahoo.vespa.model.container.docproc.ContainerDocproc;
 import com.yahoo.vespa.model.content.Content;
 import com.yahoo.vespa.model.search.SearchCluster;
 import org.w3c.dom.Element;
+
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
+import java.util.Map;
+import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.regex.Pattern;
 
 /**
  * Builds Vespa model components using the w3c dom api
@@ -43,10 +51,12 @@ public class VespaDomBuilder extends VespaModelBuilder {
     public static final String VESPAMALLOC = "vespamalloc";            // Intended for vespa engineers
     public static final String VESPAMALLOC_DEBUG = "vespamalloc-debug"; // Intended for vespa engineers
     public static final String VESPAMALLOC_DEBUG_STACKTRACE = "vespamalloc-debug-stacktrace"; // Intended for vespa engineers
-    private static final String CPU_SOCKET_ATTRIB_NAME = "cpu-socket";
     public static final String CPU_SOCKET_AFFINITY_ATTRIB_NAME = "cpu-socket-affinity";
     public static final String Allocated_MEMORY_ATTRIB_NAME = "allocated-memory";
 
+    private static final String CPU_SOCKET_ATTRIB_NAME = "cpu-socket";
+    private static final Pattern clusterPattern = Pattern.compile("([a-z0-9]|[a-z0-9][a-z0-9_-]{0,61}[a-z0-9])");
+
     public static final Logger log = Logger.getLogger(VespaDomBuilder.class.getPackage().toString());
 
 
@@ -232,13 +242,14 @@ public class VespaDomBuilder extends VespaModelBuilder {
      * @param root root config producer
      * @param configModelRepo a {@link ConfigModelRepo}
      */
-    public void postProc(DeployLogger deployLogger, AbstractConfigProducer root, ConfigModelRepo configModelRepo) {
+    public void postProc(DeployState deployState, AbstractConfigProducer root, ConfigModelRepo configModelRepo) {
         setContentSearchClusterIndexes(configModelRepo);
         createDocprocMBusServersAndClients(configModelRepo);
+        if (deployState.isHosted()) validateContainerClusterIds(configModelRepo);
     }
 
     private void createDocprocMBusServersAndClients(ConfigModelRepo pc) {
-        for (ContainerCluster cluster: ContainerModel.containerClusters(pc)) {
+        for (ContainerCluster<?> cluster: ContainerModel.containerClusters(pc)) {
             addServerAndClientsForChains(cluster.getDocproc());
         }
     }
@@ -248,6 +259,18 @@ public class VespaDomBuilder extends VespaModelBuilder {
             docproc.getChains().addServersAndClientsForChains();
     }
 
+    private void validateContainerClusterIds(ConfigModelRepo pc) {
+        Map<String, String> normalizedClusterIds = new LinkedHashMap<>();
+        for (ContainerCluster<?> cluster: ContainerModel.containerClusters(pc)) {
+            String name = cluster.getName();
+            Validation.requireMatch(name, "container cluster name", clusterPattern);
+            String clashing = normalizedClusterIds.put(name.replaceAll("_", "-"), name);
+            if (clashing != null) throw new IllegalArgumentException("container clusters '" + clashing + "' and '" + name +
+                                                                     "' have clashing endpoint names, when '_' is replaced " +
+                                                                     "with '-' to form valid domain names");
+        }
+    }
+
     /**
      * For some reason, search clusters need to be enumerated.
      * @param configModelRepo a {@link ConfigModelRepo}
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
index a5b396508ef..f0c39ecc920 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
@@ -23,8 +23,8 @@ import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.config.provision.ZoneEndpoint;
-import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn;
 import com.yahoo.config.provision.ZoneEndpoint.AccessType;
+import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn;
 import com.yahoo.config.provisioning.FlavorsConfig;
 import com.yahoo.container.ComponentsConfig;
 import com.yahoo.container.QrConfig;
@@ -181,6 +181,63 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
     }
 
     @Test
+    void container_cluster_with_invalid_name_throws_exception_when_hosted() throws IOException, SAXException {
+        String servicesXml = """
+                             <services version='1.0'>
+                               <container id='C-1' version='1.0'>
+                                 <nodes count='1' />
+                               </container>
+                             </services>
+                             """;
+
+        assertEquals("container cluster name must match '([a-z0-9]|[a-z0-9][a-z0-9_-]{0,61}[a-z0-9])', but got: 'C-1'",
+                     assertThrows(IllegalArgumentException.class,
+                                  () ->
+                                          new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                                                  .modelHostProvisioner(new InMemoryProvisioner(4, false))
+                                                  .applicationPackage(new MockApplicationPackage.Builder().withServices(servicesXml).build())
+                                                  .properties(new TestProperties().setHostedVespa(true))
+                                                  .build()))
+                             .getMessage());
+
+        new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                .modelHostProvisioner(new InMemoryProvisioner(4, false))
+                .applicationPackage(new MockApplicationPackage.Builder().withServices(servicesXml).build())
+                .properties(new TestProperties().setHostedVespa(false))
+                .build());
+    }
+
+    @Test
+    void two_clusters_with_clashing_cluster_names_throws_exception_when_hosted() throws IOException, SAXException {
+        String servicesXml = """
+                             <services version='1.0'>
+                               <container id='c-1' version='1.0'>
+                                 <nodes count='1' />
+                               </container>
+                               <container id='c_1' version='1.0'>
+                                 <nodes count='1' />
+                               </container>
+                             </services>
+                             """;
+
+        assertEquals("container clusters 'c-1' and 'c_1' have clashing endpoint names, when '_' is replaced with '-' to form valid domain names",
+                     assertThrows(IllegalArgumentException.class,
+                                  () ->
+                                          new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                                                  .modelHostProvisioner(new InMemoryProvisioner(4, false))
+                                                  .applicationPackage(new MockApplicationPackage.Builder().withServices(servicesXml).build())
+                                                  .properties(new TestProperties().setHostedVespa(true))
+                                                  .build()))
+                             .getMessage());
+
+        new VespaModel(new NullConfigModelRegistry(), new DeployState.Builder()
+                .modelHostProvisioner(new InMemoryProvisioner(4, false))
+                .applicationPackage(new MockApplicationPackage.Builder().withServices(servicesXml).build())
+                .properties(new TestProperties().setHostedVespa(false))
+                .build());
+    }
+
+    @Test
     void two_clusters_without_explicit_port_throws_exception() {
         Element cluster1Elem = DomBuilderTest.parse(
                 "<container id='cluster1' version='1.0'>",
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
index 5143aa91f56..086df4d0c33 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
@@ -49,8 +49,8 @@ public class VirtualNodeProvisioningTest {
     private static final NodeResources resources1 = new NodeResources(4, 8, 100, 1);
     private static final NodeResources resources2 = new NodeResources(1, 4, 100, 1,
                                                                       NodeResources.DiskSpeed.fast, NodeResources.StorageType.local);
-    private static final ClusterSpec contentClusterSpec = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion("6.42").build();
-    private static final ClusterSpec containerClusterSpec = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContainer")).vespaVersion("6.42").build();
+    private static final ClusterSpec contentClusterSpec = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion("6.42").build();
+    private static final ClusterSpec containerClusterSpec = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-container")).vespaVersion("6.42").build();
 
     private final ApplicationId applicationId = ProvisioningTester.applicationId("test");
 
@@ -242,7 +242,7 @@ public class VirtualNodeProvisioningTest {
         Version wantedVespaVersion = Version.fromString("6.39");
         int nodeCount = 7;
         List<HostSpec> hosts = tester.prepare(application1,
-                                              ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                                              ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                                               nodeCount, 1, resources2);
         tester.activate(application1, new HashSet<>(hosts));
 
@@ -253,7 +253,7 @@ public class VirtualNodeProvisioningTest {
         // Upgrade Vespa version on nodes
         Version upgradedWantedVespaVersion = Version.fromString("6.40");
         List<HostSpec> upgradedHosts = tester.prepare(application1,
-                                                      ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion(upgradedWantedVespaVersion).build(),
+                                                      ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion(upgradedWantedVespaVersion).build(),
                                                       nodeCount, 1, resources2);
         tester.activate(application1, new HashSet<>(upgradedHosts));
         NodeList upgradedNodes = tester.getNodes(application1, Node.State.active);
@@ -275,7 +275,7 @@ public class VirtualNodeProvisioningTest {
         int nodeCount = 7;
         try {
             List<HostSpec> nodes = tester.prepare(application1,
-                                                  ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                                                  ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                                                   nodeCount, 1, resources2);
             fail("Expected the allocation to fail due to parent hosts not being active yet");
         } catch (NodeAllocationException expected) { }
@@ -285,7 +285,7 @@ public class VirtualNodeProvisioningTest {
 
         // Try allocating tenants again
         List<HostSpec> nodes = tester.prepare(application1,
-                                              ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                                              ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                                               nodeCount, 1, resources2);
         tester.activate(application1, new HashSet<>(nodes));
 
@@ -309,14 +309,14 @@ public class VirtualNodeProvisioningTest {
 
         Version wantedVespaVersion = Version.fromString("6.39");
         List<HostSpec> nodes = tester.prepare(application2_1,
-                                              ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                                              ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                                               6, 1, resources);
         assertHostSpecParentReservation(nodes, Optional.empty(), tester); // We do not get nodes on hosts reserved to tenant1
         tester.activate(application2_1, nodes);
 
         try {
             tester.prepare(application2_2,
-                           ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                           ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                            5, 1, resources);
             fail("Expected exception");
         }
@@ -325,7 +325,7 @@ public class VirtualNodeProvisioningTest {
         }
 
         nodes = tester.prepare(application1_1,
-                               ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                               ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                                10, 1, resources);
         assertHostSpecParentReservation(nodes, Optional.of(tenant1), tester);
         tester.activate(application1_1, nodes);
@@ -346,14 +346,14 @@ public class VirtualNodeProvisioningTest {
         try {
             // No capacity for 'container' nodes
             tester.prepare(applicationId,
-                    ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                    ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                     6, 1, resources);
             fail("Expected to fail node allocation");
         } catch (NodeAllocationException ignored) { }
 
         // Same cluster, but content type is now 'content'
         List<HostSpec> nodes = tester.prepare(applicationId,
-                ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion(wantedVespaVersion).build(),
+                ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion(wantedVespaVersion).build(),
                 6, 1, resources);
         tester.activate(applicationId, nodes);
 
@@ -445,7 +445,7 @@ public class VirtualNodeProvisioningTest {
             assertEquals("No room for 3 nodes as 2 of 4 hosts are exclusive",
                          "Could not satisfy request for 3 nodes with " +
                          "[vcpu: 1.0, memory: 4.0 Gb, disk 100.0 Gb, bandwidth: 1.0 Gbps, architecture: x86_64] " +
-                         "in tenant2.app2 container cluster 'myContainer' 6.39: " +
+                         "in tenant2.app2 container cluster 'my-container' 6.39: " +
                          "Node allocation failure on group 0: " +
                          "Not enough suitable nodes available due to host exclusivity constraints",
                          e.getMessage());
@@ -465,14 +465,14 @@ public class VirtualNodeProvisioningTest {
             tester.makeReadyChildren(1, resources2, "host2");
 
             tester.prepare(application1,
-                           ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("myContent")).vespaVersion("6.42").build(),
+                           ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("my-content")).vespaVersion("6.42").build(),
                            2, 1,
                            resources2.with(NodeResources.StorageType.remote));
         }
         catch (NodeAllocationException e) {
             assertEquals("Could not satisfy request for 2 nodes with " +
                          "[vcpu: 1.0, memory: 4.0 Gb, disk 100.0 Gb, bandwidth: 1.0 Gbps, storage type: remote, architecture: x86_64] " +
-                         "in tenant.app1 content cluster 'myContent'" +
+                         "in tenant.app1 content cluster 'my-content'" +
                          " 6.42: Node allocation failure on group 0",
                          e.getMessage());
         }
@@ -672,7 +672,7 @@ public class VirtualNodeProvisioningTest {
 
     private void prepareAndActivate(ApplicationId application, int nodeCount, boolean exclusive, NodeResources resources, ProvisioningTester tester) {
         Set<HostSpec> hosts = new HashSet<>(tester.prepare(application,
-                                                           ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("myContainer")).vespaVersion("6.39").exclusive(exclusive).build(),
+                                                           ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("my-container")).vespaVersion("6.39").exclusive(exclusive).build(),
                                                            Capacity.from(new ClusterResources(nodeCount, 1, resources), false, true)));
         tester.activate(application, hosts);
     }