Merge pull request #26661 from vespa-engine/jonmv/validate-cloud-account-for-deployments-before-storing

Validate all future jobs (submit) and current (deploy) have zone in a…

4 files changed, 23 insertions, 6 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index a1534ebc533..44ea693d8cd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -47,6 +47,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.deployment.RevisionId;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.TesterId;
 import com.yahoo.vespa.hosted.controller.api.integration.noderepository.RestartFilter;
 import com.yahoo.vespa.hosted.controller.api.integration.secrets.TenantSecretStore;
+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.DeploymentMetrics.Warning;
@@ -60,6 +61,7 @@ import com.yahoo.vespa.hosted.controller.application.pkg.ApplicationPackageValid
 import com.yahoo.vespa.hosted.controller.athenz.impl.AthenzFacade;
 import com.yahoo.vespa.hosted.controller.certificate.EndpointCertificates;
 import com.yahoo.vespa.hosted.controller.concurrent.Once;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentStatus;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger;
 import com.yahoo.vespa.hosted.controller.deployment.JobStatus;
 import com.yahoo.vespa.hosted.controller.deployment.Run;
@@ -586,7 +588,16 @@ public class ApplicationController {
         }
 
         // Validate new deployment spec thoroughly before storing it.
-        controller.jobController().deploymentStatus(application.get());
+        DeploymentStatus status = controller.jobController().deploymentStatus(application.get());
+        Change dummyChange = Change.of(RevisionId.forProduction(Long.MAX_VALUE)); // Should always run everywhere.
+        for (var jobs : status.jobsToRun(applicationPackage.deploymentSpec().instanceNames().stream()
+                                                           .collect(toMap(name -> name, __ -> dummyChange)))
+                              .entrySet()) {
+            for (var job : jobs.getValue()) {
+                decideCloudAccountOf(new DeploymentId(jobs.getKey().application(), job.type().zone()),
+                                     applicationPackage.deploymentSpec());
+            }
+        }
 
         for (Notification notification : controller.notificationsDb().listNotifications(NotificationSource.from(application.get().id()), true)) {
             if (   notification.source().instance().isPresent()
@@ -696,7 +707,7 @@ public class ApplicationController {
             throw new IllegalArgumentException("Requested cloud account '" + requestedAccount.get().value() +
                                                "' is not valid for tenant '" + tenant + "'");
         }
-        if (!controller.zoneRegistry().hasZone(zoneId, requestedAccount.get())) {
+        if ( ! controller.zoneRegistry().hasZone(zoneId, requestedAccount.get())) {
             throw new IllegalArgumentException("Zone " + zoneId + " is not configured in requested cloud account '" +
                                                requestedAccount.get().value() + "'");
         }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
index a76e76611c2..050b77a391e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
@@ -390,7 +390,7 @@ public class DeploymentStatus {
     }
 
     /** The set of jobs that need to run for the given changes to be considered complete. */
-    private Map<JobId, List<Job>> jobsToRun(Map<InstanceName, Change> changes) {
+    public Map<JobId, List<Job>> jobsToRun(Map<InstanceName, Change> changes) {
         return jobsToRun(changes, false);
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index 73c64be3e47..10e4052f067 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -713,6 +713,7 @@ public class JobController {
             // TODO(mpolden): Enable for public CD once all tests have been updated
             if (controller.system() != SystemName.PublicCd) {
                 controller.applications().validatePackage(applicationPackage, application.get());
+                controller.applications().decideCloudAccountOf(new DeploymentId(id, type.zone()), applicationPackage.deploymentSpec());
             }
             controller.applications().store(application);
         });
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 8f0536480f5..d9ee82f5e90 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
@@ -1429,9 +1429,14 @@ public class ControllerTest {
 
         // Deployment fails because zone is not configured in requested cloud account
         tester.controllerTester().flagSource().withListFlag(PermanentFlags.CLOUD_ACCOUNTS.id(), List.of(cloudAccount), String.class);
-        context.submit(applicationPackage)
-               .runJobExpectingFailure(systemTest, "Zone test.us-east-1 is not configured in requested cloud account '012345678912'")
-               .abortJob(stagingTest);
+        assertEquals("Zone test.us-east-1 is not configured in requested cloud account '012345678912'",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> context.submit(applicationPackage))
+                             .getMessage());
+        assertEquals("Zone dev.us-east-1 is not configured in requested cloud account '012345678912'",
+                     assertThrows(IllegalArgumentException.class,
+                                  () -> context.runJob(devUsEast1, applicationPackage))
+                             .getMessage());
 
         // Deployment to prod succeeds once all zones are configured in requested account
         tester.controllerTester().zoneRegistry().configureCloudAccount(CloudAccount.from(cloudAccount),