From 2ee24c4628c75abb8f8495eac978b3eb75c66162 Mon Sep 17 00:00:00 2001 From: Håkon Hallingstad Date: Fri, 28 Jul 2023 14:33:32 +0200 Subject: Add cloud flag dimension --- .../com/yahoo/config/provision/zone/ZoneId.java | 11 ++ .../systemflags/v1/ConfigServerFlagsTarget.java | 36 +++- .../api/systemflags/v1/ControllerFlagsTarget.java | 35 +++- .../controller/api/systemflags/v1/FlagsTarget.java | 49 ++++- .../api/systemflags/v1/SystemFlagsDataArchive.java | 80 +++++--- .../systemflags/v1/SystemFlagsDataArchiveTest.java | 163 ++++++++-------- .../restapi/systemflags/SystemFlagsDeployer.java | 8 +- .../hosted/controller/integration/ZoneApiMock.java | 4 +- .../controller/integration/ZoneRegistryMock.java | 4 +- .../systemflags/SystemFlagsDeployResultTest.java | 11 +- .../systemflags/SystemFlagsDeployerTest.java | 103 +++++++++-- .../resources/system-flags/partial/default.json | 20 ++ .../resources/system-flags/partial/initial.json | 15 ++ .../system-flags/partial/put-controller-2.json | 15 ++ .../system-flags/partial/put-controller.json | 15 ++ .../java/com/yahoo/vespa/flags/FetchVector.java | 72 ++++--- .../src/main/java/com/yahoo/vespa/flags/Flags.java | 9 + .../com/yahoo/vespa/flags/JsonNodeRawFlag.java | 21 +++ .../yahoo/vespa/flags/json/DimensionHelper.java | 10 +- .../java/com/yahoo/vespa/flags/json/FlagData.java | 62 ++++++- .../com/yahoo/vespa/flags/json/ListCondition.java | 24 +++ .../vespa/flags/json/RelationalCondition.java | 23 +++ .../main/java/com/yahoo/vespa/flags/json/Rule.java | 43 ++++- .../com/yahoo/vespa/flags/json/FlagDataTest.java | 206 +++++++++++++++++---- 24 files changed, 817 insertions(+), 222 deletions(-) create mode 100644 controller-server/src/test/resources/system-flags/partial/default.json create mode 100644 controller-server/src/test/resources/system-flags/partial/initial.json create mode 100644 controller-server/src/test/resources/system-flags/partial/put-controller-2.json create mode 100644 controller-server/src/test/resources/system-flags/partial/put-controller.json diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/zone/ZoneId.java b/config-provisioning/src/main/java/com/yahoo/config/provision/zone/ZoneId.java index 7c5c15e23e6..b95c0cce149 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/zone/ZoneId.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/zone/ZoneId.java @@ -15,6 +15,17 @@ import java.util.Objects; */ public class ZoneId { + private static final ZoneId CONTROLLER = from(Environment.prod, RegionName.from("controller")); + + /** + * The ZoneId associated with the controller, distinct from all other zones in the system, but a constant across systems. + * + *

The controller may also be associated with a real zone, i.e. with a region defining the location like + * aws-us-east-1a. Because such a zone ID is different for different systems, and may clash with a prod zone in the + * same region and system, the virtual zone ID is often used.

+ */ + public static ZoneId ofVirtualControllerZone() { return CONTROLLER; } + private final Environment environment; private final RegionName region; diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ConfigServerFlagsTarget.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ConfigServerFlagsTarget.java index 5842ee3c3c0..585000cf22c 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ConfigServerFlagsTarget.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ConfigServerFlagsTarget.java @@ -1,9 +1,11 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.systemflags.v1; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.athenz.api.AthenzIdentity; +import com.yahoo.vespa.flags.json.FlagData; import java.net.URI; import java.util.List; @@ -20,12 +22,14 @@ import static com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget.z */ class ConfigServerFlagsTarget implements FlagsTarget { private final SystemName system; + private final CloudName cloud; private final ZoneId zone; private final URI endpoint; private final AthenzIdentity identity; - ConfigServerFlagsTarget(SystemName system, ZoneId zone, URI endpoint, AthenzIdentity identity) { + ConfigServerFlagsTarget(SystemName system, CloudName cloud, ZoneId zone, URI endpoint, AthenzIdentity identity) { this.system = Objects.requireNonNull(system); + this.cloud = Objects.requireNonNull(cloud); this.zone = Objects.requireNonNull(zone); this.endpoint = Objects.requireNonNull(endpoint); this.identity = Objects.requireNonNull(identity); @@ -36,16 +40,32 @@ class ConfigServerFlagsTarget implements FlagsTarget { @Override public Optional athenzHttpsIdentity() { return Optional.of(identity); } @Override public String asString() { return String.format("%s.%s", system.value(), zone.value()); } - @Override public boolean equals(Object o) { + @Override + public FlagData partiallyResolveFlagData(FlagData data) { + return FlagsTarget.partialResolve(data, system, cloud, zone); + } + + @Override + public String toString() { + return "ConfigServerFlagsTarget{" + + "system=" + system + + ", cloud=" + cloud + + ", zone=" + zone + + ", endpoint=" + endpoint + + ", identity=" + identity + + '}'; + } + + @Override + public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ConfigServerFlagsTarget that = (ConfigServerFlagsTarget) o; - return system == that.system && - Objects.equals(zone, that.zone) && - Objects.equals(endpoint, that.endpoint) && - Objects.equals(identity, that.identity); + return system == that.system && cloud.equals(that.cloud) && zone.equals(that.zone) && endpoint.equals(that.endpoint) && identity.equals(that.identity); } - @Override public int hashCode() { return Objects.hash(system, zone, endpoint, identity); } + @Override + public int hashCode() { + return Objects.hash(system, cloud, zone, endpoint, identity); + } } - diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ControllerFlagsTarget.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ControllerFlagsTarget.java index efeaf12de1c..043c6ea5963 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ControllerFlagsTarget.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/ControllerFlagsTarget.java @@ -1,8 +1,11 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.systemflags.v1; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.SystemName; +import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.vespa.athenz.api.AthenzIdentity; +import com.yahoo.vespa.flags.json.FlagData; import java.net.URI; import java.util.List; @@ -18,20 +21,44 @@ import static com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget.s */ class ControllerFlagsTarget implements FlagsTarget { private final SystemName system; + private final CloudName cloud; + private final ZoneId zone; - ControllerFlagsTarget(SystemName system) { this.system = Objects.requireNonNull(system); } + ControllerFlagsTarget(SystemName system, CloudName cloud, ZoneId zone) { + this.system = Objects.requireNonNull(system); + this.cloud = Objects.requireNonNull(cloud); + this.zone = Objects.requireNonNull(zone); + } @Override public List flagDataFilesPrioritized() { return List.of(controllerFile(system), systemFile(system), defaultFile()); } @Override public URI endpoint() { return URI.create("https://localhost:4443/"); } // Note: Cannot use VIPs for controllers due to network configuration on AWS @Override public Optional athenzHttpsIdentity() { return Optional.empty(); } @Override public String asString() { return String.format("%s.controller", system.value()); } - @Override public boolean equals(Object o) { + @Override + public FlagData partiallyResolveFlagData(FlagData data) { + return FlagsTarget.partialResolve(data, system, cloud, zone); + } + + @Override + public String toString() { + return "ControllerFlagsTarget{" + + "system=" + system + + ", cloud=" + cloud + + ", zone=" + zone + + '}'; + } + + @Override + public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; ControllerFlagsTarget that = (ControllerFlagsTarget) o; - return system == that.system; + return system == that.system && cloud.equals(that.cloud) && zone.equals(that.zone); } - @Override public int hashCode() { return Objects.hash(system); } + @Override + public int hashCode() { + return Objects.hash(system, cloud, zone); + } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTarget.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTarget.java index 1c8e68ff378..b42b22d3eff 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTarget.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTarget.java @@ -1,20 +1,31 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.systemflags.v1; +import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.ZoneApi; import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.config.provision.zone.ZoneList; import com.yahoo.vespa.athenz.api.AthenzIdentity; +import com.yahoo.vespa.flags.FetchVector; +import com.yahoo.vespa.flags.FlagDefinition; +import com.yahoo.vespa.flags.Flags; +import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import java.net.URI; +import java.util.EnumSet; import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; +import static com.yahoo.vespa.flags.FetchVector.Dimension.CLOUD; +import static com.yahoo.vespa.flags.FetchVector.Dimension.SYSTEM; +import static com.yahoo.vespa.flags.FetchVector.Dimension.ZONE_ID; + /** * Represents either configservers in a zone or controllers in a system. * @@ -38,24 +49,28 @@ public interface FlagsTarget { Optional athenzHttpsIdentity(); String asString(); + FlagData partiallyResolveFlagData(FlagData data); + static Set getAllTargetsInSystem(ZoneRegistry registry, boolean reachableOnly) { - SystemName system = registry.system(); Set targets = new HashSet<>(); ZoneList filteredZones = reachableOnly ? registry.zones().reachable() : registry.zones().all(); for (ZoneApi zone : filteredZones.zones()) { - targets.add(forConfigServer(registry, zone.getId())); + targets.add(forConfigServer(registry, zone)); } - targets.add(forController(system)); + targets.add(forController(registry.systemZone())); return targets; } - static FlagsTarget forController(SystemName systemName) { - return new ControllerFlagsTarget(systemName); + static FlagsTarget forController(ZoneApi controllerZone) { + return new ControllerFlagsTarget(controllerZone.getSystemName(), controllerZone.getCloudName(), controllerZone.getVirtualId()); } - static FlagsTarget forConfigServer(ZoneRegistry registry, ZoneId zoneId) { - return new ConfigServerFlagsTarget( - registry.system(), zoneId, registry.getConfigServerVipUri(zoneId), registry.getConfigServerHttpsIdentity(zoneId)); + static FlagsTarget forConfigServer(ZoneRegistry registry, ZoneApi zone) { + return new ConfigServerFlagsTarget(registry.system(), + zone.getCloudName(), + zone.getVirtualId(), + registry.getConfigServerVipUri(zone.getVirtualId()), + registry.getConfigServerHttpsIdentity(zone.getVirtualId())); } static String defaultFile() { return jsonFile("default"); } @@ -64,6 +79,24 @@ public interface FlagsTarget { static String zoneFile(SystemName system, ZoneId zone) { return jsonFile(system.value() + "." + zone.environment().value() + "." + zone.region().value()); } static String controllerFile(SystemName system) { return jsonFile(system.value() + ".controller"); } + /** Partially resolve the system, cloud, and zone dimensions, except those dimensions defined by the flag for a controller zone. */ + static FlagData partialResolve(FlagData data, SystemName system, CloudName cloud, ZoneId virtualZoneId) { + Set flagDimensions = + virtualZoneId.equals(ZoneId.ofVirtualControllerZone()) ? + Flags.getFlag(data.id()) + .map(FlagDefinition::getDimensions) + .map(Set::copyOf) + // E.g. testing: Assume unknown flag should resolve any and all dimensions below + .orElse(EnumSet.noneOf(FetchVector.Dimension.class)) : + EnumSet.noneOf(FetchVector.Dimension.class); + + var fetchVector = new FetchVector(); + if (!flagDimensions.contains(SYSTEM)) fetchVector = fetchVector.with(SYSTEM, system.value()); + if (!flagDimensions.contains(CLOUD)) fetchVector = fetchVector.with(CLOUD, cloud.value()); + if (!flagDimensions.contains(ZONE_ID)) fetchVector = fetchVector.with(ZONE_ID, virtualZoneId.value()); + return fetchVector.isEmpty() ? data : data.partialResolve(fetchVector); + } + private static String jsonFile(String nameWithoutExtension) { return nameWithoutExtension + ".json"; } } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java index 60950341a42..169387fd2ab 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java @@ -4,9 +4,16 @@ package com.yahoo.vespa.hosted.controller.api.systemflags.v1; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.CloudName; +import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.config.provision.HostName; import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.SystemName; +import com.yahoo.config.provision.TenantName; +import com.yahoo.config.provision.zone.ZoneApi; +import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.text.JSON; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; @@ -38,6 +45,9 @@ import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; +import static com.yahoo.config.provision.CloudName.AWS; +import static com.yahoo.config.provision.CloudName.GCP; +import static com.yahoo.config.provision.CloudName.YAHOO; import static com.yahoo.yolean.Exceptions.uncheck; /** @@ -189,7 +199,10 @@ public class SystemFlagsDataArchive { if (rawData.isBlank()) { flagData = new FlagData(directoryDeducedFlagId); } else { - String normalizedRawData = normalizeJson(rawData); + Set zones = systemDefinition == null ? + Set.of() : + systemDefinition.zones().all().zones().stream().map(ZoneApi::getVirtualId).collect(Collectors.toSet()); + String normalizedRawData = normalizeJson(rawData, zones); flagData = FlagData.deserialize(normalizedRawData); if (!directoryDeducedFlagId.equals(flagData.id())) { throw new IllegalArgumentException( @@ -217,41 +230,62 @@ public class SystemFlagsDataArchive { builder.addFile(filename, flagData); } - static String normalizeJson(String json) { + static String normalizeJson(String json, Set zones) { JsonNode root = uncheck(() -> mapper.readTree(json)); removeCommentsRecursively(root); - verifyValues(root); + verifyValues(root, zones); return root.toString(); } - private static void verifyValues(JsonNode root) { + private static void verifyValues(JsonNode root, Set zones) { var cursor = new JsonAccessor(root); cursor.get("rules").forEachArrayElement(rule -> rule.get("conditions").forEachArrayElement(condition -> { - var dimension = condition.get("dimension"); - if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.APPLICATION_ID))) { - condition.get("values").forEachArrayElement(conditionValue -> { - String applicationIdString = conditionValue.asString() - .orElseThrow(() -> new IllegalArgumentException("Non-string application ID: " + conditionValue)); - // Throws exception if not recognized - ApplicationId.fromSerializedForm(applicationIdString); + FetchVector.Dimension dimension = DimensionHelper + .fromWire(condition.get("dimension") + .asString() + .orElseThrow(() -> new IllegalArgumentException("Invalid dimension in condition: " + condition))); + switch (dimension) { + case APPLICATION_ID -> validateStringValues(condition, ApplicationId::fromSerializedForm); + case CONSOLE_USER_EMAIL -> validateStringValues(condition, email -> {}); + case CLOUD -> validateStringValues(condition, cloud -> { + if (!Set.of(YAHOO, AWS, GCP).contains(CloudName.from(cloud))) + throw new IllegalArgumentException("Unknown cloud: " + cloud); }); - } else if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.NODE_TYPE))) { - condition.get("values").forEachArrayElement(conditionValue -> { - String nodeTypeString = conditionValue.asString() - .orElseThrow(() -> new IllegalArgumentException("Non-string node type: " + conditionValue)); - // Throws exception if not recognized - NodeType.valueOf(nodeTypeString); + case CLUSTER_ID -> validateStringValues(condition, ClusterSpec.Id::from); + case CLUSTER_TYPE -> validateStringValues(condition, ClusterSpec.Type::from); + case HOSTNAME -> validateStringValues(condition, HostName::of); + case NODE_TYPE -> validateStringValues(condition, NodeType::valueOf); + case SYSTEM -> validateStringValues(condition, system -> { + if (!Set.of(SystemName.cd, SystemName.main, SystemName.PublicCd, SystemName.Public).contains(SystemName.from(system))) + throw new IllegalArgumentException("Unknown system: " + system); + }); + case TENANT_ID -> validateStringValues(condition, TenantName::from); + case VESPA_VERSION -> validateStringValues(condition, versionString -> { + Version vespaVersion = Version.fromString(versionString); + if (vespaVersion.getMajor() < 8) + throw new IllegalArgumentException("Major Vespa version must be at least 8: " + versionString); + }); + case ZONE_ID -> validateStringValues(condition, zoneId -> { + if (!zones.contains(ZoneId.from(zoneId))) + throw new IllegalArgumentException("Unknown zone: " + zoneId); }); - } else if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.CONSOLE_USER_EMAIL))) { - condition.get("values").forEachArrayElement(conditionValue -> conditionValue.asString() - .orElseThrow(() -> new IllegalArgumentException("Non-string email address: " + conditionValue))); - } else if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.TENANT_ID))) { - condition.get("values").forEachArrayElement(conditionValue -> conditionValue.asString() - .orElseThrow(() -> new IllegalArgumentException("Non-string tenant ID: " + conditionValue))); } })); } + private static void validateStringValues(JsonAccessor condition, Consumer valueValidator) { + condition.get("values").forEachArrayElement(conditionValue -> { + String value = conditionValue.asString() + .orElseThrow(() -> { + String dimension = condition.get("dimension").asString().orElseThrow(); + String type = condition.get("type").asString().orElseThrow(); + return new IllegalArgumentException("Non-string value in %s %s condition: %s".formatted( + dimension, type, conditionValue)); + }); + valueValidator.accept(value); + }); + } + private static void removeCommentsRecursively(JsonNode node) { if (node instanceof ObjectNode) { ObjectNode objectNode = (ObjectNode) node; diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java index d010893f1d4..b514ab8d3d7 100644 --- a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java +++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java @@ -54,15 +54,20 @@ public class SystemFlagsDataArchiveTest { @TempDir public File temporaryFolder; - private static final FlagsTarget mainControllerTarget = FlagsTarget.forController(SYSTEM); - private static final FlagsTarget cdControllerTarget = FlagsTarget.forController(SystemName.cd); + private static final FlagsTarget mainControllerTarget = createControllerTarget(SYSTEM); + private static final FlagsTarget cdControllerTarget = createControllerTarget(SystemName.cd); private static final FlagsTarget prodUsWestCfgTarget = createConfigserverTarget(Environment.prod, "us-west-1"); private static final FlagsTarget prodUsEast3CfgTarget = createConfigserverTarget(Environment.prod, "us-east-3"); private static final FlagsTarget devUsEast1CfgTarget = createConfigserverTarget(Environment.dev, "us-east-1"); + private static FlagsTarget createControllerTarget(SystemName system) { + return new ControllerFlagsTarget(system, CloudName.YAHOO, ZoneId.from(Environment.prod, RegionName.from("us-east-1"))); + } + private static FlagsTarget createConfigserverTarget(Environment environment, String region) { return new ConfigServerFlagsTarget( SYSTEM, + CloudName.YAHOO, ZoneId.from(environment, RegionName.from(region)), URI.create("https://cfg-" + region), new AthenzService("vespa.cfg-" + region)); @@ -177,102 +182,81 @@ public class SystemFlagsDataArchiveTest { " \"comment\": \"comment d\"\n" + " }\n" + " ]\n" + - "}"))); + "}", Set.of()))); } @Test - void normalize_json_fail_on_invalid_application() { - try { - SystemFlagsDataArchive.normalizeJson("{\n" + - " \"id\": \"foo\",\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"application\",\n" + - " \"values\": [ \"a.b.c\" ]\n" + - " }\n" + - " ],\n" + - " \"value\": true\n" + - " }\n" + - " ]\n" + - "}\n"); - fail(); - } catch (IllegalArgumentException e) { - assertEquals("Application ids must be on the form tenant:application:instance, but was a.b.c", e.getMessage()); - } + void normalize_json_succeed_on_valid_values() { + normalizeJson("application", "\"a:b:c\""); + normalizeJson("cloud", "\"yahoo\""); + normalizeJson("cloud", "\"aws\""); + normalizeJson("cloud", "\"gcp\""); + normalizeJson("cluster-id", "\"some-id\""); + normalizeJson("cluster-type", "\"admin\""); + normalizeJson("cluster-type", "\"container\""); + normalizeJson("cluster-type", "\"content\""); + normalizeJson("console-user-email", "\"name@domain.com\""); + normalizeJson("hostname", "\"2080046-v6-11.ostk.bm2.prod.gq1.yahoo.com\""); + normalizeJson("node-type", "\"tenant\""); + normalizeJson("node-type", "\"host\""); + normalizeJson("node-type", "\"config\""); + normalizeJson("node-type", "\"host\""); + normalizeJson("system", "\"main\""); + normalizeJson("system", "\"public\""); + normalizeJson("tenant", "\"vespa\""); + normalizeJson("vespa-version", "\"8.201.13\""); + normalizeJson("zone", "\"prod.us-west-1\"", Set.of(ZoneId.from("prod.us-west-1"))); } - @Test - void normalize_json_fail_on_invalid_node_type() { - try { - SystemFlagsDataArchive.normalizeJson("{\n" + - " \"id\": \"foo\",\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"node-type\",\n" + - " \"values\": [ \"footype\" ]\n" + - " }\n" + - " ],\n" + - " \"value\": true\n" + - " }\n" + - " ]\n" + - "}\n"); - fail(); - } catch (IllegalArgumentException e) { - assertEquals("No enum constant com.yahoo.config.provision.NodeType.footype", e.getMessage()); - } + private void normalizeJson(String dimension, String jsonValue) { + normalizeJson(dimension, jsonValue, Set.of()); } - @Test - void normalize_json_fail_on_invalid_email() { - try { - SystemFlagsDataArchive.normalizeJson("{\n" + - " \"id\": \"foo\",\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"console-user-email\",\n" + - " \"values\": [ 123 ]\n" + - " }\n" + - " ],\n" + - " \"value\": true\n" + - " }\n" + - " ]\n" + - "}\n"); - fail(); - } catch (IllegalArgumentException e) { - assertEquals("Non-string email address: 123", e.getMessage()); - } + private void normalizeJson(String dimension, String jsonValue, Set zones) { + SystemFlagsDataArchive.normalizeJson(""" + { + "id": "foo", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "%s", + "values": [ %s ] + } + ], + "value": true + } + ] + } + """.formatted(dimension, jsonValue), zones); } @Test - void normalize_json_fail_on_invalid_tenant_id() { + void normalize_json_fail_on_invalid_values() { + failNormalizeJson("application", "\"a.b.c\"", "Application ids must be on the form tenant:application:instance, but was a.b.c"); + failNormalizeJson("cloud", "\"foo\"", "Unknown cloud: foo"); + // failNormalizeJson("cluster-id", ... any String is valid + failNormalizeJson("cluster-type", "\"foo\"", "Illegal cluster type 'foo'"); + failNormalizeJson("console-user-email", "123", "Non-string value in console-user-email whitelist condition: 123"); + failNormalizeJson("hostname", "\"not:a:hostname\"", "hostname must match '(([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]{0,61}[A-Za-z0-9])\\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]{0,61}[A-Za-z0-9])\\.?', but got: 'not:a:hostname'"); + failNormalizeJson("node-type", "\"footype\"", "No enum constant com.yahoo.config.provision.NodeType.footype"); + failNormalizeJson("system", "\"bar\"", "'bar' is not a valid system"); + failNormalizeJson("tenant", "123", "Non-string value in tenant whitelist condition: 123"); + failNormalizeJson("vespa-version", "\"not-a-version\"", "For input string: \"not-a-version\""); + failNormalizeJson("zone", "\"dev.non-existing-zone\"", Set.of(ZoneId.from("prod.example-region")), "Unknown zone: dev.non-existing-zone"); + } + + private void failNormalizeJson(String dimension, String jsonValue, String expectedExceptionMessage) { + failNormalizeJson(dimension, jsonValue, Set.of(), expectedExceptionMessage); + } + + private void failNormalizeJson(String dimension, String jsonValue, Set zones, String expectedExceptionMessage) { try { - SystemFlagsDataArchive.normalizeJson("{\n" + - " \"id\": \"foo\",\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"tenant\",\n" + - " \"values\": [ 123 ]\n" + - " }\n" + - " ],\n" + - " \"value\": true\n" + - " }\n" + - " ]\n" + - "}\n"); + normalizeJson(dimension, jsonValue, zones); fail(); - } catch (IllegalArgumentException e) { - assertEquals("Non-string tenant ID: 123", e.getMessage()); + } catch (RuntimeException e) { + assertEquals(expectedExceptionMessage, e.getMessage()); } } @@ -291,6 +275,11 @@ public class SystemFlagsDataArchiveTest { // Cannot use the standard registry mock as it's located in controller-server module ZoneRegistry registryMock = mock(ZoneRegistry.class); when(registryMock.system()).thenReturn(SystemName.main); + ZoneApi zoneApi = mock(ZoneApi.class); + when(zoneApi.getSystemName()).thenReturn(SystemName.main); + when(zoneApi.getCloudName()).thenReturn(CloudName.YAHOO); + when(zoneApi.getVirtualId()).thenReturn(ZoneId.ofVirtualControllerZone()); + when(registryMock.systemZone()).thenReturn(zoneApi); when(registryMock.getConfigServerVipUri(any())).thenReturn(URI.create("http://localhost:8080/")); when(registryMock.getConfigServerHttpsIdentity(any())).thenReturn(new AthenzService("domain", "servicename")); ZoneList zoneListMock = mock(ZoneList.class); @@ -333,7 +322,7 @@ public class SystemFlagsDataArchiveTest { @Override public SystemName getSystemName() { return SystemName.main; } @Override public ZoneId getId() { return zoneId; } - @Override public CloudName getCloudName() { throw new UnsupportedOperationException(); } + @Override public CloudName getCloudName() { return CloudName.YAHOO; } @Override public String getCloudNativeRegionName() { throw new UnsupportedOperationException(); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java index abc888abccb..355f06fc753 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployer.java @@ -78,8 +78,12 @@ class SystemFlagsDeployer { return SystemFlagsDeployResult.merge(results); } - private SystemFlagsDeployResult deployFlags(FlagsTarget target, List flagData, boolean dryRun) { - Map wantedFlagData = lookupTable(flagData); + private SystemFlagsDeployResult deployFlags(FlagsTarget target, List flagDataList, boolean dryRun) { + flagDataList = flagDataList.stream() + .map(target::partiallyResolveFlagData) + .filter(flagData -> !flagData.isEmpty()) + .toList(); + Map wantedFlagData = lookupTable(flagDataList); Map currentFlagData; List definedFlags; try { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java index 6fd44e09d8d..21fe1f66bc5 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneApiMock.java @@ -36,8 +36,8 @@ public class ZoneApiMock implements ZoneApi { } } - public static ZoneApiMock fromId(String id) { - return from(ZoneId.from(id)); + public static ZoneApiMock fromId(String zoneId) { + return from(ZoneId.from(zoneId)); } public static ZoneApiMock from(Environment environment, RegionName region) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java index e6a9014df94..63d479d4c6c 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ZoneRegistryMock.java @@ -161,7 +161,7 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry @Override public ZoneApi systemZone() { - return ZoneApiMock.fromId("prod.controller"); + return ZoneApiMock.newBuilder().withSystem(system).withVirtualId(ZoneId.ofVirtualControllerZone()).build(); } @Override @@ -180,7 +180,7 @@ public class ZoneRegistryMock extends AbstractComponent implements ZoneRegistry private ZoneApiMock systemAsZone() { return ZoneApiMock.newBuilder() .with(ZoneId.from("prod.us-east-1")) - .withVirtualId(ZoneId.from("prod.controller")) + .withVirtualId(ZoneId.ofVirtualControllerZone()) .build(); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java index 36679e0dd91..d0d362abcfc 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResultTest.java @@ -20,9 +20,12 @@ import static org.assertj.core.api.Assertions.assertThat; * @author bjorncs */ public class SystemFlagsDeployResultTest { + private final ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1"); + private final ZoneRegistryMock registry = new ZoneRegistryMock(SystemName.cd).setZones(prodUsWest1Zone); + @Test void changes_and_errors_are_present_in_wire_format() { - FlagsTarget controllerTarget = FlagsTarget.forController(SystemName.cd); + FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone()); FlagId flagOne = new FlagId("flagone"); FlagId flagTwo = new FlagId("flagtwo"); SystemFlagsDeployResult result = new SystemFlagsDeployResult( @@ -41,10 +44,8 @@ public class SystemFlagsDeployResultTest { @Test void identical_errors_and_changes_from_multiple_targets_are_merged() { - ZoneApiMock prodUsWest1Zone = ZoneApiMock.fromId("prod.us-west-1"); - ZoneRegistryMock registry = new ZoneRegistryMock(SystemName.cd).setZones(prodUsWest1Zone); - FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId()); - FlagsTarget controllerTarget = FlagsTarget.forController(SystemName.cd); + FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone); + FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone()); FlagId flagOne = new FlagId("flagone"); FlagId flagTwo = new FlagId("flagtwo"); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java index 50354639f6f..14e37853191 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployerTest.java @@ -2,7 +2,9 @@ package com.yahoo.vespa.hosted.controller.restapi.systemflags; import com.yahoo.config.provision.SystemName; +import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.SystemFlagsDataArchive; @@ -15,11 +17,16 @@ import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Paths; import java.util.List; +import java.util.Optional; import java.util.Set; import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.FlagDataChange; import static com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError; +import static com.yahoo.yolean.Exceptions.uncheck; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -38,12 +45,12 @@ public class SystemFlagsDeployerTest { private final ZoneApiMock prodUsEast3Zone = ZoneApiMock.fromId("prod.us-east-3"); private final ZoneRegistryMock registry = new ZoneRegistryMock(SYSTEM).setZones(prodUsWest1Zone, prodUsEast3Zone); - private final FlagsTarget controllerTarget = FlagsTarget.forController(SYSTEM); - private final FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone.getId()); - private final FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone.getId()); + private final FlagsTarget controllerTarget = FlagsTarget.forController(registry.systemZone()); + private final FlagsTarget prodUsWest1Target = FlagsTarget.forConfigServer(registry, prodUsWest1Zone); + private final FlagsTarget prodUsEast3Target = FlagsTarget.forConfigServer(registry, prodUsEast3Zone); @Test - void deploys_flag_data_to_targets() throws IOException { + void deploys_flag_data_to_targets() { FlagsClient flagsClient = mock(FlagsClient.class); when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of()); when(flagsClient.listFlagData(prodUsWest1Target)).thenReturn(List.of(flagData("existing-prod.us-west-1.json"))); @@ -74,7 +81,81 @@ public class SystemFlagsDeployerTest { } @Test - void dryrun_should_not_change_flags() throws IOException { + void deploys_partial_flag_data_to_targets() { + // default.json contains one rule with 2 conditions, one of which has a condition on the aws cloud. + // This condition IS resolved for a config server target, but NOT for a controller target, because FLAG_ID + // has the CLOUD dimension set. + deployFlags(Optional.empty(), "partial/default.json", Optional.of("partial/put-controller.json"), true, PutType.CREATE, FetchVector.Dimension.CLOUD); + deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), false, PutType.NONE, FetchVector.Dimension.CLOUD); + deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.of("partial/put-controller-2.json"), true, PutType.UPDATE, FetchVector.Dimension.CLOUD); + deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), false, PutType.DELETE, FetchVector.Dimension.CLOUD); + + // When the CLOUD dimension is NOT set on the dimension, the controller target will also resolve that dimension, and + // the result should be identical to the config server target. Let's also verify the config server target is unchanged. + deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), true, PutType.NONE); + deployFlags(Optional.empty(), "partial/default.json", Optional.empty(), false, PutType.NONE); + deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), true, PutType.DELETE); + deployFlags(Optional.of("partial/initial.json"), "partial/default.json", Optional.empty(), false, PutType.DELETE); + } + + private enum PutType { + CREATE, + UPDATE, + DELETE, + NONE + } + + /** + * @param existingFlagDataPath path to flag data the target already has + * @param defaultFlagDataPath path to default json file + * @param putFlagDataPath path to flag data pushed to target, or empty if nothing should be pushed + * @param controller whether to target the controller, or config server + */ + private void deployFlags(Optional existingFlagDataPath, + String defaultFlagDataPath, + Optional putFlagDataPath, + boolean controller, + PutType putType, + FetchVector.Dimension... flagDimensions) { + List existingFlagData = existingFlagDataPath.map(SystemFlagsDeployerTest::flagData).map(List::of).orElse(List.of()); + FlagData defaultFlagData = flagData(defaultFlagDataPath); + FlagsTarget target = controller ? controllerTarget : prodUsWest1Target; + Optional putFlagData = putFlagDataPath.map(SystemFlagsDeployerTest::flagData); + + try (var replacer = Flags.clearFlagsForTesting()) { + Flags.defineStringFlag(FLAG_ID.toString(), "default", List.of("hakonhall"), "2023-07-27", "2123-07-27", "", "", flagDimensions); + + FlagsClient flagsClient = mock(FlagsClient.class); + when(flagsClient.listFlagData(target)).thenReturn(existingFlagData); + + SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder() + .addFile("default.json", defaultFlagData) + .build(); + + SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(target)); + + List changes = deployer.deployFlags(archive, false).flagChanges(); + + putFlagData.ifPresentOrElse(flagData -> { + verify(flagsClient).putFlagData(target, flagData); + switch (putType) { + case CREATE -> assertThat(changes).containsOnly(FlagDataChange.created(FLAG_ID, target, flagData)); + case UPDATE -> assertThat(changes).containsOnly(FlagDataChange.updated(FLAG_ID, target, flagData, existingFlagData.get(0))); + case DELETE, NONE -> throw new IllegalStateException("Flag data put to the target, but change type is " + putType); + } + }, () -> { + verify(flagsClient, never()).putFlagData(eq(target), any()); + switch (putType) { + case DELETE -> assertThat(changes).containsOnly(FlagDataChange.deleted(FLAG_ID, target)); + case NONE -> assertEquals(changes, List.of()); + default -> throw new IllegalStateException("No flag data is expected to be put to the target but change type is " + putType); + } + }); + } + } + + @Test + void dryrun_should_not_change_flags() { FlagsClient flagsClient = mock(FlagsClient.class); when(flagsClient.listFlagData(controllerTarget)).thenReturn(List.of()); when(flagsClient.listDefinedFlags(controllerTarget)).thenReturn(List.of(new FlagId("my-flag"))); @@ -97,7 +178,7 @@ public class SystemFlagsDeployerTest { } @Test - void creates_error_entries_in_result_if_flag_data_operations_fail() throws IOException { + void creates_error_entries_in_result_if_flag_data_operations_fail() { FlagsClient flagsClient = mock(FlagsClient.class); UncheckedIOException exception = new UncheckedIOException(new IOException("I/O error message")); when(flagsClient.listFlagData(prodUsWest1Target)).thenThrow(exception); @@ -120,7 +201,7 @@ public class SystemFlagsDeployerTest { } @Test - void creates_error_entry_for_invalid_flag_archive() throws IOException { + void creates_error_entry_for_invalid_flag_archive() { FlagsClient flagsClient = mock(FlagsClient.class); FlagData defaultData = flagData("flags/my-flag/main.json"); SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder() @@ -135,7 +216,7 @@ public class SystemFlagsDeployerTest { } @Test - void creates_error_entry_for_flag_data_of_undefined_flag() throws IOException { + void creates_error_entry_for_flag_data_of_undefined_flag() { FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json"); FlagsClient flagsClient = mock(FlagsClient.class); when(flagsClient.listFlagData(prodUsEast3Target)) @@ -154,7 +235,7 @@ public class SystemFlagsDeployerTest { } @Test - void creates_warning_entry_for_existing_flag_data_for_undefined_flag() throws IOException { + void creates_warning_entry_for_existing_flag_data_for_undefined_flag() { FlagData prodUsEast3Data = flagData("flags/my-flag/main.prod.us-east-3.json"); FlagsClient flagsClient = mock(FlagsClient.class); when(flagsClient.listFlagData(prodUsEast3Target)) @@ -170,8 +251,8 @@ public class SystemFlagsDeployerTest { .containsOnly(OperationError.dataForUndefinedFlag(prodUsEast3Target, new FlagId("my-flag"))); } - private static FlagData flagData(String filename) throws IOException { - return FlagData.deserializeUtf8Json(Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename))); + private static FlagData flagData(String filename) { + return FlagData.deserializeUtf8Json(uncheck(() -> Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename)))); } } diff --git a/controller-server/src/test/resources/system-flags/partial/default.json b/controller-server/src/test/resources/system-flags/partial/default.json new file mode 100644 index 00000000000..881d4170c3b --- /dev/null +++ b/controller-server/src/test/resources/system-flags/partial/default.json @@ -0,0 +1,20 @@ +{ + "id" : "my-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "system", + "values": [ "main" ] + }, + { + "type": "whitelist", + "dimension": "cloud", + "values": [ "aws" ] + } + ], + "value" : "foo-value" + } + ] +} \ No newline at end of file diff --git a/controller-server/src/test/resources/system-flags/partial/initial.json b/controller-server/src/test/resources/system-flags/partial/initial.json new file mode 100644 index 00000000000..a16ea583005 --- /dev/null +++ b/controller-server/src/test/resources/system-flags/partial/initial.json @@ -0,0 +1,15 @@ +{ + "id" : "my-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": [ "a:b:c" ] + } + ], + "value" : "bar-value" + } + ] +} \ No newline at end of file diff --git a/controller-server/src/test/resources/system-flags/partial/put-controller-2.json b/controller-server/src/test/resources/system-flags/partial/put-controller-2.json new file mode 100644 index 00000000000..47aa0af47ce --- /dev/null +++ b/controller-server/src/test/resources/system-flags/partial/put-controller-2.json @@ -0,0 +1,15 @@ +{ + "id" : "my-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "cloud", + "values": [ "aws" ] + } + ], + "value" : "foo-value" + } + ] +} \ No newline at end of file diff --git a/controller-server/src/test/resources/system-flags/partial/put-controller.json b/controller-server/src/test/resources/system-flags/partial/put-controller.json new file mode 100644 index 00000000000..47aa0af47ce --- /dev/null +++ b/controller-server/src/test/resources/system-flags/partial/put-controller.json @@ -0,0 +1,15 @@ +{ + "id" : "my-flag", + "rules" : [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "cloud", + "values": [ "aws" ] + } + ], + "value" : "foo-value" + } + ] +} \ No newline at end of file diff --git a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java index c1877373ce2..8dff69b5a60 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java @@ -3,10 +3,13 @@ package com.yahoo.vespa.flags; import com.yahoo.vespa.flags.json.DimensionHelper; +import java.util.Collection; import java.util.EnumMap; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; +import java.util.function.BiConsumer; import java.util.function.Consumer; /** @@ -20,20 +23,25 @@ public class FetchVector { * Note: If this enum is changed, you must also change {@link DimensionHelper}. */ public enum Dimension { - /** Value from TenantName::value, e.g. vespa-team */ - TENANT_ID, - /** Value from ApplicationId::serializedForm of the form tenant:applicationName:instance. */ APPLICATION_ID, - /** Node type from com.yahoo.config.provision.NodeType::name, e.g. tenant, host, confighost, controller, etc. */ - NODE_TYPE, + /** + * Cloud from com.yahoo.config.provision.CloudName::value, e.g. yahoo, aws, gcp. + * + *

Eager resolution: This dimension is resolved before putting the flag data to the config server + * or controller, unless controller and the flag has declared this dimension. + */ + CLOUD, + + /** Cluster ID from com.yahoo.config.provision.ClusterSpec.Id::value, e.g. cluster-controllers, logserver. */ + CLUSTER_ID, /** Cluster type from com.yahoo.config.provision.ClusterSpec.Type::name, e.g. content, container, admin */ CLUSTER_TYPE, - /** Cluster ID from com.yahoo.config.provision.ClusterSpec.Id::value, e.g. cluster-controllers, logserver. */ - CLUSTER_ID, + /** Email address of user - provided by auth0 in console. */ + CONSOLE_USER_EMAIL, /** * Fully qualified hostname. @@ -44,6 +52,18 @@ public class FetchVector { */ HOSTNAME, + /** Node type from com.yahoo.config.provision.NodeType::name, e.g. tenant, host, confighost, controller, etc. */ + NODE_TYPE, + + /** + * Hosted Vespa system from com.yahoo.config.provision.SystemName::value, e.g. main, cd, public, publiccd. + * Eager resolution, see {@link #CLOUD}. + */ + SYSTEM, + + /** Value from TenantName::value, e.g. vespa-team */ + TENANT_ID, + /** * Vespa version from Version::toFullString of the form Major.Minor.Micro. * @@ -53,14 +73,9 @@ public class FetchVector { */ VESPA_VERSION, - /** Email address of user - provided by auth0 in console. */ - CONSOLE_USER_EMAIL, - /** - * Zone from ZoneId::value of the form environment.region. - * - *

NOTE: There is seldom any need to set ZONE_ID, as all flags are set per zone anyways. The controller - * could PERHAPS use this where it handles multiple zones. + * Virtual zone ID from com.yahoo.config.provision.zone.ZoneId::value of the form environment.region, + * see com.yahoo.config.provision.zone.ZoneApi::getVirtualId. Eager resolution, see {@link #CLOUD}. */ ZONE_ID } @@ -83,15 +98,13 @@ public class FetchVector { return Optional.ofNullable(map.get(dimension)); } - public Map toMap() { - return map; - } + public Map toMap() { return map; } public boolean isEmpty() { return map.isEmpty(); } - public boolean hasDimension(FetchVector.Dimension dimension) { - return map.containsKey(dimension); - } + public boolean hasDimension(FetchVector.Dimension dimension) { return map.containsKey(dimension);} + + public Set dimensions() { return map.keySet(); } /** * Returns a new FetchVector, identical to {@code this} except for its value in {@code dimension}. @@ -107,13 +120,28 @@ public class FetchVector { return makeFetchVector(vector -> vector.putAll(override.map)); } - private FetchVector makeFetchVector(Consumer> mapModifier) { - EnumMap mergedMap = new EnumMap<>(Dimension.class); + private FetchVector makeFetchVector(Consumer> mapModifier) { + Map mergedMap = new EnumMap<>(Dimension.class); mergedMap.putAll(map); mapModifier.accept(mergedMap); return new FetchVector(mergedMap); } + public FetchVector without(Dimension dimension) { + return makeFetchVector(merged -> merged.remove(dimension)); + } + + public FetchVector without(Collection dimensions) { + return makeFetchVector(merged -> merged.keySet().removeAll(dimensions)); + } + + @Override + public String toString() { + return "FetchVector{" + + "map=" + map + + '}'; + } + @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index a3402a55bea..8ee6584f93a 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -526,6 +526,15 @@ public class Flags { * For instance, if APPLICATION is one of the dimensions here, you should make sure * APPLICATION is set to the ApplicationId in the fetch vector when fetching the RawFlag * from the FlagSource. + * SYSTEM, CLOUD, and ZONE_ID are special: These dimensions are resolved just before the + * flag data is published to a zone. This means there is never any need to set these + * dimensions when resolving a flag, and setting these dimensions just before resolving + * the flag will have no effect. + * There is one exception. If any of these dimensions are declared when defining a flag, + * then those dimensions are NOT resolved when published to the controllers. This allows + * the controller to resolve the flag to different values based on which cloud or zone + * it is operating on. Flags should NOT declare these dimensions unless they intend to + * use them in the controller in this way. * @param The boxed type of the flag value, e.g. Boolean for flags guarding features. * @param The type of the unbound flag, e.g. UnboundBooleanFlag. * @return An unbound flag with {@link FetchVector.Dimension#HOSTNAME HOSTNAME} and diff --git a/flags/src/main/java/com/yahoo/vespa/flags/JsonNodeRawFlag.java b/flags/src/main/java/com/yahoo/vespa/flags/JsonNodeRawFlag.java index 753f19a44f6..27852790186 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/JsonNodeRawFlag.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/JsonNodeRawFlag.java @@ -6,6 +6,7 @@ import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import java.util.Collection; +import java.util.Objects; import java.util.concurrent.atomic.AtomicReference; import static com.yahoo.yolean.Exceptions.uncheck; @@ -60,6 +61,26 @@ public class JsonNodeRawFlag implements RawFlag { return jsonNode.toString(); } + @Override + public String toString() { + return "JsonNodeRawFlag{" + + "jsonNode=" + jsonNode + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + JsonNodeRawFlag that = (JsonNodeRawFlag) o; + return jsonNode.equals(that.jsonNode); + } + + @Override + public int hashCode() { + return Objects.hash(jsonNode); + } + /** Initialize object mapper lazily */ private static ObjectMapper objectMapper() { // ObjectMapper is a heavy-weight object so we construct it only when we need it diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java index ad1242aa7e9..9516c12693d 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/DimensionHelper.java @@ -15,15 +15,17 @@ public class DimensionHelper { private static final Map serializedDimensions = new HashMap<>(); static { - serializedDimensions.put(FetchVector.Dimension.ZONE_ID, "zone"); - serializedDimensions.put(FetchVector.Dimension.HOSTNAME, "hostname"); serializedDimensions.put(FetchVector.Dimension.APPLICATION_ID, "application"); - serializedDimensions.put(FetchVector.Dimension.NODE_TYPE, "node-type"); + serializedDimensions.put(FetchVector.Dimension.CLOUD, "cloud"); serializedDimensions.put(FetchVector.Dimension.CLUSTER_ID, "cluster-id"); serializedDimensions.put(FetchVector.Dimension.CLUSTER_TYPE, "cluster-type"); - serializedDimensions.put(FetchVector.Dimension.VESPA_VERSION, "vespa-version"); serializedDimensions.put(FetchVector.Dimension.CONSOLE_USER_EMAIL, "console-user-email"); + serializedDimensions.put(FetchVector.Dimension.HOSTNAME, "hostname"); + serializedDimensions.put(FetchVector.Dimension.NODE_TYPE, "node-type"); + serializedDimensions.put(FetchVector.Dimension.SYSTEM, "system"); serializedDimensions.put(FetchVector.Dimension.TENANT_ID, "tenant"); + serializedDimensions.put(FetchVector.Dimension.VESPA_VERSION, "vespa-version"); + serializedDimensions.put(FetchVector.Dimension.ZONE_ID, "zone"); if (serializedDimensions.size() != FetchVector.Dimension.values().length) { throw new IllegalStateException(FetchVectorHelper.class.getName() + " is not in sync with " + diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java b/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java index 19837e7dbe1..acda3b9db42 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/FlagData.java @@ -13,9 +13,10 @@ import com.yahoo.vespa.flags.json.wire.WireRule; import java.io.InputStream; import java.io.OutputStream; +import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Optional; -import java.util.stream.Collectors; import java.util.stream.Stream; /** @@ -53,6 +54,27 @@ public class FlagData { public boolean isEmpty() { return rules.isEmpty() && defaultFetchVector.isEmpty(); } + public FlagData partialResolve(FetchVector fetchVector) { + // Note: As a result of partialResolve, there could be e.g. two identical rules, and the latter will always be ignored by resolve(). + // Consider deduping. Deduping is actually not specific to partialResolve and could be done e.g. at construction time. + + List newRules = new ArrayList<>(); + for (var rule : rules) { + Optional partialRule = rule.partialResolve(fetchVector); + if (partialRule.isPresent()) { + newRules.add(partialRule.get()); + if (partialRule.get().conditions().isEmpty()) { + // Any following rule will always be ignored during resolution. + break; + } + } + } + + FetchVector newDefaultFetchVector = defaultFetchVector.without(fetchVector.dimensions()); + + return new FlagData(id, newDefaultFetchVector, newRules); + } + public Optional resolve(FetchVector fetchVector) { return rules.stream() .filter(rule -> rule.match(defaultFetchVector.with(fetchVector))) @@ -91,6 +113,36 @@ public class FlagData { return wireFlagData; } + /** E.g. verify all RawFlag can be deserialized. */ + public void validate(Deserializer deserializer) { + rules.stream() + .flatMap(rule -> rule.getValueToApply().map(Stream::of).orElse(null)) + .forEach(deserializer::deserialize); + + } + + @Override + public String toString() { + return "FlagData{" + + "id=" + id + + ", rules=" + rules + + ", defaultFetchVector=" + defaultFetchVector + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + FlagData flagData = (FlagData) o; + return id.equals(flagData.id) && rules.equals(flagData.rules) && defaultFetchVector.equals(flagData.defaultFetchVector); + } + + @Override + public int hashCode() { + return Objects.hash(id, rules, defaultFetchVector); + } + public static FlagData deserializeUtf8Json(byte[] bytes) { return fromWire(WireFlagData.deserialize(bytes)); } @@ -138,13 +190,5 @@ public class FlagData { if (wireRules == null) return List.of(); return wireRules.stream().map(Rule::fromWire).toList(); } - - /** E.g. verify all RawFlag can be deserialized. */ - public void validate(Deserializer deserializer) { - rules.stream() - .flatMap(rule -> rule.getValueToApply().map(Stream::of).orElse(null)) - .forEach(deserializer::deserialize); - - } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java index c4b2d9be117..483f6750a73 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/ListCondition.java @@ -5,6 +5,7 @@ import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.json.wire.WireCondition; import java.util.List; +import java.util.Objects; /** * @author hakonhall @@ -55,4 +56,27 @@ public abstract class ListCondition implements Condition { condition.values = values.isEmpty() ? null : values; return condition; } + + @Override + public String toString() { + return "ListCondition{" + + "type=" + type + + ", dimension=" + dimension + + ", values=" + values + + ", isWhitelist=" + isWhitelist + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ListCondition that = (ListCondition) o; + return isWhitelist == that.isWhitelist && type == that.type && dimension == that.dimension && values.equals(that.values); + } + + @Override + public int hashCode() { + return Objects.hash(type, dimension, values, isWhitelist); + } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java index 0efeb831f2c..749f6830870 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java @@ -5,6 +5,7 @@ import com.yahoo.component.Version; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.json.wire.WireCondition; +import java.util.Objects; import java.util.function.Predicate; /** @@ -75,4 +76,26 @@ public class RelationalCondition implements Condition { condition.predicate = relationalPredicate.toWire(); return condition; } + + @Override + public String toString() { + return "RelationalCondition{" + + "relationalPredicate=" + relationalPredicate + + ", predicate=" + predicate + + ", dimension=" + dimension + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + RelationalCondition that = (RelationalCondition) o; + return relationalPredicate.equals(that.relationalPredicate) && predicate.equals(that.predicate) && dimension == that.dimension; + } + + @Override + public int hashCode() { + return Objects.hash(relationalPredicate, predicate, dimension); + } } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java b/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java index bddaf8c9e0e..127c2b4f9da 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/Rule.java @@ -6,10 +6,11 @@ import com.yahoo.vespa.flags.JsonNodeRawFlag; import com.yahoo.vespa.flags.RawFlag; import com.yahoo.vespa.flags.json.wire.WireRule; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.Optional; -import java.util.stream.Collectors; /** * @author hakonhall @@ -45,6 +46,25 @@ public class Rule { .allMatch(condition -> !fetchVector.hasDimension(condition.dimension()) || condition.test(fetchVector)); } + /** + * Returns a copy of this rule without those conditions that can be resolved by the fetch vector. Returns empty + * if any of those conditions are false. + */ + public Optional partialResolve(FetchVector fetchVector) { + List newConditions = new ArrayList<>(); + for (var condition : andConditions) { + if (fetchVector.hasDimension(condition.dimension())) { + if (!condition.test(fetchVector)) { + return Optional.empty(); + } + } else { + newConditions.add(condition); + } + } + + return Optional.of(new Rule(valueToApply, newConditions)); + } + public Optional getValueToApply() { return valueToApply; } @@ -68,4 +88,25 @@ public class Rule { Optional value = wireRule.value == null ? Optional.empty() : Optional.of(JsonNodeRawFlag.fromJsonNode(wireRule.value)); return new Rule(value, conditions); } + + @Override + public String toString() { + return "Rule{" + + "andConditions=" + andConditions + + ", valueToApply=" + valueToApply + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Rule rule = (Rule) o; + return andConditions.equals(rule.andConditions) && valueToApply.equals(rule.valueToApply); + } + + @Override + public int hashCode() { + return Objects.hash(andConditions, valueToApply); + } } diff --git a/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java b/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java index c89b5883fd1..c7da1abe7e2 100644 --- a/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java +++ b/flags/src/test/java/com/yahoo/vespa/flags/json/FlagDataTest.java @@ -15,44 +15,45 @@ import static org.junit.jupiter.api.Assertions.assertTrue; * @author hakonhall */ public class FlagDataTest { - private final String json = "{\n" + - " \"id\": \"id1\",\n" + - " \"rules\": [\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"hostname\",\n" + - " \"values\": [ \"host1\", \"host2\" ]\n" + - " },\n" + - " {\n" + - " \"type\": \"blacklist\",\n" + - " \"dimension\": \"application\",\n" + - " \"values\": [ \"app1\", \"app2\" ]\n" + - " }\n" + - " ],\n" + - " \"value\": true\n" + - " },\n" + - " {\n" + - " \"conditions\": [\n" + - " {\n" + - " \"type\": \"whitelist\",\n" + - " \"dimension\": \"zone\",\n" + - " \"values\": [ \"zone1\", \"zone2\" ]\n" + - " }\n" + - " ],\n" + - " \"value\": false\n" + - " }\n" + - " ],\n" + - " \"attributes\": {\n" + - " \"zone\": \"zone1\"\n" + - " }\n" + - "}"; + private final String json = """ + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": [ "host1", "host2" ] + }, + { + "type": "blacklist", + "dimension": "application", + "values": [ "app1", "app2" ] + } + ], + "value": true + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": false + } + ], + "attributes": { + "zone": "zone1" + } + }"""; private final FetchVector vector = new FetchVector(); @Test - void test() { + void testResolve() { // Second rule matches with the default zone matching verify(Optional.of("false"), vector); @@ -74,6 +75,143 @@ public class FlagDataTest { verify(Optional.empty(), vector.with(FetchVector.Dimension.ZONE_ID, "unknown zone")); } + @Test + void testPartialResolve() { + FlagData data = FlagData.deserialize(json); + assertEquals(data.partialResolve(vector), data); + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app1")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": false + } + ], + "attributes": { + "zone": "zone1" + } + }""")); + + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app1")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": false + } + ], + "attributes": { + "zone": "zone1" + } + }""")); + + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app3")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": [ "host1", "host2" ] + } + ], + "value": true + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": false + } + ], + "attributes": { + "zone": "zone1" + } + }""")); + + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app3") + .with(FetchVector.Dimension.HOSTNAME, "host1")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "value": true + } + ], + "attributes": { + "zone": "zone1" + } + }""")); + + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app3") + .with(FetchVector.Dimension.HOSTNAME, "host3")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "conditions": [ + { + "type": "whitelist", + "dimension": "zone", + "values": [ "zone1", "zone2" ] + } + ], + "value": false + } + ], + "attributes": { + "zone": "zone1" + } + }""")); + + assertEquals(data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app3") + .with(FetchVector.Dimension.HOSTNAME, "host3") + .with(FetchVector.Dimension.ZONE_ID, "zone2")), + FlagData.deserialize(""" + { + "id": "id1", + "rules": [ + { + "value": false + } + ] + }""")); + + FlagData fullyResolved = data.partialResolve(vector.with(FetchVector.Dimension.APPLICATION_ID, "app3") + .with(FetchVector.Dimension.HOSTNAME, "host3") + .with(FetchVector.Dimension.ZONE_ID, "zone3")); + assertEquals(fullyResolved, FlagData.deserialize(""" + { + "id": "id1" + }""")); + assertTrue(fullyResolved.isEmpty()); + } + private void verify(Optional expectedValue, FetchVector vector) { FlagData data = FlagData.deserialize(json); assertEquals("id1", data.id().toString()); -- cgit v1.2.3