Merge pull request #14838 from vespa-engine/mpolden/hide-shared-endpoints

Hide shared endpoints when feature flag is set

6 files changed, 84 insertions, 69 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 5f2c187c9a6..ab97757e958 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
@@ -87,12 +87,9 @@ public class RoutingController {
         boolean isSystemApplication = SystemApplication.matching(deployment.applicationId()).isPresent();
         // Avoid reading application more than once per call to this
         var application = Suppliers.memoize(() -> controller.applications().requireApplication(TenantAndApplicationId.from(deployment.applicationId())));
-        var hideSharedEndpoints = hideSharedRoutingEndpoint.with(FetchVector.Dimension.APPLICATION_ID, deployment.applicationId().serializedForm()).value();
         for (var policy : routingPolicies.get(deployment).values()) {
             if (!policy.status().isActive()) continue;
             for (var routingMethod :  controller.zoneRegistry().routingMethods(policy.id().zone())) {
-                // Hide shared endpoints if configured for application, and the application can be routed to directly
-                if (hideSharedEndpoints && !routingMethod.isDirect() && !isSystemApplication && canRouteDirectlyTo(deployment, application.get())) continue;
                 if (routingMethod.isDirect() && !isSystemApplication && !canRouteDirectlyTo(deployment, application.get())) continue;
                 endpoints.add(policy.endpointIn(controller.system(), routingMethod, controller.zoneRegistry()));
                 endpoints.add(policy.regionEndpointIn(controller.system(), routingMethod));
@@ -142,9 +139,10 @@ public class RoutingController {
     public Map<ZoneId, List<Endpoint>> zoneEndpointsOf(Collection<DeploymentId> deployments) {
         var endpoints = new TreeMap<ZoneId, List<Endpoint>>(Comparator.comparing(ZoneId::value));
         for (var deployment : deployments) {
-            var zoneEndpoints = endpointsOf(deployment).scope(Endpoint.Scope.zone).asList();
+            EndpointList zoneEndpoints = endpointsOf(deployment).scope(Endpoint.Scope.zone);
+            zoneEndpoints = directEndpoints(zoneEndpoints, deployment.applicationId());
             if  ( ! zoneEndpoints.isEmpty()) {
-                endpoints.put(deployment.zoneId(), zoneEndpoints);
+                endpoints.put(deployment.zoneId(), zoneEndpoints.asList());
             }
         }
         return Collections.unmodifiableMap(endpoints);
@@ -291,16 +289,11 @@ public class RoutingController {
         var zones = deployments.stream().map(DeploymentId::zoneId).collect(Collectors.toList());
         var availableRoutingMethods = routingMethodsOfAll(deployments, application);
 
-        // Hide global shared endpoints if at least one direct method is supported
-        // and instances referenced by the global endpoint is configured to be hidden
-        var hideSharedEndpoints = availableRoutingMethods.stream().anyMatch(RoutingMethod::isDirect)
-                && hideSharedRoutingEndpoint.with(FetchVector.Dimension.APPLICATION_ID, routingId.application().serializedForm()).value();
         for (var method : availableRoutingMethods) {
             if (method.isDirect() && ++directMethods > 1) {
                 throw new IllegalArgumentException("Invalid routing methods for " + routingId + ": Exceeded maximum " +
                                                    "direct methods");
             }
-            if (hideSharedEndpoints && !method.isDirect()) continue;
             endpoints.add(Endpoint.of(routingId.application())
                                   .target(routingId.endpointId(), cluster, zones)
                                   .on(Port.fromRoutingMethod(method))
@@ -351,4 +344,16 @@ public class RoutingController {
 
     }
 
+    /** Returns direct routing endpoints if any exist and feature flag is set for given application */
+    // TODO: Remove this when feature flag is removed, and in-line .direct() filter where relevant
+    public EndpointList directEndpoints(EndpointList endpoints, ApplicationId application) {
+        boolean hideSharedEndpoint = hideSharedRoutingEndpoint.with(FetchVector.Dimension.APPLICATION_ID, application.serializedForm()).value();
+        EndpointList directEndpoints = endpoints.direct();
+        if (hideSharedEndpoint && !directEndpoints.isEmpty()) {
+            return directEndpoints;
+        }
+        return endpoints;
+    }
+
+
 }
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 e847667bf45..358086d453e 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
@@ -64,6 +64,11 @@ public class EndpointList extends AbstractFilteringList<Endpoint, EndpointList>
         return matching(endpoint -> endpoint.scope() == scope);
     }
 
+    /** Returns the subset of endpoints that use direct routing */
+    public EndpointList direct() {
+        return matching(endpoint -> endpoint.routingMethod().isDirect());
+    }
+
     public static EndpointList copyOf(Collection<Endpoint> endpoints) {
         return new EndpointList(endpoints, false);
     }
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 81b4c4cc72f..5d3459a671e 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
@@ -70,6 +70,7 @@ import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.DeploymentMetrics;
 import com.yahoo.vespa.hosted.controller.application.Endpoint;
+import com.yahoo.vespa.hosted.controller.application.EndpointList;
 import com.yahoo.vespa.hosted.controller.application.SystemApplication;
 import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentStatus;
@@ -104,7 +105,6 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.nio.file.Paths;
 import java.security.DigestInputStream;
 import java.security.Principal;
 import java.security.PublicKey;
@@ -1007,16 +1007,17 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
 
         // Add zone endpoints
         var endpointArray = response.setArray("endpoints");
-        for (var endpoint : controller.routing().endpointsOf(deploymentId)
-                                      .scope(Endpoint.Scope.zone)
-                                      .not().legacy()) {
+        EndpointList zoneEndpoints = controller.routing().endpointsOf(deploymentId)
+                                               .scope(Endpoint.Scope.zone)
+                                               .not().legacy();
+        for (var endpoint : controller.routing().directEndpoints(zoneEndpoints, deploymentId.applicationId())) {
             toSlime(endpoint, endpointArray.addObject());
         }
         // Add global endpoints
-        var globalEndpoints = controller.routing().endpointsOf(application, deploymentId.applicationId().instance())
-                                        .not().legacy()
-                                        .targets(deploymentId.zoneId());
-        for (var endpoint : globalEndpoints) {
+        EndpointList globalEndpoints = controller.routing().endpointsOf(application, deploymentId.applicationId().instance())
+                                                 .not().legacy()
+                                                 .targets(deploymentId.zoneId());
+        for (var endpoint : controller.routing().directEndpoints(globalEndpoints, deploymentId.applicationId())) {
             toSlime(endpoint, endpointArray.addObject());
         }
 
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 daf010b861a..1eb150c8a2b 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
@@ -18,8 +18,6 @@ import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.RoutingMethod;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.path.Path;
-import com.yahoo.vespa.flags.Flags;
-import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
@@ -931,55 +929,6 @@ public class ControllerTest {
     }
 
     @Test
-    public void testDirectRoutingSupportHidingSharedEndpoint() {
-        // TODO (mortent): remove this test when shared routing is gone
-        var context = tester.newDeploymentContext();
-        var zone1 = ZoneId.from("prod", "us-west-1");
-        var zone2 = ZoneId.from("prod", "us-east-3");
-        var zone3 = ZoneId.from("staging", "us-east-3");
-        var zone4 = ZoneId.from("test", "us-east-1");
-        var applicationPackageBuilder = new ApplicationPackageBuilder()
-                .region(zone1.region())
-                .region(zone2.region());
-        tester.controllerTester().zoneRegistry()
-                .setRoutingMethod(ZoneApiMock.from(zone1), RoutingMethod.shared, RoutingMethod.sharedLayer4)
-                .setRoutingMethod(ZoneApiMock.from(zone2), RoutingMethod.shared, RoutingMethod.sharedLayer4)
-                .setRoutingMethod(ZoneApiMock.from(zone3), RoutingMethod.shared, RoutingMethod.sharedLayer4)
-                .setRoutingMethod(ZoneApiMock.from(zone4), RoutingMethod.shared, RoutingMethod.sharedLayer4);
-        Supplier<Set<RoutingMethod>> routingMethods = () -> tester.controller().routing().endpointsOf(context.deploymentIdIn(zone1))
-                .asList()
-                .stream()
-                .map(Endpoint::routingMethod)
-                .collect(Collectors.toSet());
-
-        ((InMemoryFlagSource)tester.controller().flagSource()).withBooleanFlag(Flags.HIDE_SHARED_ROUTING_ENDPOINT.id(), true);
-        // Without satisfying any requirement
-        context.submit(applicationPackageBuilder.build()).deploy();
-        assertEquals(Set.of(RoutingMethod.shared), routingMethods.get());
-
-        // Without satisfying Athenz service requirement
-        context.submit(applicationPackageBuilder.compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION).build())
-                .deploy();
-        assertEquals(Set.of(RoutingMethod.shared), routingMethods.get());
-
-        // Satisfying all requirements
-        context.submit(applicationPackageBuilder.compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION)
-                .athenzIdentity(AthenzDomain.from("domain"), AthenzService.from("service"))
-                .build()).deploy();
-        assertEquals(Set.of(RoutingMethod.sharedLayer4), routingMethods.get());
-
-        // Global endpoint is added and includes directly routed endpoint name
-        applicationPackageBuilder = applicationPackageBuilder.endpoint("default", "default");
-        context.submit(applicationPackageBuilder.build()).deploy();
-        for (var zone : List.of(zone1, zone2)) {
-            assertEquals(Set.of("rotation-id-01",
-                    "application.tenant.global.vespa.oath.cloud"),
-                    tester.configServer().rotationNames().get(context.deploymentIdIn(zone)));
-        }
-
-    }
-
-    @Test
     public void testChangeEndpointCluster() {
         var context = tester.newDeploymentContext();
         var west = ZoneId.from("prod", "us-west-1");
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 8a16e066119..dfe2930e2a8 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -20,6 +20,8 @@ import com.yahoo.vespa.athenz.api.AthenzPrincipal;
 import com.yahoo.vespa.athenz.api.AthenzUser;
 import com.yahoo.vespa.athenz.api.OktaAccessToken;
 import com.yahoo.vespa.athenz.api.OktaIdentityToken;
+import com.yahoo.vespa.flags.Flags;
+import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.ControllerTester;
 import com.yahoo.vespa.hosted.controller.Instance;
@@ -1474,6 +1476,14 @@ public class ApplicationApiTest extends ControllerContainerTest {
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-west-1/instance/instance1", GET)
                                       .userIdentity(USER_ID),
                               new File("deployment-with-routing-policy.json"));
+
+        // Hide shared endpoints
+        ((InMemoryFlagSource) tester.controller().flagSource()).withBooleanFlag(Flags.HIDE_SHARED_ROUTING_ENDPOINT.id(), true);
+
+        // GET deployment
+        tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-west-1/instance/instance1", GET)
+                                      .userIdentity(USER_ID),
+                              new File("deployment-without-shared-endpoints.json"));
     }
 
     private MultiPartStreamer createApplicationDeployData(ApplicationPackage applicationPackage, boolean deployDirectly) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-without-shared-endpoints.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-without-shared-endpoints.json
new file mode 100644
index 00000000000..1c06991a3a7
--- /dev/null
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-without-shared-endpoints.json
@@ -0,0 +1,45 @@
+{
+  "tenant": "tenant1",
+  "application": "application1",
+  "instance": "instance1",
+  "environment": "prod",
+  "region": "us-west-1",
+  "endpoints": [
+    {
+      "cluster": "default",
+      "tls": true,
+      "url": "https://instance1.application1.tenant1.us-west-1.vespa.oath.cloud/",
+      "scope": "zone",
+      "routingMethod": "exclusive"
+    }
+  ],
+  "nodes": "http://localhost:8080/zone/v2/prod/us-west-1/nodes/v2/node/?recursive=true&application=tenant1.application1.instance1",
+  "yamasUrl": "http://monitoring-system.test/?environment=prod&region=us-west-1&application=tenant1.application1.instance1",
+  "version": "6.1.0",
+  "revision": "1.0.1-commit1",
+  "deployTimeEpochMs": "(ignore)",
+  "screwdriverId": "1000",
+  "gitRepository": "repository1",
+  "gitBranch": "master",
+  "gitCommit": "commit1",
+  "applicationVersion": {
+    "hash": "1.0.1-commit1",
+    "build": 1,
+    "source": {
+      "gitRepository": "repository1",
+      "gitBranch": "master",
+      "gitCommit": "commit1"
+    },
+    "sourceUrl": "repository1/tree/commit1",
+    "commit": "commit1"
+  },
+  "status": "complete",
+  "activity": {},
+  "metrics": {
+    "queriesPerSecond": 0.0,
+    "writesPerSecond": 0.0,
+    "documentCount": 0.0,
+    "queryLatencyMillis": 0.0,
+    "writeLatencyMillis": 0.0
+  }
+}