Validate cloud account at deployment time

8 files changed, 99 insertions, 85 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 64e2c20e68c..259c091e074 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
@@ -138,6 +138,7 @@ public class ApplicationController {
     private final StringFlag dockerImageRepoFlag;
     private final ListFlag<String> incompatibleVersions;
     private final BillingController billingController;
+    private final ListFlag<String> cloudAccountsFlag;
 
     ApplicationController(Controller controller, CuratorDb curator, AccessControl accessControl, Clock clock,
                           FlagSource flagSource, BillingController billingController) {
@@ -152,6 +153,7 @@ public class ApplicationController {
         applicationStore = controller.serviceRegistry().applicationStore();
         dockerImageRepoFlag = PermanentFlags.DOCKER_IMAGE_REPO.bindTo(flagSource);
         incompatibleVersions = PermanentFlags.INCOMPATIBLE_VERSIONS.bindTo(flagSource);
+        cloudAccountsFlag = PermanentFlags.CLOUD_ACCOUNTS.bindTo(flagSource);
         deploymentTrigger = new DeploymentTrigger(controller, clock);
         applicationPackageValidator = new ApplicationPackageValidator(controller);
         endpointCertificates = new EndpointCertificates(controller,
@@ -629,9 +631,7 @@ public class ApplicationController {
             List<X509Certificate> operatorCertificates = controller.supportAccess().activeGrantsFor(deployment).stream()
                                                                    .map(SupportAccessGrant::certificate)
                                                                    .collect(toList());
-            Optional<CloudAccount> cloudAccount = applicationPackage.deploymentSpec()
-                                                                    .instance(application.instance())
-                                                                    .flatMap(spec -> spec.cloudAccount(zone.environment(), Optional.of(zone.region())));
+            Optional<CloudAccount> cloudAccount = decideCloudAccountOf(deployment, applicationPackage.deploymentSpec());
             ConfigServer.PreparedApplication preparedApplication =
                     configServer.deploy(new DeploymentData(application, zone, applicationPackage.zippedContent(), platform,
                                                            endpoints, endpointCertificateMetadata, dockerImageRepo, domain,
@@ -648,6 +648,30 @@ public class ApplicationController {
         }
     }
 
+    private Optional<CloudAccount> decideCloudAccountOf(DeploymentId deployment, DeploymentSpec spec) {
+        ZoneId zoneId = deployment.zoneId();
+        Optional<CloudAccount> requestedAccount = spec.instance(deployment.applicationId().instance())
+                                                      .flatMap(instanceSpec -> instanceSpec.cloudAccount(zoneId.environment(),
+                                                                                                         Optional.of(zoneId.region())));
+        if (requestedAccount.isEmpty()) {
+            return Optional.empty();
+        }
+        TenantName tenant = deployment.applicationId().tenant();
+        Set<CloudAccount> tenantAccounts = cloudAccountsFlag.with(FetchVector.Dimension.TENANT_ID, tenant.value())
+                                                            .value().stream()
+                                                            .map(CloudAccount::new)
+                                                            .collect(Collectors.toSet());
+        if (!tenantAccounts.contains(requestedAccount.get())) {
+            throw new IllegalArgumentException("Requested cloud account '" + requestedAccount.get().value() +
+                                               "' is not valid for tenant '" + tenant + "'");
+        }
+        if (!controller.zoneRegistry().hasZone(zoneId, requestedAccount.get())) {
+            throw new IllegalArgumentException("Zone " + zoneId + " is not configured in requested cloud account '" +
+                                               requestedAccount.get().value() + "'");
+        }
+        return requestedAccount;
+    }
+
     private LockedApplication withoutDeletedDeployments(LockedApplication application, InstanceName instance) {
         DeploymentSpec deploymentSpec = application.get().deploymentSpec();
         List<ZoneId> deploymentsToRemove = application.get().require(instance).productionDeployments().values().stream()
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
index cb48b4aa621..8e8a4e24970 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/pkg/ApplicationPackageValidator.java
@@ -7,17 +7,12 @@ import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.application.api.Endpoint;
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.application.api.ValidationOverrides;
-import com.yahoo.config.provision.CloudAccount;
 import com.yahoo.config.provision.CloudName;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
-import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneApi;
 import com.yahoo.config.provision.zone.ZoneId;
-import com.yahoo.vespa.flags.FetchVector;
-import com.yahoo.vespa.flags.ListFlag;
-import com.yahoo.vespa.flags.PermanentFlags;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.application.EndpointId;
@@ -31,7 +26,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.Set;
 import java.util.stream.Collectors;
 
 /**
@@ -42,11 +36,9 @@ import java.util.stream.Collectors;
 public class ApplicationPackageValidator {
 
     private final Controller controller;
-    private final ListFlag<String> cloudAccountsFlag;
 
     public ApplicationPackageValidator(Controller controller) {
         this.controller = Objects.requireNonNull(controller, "controller must be non-null");
-        this.cloudAccountsFlag = PermanentFlags.CLOUD_ACCOUNTS.bindTo(controller.flagSource());
     }
 
     /**
@@ -55,7 +47,6 @@ public class ApplicationPackageValidator {
      * @throws IllegalArgumentException if any validations fail
      */
     public void validate(Application application, ApplicationPackage applicationPackage, Instant instant) {
-        validateCloudAccounts(application, applicationPackage.deploymentSpec());
         validateSteps(applicationPackage.deploymentSpec());
         validateEndpointRegions(applicationPackage.deploymentSpec());
         validateEndpointChange(application, applicationPackage, instant);
@@ -90,20 +81,10 @@ public class ApplicationPackageValidator {
         for (var spec : deploymentSpec.instances()) {
             for (var zone : spec.zones()) {
                 Environment environment = zone.environment();
-                if (environment.isManuallyDeployed())
-                    throw new IllegalArgumentException("region must be one with automated deployments, but got: " + environment);
-
-                if (environment == Environment.prod) {
-                    RegionName region = zone.region().orElseThrow();
-                    if (!controller.zoneRegistry().hasZone(ZoneId.from(environment, region))) {
-                        throw new IllegalArgumentException("Zone " + zone + " in deployment spec was not found in this system!");
-                    }
-                    Optional<CloudAccount> cloudAccount = spec.cloudAccount(environment, zone.region());
-                    if (cloudAccount.isPresent() && !controller.zoneRegistry().hasZone(ZoneId.from(environment, region), cloudAccount.get())) {
-                        throw new IllegalArgumentException("Zone " + zone + " in deployment spec is not configured for " +
-                                                           "use in cloud account '" + cloudAccount.get().value() +
-                                                           "', in this system");
-                    }
+                if (zone.region().isEmpty()) continue;
+                ZoneId zoneId = ZoneId.from(environment, zone.region().get());
+                if (!controller.zoneRegistry().hasZone(zoneId)) {
+                    throw new IllegalArgumentException("Zone " + zone + " in deployment spec was not found in this system!");
                 }
             }
         }
@@ -185,25 +166,6 @@ public class ApplicationPackageValidator {
                                            ". " + ValidationOverrides.toAllowMessage(validationId));
     }
 
-    /** Verify that declared cloud accounts are allowed to be used by the tenant */
-    private void validateCloudAccounts(Application application, DeploymentSpec deploymentSpec) {
-        TenantName tenant = application.id().tenant();
-        Set<CloudAccount> validAccounts = cloudAccountsFlag.with(FetchVector.Dimension.TENANT_ID, tenant.value())
-                                                           .value().stream()
-                                                           .map(CloudAccount::new)
-                                                           .collect(Collectors.toSet());
-        for (var spec : deploymentSpec.instances()) {
-            for (var zone : spec.zones()) {
-                if (!zone.environment().isProduction()) continue;
-                Optional<CloudAccount> cloudAccount = spec.cloudAccount(zone.environment(), zone.region());
-                if (cloudAccount.isEmpty()) continue;
-                if (validAccounts.contains(cloudAccount.get())) continue;
-                throw new IllegalArgumentException("Cloud account '" + cloudAccount.get().value() +
-                                                   "' is not valid for tenant '" + tenant + "'");
-            }
-        }
-    }
-
     /** Returns whether newEndpoints contains all destinations in endpoints */
     private static boolean containsAllDestinationsOf(List<Endpoint> endpoints, List<Endpoint> newEndpoints) {
         var containsAllRegions = true;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
index ef3474e0c1e..965f1b09819 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java
@@ -258,6 +258,10 @@ public class InternalStepRunner implements StepRunner {
 
             throw e;
         }
+        catch (IllegalArgumentException e) {
+            logger.log(WARNING, e.getMessage());
+            return Optional.of(deploymentFailed);
+        }
         catch (EndpointCertificateException e) {
             switch (e.type()) {
                 case CERT_NOT_AVAILABLE:
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 96ca239b527..0c84016ad06 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
@@ -67,6 +67,7 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.config.provision.SystemName.main;
+import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.devUsEast1;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.productionUsEast3;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.productionUsWest1;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.stagingTest;
@@ -92,6 +93,7 @@ public class ControllerTest {
     void testDeployment() {
         // Setup system
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .explicitEnvironment(Environment.dev, Environment.perf)
                 .region("us-west-1")
                 .region("us-east-3")
                 .build();
@@ -1271,33 +1273,38 @@ public class ControllerTest {
     @Test
     void testCloudAccount() {
         DeploymentContext context = tester.newDeploymentContext();
-        ZoneId zone = ZoneId.from("prod", "us-west-1");
+        ZoneId devZone = devUsEast1.zone();
+        ZoneId prodZone = productionUsWest1.zone();
         String cloudAccount = "012345678912";
         var applicationPackage = new ApplicationPackageBuilder()
                 .cloudAccount(cloudAccount)
-                .region(zone.region())
+                .region(prodZone.region())
                 .build();
-        // Deployment fails because cloud account is not declared for this tenant
-        try {
-            context.submit(applicationPackage).deploy();
-            fail("Expected exception");
-        } catch (IllegalArgumentException e) {
-            assertEquals("Cloud account '012345678912' is not valid for tenant 'tenant'", e.getMessage());
-        }
+        // Prod and dev deployments fail because cloud account is not declared for this tenant
+        context.submit(applicationPackage).runJobExpectingFailure(systemTest, "Requested cloud account '012345678912' is not valid for tenant 'tenant'");
 
-        // Deployment fails because requested region is not configured in cloud account
+        // Deployment fails because zone is not configured in requested cloud account
         tester.controllerTester().flagSource().withListFlag(PermanentFlags.CLOUD_ACCOUNTS.id(), List.of(cloudAccount), String.class);
-        try {
-            context.submit(applicationPackage).deploy();
-            fail("Expected exception");
-        } catch (IllegalArgumentException e) {
-            assertEquals("Zone prod.us-west-1 in deployment spec is not configured for use in cloud account '012345678912', in this system", e.getMessage());
-        }
-
-        // Deployment succeeds
-        tester.controllerTester().zoneRegistry().setCloudAccountZones(new CloudAccount(cloudAccount), zone);
+        context.runJobExpectingFailure(systemTest, "Zone test.us-east-1 is not configured in requested cloud account '012345678912'")
+               .abortJob(stagingTest);
+
+        // Deployment to prod succeeds once all zones are configured in requested account
+        tester.controllerTester().zoneRegistry().configureCloudAccount(new CloudAccount(cloudAccount),
+                                                                       systemTest.zone(),
+                                                                       stagingTest.zone(),
+                                                                       prodZone);
         context.submit(applicationPackage).deploy();
-        assertEquals(cloudAccount, tester.controllerTester().configServer().cloudAccount(context.deploymentIdIn(zone)).get().value());
+
+        // Dev zone is added as a configured zone and deployment succeeds
+        tester.controllerTester().zoneRegistry().configureCloudAccount(new CloudAccount(cloudAccount), devZone);
+        context.runJob(devZone, applicationPackage);
+
+        // All deployments use the custom account
+        for (var zoneId : List.of(systemTest.zone(), stagingTest.zone(), devZone, prodZone)) {
+            assertEquals(cloudAccount, tester.controllerTester().configServer()
+                                             .cloudAccount(context.deploymentIdIn(zoneId))
+                                             .get().value());
+        }
     }
 
     @Test
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index 146135491a3..8ddd0ef2be3 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -5,6 +5,7 @@ import com.yahoo.component.Version;
 import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.provision.AthenzDomain;
 import com.yahoo.config.provision.AthenzService;
+import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.security.SignatureAlgorithm;
@@ -26,6 +27,7 @@ import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Date;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.OptionalInt;
@@ -55,6 +57,7 @@ public class ApplicationPackageBuilder {
     private final StringBuilder endpointsBody = new StringBuilder();
     private final StringBuilder applicationEndpointsBody = new StringBuilder();
     private final List<X509Certificate> trustedCertificates = new ArrayList<>();
+    private final Map<Environment, Map<String, String>> nonProductionEnvironments = new LinkedHashMap<>();
 
     private OptionalInt majorVersion = OptionalInt.empty();
     private String instances = "default";
@@ -65,8 +68,6 @@ public class ApplicationPackageBuilder {
     private String globalServiceId = null;
     private String athenzIdentityAttributes = "athenz-domain='domain' athenz-service='service'";
     private String searchDefinition = "search test { }";
-    private boolean explicitSystemTest = false;
-    private boolean explicitStagingTest = false;
     private Version compileVersion = Version.fromString("6.1");
     private String cloudAccount = null;
 
@@ -135,12 +136,22 @@ public class ApplicationPackageBuilder {
     }
 
     public ApplicationPackageBuilder systemTest() {
-        explicitSystemTest = true;
-        return this;
+        return explicitEnvironment(Environment.test);
     }
 
     public ApplicationPackageBuilder stagingTest() {
-        explicitStagingTest = true;
+        return explicitEnvironment(Environment.staging);
+    }
+
+    public ApplicationPackageBuilder explicitEnvironment(Environment environment, Environment... rest) {
+        Stream.concat(Stream.of(environment), Arrays.stream(rest))
+              .forEach(env -> nonProductionEnvironment(env, Map.of()));
+        return this;
+    }
+
+    private ApplicationPackageBuilder nonProductionEnvironment(Environment environment, Map<String, String> attributes) {
+        if (environment.isProduction()) throw new IllegalArgumentException("Expected non-production environment, got " + environment);
+        nonProductionEnvironments.put(environment, attributes);
         return this;
     }
 
@@ -267,6 +278,10 @@ public class ApplicationPackageBuilder {
         return this;
     }
 
+    public ApplicationPackageBuilder cloudAccount(Environment environment, String cloudAccount) {
+        return nonProductionEnvironment(environment, Map.of("cloud-account", cloudAccount));
+    }
+
     private byte[] deploymentSpec() {
         StringBuilder xml = new StringBuilder();
         xml.append("<deployment version='1.0' ");
@@ -291,10 +306,13 @@ public class ApplicationPackageBuilder {
                 xml.append("/>\n");
             }
             xml.append(notifications);
-            if (explicitSystemTest)
-                xml.append("    <test />\n");
-            if (explicitStagingTest)
-                xml.append("    <staging />\n");
+            nonProductionEnvironments.forEach((environment, attributes) -> {
+                xml.append("    <").append(environment.value());
+                attributes.forEach((attribute, value) -> {
+                    xml.append(" ").append(attribute).append("='").append(value).append("'");
+                });
+                xml.append(" />\n");
+            });
             xml.append(blockChange);
             xml.append("    <prod");
             if (globalServiceId != null) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index 934c6b07c29..8dd110e7df0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -338,18 +338,18 @@ public class DeploymentContext {
 
     /** Fail current deployment in given job */
     public DeploymentContext failDeployment(JobType type) {
-        return failDeployment(type, new IllegalArgumentException("Exception from test code"));
+        return failDeployment(type, new RuntimeException("Exception from test code"));
     }
 
     /** Fail current deployment in given job */
     private DeploymentContext failDeployment(JobType type, RuntimeException exception) {
         configServer().throwOnNextPrepare(exception);
-        runJobExpectingFailure(type, Optional.empty());
+        runJobExpectingFailure(type, null);
         return this;
     }
 
     /** Run given job and expect it to fail with given message, if any */
-    public DeploymentContext runJobExpectingFailure(JobType type, Optional<String> messagePart) {
+    public DeploymentContext runJobExpectingFailure(JobType type, String messagePart) {
         triggerJobs();
         var job = jobId(type);
         RunId id = currentRun(job).id();
@@ -357,7 +357,7 @@ public class DeploymentContext {
         Run run = jobs.run(id);
         assertTrue(run.hasFailed());
         assertTrue(run.hasEnded());
-        if (messagePart.isPresent()) {
+        if (messagePart != null) {
             Optional<Step> firstFailing = run.stepStatuses().entrySet().stream()
                                              .filter(kv -> kv.getValue() == failed)
                                              .map(Entry::getKey)
@@ -366,8 +366,8 @@ public class DeploymentContext {
             Optional<RunLog> details = jobs.details(id);
             assertTrue(details.isPresent(), "Found log entries for run " + id);
             assertTrue(details.get().get(firstFailing.get()).stream()
-                              .anyMatch(entry -> entry.message().contains(messagePart.get())),
-                       "Found log message containing '" + messagePart.get() + "'");
+                              .anyMatch(entry -> entry.message().contains(messagePart)),
+                       "Found log message containing '" + messagePart + "'");
         }
         return this;
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
index 680e69998b6..0211a052f76 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java
@@ -148,8 +148,8 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry
         return this;
     }
 
-    public ZoneRegistryMock setCloudAccountZones(CloudAccount cloudAccount, ZoneId... zones) {
-        this.cloudAccountZones.put(cloudAccount, Set.of(zones));
+    public ZoneRegistryMock configureCloudAccount(CloudAccount cloudAccount, ZoneId... zones) {
+        this.cloudAccountZones.computeIfAbsent(cloudAccount, (k) -> new HashSet<>()).addAll(Set.of(zones));
         return this;
     }
 
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 7477b94e3f4..cd8a9e72051 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
@@ -17,7 +17,6 @@ import org.junit.jupiter.api.Test;
 
 import java.net.URI;
 import java.util.List;
-import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -114,7 +113,7 @@ public class RotationRepositoryTest {
         // We're now out of rotations and next deployment fails
         var application3 = tester.newDeploymentContext("tenant3", "app3", "default");
         application3.submit(applicationPackage)
-                .runJobExpectingFailure(DeploymentContext.systemTest, Optional.of("out of rotations"));
+                    .runJobExpectingFailure(DeploymentContext.systemTest, "out of rotations");
     }
 
     @Test
@@ -123,7 +122,7 @@ public class RotationRepositoryTest {
                 .globalServiceId("foo")
                 .region("us-east-3")
                 .build();
-        application.submit(applicationPackage).runJobExpectingFailure(DeploymentContext.systemTest, Optional.of("less than 2 prod zones are defined"));
+        application.submit(applicationPackage).runJobExpectingFailure(DeploymentContext.systemTest, "less than 2 prod zones are defined");
     }
 
     @Test