diff options
author | Håkon Hallingstad <hakon@yahooinc.com> | 2023-08-06 23:33:11 +0200 |
---|---|---|
committer | Håkon Hallingstad <hakon@yahooinc.com> | 2023-08-06 23:33:11 +0200 |
commit | caa6f4e85fad97c2c592c3a17d90690d07114302 (patch) | |
tree | 6f4fdc9ce13ec5d07f8b48e3c90807b2bb7f3f8b /controller-api | |
parent | 0f46015e498ecb622473cd3e2403283c99f9f5d5 (diff) |
Hosted feature flags validation
Diffstat (limited to 'controller-api')
5 files changed, 240 insertions, 113 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagValidationException.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagValidationException.java new file mode 100644 index 00000000000..00c88102819 --- /dev/null +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagValidationException.java @@ -0,0 +1,11 @@ +// 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; + +/** + * @author hakonhall + */ +public class FlagValidationException extends RuntimeException { + public FlagValidationException(String message) { + super(message); + } +} 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 bad53620c81..7403f0a1b01 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 @@ -80,6 +80,54 @@ public interface FlagsTarget { static String zoneFile(SystemName system, ZoneId zone) { return jsonFile(system.value() + "." + zone.environment().value() + "." + zone.region().value()); } static String controllerFile(SystemName system) { return jsonFile(system.value() + ".controller"); } + /** Return true if the filename applies to the system. Throws on invalid filename format. */ + static boolean filenameForSystem(String filename, SystemName system) throws FlagValidationException { + if (filename.equals(defaultFile())) return true; + + String[] parts = filename.split("\\.", -1); + if (parts.length < 2) throw new FlagValidationException("Invalid flag filename: " + filename); + + if (!parts[parts.length - 1].equals("json")) throw new FlagValidationException("Invalid flag filename: " + filename); + + SystemName systemFromFile; + try { + systemFromFile = SystemName.from(parts[0]); + } catch (IllegalArgumentException e) { + throw new FlagValidationException("First part of flag filename is neither 'default' nor a valid system: " + filename); + } + if (!SystemName.hostedVespa().contains(systemFromFile)) + throw new FlagValidationException("Unknown system in flag filename: " + filename); + if (!systemFromFile.equals(system)) return false; + + if (parts.length == 2) return true; // systemFile + + if (parts.length == 3) { + if (parts[1].equals("controller")) return true; // controllerFile + try { + Environment.from(parts[1]); + } catch (IllegalArgumentException e) { + throw new FlagValidationException("Invalid environment in flag filename: " + filename); + } + return true; // environmentFile + } + + if (parts.length == 4) { + try { + Environment.from(parts[1]); + } catch (IllegalArgumentException e) { + throw new FlagValidationException("Invalid environment in flag filename: " + filename); + } + try { + RegionName.from(parts[2]); + } catch (IllegalArgumentException e) { + throw new FlagValidationException("Invalid region in flag filename: " + filename); + } + return true; // zoneFile + } + + throw new FlagValidationException("Invalid flag filename: " + filename); + } + /** Partially resolve inter-zone dimensions, except those dimensions defined by the flag for a controller zone. */ static FlagData partialResolve(FlagData data, SystemName system, CloudName cloud, ZoneId virtualZoneId) { Set<FetchVector.Dimension> flagDimensions = 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 8ca4c37a85a..8df2a1b483d 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 @@ -23,6 +23,7 @@ import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import java.io.BufferedInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -32,7 +33,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Objects; @@ -74,7 +74,7 @@ public class SystemFlagsDataArchive { this.files = files; } - public static SystemFlagsDataArchive fromZip(InputStream rawIn) { + public static SystemFlagsDataArchive fromZip(InputStream rawIn, ZoneRegistry zoneRegistry) { Builder builder = new Builder(); try (ZipInputStream zipIn = new ZipInputStream(new BufferedInputStream(rawIn))) { ZipEntry entry; @@ -83,7 +83,7 @@ public class SystemFlagsDataArchive { if (!entry.isDirectory() && name.startsWith("flags/")) { Path filePath = Paths.get(name); String rawData = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8); - addFile(builder, rawData, filePath, Set.of(), null); + addFile(builder, rawData, filePath, zoneRegistry, true); } } return builder.build(); @@ -92,27 +92,19 @@ public class SystemFlagsDataArchive { } } - public static SystemFlagsDataArchive fromDirectoryAndSystem(Path directory, ZoneRegistry systemDefinition) { - return fromDirectory(directory, systemDefinition); - } - - public static SystemFlagsDataArchive fromDirectory(Path directory) { return fromDirectory(directory, null); } - - private static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry systemDefinition) { - Set<String> filenamesForSystem = getFilenamesForSystem(systemDefinition); + public static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry zoneRegistry, boolean forceAddFiles) { Path root = directory.toAbsolutePath(); Path flagsDirectory = directory.resolve("flags"); if (!Files.isDirectory(flagsDirectory)) { - throw new IllegalArgumentException("Sub-directory 'flags' does not exist: " + flagsDirectory); + throw new FlagValidationException("Sub-directory 'flags' does not exist: " + flagsDirectory); } - try (Stream<Path> directoryStream = Files.walk(root)) { + try (Stream<Path> directoryStream = Files.walk(flagsDirectory)) { Builder builder = new Builder(); - directoryStream.forEach(absolutePath -> { - Path relativePath = root.relativize(absolutePath); - if (!Files.isDirectory(absolutePath) && - relativePath.startsWith("flags")) { - String rawData = uncheck(() -> Files.readString(absolutePath, StandardCharsets.UTF_8)); - addFile(builder, rawData, relativePath, filenamesForSystem, systemDefinition); + 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); } }); return builder.build(); @@ -121,6 +113,14 @@ public class SystemFlagsDataArchive { } } + public byte[] toZipBytes() { + try (ByteArrayOutputStream out = new ByteArrayOutputStream()) { + toZip(out); + return out.toByteArray(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } public void toZip(OutputStream out) { ZipOutputStream zipOut = new ZipOutputStream(out); @@ -153,67 +153,48 @@ public class SystemFlagsDataArchive { return targetData; } - public void validateAllFilesAreForTargets(SystemName currentSystem, Set<FlagsTarget> targets) throws IllegalArgumentException { + public void validateAllFilesAreForTargets(Set<FlagsTarget> targets) throws FlagValidationException { 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)); - } + .flatMap(target -> target.flagDataFilesPrioritized().stream()) + .collect(Collectors.toSet()); + files.forEach((flagId, fileMap) -> fileMap.keySet().forEach(filename -> { + if (!validFiles.contains(filename)) { + throw new FlagValidationException("Unknown flag file: " + toFilePath(flagId, filename)); } - }); + })); } - private static Set<String> getFilenamesForSystem(ZoneRegistry systemDefinition) { - if (systemDefinition == null) return Set.of(); - return FlagsTarget.getAllTargetsInSystem(systemDefinition, false).stream() - .flatMap(target -> target.flagDataFilesPrioritized().stream()) - .collect(Collectors.toSet()); + boolean hasFlagData(FlagId flagId, String filename) { + return files.getOrDefault(flagId, Map.of()).containsKey(filename); } - private static void addFile(Builder builder, String rawData, Path filePath, Set<String> filenamesForSystem, - ZoneRegistry systemDefinition) { + 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 (!filenamesForSystem.isEmpty() && !filenamesForSystem.contains(filename)) { - if (systemDefinition != null && filename.startsWith(systemDefinition.system().value() + '.')) { - throw new IllegalArgumentException(String.format( - "Environment or zone in filename '%s' does not exist", filename)); - } - return; // Ignore files irrelevant for system - } - if (!filename.endsWith(".json")) { - throw new IllegalArgumentException(String.format("Only JSON files are allowed in 'flags/' directory (found '%s')", filePath.toString())); + + if (!force) { + if (filename.startsWith(".")) + return; // Ignore files starting with '.' + + if (!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 = systemDefinition == null ? - Set.of() : - systemDefinition.zones().all().zones().stream().map(ZoneApi::getVirtualId).collect(Collectors.toSet()); + Set<ZoneId> zones = zoneRegistry.zones().all().zones().stream().map(ZoneApi::getVirtualId).collect(Collectors.toSet()); String normalizedRawData = normalizeJson(rawData, zones); flagData = FlagData.deserialize(normalizedRawData); if (!directoryDeducedFlagId.equals(flagData.id())) { - throw new IllegalArgumentException( - String.format("Flag data file with flag id '%s' in directory for '%s'", - flagData.id(), directoryDeducedFlagId.toString())); + 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 IllegalArgumentException(""" + 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: @@ -225,9 +206,8 @@ public class SystemFlagsDataArchive { } if (builder.hasFile(filename, flagData)) { - throw new IllegalArgumentException( - String.format("Flag data file in '%s' contains redundant flag data for id '%s' already set in another directory!", - filePath, flagData.id())); + 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); @@ -247,13 +227,13 @@ public class SystemFlagsDataArchive { FetchVector.Dimension dimension = DimensionHelper .fromWire(condition.get("dimension") .asString() - .orElseThrow(() -> new IllegalArgumentException("Invalid dimension in condition: " + condition))); + .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 IllegalArgumentException("Unknown cloud: " + cloud); + throw new FlagValidationException("Unknown cloud: " + cloud); }); case CLUSTER_ID -> validateStringValues(condition, ClusterSpec.Id::from); case CLUSTER_TYPE -> validateStringValues(condition, ClusterSpec.Type::from); @@ -261,18 +241,17 @@ public class SystemFlagsDataArchive { case HOSTNAME -> validateStringValues(condition, HostName::of); case NODE_TYPE -> validateStringValues(condition, NodeType::valueOf); case SYSTEM -> validateStringValues(condition, system -> { - if (!Set.of(SystemName.cd, SystemName.main, SystemName.PublicCd, SystemName.Public).contains(SystemName.from(system))) - throw new IllegalArgumentException("Unknown system: " + 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 -> { - Version vespaVersion = Version.fromString(versionString); - if (vespaVersion.getMajor() < 8) - throw new IllegalArgumentException("Major Vespa version must be at least 8: " + versionString); + if (Version.fromString(versionString).getMajor() < 8) + throw new FlagValidationException("Major Vespa version must be at least 8: " + versionString); }); - case ZONE_ID -> validateStringValues(condition, zoneId -> { - if (!zones.contains(ZoneId.from(zoneId))) - throw new IllegalArgumentException("Unknown zone: " + zoneId); + case ZONE_ID -> validateStringValues(condition, zoneIdString -> { + if (!zones.contains(ZoneId.from(zoneIdString))) + throw new FlagValidationException("Unknown zone: " + zoneIdString); }); } })); @@ -284,10 +263,16 @@ public class SystemFlagsDataArchive { .orElseThrow(() -> { String dimension = condition.get("dimension").asString().orElseThrow(); String type = condition.get("type").asString().orElseThrow(); - return new IllegalArgumentException("Non-string value in %s %s condition: %s".formatted( + return new FlagValidationException("Non-string %s in %s condition: %s".formatted( dimension, type, conditionValue)); }); - valueValidator.accept(value); + try { + valueValidator.accept(value); + } catch (IllegalArgumentException e) { + String dimension = condition.get("dimension").asString().orElseThrow(); + String type = condition.get("type").asString().orElseThrow(); + throw new FlagValidationException("Invalid %s '%s' in %s condition: %s".formatted(dimension, value, type, e.getMessage())); + } }); } diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTargetTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTargetTest.java new file mode 100644 index 00000000000..9177813e38f --- /dev/null +++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/FlagsTargetTest.java @@ -0,0 +1,41 @@ +// 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.yahoo.config.provision.SystemName; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * @author hakonhall + */ +class FlagsTargetTest { + @Test + void sanityCheckFilename() { + assertTrue(FlagsTarget.filenameForSystem("default.json", SystemName.main)); + assertTrue(FlagsTarget.filenameForSystem("main.json", SystemName.main)); + assertTrue(FlagsTarget.filenameForSystem("main.controller.json", SystemName.main)); + assertTrue(FlagsTarget.filenameForSystem("main.prod.json", SystemName.main)); + assertTrue(FlagsTarget.filenameForSystem("main.prod.us-west-1.json", SystemName.main)); + assertTrue(FlagsTarget.filenameForSystem("main.prod.abc-foo-3.json", SystemName.main)); + + assertFalse(FlagsTarget.filenameForSystem("public.json", SystemName.main)); + assertFalse(FlagsTarget.filenameForSystem("public.controller.json", SystemName.main)); + assertFalse(FlagsTarget.filenameForSystem("public.prod.json", SystemName.main)); + assertFalse(FlagsTarget.filenameForSystem("public.prod.us-west-1.json", SystemName.main)); + assertFalse(FlagsTarget.filenameForSystem("public.prod.abc-foo-3.json", SystemName.main)); + + assertFlagValidationException("First part of flag filename is neither 'default' nor a valid system: defaults.json", "defaults.json"); + assertFlagValidationException("Invalid flag filename: default", "default"); + assertFlagValidationException("Invalid flag filename: README", "README"); + assertFlagValidationException("First part of flag filename is neither 'default' nor a valid system: nosystem.json", "nosystem.json"); + assertFlagValidationException("Invalid environment in flag filename: main.noenv.json", "main.noenv.json"); + assertFlagValidationException("Invalid region in flag filename: main.prod.%badregion.json", "main.prod.%badregion.json"); + } + + private void assertFlagValidationException(String expectedMessage, String filename) { + FlagValidationException e = assertThrows(FlagValidationException.class, () -> FlagsTarget.filenameForSystem(filename, SystemName.main)); + assertEquals(expectedMessage, e.getMessage()); + } + +}
\ No newline at end of file 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 3417dc85224..2d0374dc888 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 @@ -28,10 +28,8 @@ 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.Map; import java.util.Optional; import java.util.Set; @@ -76,40 +74,73 @@ public class SystemFlagsDataArchiveTest { @Test void can_serialize_and_deserialize_archive() throws IOException { + can_serialize_and_deserialize_archive(false); + can_serialize_and_deserialize_archive(true); + } + + private void can_serialize_and_deserialize_archive(boolean forceAddFiles) throws IOException { File tempFile = File.createTempFile("serialized-flags-archive", null, temporaryFolder); try (OutputStream out = new BufferedOutputStream(new FileOutputStream(tempFile))) { - var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags/")); + var archive = fromDirectory("system-flags", forceAddFiles); + if (forceAddFiles) + archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); archive.toZip(out); } try (InputStream in = new BufferedInputStream(new FileInputStream(tempFile))) { - SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(in); + SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(in, createZoneRegistryMock()); assertArchiveReturnsCorrectTestFlagDataForTarget(archive); } } @Test void retrieves_correct_flag_data_for_target() { - var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags/")); + retrieves_correct_flag_data_for_target(false); + 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) + archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); assertArchiveReturnsCorrectTestFlagDataForTarget(archive); } @Test void supports_multi_level_flags_directory() { - var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-multi-level/")); + supports_multi_level_flags_directory(false); + 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) + archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "default"); } @Test void duplicated_flagdata_is_detected() { - Throwable exception = assertThrows(IllegalArgumentException.class, () -> { - var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-multi-level-with-duplicated-flagdata/")); - }); + duplicated_flagdata_is_detected(false); + duplicated_flagdata_is_detected(true); + } + + private void duplicated_flagdata_is_detected(boolean forceAddFiles) { + Throwable exception = assertThrows(FlagValidationException.class, () -> { + fromDirectory("system-flags-multi-level-with-duplicated-flagdata", forceAddFiles); + }); assertTrue(exception.getMessage().contains("contains redundant flag data for id 'my-test-flag' already set in another directory!")); } @Test void empty_files_are_handled_as_no_flag_data_for_target() { - var archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags/")); + empty_files_are_handled_as_no_flag_data_for_target(false); + 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) + 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"); assertNoFlagData(archive, FLAG_WITH_EMPTY_DATA, prodUsEast3CfgTarget); @@ -117,35 +148,43 @@ public class SystemFlagsDataArchiveTest { } @Test - void throws_exception_on_non_json_file() { - Throwable exception = assertThrows(IllegalArgumentException.class, () -> { - SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-invalid-file-name/")); + void hv_throws_exception_on_non_json_file() { + Throwable exception = assertThrows(FlagValidationException.class, () -> { + fromDirectory("system-flags-with-invalid-file-name", false); }); - assertTrue(exception.getMessage().contains("Only JSON files are allowed in 'flags/' directory (found 'flags/my-test-flag/file-name-without-dot-json')")); + assertEquals("Invalid flag filename: file-name-without-dot-json", + exception.getMessage()); } @Test void throws_exception_on_unknown_file() { - Throwable exception = assertThrows(IllegalArgumentException.class, () -> { - SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-unknown-file-name/")); - archive.validateAllFilesAreForTargets(SystemName.main, Set.of(mainControllerTarget, prodUsWestCfgTarget)); + Throwable exception = assertThrows(FlagValidationException.class, () -> { + SystemFlagsDataArchive archive = fromDirectory("system-flags-with-unknown-file-name", true); + archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); }); - assertTrue(exception.getMessage().contains("Unknown flag file: flags/my-test-flag/main.prod.unknown-region.json")); + assertEquals("Unknown flag file: flags/my-test-flag/main.prod.unknown-region.json", exception.getMessage()); + } + + @Test + void unknown_region_is_still_zipped() { + // This is useful when the program zipping the files is on a different version than the controller + var archive = fromDirectory("system-flags-with-unknown-file-name", false); + assertTrue(archive.hasFlagData(MY_TEST_FLAG, "main.prod.unknown-region.json")); } @Test void throws_exception_on_unknown_region() { - Throwable exception = assertThrows(IllegalArgumentException.class, () -> { - Path directory = Paths.get("src/test/resources/system-flags-with-unknown-file-name/"); - SystemFlagsDataArchive.fromDirectoryAndSystem(directory, createZoneRegistryMock()); + Throwable exception = assertThrows(FlagValidationException.class, () -> { + var archive = fromDirectory("system-flags-with-unknown-file-name", true); + archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); }); - assertTrue(exception.getMessage().contains("Environment or zone in filename 'main.prod.unknown-region.json' does not exist")); + assertEquals("Unknown flag file: flags/my-test-flag/main.prod.unknown-region.json", exception.getMessage()); } @Test void throws_on_unknown_field() { - Throwable exception = assertThrows(IllegalArgumentException.class, () -> { - SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-unknown-field-name/")); + Throwable exception = assertThrows(FlagValidationException.class, () -> { + 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: @@ -160,7 +199,7 @@ public class SystemFlagsDataArchiveTest { @Test void handles_absent_rule_value() { - SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-null-value/")); + SystemFlagsDataArchive archive = fromDirectory("system-flags-with-null-value", true); // west has null value on first rule List<FlagData> westFlagData = archive.flagData(prodUsWestCfgTarget); @@ -305,17 +344,18 @@ public class SystemFlagsDataArchiveTest { @Test void normalize_json_fail_on_invalid_values() { - failNormalizeJson("application", "\"a.b.c\"", "Application ids must be on the form tenant:application:instance, but was a.b.c"); + 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\"", "Illegal cluster type 'foo'"); - failNormalizeJson("console-user-email", "123", "Non-string value in console-user-email whitelist condition: 123"); - failNormalizeJson("environment", "\"foo\"", "'foo' is not a valid environment identifier"); - failNormalizeJson("hostname", "\"not:a:hostname\"", "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\"", "No enum constant com.yahoo.config.provision.NodeType.footype"); - failNormalizeJson("system", "\"bar\"", "'bar' is not a valid system"); - failNormalizeJson("tenant", "123", "Non-string value in tenant whitelist condition: 123"); - failNormalizeJson("vespa-version", "\"not-a-version\"", "Invalid version component in 'not-a-version'"); + 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"); } @@ -334,14 +374,16 @@ public class SystemFlagsDataArchiveTest { @Test void ignores_files_not_related_to_specified_system_definition() { - ZoneRegistry registry = createZoneRegistryMock(); - Path testDirectory = Paths.get("src/test/resources/system-flags-for-multiple-systems/"); - var archive = SystemFlagsDataArchive.fromDirectoryAndSystem(testDirectory, registry); + var archive = fromDirectory("system-flags-for-multiple-systems", false); assertFlagDataHasValue(archive, MY_TEST_FLAG, cdControllerTarget, "default"); // Would be 'cd.controller' if files for CD system were included assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "default"); 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); + } + @SuppressWarnings("unchecked") // workaround for mocking a method for generic return type private static ZoneRegistry createZoneRegistryMock() { // Cannot use the standard registry mock as it's located in controller-server module @@ -373,7 +415,7 @@ public class SystemFlagsDataArchiveTest { List<FlagData> data = getData(archive, flagId, target); assertEquals(1, data.size()); FlagData flagData = data.get(0); - RawFlag rawFlag = flagData.resolve(FetchVector.fromMap(Map.of())).get(); + RawFlag rawFlag = flagData.resolve(new FetchVector()).get(); assertEquals(String.format("\"%s\"", value), rawFlag.asJson()); } |