Skip to content

Commit dfed747

Browse files
committed
config: don't apply values that fail validation
Previously, config_store::read_yaml() would apply property values even when custom validation failed, only recording the error. This meant invalid configuration values would silently take effect despite being reported as errors. This behavior was also inconsistent: conversion exceptions (e.g., YAML::BadConversion) correctly prevented values from being set, but custom validator errors did not. Fix by moving set_value() inside the else branch so values are only applied when validation succeeds. This has two effects: 1. Optional properties now correctly retain their default value when validation fails. 2. Required properties now correctly throw std::invalid_argument when validation fails. Previously, the throw logic was bypassed because ok was set to true even after validation errors. Note: It's unclear why we don't stop server startup on invalid optional config values. To avoid breaking something I don't fully understand, this change takes the conservative approach of retaining defaults.
1 parent 4d63f53 commit dfed747

File tree

2 files changed

+5
-10
lines changed

2 files changed

+5
-10
lines changed

src/v/config/config_store.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,10 @@ class config_store {
9494
errors[name] = fmt::format(
9595
"Validation error: {}",
9696
validation_err.value().error_message());
97+
} else {
98+
prop->set_value(node.second);
99+
ok = true;
97100
}
98-
prop->set_value(node.second);
99-
ok = true;
100101
} catch (const YAML::InvalidNode& e) {
101102
errors[name] = fmt::format("Invalid syntax: {}", e);
102103
} catch (const YAML::ParserException& e) {

src/v/config/tests/config_store_test.cc

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -309,9 +309,7 @@ SEASTAR_THREAD_TEST_CASE(validate_with_validator_error) {
309309
BOOST_TEST(errors.size() > 0);
310310

311311
// Property should retain default value
312-
// Surprising but the current behavior is that invalid values actually
313-
// update the property.
314-
BOOST_TEST(cfg.validated_string() == "invalid_value");
312+
BOOST_TEST(cfg.validated_string() == "magic_foo");
315313
}
316314

317315
SEASTAR_THREAD_TEST_CASE(validate_with_type_mismatch) {
@@ -346,11 +344,7 @@ SEASTAR_THREAD_TEST_CASE(validate_required_with_validator_error) {
346344
"required_validated_string: invalid_value\n");
347345

348346
// Required property with validation error throws std::invalid_argument
349-
// BOOST_CHECK_THROW(cfg.read_yaml(invalid_yaml), std::invalid_argument);
350-
351-
auto errors = cfg.read_yaml(invalid_yaml);
352-
BOOST_TEST(errors.size() > 0);
353-
BOOST_TEST(cfg.required_validated_string() == "invalid_value");
347+
BOOST_CHECK_THROW(cfg.read_yaml(invalid_yaml), std::invalid_argument);
354348
}
355349

356350
SEASTAR_THREAD_TEST_CASE(config_json_serialization) {

0 commit comments

Comments
 (0)