diff options
17 files changed, 136 insertions, 190 deletions
diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index 7e17cd0f600..672ad4071d6 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -208,7 +208,7 @@ "public boolean canUpgradeAt(java.time.Instant)", "public boolean canChangeRevisionAt(java.time.Instant)", "public java.util.Optional athenzService(com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", - "public java.util.Optional cloudAccount(com.yahoo.config.provision.Environment, java.util.Optional)", + "public java.util.Optional cloudAccount(com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", "public com.yahoo.config.application.api.Notifications notifications()", "public java.util.List endpoints()", "public boolean deploysTo(com.yahoo.config.provision.Environment, com.yahoo.config.provision.RegionName)", diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java index cd20b5b8910..8cb70d50a59 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java @@ -226,9 +226,10 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { } /** Returns the cloud account to use for given environment and region, if any */ - public Optional<CloudAccount> cloudAccount(Environment environment, Optional<RegionName> region) { + public Optional<CloudAccount> cloudAccount(Environment environment, RegionName region) { + if (!environment.isProduction()) return Optional.empty(); return zones().stream() - .filter(zone -> zone.concerns(environment, region)) + .filter(zone -> zone.concerns(environment, Optional.of(region))) .findFirst() .flatMap(DeploymentSpec.DeclaredZone::cloudAccount) .or(() -> cloudAccount); diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java index ffdceaf7567..c4fec6e668e 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java @@ -376,6 +376,8 @@ public class DeploymentSpec { illegal("Non-prod environments cannot specify a region"); if (environment == Environment.prod && region.isEmpty()) illegal("Prod environments must be specified with a region"); + if (environment != Environment.prod && cloudAccount.isPresent()) + illegal("Non-prod environments cannot specify cloud account"); this.environment = Objects.requireNonNull(environment); this.region = Objects.requireNonNull(region); this.active = active; @@ -401,7 +403,7 @@ public class DeploymentSpec { } @Override - public List<DeclaredZone> zones() { return List.of(this); } + public List<DeclaredZone> zones() { return Collections.singletonList(this); } @Override public boolean concerns(Environment environment, Optional<RegionName> region) { @@ -490,7 +492,7 @@ public class DeploymentSpec { public List<DeclaredZone> zones() { return steps.stream() .flatMap(step -> step.zones().stream()) - .toList(); + .collect(Collectors.toUnmodifiableList()); } @Override diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java index 83fba75325e..09d61835ae3 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java @@ -57,8 +57,6 @@ public class DeploymentSpecXmlReader { private static final String instanceTag = "instance"; private static final String testTag = "test"; private static final String stagingTag = "staging"; - private static final String devTag = "dev"; - private static final String perfTag = "perf"; private static final String upgradeTag = "upgrade"; private static final String blockChangeTag = "block-change"; private static final String prodTag = "prod"; @@ -236,12 +234,11 @@ public class DeploymentSpecXmlReader { case testTag: if (Stream.iterate(stepTag, Objects::nonNull, Node::getParentNode) .anyMatch(node -> prodTag.equals(node.getNodeName()))) { - // A production test return List.of(new DeclaredTest(RegionName.from(XML.getValue(stepTag).trim()))); } - return List.of(new DeclaredZone(Environment.from(stepTag.getTagName()), Optional.empty(), false, athenzService, testerFlavor, readCloudAccount(stepTag))); - case devTag, perfTag, stagingTag: - return List.of(new DeclaredZone(Environment.from(stepTag.getTagName()), Optional.empty(), false, athenzService, testerFlavor, readCloudAccount(stepTag))); + return List.of(new DeclaredZone(Environment.from(stepTag.getTagName()), Optional.empty(), false, athenzService, testerFlavor, Optional.empty())); + case stagingTag: + return List.of(new DeclaredZone(Environment.from(stepTag.getTagName()), Optional.empty(), false, athenzService, testerFlavor, Optional.empty())); case prodTag: // regions, delay and parallel may be nested within, but we can flatten them return XML.getChildren(stepTag).stream() .flatMap(child -> readNonInstanceSteps(child, prodAttributes, stepTag).stream()) @@ -436,11 +433,7 @@ public class DeploymentSpecXmlReader { Optional<String> testerFlavor, Element regionTag) { return new DeclaredZone(environment, Optional.of(RegionName.from(XML.getValue(regionTag).trim())), readActive(regionTag), athenzService, testerFlavor, - readCloudAccount(regionTag)); - } - - private Optional<CloudAccount> readCloudAccount(Element tag) { - return mostSpecificAttribute(tag, cloudAccountAttribute).map(CloudAccount::new); + stringAttribute(cloudAccountAttribute, regionTag).map(CloudAccount::new)); } private Optional<String> readGlobalServiceId(Element environmentTag) { @@ -490,42 +483,42 @@ public class DeploymentSpecXmlReader { } private DeploymentSpec.UpgradePolicy readUpgradePolicy(String policy) { - return switch (policy) { - case "canary" -> UpgradePolicy.canary; - case "default" -> UpgradePolicy.defaultPolicy; - case "conservative" -> UpgradePolicy.conservative; - default -> throw new IllegalArgumentException("Illegal upgrade policy '" + policy + "': " + - "Must be one of 'canary', 'default', 'conservative'"); - }; + switch (policy) { + case "canary": return DeploymentSpec.UpgradePolicy.canary; + case "default": return DeploymentSpec.UpgradePolicy.defaultPolicy; + case "conservative": return DeploymentSpec.UpgradePolicy.conservative; + default: throw new IllegalArgumentException("Illegal upgrade policy '" + policy + "': " + + "Must be one of 'canary', 'default', 'conservative'"); + } } private DeploymentSpec.RevisionChange readRevisionChange(String revision) { - return switch (revision) { - case "when-clear" -> RevisionChange.whenClear; - case "when-failing" -> RevisionChange.whenFailing; - case "always" -> RevisionChange.always; - default -> throw new IllegalArgumentException("Illegal upgrade revision change policy '" + revision + "': " + - "Must be one of 'always', 'when-failing', 'when-clear'"); - }; + switch (revision) { + case "when-clear": return DeploymentSpec.RevisionChange.whenClear; + case "when-failing": return DeploymentSpec.RevisionChange.whenFailing; + case "always": return DeploymentSpec.RevisionChange.always; + default: throw new IllegalArgumentException("Illegal upgrade revision change policy '" + revision + "': " + + "Must be one of 'always', 'when-failing', 'when-clear'"); + } } private DeploymentSpec.RevisionTarget readRevisionTarget(String revision) { - return switch (revision) { - case "next" -> RevisionTarget.next; - case "latest" -> RevisionTarget.latest; - default -> throw new IllegalArgumentException("Illegal upgrade revision target '" + revision + "': " + - "Must be one of 'next', 'latest'"); - }; + switch (revision) { + case "next": return DeploymentSpec.RevisionTarget.next; + case "latest": return DeploymentSpec.RevisionTarget.latest; + default: throw new IllegalArgumentException("Illegal upgrade revision target '" + revision + "': " + + "Must be one of 'next', 'latest'"); + } } private DeploymentSpec.UpgradeRollout readUpgradeRollout(String rollout) { - return switch (rollout) { - case "separate" -> UpgradeRollout.separate; - case "leading" -> UpgradeRollout.leading; - case "simultaneous" -> UpgradeRollout.simultaneous; - default -> throw new IllegalArgumentException("Illegal upgrade rollout '" + rollout + "': " + - "Must be one of 'separate', 'leading', 'simultaneous'"); - }; + switch (rollout) { + case "separate": return DeploymentSpec.UpgradeRollout.separate; + case "leading": return DeploymentSpec.UpgradeRollout.leading; + case "simultaneous": return DeploymentSpec.UpgradeRollout.simultaneous; + default: throw new IllegalArgumentException("Illegal upgrade rollout '" + rollout + "': " + + "Must be one of 'separate', 'leading', 'simultaneous'"); + } } private boolean readActive(Element regionTag) { diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java index 6373e0e0232..7f0e9b4cae8 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java @@ -1514,21 +1514,12 @@ public class DeploymentSpecTest { public void cloudAccount() { StringReader r = new StringReader( "<deployment version='1.0' cloud-account='100000000000'>" + - " <instance id='alpha'>" + - " <prod cloud-account='800000000000'>" + - " <region>us-east-1</region>" + - " </prod>" + - " </instance>" + " <instance id='beta' cloud-account='200000000000'>" + - " <staging cloud-account='600000000000'/>" + - " <perf cloud-account='700000000000'/>" + " <prod>" + " <region>us-west-1</region>" + " </prod>" + " </instance>" + " <instance id='main'>" + - " <test cloud-account='500000000000'/>" + - " <dev cloud-account='400000000000'/>" + " <prod>" + " <region cloud-account='300000000000'>us-east-1</region>" + " <region>eu-west-1</region>" + @@ -1537,20 +1528,9 @@ public class DeploymentSpecTest { "</deployment>" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertCloudAccount("800000000000", spec.requireInstance("alpha"), Environment.prod, "us-east-1"); - assertCloudAccount("200000000000", spec.requireInstance("beta"), Environment.prod, "us-west-1"); - assertCloudAccount("600000000000", spec.requireInstance("beta"), Environment.staging, ""); - assertCloudAccount("700000000000", spec.requireInstance("beta"), Environment.perf, ""); - assertCloudAccount("200000000000", spec.requireInstance("beta"), Environment.dev, ""); - assertCloudAccount("300000000000", spec.requireInstance("main"), Environment.prod, "us-east-1"); - assertCloudAccount("100000000000", spec.requireInstance("main"), Environment.prod, "eu-west-1"); - assertCloudAccount("400000000000", spec.requireInstance("main"), Environment.dev, ""); - assertCloudAccount("500000000000", spec.requireInstance("main"), Environment.test, ""); - assertCloudAccount("100000000000", spec.requireInstance("main"), Environment.staging, ""); - } - - private void assertCloudAccount(String expected, DeploymentInstanceSpec instance, Environment environment, String region) { - assertEquals(Optional.of(expected).map(CloudAccount::new), instance.cloudAccount(environment, Optional.of(region).filter(s -> !s.isEmpty()).map(RegionName::from))); + assertEquals(Optional.of(new CloudAccount("200000000000")), spec.requireInstance("beta").cloudAccount(Environment.prod, RegionName.from("us-west-1"))); + assertEquals(Optional.of(new CloudAccount("300000000000")), spec.requireInstance("main").cloudAccount(Environment.prod, RegionName.from("us-east-1"))); + assertEquals(Optional.of(new CloudAccount("100000000000")), spec.requireInstance("main").cloudAccount(Environment.prod, RegionName.from("eu-west-1"))); } private static void assertInvalid(String deploymentSpec, String errorMessagePart) { diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java index 4acf0c692fb..1232f700fee 100644 --- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java +++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java @@ -715,9 +715,9 @@ public class DeploymentSpecWithoutInstanceTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); DeploymentInstanceSpec instance = spec.requireInstance("default"); - assertEquals(Optional.of(new CloudAccount("219876543210")), instance.cloudAccount(Environment.prod, Optional.of(RegionName.from("us-east-1")))); - assertEquals(Optional.of(new CloudAccount("012345678912")), instance.cloudAccount(Environment.prod, Optional.of(RegionName.from("us-west-1")))); - assertEquals(Optional.of(new CloudAccount("012345678912")), instance.cloudAccount(Environment.staging, Optional.empty())); + assertEquals(Optional.of(new CloudAccount("219876543210")), instance.cloudAccount(Environment.prod, RegionName.from("us-east-1"))); + assertEquals(Optional.of(new CloudAccount("012345678912")), instance.cloudAccount(Environment.prod, RegionName.from("us-west-1"))); + assertEquals(Optional.empty(), instance.cloudAccount(Environment.staging, RegionName.defaultName())); r = new StringReader( "<deployment version='1.0'>" + @@ -728,8 +728,8 @@ public class DeploymentSpecWithoutInstanceTest { "</deployment>" ); spec = DeploymentSpec.fromXml(r); - assertEquals(Optional.of(new CloudAccount("219876543210")), spec.requireInstance("default").cloudAccount(Environment.prod, Optional.of(RegionName.from("us-east-1")))); - assertEquals(Optional.empty(), spec.requireInstance("default").cloudAccount(Environment.prod, Optional.of(RegionName.from("us-west-1")))); + assertEquals(Optional.of(new CloudAccount("219876543210")), spec.requireInstance("default").cloudAccount(Environment.prod, RegionName.from("us-east-1"))); + assertEquals(Optional.empty(), spec.requireInstance("default").cloudAccount(Environment.prod, RegionName.from("us-west-1"))); } private static Set<String> endpointRegions(String endpointId, DeploymentSpec spec) { diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc index 9723e531bd2..251134c91e8 100644 --- a/config-model/src/main/resources/schema/deployment.rnc +++ b/config-model/src/main/resources/schema/deployment.rnc @@ -24,8 +24,6 @@ StepExceptInstance = Endpoints? & Test? & Staging? & - Dev? & - Perf? & Prod* PrimitiveStep = @@ -103,19 +101,10 @@ Staging = element staging { text } -Dev = element dev { - attribute cloud-account { xsd:string }? -} - -Perf = element perf { - attribute cloud-account { xsd:string }? -} - Prod = element prod { attribute global-service-id { text }? & attribute athenz-service { xsd:string }? & attribute tester-flavor { xsd:string }? & - attribute cloud-account { xsd:string }? & Region* & Delay* & ProdTest* & 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); }); 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 0c84016ad06..96ca239b527 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,7 +67,6 @@ 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; @@ -93,7 +92,6 @@ 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(); @@ -1273,38 +1271,33 @@ public class ControllerTest { @Test void testCloudAccount() { DeploymentContext context = tester.newDeploymentContext(); - ZoneId devZone = devUsEast1.zone(); - ZoneId prodZone = productionUsWest1.zone(); + ZoneId zone = ZoneId.from("prod", "us-west-1"); String cloudAccount = "012345678912"; var applicationPackage = new ApplicationPackageBuilder() .cloudAccount(cloudAccount) - .region(prodZone.region()) + .region(zone.region()) .build(); - // 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 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()); + } - // Deployment fails because zone is not configured in requested cloud account + // Deployment fails because requested region is not configured in cloud account tester.controllerTester().flagSource().withListFlag(PermanentFlags.CLOUD_ACCOUNTS.id(), List.of(cloudAccount), String.class); - 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(); - - // 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()); + 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.submit(applicationPackage).deploy(); + assertEquals(cloudAccount, tester.controllerTester().configServer().cloudAccount(context.deploymentIdIn(zone)).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 8ddd0ef2be3..146135491a3 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,7 +5,6 @@ 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; @@ -27,7 +26,6 @@ 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; @@ -57,7 +55,6 @@ 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"; @@ -68,6 +65,8 @@ 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; @@ -136,22 +135,12 @@ public class ApplicationPackageBuilder { } public ApplicationPackageBuilder systemTest() { - return explicitEnvironment(Environment.test); - } - - public ApplicationPackageBuilder stagingTest() { - 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())); + explicitSystemTest = true; 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); + public ApplicationPackageBuilder stagingTest() { + explicitStagingTest = true; return this; } @@ -278,10 +267,6 @@ 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' "); @@ -306,13 +291,10 @@ public class ApplicationPackageBuilder { xml.append("/>\n"); } xml.append(notifications); - nonProductionEnvironments.forEach((environment, attributes) -> { - xml.append(" <").append(environment.value()); - attributes.forEach((attribute, value) -> { - xml.append(" ").append(attribute).append("='").append(value).append("'"); - }); - xml.append(" />\n"); - }); + if (explicitSystemTest) + xml.append(" <test />\n"); + if (explicitStagingTest) + xml.append(" <staging />\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 8dd110e7df0..934c6b07c29 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 RuntimeException("Exception from test code")); + return failDeployment(type, new IllegalArgumentException("Exception from test code")); } /** Fail current deployment in given job */ private DeploymentContext failDeployment(JobType type, RuntimeException exception) { configServer().throwOnNextPrepare(exception); - runJobExpectingFailure(type, null); + runJobExpectingFailure(type, Optional.empty()); return this; } /** Run given job and expect it to fail with given message, if any */ - public DeploymentContext runJobExpectingFailure(JobType type, String messagePart) { + public DeploymentContext runJobExpectingFailure(JobType type, Optional<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 != null) { + if (messagePart.isPresent()) { 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)), - "Found log message containing '" + messagePart + "'"); + .anyMatch(entry -> entry.message().contains(messagePart.get())), + "Found log message containing '" + messagePart.get() + "'"); } 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 0211a052f76..680e69998b6 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 configureCloudAccount(CloudAccount cloudAccount, ZoneId... zones) { - this.cloudAccountZones.computeIfAbsent(cloudAccount, (k) -> new HashSet<>()).addAll(Set.of(zones)); + public ZoneRegistryMock setCloudAccountZones(CloudAccount cloudAccount, ZoneId... zones) { + this.cloudAccountZones.put(cloudAccount, Set.of(zones)); return this; } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiCloudTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiCloudTest.java index cfe25232408..674424fbdd9 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiCloudTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiCloudTest.java @@ -413,7 +413,7 @@ public class ApplicationApiCloudTest extends ControllerContainerCloudTest { @Test void create_application_on_deploy() { var application = ApplicationName.from("unique"); - var applicationPackage = new ApplicationPackageBuilder().trustDefaultCertificate().withoutAthenzIdentity().build(); + var applicationPackage = new ApplicationPackageBuilder().withoutAthenzIdentity().build(); new ControllerTester(tester).upgradeSystem(new Version("6.1")); assertTrue(tester.controller().applications().getApplication(TenantAndApplicationId.from(tenantName, application)).isEmpty()); @@ -473,7 +473,6 @@ public class ApplicationApiCloudTest extends ControllerContainerCloudTest { private void deployApplication() { var applicationPackage = new ApplicationPackageBuilder() - .trustDefaultCertificate() .instances("default") .globalServiceId("foo") .region("aws-us-east-1c") 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 cd8a9e72051..7477b94e3f4 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,6 +17,7 @@ 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; @@ -113,7 +114,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, "out of rotations"); + .runJobExpectingFailure(DeploymentContext.systemTest, Optional.of("out of rotations")); } @Test @@ -122,7 +123,7 @@ public class RotationRepositoryTest { .globalServiceId("foo") .region("us-east-3") .build(); - application.submit(applicationPackage).runJobExpectingFailure(DeploymentContext.systemTest, "less than 2 prod zones are defined"); + application.submit(applicationPackage).runJobExpectingFailure(DeploymentContext.systemTest, Optional.of("less than 2 prod zones are defined")); } @Test |