Merge pull request #28644 from vespa-engine/revert-28639-mpolden/remove-duplicate-endpoint-building

Revert "Remove unused endpoint name building from config-model"

4 files changed, 167 insertions, 32 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java
index 215439aa42a..69749ee6f96 100644
--- a/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java
+++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java
@@ -2,8 +2,15 @@
 
 package com.yahoo.config.model.api;
 
+import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.SystemName;
+
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Represents one endpoint for an application cluster
@@ -147,21 +154,83 @@ public class ApplicationClusterEndpoint {
 
     }
 
-    public record DnsName(String name) implements Comparable<DnsName> {
+    public static class DnsName implements Comparable<DnsName> {
+
+        private static final int MAX_LABEL_LENGTH = 63;
+
+        private final String name;
+
+        private DnsName(String name) {
+            this.name = name;
+        }
 
         public String value() {
             return name;
         }
 
+        public static DnsName sharedNameFrom(SystemName systemName, ClusterSpec.Id cluster, ApplicationId applicationId, String suffix) {
+            String name = dnsParts(systemName, cluster, applicationId)
+                    .filter(Objects::nonNull)             // remove null values that were "default"
+                    .collect(Collectors.joining("--"));
+            return new DnsName(sanitize(name) + suffix); // Need to sanitize name since it is considered one label
+        }
+
+        public static DnsName sharedL4NameFrom(SystemName systemName, ClusterSpec.Id cluster, ApplicationId applicationId, String suffix) {
+            String name = dnsParts(systemName, cluster, applicationId)
+                    .filter(Objects::nonNull) // remove null values that were "default"
+                    .map(DnsName::sanitize)
+                    .collect(Collectors.joining("."));
+            return new DnsName(name + suffix);
+        }
+
         public static DnsName from(String name) {
             return new DnsName(name);
         }
 
+        private static Stream<String> dnsParts(SystemName systemName, ClusterSpec.Id cluster, ApplicationId applicationId) {
+            return Stream.of(
+                    nullIfDefault(cluster.value()),
+                    systemPart(systemName),
+                    nullIfDefault(applicationId.instance().value()),
+                    applicationId.application().value(),
+                    applicationId.tenant().value()
+            );
+        }
+
+        /**
+         * Remove any invalid characters from the hostnames
+         */
+        private static String sanitize(String id) {
+            return shortenIfNeeded(id.toLowerCase()
+                                           .replace('_', '-')
+                                           .replaceAll("[^a-z0-9-]*", ""));
+        }
+
+        /**
+         * Truncate the given string at the front so its length does not exceed 63 characters.
+         */
+        private static String shortenIfNeeded(String id) {
+            return id.substring(Math.max(0, id.length() - MAX_LABEL_LENGTH));
+        }
+
+        private static String nullIfDefault(String string) {
+            return Optional.of(string).filter(s -> !s.equals("default")).orElse(null);
+        }
+
+        private static String systemPart(SystemName systemName) {
+            return "cd".equals(systemName.value()) ? systemName.value() : null;
+        }
+
+        @Override
+        public String toString() {
+            return "DnsName{" +
+                   "name='" + name + '\'' +
+                   '}';
+        }
+
         @Override
         public int compareTo(DnsName o) {
             return name.compareTo(o.name);
         }
-
     }
-
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
index 0945dfaf54a..2227831a8a0 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
@@ -11,11 +11,13 @@ import com.yahoo.config.application.api.ComponentInfo;
 import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.api.ApplicationClusterEndpoint;
 import com.yahoo.config.model.api.ApplicationClusterInfo;
+import com.yahoo.config.model.api.ContainerEndpoint;
 import com.yahoo.config.model.api.Model;
 import com.yahoo.config.model.api.OnnxModelCost;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.model.producer.TreeConfigProducer;
 import com.yahoo.config.provision.AllocatedHosts;
+import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.HostSpec;
 import com.yahoo.container.bundle.BundleInstantiationSpecification;
 import com.yahoo.container.di.config.ApplicationBundlesConfig;
@@ -224,23 +226,49 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
     private void createEndpoints(DeployState deployState) {
         if (!deployState.isHosted()) return;
         if (deployState.getProperties().applicationId().instance().isTester()) return;
-        // Add endpoints provided by the controller
-        List<String> hosts = getContainers().stream().map(AbstractService::getHostName).sorted().toList();
         List<ApplicationClusterEndpoint> endpoints = new ArrayList<>();
-        deployState.getEndpoints().stream()
-                   .filter(ce -> ce.clusterId().equals(getName()))
-                   .forEach(ce -> ce.names().forEach(
-                           name -> endpoints.add(ApplicationClusterEndpoint.builder()
-                                                                           .scope(ce.scope())
-                                                                           .weight(ce.weight().orElse(1))
-                                                                           .routingMethod(ce.routingMethod())
-                                                                           .dnsName(ApplicationClusterEndpoint.DnsName.from(name))
-                                                                           .hosts(hosts)
-                                                                           .clusterId(getName())
-                                                                           .authMethod(ce.authMethod())
-                                                                           .build())
-                   ));
-        this.endpoints = Collections.unmodifiableList(endpoints);
+
+        List<String> hosts = getContainers().stream()
+                .map(AbstractService::getHostName)
+                .sorted()
+                .toList();
+
+        Set<ContainerEndpoint> endpointsFromController = deployState.getEndpoints();
+        // Add zone-scoped endpoints if not provided by the controller
+        // TODO(mpolden): Remove this when controller always includes zone-scope endpoints, and config models < 8.230 are gone
+        if (endpointsFromController.stream().noneMatch(endpoint -> endpoint.scope() == ApplicationClusterEndpoint.Scope.zone)) {
+            for (String suffix : deployState.getProperties().zoneDnsSuffixes()) {
+                ApplicationClusterEndpoint.DnsName l4Name = ApplicationClusterEndpoint.DnsName.sharedL4NameFrom(
+                        deployState.zone().system(),
+                        ClusterSpec.Id.from(getName()),
+                        deployState.getProperties().applicationId(),
+                        suffix);
+                endpoints.add(ApplicationClusterEndpoint.builder()
+                                                        .zoneScope()
+                                                        .sharedL4Routing()
+                                                        .dnsName(l4Name)
+                                                        .hosts(hosts)
+                                                        .clusterId(getName())
+                                                        .authMethod(ApplicationClusterEndpoint.AuthMethod.mtls)
+                                                        .build());
+            }
+        }
+
+        // Include all endpoints provided by controller
+        endpointsFromController.stream()
+                               .filter(ce -> ce.clusterId().equals(getName()))
+                               .forEach(ce -> ce.names().forEach(
+                                       name -> endpoints.add(ApplicationClusterEndpoint.builder()
+                                                                                       .scope(ce.scope())
+                                                                                       .weight(ce.weight().orElse(1)) // Default to weight=1 if not set
+                                                                                       .routingMethod(ce.routingMethod())
+                                                                                       .dnsName(ApplicationClusterEndpoint.DnsName.from(name))
+                                                                                       .hosts(hosts)
+                                                                                       .clusterId(getName())
+                                                                                       .authMethod(ce.authMethod())
+                                                                                       .build())
+                               ));
+        this.endpoints = List.copyOf(endpoints);
     }
 
     @Override
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
index 3bdb60a0a8d..894fc55c014 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
@@ -42,14 +42,14 @@ import java.util.Objects;
 import java.util.OptionalInt;
 import java.util.Set;
 
+import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.exclusive;
+import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.shared;
 import static com.yahoo.config.model.api.ApplicationClusterEndpoint.RoutingMethod.sharedLayer4;
 import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.application;
 import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.global;
-import static com.yahoo.config.model.api.ApplicationClusterEndpoint.Scope.zone;
+import static com.yahoo.config.provision.SystemName.cd;
 import static com.yahoo.config.provision.SystemName.main;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;
 
 /**
  * @author Simon Thoresen Hult
@@ -365,23 +365,62 @@ public class ContainerClusterTest {
 
     @Test
     void generatesCorrectRoutingInfo() {
+        // main system:
         assertNames(main,
                 ApplicationId.from("t1", "a1", "i1"),
-                Set.of(new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.i1.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)),
+                Set.of(),
                 List.of("search-cluster.i1.a1.t1.endpoint.suffix"));
 
         assertNames(main,
                 ApplicationId.from("t1", "a1", "default"),
-                Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar")),
-                       new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)),
+                Set.of(),
+                List.of("search-cluster.a1.t1.endpoint.suffix"));
+
+        assertNames(main,
+                ApplicationId.from("t1", "default", "default"),
+                Set.of(),
+                List.of("search-cluster.default.t1.endpoint.suffix"));
+
+        assertNames(main,
+                ApplicationId.from("t1", "a1", "default"),
+                Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar"))),
                 List.of("search-cluster.a1.t1.endpoint.suffix"));
 
         assertNames(main,
                 ApplicationId.from("t1", "a1", "default"),
-                    Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4),
-                           new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4),
-                           new ContainerEndpoint("search-cluster", zone, List.of("search-cluster.a1.t1.endpoint.suffix"), OptionalInt.empty(), sharedLayer4)),
+                Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4),
+                        new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4)),
                 List.of("search-cluster.a1.t1.endpoint.suffix", "rotation-1.x.y.z", "rotation-2.x.y.z", "app-rotation.x.y.z"));
+
+        // cd system:
+        assertNames(cd,
+                ApplicationId.from("t1", "a1", "i1"),
+                Set.of(),
+                List.of("search-cluster.cd.i1.a1.t1.endpoint.suffix"));
+
+        assertNames(cd,
+                ApplicationId.from("t1", "a1", "default"),
+                Set.of(),
+                List.of("search-cluster.cd.a1.t1.endpoint.suffix"));
+
+        assertNames(cd,
+                ApplicationId.from("t1", "default", "default"),
+                Set.of(),
+                List.of("search-cluster.cd.default.t1.endpoint.suffix"));
+
+        assertNames(cd,
+                ApplicationId.from("t1", "a1", "default"),
+                Set.of(new ContainerEndpoint("not-in-this-cluster", global, List.of("foo", "bar"))),
+                List.of("search-cluster.cd.a1.t1.endpoint.suffix"));
+
+        assertNames(cd,
+                ApplicationId.from("t1", "a1", "default"),
+                Set.of(new ContainerEndpoint("search-cluster", global, List.of("rotation-1.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), sharedLayer4),
+                        new ContainerEndpoint("search-cluster", global, List.of("a.b.x.y.z", "rotation-2.x.y.z"), OptionalInt.empty(), shared),
+                        new ContainerEndpoint("search-cluster", application, List.of("app-rotation.x.y.z"), OptionalInt.of(3), sharedLayer4),
+                        new ContainerEndpoint("not-supported", global, List.of("not.supported"), OptionalInt.empty(), exclusive)),
+                List.of("search-cluster.cd.a1.t1.endpoint.suffix", "rotation-1.x.y.z", "rotation-2.x.y.z", "app-rotation.x.y.z"));
+
     }
 
     private void assertNames(SystemName systemName, ApplicationId appId, Set<ContainerEndpoint> globalEndpoints, List<String> expectedSharedL4Names) {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/model/LbServicesProducerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/model/LbServicesProducerTest.java
index 8d1b0fefbaf..ca2f9da3273 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/model/LbServicesProducerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/model/LbServicesProducerTest.java
@@ -50,11 +50,10 @@ import static org.junit.Assert.assertTrue;
 public class LbServicesProducerTest {
 
     private static final Set<ContainerEndpoint> endpoints = Set.of(
-            new ContainerEndpoint("mydisc", ApplicationClusterEndpoint.Scope.zone, List.of("mydisc.foo.foo.endpoint1.suffix")),
-            new ContainerEndpoint("mydisc", ApplicationClusterEndpoint.Scope.zone, List.of("mydisc.foo.foo.endpoint2.suffix")),
             new ContainerEndpoint("mydisc", ApplicationClusterEndpoint.Scope.global, List.of("rotation-1", "rotation-2")),
             new ContainerEndpoint("mydisc", ApplicationClusterEndpoint.Scope.application, List.of("app-endpoint"))
     );
+    private static final List<String> zoneDnsSuffixes = List.of(".endpoint1.suffix", ".endpoint2.suffix");
 
     private final InMemoryFlagSource flagSource = new InMemoryFlagSource();
 
@@ -229,7 +228,7 @@ public class LbServicesProducerTest {
     private TestProperties getTestproperties(ApplicationId applicationId) {
         return new TestProperties()
                 .setHostedVespa(true)
+                .setZoneDnsSuffixes(zoneDnsSuffixes)
                 .setApplicationId(applicationId);
     }
-
 }