Ensure that cross-instance generated endpoints are shared

5 files changed, 78 insertions, 19 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
index a29df3643c7..2b2ec725d7e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/RoutingController.java
@@ -135,33 +135,31 @@ public class RoutingController {
         Map<EndpointId, List<GeneratedEndpoint>> generatedForDeclaredEndpoints = new HashMap<>();
         Set<ClusterSpec.Id> clustersWithToken = new HashSet<>();
         if (randomizedEndpointsEnabled(deployment.applicationId())) { // TODO(mpolden): Remove this guard once config-models < 8.220 are gone
-            RoutingPolicyList deploymentPolicies = policies().read(deployment);
+            RoutingPolicyList applicationPolicies = policies().read(TenantAndApplicationId.from(deployment.applicationId()));
+            RoutingPolicyList deploymentPolicies = applicationPolicies.deployment(deployment);
             for (var container : services.containers()) {
                 ClusterSpec.Id clusterId = ClusterSpec.Id.from(container.id());
                 boolean tokenSupported = container.authMethods().contains(BasicServicesXml.Container.AuthMethod.token);
                 if (tokenSupported) {
                     clustersWithToken.add(clusterId);
                 }
-                // Use already existing generated endpoints, if any
                 Optional<RoutingPolicy> clusterPolicy = deploymentPolicies.cluster(clusterId).first();
-                List<GeneratedEndpoint> generatedForCluster = new ArrayList<>();
-                if (clusterPolicy.isPresent()) {
-                    for (var ge : clusterPolicy.get().generatedEndpoints()) {
-                        if (ge.declared()) {
-                            generatedForDeclaredEndpoints.computeIfAbsent(ge.endpoint().get(), (k) -> new ArrayList<>())
-                                                         .add(ge);
-                        } else {
-                            generatedForCluster.add(ge);
-                        }
-                    }
-                }
+                List<GeneratedEndpoint> generatedForCluster = clusterPolicy.map(policy -> policy.generatedEndpoints().cluster().asList())
+                                                                           .orElseGet(List::of);
+                // Generate endpoints if cluster does not have any
                 if (generatedForCluster.isEmpty()) {
                     generatedForCluster = generateEndpoints(tokenSupported, certificate, Optional.empty());
                 }
                 endpoints = endpoints.and(endpointsOf(deployment, clusterId, GeneratedEndpointList.copyOf(generatedForCluster)).scope(Scope.zone));
             }
 
-            // For all declared endpoints, generate new endpoints if none exist
+            // Generate endpoints if declared endpoint does not have any
+            for (var container : services.containers()) {
+                ClusterSpec.Id clusterId = ClusterSpec.Id.from(container.id());
+                applicationPolicies.cluster(clusterId).asList().stream()
+                                   .flatMap(policy -> policy.generatedEndpoints().declared().asList().stream())
+                                   .forEach(ge -> generatedForDeclaredEndpoints.computeIfAbsent(ge.endpoint().get(), (k) -> List.of(ge)));
+            }
             Stream.concat(spec.endpoints().stream(), spec.instances().stream().flatMap(i -> i.endpoints().stream()))
                   .forEach(endpoint -> {
                       EndpointId endpointId = EndpointId.of(endpoint.endpointId());
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GeneratedEndpointList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GeneratedEndpointList.java
index 376ac578363..62734091a57 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GeneratedEndpointList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/GeneratedEndpointList.java
@@ -42,6 +42,10 @@ public class GeneratedEndpointList extends AbstractFilteringList<GeneratedEndpoi
         return matching(ge -> ge.authMethod() == authMethod);
     }
 
+    public static GeneratedEndpointList of(GeneratedEndpoint... generatedEndpoint) {
+        return copyOf(List.of(generatedEndpoint));
+    }
+
     public static GeneratedEndpointList copyOf(Collection<GeneratedEndpoint> generatedEndpoints) {
         return generatedEndpoints.isEmpty() ? EMPTY : new GeneratedEndpointList(generatedEndpoints, false);
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
index 8f60bdb3a51..de25161c461 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/routing/RoutingPolicies.java
@@ -763,9 +763,9 @@ public class RoutingPolicies {
 
     private static void requireNonClashing(GeneratedEndpoint generatedEndpoint, RoutingPolicyList applicationPolicies) {
         for (var policy : applicationPolicies) {
-            for (var ge : policy.generatedEndpoints()) {
-                if (ge.clusterPart().equals(generatedEndpoint.clusterPart())) {
-                    throw new IllegalStateException(generatedEndpoint + " clashes with " + ge + " in " + policy.id());
+            for (var other : policy.generatedEndpoints()) {
+                if (other.clusterPart().equals(generatedEndpoint.clusterPart()) && !other.endpoint().equals(generatedEndpoint.endpoint())) {
+                    throw new IllegalStateException(generatedEndpoint + " clashes with " + other + " in " + policy.id());
                 }
             }
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
index 007e57cc615..8844b2deeac 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/RoutingPolicySerializerTest.java
@@ -48,7 +48,7 @@ public class RoutingPolicySerializerTest {
                                                  Set.of(),
                                                  RoutingStatus.DEFAULT,
                                                  false,
-                                                 GeneratedEndpointList.copyOf(List.of(new GeneratedEndpoint("deadbeef", "cafed00d", AuthMethod.mtls, Optional.of(EndpointId.of("foo")))))),
+                                                 GeneratedEndpointList.of(new GeneratedEndpoint("deadbeef", "cafed00d", AuthMethod.mtls, Optional.of(EndpointId.of("foo"))))),
                                new RoutingPolicy(id2,
                                                  Optional.of(HostName.of("long-and-ugly-name-2")),
                                                  Optional.empty(),
@@ -59,7 +59,7 @@ public class RoutingPolicySerializerTest {
                                                                    RoutingStatus.Agent.tenant,
                                                                    Instant.ofEpochSecond(123)),
                                                  true,
-                                                 GeneratedEndpointList.copyOf(List.of(new GeneratedEndpoint("cafed00d", "deadbeef", AuthMethod.token, Optional.empty())))),
+                                                 GeneratedEndpointList.of(new GeneratedEndpoint("cafed00d", "deadbeef", AuthMethod.token, Optional.empty()))),
                                new RoutingPolicy(id1,
                                                  Optional.empty(),
                                                  Optional.of("127.0.0.1"),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
index 7147c7ea709..dd7ab709161 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/RoutingPoliciesTest.java
@@ -1116,6 +1116,63 @@ public class RoutingPoliciesTest {
         assertEquals(List.of(), tester.recordNames());
     }
 
+    @Test
+    public void generated_endpoints_multi_instance() {
+        var tester = new RoutingPoliciesTester(SystemName.Public);
+        var context0 = tester.newDeploymentContext("tenant1", "app1", "default");
+        var context1 = tester.newDeploymentContext("tenant1", "app1", "beta");
+        tester.controllerTester().flagSource().withBooleanFlag(Flags.RANDOMIZED_ENDPOINT_NAMES.id(), true);
+        addCertificateToPool("cafed00d", UnassignedCertificate.State.ready, tester);
+
+        // Deploy application
+        int clustersPerZone = 1;
+        var zone1 = ZoneId.from("prod", "aws-us-east-1c");
+        ApplicationPackage applicationPackage = applicationPackageBuilder().instances("default,beta")
+                                                                           .region(zone1.region())
+                                                                           .container("c0", AuthMethod.mtls)
+                                                                           .applicationEndpoint("a0", "c0", Map.of(zone1.region().value(),
+                                                                                                                   Map.of(context0.instanceId().instance(), 1,
+                                                                                                                          context1.instanceId().instance(), 1)))
+                                                                           .build();
+        tester.provisionLoadBalancers(clustersPerZone, context0.instanceId(), zone1);
+        tester.provisionLoadBalancers(clustersPerZone, context1.instanceId(), zone1);
+        context0.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        assertEquals(List.of("a0.app1.tenant1.a.vespa-app.cloud",
+                             "a9c8c045.cafed00d.z.vespa-app.cloud",
+                             "c0.app1.tenant1.aws-us-east-1c.z.vespa-app.cloud",
+                             "c0.beta.app1.tenant1.aws-us-east-1c.z.vespa-app.cloud",
+                             "e144a11b.cafed00d.z.vespa-app.cloud",
+                             "ee82b867.cafed00d.a.vespa-app.cloud"),
+                     tester.recordNames());
+        tester.assertTargets(context0.application().id(), EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
+                             Map.of(context0.deploymentIdIn(zone1), 1, context1.deploymentIdIn(zone1), 1));
+
+        // Remove one instance from application endpoint
+        applicationPackage = applicationPackageBuilder().instances("default,beta")
+                                                        .region(zone1.region())
+                                                        .container("c0", AuthMethod.mtls)
+                                                        .applicationEndpoint("a0", "c0", Map.of(zone1.region().value(),
+                                                                                                Map.of(context1.instanceId().instance(), 1)))
+                                                        .build();
+        context0.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
+        assertEquals(List.of("a0.app1.tenant1.a.vespa-app.cloud",
+                             "a9c8c045.cafed00d.z.vespa-app.cloud",
+                             "c0.app1.tenant1.aws-us-east-1c.z.vespa-app.cloud",
+                             "c0.beta.app1.tenant1.aws-us-east-1c.z.vespa-app.cloud",
+                             "e144a11b.cafed00d.z.vespa-app.cloud",
+                             "ee82b867.cafed00d.a.vespa-app.cloud"),
+                     tester.recordNames());
+        tester.assertTargets(context0.application().id(), EndpointId.of("a0"), ClusterSpec.Id.from("c0"), 0,
+                             Map.of(context1.deploymentIdIn(zone1), 1));
+
+        // Removing application removes all records
+        context0.submit(ApplicationPackageBuilder.fromDeploymentXml("<deployment version='1.0'/>",
+                                                                   ValidationId.deploymentRemoval,
+                                                                   ValidationId.globalEndpointChange));
+        context0.flushDnsUpdates();
+        assertEquals(List.of(), tester.recordNames());
+    }
+
     private void addCertificateToPool(String id, UnassignedCertificate.State state, RoutingPoliciesTester tester) {
         EndpointCertificate cert = new EndpointCertificate("testKey", "testCert", 1, 0,
                                                            "request-id",