diff options
7 files changed, 62 insertions, 55 deletions
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 856af9f4132..1d9fc319d95 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 @@ -8,6 +8,7 @@ import com.fasterxml.jackson.databind.node.ObjectNode; import com.fasterxml.jackson.databind.node.TextNode; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.ApplicationName; import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.CloudName; import com.yahoo.config.provision.ClusterSpec; @@ -191,7 +192,7 @@ public class SystemFlagsDataArchive { flagData.rules().forEach(rule -> rule.conditions().forEach(condition -> { int force_switch_expression_dummy = switch (condition.type()) { case RELATIONAL -> switch (condition.dimension()) { - case INSTANCE_ID, CLOUD, CLOUD_ACCOUNT, CLUSTER_ID, CLUSTER_TYPE, CONSOLE_USER_EMAIL, + case APPLICATION_ID, INSTANCE_ID, CLOUD, CLOUD_ACCOUNT, 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()) + @@ -206,6 +207,7 @@ public class SystemFlagsDataArchive { }; case WHITELIST, BLACKLIST -> switch (condition.dimension()) { + case APPLICATION_ID -> validateConditionValues(condition, SystemFlagsDataArchive::validateTenantApplication); case INSTANCE_ID -> validateConditionValues(condition, ApplicationId::fromSerializedForm); case CONSOLE_USER_EMAIL -> validateConditionValues(condition, email -> { if (!email.contains("@")) @@ -250,23 +252,20 @@ public class SystemFlagsDataArchive { return 0; // dummy to force switch expression } + private static void validateTenantApplication(String application) { + String[] parts = application.split(":"); + if (parts.length != 2) + throw new IllegalArgumentException("Applications must be on the form tenant:application, but was %s".formatted(application)); + TenantName.from(parts[0]); + ApplicationName.from(parts[1]); + } + 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); - // TODO (mortent): Remove this after completing migration of APPLICATION_ID dimension - // replace "application" with "instance" for all dimension fields - List<JsonNode> dimensionParents = root.findParents("dimension"); - for (JsonNode parentNode : dimensionParents) { - JsonNode dimension = parentNode.get("dimension"); - if (dimension.isTextual() && "application".equals(dimension.textValue())) { - ObjectNode parent = (ObjectNode) parentNode; - parent.remove("dimension"); - parent.put("dimension", "instance"); - } - } } catch (JsonProcessingException e) { throw new FlagValidationException("Invalid JSON: " + e.getMessage()); } 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 373f8ba9de2..2631f518720 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 @@ -250,6 +250,16 @@ public class SystemFlagsDataArchiveTest { } ], "value": true + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": [ "f:o" ] + } + ], + "value": true } ] }""", @@ -292,6 +302,16 @@ public class SystemFlagsDataArchiveTest { } ], "value": true + }, + { + "conditions": [ + { + "type": "whitelist", + "dimension": "application", + "values": [ "f:o" ] + } + ], + "value": true } ] }"""))); @@ -307,8 +327,7 @@ public class SystemFlagsDataArchiveTest { @Test void normalize_json_succeed_on_valid_values() { - addFile(Condition.Type.WHITELIST, "application", "a:b:c"); - addFile(Condition.Type.WHITELIST, "instance", "a:b:c"); + addFile(Condition.Type.WHITELIST, "application", "a:b"); addFile(Condition.Type.WHITELIST, "cloud", "yahoo"); addFile(Condition.Type.WHITELIST, "cloud", "aws"); addFile(Condition.Type.WHITELIST, "cloud", "gcp"); @@ -321,6 +340,7 @@ public class SystemFlagsDataArchiveTest { 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, "instance", "a:b:c"); addFile(Condition.Type.WHITELIST, "node-type", "tenant"); addFile(Condition.Type.WHITELIST, "node-type", "host"); addFile(Condition.Type.WHITELIST, "node-type", "config"); @@ -362,12 +382,13 @@ public class SystemFlagsDataArchiveTest { @Test void normalize_json_fail_on_invalid_values() { - failAddFile(Condition.Type.WHITELIST, "application", "a.b.c", "In file flags/temporary/foo/default.json: Invalid instance '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, "application", "a.b", "In file flags/temporary/foo/default.json: Invalid application 'a.b' in whitelist condition: Applications must be on the form tenant:application, but was a.b"); 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, "instance", "a.b.c", "In file flags/temporary/foo/default.json: Invalid instance '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, "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"); diff --git a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json index b79e0913c22..c4dca9aa2e1 100644 --- a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json +++ b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-east-3.json @@ -5,7 +5,7 @@ "conditions": [ { "type": "whitelist", - "dimension": "application", + "dimension": "instance", "values": ["a:b:c"] } ] diff --git a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json index 75cffdea009..283b09d5c0b 100644 --- a/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json +++ b/controller-api/src/test/resources/system-flags-with-null-value/flags/my-test-flag/main.prod.us-west-1.json @@ -5,7 +5,7 @@ "conditions": [ { "type": "whitelist", - "dimension": "application", + "dimension": "instance", "values": ["a:b:c"] } ], 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 b16d26a04a4..9903d71c237 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/FetchVector.java @@ -22,6 +22,9 @@ public class FetchVector { * Note: If this enum is changed, you must also change {@link DimensionHelper}. */ public enum Dimension { + /** Application id on the form tenant:applicationName. */ + APPLICATION_ID, + /** * Cloud from com.yahoo.config.provision.CloudName::value, e.g. yahoo, aws, gcp. * 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 8fb48c8a82f..04beec19d73 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 @@ -7,28 +7,30 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; /** * @author hakonhall */ public class DimensionHelper { - private static final Map<FetchVector.Dimension, List<String>> serializedDimensions = new HashMap<>(); + private static final Map<FetchVector.Dimension, String> serializedDimensions = new HashMap<>(); static { - serializedDimensions.put(FetchVector.Dimension.CLOUD, List.of("cloud")); - serializedDimensions.put(FetchVector.Dimension.CLOUD_ACCOUNT, List.of("cloud-account")); - serializedDimensions.put(FetchVector.Dimension.CLUSTER_ID, List.of("cluster-id")); - serializedDimensions.put(FetchVector.Dimension.CLUSTER_TYPE, List.of("cluster-type")); - serializedDimensions.put(FetchVector.Dimension.CONSOLE_USER_EMAIL, List.of("console-user-email")); - serializedDimensions.put(FetchVector.Dimension.ENVIRONMENT, List.of("environment")); - serializedDimensions.put(FetchVector.Dimension.HOSTNAME, List.of("hostname")); - serializedDimensions.put(FetchVector.Dimension.INSTANCE_ID, List.of("instance", "application")); - serializedDimensions.put(FetchVector.Dimension.NODE_TYPE, List.of("node-type")); - serializedDimensions.put(FetchVector.Dimension.SYSTEM, List.of("system")); - serializedDimensions.put(FetchVector.Dimension.TENANT_ID, List.of("tenant")); - serializedDimensions.put(FetchVector.Dimension.VESPA_VERSION, List.of("vespa-version")); - serializedDimensions.put(FetchVector.Dimension.ZONE_ID, List.of("zone")); + serializedDimensions.put(FetchVector.Dimension.APPLICATION_ID, "application"); + serializedDimensions.put(FetchVector.Dimension.CLOUD, "cloud"); + serializedDimensions.put(FetchVector.Dimension.CLOUD_ACCOUNT, "cloud-account"); + serializedDimensions.put(FetchVector.Dimension.CLUSTER_ID, "cluster-id"); + serializedDimensions.put(FetchVector.Dimension.CLUSTER_TYPE, "cluster-type"); + serializedDimensions.put(FetchVector.Dimension.CONSOLE_USER_EMAIL, "console-user-email"); + serializedDimensions.put(FetchVector.Dimension.ENVIRONMENT, "environment"); + serializedDimensions.put(FetchVector.Dimension.HOSTNAME, "hostname"); + serializedDimensions.put(FetchVector.Dimension.INSTANCE_ID, "instance"); + 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 " + @@ -36,27 +38,16 @@ public class DimensionHelper { } } - private static final Map<String, FetchVector.Dimension> deserializedDimensions = reverseMapping(serializedDimensions); - - private static Map<String, FetchVector.Dimension> reverseMapping(Map<FetchVector.Dimension, List<String>> mapping) { - Map<String, FetchVector.Dimension> reverseMapping = new LinkedHashMap<>(); - mapping.forEach((dimension, serializedDimensions) -> { - serializedDimensions.forEach(serializedDimension -> { - if (reverseMapping.put(serializedDimension, dimension) != null) { - throw new IllegalStateException("Duplicate serialized dimension: '" + serializedDimension + "'"); - } - }); - }); - return Map.copyOf(reverseMapping); - } + private static final Map<String, FetchVector.Dimension> deserializedDimensions = serializedDimensions. + entrySet().stream().collect(Collectors.toMap(Map.Entry::getValue, Map.Entry::getKey)); public static String toWire(FetchVector.Dimension dimension) { - List<String> serializedDimension = serializedDimensions.get(dimension); - if (serializedDimension == null || serializedDimension.isEmpty()) { + String serializedDimension = serializedDimensions.get(dimension); + if (serializedDimension == null) { throw new IllegalArgumentException("Unsupported dimension (please add it): '" + dimension + "'"); } - return serializedDimension.get(0); + return serializedDimension; } public static FetchVector.Dimension fromWire(String serializedDimension) { 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 ed81afc8054..40310c47f78 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 @@ -30,7 +30,7 @@ public class FlagDataTest { }, { "type": "blacklist", - "dimension": "application", + "dimension": "instance", "values": [ "app1", "app2" ] } ], @@ -52,8 +52,6 @@ public class FlagDataTest { } }"""; - private final String json_with_instance = json.replace("application", "instance"); - private final FetchVector vector = new FetchVector(); @Test @@ -273,11 +271,6 @@ public class FlagDataTest { } private void verify(Optional<String> expectedValue, FetchVector vector) { - verify(json, expectedValue, vector); - verify(json_with_instance, expectedValue, vector); - } - - private void verify(String json, Optional<String> expectedValue, FetchVector vector) { FlagData data = FlagData.deserialize(json); assertEquals("id1", data.id().toString()); Optional<RawFlag> rawFlag = data.resolve(vector); |