Skip to content

Commit 2edb60e

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 bf6f3c5 commit 2edb60e

File tree

8 files changed

+37
-12
lines changed

8 files changed

+37
-12
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) {

tests/rptest/tests/adjacent_segment_merging_test.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,10 @@ def __init__(self, test_context, *args, **kwargs):
150150
test_context, extra_rp_conf=xtra_conf, num_brokers=1, *args, **kwargs
151151
)
152152

153+
self.redpanda.set_environment(
154+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
155+
)
156+
153157
@cluster(num_nodes=2)
154158
@matrix(
155159
acks=[

tests/rptest/tests/e2e_shadow_indexing_test.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1232,6 +1232,10 @@ def __init__(self, test_context):
12321232
self.msg_size = 1024 * 256
12331233
self.msg_count = 3000
12341234

1235+
self.redpanda.set_environment(
1236+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
1237+
)
1238+
12351239
def produce(self):
12361240
topic_name = self.topics[0].name
12371241
producer = KgoVerifierProducer(

tests/rptest/tests/nodes_decommissioning_test.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ def __init__(self, test_context):
7070
extra_rp_conf=extra_rp_conf,
7171
)
7272

73+
self.redpanda.set_environment(
74+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
75+
)
76+
7377
def setup(self):
7478
# defer starting redpanda to test body
7579
pass

tests/rptest/tests/offset_for_leader_epoch_archival_test.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ def __init__(self, test_context):
6060
si_settings=si_settings,
6161
)
6262

63+
self.redpanda.set_environment(
64+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
65+
)
66+
6367
def _alter_topic_retention_with_retry(self, topic):
6468
rpk = RpkTool(self.redpanda)
6569

tests/rptest/tests/partition_movement_test.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1025,7 +1025,10 @@ def test_shadow_indexing(
10251025
throughput, records, moves, partitions = self._get_scale_params()
10261026
install_opts = InstallOptions(install_previous_version=test_mixed_versions)
10271027
self.start_redpanda(
1028-
num_nodes=3, install_opts=install_opts, extra_rp_conf=extra_rp_conf
1028+
num_nodes=3,
1029+
install_opts=install_opts,
1030+
extra_rp_conf=extra_rp_conf,
1031+
environment={"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"},
10291032
)
10301033

10311034
self.topic = "topic"
@@ -1094,7 +1097,10 @@ def test_cross_shard(
10941097

10951098
install_opts = InstallOptions(install_previous_version=test_mixed_versions)
10961099
self.start_redpanda(
1097-
num_nodes=3, install_opts=install_opts, extra_rp_conf=extra_rp_conf
1100+
num_nodes=3,
1101+
install_opts=install_opts,
1102+
extra_rp_conf=extra_rp_conf,
1103+
environment={"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"},
10981104
)
10991105

11001106
self.topic = "topic"

tests/rptest/tests/retention_policy_test.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,10 @@ def __init__(self, test_context):
254254
self.rpk = RpkTool(self.redpanda)
255255
self.s3_bucket_name = si_settings.cloud_storage_bucket
256256

257+
self.redpanda.set_environment(
258+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
259+
)
260+
257261
def query_segments(self):
258262
return self.redpanda.node_storage(self.redpanda.nodes[0]).segments(
259263
"kafka", self.topic_name, 0
@@ -457,6 +461,10 @@ def __init__(self, test_context):
457461
self.rpk = RpkTool(self.redpanda)
458462
self.s3_bucket_name = si_settings.cloud_storage_bucket
459463

464+
self.redpanda.set_environment(
465+
{"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}
466+
)
467+
460468
@cluster(num_nodes=3)
461469
@matrix(cloud_storage_type=get_cloud_storage_type())
462470
def test_cloud_retention_deleted_segments_count(self, cloud_storage_type):

0 commit comments

Comments
 (0)