diff options
10 files changed, 267 insertions, 164 deletions
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/SystemName.java b/config-provisioning/src/main/java/com/yahoo/config/provision/SystemName.java index e2349f6f63f..c6eecf1e705 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/SystemName.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/SystemName.java @@ -75,4 +75,6 @@ public enum SystemName { return Stream.of(values()).filter(predicate).collect(Collectors.toUnmodifiableSet()); } + public static Set<SystemName> hostedVespa() { return EnumSet.of(main, cd, Public, PublicCd); } + } 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..9426952f57e 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,47 @@ 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(".")) { + + 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 && !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 = force ? 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 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 +205,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 +226,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 +240,18 @@ 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 -> { + ZoneId zoneId = ZoneId.from(zoneIdString); + if (!zones.isEmpty() && !zones.contains(zoneId)) + 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()); } 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 355f06fc753..2c38066eddd 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 @@ -8,6 +8,7 @@ import com.yahoo.vespa.flags.FlagId; import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.integration.ControllerIdentityProvider; +import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagValidationException; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.SystemFlagsDataArchive; import com.yahoo.vespa.hosted.controller.restapi.systemflags.SystemFlagsDeployResult.OperationError; @@ -57,6 +58,12 @@ class SystemFlagsDeployer { } SystemFlagsDeployResult deployFlags(SystemFlagsDataArchive archive, boolean dryRun) { + try { + archive.validateAllFilesAreForTargets(targets); + } catch (FlagValidationException e) { + return new SystemFlagsDeployResult(List.of(OperationError.archiveValidationFailed(e.getMessage()))); + } + Map<FlagsTarget, Future<SystemFlagsDeployResult>> futures = new HashMap<>(); for (FlagsTarget target : targets) { futures.put(target, executor.submit(() -> deployFlags(target, archive.flagData(target), dryRun))); @@ -70,11 +77,6 @@ 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 e9b087690ff..bb285b8b742 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 @@ -10,10 +10,13 @@ import com.yahoo.restapi.JacksonJsonResponse; import com.yahoo.restapi.Path; import com.yahoo.vespa.hosted.controller.api.integration.ControllerIdentityProvider; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; +import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagValidationException; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.FlagsTarget; import com.yahoo.vespa.hosted.controller.api.systemflags.v1.SystemFlagsDataArchive; import com.yahoo.vespa.hosted.controller.restapi.ErrorResponses; +import java.io.InputStream; +import java.util.List; import java.util.concurrent.Executor; /** @@ -27,12 +30,14 @@ public class SystemFlagsHandler extends ThreadedHttpRequestHandler { private static final String API_PREFIX = "/system-flags/v1"; private final SystemFlagsDeployer deployer; + private final ZoneRegistry zoneRegistry; @Inject public SystemFlagsHandler(ZoneRegistry zoneRegistry, ControllerIdentityProvider identityProvider, Executor executor) { super(executor); + this.zoneRegistry = zoneRegistry; this.deployer = new SystemFlagsDeployer(identityProvider, zoneRegistry.system(), FlagsTarget.getAllTargetsInSystem(zoneRegistry, true)); } @@ -57,12 +62,22 @@ public class SystemFlagsHandler extends ThreadedHttpRequestHandler { if (!contentType.equalsIgnoreCase("application/zip")) { return ErrorResponse.badRequest("Invalid content type: " + contentType); } - SystemFlagsDataArchive archive = SystemFlagsDataArchive.fromZip(request.getData()); - SystemFlagsDeployResult result = deployer.deployFlags(archive, dryRun); + SystemFlagsDeployResult result = deploy(request.getData(), dryRun); return new JacksonJsonResponse<>(200, result.toWire()); } catch (Exception e) { return ErrorResponses.logThrowing(request, log, e); } } + private SystemFlagsDeployResult deploy(InputStream zipStream, boolean dryRun) { + SystemFlagsDataArchive archive; + try { + archive = SystemFlagsDataArchive.fromZip(zipStream, zoneRegistry); + } catch (FlagValidationException e) { + return new SystemFlagsDeployResult(List.of(SystemFlagsDeployResult.OperationError.archiveValidationFailed(e.getMessage()))); + } + + return deployer.deployFlags(archive, dryRun); + } + } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 812fb65d15a..c6d141764fb 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -302,14 +302,6 @@ public class Flags { "Takes effect at redeployment", APPLICATION_ID); - public static final UnboundBooleanFlag NODE_ADMIN_TENANT_SERVICE_REGISTRY = defineFeatureFlag( - "node-admin-tenant-service-registry", true, - List.of("olaa"), "2023-04-12", "2023-08-07", - "Whether AthenzCredentialsMaintainer in node-admin should create tenant service identity certificate", - "Takes effect on next tick", - HOSTNAME, VESPA_VERSION, APPLICATION_ID - ); - public static final UnboundBooleanFlag ENABLE_CROWDSTRIKE = defineFeatureFlag( "enable-crowdstrike", true, List.of("andreer"), "2023-04-13", "2023-08-31", "Whether to enable CrowdStrike.", "Takes effect on next host admin tick", diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java index b6ec0ebbd94..830b7f4ed33 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/identity/AthenzCredentialsMaintainer.java @@ -80,7 +80,6 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { private final String certificateDnsSuffix; private final ServiceIdentityProvider hostIdentityProvider; private final IdentityDocumentClient identityDocumentClient; - private final BooleanFlag tenantServiceIdentityFlag; // Used as an optimization to ensure ZTS is not DDoS'ed on continuously failing refresh attempts private final Map<ContainerName, Instant> lastRefreshAttempt = new ConcurrentHashMap<>(); @@ -89,7 +88,6 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { ConfigServerInfo configServerInfo, String certificateDnsSuffix, ServiceIdentityProvider hostIdentityProvider, - FlagSource flagSource, Timer timer) { this.ztsTrustStorePath = ztsTrustStorePath; this.certificateDnsSuffix = certificateDnsSuffix; @@ -99,7 +97,6 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { hostIdentityProvider, new AthenzIdentityVerifier(Set.of(configServerInfo.getConfigServerIdentity()))); this.timer = timer; - this.tenantServiceIdentityFlag = Flags.NODE_ADMIN_TENANT_SERVICE_REGISTRY.bindTo(flagSource); } public boolean converge(NodeAgentContext context) { @@ -109,11 +106,7 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { if (context.zone().getSystemName().isPublic()) return modified; - if (shouldWriteTenantServiceIdentity(context)) { - modified |= maintain(context, TENANT); - } else { - modified |= deleteTenantCredentials(context); - } + modified |= maintain(context, TENANT); return modified; } @@ -268,24 +261,6 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { return "node-certificate"; } - private boolean deleteTenantCredentials(NodeAgentContext context) { - var siaDirectory = context.paths().of(CONTAINER_SIA_DIRECTORY, context.users().vespa()); - var identityDocumentFile = siaDirectory.resolve(TENANT.getIdentityDocument()); - if (!Files.exists(identityDocumentFile)) return false; - return getAthenzIdentity(context, TENANT, identityDocumentFile).map(athenzIdentity -> { - var privateKeyFile = (ContainerPath) SiaUtils.getPrivateKeyFile(siaDirectory, athenzIdentity); - var certificateFile = (ContainerPath) SiaUtils.getCertificateFile(siaDirectory, athenzIdentity); - try { - var modified = Files.deleteIfExists(identityDocumentFile); - modified |= Files.deleteIfExists(privateKeyFile); - modified |= Files.deleteIfExists(certificateFile); - return modified; - } catch (IOException e) { - throw new UncheckedIOException(e); - } - }).orElse(false); - } - private boolean shouldRefreshCredentials(Duration age) { return age.compareTo(REFRESH_PERIOD) >= 0; } @@ -399,16 +374,6 @@ public class AthenzCredentialsMaintainer implements CredentialsMaintainer { } } - private boolean shouldWriteTenantServiceIdentity(NodeAgentContext context) { - var version = context.node().currentVespaVersion() - .orElse(context.node().wantedVespaVersion().orElse(Version.emptyVersion)); - var appId = context.node().owner().orElse(ApplicationId.defaultId()); - return tenantServiceIdentityFlag - .with(FetchVector.Dimension.VESPA_VERSION, version.toFullString()) - .with(FetchVector.Dimension.APPLICATION_ID, appId.serializedForm()) - .value(); - } - private void copyCredsToLegacyPath(NodeAgentContext context, ContainerPath privateKeyFile, ContainerPath certificateFile) throws IOException { var legacySiaDirectory = context.paths().of(LEGACY_SIA_DIRECTORY, context.users().vespa()); var keysDirectory = legacySiaDirectory.resolve("keys"); |