diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2020-03-23 17:16:09 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-03-23 17:16:09 +0100 |
commit | 07aa2f3c8fe1b76da5d719b1b0972bdafdfcabaa (patch) | |
tree | 4468f7c186b901ad0798d4777c44049416bb846d | |
parent | 23dcf1db150a66ec66cf746d4234982fdbb0e6e2 (diff) | |
parent | 75731d232e5a9c9e0c9f3fa9679c25e4ed16f46b (diff) |
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))); } |