diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-09-08 15:58:16 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2022-09-12 09:30:08 +0200 |
commit | 8a2d1b2bfdea077b8f3018677e406b1452c6d548 (patch) | |
tree | 530c30fba718aaddd9c8c5b27cc6809d75c642d8 /controller-server | |
parent | 6dfaaa60db9b60f5f48aa30e77da0b1cc08ab8f9 (diff) |
Validate cloud account at deployment time
Diffstat (limited to 'controller-server')
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 |