From 669ae1bc6572c198e609bc20cacbdc592e8e2731 Mon Sep 17 00:00:00 2001 From: jonmv Date: Wed, 18 Jan 2023 18:59:51 +0100 Subject: Revert "Merge pull request #25624 from vespa-engine/revert-25617-jonmv/private-endpoints" This reverts commit c47ed544a31a6b56f518901247212a47d8eb9d31, reversing changes made to e0191b4d49048f9398395dc8c1c60dfcb383f705. --- .../application/api/DeploymentInstanceSpec.java | 31 ++- .../config/application/api/DeploymentSpec.java | 26 +- .../yahoo/config/application/api/ValidationId.java | 1 + .../api/xml/DeploymentSpecXmlReader.java | 214 +++++++++++++--- .../model/api/ApplicationClusterEndpoint.java | 2 +- .../config/application/api/DeploymentSpecTest.java | 282 ++++++++++++++++----- .../api/DeploymentSpecWithoutInstanceTest.java | 52 +++- 7 files changed, 495 insertions(+), 113 deletions(-) (limited to 'config-model-api/src') 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 fdde4c38fb8..b36c1409459 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 @@ -3,17 +3,23 @@ package com.yahoo.config.application.api; import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.CloudAccount; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.Tags; +import com.yahoo.config.provision.ZoneEndpoint; +import com.yahoo.config.provision.zone.ZoneId; import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -26,7 +32,6 @@ import static ai.vespa.validation.Validation.requireInRange; import static com.yahoo.config.application.api.DeploymentSpec.RevisionChange.whenClear; import static com.yahoo.config.application.api.DeploymentSpec.RevisionTarget.next; import static com.yahoo.config.provision.Environment.prod; -import static java.util.stream.Collectors.toList; /** * The deployment spec for an application instance @@ -55,6 +60,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { private final Optional cloudAccount; private final Notifications notifications; private final List endpoints; + private final Map> zoneEndpoints; public DeploymentInstanceSpec(InstanceName name, Tags tags, @@ -70,6 +76,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { Optional cloudAccount, Notifications notifications, List endpoints, + Map> zoneEndpoints, Instant now) { super(steps); this.name = Objects.requireNonNull(name); @@ -91,6 +98,9 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { this.cloudAccount = Objects.requireNonNull(cloudAccount); this.notifications = Objects.requireNonNull(notifications); this.endpoints = List.copyOf(Objects.requireNonNull(endpoints)); + Map> zoneEndpointsCopy = new HashMap<>(); + for (var entry : zoneEndpoints.entrySet()) zoneEndpointsCopy.put(entry.getKey(), Collections.unmodifiableMap(new HashMap<>(entry.getValue()))); + this.zoneEndpoints = Collections.unmodifiableMap(zoneEndpointsCopy); validateZones(new HashSet<>(), new HashSet<>(), this); validateEndpoints(steps(), globalServiceId, this.endpoints); validateChangeBlockers(changeBlockers, now); @@ -251,6 +261,25 @@ 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 empty. */ + Optional zoneEndpoint(ZoneId zone, ClusterSpec.Id cluster) { + return Optional.ofNullable(zoneEndpoints.get(cluster)) + .filter(__ -> deploysTo(zone.environment(), zone.region())) + .map(zoneEndpoints -> zoneEndpoints.get(zoneEndpoints.containsKey(zone) ? zone : null)); + } + + /** Returns the zone endpoint data for this instance. */ + Map> zoneEndpoints() { + return zoneEndpoints; + } + + /** The zone endpoints in the given zone, possibly default values. */ + public Map zoneEndpoints(ZoneId zone) { + return zoneEndpoints.keySet().stream() + .collect(Collectors.toMap(cluster -> cluster, + cluster -> zoneEndpoint(zone, cluster).orElse(ZoneEndpoint.defaultEndpoint))); + } + @Override public boolean equals(Object o) { if (this == o) return true; 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 6c519a4656e..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 @@ -6,16 +6,21 @@ import com.yahoo.config.application.api.xml.DeploymentSpecXmlReader; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.CloudAccount; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.ZoneEndpoint; +import com.yahoo.config.provision.zone.ZoneId; import java.io.Reader; import java.time.Duration; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.stream.Collectors; @@ -47,7 +52,7 @@ public class DeploymentSpec { private final List steps; - // Attributes which can be set on the root tag and which must be available outside of any particular instance + // Attributes which can be set on the root tag and which must be available outside any particular instance private final Optional majorVersion; private final Optional athenzDomain; private final Optional athenzService; @@ -145,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'"); } } } @@ -175,6 +184,21 @@ public class DeploymentSpec { /** Cloud account set on the deployment root; see discussion for {@link #athenzService}. */ public Optional cloudAccount() { return cloudAccount; } + /** + * Returns the most specific zone endpoint, where specificity is given, in decreasing order: + * 1. The given instance has declared a zone endpoint for the cluster, for the given region. + * 2. The given instance has declared a universal zone endpoint for the cluster. + * 3. The application has declared a zone endpoint for the cluster, for the given region. + * 4. The application has declared a universal zone endpoint for the cluster. + * 5. None of the above apply, and the default of a publicly visible endpoint is used. + */ + public ZoneEndpoint zoneEndpoint(InstanceName instance, ZoneId zone, ClusterSpec.Id cluster) { + // TODO: look up endpoints from tag, or so, if we're to support non-prod settings. + if (zone.environment() != Environment.prod) return ZoneEndpoint.defaultEndpoint; + return instance(instance).flatMap(spec -> spec.zoneEndpoint(zone, cluster)) + .orElse(ZoneEndpoint.defaultEndpoint); + } + /** Returns the XML form of this spec, or null if it was not created by fromXml, nor is empty */ public String xmlForm() { return xmlForm; } diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java index 571cc3c7d5c..b4be99ad20b 100644 --- a/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java +++ b/config-model-api/src/main/java/com/yahoo/config/application/api/ValidationId.java @@ -24,6 +24,7 @@ public enum ValidationId { skipOldConfigModels("skip-old-config-models"), // Internal use accessControl("access-control"), // Internal use, used in zones where there should be no access-control globalEndpointChange("global-endpoint-change"), // Changing global endpoints + zoneEndpointChange("zone-endpoint-change"), // Changing zone (possibly private) endpoint settings redundancyIncrease("redundancy-increase"), // Increasing redundancy - may easily cause feed blocked redundancyOne("redundancy-one"), // redundancy=1 requires a validation override on first deployment pagedSettingRemoval("paged-setting-removal"), // May cause content nodes to run out of memory 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 8182e697e7e..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 @@ -15,6 +15,8 @@ import com.yahoo.config.application.api.DeploymentSpec.Steps; import com.yahoo.config.application.api.DeploymentSpec.UpgradePolicy; import com.yahoo.config.application.api.DeploymentSpec.UpgradeRollout; import com.yahoo.config.application.api.Endpoint; +import com.yahoo.config.application.api.Endpoint.Level; +import com.yahoo.config.application.api.Endpoint.Target; import com.yahoo.config.application.api.Notifications; import com.yahoo.config.application.api.Notifications.Role; import com.yahoo.config.application.api.Notifications.When; @@ -22,10 +24,15 @@ import com.yahoo.config.application.api.TimeWindow; import com.yahoo.config.provision.AthenzDomain; import com.yahoo.config.provision.AthenzService; import com.yahoo.config.provision.CloudAccount; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.Tags; +import com.yahoo.config.provision.ZoneEndpoint; +import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn; +import com.yahoo.config.provision.ZoneEndpoint.AccessType; +import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.io.IOUtils; import com.yahoo.text.XML; import org.w3c.dom.Element; @@ -39,16 +46,20 @@ 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; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Function; -import java.util.stream.Collectors; import java.util.stream.Stream; +import static java.util.Comparator.comparingInt; + /** * @author bratseth */ @@ -123,7 +134,7 @@ public class DeploymentSpecXmlReader { return DeploymentSpec.empty; List steps = new ArrayList<>(); - List applicationEndpoints = List.of(); + List applicationEndpoints = new ArrayList<>(); if ( ! containsTag(instanceTag, root)) { // deployment spec skipping explicit instance -> "default" instance steps.addAll(readInstanceContent("default", root, new HashMap<>(), root)); } @@ -141,7 +152,7 @@ public class DeploymentSpecXmlReader { steps.addAll(readNonInstanceSteps(child, new HashMap<>(), root)); // (No global service id here) } } - applicationEndpoints = readEndpoints(root, Optional.empty(), steps); + readEndpoints(root, Optional.empty(), steps, applicationEndpoints, Map.of()); } return new DeploymentSpec(steps, @@ -190,11 +201,13 @@ public class DeploymentSpecXmlReader { Notifications notifications = readNotifications(instanceElement, parentTag); // Values where there is no default - Tags tags = XML.attribute(tagsTag, instanceElement).map(value -> Tags.fromString(value)).orElse(Tags.empty()); + Tags tags = XML.attribute(tagsTag, instanceElement).map(Tags::fromString).orElse(Tags.empty()); List steps = new ArrayList<>(); for (Element instanceChild : XML.getChildren(instanceElement)) steps.addAll(readNonInstanceSteps(instanceChild, prodAttributes, instanceChild)); - List endpoints = readEndpoints(instanceElement, Optional.of(instanceNameString), steps); + List endpoints = new ArrayList<>(); + Map> zoneEndpoints = new LinkedHashMap<>(); + readEndpoints(instanceElement, Optional.of(instanceNameString), steps, endpoints, zoneEndpoints); // Build and return instances with these values Instant now = clock.instant(); @@ -214,6 +227,7 @@ public class DeploymentSpecXmlReader { cloudAccount, notifications, endpoints, + zoneEndpoints, now)) .toList(); } @@ -306,19 +320,54 @@ public class DeploymentSpecXmlReader { return Notifications.of(emailAddresses, emailRoles); } - private List readEndpoints(Element parent, Optional instance, List steps) { + private void readEndpoints(Element parent, Optional instance, List steps, List endpoints, + Map> zoneEndpoints) { var endpointsElement = XML.getChild(parent, endpointsTag); - if (endpointsElement == null) return List.of(); + if (endpointsElement == null) return; Endpoint.Level level = instance.isEmpty() ? Endpoint.Level.application : Endpoint.Level.instance; - Map endpoints = new LinkedHashMap<>(); - for (var endpointElement : XML.getChildren(endpointsElement, endpointTag)) { - String endpointId = stringAttribute("id", endpointElement).orElse(Endpoint.DEFAULT_ID); + Map endpointsById = new LinkedHashMap<>(); + Map>> endpointsByZone = new LinkedHashMap<>(); + 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 endpointId = stringAttribute("id", endpointElement); + Optional zoneEndpointType = getZoneEndpointType(endpointElement, level); String msgPrefix = (level == Endpoint.Level.application ? "Application-level" : "Instance-level") + - " endpoint '" + endpointId + "': "; + " 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 + "'"); + 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 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 targets = new ArrayList<>(); if (level == Endpoint.Level.application) { @@ -345,33 +394,132 @@ public class DeploymentSpecXmlReader { if (weightSum == 0) illegal(msgPrefix + "sum of all weights must be positive, got " + weightSum); } else { if (stringAttribute("region", endpointElement).isPresent()) illegal(msgPrefix + "invalid 'region' attribute"); + Set regions = new LinkedHashSet<>(); for (var regionElement : XML.getChildren(endpointElement, "region")) { String region = regionElement.getTextContent(); - if (region == null || region.isBlank()) illegal(msgPrefix + "empty 'region' element"); - targets.add(new Endpoint.Target(RegionName.from(region), - InstanceName.from(instance.get()), - 1)); + 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()) { + if (regions.isEmpty()) regions.add(null); + 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); + } + else { + if (regions.isEmpty()) { + // No explicit targets given for instance level endpoint. Include all declared, enabled regions by default + List 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)); } - } - if (targets.isEmpty() && level == Endpoint.Level.instance) { - // No explicit targets given for instance level endpoint. Include all declared regions by default - InstanceName instanceName = instance.map(InstanceName::from).get(); - steps.stream() - .filter(step -> step.concerns(Environment.prod)) - .flatMap(step -> step.zones().stream()) - .flatMap(zone -> zone.region().stream()) - .distinct() - .map(region -> new Endpoint.Target(region, instanceName, 1)) - .forEach(targets::add); } - Endpoint endpoint = new Endpoint(endpointId, containerId, level, targets); - if (endpoints.containsKey(endpoint.endpointId())) { - illegal("Endpoint ID '" + endpoint.endpointId() + "' is specified multiple times"); + if (zoneEndpointType.isEmpty()) { + Endpoint endpoint = new Endpoint(endpointId.orElse(Endpoint.DEFAULT_ID), containerId, level, targets); + if (endpointsById.containsKey(endpoint.endpointId())) { + illegal("Endpoint ID '" + endpoint.endpointId() + "' is specified multiple times"); + } + endpointsById.put(endpoint.endpointId(), endpoint); } - endpoints.put(endpoint.endpointId(), endpoint); - } - return List.copyOf(endpoints.values()); + }); + endpoints.addAll(endpointsById.values()); + validateAndConsolidate(endpointsByZone, zoneEndpoints); + } + + static void validateAndConsolidate(Map>> in, Map> out) { + in.forEach((cluster, regions) -> { + List 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; + } + } + } + for (RegionName region : regions.keySet()) { + ZoneEndpoint zoneEndpoint = null; + ZoneEndpoint privateEndpoint = null; + for (ZoneEndpoint endpoint : regions.getOrDefault(region, List.of())) { + if (endpoint.isPrivateEndpoint()) { + if (privateEndpoint != null) illegal("Multiple private endpoints declared for " + + "container id '" + cluster + "' in region '" + region + "'"); + privateEndpoint = endpoint; + } + else { + if (zoneEndpoint != null) illegal("Multiple zone endpoints (without regions) declared " + + "for container id '" + cluster + "' in region '" + region + "'"); + zoneEndpoint = endpoint; + } + } + if (wildcardZoneEndpoint != null && zoneEndpoint != null) illegal("Zone endpoint for container id '" + cluster + "' declared " + + "both with region '" + region + "', and for all regions."); + if (wildcardPrivateEndpoint != null && privateEndpoint != null) illegal("Private endpoint for container id '" + cluster + "' declared " + + "both with region '" + region + "', and for all regions."); + + if (zoneEndpoint == null) zoneEndpoint = wildcardZoneEndpoint; + if (privateEndpoint == null) privateEndpoint = wildcardPrivateEndpoint; + + // Gosh, we made it here! Now we'll combine the settings for zone and private types into one ZoneEndpoint to rule them all. + out.computeIfAbsent(ClusterSpec.Id.from(cluster), __ -> new LinkedHashMap<>()) + .put(ZoneId.from(Environment.prod, region), new ZoneEndpoint(zoneEndpoint == null || zoneEndpoint.isPublicEndpoint(), + privateEndpoint != null, + privateEndpoint != null ? privateEndpoint.allowedUrns() : List.of())); + } + out.computeIfAbsent(ClusterSpec.Id.from(cluster), __ -> new LinkedHashMap<>()) + .put(null, new ZoneEndpoint(wildcardZoneEndpoint == null || wildcardZoneEndpoint.isPublicEndpoint(), + wildcardPrivateEndpoint != null, + wildcardPrivateEndpoint != null ? wildcardPrivateEndpoint.allowedUrns() : List.of())); + }); + } + + /** Returns endpoint type if a private or zone type endpoint, throws if invalid, or otherwise returns empty (global, application). */ + static Optional getZoneEndpointType(Element endpoint, Level level) { + Optional type = XML.attribute("type", endpoint); + if (type.isPresent() && ! List.of("zone", "private", "global", "application").contains(type.get())) + illegal("Illegal endpoint type '" + type.get() + "'"); + + String implied = switch (level) { case instance -> "global"; case application -> "application"; }; + if (type.isEmpty() || type.get().equals(implied)) return Optional.empty(); + if (level == Level.instance && (type.get().equals("zone") || type.get().equals("private"))) return type; + throw new IllegalArgumentException("Endpoints at " + level + " level cannot be of type '" + type.get() + "'"); } /** diff --git a/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java b/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java index 99bf768f67e..b7969267328 100644 --- a/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java +++ b/config-model-api/src/main/java/com/yahoo/config/model/api/ApplicationClusterEndpoint.java @@ -26,7 +26,7 @@ public class ApplicationClusterEndpoint { ", routingMethod=" + routingMethod + ", weight=" + weight + ", hostNames=" + hostNames + - ", clusterId='" + clusterId + '\'' + + ", clusterId='" + clusterId + "'" + '}'; } 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 96cd4810ec4..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 @@ -6,10 +6,14 @@ import com.yahoo.config.application.api.Endpoint.Level; import com.yahoo.config.application.api.Endpoint.Target; import com.yahoo.config.application.api.xml.DeploymentSpecXmlReader; import com.yahoo.config.provision.CloudAccount; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.Tags; +import com.yahoo.config.provision.ZoneEndpoint; +import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn; +import com.yahoo.config.provision.ZoneEndpoint.AccessType; import com.yahoo.test.ManualClock; import org.junit.Test; @@ -19,6 +23,7 @@ import java.time.Duration; import java.time.Instant; import java.time.ZoneId; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -27,9 +32,12 @@ import java.util.stream.Collectors; import static com.yahoo.config.application.api.Notifications.Role.author; import static com.yahoo.config.application.api.Notifications.When.failing; import static com.yahoo.config.application.api.Notifications.When.failingCommit; +import static com.yahoo.config.provision.zone.ZoneId.defaultId; +import static com.yahoo.config.provision.zone.ZoneId.from; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -1183,15 +1191,16 @@ public class DeploymentSpecTest { @Test public void customTesterFlavor() { - DeploymentSpec spec = DeploymentSpec.fromXml("" + - " " + - " " + - " " + - " " + - " us-north-7" + - " " + - " " + - ""); + DeploymentSpec spec = DeploymentSpec.fromXml(""" + + + + + + us-north-7 + + + """); assertEquals(Optional.of("d-1-4-20"), spec.requireInstance("default").steps().get(0).zones().get(0).testerFlavor()); assertEquals(Optional.empty(), spec.requireInstance("default").steps().get(1).zones().get(0).testerFlavor()); assertEquals(Optional.of("d-2-8-50"), spec.requireInstance("default").steps().get(2).zones().get(0).testerFlavor()); @@ -1199,39 +1208,55 @@ public class DeploymentSpecTest { @Test public void noEndpoints() { - assertEquals(Collections.emptyList(), - DeploymentSpec.fromXml("" + - " " + - "").requireInstance("default").endpoints()); + DeploymentSpec spec = DeploymentSpec.fromXml(""" + + + + """); + assertEquals(Collections.emptyList(), spec.requireInstance("default").endpoints()); + assertEquals(ZoneEndpoint.defaultEndpoint, spec.zoneEndpoint(InstanceName.defaultName(), + defaultId(), + ClusterSpec.Id.from("cluster"))); } @Test public void emptyEndpoints() { - var spec = DeploymentSpec.fromXml("" + - " " + - " " + - " " + - ""); + var spec = DeploymentSpec.fromXml(""" + + + + + """); assertEquals(List.of(), spec.requireInstance("default").endpoints()); + assertEquals(ZoneEndpoint.defaultEndpoint, spec.zoneEndpoint(InstanceName.defaultName(), + defaultId(), + ClusterSpec.Id.from("cluster"))); } @Test public void someEndpoints() { - var spec = DeploymentSpec.fromXml("" + - "" + - " " + - " " + - " us-east" + - " " + - " " + - " " + - " us-east" + - " " + - " " + - " " + - " " + - " " + - ""); + var spec = DeploymentSpec.fromXml(""" + + + + us-east + + + + us-east + + + + + + + us-east + + + + + + """); assertEquals( List.of("foo", "nalle", "default"), @@ -1244,18 +1269,59 @@ public class DeploymentSpecTest { ); assertEquals(List.of(RegionName.from("us-east")), spec.requireInstance("default").endpoints().get(0).regions()); + + var zone = from(Environment.prod, RegionName.from("us-east")); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("custom"), zone, ClusterSpec.Id.from("bax"))); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("default"), defaultId(), ClusterSpec.Id.from("bax"))); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("default"), zone, ClusterSpec.Id.from("bax"))); + + assertEquals(new ZoneEndpoint(false, true, List.of(new AllowedUrn(AccessType.awsPrivateLink, "barn"), + new AllowedUrn(AccessType.gcpServiceConnect, "nine"))), + spec.zoneEndpoint(InstanceName.from("default"), zone, ClusterSpec.Id.from("froz"))); } @Test public void invalidEndpoints() { - assertInvalidEndpoints(""); // Uppercase - assertInvalidEndpoints(""); // Starting with non-character - assertInvalidEndpoints(""); // Non-alphanumeric - assertInvalidEndpoints(""); - assertInvalidEndpoints(""); // Multiple consecutive dashes - assertInvalidEndpoints(""); // Trailing dash - assertInvalidEndpoints(""); // Too long - assertInvalidEndpoints(""); // Duplicate + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'FOO'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got '123'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'foo!'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'foo.bar'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'foo--bar'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'foo-'"); + assertInvalidEndpoints("", + "Endpoint ID must be all lowercase, alphanumeric, with no consecutive dashes, of length 1 to 12, and begin with a character; but got 'foooooooooooo'"); + + assertInvalidEndpoints("", + "Endpoint ID 'foo' is specified multiple times"); + assertInvalidEndpoints("", + "Instance-level endpoint 'default': cannot declare 'id' with type 'zone' or 'private'"); + assertInvalidEndpoints("", + "Instance-level endpoint 'default': cannot declare 'id' with type 'zone' or 'private'"); + assertInvalidEndpoints("", + "Missing required attribute 'container-id' in 'endpoint'"); + assertInvalidEndpoints("", + "Missing required attribute 'container-id' in 'endpoint'"); + assertInvalidEndpoints("", + "Instance-level endpoint 'default': only endpoints of type 'private' can specify 'allow' children"); + assertInvalidEndpoints("", + "Instance-level endpoint 'default': only endpoints of type 'zone' can specify 'enabled'"); + assertInvalidEndpoints("", + "Multiple zone endpoints (for all regions) declared for container id 'qrs'"); + assertInvalidEndpoints("us" + + "us", + "Multiple private endpoints declared for container id 'qrs' in region 'us'"); + assertInvalidEndpoints("" + + "us", + "Zone endpoint for container id 'qrs' declared both with region 'us', and for all regions."); } @Test @@ -1271,25 +1337,44 @@ public class DeploymentSpecTest { @Test public void endpointDefaultRegions() { - var spec = DeploymentSpec.fromXml("" + - " " + - " " + - " us-east" + - " us-west" + - " " + - " " + - " " + - " us-east" + - " " + - " " + - " " + - " " + - " " + - ""); + var spec = DeploymentSpec.fromXml(""" + + + + us-east + us-west + + + + us-east + + + us-east + + + + + + + """); 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(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"))); + assertEquals(new ZoneEndpoint(true, true, List.of()), + spec.zoneEndpoint(InstanceName.from("default"), from("prod", "us-east"), ClusterSpec.Id.from("quux"))); + assertEquals(new ZoneEndpoint(true, true, List.of()), + spec.zoneEndpoint(InstanceName.from("default"), from("prod", "us-west"), ClusterSpec.Id.from("quux"))); + assertEquals(new HashSet<>() {{ add(null); add(from("prod", "us-east")); }}, + spec.requireInstance("default").zoneEndpoints().get(ClusterSpec.Id.from("bar")).keySet()); + assertEquals(new HashSet<>() {{ add(null); }}, + spec.requireInstance("default").zoneEndpoints().get(ClusterSpec.Id.from("quux")).keySet()); + assertEquals(Set.of(ClusterSpec.Id.from("bar"), ClusterSpec.Id.from("quux")), + spec.requireInstance("default").zoneEndpoints().keySet()); } @Test @@ -1302,14 +1387,16 @@ public class DeploymentSpecTest { us-west - + %s """; - assertInvalid(String.format(xmlForm, "region='us-east'", "us-east"), "Instance-level endpoint 'foo': invalid 'region' attribute"); - assertInvalid(String.format(xmlForm, "", "us-east"), "Instance-level endpoint 'foo': invalid element 'instance'"); + assertInvalid(String.format(xmlForm, "id='foo' region='us-east'", "us-east"), "Instance-level endpoint 'foo': invalid 'region' attribute"); + assertInvalid(String.format(xmlForm, "id='foo'", "us-east"), "Instance-level endpoint 'foo': invalid element 'instance'"); + assertInvalid(String.format(xmlForm, "type='zone'", "us-east"), "Instance-level endpoint 'default': invalid element 'instance'"); + assertInvalid(String.format(xmlForm, "type='private'", "us-east"), "Instance-level endpoint 'default': invalid element 'instance'"); } @Test @@ -1343,6 +1430,73 @@ public class DeploymentSpecTest { assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='foo'", "", "main", ""), "Application-level endpoint 'foo': invalid weight value 'foo'"); assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='1'", "", "main", "us-east-3"), "Application-level endpoint 'foo': invalid element 'region'"); assertInvalid(String.format(xmlForm, "region='us-west-1'", "weight='0'", "", "main", ""), "Application-level endpoint 'foo': sum of all weights must be positive, got 0"); + assertInvalid(String.format(xmlForm, "type='zone'", "weight='1'", "", "main", ""), "Endpoints at application level cannot be of type 'zone'"); + assertInvalid(String.format(xmlForm, "type='private'", "weight='1'", "", "main", ""), "Endpoints at application level cannot be of type 'private'"); + } + + @Test + public void cannotTargetDisabledEndpoints() { + assertEquals("Instance-level endpoint 'default': all eligible zone endpoints have 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + + + + us + eu + + + + + + + + """)) + .getMessage()); + + assertEquals("Instance-level endpoint 'default': targets zone endpoint in 'us' with 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + + + + us + eu + + + + us + + + + + + """)) + .getMessage()); + + assertEquals("Application-level endpoint 'default': targets 'us' in 'default', but its zone endpoint has 'enabled' set to 'false'", + assertThrows(IllegalArgumentException.class, + () -> DeploymentSpec.fromXml(""" + + + + us + eu + + + + us + + + + + + default + + + + """)) + .getMessage()); } @Test @@ -1648,11 +1802,11 @@ public class DeploymentSpecTest { } } - private static void assertInvalidEndpoints(String endpointsBody) { - try { - endpointIds(endpointsBody); - fail("Expected exception for input '" + endpointsBody + "'"); - } catch (IllegalArgumentException ignored) {} + private static void assertInvalidEndpoints(String endpointsBody, String error) { + assertEquals(error, + assertThrows(IllegalArgumentException.class, + () -> endpointIds(endpointsBody)) + .getMessage()); } private static Set endpointRegions(String endpointId, DeploymentSpec spec) { 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 6ff5616a80f..38410cc5b37 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 @@ -3,8 +3,13 @@ package com.yahoo.config.application.api; import com.google.common.collect.ImmutableSet; import com.yahoo.config.provision.CloudAccount; +import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.InstanceName; import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.ZoneEndpoint; +import com.yahoo.config.provision.ZoneEndpoint.AllowedUrn; +import com.yahoo.config.provision.ZoneEndpoint.AccessType; import org.junit.Test; import java.io.StringReader; @@ -20,6 +25,8 @@ import java.util.stream.Collectors; import static com.yahoo.config.application.api.Notifications.Role.author; import static com.yahoo.config.application.api.Notifications.When.failing; import static com.yahoo.config.application.api.Notifications.When.failingCommit; +import static com.yahoo.config.provision.zone.ZoneId.defaultId; +import static com.yahoo.config.provision.zone.ZoneId.from; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -654,19 +661,26 @@ public class DeploymentSpecWithoutInstanceTest { @Test public void someEndpoints() { - var spec = DeploymentSpec.fromXml("" + - "" + - " " + - " us-east" + - " " + - " " + - " " + - " us-east" + - " " + - " " + - " " + - " " + - ""); + var spec = DeploymentSpec.fromXml(""" + + + us-east + + + + us-east + + + + + + + us-east + + + + + """); assertEquals( List.of("foo", "nalle", "default"), @@ -679,6 +693,18 @@ public class DeploymentSpecWithoutInstanceTest { ); assertEquals(List.of(RegionName.from("us-east")), spec.requireInstance("default").endpoints().get(0).regions()); + + var zone = from(Environment.prod, RegionName.from("us-east")); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("custom"), zone, ClusterSpec.Id.from("bax"))); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("default"), defaultId(), ClusterSpec.Id.from("bax"))); + assertEquals(ZoneEndpoint.defaultEndpoint, + spec.zoneEndpoint(InstanceName.from("default"), zone, ClusterSpec.Id.from("bax"))); + + assertEquals(new ZoneEndpoint(false, true, List.of(new AllowedUrn(AccessType.awsPrivateLink, "barn"), + new AllowedUrn(AccessType.gcpServiceConnect, "nine"))), + spec.zoneEndpoint(InstanceName.from("default"), zone, ClusterSpec.Id.from("froz"))); } @Test -- cgit v1.2.3