From 1e628dccb9a9009802fa3af5f202fd4f827ae3e9 Mon Sep 17 00:00:00 2001 From: Bjørn Christian Seime Date: Wed, 15 Jan 2020 16:24:01 +0100 Subject: Revert "Athenz domain cannot be set on instance level, just on the root" --- config-model-api/abi-spec.json | 4 +-- .../application/api/DeploymentInstanceSpec.java | 35 +++++++++++++++++++--- .../config/application/api/DeploymentSpec.java | 30 ------------------- .../api/xml/DeploymentSpecXmlReader.java | 25 ++++++++-------- .../config/application/api/DeploymentSpecTest.java | 19 +++++++----- .../api/DeploymentSpecWithoutInstanceTest.java | 5 ++-- .../model/container/xml/ContainerModelBuilder.java | 4 ++- .../src/main/resources/schema/deployment.rnc | 1 + .../hosted/controller/ApplicationController.java | 33 +++++++++++++++----- .../controller/deployment/InternalStepRunner.java | 2 +- 10 files changed, 91 insertions(+), 67 deletions(-) diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json index 25f35781a1f..d8482499a93 100644 --- a/config-model-api/abi-spec.json +++ b/config-model-api/abi-spec.json @@ -194,7 +194,7 @@ "public" ], "methods": [ - "public void (com.yahoo.config.provision.InstanceName, java.util.List, com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy, java.util.List, java.util.Optional, java.util.Optional, com.yahoo.config.application.api.Notifications, java.util.List)", + "public void (com.yahoo.config.provision.InstanceName, java.util.List, com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy, java.util.List, java.util.Optional, java.util.Optional, java.util.Optional, com.yahoo.config.application.api.Notifications, java.util.List)", "public com.yahoo.config.provision.InstanceName name()", "public com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy upgradePolicy()", "public java.util.List changeBlocker()", @@ -605,4 +605,4 @@ "public static final com.yahoo.config.application.api.ValidationOverrides all" ] } -} +} \ No newline at end of file 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 9c6013a127d..8e7c255f27a 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 @@ -29,6 +29,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { private final DeploymentSpec.UpgradePolicy upgradePolicy; private final List changeBlockers; private final Optional globalServiceId; + private final Optional athenzDomain; private final Optional athenzService; private final Notifications notifications; private final List endpoints; @@ -38,6 +39,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { DeploymentSpec.UpgradePolicy upgradePolicy, List changeBlockers, Optional globalServiceId, + Optional athenzDomain, Optional athenzService, Notifications notifications, List endpoints) { @@ -46,11 +48,13 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { this.upgradePolicy = upgradePolicy; this.changeBlockers = changeBlockers; this.globalServiceId = globalServiceId; + this.athenzDomain = athenzDomain; this.athenzService = athenzService; this.notifications = notifications; this.endpoints = List.copyOf(validateEndpoints(endpoints, steps())); validateZones(new HashSet<>(), new HashSet<>(), this); validateEndpoints(steps(), globalServiceId, this.endpoints); + validateAthenz(); } public InstanceName name() { return name; } @@ -133,6 +137,29 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { } } + /** + * Throw an IllegalArgumentException if Athenz configuration violates: + * domain not configured -> no zone can configure service + * domain configured -> all zones must configure service + */ + private void validateAthenz() { + // If athenz domain is not set, athenz service cannot be set on any level + if (athenzDomain.isEmpty()) { + for (DeploymentSpec.DeclaredZone zone : zones()) { + if (zone.athenzService().isPresent()) { + throw new IllegalArgumentException("Athenz service configured for zone: " + zone + ", but Athenz domain is not configured"); + } + } + // if athenz domain is not set, athenz service must be set implicitly or directly on all zones. + } else if (athenzService.isEmpty()) { + for (DeploymentSpec.DeclaredZone zone : zones()) { + if (zone.athenzService().isEmpty()) { + throw new IllegalArgumentException("Athenz domain is configured, but Athenz service not configured for zone: " + zone); + } + } + } + } + /** Returns the upgrade policy of this, which is defaultPolicy if none is specified */ public DeploymentSpec.UpgradePolicy upgradePolicy() { return upgradePolicy; } @@ -155,10 +182,9 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { } /** Returns the athenz domain if configured */ - // TODO jonmv: Remove when 7.162 is older than the oldest deployed version. - public Optional athenzDomain() { return Optional.empty(); } + public Optional athenzDomain() { return athenzDomain; } - /** Returns the athenz service for environment/region if configured, defaulting to that of the instance */ + /** Returns the athenz service for environment/region if configured */ public Optional athenzService(Environment environment, RegionName region) { return zones().stream() .filter(zone -> zone.concerns(environment, Optional.of(region))) @@ -187,6 +213,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { upgradePolicy == other.upgradePolicy && changeBlockers.equals(other.changeBlockers) && steps().equals(other.steps()) && + athenzDomain.equals(other.athenzDomain) && athenzService.equals(other.athenzService) && notifications.equals(other.notifications) && endpoints.equals(other.endpoints); @@ -194,7 +221,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { @Override public int hashCode() { - return Objects.hash(globalServiceId, upgradePolicy, changeBlockers, steps(), athenzService, notifications, endpoints); + return Objects.hash(globalServiceId, upgradePolicy, changeBlockers, steps(), athenzDomain, athenzService, notifications, endpoints); } @Override 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 a19a83f1207..f778c2c2d0e 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 @@ -62,7 +62,6 @@ public class DeploymentSpec { this.xmlForm = xmlForm; validateTotalDelay(steps); validateUpgradePoliciesOfIncreasingConservativeness(steps); - validateAthenz(); } /** Throw an IllegalArgumentException if the total delay exceeds 24 hours */ @@ -90,35 +89,6 @@ public class DeploymentSpec { } } - /** - * Throw an IllegalArgumentException if Athenz configuration violates: - * domain not configured -> no zone can configure service - * domain configured -> all zones must configure service - */ - private void validateAthenz() { - // If athenz domain is not set, athenz service cannot be set on any level - if (athenzDomain.isEmpty()) { - for (DeploymentInstanceSpec instance : instances()) { - for (DeploymentSpec.DeclaredZone zone : instance.zones()) { - if (zone.athenzService().isPresent()) { - throw new IllegalArgumentException("Athenz service configured for zone: " + zone + ", but Athenz domain is not configured"); - } - } - } - // if athenz domain is not set, athenz service must be set implicitly or directly on all zones. - } - else if (athenzService.isEmpty()) { - for (DeploymentInstanceSpec instance : instances()) { - for (DeploymentSpec.DeclaredZone zone : instance.zones()) { - if (zone.athenzService().isEmpty()) { - throw new IllegalArgumentException("Athenz domain is configured, but Athenz service not configured for zone: " + zone); - } - } - } - } - } - - /** Returns the major version this application is pinned to, or empty (default) to allow all major versions */ public Optional majorVersion() { return majorVersion; } 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 68f27a21ce4..faa7c8cf932 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 @@ -139,7 +139,12 @@ public class DeploymentSpecXmlReader { // Values where the parent may provide a default DeploymentSpec.UpgradePolicy upgradePolicy = readUpgradePolicy(instanceTag, parentTag); List changeBlockers = readChangeBlockers(instanceTag, parentTag); - Optional athenzService = mostSpecificAttribute(instanceTag, athenzServiceAttribute).map(AthenzService::from); + Optional athenzDomain = stringAttribute(athenzDomainAttribute, instanceTag) + .or(() -> stringAttribute(athenzDomainAttribute, parentTag)) + .map(AthenzDomain::from); + Optional athenzService = stringAttribute(athenzServiceAttribute, instanceTag) + .or(() -> stringAttribute(athenzServiceAttribute, parentTag)) + .map(AthenzService::from); Notifications notifications = readNotifications(instanceTag, parentTag); // Values where there is no default @@ -156,6 +161,7 @@ public class DeploymentSpecXmlReader { upgradePolicy, changeBlockers, globalServiceId.asOptional(), + athenzDomain, athenzService, notifications, endpoints)) @@ -173,8 +179,11 @@ public class DeploymentSpecXmlReader { // Consume the given tag as 0-N steps. 0 if it is not a step, >1 if it contains multiple nested steps that should be flattened @SuppressWarnings("fallthrough") private List readNonInstanceSteps(Element stepTag, MutableOptional globalServiceId, Element parentTag) { - Optional athenzService = mostSpecificAttribute(stepTag, athenzServiceAttribute).map(AthenzService::from); - Optional testerFlavor = mostSpecificAttribute(stepTag, testerFlavorAttribute); + Optional athenzService = stringAttribute(athenzServiceAttribute, stepTag) + .or(() -> stringAttribute(athenzServiceAttribute, parentTag)) + .map(AthenzService::from); + Optional testerFlavor = stringAttribute(testerFlavorAttribute, stepTag) + .or(() -> stringAttribute(testerFlavorAttribute, parentTag)); if (prodTag.equals(stepTag.getTagName())) globalServiceId.set(readGlobalServiceId(stepTag)); @@ -340,7 +349,7 @@ public class DeploymentSpecXmlReader { /** * Returns the given attribute as a string, or Optional.empty if it is not present or empty */ - private static Optional stringAttribute(String attributeName, Element tag) { + private Optional stringAttribute(String attributeName, Element tag) { String value = tag.getAttribute(attributeName); return Optional.ofNullable(value).filter(s -> !s.equals("")); } @@ -416,14 +425,6 @@ public class DeploymentSpecXmlReader { || root.getAttributes().getLength() == 1 && root.hasAttribute("version"); } - /** Returns the given attribute from the given tag or its closest ancestor with the attribute. */ - private static Optional mostSpecificAttribute(Element tag, String attributeName) { - return Stream.iterate(tag, Objects::nonNull, Node::getParentNode) - .filter(Element.class::isInstance) - .map(Element.class::cast) - .flatMap(element -> stringAttribute(attributeName, element).stream()) - .findFirst(); - } private static class MutableOptional { 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 5bf103d1836..d0740b3e9b9 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 @@ -805,6 +805,7 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); assertEquals("domain", spec.athenzDomain().get().value()); + assertEquals("domain", spec.requireInstance("instance1").athenzDomain().get().value()); assertEquals("service", spec.athenzService().get().value()); assertEquals("service", spec.requireInstance("instance1").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value()); @@ -829,6 +830,7 @@ public class DeploymentSpecTest { assertEquals("domain", spec.athenzDomain().get().value()); assertEquals("service", spec.athenzService().get().value()); + assertEquals("domain", spec.requireInstance("instance1").athenzDomain().get().value()); assertEquals("prod-service", spec.requireInstance("instance1").athenzService(Environment.prod, RegionName.from("us-central-1")).get().value()); assertEquals("prod-service", spec.requireInstance("instance1").athenzService(Environment.prod, @@ -863,6 +865,7 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); assertEquals("domain", spec.athenzDomain().get().value()); + assertEquals("domain", spec.requireInstance("instance1").athenzDomain().get().value()); assertEquals("service", spec.requireInstance("instance1").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value()); assertEquals("service", spec.requireInstance("instance1").athenzService(Environment.prod, @@ -874,8 +877,8 @@ public class DeploymentSpecTest { @Test public void athenz_config_is_read_from_instance() { StringReader r = new StringReader( - "" + - " " + + "" + + " " + " " + " us-west-1" + " " + @@ -883,16 +886,17 @@ public class DeploymentSpecTest { "" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals("domain", spec.athenzDomain().get().value()); + assertEquals(Optional.empty(), spec.athenzDomain()); assertEquals(Optional.empty(), spec.athenzService()); - assertEquals("service", spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value()); + assertEquals(spec.requireInstance("default").athenzDomain().get().value(), "domain"); + assertEquals(spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value(), "service"); } @Test public void athenz_service_is_overridden_from_environment() { StringReader r = new StringReader( "" + - " " + + " " + " " + " " + " us-west-1" + @@ -901,6 +905,7 @@ public class DeploymentSpecTest { "" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); + assertEquals(spec.requireInstance("default").athenzDomain().get().value(), "domain"); assertEquals("prod-service", spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value()); @@ -909,8 +914,8 @@ public class DeploymentSpecTest { @Test(expected = IllegalArgumentException.class) public void it_fails_when_athenz_service_is_not_defined() { StringReader r = new StringReader( - "" + - " " + + "" + + " " + " " + " us-west-1" + " " + 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 6a815a467f5..b5e5946262a 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 @@ -506,7 +506,7 @@ public class DeploymentSpecWithoutInstanceTest { "" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(spec.athenzDomain().get().value(), "domain"); + assertEquals(spec.requireInstance("default").athenzDomain().get().value(), "domain"); assertEquals(spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value(), "service"); } @@ -525,6 +525,7 @@ public class DeploymentSpecWithoutInstanceTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); assertEquals("domain", spec.athenzDomain().get().value()); + assertEquals("domain", spec.requireInstance("default").athenzDomain().get().value()); assertEquals("service", spec.athenzService().get().value()); assertEquals("prod-service", spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-central-1")) .get().value()); @@ -546,7 +547,7 @@ public class DeploymentSpecWithoutInstanceTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); assertEquals("service", spec.athenzService().get().value()); - assertEquals(spec.athenzDomain().get().value(), "domain"); + assertEquals(spec.requireInstance("default").athenzDomain().get().value(), "domain"); assertEquals(spec.requireInstance("default").athenzService(Environment.prod, RegionName.from("us-west-1")).get().value(), "prod-service"); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java index 7dba9d7cfff..0f808988a15 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java @@ -860,7 +860,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder { String athenzDnsSuffix, Zone zone, DeploymentSpec spec) { - spec.athenzDomain() + spec.instance(app.getApplicationId().instance()) + .flatMap(instanceSpec -> instanceSpec.athenzDomain()) + .or(() -> spec.athenzDomain()) .ifPresent(domain -> { AthenzService service = spec.instance(app.getApplicationId().instance()) .flatMap(instanceSpec -> instanceSpec.athenzService(zone.environment(), zone.region())) diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc index 171112f6bd7..6a8bc8f77b9 100644 --- a/config-model/src/main/resources/schema/deployment.rnc +++ b/config-model/src/main/resources/schema/deployment.rnc @@ -32,6 +32,7 @@ PrimitiveStep = Instance = element instance { attribute id { xsd:string } & + attribute athenz-domain { xsd:string }? & attribute athenz-service { xsd:string }? & StepExceptInstance } 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 a5821812ce9..e4197dab2cf 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 @@ -917,8 +917,7 @@ public class ApplicationController { * @param deployer principal initiating the deployment, possibly empty */ public void verifyApplicationIdentityConfiguration(TenantName tenantName, Optional instanceName, Optional zoneId, ApplicationPackage applicationPackage, Optional deployer) { - Optional identityDomain = applicationPackage.deploymentSpec().athenzDomain() - .map(domain -> new AthenzDomain(domain.value())); + Optional identityDomain = getIdentityDomain(applicationPackage); if(identityDomain.isEmpty()) { // If there is no domain configured in deployment.xml there is nothing to do. return; @@ -941,8 +940,8 @@ public class ApplicationController { var zone = zoneId.orElseThrow(() -> new IllegalArgumentException("Unable to evaluate access, no zone provided in deployment")); var serviceToLaunch = instanceName .flatMap(instance -> applicationPackage.deploymentSpec().instance(instance)) - .flatMap(instanceSpec -> instanceSpec.athenzService(zone.environment(), zone.region())) - .or(() -> applicationPackage.deploymentSpec().athenzService()) + .map(instanceSpec -> instanceSpec.athenzService(zone.environment(), zone.region())) + .orElse(applicationPackage.deploymentSpec().athenzService()) .map(service -> new AthenzService(identityDomain.get(), service.value())); if(serviceToLaunch.isPresent()) { @@ -981,17 +980,35 @@ public class ApplicationController { .map(AthenzUser.class::cast); } + private Optional getIdentityDomain(ApplicationPackage applicationPackage) { + List domains = Stream.concat(applicationPackage.deploymentSpec().athenzDomain().stream(), + applicationPackage.deploymentSpec().instances().stream() + .flatMap(spec -> spec.athenzDomain().stream())) + .distinct() + .map(domain -> new AthenzDomain(domain.value())) + .collect(toList()); + + + // We cannot have more than one domain, does not make sense that the domains are different ... + if(domains.size() > 1) { + throw new IllegalArgumentException(String.format("Multiple athenz-domain configured in deployment.xml (%s), can only have one.", domains.toString())); + } + return domains.stream().findAny(); + } + /* * Verifies that the configured athenz service (if any) can be launched. */ private void verifyAllowedLaunchAthenzService(DeploymentSpec deploymentSpec) { - deploymentSpec.athenzDomain().ifPresent(domain -> { - controller.zoneRegistry().zones().reachable().ids().forEach(zone -> { - AthenzIdentity configServerAthenzIdentity = controller.zoneRegistry().getConfigServerHttpsIdentity(zone); + controller.zoneRegistry().zones().reachable().ids().forEach(zone -> { + AthenzIdentity configServerAthenzIdentity = controller.zoneRegistry().getConfigServerHttpsIdentity(zone); + deploymentSpec.athenzDomain().ifPresent(domain -> { deploymentSpec.athenzService().ifPresent(service -> { verifyAthenzServiceCanBeLaunchedBy(configServerAthenzIdentity, new AthenzService(domain.value(), service.value())); }); - deploymentSpec.instances().forEach(spec -> { + }); + deploymentSpec.instances().forEach(spec -> { + spec.athenzDomain().ifPresent(domain -> { spec.athenzService(zone.environment(), zone.region()).ifPresent(service -> { verifyAthenzServiceCanBeLaunchedBy(configServerAthenzIdentity, new AthenzService(domain.value(), service.value())); }); 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 10025f86540..20c64751335 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 @@ -665,7 +665,7 @@ public class InternalStepRunner implements StepRunner { DEFAULT_TESTER_RESOURCES_AWS : DEFAULT_TESTER_RESOURCES)); byte[] testPackage = controller.applications().applicationStore().getTester(id.application().tenant(), id.application().application(), version); byte[] deploymentXml = deploymentXml(id.tester(), - spec.athenzDomain(), + spec.requireInstance(id.application().instance()).athenzDomain(), spec.requireInstance(id.application().instance()).athenzService(zone.environment(), zone.region())); try (ZipBuilder zipBuilder = new ZipBuilder(testPackage.length + servicesXml.length + 1000)) { -- cgit v1.2.3