Merge pull request #12674 from vespa-engine/bjorncs/system-feature-flags

Bjorncs/system feature flags

8 files changed, 106 insertions, 11 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 7e9da53b1c9..0cb4baab790 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 2019 Oath Inc. 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.SystemName;
 import com.yahoo.vespa.flags.FlagId;
 import com.yahoo.vespa.flags.json.FlagData;
 
@@ -13,11 +14,13 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
@@ -47,7 +50,7 @@ public class SystemFlagsDataArchive {
             ZipEntry entry;
             while ((entry = zipIn.getNextEntry()) != null) {
                 String name = entry.getName();
-                if (!entry.isDirectory() && name.startsWith("flags/") && name.endsWith(".json")) {
+                if (!entry.isDirectory() && name.startsWith("flags/")) {
                     Path filePath = Paths.get(name);
                     String rawData = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8);
                     addFile(builder, rawData, filePath);
@@ -70,7 +73,7 @@ public class SystemFlagsDataArchive {
             directoryStream.forEach(absolutePath -> {
                 Path relativePath = root.relativize(absolutePath);
                 if (!Files.isDirectory(absolutePath) &&
-                        relativePath.startsWith("flags") && relativePath.toString().endsWith(".json")) {
+                        relativePath.startsWith("flags")) {
                     String rawData = uncheck(() -> Files.readString(absolutePath, StandardCharsets.UTF_8));
                     addFile(builder, rawData, relativePath);
                 }
@@ -86,7 +89,7 @@ public class SystemFlagsDataArchive {
         files.forEach((flagId, fileMap) -> {
             fileMap.forEach((filename, flagData) -> {
                 uncheck(() -> {
-                    zipOut.putNextEntry(new ZipEntry("flags/" + flagId.toString() + "/" + filename));
+                    zipOut.putNextEntry(new ZipEntry(toFilePath(flagId, filename)));
                     zipOut.write(flagData.serializeToUtf8Json());
                     zipOut.closeEntry();
                 });
@@ -112,8 +115,33 @@ public class SystemFlagsDataArchive {
         return targetData;
     }
 
+    public void validateAllFilesAreForTargets(SystemName currentSystem, Set<FlagsTarget> targets) throws IllegalArgumentException {
+        Set<String> validFiles = targets.stream()
+                .flatMap(target -> target.flagDataFilesPrioritized().stream())
+                .collect(Collectors.toSet());
+        Set<SystemName> otherSystems = Arrays.stream(SystemName.values())
+                .filter(systemName -> systemName != currentSystem)
+                .collect(Collectors.toSet());
+        files.forEach((flagId, fileMap) -> {
+            for (String filename : fileMap.keySet()) {
+                boolean isFileForOtherSystem = otherSystems.stream()
+                        .anyMatch(system -> filename.startsWith(system.value() + "."));
+                boolean isFileForCurrentSystem = validFiles.contains(filename);
+                if (!isFileForOtherSystem && !isFileForCurrentSystem) {
+                    throw new IllegalArgumentException("Unknown flag file: " + toFilePath(flagId, filename));
+                }
+            }
+        });
+    }
+
     private static void addFile(Builder builder, String rawData, Path filePath) {
         String filename = filePath.getFileName().toString();
+        if (filename.startsWith(".")) {
+            return; // Ignore files starting with '.'
+        }
+        if (!filename.endsWith(".json")) {
+            throw new IllegalArgumentException(String.format("Only JSON files are allowed in 'flags/' directory (found '%s')", filePath.toString()));
+        }
         FlagId directoryDeducedFlagId = new FlagId(filePath.getName(1).toString());
         FlagData flagData;
         if (rawData.isBlank()) {
@@ -129,6 +157,10 @@ public class SystemFlagsDataArchive {
         builder.addFile(filename, flagData);
     }
 
+    private static String toFilePath(FlagId flagId, String filename) {
+        return "flags/" + flagId.toString() + "/" + filename;
+    }
+
     public static class Builder {
         private final Map<FlagId, Map<String, FlagData>> files = new TreeMap<>();
 
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 14584650d3b..6e78b8da91c 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
@@ -13,6 +13,7 @@ import com.yahoo.vespa.flags.RawFlag;
 import com.yahoo.vespa.flags.json.FlagData;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 
 import java.io.BufferedInputStream;
@@ -27,6 +28,7 @@ import java.net.URI;
 import java.nio.file.Paths;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import static java.util.stream.Collectors.toList;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -43,6 +45,9 @@ public class SystemFlagsDataArchiveTest {
     @Rule
     public final TemporaryFolder temporaryFolder = new TemporaryFolder();
 
+    @Rule
+    public final ExpectedException expectedException = ExpectedException.none();
+
     private static FlagsTarget mainControllerTarget = FlagsTarget.forController(SYSTEM);
     private static FlagsTarget prodUsWestCfgTarget = createConfigserverTarget(Environment.prod, "us-west-1");
     private static FlagsTarget prodUsEast3CfgTarget = createConfigserverTarget(Environment.prod, "us-east-3");
@@ -84,6 +89,21 @@ public class SystemFlagsDataArchiveTest {
         assertFlagDataHasValue(archive, FLAG_WITH_EMPTY_DATA, devUsEast1CfgTarget, "main");
     }
 
+    @Test
+    public void throws_exception_on_non_json_file() {
+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage("Only JSON files are allowed in 'flags/' directory (found 'flags/my-test-flag/file-name-without-dot-json')");
+        SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-invalid-file-name/"));
+    }
+
+    @Test
+    public void throws_exception_on_unknown_file() {
+        SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-unknown-file-name/"));
+        expectedException.expect(IllegalArgumentException.class);
+        expectedException.expectMessage("Unknown flag file: flags/my-test-flag/main.prod.unknown-region.json");
+        archive.validateAllFilesAreForTargets(SystemName.main, Set.of(mainControllerTarget, prodUsWestCfgTarget));
+    }
+
     private static void assertArchiveReturnsCorrectTestFlagDataForTarget(SystemFlagsDataArchive archive) {
         assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "main.controller");
         assertFlagDataHasValue(archive, MY_TEST_FLAG, prodUsWestCfgTarget, "main.prod.us-west-1");
diff --git a/controller-api/src/test/resources/system-flags-with-invalid-file-name/flags/my-test-flag/file-name-without-dot-json b/controller-api/src/test/resources/system-flags-with-invalid-file-name/flags/my-test-flag/file-name-without-dot-json
new file mode 100644
index 00000000000..5924eb860c0
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags-with-invalid-file-name/flags/my-test-flag/file-name-without-dot-json
@@ -0,0 +1,8 @@
+{
+  "id" : "my-test-flag",
+  "rules" : [
+    {
+      "value" : "default"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-api/src/test/resources/system-flags-with-unknown-file-name/flags/my-test-flag/main.prod.unknown-region.json b/controller-api/src/test/resources/system-flags-with-unknown-file-name/flags/my-test-flag/main.prod.unknown-region.json
new file mode 100644
index 00000000000..5924eb860c0
--- /dev/null
+++ b/controller-api/src/test/resources/system-flags-with-unknown-file-name/flags/my-test-flag/main.prod.unknown-region.json
@@ -0,0 +1,8 @@
+{
+  "id" : "my-test-flag",
+  "rules" : [
+    {
+      "value" : "default"
+    }
+  ]
+}
\ No newline at end of file
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
index bc00090dc88..c55d9d9a5e9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsDeployResult.java
@@ -218,6 +218,10 @@ class SystemFlagsDeployResult {
             return new OperationError(message, Set.of(target), OperationType.DELETE, id, null);
         }
 
+        static OperationError archiveValidationFailed(String message) {
+            return new OperationError(message, Set.of(), OperationType.VALIDATE_ARCHIVE, null, null);
+        }
+
         String message() { return message; }
         Set<FlagsTarget> targets() { return targets; }
         OperationType operation() { return operation; }
@@ -251,7 +255,7 @@ class SystemFlagsDeployResult {
     }
 
     enum OperationType {
-        CREATE("create"), DELETE("delete"), UPDATE("update"), LIST("list");
+        CREATE("create"), DELETE("delete"), UPDATE("update"), LIST("list"), VALIDATE_ARCHIVE("validate-archive");
 
         private final String stringValue;
 
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 b89a9dd7f5a..c75a6d7b413 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
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.restapi.systemflags;
 
 import com.yahoo.concurrent.DaemonThreadFactory;
+import com.yahoo.config.provision.SystemName;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider;
 import com.yahoo.vespa.flags.FlagId;
@@ -38,16 +39,18 @@ class SystemFlagsDeployer  {
     private static final Logger log = Logger.getLogger(SystemFlagsDeployer.class.getName());
 
     private final FlagsClient client;
+    private final SystemName system;
     private final Set<FlagsTarget> targets;
     private final ExecutorService executor = Executors.newCachedThreadPool(new DaemonThreadFactory("system-flags-deployer-"));
 
 
-    SystemFlagsDeployer(ServiceIdentityProvider identityProvider, Set<FlagsTarget> targets) {
-        this(new FlagsClient(identityProvider, targets), targets);
+    SystemFlagsDeployer(ServiceIdentityProvider identityProvider, SystemName system, Set<FlagsTarget> targets) {
+        this(new FlagsClient(identityProvider, targets), system, targets);
     }
 
-    SystemFlagsDeployer(FlagsClient client, Set<FlagsTarget> targets) {
+    SystemFlagsDeployer(FlagsClient client, SystemName system, Set<FlagsTarget> targets) {
         this.client = client;
+        this.system = system;
         this.targets = targets;
     }
 
@@ -65,6 +68,11 @@ class SystemFlagsDeployer  {
                 throw new RuntimeException(e);
             }
         });
+        try {
+            archive.validateAllFilesAreForTargets(system, targets);
+        } catch (IllegalArgumentException e) {
+            results.add(new SystemFlagsDeployResult(List.of(OperationError.archiveValidationFailed(e.getMessage()))));
+        }
         return SystemFlagsDeployResult.merge(results);
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
index fd594cd57bc..2f7716aaff0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/systemflags/SystemFlagsHandler.java
@@ -35,7 +35,7 @@ public class SystemFlagsHandler extends LoggingRequestHandler {
                               Executor executor,
                               AccessLog accessLog) {
         super(executor, accessLog);
-        this.deployer = new SystemFlagsDeployer(identityProvider, FlagsTarget.getAllTargetsInSystem(zoneRegistry));
+        this.deployer = new SystemFlagsDeployer(identityProvider, zoneRegistry.system(), FlagsTarget.getAllTargetsInSystem(zoneRegistry));
     }
 
     @Override
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 1eb4523c344..4c41b54585f 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
@@ -58,7 +58,7 @@ public class SystemFlagsDeployerTest {
                 .build();
 
         SystemFlagsDeployer deployer =
-                new SystemFlagsDeployer(flagsClient, Set.of(controllerTarget, prodUsWest1Target, prodUsEast3Target));
+                new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(controllerTarget, prodUsWest1Target, prodUsEast3Target));
 
 
         SystemFlagsDeployResult result = deployer.deployFlags(archive, false);
@@ -83,7 +83,7 @@ public class SystemFlagsDeployerTest {
                 .addFile("main.json", defaultData)
                 .build();
 
-        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, Set.of(controllerTarget));
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(controllerTarget));
         SystemFlagsDeployResult result = deployer.deployFlags(archive, true);
 
         verify(flagsClient, times(1)).listFlagData(controllerTarget);
@@ -107,7 +107,7 @@ public class SystemFlagsDeployerTest {
                 .addFile("main.json", defaultData)
                 .build();
 
-        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, Set.of(prodUsWest1Target, prodUsEast3Target));
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(prodUsWest1Target, prodUsEast3Target));
 
         SystemFlagsDeployResult result = deployer.deployFlags(archive, false);
 
@@ -117,6 +117,21 @@ public class SystemFlagsDeployerTest {
                 FlagDataChange.created(FLAG_ID, prodUsEast3Target, defaultData));
     }
 
+    @Test
+    public void creates_error_entry_for_invalid_flag_archive() throws IOException {
+        FlagsClient flagsClient = mock(FlagsClient.class);
+        FlagData defaultData = flagData("flags/my-flag/main.json");
+        SystemFlagsDataArchive archive = new SystemFlagsDataArchive.Builder()
+                .addFile("main.prod.unknown-region.json", defaultData)
+                .build();
+        SystemFlagsDeployer deployer = new SystemFlagsDeployer(flagsClient, SYSTEM, Set.of(controllerTarget));
+        SystemFlagsDeployResult result = deployer.deployFlags(archive, false);
+        assertThat(result.flagChanges())
+                .isEmpty();
+        assertThat(result.errors())
+                .containsOnly(OperationError.archiveValidationFailed("Unknown flag file: flags/my-flag/main.prod.unknown-region.json"));
+    }
+
     private static FlagData flagData(String filename) throws IOException {
         return FlagData.deserializeUtf8Json(Files.readAllBytes(Paths.get("src/test/resources/system-flags/" + filename)));
     }