summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorValerij Fredriksen <freva@users.noreply.github.com>2023-08-10 18:37:47 +0200
committerGitHub <noreply@github.com>2023-08-10 18:37:47 +0200
commit4bdf3863de09491ca87e9a42db9a1f118f4e0114 (patch)
treee7f7db875bcee4f1d46c8203c12be565445ae011
parentdc46fc272ddb75f97357ad5602e6217821310ea7 (diff)
parent2444acd9e9fedaa3ea22feca4855135be6a4bed3 (diff)
Merge pull request #28020 from vespa-engine/hakonhall/fix-checking-of-zones
Fix checking of zones
-rw-r--r--controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTarget.java2
-rw-r--r--controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java273
-rw-r--r--controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java174
-rw-r--r--flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java4
4 files changed, 239 insertions, 214 deletions
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 7403f0a1b01..fbf3a5d9a03 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
@@ -142,7 +142,7 @@ public interface FlagsTarget {
var fetchVector = new FetchVector();
if (!flagDimensions.contains(CLOUD)) fetchVector = fetchVector.with(CLOUD, cloud.value());
if (!flagDimensions.contains(ENVIRONMENT)) fetchVector = fetchVector.with(ENVIRONMENT, virtualZoneId.environment().value());
- if (!flagDimensions.contains(SYSTEM)) fetchVector = fetchVector.with(SYSTEM, system.value());
+ fetchVector = fetchVector.with(SYSTEM, system.value());
if (!flagDimensions.contains(ZONE_ID)) fetchVector = fetchVector.with(ZONE_ID, virtualZoneId.value());
return fetchVector.isEmpty() ? data : data.partialResolve(fetchVector);
}
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 577769baf1e..c6f1d96ed43 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
@@ -1,6 +1,7 @@
// 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.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
@@ -18,8 +19,10 @@ import com.yahoo.config.provision.zone.ZoneId;
import com.yahoo.text.JSON;
import com.yahoo.vespa.flags.FetchVector;
import com.yahoo.vespa.flags.FlagId;
+import com.yahoo.vespa.flags.json.Condition;
import com.yahoo.vespa.flags.json.DimensionHelper;
import com.yahoo.vespa.flags.json.FlagData;
+import com.yahoo.vespa.flags.json.RelationalCondition;
import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
import java.io.BufferedInputStream;
@@ -49,6 +52,7 @@ 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.vespa.flags.FetchVector.Dimension.SYSTEM;
import static com.yahoo.yolean.Exceptions.uncheck;
/**
@@ -82,8 +86,8 @@ public class SystemFlagsDataArchive {
String name = entry.getName();
if (!entry.isDirectory() && name.startsWith("flags/")) {
Path filePath = Paths.get(name);
- String rawData = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8);
- addFile(builder, rawData, filePath, zoneRegistry, true);
+ String fileContent = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8);
+ builder.maybeAddFile(filePath, fileContent, zoneRegistry, true);
}
}
return builder.build();
@@ -92,7 +96,7 @@ public class SystemFlagsDataArchive {
}
}
- public static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry zoneRegistry, boolean forceAddFiles) {
+ public static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry zoneRegistry, boolean simulateInController) {
Path root = directory.toAbsolutePath();
Path flagsDirectory = directory.resolve("flags");
if (!Files.isDirectory(flagsDirectory)) {
@@ -103,8 +107,8 @@ public class SystemFlagsDataArchive {
directoryStream.forEach(path -> {
Path relativePath = root.relativize(path.toAbsolutePath());
if (Files.isRegularFile(path)) {
- String rawData = uncheck(() -> Files.readString(path, StandardCharsets.UTF_8));
- addFile(builder, rawData, relativePath, zoneRegistry, forceAddFiles);
+ String fileContent = uncheck(() -> Files.readString(path, StandardCharsets.UTF_8));
+ builder.maybeAddFile(relativePath, fileContent, zoneRegistry, simulateInController);
}
});
return builder.build();
@@ -168,114 +172,119 @@ public class SystemFlagsDataArchive {
return files.getOrDefault(flagId, Map.of()).containsKey(filename);
}
- private static void addFile(Builder builder, String rawData, Path filePath, ZoneRegistry zoneRegistry, boolean force) {
- String filename = filePath.getFileName().toString();
-
- if (filename.startsWith("."))
- return; // Ignore files starting with '.'
-
- if (!force && !FlagsTarget.filenameForSystem(filename, zoneRegistry.system()))
- return; // Ignore files for other systems
-
- FlagId directoryDeducedFlagId = new FlagId(filePath.getName(filePath.getNameCount()-2).toString());
- FlagData flagData;
- if (rawData.isBlank()) {
- flagData = new FlagData(directoryDeducedFlagId);
- } else {
- Set<ZoneId> zones = force ? Stream.concat(Stream.of(ZoneId.ofVirtualControllerZone()),
- zoneRegistry.zones().all().zones().stream().map(ZoneApi::getVirtualId))
- .collect(Collectors.toSet())
- : Set.of();
- String normalizedRawData = normalizeJson(rawData, zones);
- flagData = FlagData.deserialize(normalizedRawData);
- if (!directoryDeducedFlagId.equals(flagData.id())) {
- throw new FlagValidationException("Flag data file with flag id '%s' in directory for '%s'"
- .formatted(flagData.id(), directoryDeducedFlagId.toString()));
- }
-
- String serializedData = flagData.serializeToJson();
- if (!JSON.equals(serializedData, normalizedRawData)) {
- throw new FlagValidationException("""
- %s contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is:
- %s
- but deserializing this ended up with:
- %s
- These fields may be spelled wrong, or remove them?
- See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax
- """.formatted(filePath, normalizedRawData, serializedData));
- }
- }
-
- if (builder.hasFile(filename, flagData)) {
- throw new FlagValidationException("Flag data file in '%s' contains redundant flag data for id '%s' already set in another directory!"
- .formatted(filePath, flagData.id()));
- }
-
- builder.addFile(filename, flagData);
- }
-
- static String normalizeJson(String json, Set<ZoneId> zones) {
- JsonNode root = uncheck(() -> mapper.readTree(json));
- removeCommentsRecursively(root);
- removeNullRuleValues(root);
- verifyValues(root, zones);
- return root.toString();
- }
-
- private static void verifyValues(JsonNode root, Set<ZoneId> zones) {
- var cursor = new JsonAccessor(root);
- cursor.get("rules").forEachArrayElement(rule -> rule.get("conditions").forEachArrayElement(condition -> {
- FetchVector.Dimension dimension = DimensionHelper
- .fromWire(condition.get("dimension")
- .asString()
- .orElseThrow(() -> new FlagValidationException("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 FlagValidationException("Unknown cloud: " + cloud);
- });
- case CLUSTER_ID -> validateStringValues(condition, ClusterSpec.Id::from);
- case CLUSTER_TYPE -> validateStringValues(condition, ClusterSpec.Type::from);
- case ENVIRONMENT -> validateStringValues(condition, Environment::from);
- case HOSTNAME -> validateStringValues(condition, HostName::of);
- case NODE_TYPE -> validateStringValues(condition, NodeType::valueOf);
- case SYSTEM -> validateStringValues(condition, system -> {
+ private static void validateSystems(FlagData flagData) throws FlagValidationException {
+ flagData.rules().forEach(rule -> rule.conditions().forEach(condition -> {
+ if (condition.dimension() == SYSTEM) {
+ validateConditionValues(condition, system -> {
if (!SystemName.hostedVespa().contains(SystemName.from(system)))
throw new FlagValidationException("Unknown system: " + system);
});
- case TENANT_ID -> validateStringValues(condition, TenantName::from);
- case VESPA_VERSION -> validateStringValues(condition, versionString -> {
- if (Version.fromString(versionString).getMajor() < 8)
- throw new FlagValidationException("Major Vespa version must be at least 8: " + versionString);
- });
- case ZONE_ID -> validateStringValues(condition, zoneIdString -> {
- ZoneId zoneId = ZoneId.from(zoneIdString);
- if (!zones.isEmpty() && !zones.contains(zoneId))
- throw new FlagValidationException("Unknown zone: " + zoneIdString);
- });
}
}));
}
- private static void validateStringValues(JsonAccessor condition, Consumer<String> 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 FlagValidationException("Non-string %s in %s condition: %s".formatted(
- dimension, type, conditionValue));
- });
+ private static void validateForSystem(FlagData flagData, ZoneRegistry zoneRegistry, boolean inController) throws FlagValidationException {
+ Set<ZoneId> zones = inController ?
+ zoneRegistry.zonesIncludingSystem().all().zones().stream().map(ZoneApi::getVirtualId).collect(Collectors.toSet()) :
+ null;
+
+ flagData.rules().forEach(rule -> rule.conditions().forEach(condition -> {
+ int force_switch_expression_dummy = switch (condition.type()) {
+ case RELATIONAL -> switch (condition.dimension()) {
+ case APPLICATION_ID, CLOUD, CLUSTER_ID, CLUSTER_TYPE, CONSOLE_USER_EMAIL, ENVIRONMENT,
+ HOSTNAME, NODE_TYPE, SYSTEM, TENANT_ID, ZONE_ID ->
+ throw new FlagValidationException(condition.type().toWire() + " " +
+ DimensionHelper.toWire(condition.dimension()) +
+ " condition is not supported");
+ case VESPA_VERSION -> {
+ RelationalCondition rCond = RelationalCondition.create(condition.toCreateParams());
+ Version version = Version.fromString(rCond.relationalPredicate().rightOperand());
+ if (version.getMajor() < 8)
+ throw new FlagValidationException("Major Vespa version must be at least 8: " + version);
+ yield 0;
+ }
+ };
+
+ case WHITELIST, BLACKLIST -> switch (condition.dimension()) {
+ case APPLICATION_ID -> validateConditionValues(condition, ApplicationId::fromSerializedForm);
+ case CONSOLE_USER_EMAIL -> validateConditionValues(condition, email -> {
+ if (!email.contains("@"))
+ throw new FlagValidationException("Invalid email address: " + email);
+ });
+ case CLOUD -> validateConditionValues(condition, cloud -> {
+ if (!Set.of(YAHOO, AWS, GCP).contains(CloudName.from(cloud)))
+ throw new FlagValidationException("Unknown cloud: " + cloud);
+ });
+ case CLUSTER_ID -> validateConditionValues(condition, ClusterSpec.Id::from);
+ case CLUSTER_TYPE -> validateConditionValues(condition, ClusterSpec.Type::from);
+ case ENVIRONMENT -> validateConditionValues(condition, Environment::from);
+ case HOSTNAME -> validateConditionValues(condition, HostName::of);
+ case NODE_TYPE -> validateConditionValues(condition, NodeType::valueOf);
+ case SYSTEM -> throw new IllegalStateException("Flag data contains system dimension");
+ case TENANT_ID -> validateConditionValues(condition, TenantName::from);
+ case VESPA_VERSION -> throw new FlagValidationException(condition.type().toWire() + " " +
+ DimensionHelper.toWire(condition.dimension()) +
+ " condition is not supported");
+ case ZONE_ID -> validateConditionValues(condition, zoneIdString -> {
+ ZoneId zoneId = ZoneId.from(zoneIdString);
+ if (inController && !zones.contains(zoneId))
+ throw new FlagValidationException("Unknown zone: " + zoneIdString);
+ });
+ };
+ };
+ }));
+ }
+
+ private static int validateConditionValues(Condition condition, Consumer<String> valueValidator) {
+ condition.toCreateParams().values().forEach(value -> {
try {
valueValidator.accept(value);
} catch (IllegalArgumentException e) {
- String dimension = condition.get("dimension").asString().orElseThrow();
- String type = condition.get("type").asString().orElseThrow();
+ String dimension = DimensionHelper.toWire(condition.dimension());
+ String type = condition.type().toWire();
throw new FlagValidationException("Invalid %s '%s' in %s condition: %s".formatted(dimension, value, type, e.getMessage()));
}
});
+
+ return 0; // dummy to force switch expression
+ }
+
+ private static FlagData parseFlagData(FlagId flagId, String fileContent, ZoneRegistry zoneRegistry, boolean inController) {
+ if (fileContent.isBlank()) return new FlagData(flagId);
+
+ final JsonNode root;
+ try {
+ root = mapper.readTree(fileContent);
+ } catch (JsonProcessingException e) {
+ throw new FlagValidationException("Invalid JSON: " + e.getMessage());
+ }
+
+ removeCommentsRecursively(root);
+ removeNullRuleValues(root);
+ String normalizedRawData = root.toString();
+ FlagData flagData = FlagData.deserialize(normalizedRawData);
+
+ if (!flagId.equals(flagData.id()))
+ throw new FlagValidationException("Flag ID specified in file (%s) doesn't match the directory name (%s)"
+ .formatted(flagData.id(), flagId.toString()));
+
+ String serializedData = flagData.serializeToJson();
+ if (!JSON.equals(serializedData, normalizedRawData))
+ throw new FlagValidationException("""
+ Unknown non-comment fields or rules with null values: after removing any comment fields the JSON is:
+ %s
+ but deserializing this ended up with:
+ %s
+ These fields may be spelled wrong, or remove them?
+ See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax
+ """.formatted(normalizedRawData, serializedData));
+
+ validateSystems(flagData);
+ flagData = flagData.partialResolve(new FetchVector().with(SYSTEM, zoneRegistry.system().value()));
+
+ validateForSystem(flagData, zoneRegistry, inController);
+
+ return flagData;
}
private static void removeCommentsRecursively(JsonNode node) {
@@ -312,56 +321,46 @@ public class SystemFlagsDataArchive {
public Builder() {}
- public Builder addFile(String filename, FlagData data) {
- files.computeIfAbsent(data.id(), k -> new TreeMap<>()).put(filename, data);
- return this;
- }
+ boolean maybeAddFile(Path filePath, String fileContent, ZoneRegistry zoneRegistry, boolean inController) {
+ String filename = filePath.getFileName().toString();
- public boolean hasFile(String filename, FlagData data) {
- return files.containsKey(data.id()) && files.get(data.id()).containsKey(filename);
- }
-
- public SystemFlagsDataArchive build() {
- Map<FlagId, Map<String, FlagData>> copy = new TreeMap<>();
- files.forEach((flagId, map) -> copy.put(flagId, new TreeMap<>(map)));
- return new SystemFlagsDataArchive(copy);
- }
+ if (filename.startsWith("."))
+ return false; // Ignore files starting with '.'
- }
+ if (!inController && !FlagsTarget.filenameForSystem(filename, zoneRegistry.system()))
+ return false; // Ignore files for other systems
- private static class JsonAccessor {
- private final JsonNode jsonNode;
+ FlagId directoryDeducedFlagId = new FlagId(filePath.getName(filePath.getNameCount()-2).toString());
- public JsonAccessor(JsonNode jsonNode) {
- this.jsonNode = jsonNode;
- }
+ if (hasFile(filename, directoryDeducedFlagId))
+ throw new FlagValidationException("Flag data file in '%s' contains redundant flag data for id '%s' already set in another directory!"
+ .formatted(filePath, directoryDeducedFlagId));
- public JsonAccessor get(String fieldName) {
- if (jsonNode == null) {
- return this;
- } else {
- return new JsonAccessor(jsonNode.get(fieldName));
+ final FlagData flagData;
+ try {
+ flagData = parseFlagData(directoryDeducedFlagId, fileContent, zoneRegistry, inController);
+ } catch (FlagValidationException e) {
+ throw new FlagValidationException("In file " + filePath + ": " + e.getMessage());
}
- }
- public Optional<String> asString() {
- return jsonNode != null && jsonNode.isTextual() ? Optional.of(jsonNode.textValue()) : Optional.empty();
+ addFile(filename, flagData);
+ return true;
}
- public void forEachArrayElement(Consumer<JsonAccessor> consumer) {
- if (jsonNode != null && jsonNode.isArray()) {
- jsonNode.forEach(jsonNodeElement -> consumer.accept(new JsonAccessor(jsonNodeElement)));
- }
+ public Builder addFile(String filename, FlagData data) {
+ files.computeIfAbsent(data.id(), k -> new TreeMap<>()).put(filename, data);
+ return this;
}
- /** Returns true if this (JsonNode) is a string and equal to value. */
- public boolean isEqualTo(String value) {
- return jsonNode != null && jsonNode.isTextual() && Objects.equals(jsonNode.textValue(), value);
+ public boolean hasFile(String filename, FlagId id) {
+ return files.containsKey(id) && files.get(id).containsKey(filename);
}
- @Override
- public String toString() {
- return jsonNode == null ? "undefined" : jsonNode.toString();
+ public SystemFlagsDataArchive build() {
+ Map<FlagId, Map<String, FlagData>> copy = new TreeMap<>();
+ files.forEach((flagId, map) -> copy.put(flagId, new TreeMap<>(map)));
+ return new SystemFlagsDataArchive(copy);
}
+
}
}
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 2d0374dc888..759f21579d4 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
@@ -14,6 +14,7 @@ import com.yahoo.vespa.athenz.api.AthenzService;
import com.yahoo.vespa.flags.FetchVector;
import com.yahoo.vespa.flags.FlagId;
import com.yahoo.vespa.flags.RawFlag;
+import com.yahoo.vespa.flags.json.Condition;
import com.yahoo.vespa.flags.json.FlagData;
import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
import org.junit.jupiter.api.Test;
@@ -28,10 +29,12 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URI;
+import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.Optional;
import java.util.Set;
+import java.util.stream.Stream;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -78,11 +81,11 @@ public class SystemFlagsDataArchiveTest {
can_serialize_and_deserialize_archive(true);
}
- private void can_serialize_and_deserialize_archive(boolean forceAddFiles) throws IOException {
+ private void can_serialize_and_deserialize_archive(boolean simulateInController) throws IOException {
File tempFile = File.createTempFile("serialized-flags-archive", null, temporaryFolder);
try (OutputStream out = new BufferedOutputStream(new FileOutputStream(tempFile))) {
- var archive = fromDirectory("system-flags", forceAddFiles);
- if (forceAddFiles)
+ var archive = fromDirectory("system-flags", simulateInController);
+ if (simulateInController)
archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget));
archive.toZip(out);
}
@@ -98,9 +101,9 @@ public class SystemFlagsDataArchiveTest {
retrieves_correct_flag_data_for_target(true);
}
- private void retrieves_correct_flag_data_for_target(boolean forceAddFiles) {
- var archive = fromDirectory("system-flags", forceAddFiles);
- if (forceAddFiles)
+ private void retrieves_correct_flag_data_for_target(boolean simulateInController) {
+ var archive = fromDirectory("system-flags", simulateInController);
+ if (simulateInController)
archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget));
assertArchiveReturnsCorrectTestFlagDataForTarget(archive);
}
@@ -111,9 +114,9 @@ public class SystemFlagsDataArchiveTest {
supports_multi_level_flags_directory(true);
}
- private void supports_multi_level_flags_directory(boolean forceAddFiles) {
- var archive = fromDirectory("system-flags-multi-level", forceAddFiles);
- if (forceAddFiles)
+ private void supports_multi_level_flags_directory(boolean simulateInController) {
+ var archive = fromDirectory("system-flags-multi-level", simulateInController);
+ if (simulateInController)
archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget));
assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "default");
}
@@ -124,9 +127,9 @@ public class SystemFlagsDataArchiveTest {
duplicated_flagdata_is_detected(true);
}
- private void duplicated_flagdata_is_detected(boolean forceAddFiles) {
+ private void duplicated_flagdata_is_detected(boolean simulateInController) {
Throwable exception = assertThrows(FlagValidationException.class, () -> {
- fromDirectory("system-flags-multi-level-with-duplicated-flagdata", forceAddFiles);
+ fromDirectory("system-flags-multi-level-with-duplicated-flagdata", simulateInController);
});
assertTrue(exception.getMessage().contains("contains redundant flag data for id 'my-test-flag' already set in another directory!"));
}
@@ -137,9 +140,9 @@ public class SystemFlagsDataArchiveTest {
empty_files_are_handled_as_no_flag_data_for_target(true);
}
- private void empty_files_are_handled_as_no_flag_data_for_target(boolean forceAddFiles) {
- var archive = fromDirectory("system-flags", forceAddFiles);
- if (forceAddFiles)
+ private void empty_files_are_handled_as_no_flag_data_for_target(boolean simulateInController) {
+ var archive = fromDirectory("system-flags", simulateInController);
+ if (simulateInController)
archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget));
assertNoFlagData(archive, FLAG_WITH_EMPTY_DATA, mainControllerTarget);
assertFlagDataHasValue(archive, FLAG_WITH_EMPTY_DATA, prodUsWestCfgTarget, "main.prod.us-west-1");
@@ -187,7 +190,7 @@ public class SystemFlagsDataArchiveTest {
fromDirectory("system-flags-with-unknown-field-name", true);
});
assertEquals("""
- flags/my-test-flag/main.prod.us-west-1.json contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is:
+ In file flags/my-test-flag/main.prod.us-west-1.json: Unknown non-comment fields or rules with null values: after removing any comment fields the JSON is:
{"id":"my-test-flag","rules":[{"condition":[{"type":"whitelist","dimension":"hostname","values":["foo.com"]}],"value":"default"}]}
but deserializing this ended up with:
{"id":"my-test-flag","rules":[{"value":"default"}]}
@@ -218,6 +221,7 @@ public class SystemFlagsDataArchiveTest {
void remove_comments_and_null_value_in_rules() {
assertTrue(JSON.equals("""
{
+ "id": "foo",
"rules": [
{
"conditions": [
@@ -249,8 +253,9 @@ public class SystemFlagsDataArchiveTest {
}
]
}""",
- SystemFlagsDataArchive.normalizeJson("""
+ normalizeJson("""
{
+ "id": "foo",
"comment": "bar",
"rules": [
{
@@ -289,83 +294,91 @@ public class SystemFlagsDataArchiveTest {
"value": true
}
]
- }""", Set.of(ZoneId.from("prod.us-west-1")))));
+ }""")));
+ }
+
+ private static String normalizeJson(String json) {
+ SystemFlagsDataArchive.Builder builder = new SystemFlagsDataArchive.Builder();
+ assertTrue(builder.maybeAddFile(Path.of("flags/temporary/foo/default.json"), json, createZoneRegistryMock(), true));
+ List<FlagData> flagData = builder.build().flagData(prodUsWestCfgTarget);
+ assertEquals(1, flagData.size());
+ return JSON.canonical(flagData.get(0).serializeToJson());
}
@Test
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("environment", "\"prod\"");
- normalizeJson("environment", "\"staging\"");
- normalizeJson("environment", "\"test\"");
- 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")));
- }
-
- private void normalizeJson(String dimension, String jsonValue) {
- normalizeJson(dimension, jsonValue, Set.of());
- }
-
- private void normalizeJson(String dimension, String jsonValue, Set<ZoneId> zones) {
- SystemFlagsDataArchive.normalizeJson("""
+ addFile(Condition.Type.WHITELIST, "application", "a:b:c");
+ addFile(Condition.Type.WHITELIST, "cloud", "yahoo");
+ addFile(Condition.Type.WHITELIST, "cloud", "aws");
+ addFile(Condition.Type.WHITELIST, "cloud", "gcp");
+ addFile(Condition.Type.WHITELIST, "cluster-id", "some-id");
+ addFile(Condition.Type.WHITELIST, "cluster-type", "admin");
+ addFile(Condition.Type.WHITELIST, "cluster-type", "container");
+ addFile(Condition.Type.WHITELIST, "cluster-type", "content");
+ addFile(Condition.Type.WHITELIST, "console-user-email", "name@domain.com");
+ addFile(Condition.Type.WHITELIST, "environment", "prod");
+ addFile(Condition.Type.WHITELIST, "environment", "staging");
+ addFile(Condition.Type.WHITELIST, "environment", "test");
+ addFile(Condition.Type.WHITELIST, "hostname", "2080046-v6-11.ostk.bm2.prod.gq1.yahoo.com");
+ addFile(Condition.Type.WHITELIST, "node-type", "tenant");
+ addFile(Condition.Type.WHITELIST, "node-type", "host");
+ addFile(Condition.Type.WHITELIST, "node-type", "config");
+ addFile(Condition.Type.WHITELIST, "node-type", "host");
+ addFile(Condition.Type.WHITELIST, "system", "main");
+ addFile(Condition.Type.WHITELIST, "system", "public");
+ addFile(Condition.Type.WHITELIST, "tenant", "vespa");
+ addFile(Condition.Type.RELATIONAL, "vespa-version", ">=8.201.13");
+ addFile(Condition.Type.WHITELIST, "zone", "prod.us-west-1");
+ }
+
+ private void addFile(Condition.Type type, String dimension, String jsonValue) {
+ SystemFlagsDataArchive.Builder builder = new SystemFlagsDataArchive.Builder();
+
+ String valuesField = type == Condition.Type.RELATIONAL ?
+ "\"predicate\": \"%s\"".formatted(jsonValue) :
+ "\"values\": [ \"%s\" ]".formatted(jsonValue);
+
+ assertTrue(builder.maybeAddFile(Path.of("flags/temporary/foo/default.json"), """
{
"id": "foo",
"rules": [
{
"conditions": [
{
- "type": "whitelist",
+ "type": "%s",
"dimension": "%s",
- "values": [ %s ]
+ %s
}
],
"value": true
}
]
}
- """.formatted(dimension, jsonValue), zones);
+ """.formatted(type.toWire(), dimension, valuesField),
+ createZoneRegistryMock(),
+ true));
}
@Test
void normalize_json_fail_on_invalid_values() {
- failNormalizeJson("application", "\"a.b.c\"", "Invalid application 'a.b.c' in whitelist condition: 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\"", "Invalid cluster-type 'foo' in whitelist condition: Illegal cluster type 'foo'");
- failNormalizeJson("console-user-email", "123", "Non-string console-user-email in whitelist condition: 123");
- failNormalizeJson("environment", "\"foo\"", "Invalid environment 'foo' in whitelist condition: 'foo' is not a valid environment identifier");
- failNormalizeJson("hostname", "\"not:a:hostname\"", "Invalid hostname 'not:a:hostname' in whitelist condition: 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\"", "Invalid node-type 'footype' in whitelist condition: No enum constant com.yahoo.config.provision.NodeType.footype");
- failNormalizeJson("system", "\"bar\"", "Invalid system 'bar' in whitelist condition: 'bar' is not a valid system");
- failNormalizeJson("tenant", "123", "Non-string tenant in whitelist condition: 123");
- failNormalizeJson("vespa-version", "\"not-a-version\"", "Invalid vespa-version 'not-a-version' in whitelist condition: Invalid version component in 'not-a-version'");
- failNormalizeJson("zone", "\"dev.%illegal\"", Set.of(ZoneId.from("prod.example-region")), "Invalid zone 'dev.%illegal' in whitelist condition: region name must match '[a-z]([a-z0-9-]*[a-z0-9])*', but got: '%illegal'");
- 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<ZoneId> zones, String expectedExceptionMessage) {
+ failAddFile(Condition.Type.WHITELIST, "application", "a.b.c", "In file flags/temporary/foo/default.json: Invalid application 'a.b.c' in whitelist condition: Application ids must be on the form tenant:application:instance, but was a.b.c");
+ failAddFile(Condition.Type.WHITELIST, "cloud", "foo", "In file flags/temporary/foo/default.json: Unknown cloud: foo");
+ // cluster-id: any String is valid
+ failAddFile(Condition.Type.WHITELIST, "cluster-type", "foo", "In file flags/temporary/foo/default.json: Invalid cluster-type 'foo' in whitelist condition: Illegal cluster type 'foo'");
+ failAddFile(Condition.Type.WHITELIST, "console-user-email", "not-valid-email-address", "In file flags/temporary/foo/default.json: Invalid email address: not-valid-email-address");
+ failAddFile(Condition.Type.WHITELIST, "environment", "foo", "In file flags/temporary/foo/default.json: Invalid environment 'foo' in whitelist condition: 'foo' is not a valid environment identifier");
+ failAddFile(Condition.Type.WHITELIST, "hostname", "not:a:hostname", "In file flags/temporary/foo/default.json: Invalid hostname 'not:a:hostname' in whitelist condition: 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'");
+ failAddFile(Condition.Type.WHITELIST, "node-type", "footype", "In file flags/temporary/foo/default.json: Invalid node-type 'footype' in whitelist condition: No enum constant com.yahoo.config.provision.NodeType.footype");
+ failAddFile(Condition.Type.WHITELIST, "system", "bar", "In file flags/temporary/foo/default.json: Invalid system 'bar' in whitelist condition: 'bar' is not a valid system");
+ failAddFile(Condition.Type.WHITELIST, "tenant", "a tenant", "In file flags/temporary/foo/default.json: Invalid tenant 'a tenant' in whitelist condition: tenant name must match '[a-zA-Z0-9_-]{1,256}', but got: 'a tenant'");
+ failAddFile(Condition.Type.WHITELIST, "vespa-version", "not-a-version", "In file flags/temporary/foo/default.json: whitelist vespa-version condition is not supported");
+ failAddFile(Condition.Type.RELATIONAL, "vespa-version", ">7.1.2", "In file flags/temporary/foo/default.json: Major Vespa version must be at least 8: 7.1.2");
+ failAddFile(Condition.Type.WHITELIST, "zone", "dev.%illegal", "In file flags/temporary/foo/default.json: Invalid zone 'dev.%illegal' in whitelist condition: region name must match '[a-z]([a-z0-9-]*[a-z0-9])*', but got: '%illegal'");
+ }
+
+ private void failAddFile(Condition.Type type, String dimension, String jsonValue, String expectedExceptionMessage) {
try {
- normalizeJson(dimension, jsonValue, zones);
+ addFile(type, dimension, jsonValue);
fail();
} catch (RuntimeException e) {
assertEquals(expectedExceptionMessage, e.getMessage());
@@ -380,8 +393,8 @@ public class SystemFlagsDataArchiveTest {
assertFlagDataHasValue(archive, MY_TEST_FLAG, prodUsWestCfgTarget, "main.prod.us-west-1");
}
- private SystemFlagsDataArchive fromDirectory(String testDirectory, boolean forceAddFiles) {
- return SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/" + testDirectory), createZoneRegistryMock(), forceAddFiles);
+ private SystemFlagsDataArchive fromDirectory(String testDirectory, boolean simulateInController) {
+ return SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/" + testDirectory), createZoneRegistryMock(), simulateInController);
}
@SuppressWarnings("unchecked") // workaround for mocking a method for generic return type
@@ -396,12 +409,21 @@ public class SystemFlagsDataArchiveTest {
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 zones = mockZoneList("prod.us-west-1", "prod.us-east-3");
+ when(registryMock.zones()).thenReturn(zones);
+ ZoneList zonesIncludingSystem = mockZoneList("prod.us-west-1", "prod.us-east-3", "prod.controller");
+ when(registryMock.zonesIncludingSystem()).thenReturn(zonesIncludingSystem);
+ return registryMock;
+ }
+
+ @SuppressWarnings("unchecked") // workaround for mocking a method for generic return type
+ private static ZoneList mockZoneList(String... zones) {
ZoneList zoneListMock = mock(ZoneList.class);
when(zoneListMock.reachable()).thenReturn(zoneListMock);
when(zoneListMock.all()).thenReturn(zoneListMock);
- when(zoneListMock.zones()).thenReturn((List)List.of(new SimpleZone("prod.us-west-1"), new SimpleZone("prod.us-east-3")));
- when(registryMock.zones()).thenReturn(zoneListMock);
- return registryMock;
+ List<? extends ZoneApi> zoneList = Stream.of(zones).map(SimpleZone::new).toList();
+ when(zoneListMock.zones()).thenReturn((List) zoneList);
+ return zoneListMock;
}
private static void assertArchiveReturnsCorrectTestFlagDataForTarget(SystemFlagsDataArchive archive) {
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 749f6830870..031b61c8e7e 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
@@ -68,6 +68,10 @@ public class RelationalCondition implements Condition {
return fetchVector.getValue(dimension).map(predicate::test).orElse(false);
}
+ public RelationalPredicate relationalPredicate() {
+ return relationalPredicate;
+ }
+
@Override
public WireCondition toWire() {
var condition = new WireCondition();