Fix checking of zones

1 files changed, 136 insertions, 137 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 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);
         }
+
     }
 }