Merge pull request #28774 from vespa-engine/mpolden/legacy-endpoints

Mark old endpoints as legacy if generated exist

7 files changed, 62 insertions, 39 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 b1ffce65852..90c4a506f10 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
@@ -197,19 +197,22 @@ public class RoutingController {
     /** Returns the zone- and region-scoped endpoints of given deployment */
     public EndpointList endpointsOf(DeploymentId deployment, ClusterSpec.Id cluster, GeneratedEndpointList generatedEndpoints) {
         requireGeneratedEndpoints(generatedEndpoints, false);
+        boolean generatedEndpointsAvailable = !generatedEndpoints.isEmpty();
         boolean tokenSupported = !generatedEndpoints.authMethod(AuthMethod.token).isEmpty();
-        RoutingMethod routingMethod = controller.zoneRegistry().routingMethod(deployment.zoneId());
         boolean isProduction = deployment.zoneId().environment().isProduction();
+        RoutingMethod routingMethod = controller.zoneRegistry().routingMethod(deployment.zoneId());
         List<Endpoint> endpoints = new ArrayList<>();
         Endpoint.EndpointBuilder zoneEndpoint = Endpoint.of(deployment.applicationId())
                                                         .routingMethod(routingMethod)
                                                         .on(Port.fromRoutingMethod(routingMethod))
+                                                        .legacy(generatedEndpointsAvailable)
                                                         .target(cluster, deployment);
         endpoints.add(zoneEndpoint.in(controller.system()));
         ZoneApi zone = controller.zoneRegistry().zones().all().get(deployment.zoneId()).get();
         Endpoint.EndpointBuilder regionEndpoint = Endpoint.of(deployment.applicationId())
                                                           .routingMethod(routingMethod)
                                                           .on(Port.fromRoutingMethod(routingMethod))
+                                                          .legacy(generatedEndpointsAvailable)
                                                           .targetRegion(cluster,
                                                                         zone.getCloudNativeRegionName(),
                                                                         zone.getCloudName());
@@ -226,12 +229,14 @@ public class RoutingController {
             };
             if (include) {
                 endpoints.add(zoneEndpoint.generatedFrom(generatedEndpoint)
+                                          .legacy(false)
                                           .authMethod(generatedEndpoint.authMethod())
                                           .in(controller.system()));
                 // Only a single region endpoint is needed, not one per auth method
                 if (isProduction && generatedEndpoint.authMethod() == AuthMethod.mtls) {
                     GeneratedEndpoint weightedGeneratedEndpoint = generatedEndpoint.withClusterPart(weightedClusterPart(cluster, deployment));
                     endpoints.add(regionEndpoint.generatedFrom(weightedGeneratedEndpoint)
+                                                .legacy(false)
                                                 .authMethod(AuthMethod.none)
                                                 .in(controller.system()));
                 }
@@ -257,6 +262,7 @@ public class RoutingController {
         var endpoints = new ArrayList<Endpoint>();
         var directMethods = 0;
         var availableRoutingMethods = routingMethodsOfAll(deployments);
+        boolean generatedEndpointsAvailable = !generatedEndpoints.isEmpty();
         for (var method : availableRoutingMethods) {
             if (method.isDirect() && ++directMethods > 1) {
                 throw new IllegalArgumentException("Invalid routing methods for " + routingId + ": Exceeded maximum " +
@@ -265,10 +271,11 @@ public class RoutingController {
             Endpoint.EndpointBuilder builder = Endpoint.of(routingId.instance())
                                                        .target(routingId.endpointId(), cluster, deployments)
                                                        .on(Port.fromRoutingMethod(method))
+                                                       .legacy(generatedEndpointsAvailable)
                                                        .routingMethod(method);
             endpoints.add(builder.in(controller.system()));
             for (var ge : generatedEndpoints) {
-                endpoints.add(builder.generatedFrom(ge).authMethod(ge.authMethod()).in(controller.system()));
+                endpoints.add(builder.generatedFrom(ge).legacy(false).authMethod(ge.authMethod()).in(controller.system()));
             }
         }
         return filterEndpoints(routingId.instance(), EndpointList.copyOf(endpoints));
@@ -280,16 +287,18 @@ public class RoutingController {
         requireGeneratedEndpoints(generatedEndpoints, true);
         ZoneId zone = deployments.keySet().iterator().next().zoneId(); // Where multiple zones are possible, they all have the same routing method.
         RoutingMethod routingMethod = usesSharedRouting(zone) ? RoutingMethod.sharedLayer4 : RoutingMethod.exclusive;
+        boolean generatedEndpointsAvailable = !generatedEndpoints.isEmpty();
         Endpoint.EndpointBuilder builder = Endpoint.of(application)
                                                    .targetApplication(endpoint,
                                                                       cluster,
                                                                       deployments)
                                                    .routingMethod(routingMethod)
+                                                   .legacy(generatedEndpointsAvailable)
                                                    .on(Port.fromRoutingMethod(routingMethod));
         List<Endpoint> endpoints = new ArrayList<>();
         endpoints.add(builder.in(controller.system()));
         for (var ge : generatedEndpoints) {
-            endpoints.add(builder.generatedFrom(ge).authMethod(ge.authMethod()).in(controller.system()));
+            endpoints.add(builder.generatedFrom(ge).legacy(false).authMethod(ge.authMethod()).in(controller.system()));
         }
         return EndpointList.copyOf(endpoints);
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
index 5c6611f80c3..2c13a7ddb11 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Endpoint.java
@@ -122,7 +122,7 @@ public class Endpoint {
         return scope;
     }
 
-    /** Returns whether this is considered a legacy DNS name that is due for removal */
+    /** Returns whether this is considered a legacy DNS name intended to be removed at some point */
     public boolean legacy() {
         return legacy;
     }
@@ -557,9 +557,9 @@ public class Endpoint {
             return this;
         }
 
-        /** Marks this as a legacy endpoint */
-        public EndpointBuilder legacy() {
-            this.legacy = true;
+        /** Set whether this is a legacy endpoint */
+        public EndpointBuilder legacy(boolean legacy) {
+            this.legacy = legacy;
             return this;
         }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/EndpointList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/EndpointList.java
index 310a78e45f0..6e8cd16336a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/EndpointList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/EndpointList.java
@@ -28,11 +28,6 @@ public class EndpointList extends AbstractFilteringList<Endpoint, EndpointList>
         }
     }
 
-    /** Returns the primary (non-legacy) endpoint, if any */
-    public Optional<Endpoint> primary() {
-        return not().legacy().asList().stream().findFirst();
-    }
-
     /** Returns the subset of endpoints named according to given ID and scope */
     public EndpointList named(EndpointId id, Endpoint.Scope scope) {
         return matching(endpoint -> endpoint.scope() == scope && // ID is only unique within a scope
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java
index 33af58a9790..b5e012253c7 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/certificate/EndpointCertificates.java
@@ -117,12 +117,12 @@ public class EndpointCertificates {
         // * Use per application certificate if it exits and is assigned a randomized id
         // * Assign from pool
 
-        Optional<AssignedCertificate> perInstanceAssignedCertificate = curator.readAssignedCertificate(TenantAndApplicationId.from(instance.id()), Optional.of(instance.name()));
+        TenantAndApplicationId application = TenantAndApplicationId.from(instance.id());
+        Optional<AssignedCertificate> perInstanceAssignedCertificate = curator.readAssignedCertificate(application, Optional.of(instance.name()));
         if (perInstanceAssignedCertificate.isPresent() && perInstanceAssignedCertificate.get().certificate().randomizedId().isPresent()) {
             return updateLastRequested(perInstanceAssignedCertificate.get()).certificate();
-        } else if (! zone.environment().isManuallyDeployed()){
-            TenantAndApplicationId application = TenantAndApplicationId.from(instance.id());
-            Optional<AssignedCertificate> perApplicationAssignedCertificate = curator.readAssignedCertificate(TenantAndApplicationId.from(instance.id()), Optional.empty());
+        } else if (! zone.environment().isManuallyDeployed()) {
+            Optional<AssignedCertificate> perApplicationAssignedCertificate = curator.readAssignedCertificate(application, Optional.empty());
             if (perApplicationAssignedCertificate.isPresent() && perApplicationAssignedCertificate.get().certificate().randomizedId().isPresent()) {
                 return updateLastRequested(perApplicationAssignedCertificate.get()).certificate();
             }
@@ -132,8 +132,6 @@ public class EndpointCertificates {
         // Assign certificate per instance only in manually deployed environments. In other environments, we share the
         // certificate because application endpoints can span instances
         Optional<InstanceName> instanceName = zone.environment().isManuallyDeployed() ? Optional.of(instance.name()) : Optional.empty();
-        TenantAndApplicationId application = TenantAndApplicationId.from(instance.id());
-
         try (Mutex lock = controller.curator().lockCertificatePool()) {
             Optional<UnassignedCertificate> candidate = curator.readUnassignedCertificates().stream()
                                                                .filter(pc -> pc.state() == State.ready)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index 411d6c48686..4fdbdb50836 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -175,7 +175,6 @@ import static com.yahoo.jdisc.Response.Status.CONFLICT;
 import static com.yahoo.vespa.hosted.controller.api.application.v4.EnvironmentResource.APPLICATION_TEST_ZIP;
 import static com.yahoo.vespa.hosted.controller.api.application.v4.EnvironmentResource.APPLICATION_ZIP;
 import static com.yahoo.yolean.Exceptions.uncheck;
-import static java.util.Comparator.comparing;
 import static java.util.Comparator.comparingInt;
 import static java.util.Map.Entry.comparingByKey;
 import static java.util.stream.Collectors.collectingAndThen;
@@ -2000,10 +1999,11 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
         response.setString("region", deploymentId.zoneId().region().value());
         addAvailabilityZone(response, deployment.zone());
         var application = controller.applications().requireApplication(TenantAndApplicationId.from(deploymentId.applicationId()));
-        boolean includeAllEndpoints = request.getBooleanProperty("includeAllEndpoints") ||
-                                      request.getBooleanProperty("includeLegacyEndpoints");
+        boolean includeAllEndpoints = request.getBooleanProperty("includeAllEndpoints");
+        boolean includeWeightedEndpoints = includeAllEndpoints || request.getBooleanProperty("includeWeightedEndpoints");
+        boolean includeLegacyEndpoints  = includeAllEndpoints || request.getBooleanProperty("includeLegacyEndpoints");
         var endpointArray = response.setArray("endpoints");
-        for (var endpoint : endpointsOf(deploymentId, application, includeAllEndpoints)) {
+        for (var endpoint : endpointsOf(deploymentId, application, includeLegacyEndpoints, includeWeightedEndpoints)) {
             toSlime(endpoint, endpointArray.addObject());
         }
         response.setString("clusters", withPath(toPath(deploymentId) + "/clusters", request.getUri()).toString());
@@ -2078,19 +2078,15 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
         metrics.instant().ifPresent(instant -> metricsObject.setLong("lastUpdated", instant.toEpochMilli()));
     }
 
-    private EndpointList endpointsOf(DeploymentId deploymentId, Application application, boolean includeHidden) {
+    private EndpointList endpointsOf(DeploymentId deploymentId, Application application, boolean includeLegacy, boolean includeWeighted) {
         EndpointList zoneEndpoints = controller.routing().readEndpointsOf(deploymentId).direct();
         EndpointList declaredEndpoints = controller.routing().readDeclaredEndpointsOf(application).targets(deploymentId);
         EndpointList endpoints = zoneEndpoints.and(declaredEndpoints);
-        EndpointList generatedEndpoints = endpoints.generated();
-        if (!includeHidden) {
-            // If we have generated endpoints, hide non-generated
-            if (!generatedEndpoints.isEmpty()) {
-                endpoints = endpoints.generated();
-            }
-            // Hide legacy and weighted endpoints
-            endpoints = endpoints.not().legacy()
-                                 .not().scope(Endpoint.Scope.weighted);
+        if (!includeLegacy) {
+            endpoints = endpoints.not().legacy();
+        }
+        if (!includeWeighted) {
+            endpoints = endpoints.not().scope(Endpoint.Scope.weighted);
         }
         return endpoints;
     }
@@ -2240,7 +2236,7 @@ public class ApplicationApiHandler extends AuditLoggingRequestHandler {
         Cursor array = slime.setObject().setArray("globalrotationoverride");
         Optional<Endpoint> primaryEndpoint = controller.routing().readDeclaredEndpointsOf(deploymentId.applicationId())
                                                        .requiresRotation()
-                                                       .primary();
+                                                       .first();
         if (primaryEndpoint.isPresent()) {
             DeploymentRoutingContext context = controller.routing().of(deploymentId);
             RoutingStatus status = context.routingStatus();
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 b2b34441219..22523103208 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
@@ -141,6 +141,13 @@ public class RoutingPoliciesTest {
         assertEquals(numberOfDeployments * clustersPerZone,
                 tester.policiesOf(context1.instance().id()).size(),
                 "Routing policy count is equal to cluster count");
+        assertEquals(List.of(),
+                     tester.controllerTester().controller().routing()
+                           .readDeclaredEndpointsOf(context1.instanceId())
+                           .scope(Endpoint.Scope.zone)
+                           .legacy()
+                           .asList(),
+                     "No endpoints marked as legacy");
 
         // Applications gains a new deployment
         ApplicationPackage applicationPackage2 = applicationPackageBuilder()
@@ -305,6 +312,13 @@ public class RoutingPoliciesTest {
         );
         assertEquals(expectedRecords, tester.recordNames());
         assertEquals(4, tester.policiesOf(context1.instanceId()).size());
+        assertEquals(List.of(),
+                     tester.controllerTester().controller().routing()
+                           .readEndpointsOf(context1.deploymentIdIn(zone1))
+                           .scope(Endpoint.Scope.zone)
+                           .legacy()
+                           .asList(),
+                     "No endpoints marked as legacy");
 
         // Next deploy does nothing
         context1.submit(applicationPackage).deferLoadBalancerProvisioningIn(Environment.prod).deploy();
@@ -1107,16 +1121,27 @@ public class RoutingPoliciesTest {
         assertEquals(6, tester.policiesOf(context.instanceId()).size());
         ClusterSpec.Id cluster0 = ClusterSpec.Id.from("c0");
         ClusterSpec.Id cluster1 = ClusterSpec.Id.from("c1");
+        // The expected number of endpoints are created
         for (var zone : List.of(zone1, zone2)) {
-            EndpointList generated = tester.controllerTester().controller().routing()
-                                           .readEndpointsOf(context.deploymentIdIn(zone))
-                                           .scope(Endpoint.Scope.zone)
-                                           .generated();
+            EndpointList zoneEndpoints = tester.controllerTester().controller().routing()
+                                               .readEndpointsOf(context.deploymentIdIn(zone))
+                                               .scope(Endpoint.Scope.zone);
+            EndpointList generated = zoneEndpoints.generated();
             assertEquals(1, generated.cluster(cluster0).size());
             assertEquals(0, generated.cluster(cluster0).authMethod(AuthMethod.token).size());
             assertEquals(2, generated.cluster(cluster1).size());
             assertEquals(1, generated.cluster(cluster1).authMethod(AuthMethod.token).size());
+            EndpointList legacy = zoneEndpoints.legacy();
+            assertEquals(1, legacy.cluster(cluster0).size());
+            assertEquals(0, legacy.cluster(cluster0).authMethod(AuthMethod.token).size());
+            assertEquals(1, legacy.cluster(cluster1).size());
+            assertEquals(0, legacy.cluster(cluster1).authMethod(AuthMethod.token).size());
         }
+        EndpointList declaredEndpoints = tester.controllerTester().controller().routing().readDeclaredEndpointsOf(context.application());
+        assertEquals(1, declaredEndpoints.scope(Endpoint.Scope.global).generated().size());
+        assertEquals(1, declaredEndpoints.scope(Endpoint.Scope.global).legacy().size());
+        assertEquals(1, declaredEndpoints.scope(Endpoint.Scope.application).generated().size());
+        assertEquals(1, declaredEndpoints.scope(Endpoint.Scope.application).legacy().size());
         Map<DeploymentId, Set<ContainerEndpoint>> containerEndpointsInProd = tester.containerEndpoints(Environment.prod);
 
         // Ordinary endpoints point to expected targets
@@ -1555,7 +1580,7 @@ public class RoutingPoliciesTest {
             } else {
                 global = global.not().generated();
             }
-            String globalEndpoint = global.primary()
+            String globalEndpoint = global.first()
                                           .map(Endpoint::dnsName)
                                           .orElse("<none>");
             assertEquals(latencyTargets, Set.copyOf(aliasDataOf(globalEndpoint)), "Global endpoint " + globalEndpoint + " points to expected latency targets");
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepositoryTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepositoryTest.java
index 6190680d098..e053e432862 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepositoryTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/routing/rotation/RotationRepositoryTest.java
@@ -146,7 +146,7 @@ public class RotationRepositoryTest {
         application2.submit(applicationPackage).deploy();
         assertEquals(List.of(new RotationId("foo-1")), rotationIds(application2.instance().rotations()));
         assertEquals("https://cd.app2.tenant2.global.cd.vespa.oath.cloud/",
-                tester.controller().routing().readDeclaredEndpointsOf(application2.instanceId()).primary().get().url().toString());
+                tester.controller().routing().readDeclaredEndpointsOf(application2.instanceId()).first().get().url().toString());
     }
 
     @Test