diff options
author | jonmv <venstad@gmail.com> | 2023-01-17 10:23:06 +0100 |
---|---|---|
committer | jonmv <venstad@gmail.com> | 2023-01-17 15:49:46 +0100 |
commit | 0e6b82ab3021e57d21663b2d1baa2c8ba7d673b9 (patch) | |
tree | 0a34d8e75e243597af8a2188989ab4de61b57b80 /config-model-api | |
parent | 54b8e3357c3c4fdc6b23b7c3fb673555bec95d2b (diff) |
Address review
Diffstat (limited to 'config-model-api')
3 files changed, 48 insertions, 36 deletions
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 f72f09232ab..9d584efcd6b 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 @@ -261,7 +261,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { return zones().stream().anyMatch(zone -> zone.concerns(environment, Optional.of(region))); } - /** Returns the zone endpoint specified for the given region, or the default, or {@code null}. */ + /** Returns the zone endpoint specified for the given region, or empty. */ Optional<ZoneEndpoint> zoneEndpoint(ZoneId zone, ClusterSpec.Id cluster) { return Optional.ofNullable(zoneEndpoints.get(cluster)) .filter(__ -> deploysTo(zone.environment(), zone.region())) 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 a7c7ee3aab4..81c081fd17d 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 @@ -331,12 +331,39 @@ public class DeploymentSpecXmlReader { Optional<String> zoneEndpointType = getZoneEndpointType(endpointElement, level); String msgPrefix = (level == Endpoint.Level.application ? "Application-level" : "Instance-level") + " endpoint '" + endpointId.orElse(Endpoint.DEFAULT_ID) + "': "; + if (zoneEndpointType.isPresent() && endpointId.isPresent()) illegal(msgPrefix + "cannot declare 'id' with type 'zone' or 'private'"); + String invalidChild = level == Endpoint.Level.application ? "region" : "instance"; if ( ! XML.getChildren(endpointElement, invalidChild).isEmpty()) illegal(msgPrefix + "invalid element '" + invalidChild + "'"); + boolean enabled = XML.attribute("enabled", endpointElement) + .map(value -> { + if (zoneEndpointType.isEmpty() || ! zoneEndpointType.get().equals("zone")) + illegal(msgPrefix + "only endpoints of type 'zone' can specify 'enabled'"); + + return switch (value) { + case "true" -> true; + case "false" -> false; + default -> throw new IllegalArgumentException(msgPrefix + "invalid 'enabled' value; must be 'true' or 'false'"); + }; + }).orElse(true); + + List<AllowedUrn> allowedUrns = new ArrayList<>(); + for (var allow : XML.getChildren(endpointElement, "allow")) { + if (zoneEndpointType.isEmpty() || ! zoneEndpointType.get().equals("private")) + illegal(msgPrefix + "only endpoints of type 'private' can specify 'allow' children"); + + switch (requireStringAttribute("with", allow)) { + case "aws-private-link" -> allowedUrns.add(new AllowedUrn(AccessType.awsPrivateLink, requireStringAttribute("arn", allow))); + case "gcp-service-connect" -> allowedUrns.add(new AllowedUrn(AccessType.gcpServiceConnect, requireStringAttribute("project", allow))); + default -> illegal("Private endpoint for container-id '" + containerId + "': " + + "invalid attribute 'with': '" + requireStringAttribute("with", allow) + "'"); + } + } + List<Endpoint.Target> targets = new ArrayList<>(); if (level == Endpoint.Level.application) { Optional<String> endpointRegion = stringAttribute("region", endpointElement); @@ -371,30 +398,11 @@ public class DeploymentSpecXmlReader { if (zoneEndpointType.isPresent()) { if (regions.isEmpty()) regions.add(null); - ZoneEndpoint endpoint; - Optional<String> enabled = XML.attribute("enabled", endpointElement); - if ("zone".equals(zoneEndpointType.get())) { - switch (enabled.orElse("true")) { - case "true" -> endpoint = new ZoneEndpoint(true, false, List.of()); - case "false" -> endpoint = new ZoneEndpoint(false, false, List.of()); - default -> throw new IllegalArgumentException("Zone endpoint for container-id '" + containerId + "': " + - "invalid 'enabled' value; must be 'true' or 'false'"); - } - } - else { - if (enabled.isPresent()) illegal("Private endpoint for container-id '" + containerId + "': " + - "only endpoints of type 'zone' can specify 'enabled'"); - List<AllowedUrn> allowedUrns = new ArrayList<>(); - for (var allow : XML.getChildren(endpointElement, "allow")) { - switch (requireStringAttribute("with", allow)) { - case "aws-private-link" -> allowedUrns.add(new AllowedUrn(AccessType.awsPrivateLink, requireStringAttribute("arn", allow))); - case "gcp-service-connect" -> allowedUrns.add(new AllowedUrn(AccessType.gcpServiceConnect, requireStringAttribute("project", allow))); - default -> illegal("Private endpoint for container-id '" + containerId + "': " + - "invalid attribute 'with': '" + requireStringAttribute("with", allow) + "'"); - } - } - endpoint = new ZoneEndpoint(true, true, allowedUrns); // Doesn't turn off public visibility. - } + ZoneEndpoint endpoint = switch (zoneEndpointType.get()) { + case "zone" -> new ZoneEndpoint(enabled, false, List.of()); + case "private" -> new ZoneEndpoint(true, true, allowedUrns); // Doesn't turn off public visibility. + default -> throw new IllegalArgumentException("unsupported zone endpoint type '" + zoneEndpointType.get() + "'"); + }; for (RegionName region : regions) endpointsByZone.computeIfAbsent(containerId, __ -> new LinkedHashMap<>()) .computeIfAbsent(region, __ -> new ArrayList<>()) .add(endpoint); @@ -430,16 +438,18 @@ public class DeploymentSpecXmlReader { List<ZoneEndpoint> wildcards = regions.remove(null); ZoneEndpoint wildcardZoneEndpoint = null; ZoneEndpoint wildcardPrivateEndpoint = null; - if (wildcards != null) for (ZoneEndpoint endpoint : wildcards) { - if (endpoint.isPrivateEndpoint()) { - if (wildcardPrivateEndpoint != null) illegal("Multiple private endpoints (for all regions) declared for " + - "container id '" + cluster + "'"); - wildcardPrivateEndpoint = endpoint; - } - else { - if (wildcardZoneEndpoint != null) illegal("Multiple zone endpoints (for all regions) declared " + - "for container id '" + cluster + "'"); - wildcardZoneEndpoint = endpoint; + if (wildcards != null) { + for (ZoneEndpoint endpoint : wildcards) { + if (endpoint.isPrivateEndpoint()) { + if (wildcardPrivateEndpoint != null) illegal("Multiple private endpoints (for all regions) declared for " + + "container id '" + cluster + "'"); + wildcardPrivateEndpoint = endpoint; + } + else { + if (wildcardZoneEndpoint != null) illegal("Multiple zone endpoints (for all regions) declared " + + "for container id '" + cluster + "'"); + wildcardZoneEndpoint = endpoint; + } } } for (RegionName region : regions.keySet()) { 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 4c14717886c..918239a646a 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 @@ -1310,8 +1310,10 @@ public class DeploymentSpecTest { "Missing required attribute 'container-id' in 'endpoint'"); assertInvalidEndpoints("<endpoint type='private' />", "Missing required attribute 'container-id' in 'endpoint'"); + assertInvalidEndpoints("<endpoint container-id='foo' type='zone'><allow /></endpoint>", + "Instance-level endpoint 'default': only endpoints of type 'private' can specify 'allow' children"); assertInvalidEndpoints("<endpoint type='private' container-id='foo' enabled='true' />", - "Private endpoint for container-id 'foo': only endpoints of type 'zone' can specify 'enabled'"); + "Instance-level endpoint 'default': only endpoints of type 'zone' can specify 'enabled'"); assertInvalidEndpoints("<endpoint type='zone' container-id='qrs'/><endpoint type='zone' container-id='qrs'/>", "Multiple zone endpoints (for all regions) declared for container id 'qrs'"); assertInvalidEndpoints("<endpoint type='private' container-id='qrs'><region>us</region></endpoint>" + |