diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java index bec1dbe5c38..e08b575becb 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigParser.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.function.Predicate; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -75,6 +76,7 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx return new StableConfigSource.StableConfig(configId, mergedConfigMap); } } + log.debug("No matching rule found in stable configuration file {}", filePath); } // If configs were found in apm_configuration_default, use them if (!configMap.isEmpty()) { @@ -85,7 +87,6 @@ public static StableConfigSource.StableConfig parse(String filePath) throws IOEx if (configId != null) { return new StableConfigSource.StableConfig(configId, Collections.emptyMap()); } - } catch (IOException e) { log.debug("Failed to read the stable configuration file: {}", filePath, e); } @@ -117,22 +118,32 @@ private static boolean matchOperator(String value, String operator, List return false; } value = value.toLowerCase(Locale.ROOT); + + Predicate comparator; + switch (operator) { + case "equals": + comparator = value::equals; + break; + case "starts_with": + comparator = value::startsWith; + break; + case "ends_with": + comparator = value::endsWith; + break; + case "contains": + comparator = value::contains; + break; + default: + return false; + } + for (String match : matches) { if (match == null) { continue; } match = match.toLowerCase(Locale.ROOT); - switch (operator) { - case "equals": - return value.equals(match); - case "starts_with": - return value.startsWith(match); - case "ends_with": - return value.endsWith(match); - case "contains": - return value.contains(match); - default: - return false; + if (comparator.test(match)) { + return true; } } return false; diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java index ab634b6fc14..917fbec2bda 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/StableConfigSource.java @@ -3,6 +3,7 @@ import static datadog.trace.util.Strings.propertyNameToEnvironmentVariableName; import datadog.trace.api.ConfigOrigin; +import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException; import java.io.File; import java.util.Collections; import java.util.Map; @@ -38,10 +39,23 @@ public final class StableConfigSource extends ConfigProvider.Source { log.debug("Stable configuration file found at path: {}", file); cfg = StableConfigParser.parse(filePath); } catch (Throwable e) { - log.warn( - "Encountered the following exception when attempting to read stable configuration file at path: {}, dropping configs.", - file, - e); + if (e instanceof StableConfigMappingException + || e instanceof IllegalArgumentException + || e instanceof ClassCastException + || e instanceof NullPointerException) { + log.warn( + "YAML mapping error in stable configuration file: {}, error: {}", + filePath, + e.getMessage()); + } else if (log.isDebugEnabled()) { + log.error("Unexpected error while reading stable configuration file: {}", filePath, e); + } else { + log.error( + "Unexpected error while reading stable configuration file: {}, error: {}", + filePath, + e.getMessage()); + } + cfg = StableConfig.EMPTY; } this.config = cfg; diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java index 2d464b41ed1..673c7f8c2a2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Rule.java @@ -1,5 +1,6 @@ package datadog.trace.bootstrap.config.provider.stableconfig; +import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException; import static java.util.Collections.*; import static java.util.stream.Collectors.toList; @@ -25,13 +26,47 @@ public Rule(List selectors, Map configuration) { this.configuration = configuration; } - public Rule(Object yaml) { - Map map = (Map) yaml; - selectors = - unmodifiableList( - ((List) map.get("selectors")) - .stream().filter(Objects::nonNull).map(Selector::new).collect(toList())); - configuration = unmodifiableMap((Map) map.get("configuration")); + public static Rule from(Map map) { + Object selectorsObj = map.get("selectors"); + if (selectorsObj == null) { + throwStableConfigMappingException("Missing 'selectors' in rule:", map); + } + + if (!(selectorsObj instanceof List)) { + throwStableConfigMappingException( + "'selectors' must be a list, but got: " + selectorsObj.getClass().getSimpleName() + ": ", + selectorsObj); + } + + Object configObj = map.get("configuration"); + if (configObj == null) { + throwStableConfigMappingException("Missing 'configuration' in rule:", map); + } + if (!(configObj instanceof Map)) { + throwStableConfigMappingException( + "'configuration' must be a map, but got: " + configObj.getClass().getSimpleName() + ": ", + configObj); + } + + List selectors = + ((List) selectorsObj) + .stream() + .filter(Objects::nonNull) + .map( + s -> { + if (!(s instanceof Map)) { + throwStableConfigMappingException( + "Each selector must be a map, but got: " + + s.getClass().getSimpleName() + + ": ", + s); + } + + return Selector.from((Map) s); + }) + .collect(toList()); + + return new Rule(unmodifiableList(selectors), (Map) configObj); } public List getSelectors() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java index ac611751fa5..b06362cd0cd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/Selector.java @@ -1,5 +1,7 @@ package datadog.trace.bootstrap.config.provider.stableconfig; +import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException; + import java.util.Collections; import java.util.List; import java.util.Map; @@ -17,14 +19,43 @@ public Selector(String origin, String key, List matches, String operator this.operator = operator; } - public Selector(Object yaml) { - Map map = (Map) yaml; - origin = (String) map.get("origin"); - key = (String) map.get("key"); - List rawMatches = (List) map.get("matches"); - matches = + public static Selector from(Map map) { + Object originObj = map.get("origin"); + if (originObj == null) { + throwStableConfigMappingException("Missing 'origin' in selector:", map); + } + if (!(originObj instanceof String)) { + throwStableConfigMappingException( + "'origin' must be a string, but got: " + originObj.getClass().getSimpleName() + ": ", + originObj); + } + String origin = (String) originObj; + + Object keyObj = map.get("key"); + String key = (keyObj instanceof String) ? (String) keyObj : null; + + Object matchesObj = map.get("matches"); + if (matchesObj != null && !(matchesObj instanceof List)) { + throwStableConfigMappingException( + "'matches' must be a list, but got: " + matchesObj.getClass().getSimpleName() + ": ", + matchesObj); + } + List rawMatches = (List) matchesObj; + List matches = rawMatches != null ? Collections.unmodifiableList(rawMatches) : Collections.emptyList(); - operator = (String) map.get("operator"); + + Object operatorObj = map.get("operator"); + if (operatorObj == null) { + throwStableConfigMappingException("Missing 'operator' in selector:", map); + } + if (!(operatorObj instanceof String)) { + throwStableConfigMappingException( + "'operator' must be a string, but got: " + operatorObj.getClass().getSimpleName() + ": ", + operatorObj); + } + String operator = (String) operatorObj; + + return new Selector(origin, key, matches, operator); } public String getOrigin() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java index 58fa3c4f826..f5b09d0d579 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfig.java @@ -1,10 +1,10 @@ package datadog.trace.bootstrap.config.provider.stableconfig; +import static datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException.throwStableConfigMappingException; import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableList; import static java.util.Collections.unmodifiableMap; -import static java.util.stream.Collectors.toList; import java.util.ArrayList; import java.util.List; @@ -21,10 +21,7 @@ public StableConfig(Object yaml) { this.apmConfigurationDefault = unmodifiableMap( (Map) map.getOrDefault("apm_configuration_default", emptyMap())); - this.apmConfigurationRules = - unmodifiableList( - ((List) map.getOrDefault("apm_configuration_rules", emptyList())) - .stream().map(Rule::new).collect(toList())); + this.apmConfigurationRules = parseRules(map); } // test only @@ -45,4 +42,23 @@ public Map getApmConfigurationDefault() { public List getApmConfigurationRules() { return apmConfigurationRules; } + + private List parseRules(Map map) { + Object rulesObj = map.get("apm_configuration_rules"); + if (rulesObj instanceof List) { + List rulesList = (List) rulesObj; + List rules = new ArrayList<>(); + for (Object ruleObj : rulesList) { + if (ruleObj instanceof Map) { + rules.add(Rule.from((Map) ruleObj)); + } else { + throwStableConfigMappingException( + "Rule must be a map, but got: " + ruleObj.getClass().getSimpleName() + ": ", ruleObj); + return emptyList(); + } + } + return unmodifiableList(rules); + } + return emptyList(); + } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java new file mode 100644 index 00000000000..5b0f10a3cda --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/bootstrap/config/provider/stableconfig/StableConfigMappingException.java @@ -0,0 +1,40 @@ +package datadog.trace.bootstrap.config.provider.stableconfig; + +public class StableConfigMappingException extends RuntimeException { + private static final int MAX_LEN = 100; + + public StableConfigMappingException(String message) { + super(message); + } + + /** + * Safely converts an object to a string for error reporting, truncating the result if it exceeds + * a maximum length. + * + * @param value the object to convert to a string + * @return a string representation of the object, truncated if necessary + */ + static String safeToString(Object value) { + if (value == null) return "null"; + String str = value.toString(); + if (str.length() > MAX_LEN) { + int partLen = MAX_LEN / 2; + return str.substring(0, partLen) + + "...(truncated)..." + + str.substring(str.length() - partLen); + } + return str; + } + + /** + * Throws a StableConfigMappingException with a message that includes a safe string representation + * of the provided value. + * + * @param message the error message to include + * @param value the value to include in the error message, safely stringified + * @throws StableConfigMappingException always thrown by this method + */ + public static void throwStableConfigMappingException(String message, Object value) { + throw new StableConfigMappingException(message + " " + safeToString(value)); + } +} diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigMappingExceptionTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigMappingExceptionTest.groovy new file mode 100644 index 00000000000..2a1af178562 --- /dev/null +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigMappingExceptionTest.groovy @@ -0,0 +1,35 @@ +package datadog.trace.bootstrap.config.provider + +import datadog.trace.bootstrap.config.provider.stableconfig.StableConfigMappingException +import spock.lang.Specification + +class StableConfigMappingExceptionTest extends Specification { + + def "constructors work as expected"() { + when: + def ex1 = new StableConfigMappingException("msg") + def ex2 = new StableConfigMappingException("msg2") + + then: + ex1.message == "msg" + ex1.cause == null + ex2.message == "msg2" + } + + def "safeToString handles null"() { + expect: + StableConfigMappingException.safeToString(null) == "null" + } + + def "safeToString handles short string"() { + expect: + StableConfigMappingException.safeToString("short string") == "short string" + } + + def "safeToString handles long string"() { + given: + def longStr = "a" * 101 + expect: + StableConfigMappingException.safeToString(longStr) == ("a" * 50) + "...(truncated)..." + ("a" * 51).substring(1) + } +} diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy index b3ec755ddc1..4dee86f5ae7 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/config/provider/StableConfigSourceTest.groovy @@ -9,6 +9,10 @@ import datadog.trace.bootstrap.config.provider.stableconfig.StableConfig import datadog.trace.test.util.DDSpecification import org.snakeyaml.engine.v2.api.Dump import org.snakeyaml.engine.v2.api.DumpSettings +import ch.qos.logback.classic.Logger +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.core.read.ListAppender +import org.slf4j.LoggerFactory import java.nio.file.Files import java.nio.file.Path @@ -26,18 +30,17 @@ class StableConfigSourceTest extends DDSpecification { } def "test empty file"() { - when: + given: Path filePath = Files.createTempFile("testFile_", ".yaml") - then: - if (filePath == null) { - throw new AssertionError("Failed to create: " + filePath) - } when: StableConfigSource config = new StableConfigSource(filePath.toString(), ConfigOrigin.LOCAL_STABLE_CONFIG) then: config.getKeys().size() == 0 config.getConfigId() == null + + cleanup: + Files.delete(filePath) } def "test file invalid format"() { @@ -56,6 +59,8 @@ class StableConfigSourceTest extends DDSpecification { then: stableCfg.getConfigId() == null stableCfg.getKeys().size() == 0 + + cleanup: Files.delete(filePath) where: @@ -65,12 +70,8 @@ class StableConfigSourceTest extends DDSpecification { } def "test file valid format"() { - when: + given: Path filePath = Files.createTempFile("testFile_", ".yaml") - then: - if (filePath == null) { - throw new AssertionError("Failed to create: " + filePath) - } when: StableConfig stableConfigYaml = new StableConfig(configId, defaultConfigs) @@ -100,6 +101,8 @@ class StableConfigSourceTest extends DDSpecification { !cfgKeys.contains(keyString) } } + + cleanup: Files.delete(filePath) where: @@ -108,6 +111,114 @@ class StableConfigSourceTest extends DDSpecification { "12345" | ["DD_KEY_ONE": "one", "DD_KEY_TWO": "two"] | Arrays.asList(sampleMatchingRule, sampleNonMatchingRule) } + def "test parse invalid logs mapping errors"() { + given: + Logger logbackLogger = (Logger) LoggerFactory.getLogger(StableConfigSource) + def listAppender = new ListAppender() + listAppender.start() + logbackLogger.addAppender(listAppender) + + def tempFile = File.createTempFile("testFile_", ".yaml") + tempFile.text = yaml + + when: + def stableCfg = new StableConfigSource(tempFile.absolutePath, ConfigOrigin.LOCAL_STABLE_CONFIG) + + then: + stableCfg.config == StableConfigSource.StableConfig.EMPTY + def warnLogs = listAppender.list.findAll { it.level.toString() == 'WARN' } + warnLogs.any { it.formattedMessage.contains(expectedLogSubstring) } + + cleanup: + tempFile.delete() + logbackLogger.detachAppender(listAppender) + + where: + yaml | expectedLogSubstring + '''apm_configuration_rules: + - selectors: + - key: "someKey" + matches: ["someValue"] + operator: equals + configuration: + DD_SERVICE: "test" + ''' | "Missing 'origin' in selector" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: ["bar"] + operator: equals + ''' | "Missing 'configuration' in rule" + '''apm_configuration_rules: + - configuration: + DD_SERVICE: "test" + ''' | "Missing 'selectors' in rule" + '''apm_configuration_rules: + - selectors: "not-a-list" + configuration: + DD_SERVICE: "test" + ''' | "'selectors' must be a list, but got: String" + '''apm_configuration_rules: + - selectors: + - "not-a-map" + configuration: + DD_SERVICE: "test" + ''' | "Each selector must be a map, but got: String" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: ["bar"] + operator: equals + configuration: "not-a-map" + ''' | "'configuration' must be a map, but got: String" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: ["bar"] + operator: equals + configuration: 12345 + ''' | "'configuration' must be a map, but got: Integer" + '''apm_configuration_rules: + - "not-a-map" + ''' | "Rule must be a map, but got: String" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: "not-a-list" + operator: equals + configuration: + DD_SERVICE: "test" + ''' | "'matches' must be a list, but got: String" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: ["bar"] + configuration: + DD_SERVICE: "test" + ''' | "Missing 'operator' in selector" + '''apm_configuration_rules: + - selectors: + - origin: process_arguments + key: "-Dfoo" + matches: ["bar"] + operator: 12345 + configuration: + DD_SERVICE: "test" + ''' | "'operator' must be a string, but got: Integer" + '''apm_configuration_rules: + - selectors: + # origin is missing entirely, should trigger NullPointerException + - key: "-Dfoo" + matches: ["bar"] + operator: equals + ''' | "YAML mapping error in stable configuration file" + } + // Corrupt YAML string variable used for testing, defined outside the 'where' block for readability def static corruptYaml = ''' abc: 123