diff options
author | jonmv <venstad@gmail.com> | 2023-01-17 16:30:36 +0100 |
---|---|---|
committer | jonmv <venstad@gmail.com> | 2023-01-17 16:30:36 +0100 |
commit | f76a2f86a3dc8417eef6bdfa0a24f6ba24364e13 (patch) | |
tree | 66247e153fa2450426695551ae829dd9df5c59fe /config-model-api | |
parent | 8d59490a9ac398b75719a8c4736e2c27729b2671 (diff) |
Validate disabled regions are not targets
Diffstat (limited to 'config-model-api')
3 files changed, 104 insertions, 13 deletions
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 7d3014c15a3..d731e09d4e4 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 @@ -150,6 +150,10 @@ public class DeploymentSpec { illegal(prefix + "targets undeclared region '" + target.region() + "' in instance '" + target.instance() + "'"); } + if (instance.get().zoneEndpoint(ZoneId.from(Environment.prod, target.region()), ClusterSpec.Id.from(endpoint.containerId())) + .map(zoneEndpoint -> ! zoneEndpoint.isPublicEndpoint()).orElse(false)) + illegal(prefix + "targets '" + target.region().value() + "' in '" + target.instance().value() + + "', but its zone endpoint has 'enabled' set to 'false'"); } } } 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 81c081fd17d..fb6d834f783 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 @@ -46,6 +46,7 @@ import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.LinkedHashSet; @@ -57,6 +58,8 @@ import java.util.Set; import java.util.function.Function; import java.util.stream.Stream; +import static java.util.Comparator.comparingInt; + /** * @author bratseth */ @@ -149,7 +152,7 @@ public class DeploymentSpecXmlReader { steps.addAll(readNonInstanceSteps(child, new HashMap<>(), root)); // (No global service id here) } } - readEndpoints(root, Optional.empty(), steps, applicationEndpoints, Map.of()); + readEndpoints(root, Optional.empty(), steps, applicationEndpoints, Map.of()); } return new DeploymentSpec(steps, @@ -325,7 +328,9 @@ public class DeploymentSpecXmlReader { Endpoint.Level level = instance.isEmpty() ? Endpoint.Level.application : Endpoint.Level.instance; Map<String, Endpoint> endpointsById = new LinkedHashMap<>(); Map<String, Map<RegionName, List<ZoneEndpoint>>> endpointsByZone = new LinkedHashMap<>(); - for (var endpointElement : XML.getChildren(endpointsElement, endpointTag)) { + XML.getChildren(endpointsElement, endpointTag).stream() // Read zone settings first. + .sorted(comparingInt(endpoint -> getZoneEndpointType(endpoint, level).isPresent() ? 0 : 1)) + .forEach(endpointElement -> { String containerId = requireStringAttribute("container-id", endpointElement); Optional<String> endpointId = stringAttribute("id", endpointElement); Optional<String> zoneEndpointType = getZoneEndpointType(endpointElement, level); @@ -392,8 +397,16 @@ public class DeploymentSpecXmlReader { Set<RegionName> regions = new LinkedHashSet<>(); for (var regionElement : XML.getChildren(endpointElement, "region")) { String region = regionElement.getTextContent(); - if (region == null || region.isBlank()) illegal(msgPrefix + "empty 'region' element"); - if ( ! regions.add(RegionName.from(region))) illegal(msgPrefix + "duplicate 'region' element: '" + region + "'"); + if (region == null || region.isBlank()) + illegal(msgPrefix + "empty 'region' element"); + if ( zoneEndpointType.isEmpty() + && Stream.of(RegionName.from(region), null) + .map(endpointsByZone.getOrDefault(containerId, new HashMap<>())::get) + .flatMap(maybeEndpoints -> maybeEndpoints == null ? Stream.empty() : maybeEndpoints.stream()) + .anyMatch(endpoint -> ! endpoint.isPublicEndpoint())) + illegal(msgPrefix + "targets zone endpoint in '" + region + "' with 'enabled' set to 'false'"); + if ( ! regions.add(RegionName.from(region))) + illegal(msgPrefix + "duplicate 'region' element: '" + region + "'"); } if (zoneEndpointType.isPresent()) { @@ -409,13 +422,22 @@ public class DeploymentSpecXmlReader { } else { if (regions.isEmpty()) { - // No explicit targets given for instance level endpoint. Include all declared regions by default - steps.stream() - .filter(step -> step.concerns(Environment.prod)) - .flatMap(step -> step.zones().stream()) - .flatMap(zone -> zone.region().stream()) - .forEach(regions::add); + // No explicit targets given for instance level endpoint. Include all declared, enabled regions by default + List<RegionName> declared = + steps.stream() + .filter(step -> step.concerns(Environment.prod)) + .flatMap(step -> step.zones().stream()) + .flatMap(zone -> zone.region().stream()) + .toList(); + if (declared.isEmpty()) illegal(msgPrefix + "no declared regions to target"); + + declared.stream().filter(region -> Stream.of(region, null) + .map(endpointsByZone.getOrDefault(containerId, new HashMap<>())::get) + .flatMap(maybeEndpoints -> maybeEndpoints == null ? Stream.empty() : maybeEndpoints.stream()) + .allMatch(ZoneEndpoint::isPublicEndpoint)) + .forEach(regions::add); } + if (regions.isEmpty()) illegal(msgPrefix + "all eligible zone endpoints have 'enabled' set to 'false'"); InstanceName instanceName = instance.map(InstanceName::from).get(); for (RegionName region : regions) targets.add(new Target(region, instanceName, 1)); } @@ -428,7 +450,7 @@ public class DeploymentSpecXmlReader { } endpointsById.put(endpoint.endpointId(), endpoint); } - } + }); endpoints.addAll(endpointsById.values()); validateAndConsolidate(endpointsByZone, zoneEndpoints); } 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 918239a646a..355ce651c34 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 @@ -1348,7 +1348,7 @@ public class DeploymentSpecTest { <endpoint id="foo" container-id="bar"> <region>us-east</region> </endpoint> - <endpoint container-id="bar" type='zone' enabled='false'> + <endpoint container-id="bar" type='private'> <region>us-east</region> </endpoint> <endpoint id="nalle" container-id="frosk" /> @@ -1361,7 +1361,7 @@ public class DeploymentSpecTest { assertEquals(Set.of("us-east"), endpointRegions("foo", spec)); assertEquals(Set.of("us-east", "us-west"), endpointRegions("nalle", spec)); assertEquals(Set.of("us-east", "us-west"), endpointRegions("default", spec)); - assertEquals(new ZoneEndpoint(false, false, List.of()), + assertEquals(new ZoneEndpoint(true, true, List.of()), spec.zoneEndpoint(InstanceName.from("default"), from("prod", "us-east"), ClusterSpec.Id.from("bar"))); assertEquals(new ZoneEndpoint(true, false, List.of()), spec.zoneEndpoint(InstanceName.from("default"), from("prod", "us-west"), ClusterSpec.Id.from("bar"))); @@ -1435,6 +1435,71 @@ public class DeploymentSpecTest { } @Test + public void cannotTargetDisabledEndpoints() { + assertEquals("Instance-level endpoint 'default': all eligible zone endpoints have 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + <deployment> + <instance id="default"> + <prod> + <region>us</region> + <region>eu</region> + </prod> + <endpoints> + <endpoint container-id='id' /> + <endpoint type='zone' container-id='id' enabled='false' /> + </endpoints> + </instance> + </deployment> + """)) + .getMessage()); + + assertEquals("Instance-level endpoint 'default': targets zone endpoint in 'us' with 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + <deployment> + <instance id="default"> + <prod> + <region>us</region> + <region>eu</region> + </prod> + <endpoints> + <endpoint container-id='id'> + <region>us</region> + </endpoint> + <endpoint type='zone' container-id='id' enabled='false' /> + </endpoints> + </instance> + </deployment> + """)) + .getMessage()); + + assertEquals("Application-level endpoint 'default': targets 'us' in 'default', but its zone endpoint has 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + <deployment> + <instance id="default"> + <prod> + <region>us</region> + <region>eu</region> + </prod> + <endpoints> + <endpoint type='zone' container-id='id' enabled='false'> + <region>us</region> + </endpoint> + </endpoints> + </instance> + <endpoints> + <endpoint container-id='id' region='us'> + <instance weight='1'>default</instance> + </endpoint> + </endpoints> + </deployment> + """)) + .getMessage()); + } + + @Test public void applicationLevelEndpoint() { DeploymentSpec spec = DeploymentSpec.fromXml(""" <deployment> |