Revert "Allow setting cloud account for non-production environments"

4 files changed, 48 insertions, 42 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 92c3198175a..cb5bef81890 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
@@ -59,6 +59,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;
@@ -138,7 +139,6 @@ 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) {
@@ -153,7 +153,6 @@ 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,
@@ -179,11 +178,6 @@ public class ApplicationController {
         });
     }
 
-    /** Validate the given application package */
-    public void validatePackage(ApplicationPackage applicationPackage, Application application) {
-        applicationPackageValidator.validate(application, applicationPackage, clock.instant());
-    }
-
     /** Returns the application with the given id, or null if it is not present */
     public Optional<Application> getApplication(TenantAndApplicationId id) {
         return curator.readApplication(id);
@@ -544,7 +538,7 @@ public class ApplicationController {
 
     /** Stores the deployment spec and validation overrides from the application package, and runs cleanup. */
     public void storeWithUpdatedConfig(LockedApplication application, ApplicationPackage applicationPackage) {
-        validatePackage(applicationPackage, application.get());
+        applicationPackageValidator.validate(application.get(), applicationPackage, clock.instant());
 
         application = application.with(applicationPackage.deploymentSpec());
         application = application.with(applicationPackage.validationOverrides());
@@ -636,7 +630,9 @@ public class ApplicationController {
             List<X509Certificate> operatorCertificates = controller.supportAccess().activeGrantsFor(deployment).stream()
                                                                    .map(SupportAccessGrant::certificate)
                                                                    .collect(toList());
-            Optional<CloudAccount> cloudAccount = decideCloudAccountOf(deployment, applicationPackage.deploymentSpec());
+            Optional<CloudAccount> cloudAccount = applicationPackage.deploymentSpec()
+                                                                    .instance(application.instance())
+                                                                    .flatMap(spec -> spec.cloudAccount(zone.environment(), zone.region()));
             ConfigServer.PreparedApplication preparedApplication =
                     configServer.deploy(new DeploymentData(application, zone, applicationPackage.zippedContent(), platform,
                                                            endpoints, endpointCertificateMetadata, dockerImageRepo, domain,
@@ -653,30 +649,6 @@ 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 8e8a4e24970..5a131ba4a29 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,12 +7,17 @@ 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;
@@ -26,6 +31,7 @@ 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;
 
 /**
@@ -36,9 +42,11 @@ 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());
     }
 
     /**
@@ -47,6 +55,7 @@ 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);
@@ -81,10 +90,20 @@ public class ApplicationPackageValidator {
         for (var spec : deploymentSpec.instances()) {
             for (var zone : spec.zones()) {
                 Environment environment = zone.environment();
-                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!");
+                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, 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");
+                    }
                 }
             }
         }
@@ -166,6 +185,25 @@ 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().get());
+                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 965f1b09819..ef3474e0c1e 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,10 +258,6 @@ 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/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 993bf7fee19..31ac0606a1f 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
@@ -702,7 +702,7 @@ public class JobController {
         controller.applications().lockApplicationOrThrow(TenantAndApplicationId.from(id), application -> {
             if ( ! application.get().instances().containsKey(id.instance()))
                 application = controller.applications().withNewInstance(application, id);
-            controller.applications().validatePackage(applicationPackage, application.get());
+
             controller.applications().store(application);
         });