Merge pull request #15322 from vespa-engine/mpolden/check-deployment

Require valid deployment when changing routing status

2 files changed, 70 insertions, 10 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiHandler.java
index ace176bd91e..f26477beb3b 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiHandler.java
@@ -204,9 +204,10 @@ public class RoutingApiHandler extends AuditLoggingRequestHandler {
         var instance = controller.applications().requireInstance(deployment.applicationId());
         var status = in ? GlobalRouting.Status.in : GlobalRouting.Status.out;
         var agent = GlobalRouting.Agent.operator; // Always operator as this is an operator API
+        requireDeployment(deployment, instance);
 
-        // Set rotation status, if any assigned
-        if (rotationCanRouteTo(deployment.zoneId(), instance)) {
+        // Set rotation status, if rotations can route to this zone
+        if (rotationCanRouteTo(deployment.zoneId())) {
             var endpointStatus = new EndpointStatus(in ? EndpointStatus.Status.in : EndpointStatus.Status.out, "",
                                                     agent.name(),
                                                     controller.clock().instant().getEpochSecond());
@@ -239,9 +240,9 @@ public class RoutingApiHandler extends AuditLoggingRequestHandler {
                                   .collect(Collectors.toList())
                         : List.of(zoneId);
                 for (var zone : zones) {
-                    var deploymentId = new DeploymentId(instance.id(), zone);
+                    var deploymentId = requireDeployment(new DeploymentId(instance.id(), zone), instance);
                     // Include status from rotation
-                    if (rotationCanRouteTo(zone, instance)) {
+                    if (rotationCanRouteTo(zone)) {
                         var rotationStatus = controller.routing().globalRotationStatus(deploymentId);
                         // Status is equal across all global endpoints, as the status is per deployment, not per endpoint.
                         var endpointStatus = rotationStatus.values().stream().findFirst();
@@ -277,11 +278,12 @@ public class RoutingApiHandler extends AuditLoggingRequestHandler {
 
     }
 
-    /** Returns whether instance has an assigned rotation that can route to given zone */
-    private boolean rotationCanRouteTo(ZoneId zone, Instance instance) {
-        return !instance.rotations().isEmpty() &&
-               instance.deployments().containsKey(zone) &&
-               controller.zoneRegistry().routingMethods(zone).get(0).isShared();
+    /** Returns whether a rotation can route traffic to given zone */
+    private boolean rotationCanRouteTo(ZoneId zone) {
+        // A system may support multiple routing methods, i.e. it has both exclusively routed zones and zones using
+        // shared routing. When changing or reading routing status in the context of a specific deployment, rotation
+        // status should only be considered if the zone supports shared routing.
+        return controller.zoneRegistry().routingMethods(zone).stream().anyMatch(RoutingMethod::isShared);
     }
 
     private static void zoneStatusToSlime(Cursor object, ZoneId zone, GlobalRouting globalRouting, RoutingMethod method) {
@@ -331,6 +333,13 @@ public class RoutingApiHandler extends AuditLoggingRequestHandler {
         return zone;
     }
 
+    private static DeploymentId requireDeployment(DeploymentId deployment, Instance instance) {
+        if (!instance.deployments().containsKey(deployment.zoneId())) {
+            throw new IllegalArgumentException("No such deployment: " + deployment);
+        }
+        return deployment;
+    }
+
     private static boolean isRecursive(HttpRequest request) {
         return "true".equals(request.getProperty("recursive"));
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiTest.java
index 9bd8485db77..0a9e2dac49e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/routing/RoutingApiTest.java
@@ -262,7 +262,7 @@ public class RoutingApiTest extends ControllerContainerTest {
 
     // TODO(mpolden): Remove this once a zone supports either of routing policy and rotation
     @Test
-    public void mixed_routing() {
+    public void mixed_routing_single_zone() {
         var westZone = ZoneId.from("prod", "us-west-1");
         var eastZone = ZoneId.from("prod", "us-east-3");
 
@@ -303,6 +303,41 @@ public class RoutingApiTest extends ControllerContainerTest {
     }
 
     @Test
+    public void mixed_routing_multiple_zones() {
+        var westZone = ZoneId.from("prod", "us-west-1");
+        var eastZone = ZoneId.from("prod", "us-east-3");
+
+        // One shared and one exclusive zone
+        deploymentTester.controllerTester().zoneRegistry().setRoutingMethod(ZoneApiMock.from(westZone),
+                                                                            RoutingMethod.shared);
+        deploymentTester.controllerTester().zoneRegistry().setRoutingMethod(ZoneApiMock.from(eastZone),
+                                                                            RoutingMethod.exclusive);
+
+        // Deploy application
+        var context = deploymentTester.newDeploymentContext();
+        var applicationPackage = new ApplicationPackageBuilder()
+                .region(westZone.region())
+                .region(eastZone.region())
+                .athenzIdentity(AthenzDomain.from("domain"), AthenzService.from("service"))
+                .compileVersion(RoutingController.DIRECT_ROUTING_MIN_VERSION)
+                .endpoint("endpoint1", "default", westZone.region().value())
+                .endpoint("endpoint2", "default", eastZone.region().value())
+                .build();
+        context.submit(applicationPackage).deploy();
+
+        // GET status for zone using shared routing
+        tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/status/tenant/tenant/application/application/instance/default/environment/prod/region/us-west-1",
+                                              "", Request.Method.GET),
+                              new File("rotation/deployment-status-initial.json"));
+
+        // GET status for zone using exclusive routing
+        tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/status/tenant/tenant/application/application/instance/default/environment/prod/region/us-east-3",
+                                              "", Request.Method.GET),
+                              "{\"deployments\":[{\"routingMethod\":\"exclusive\",\"instance\":\"tenant:application:default\"," +
+                              "\"environment\":\"prod\",\"region\":\"us-east-3\",\"status\":\"in\",\"agent\":\"system\",\"changedAt\":0}]}");
+    }
+
+    @Test
     public void invalid_requests() {
         // GET non-existent application
         tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/status/tenant/t1/application/a1/instance/default/environment/prod/region/us-west-1",
@@ -310,6 +345,22 @@ public class RoutingApiTest extends ControllerContainerTest {
                               "{\"error-code\":\"BAD_REQUEST\",\"message\":\"t1.a1 not found\"}",
                               400);
 
+        // GET, DELETE non-existent deployment
+        var context = deploymentTester.newDeploymentContext();
+        var applicationPackage = new ApplicationPackageBuilder()
+                .region("us-east-3")
+                .endpoint("default", "default")
+                .build();
+        context.submit(applicationPackage).deploy();
+        tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/status/tenant/tenant/application/application/instance/default/environment/prod/region/us-west-1",
+                                              "", Request.Method.GET),
+                              "{\"error-code\":\"BAD_REQUEST\",\"message\":\"No such deployment: tenant.application in prod.us-west-1\"}",
+                              400);
+        tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/inactive/tenant/tenant/application/application/instance/default/environment/prod/region/us-west-1",
+                                              "", Request.Method.DELETE),
+                              "{\"error-code\":\"BAD_REQUEST\",\"message\":\"No such deployment: tenant.application in prod.us-west-1\"}",
+                              400);
+
         // GET non-existent zone
         tester.assertResponse(operatorRequest("http://localhost:8080/routing/v1/status/environment/prod/region/us-north-1",
                                                    "", Request.Method.GET),