From b7778b0652a24e7afb670fcfa1750fbebe3c3a36 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 18 Jan 2023 11:46:22 +0100 Subject: Revert "Parse, validate and use new zone endpoint syntax" --- .../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, 113 insertions(+), 495 deletions(-) (limited to 'config-model-api') 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 b36c1409459..fdde4c38fb8 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,23 +3,17 @@ 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; @@ -32,6 +26,7 @@ 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 @@ -60,7 +55,6 @@ 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, @@ -76,7 +70,6 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps { Optional cloudAccount, Notifications notifications, List endpoints, - Map> zoneEndpoints, Instant now) { super(steps); this.name = Objects.requireNonNull(name); @@ -98,9 +91,6 @@ 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); @@ -261,25 +251,6 @@ 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 d731e09d4e4..6c519a4656e 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,21 +6,16 @@ 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; @@ -52,7 +47,7 @@ public class DeploymentSpec { private final List steps; - // Attributes which can be set on the root tag and which must be available outside any particular instance + // Attributes which can be set on the root tag and which must be available outside of any particular instance private final Optional majorVersion; private final Optional athenzDomain; private final Optional athenzService; @@ -150,10 +145,6 @@ 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'"); } } } @@ -184,21 +175,6 @@ 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 b4be99ad20b..571cc3c7d5c 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,7 +24,6 @@ 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 fb6d834f783..8182e697e7e 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,8 +15,6 @@ 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; @@ -24,15 +22,10 @@ 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; @@ -46,20 +39,16 @@ 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 */ @@ -134,7 +123,7 @@ public class DeploymentSpecXmlReader { return DeploymentSpec.empty; List steps = new ArrayList<>(); - List applicationEndpoints = new ArrayList<>(); + List applicationEndpoints = List.of(); if ( ! containsTag(instanceTag, root)) { // deployment spec skipping explicit instance -> "default" instance steps.addAll(readInstanceContent("default", root, new HashMap<>(), root)); } @@ -152,7 +141,7 @@ public class DeploymentSpecXmlReader { steps.addAll(readNonInstanceSteps(child, new HashMap<>(), root)); // (No global service id here) } } - readEndpoints(root, Optional.empty(), steps, applicationEndpoints, Map.of()); + applicationEndpoints = readEndpoints(root, Optional.empty(), steps); } return new DeploymentSpec(steps, @@ -201,13 +190,11 @@ public class DeploymentSpecXmlReader { Notifications notifications = readNotifications(instanceElement, parentTag); // Values where there is no default - Tags tags = XML.attribute(tagsTag, instanceElement).map(Tags::fromString).orElse(Tags.empty()); + Tags tags = XML.attribute(tagsTag, instanceElement).map(value -> Tags.fromString(value)).orElse(Tags.empty()); List steps = new ArrayList<>(); for (Element instanceChild : XML.getChildren(instanceElement)) steps.addAll(readNonInstanceSteps(instanceChild, prodAttributes, instanceChild)); - List endpoints = new ArrayList<>(); - Map> zoneEndpoints = new LinkedHashMap<>(); - readEndpoints(instanceElement, Optional.of(instanceNameString), steps, endpoints, zoneEndpoints); + List endpoints = readEndpoints(instanceElement, Optional.of(instanceNameString), steps); // Build and return instances with these values Instant now = clock.instant(); @@ -227,7 +214,6 @@ public class DeploymentSpecXmlReader { cloudAccount, notifications, endpoints, - zoneEndpoints, now)) .toList(); } @@ -320,54 +306,19 @@ public class DeploymentSpecXmlReader { return Notifications.of(emailAddresses, emailRoles); } - private void readEndpoints(Element parent, Optional instance, List steps, List endpoints, - Map> zoneEndpoints) { + private List readEndpoints(Element parent, Optional instance, List steps) { var endpointsElement = XML.getChild(parent, endpointsTag); - if (endpointsElement == null) return; + if (endpointsElement == null) return List.of(); Endpoint.Level level = instance.isEmpty() ? Endpoint.Level.application : Endpoint.Level.instance; - 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 -> { + Map endpoints = new LinkedHashMap<>(); + for (var endpointElement : XML.getChildren(endpointsElement, endpointTag)) { + String endpointId = stringAttribute("id", endpointElement).orElse(Endpoint.DEFAULT_ID); 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.orElse(Endpoint.DEFAULT_ID) + "': "; - - if (zoneEndpointType.isPresent() && endpointId.isPresent()) - illegal(msgPrefix + "cannot declare 'id' with type 'zone' or 'private'"); - + " endpoint '" + endpointId + "': "; 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 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) + "'"); - } - } + if (!XML.getChildren(endpointElement, invalidChild).isEmpty()) illegal(msgPrefix + "invalid element '" + invalidChild + "'"); List targets = new ArrayList<>(); if (level == Endpoint.Level.application) { @@ -394,132 +345,33 @@ 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"); - 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 (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"); + if (region == null || region.isBlank()) illegal(msgPrefix + "empty 'region' element"); + targets.add(new Endpoint.Target(RegionName.from(region), + InstanceName.from(instance.get()), + 1)); } - endpointsById.put(endpoint.endpointId(), endpoint); } - }); - 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; - } - } + 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); } - 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())); + + Endpoint endpoint = new Endpoint(endpointId, containerId, level, targets); + if (endpoints.containsKey(endpoint.endpointId())) { + illegal("Endpoint ID '" + endpoint.endpointId() + "' is specified multiple times"); } - 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() + "'"); + endpoints.put(endpoint.endpointId(), endpoint); + } + return List.copyOf(endpoints.values()); } /** 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 b7969267328..99bf768f67e 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 355ce651c34..96cd4810ec4 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,14 +6,10 @@ 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; @@ -23,7 +19,6 @@ 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; @@ -32,12 +27,9 @@ 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; @@ -1191,16 +1183,15 @@ 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()); @@ -1208,55 +1199,39 @@ public class DeploymentSpecTest { @Test public void noEndpoints() { - DeploymentSpec spec = DeploymentSpec.fromXml(""" - - - - """); - assertEquals(Collections.emptyList(), spec.requireInstance("default").endpoints()); - assertEquals(ZoneEndpoint.defaultEndpoint, spec.zoneEndpoint(InstanceName.defaultName(), - defaultId(), - ClusterSpec.Id.from("cluster"))); + assertEquals(Collections.emptyList(), + DeploymentSpec.fromXml("" + + " " + + "").requireInstance("default").endpoints()); } @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 - - - - - - - us-east - - - - - - """); + var spec = DeploymentSpec.fromXml("" + + "" + + " " + + " " + + " us-east" + + " " + + " " + + " " + + " us-east" + + " " + + " " + + " " + + " " + + " " + + ""); assertEquals( List.of("foo", "nalle", "default"), @@ -1269,59 +1244,18 @@ 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("", - "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."); + assertInvalidEndpoints(""); // Uppercase + assertInvalidEndpoints(""); // Starting with non-character + assertInvalidEndpoints(""); // Non-alphanumeric + assertInvalidEndpoints(""); + assertInvalidEndpoints(""); // Multiple consecutive dashes + assertInvalidEndpoints(""); // Trailing dash + assertInvalidEndpoints(""); // Too long + assertInvalidEndpoints(""); // Duplicate } @Test @@ -1337,44 +1271,25 @@ public class DeploymentSpecTest { @Test public void endpointDefaultRegions() { - var spec = DeploymentSpec.fromXml(""" - - - - us-east - us-west - - - - us-east - - - us-east - - - - - - - """); + var spec = DeploymentSpec.fromXml("" + + " " + + " " + + " us-east" + + " us-west" + + " " + + " " + + " " + + " 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 @@ -1387,16 +1302,14 @@ public class DeploymentSpecTest { us-west - + %s """; - 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'"); + 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'"); } @Test @@ -1430,73 +1343,6 @@ 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 @@ -1802,11 +1648,11 @@ public class DeploymentSpecTest { } } - private static void assertInvalidEndpoints(String endpointsBody, String error) { - assertEquals(error, - assertThrows(IllegalArgumentException.class, - () -> endpointIds(endpointsBody)) - .getMessage()); + private static void assertInvalidEndpoints(String endpointsBody) { + try { + endpointIds(endpointsBody); + fail("Expected exception for input '" + endpointsBody + "'"); + } catch (IllegalArgumentException ignored) {} } 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 38410cc5b37..6ff5616a80f 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,13 +3,8 @@ 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; @@ -25,8 +20,6 @@ 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; @@ -661,26 +654,19 @@ public class DeploymentSpecWithoutInstanceTest { @Test public void someEndpoints() { - var spec = DeploymentSpec.fromXml(""" - - - us-east - - - - us-east - - - - - - - us-east - - - - - """); + var spec = DeploymentSpec.fromXml("" + + "" + + " " + + " us-east" + + " " + + " " + + " " + + " us-east" + + " " + + " " + + " " + + " " + + ""); assertEquals( List.of("foo", "nalle", "default"), @@ -693,18 +679,6 @@ 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