diff options
8 files changed, 490 insertions, 214 deletions
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 7403f0a1b01..fbf3a5d9a03 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 @@ -142,7 +142,7 @@ public interface FlagsTarget { var fetchVector = new FetchVector(); if (!flagDimensions.contains(CLOUD)) fetchVector = fetchVector.with(CLOUD, cloud.value()); if (!flagDimensions.contains(ENVIRONMENT)) fetchVector = fetchVector.with(ENVIRONMENT, virtualZoneId.environment().value()); - if (!flagDimensions.contains(SYSTEM)) fetchVector = fetchVector.with(SYSTEM, system.value()); + fetchVector = fetchVector.with(SYSTEM, system.value()); if (!flagDimensions.contains(ZONE_ID)) fetchVector = fetchVector.with(ZONE_ID, virtualZoneId.value()); return fetchVector.isEmpty() ? data : data.partialResolve(fetchVector); } diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java index 577769baf1e..c6f1d96ed43 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java @@ -1,6 +1,7 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.api.systemflags.v1; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; @@ -18,8 +19,10 @@ import com.yahoo.config.provision.zone.ZoneId; import com.yahoo.text.JSON; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; +import com.yahoo.vespa.flags.json.Condition; import com.yahoo.vespa.flags.json.DimensionHelper; import com.yahoo.vespa.flags.json.FlagData; +import com.yahoo.vespa.flags.json.RelationalCondition; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import java.io.BufferedInputStream; @@ -49,6 +52,7 @@ import java.util.zip.ZipOutputStream; import static com.yahoo.config.provision.CloudName.AWS; import static com.yahoo.config.provision.CloudName.GCP; import static com.yahoo.config.provision.CloudName.YAHOO; +import static com.yahoo.vespa.flags.FetchVector.Dimension.SYSTEM; import static com.yahoo.yolean.Exceptions.uncheck; /** @@ -82,8 +86,8 @@ public class SystemFlagsDataArchive { String name = entry.getName(); if (!entry.isDirectory() && name.startsWith("flags/")) { Path filePath = Paths.get(name); - String rawData = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8); - addFile(builder, rawData, filePath, zoneRegistry, true); + String fileContent = new String(zipIn.readAllBytes(), StandardCharsets.UTF_8); + builder.maybeAddFile(filePath, fileContent, zoneRegistry, true); } } return builder.build(); @@ -92,7 +96,7 @@ public class SystemFlagsDataArchive { } } - public static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry zoneRegistry, boolean forceAddFiles) { + public static SystemFlagsDataArchive fromDirectory(Path directory, ZoneRegistry zoneRegistry, boolean simulateInController) { Path root = directory.toAbsolutePath(); Path flagsDirectory = directory.resolve("flags"); if (!Files.isDirectory(flagsDirectory)) { @@ -103,8 +107,8 @@ public class SystemFlagsDataArchive { directoryStream.forEach(path -> { Path relativePath = root.relativize(path.toAbsolutePath()); if (Files.isRegularFile(path)) { - String rawData = uncheck(() -> Files.readString(path, StandardCharsets.UTF_8)); - addFile(builder, rawData, relativePath, zoneRegistry, forceAddFiles); + String fileContent = uncheck(() -> Files.readString(path, StandardCharsets.UTF_8)); + builder.maybeAddFile(relativePath, fileContent, zoneRegistry, simulateInController); } }); return builder.build(); @@ -168,114 +172,119 @@ public class SystemFlagsDataArchive { return files.getOrDefault(flagId, Map.of()).containsKey(filename); } - private static void addFile(Builder builder, String rawData, Path filePath, ZoneRegistry zoneRegistry, boolean force) { - String filename = filePath.getFileName().toString(); - - if (filename.startsWith(".")) - return; // Ignore files starting with '.' - - if (!force && !FlagsTarget.filenameForSystem(filename, zoneRegistry.system())) - return; // Ignore files for other systems - - FlagId directoryDeducedFlagId = new FlagId(filePath.getName(filePath.getNameCount()-2).toString()); - FlagData flagData; - if (rawData.isBlank()) { - flagData = new FlagData(directoryDeducedFlagId); - } else { - Set<ZoneId> zones = force ? Stream.concat(Stream.of(ZoneId.ofVirtualControllerZone()), - zoneRegistry.zones().all().zones().stream().map(ZoneApi::getVirtualId)) - .collect(Collectors.toSet()) - : Set.of(); - String normalizedRawData = normalizeJson(rawData, zones); - flagData = FlagData.deserialize(normalizedRawData); - if (!directoryDeducedFlagId.equals(flagData.id())) { - throw new FlagValidationException("Flag data file with flag id '%s' in directory for '%s'" - .formatted(flagData.id(), directoryDeducedFlagId.toString())); - } - - String serializedData = flagData.serializeToJson(); - if (!JSON.equals(serializedData, normalizedRawData)) { - throw new FlagValidationException(""" - %s contains unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: - %s - but deserializing this ended up with: - %s - These fields may be spelled wrong, or remove them? - See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax - """.formatted(filePath, normalizedRawData, serializedData)); - } - } - - if (builder.hasFile(filename, flagData)) { - throw new FlagValidationException("Flag data file in '%s' contains redundant flag data for id '%s' already set in another directory!" - .formatted(filePath, flagData.id())); - } - - builder.addFile(filename, flagData); - } - - static String normalizeJson(String json, Set<ZoneId> zones) { - JsonNode root = uncheck(() -> mapper.readTree(json)); - removeCommentsRecursively(root); - removeNullRuleValues(root); - verifyValues(root, zones); - return root.toString(); - } - - private static void verifyValues(JsonNode root, Set<ZoneId> zones) { - var cursor = new JsonAccessor(root); - cursor.get("rules").forEachArrayElement(rule -> rule.get("conditions").forEachArrayElement(condition -> { - FetchVector.Dimension dimension = DimensionHelper - .fromWire(condition.get("dimension") - .asString() - .orElseThrow(() -> new FlagValidationException("Invalid dimension in condition: " + condition))); - switch (dimension) { - case APPLICATION_ID -> validateStringValues(condition, ApplicationId::fromSerializedForm); - case CONSOLE_USER_EMAIL -> validateStringValues(condition, email -> {}); - case CLOUD -> validateStringValues(condition, cloud -> { - if (!Set.of(YAHOO, AWS, GCP).contains(CloudName.from(cloud))) - throw new FlagValidationException("Unknown cloud: " + cloud); - }); - case CLUSTER_ID -> validateStringValues(condition, ClusterSpec.Id::from); - case CLUSTER_TYPE -> validateStringValues(condition, ClusterSpec.Type::from); - case ENVIRONMENT -> validateStringValues(condition, Environment::from); - case HOSTNAME -> validateStringValues(condition, HostName::of); - case NODE_TYPE -> validateStringValues(condition, NodeType::valueOf); - case SYSTEM -> validateStringValues(condition, system -> { + private static void validateSystems(FlagData flagData) throws FlagValidationException { + flagData.rules().forEach(rule -> rule.conditions().forEach(condition -> { + if (condition.dimension() == SYSTEM) { + validateConditionValues(condition, system -> { if (!SystemName.hostedVespa().contains(SystemName.from(system))) throw new FlagValidationException("Unknown system: " + system); }); - case TENANT_ID -> validateStringValues(condition, TenantName::from); - case VESPA_VERSION -> validateStringValues(condition, versionString -> { - if (Version.fromString(versionString).getMajor() < 8) - throw new FlagValidationException("Major Vespa version must be at least 8: " + versionString); - }); - case ZONE_ID -> validateStringValues(condition, zoneIdString -> { - ZoneId zoneId = ZoneId.from(zoneIdString); - if (!zones.isEmpty() && !zones.contains(zoneId)) - throw new FlagValidationException("Unknown zone: " + zoneIdString); - }); } })); } - private static void validateStringValues(JsonAccessor condition, Consumer<String> valueValidator) { - condition.get("values").forEachArrayElement(conditionValue -> { - String value = conditionValue.asString() - .orElseThrow(() -> { - String dimension = condition.get("dimension").asString().orElseThrow(); - String type = condition.get("type").asString().orElseThrow(); - return new FlagValidationException("Non-string %s in %s condition: %s".formatted( - dimension, type, conditionValue)); - }); + private static void validateForSystem(FlagData flagData, ZoneRegistry zoneRegistry, boolean inController) throws FlagValidationException { + Set<ZoneId> zones = inController ? + zoneRegistry.zonesIncludingSystem().all().zones().stream().map(ZoneApi::getVirtualId).collect(Collectors.toSet()) : + null; + + flagData.rules().forEach(rule -> rule.conditions().forEach(condition -> { + int force_switch_expression_dummy = switch (condition.type()) { + case RELATIONAL -> switch (condition.dimension()) { + case APPLICATION_ID, CLOUD, CLUSTER_ID, CLUSTER_TYPE, CONSOLE_USER_EMAIL, ENVIRONMENT, + HOSTNAME, NODE_TYPE, SYSTEM, TENANT_ID, ZONE_ID -> + throw new FlagValidationException(condition.type().toWire() + " " + + DimensionHelper.toWire(condition.dimension()) + + " condition is not supported"); + case VESPA_VERSION -> { + RelationalCondition rCond = RelationalCondition.create(condition.toCreateParams()); + Version version = Version.fromString(rCond.relationalPredicate().rightOperand()); + if (version.getMajor() < 8) + throw new FlagValidationException("Major Vespa version must be at least 8: " + version); + yield 0; + } + }; + + case WHITELIST, BLACKLIST -> switch (condition.dimension()) { + case APPLICATION_ID -> validateConditionValues(condition, ApplicationId::fromSerializedForm); + case CONSOLE_USER_EMAIL -> validateConditionValues(condition, email -> { + if (!email.contains("@")) + throw new FlagValidationException("Invalid email address: " + email); + }); + case CLOUD -> validateConditionValues(condition, cloud -> { + if (!Set.of(YAHOO, AWS, GCP).contains(CloudName.from(cloud))) + throw new FlagValidationException("Unknown cloud: " + cloud); + }); + case CLUSTER_ID -> validateConditionValues(condition, ClusterSpec.Id::from); + case CLUSTER_TYPE -> validateConditionValues(condition, ClusterSpec.Type::from); + case ENVIRONMENT -> validateConditionValues(condition, Environment::from); + case HOSTNAME -> validateConditionValues(condition, HostName::of); + case NODE_TYPE -> validateConditionValues(condition, NodeType::valueOf); + case SYSTEM -> throw new IllegalStateException("Flag data contains system dimension"); + case TENANT_ID -> validateConditionValues(condition, TenantName::from); + case VESPA_VERSION -> throw new FlagValidationException(condition.type().toWire() + " " + + DimensionHelper.toWire(condition.dimension()) + + " condition is not supported"); + case ZONE_ID -> validateConditionValues(condition, zoneIdString -> { + ZoneId zoneId = ZoneId.from(zoneIdString); + if (inController && !zones.contains(zoneId)) + throw new FlagValidationException("Unknown zone: " + zoneIdString); + }); + }; + }; + })); + } + + private static int validateConditionValues(Condition condition, Consumer<String> valueValidator) { + condition.toCreateParams().values().forEach(value -> { try { valueValidator.accept(value); } catch (IllegalArgumentException e) { - String dimension = condition.get("dimension").asString().orElseThrow(); - String type = condition.get("type").asString().orElseThrow(); + String dimension = DimensionHelper.toWire(condition.dimension()); + String type = condition.type().toWire(); throw new FlagValidationException("Invalid %s '%s' in %s condition: %s".formatted(dimension, value, type, e.getMessage())); } }); + + return 0; // dummy to force switch expression + } + + private static FlagData parseFlagData(FlagId flagId, String fileContent, ZoneRegistry zoneRegistry, boolean inController) { + if (fileContent.isBlank()) return new FlagData(flagId); + + final JsonNode root; + try { + root = mapper.readTree(fileContent); + } catch (JsonProcessingException e) { + throw new FlagValidationException("Invalid JSON: " + e.getMessage()); + } + + removeCommentsRecursively(root); + removeNullRuleValues(root); + String normalizedRawData = root.toString(); + FlagData flagData = FlagData.deserialize(normalizedRawData); + + if (!flagId.equals(flagData.id())) + throw new FlagValidationException("Flag ID specified in file (%s) doesn't match the directory name (%s)" + .formatted(flagData.id(), flagId.toString())); + + String serializedData = flagData.serializeToJson(); + if (!JSON.equals(serializedData, normalizedRawData)) + throw new FlagValidationException(""" + Unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: + %s + but deserializing this ended up with: + %s + These fields may be spelled wrong, or remove them? + See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax + """.formatted(normalizedRawData, serializedData)); + + validateSystems(flagData); + flagData = flagData.partialResolve(new FetchVector().with(SYSTEM, zoneRegistry.system().value())); + + validateForSystem(flagData, zoneRegistry, inController); + + return flagData; } private static void removeCommentsRecursively(JsonNode node) { @@ -312,56 +321,46 @@ public class SystemFlagsDataArchive { public Builder() {} - public Builder addFile(String filename, FlagData data) { - files.computeIfAbsent(data.id(), k -> new TreeMap<>()).put(filename, data); - return this; - } + boolean maybeAddFile(Path filePath, String fileContent, ZoneRegistry zoneRegistry, boolean inController) { + String filename = filePath.getFileName().toString(); - public boolean hasFile(String filename, FlagData data) { - return files.containsKey(data.id()) && files.get(data.id()).containsKey(filename); - } - - public SystemFlagsDataArchive build() { - Map<FlagId, Map<String, FlagData>> copy = new TreeMap<>(); - files.forEach((flagId, map) -> copy.put(flagId, new TreeMap<>(map))); - return new SystemFlagsDataArchive(copy); - } + if (filename.startsWith(".")) + return false; // Ignore files starting with '.' - } + if (!inController && !FlagsTarget.filenameForSystem(filename, zoneRegistry.system())) + return false; // Ignore files for other systems - private static class JsonAccessor { - private final JsonNode jsonNode; + FlagId directoryDeducedFlagId = new FlagId(filePath.getName(filePath.getNameCount()-2).toString()); - public JsonAccessor(JsonNode jsonNode) { - this.jsonNode = jsonNode; - } + if (hasFile(filename, directoryDeducedFlagId)) + throw new FlagValidationException("Flag data file in '%s' contains redundant flag data for id '%s' already set in another directory!" + .formatted(filePath, directoryDeducedFlagId)); - public JsonAccessor get(String fieldName) { - if (jsonNode == null) { - return this; - } else { - return new JsonAccessor(jsonNode.get(fieldName)); + final FlagData flagData; + try { + flagData = parseFlagData(directoryDeducedFlagId, fileContent, zoneRegistry, inController); + } catch (FlagValidationException e) { + throw new FlagValidationException("In file " + filePath + ": " + e.getMessage()); } - } - public Optional<String> asString() { - return jsonNode != null && jsonNode.isTextual() ? Optional.of(jsonNode.textValue()) : Optional.empty(); + addFile(filename, flagData); + return true; } - public void forEachArrayElement(Consumer<JsonAccessor> consumer) { - if (jsonNode != null && jsonNode.isArray()) { - jsonNode.forEach(jsonNodeElement -> consumer.accept(new JsonAccessor(jsonNodeElement))); - } + public Builder addFile(String filename, FlagData data) { + files.computeIfAbsent(data.id(), k -> new TreeMap<>()).put(filename, data); + return this; } - /** Returns true if this (JsonNode) is a string and equal to value. */ - public boolean isEqualTo(String value) { - return jsonNode != null && jsonNode.isTextual() && Objects.equals(jsonNode.textValue(), value); + public boolean hasFile(String filename, FlagId id) { + return files.containsKey(id) && files.get(id).containsKey(filename); } - @Override - public String toString() { - return jsonNode == null ? "undefined" : jsonNode.toString(); + public SystemFlagsDataArchive build() { + Map<FlagId, Map<String, FlagData>> copy = new TreeMap<>(); + files.forEach((flagId, map) -> copy.put(flagId, new TreeMap<>(map))); + return new SystemFlagsDataArchive(copy); } + } } 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 2d0374dc888..759f21579d4 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 @@ -14,6 +14,7 @@ import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; import com.yahoo.vespa.flags.RawFlag; +import com.yahoo.vespa.flags.json.Condition; import com.yahoo.vespa.flags.json.FlagData; import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry; import org.junit.jupiter.api.Test; @@ -28,10 +29,12 @@ 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.Optional; import java.util.Set; +import java.util.stream.Stream; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -78,11 +81,11 @@ public class SystemFlagsDataArchiveTest { can_serialize_and_deserialize_archive(true); } - private void can_serialize_and_deserialize_archive(boolean forceAddFiles) throws IOException { + private void can_serialize_and_deserialize_archive(boolean simulateInController) throws IOException { File tempFile = File.createTempFile("serialized-flags-archive", null, temporaryFolder); try (OutputStream out = new BufferedOutputStream(new FileOutputStream(tempFile))) { - var archive = fromDirectory("system-flags", forceAddFiles); - if (forceAddFiles) + var archive = fromDirectory("system-flags", simulateInController); + if (simulateInController) archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); archive.toZip(out); } @@ -98,9 +101,9 @@ public class SystemFlagsDataArchiveTest { 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) + private void retrieves_correct_flag_data_for_target(boolean simulateInController) { + var archive = fromDirectory("system-flags", simulateInController); + if (simulateInController) archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); assertArchiveReturnsCorrectTestFlagDataForTarget(archive); } @@ -111,9 +114,9 @@ public class SystemFlagsDataArchiveTest { 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) + private void supports_multi_level_flags_directory(boolean simulateInController) { + var archive = fromDirectory("system-flags-multi-level", simulateInController); + if (simulateInController) archive.validateAllFilesAreForTargets(Set.of(mainControllerTarget, prodUsWestCfgTarget)); assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "default"); } @@ -124,9 +127,9 @@ public class SystemFlagsDataArchiveTest { duplicated_flagdata_is_detected(true); } - private void duplicated_flagdata_is_detected(boolean forceAddFiles) { + private void duplicated_flagdata_is_detected(boolean simulateInController) { Throwable exception = assertThrows(FlagValidationException.class, () -> { - fromDirectory("system-flags-multi-level-with-duplicated-flagdata", forceAddFiles); + fromDirectory("system-flags-multi-level-with-duplicated-flagdata", simulateInController); }); assertTrue(exception.getMessage().contains("contains redundant flag data for id 'my-test-flag' already set in another directory!")); } @@ -137,9 +140,9 @@ public class SystemFlagsDataArchiveTest { 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) + private void empty_files_are_handled_as_no_flag_data_for_target(boolean simulateInController) { + var archive = fromDirectory("system-flags", simulateInController); + if (simulateInController) 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"); @@ -187,7 +190,7 @@ public class SystemFlagsDataArchiveTest { 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: + In file flags/my-test-flag/main.prod.us-west-1.json: Unknown non-comment fields or rules with null values: after removing any comment fields the JSON is: {"id":"my-test-flag","rules":[{"condition":[{"type":"whitelist","dimension":"hostname","values":["foo.com"]}],"value":"default"}]} but deserializing this ended up with: {"id":"my-test-flag","rules":[{"value":"default"}]} @@ -218,6 +221,7 @@ public class SystemFlagsDataArchiveTest { void remove_comments_and_null_value_in_rules() { assertTrue(JSON.equals(""" { + "id": "foo", "rules": [ { "conditions": [ @@ -249,8 +253,9 @@ public class SystemFlagsDataArchiveTest { } ] }""", - SystemFlagsDataArchive.normalizeJson(""" + normalizeJson(""" { + "id": "foo", "comment": "bar", "rules": [ { @@ -289,83 +294,91 @@ public class SystemFlagsDataArchiveTest { "value": true } ] - }""", Set.of(ZoneId.from("prod.us-west-1"))))); + }"""))); + } + + private static String normalizeJson(String json) { + SystemFlagsDataArchive.Builder builder = new SystemFlagsDataArchive.Builder(); + assertTrue(builder.maybeAddFile(Path.of("flags/temporary/foo/default.json"), json, createZoneRegistryMock(), true)); + List<FlagData> flagData = builder.build().flagData(prodUsWestCfgTarget); + assertEquals(1, flagData.size()); + return JSON.canonical(flagData.get(0).serializeToJson()); } @Test void normalize_json_succeed_on_valid_values() { - normalizeJson("application", "\"a:b:c\""); - normalizeJson("cloud", "\"yahoo\""); - normalizeJson("cloud", "\"aws\""); - normalizeJson("cloud", "\"gcp\""); - normalizeJson("cluster-id", "\"some-id\""); - normalizeJson("cluster-type", "\"admin\""); - normalizeJson("cluster-type", "\"container\""); - normalizeJson("cluster-type", "\"content\""); - normalizeJson("console-user-email", "\"name@domain.com\""); - normalizeJson("environment", "\"prod\""); - normalizeJson("environment", "\"staging\""); - normalizeJson("environment", "\"test\""); - normalizeJson("hostname", "\"2080046-v6-11.ostk.bm2.prod.gq1.yahoo.com\""); - normalizeJson("node-type", "\"tenant\""); - normalizeJson("node-type", "\"host\""); - normalizeJson("node-type", "\"config\""); - normalizeJson("node-type", "\"host\""); - normalizeJson("system", "\"main\""); - normalizeJson("system", "\"public\""); - normalizeJson("tenant", "\"vespa\""); - normalizeJson("vespa-version", "\"8.201.13\""); - normalizeJson("zone", "\"prod.us-west-1\"", Set.of(ZoneId.from("prod.us-west-1"))); - } - - private void normalizeJson(String dimension, String jsonValue) { - normalizeJson(dimension, jsonValue, Set.of()); - } - - private void normalizeJson(String dimension, String jsonValue, Set<ZoneId> zones) { - SystemFlagsDataArchive.normalizeJson(""" + addFile(Condition.Type.WHITELIST, "application", "a:b:c"); + addFile(Condition.Type.WHITELIST, "cloud", "yahoo"); + addFile(Condition.Type.WHITELIST, "cloud", "aws"); + addFile(Condition.Type.WHITELIST, "cloud", "gcp"); + addFile(Condition.Type.WHITELIST, "cluster-id", "some-id"); + addFile(Condition.Type.WHITELIST, "cluster-type", "admin"); + addFile(Condition.Type.WHITELIST, "cluster-type", "container"); + addFile(Condition.Type.WHITELIST, "cluster-type", "content"); + addFile(Condition.Type.WHITELIST, "console-user-email", "name@domain.com"); + addFile(Condition.Type.WHITELIST, "environment", "prod"); + addFile(Condition.Type.WHITELIST, "environment", "staging"); + addFile(Condition.Type.WHITELIST, "environment", "test"); + addFile(Condition.Type.WHITELIST, "hostname", "2080046-v6-11.ostk.bm2.prod.gq1.yahoo.com"); + addFile(Condition.Type.WHITELIST, "node-type", "tenant"); + addFile(Condition.Type.WHITELIST, "node-type", "host"); + addFile(Condition.Type.WHITELIST, "node-type", "config"); + addFile(Condition.Type.WHITELIST, "node-type", "host"); + addFile(Condition.Type.WHITELIST, "system", "main"); + addFile(Condition.Type.WHITELIST, "system", "public"); + addFile(Condition.Type.WHITELIST, "tenant", "vespa"); + addFile(Condition.Type.RELATIONAL, "vespa-version", ">=8.201.13"); + addFile(Condition.Type.WHITELIST, "zone", "prod.us-west-1"); + } + + private void addFile(Condition.Type type, String dimension, String jsonValue) { + SystemFlagsDataArchive.Builder builder = new SystemFlagsDataArchive.Builder(); + + String valuesField = type == Condition.Type.RELATIONAL ? + "\"predicate\": \"%s\"".formatted(jsonValue) : + "\"values\": [ \"%s\" ]".formatted(jsonValue); + + assertTrue(builder.maybeAddFile(Path.of("flags/temporary/foo/default.json"), """ { "id": "foo", "rules": [ { "conditions": [ { - "type": "whitelist", + "type": "%s", "dimension": "%s", - "values": [ %s ] + %s } ], "value": true } ] } - """.formatted(dimension, jsonValue), zones); + """.formatted(type.toWire(), dimension, valuesField), + createZoneRegistryMock(), + true)); } @Test void normalize_json_fail_on_invalid_values() { - 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\"", "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"); - } - - private void failNormalizeJson(String dimension, String jsonValue, String expectedExceptionMessage) { - failNormalizeJson(dimension, jsonValue, Set.of(), expectedExceptionMessage); - } - - private void failNormalizeJson(String dimension, String jsonValue, Set<ZoneId> zones, String expectedExceptionMessage) { + failAddFile(Condition.Type.WHITELIST, "application", "a.b.c", "In file flags/temporary/foo/default.json: Invalid application 'a.b.c' in whitelist condition: Application ids must be on the form tenant:application:instance, but was a.b.c"); + failAddFile(Condition.Type.WHITELIST, "cloud", "foo", "In file flags/temporary/foo/default.json: Unknown cloud: foo"); + // cluster-id: any String is valid + failAddFile(Condition.Type.WHITELIST, "cluster-type", "foo", "In file flags/temporary/foo/default.json: Invalid cluster-type 'foo' in whitelist condition: Illegal cluster type 'foo'"); + failAddFile(Condition.Type.WHITELIST, "console-user-email", "not-valid-email-address", "In file flags/temporary/foo/default.json: Invalid email address: not-valid-email-address"); + failAddFile(Condition.Type.WHITELIST, "environment", "foo", "In file flags/temporary/foo/default.json: Invalid environment 'foo' in whitelist condition: 'foo' is not a valid environment identifier"); + failAddFile(Condition.Type.WHITELIST, "hostname", "not:a:hostname", "In file flags/temporary/foo/default.json: 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'"); + failAddFile(Condition.Type.WHITELIST, "node-type", "footype", "In file flags/temporary/foo/default.json: Invalid node-type 'footype' in whitelist condition: No enum constant com.yahoo.config.provision.NodeType.footype"); + failAddFile(Condition.Type.WHITELIST, "system", "bar", "In file flags/temporary/foo/default.json: Invalid system 'bar' in whitelist condition: 'bar' is not a valid system"); + failAddFile(Condition.Type.WHITELIST, "tenant", "a tenant", "In file flags/temporary/foo/default.json: Invalid tenant 'a tenant' in whitelist condition: tenant name must match '[a-zA-Z0-9_-]{1,256}', but got: 'a tenant'"); + failAddFile(Condition.Type.WHITELIST, "vespa-version", "not-a-version", "In file flags/temporary/foo/default.json: whitelist vespa-version condition is not supported"); + failAddFile(Condition.Type.RELATIONAL, "vespa-version", ">7.1.2", "In file flags/temporary/foo/default.json: Major Vespa version must be at least 8: 7.1.2"); + failAddFile(Condition.Type.WHITELIST, "zone", "dev.%illegal", "In file flags/temporary/foo/default.json: Invalid zone 'dev.%illegal' in whitelist condition: region name must match '[a-z]([a-z0-9-]*[a-z0-9])*', but got: '%illegal'"); + } + + private void failAddFile(Condition.Type type, String dimension, String jsonValue, String expectedExceptionMessage) { try { - normalizeJson(dimension, jsonValue, zones); + addFile(type, dimension, jsonValue); fail(); } catch (RuntimeException e) { assertEquals(expectedExceptionMessage, e.getMessage()); @@ -380,8 +393,8 @@ public class SystemFlagsDataArchiveTest { 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); + private SystemFlagsDataArchive fromDirectory(String testDirectory, boolean simulateInController) { + return SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/" + testDirectory), createZoneRegistryMock(), simulateInController); } @SuppressWarnings("unchecked") // workaround for mocking a method for generic return type @@ -396,12 +409,21 @@ public class SystemFlagsDataArchiveTest { when(registryMock.systemZone()).thenReturn(zoneApi); when(registryMock.getConfigServerVipUri(any())).thenReturn(URI.create("http://localhost:8080/")); when(registryMock.getConfigServerHttpsIdentity(any())).thenReturn(new AthenzService("domain", "servicename")); + ZoneList zones = mockZoneList("prod.us-west-1", "prod.us-east-3"); + when(registryMock.zones()).thenReturn(zones); + ZoneList zonesIncludingSystem = mockZoneList("prod.us-west-1", "prod.us-east-3", "prod.controller"); + when(registryMock.zonesIncludingSystem()).thenReturn(zonesIncludingSystem); + return registryMock; + } + + @SuppressWarnings("unchecked") // workaround for mocking a method for generic return type + private static ZoneList mockZoneList(String... zones) { ZoneList zoneListMock = mock(ZoneList.class); when(zoneListMock.reachable()).thenReturn(zoneListMock); when(zoneListMock.all()).thenReturn(zoneListMock); - when(zoneListMock.zones()).thenReturn((List)List.of(new SimpleZone("prod.us-west-1"), new SimpleZone("prod.us-east-3"))); - when(registryMock.zones()).thenReturn(zoneListMock); - return registryMock; + List<? extends ZoneApi> zoneList = Stream.of(zones).map(SimpleZone::new).toList(); + when(zoneListMock.zones()).thenReturn((List) zoneList); + return zoneListMock; } private static void assertArchiveReturnsCorrectTestFlagDataForTarget(SystemFlagsDataArchive archive) { diff --git a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java index 749f6830870..031b61c8e7e 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/json/RelationalCondition.java @@ -68,6 +68,10 @@ public class RelationalCondition implements Condition { return fetchVector.getValue(dimension).map(predicate::test).orElse(false); } + public RelationalPredicate relationalPredicate() { + return relationalPredicate; + } + @Override public WireCondition toWire() { var condition = new WireCondition(); diff --git a/parent/pom.xml b/parent/pom.xml index df20b94ec79..56c896d57cc 100644 --- a/parent/pom.xml +++ b/parent/pom.xml @@ -1161,6 +1161,21 @@ <artifactId>checker-qual</artifactId> <version>3.30.0</version> </dependency> + <dependency> + <groupId>com.google.http-client</groupId> + <artifactId>google-http-client-apache-v2</artifactId> + <version>1.43.3</version> + </dependency> + <dependency> + <groupId>com.google.http-client</groupId> + <artifactId>google-http-client</artifactId> + <version>1.43.3</version> + </dependency> + <dependency> + <groupId>com.google.auth</groupId> + <artifactId>google-auth-library-oauth2-http</artifactId> + <version>1.19.0</version> + </dependency> </dependencies> </dependencyManagement> diff --git a/vespa-athenz/pom.xml b/vespa-athenz/pom.xml index 7c3c982af84..66b369f00fe 100644 --- a/vespa-athenz/pom.xml +++ b/vespa-athenz/pom.xml @@ -275,6 +275,52 @@ <groupId>commons-codec</groupId> <artifactId>commons-codec</artifactId> </dependency> + <dependency> + <groupId>com.google.http-client</groupId> + <artifactId>google-http-client-apache-v2</artifactId> + <exclusions> + <exclusion> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpcore</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpclient</artifactId> + </exclusion> + <exclusion> + <groupId>com.google.http-client</groupId> + <artifactId>google-http-client</artifactId> + </exclusion> + </exclusions> + </dependency> + <dependency> + <groupId>com.google.http-client</groupId> + <artifactId>google-http-client</artifactId> + <exclusions> + <exclusion> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpcore</artifactId> + </exclusion> + <exclusion> + <groupId>org.apache.httpcomponents</groupId> + <artifactId>httpclient</artifactId> + </exclusion> + <exclusion> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + </exclusion> + </exclusions> + </dependency> + <dependency> + <groupId>com.google.auth</groupId> + <artifactId>google-auth-library-oauth2-http</artifactId> + <exclusions> + <exclusion> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + </exclusion> + </exclusions> + </dependency> </dependencies> <build> diff --git a/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/gcp/GcpCredentials.java b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/gcp/GcpCredentials.java new file mode 100644 index 00000000000..bbdc3c2b372 --- /dev/null +++ b/vespa-athenz/src/main/java/com/yahoo/vespa/athenz/gcp/GcpCredentials.java @@ -0,0 +1,180 @@ +package com.yahoo.vespa.athenz.gcp; + +import com.google.api.client.http.apache.v2.ApacheHttpTransport; +import com.google.auth.http.HttpTransportFactory; +import com.google.auth.oauth2.ExternalAccountCredentials; +import com.yahoo.security.token.TokenDomain; +import com.yahoo.security.token.TokenGenerator; +import com.yahoo.slime.Cursor; +import com.yahoo.slime.Slime; +import com.yahoo.slime.SlimeUtils; +import com.yahoo.vespa.athenz.api.AthenzDomain; +import com.yahoo.vespa.athenz.identity.ServiceIdentityProvider; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.impl.client.HttpClientBuilder; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Objects; + +public class GcpCredentials { + private static final TokenDomain domain = TokenDomain.of("athenz-gcp-oauth2-nonce"); + + final private InputStream tokenApiStream; + private final HttpTransportFactory httpTransportFactory; + + private GcpCredentials(Builder builder) { + String clientId = builder.athenzDomain.getName() + ".gcp"; + String audience = String.format("//iam.googleapis.com/projects/%s/locations/global/workloadIdentityPools/%s/providers/%s", + builder.projectNumber, builder.workloadPoolName, builder.workloadProviderName); + String serviceUrl = String.format("https://iamcredentials.googleapis.com/v1/projects/-/serviceAccounts/%s@%s.iam.gserviceaccount.com:generateAccessToken", + builder.serviceAccountName, builder.projectName); + String scope = URLEncoder.encode(generateIdTokenScope(builder.athenzDomain.getName(), builder.role), StandardCharsets.UTF_8); + String redirectUri = URLEncoder.encode(generateRedirectUri(clientId, builder.redirectURISuffix), StandardCharsets.UTF_8); + String tokenUrl = String.format("%s/oauth2/auth?response_type=id_token&client_id=%s&redirect_uri=%s&scope=%s&nonce=%s&keyType=EC&fullArn=true&output=json", + builder.ztsUrl, clientId, redirectUri, scope, TokenGenerator.generateToken(domain, "", 32).secretTokenString()); + + tokenApiStream = createTokenAPIStream(audience, serviceUrl, tokenUrl, builder.tokenLifetimeSeconds); + SSLConnectionSocketFactory sslConnectionSocketFactory = new SSLConnectionSocketFactory(builder.identityProvider.getIdentitySslContext()); + HttpClientBuilder httpClientBuilder = ApacheHttpTransport.newDefaultHttpClientBuilder() + .setSSLSocketFactory(sslConnectionSocketFactory); + httpTransportFactory = () -> new ApacheHttpTransport(httpClientBuilder.build()); + } + + public ExternalAccountCredentials getCredential() throws IOException { + return ExternalAccountCredentials.fromStream(tokenApiStream, httpTransportFactory); + } + + private InputStream createTokenAPIStream(final String audience, final String serviceUrl, final String tokenUrl, + int tokenLifetimeSeconds) { + + Slime root = new Slime(); + Cursor c = root.setObject(); + + c.setString("type", "external_account"); + c.setString("audience", audience); + c.setString("subject_token_type", "urn:ietf:params:oauth:token-type:jwt"); + c.setString("token_url", "https://sts.googleapis.com/v1/token"); + + c.setString("service_account_impersonation_url", serviceUrl); + Cursor sai = c.setObject("service_account_impersonation"); + sai.setLong("token_lifetime_seconds", tokenLifetimeSeconds); + + Cursor credentialSource = c.setObject("credential_source"); + credentialSource.setString("url", tokenUrl); + + Cursor credentialSourceFormat = credentialSource.setObject("format"); + credentialSourceFormat.setString("type", "json"); + credentialSourceFormat.setString("subject_token_field_name", "id_token"); + + try { + return new ByteArrayInputStream(SlimeUtils.toJsonBytes(root)); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private static String generateIdTokenScope(final String domainName, String roleName) { + StringBuilder scope = new StringBuilder(256); + scope.append("openid"); + scope.append(' ').append(domainName).append(":role.").append(roleName); + return scope.toString(); + } + + private static String generateRedirectUri(final String clientId, String uriSuffix) { + int idx = clientId.lastIndexOf('.'); + if (idx == -1) { + return ""; + } + final String dashDomain = clientId.substring(0, idx).replace('.', '-'); + final String service = clientId.substring(idx + 1); + return "https://" + service + "." + dashDomain + "." + uriSuffix; + } + + + public static class Builder { + private String ztsUrl; + private ServiceIdentityProvider identityProvider; + private String redirectURISuffix; + private AthenzDomain athenzDomain; + private String role; + private String projectName; + private String projectNumber; + private String serviceAccountName; + + private int tokenLifetimeSeconds = 3600; // default to 1 hour lifetime + private String workloadPoolName = "athenz"; + private String workloadProviderName = "athenz"; + + public GcpCredentials build() { + Objects.requireNonNull(ztsUrl); + Objects.requireNonNull(identityProvider); + Objects.requireNonNull(redirectURISuffix); + Objects.requireNonNull(athenzDomain); + Objects.requireNonNull(role); + Objects.requireNonNull(projectName); + Objects.requireNonNull(projectNumber); + Objects.requireNonNull(serviceAccountName); + + return new GcpCredentials(this); + } + + public Builder setZtsUrl(String ztsUrl) { + this.ztsUrl = ztsUrl; + return this; + } + + public Builder identityProvider(ServiceIdentityProvider provider) { + this.identityProvider = provider; + return this; + } + + public Builder redirectURISuffix(String redirectURISuffix) { + this.redirectURISuffix = redirectURISuffix; + return this; + } + + public Builder athenzDomain(AthenzDomain athenzDomain) { + this.athenzDomain = athenzDomain; + return this; + } + + public Builder role(String gcpRole) { + this.role = gcpRole; + return this; + } + + public Builder projectName(String projectName) { + this.projectName = projectName; + return this; + } + + public Builder projectNumber(String projectNumber) { + this.projectNumber = projectNumber; + return this; + } + + public Builder serviceAccountName(String serviceAccountName) { + this.serviceAccountName = serviceAccountName; + return this; + } + + public Builder tokenLifetimeSeconds(int tokenLifetimeSeconds) { + this.tokenLifetimeSeconds = tokenLifetimeSeconds; + return this; + } + + public Builder workloadPoolName(String workloadPoolName) { + this.workloadPoolName = workloadPoolName; + return this; + } + + public Builder workloadProviderName(String workloadProviderName) { + this.workloadProviderName = workloadProviderName; + return this; + } + } +} diff --git a/vespa-dependencies-enforcer/allowed-maven-dependencies.txt b/vespa-dependencies-enforcer/allowed-maven-dependencies.txt index 7684e3ea2ae..cb2806b66d8 100644 --- a/vespa-dependencies-enforcer/allowed-maven-dependencies.txt +++ b/vespa-dependencies-enforcer/allowed-maven-dependencies.txt @@ -24,10 +24,17 @@ com.fasterxml.jackson.jaxrs:jackson-jaxrs-base:2.15.2 com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:2.15.2 com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.15.2 com.github.spotbugs:spotbugs-annotations:3.1.9 +com.google.auth:google-auth-library-credentials:1.19.0 +com.google.auth:google-auth-library-oauth2-http:1.19.0 +com.google.auto.value:auto-value-annotations:1.10.1 com.google.code.findbugs:jsr305:3.0.2 +com.google.code.gson:gson:2.10 com.google.errorprone:error_prone_annotations:2.18.0 com.google.guava:failureaccess:1.0.1 com.google.guava:guava:32.1.1-jre +com.google.http-client:google-http-client:1.43.3 +com.google.http-client:google-http-client-apache-v2:1.43.3 +com.google.http-client:google-http-client-gson:1.42.3 com.google.inject:guice:4.2.3:no_aop com.google.j2objc:j2objc-annotations:2.8 com.google.protobuf:protobuf-java:3.21.7 @@ -53,6 +60,7 @@ commons-io:commons-io:2.11.0 commons-logging:commons-logging:1.2 io.airlift:airline:0.9 io.dropwizard.metrics:metrics-core:3.2.5 +io.grpc:grpc-context:1.27.2 io.jsonwebtoken:jjwt-api:0.11.5 io.jsonwebtoken:jjwt-impl:0.11.5 io.jsonwebtoken:jjwt-jackson:0.11.5 @@ -67,6 +75,8 @@ io.netty:netty-transport:4.1.94.Final io.netty:netty-transport-classes-epoll:4.1.94.Final io.netty:netty-transport-native-epoll:4.1.94.Final io.netty:netty-transport-native-unix-common:4.1.94.Final +io.opencensus:opencensus-api:0.31.1 +io.opencensus:opencensus-contrib-http-util:0.31.1 io.prometheus:simpleclient:0.6.0 io.prometheus:simpleclient_common:0.6.0 javax.annotation:javax.annotation-api:1.2 |