diff --git a/src/v/config/config_store.h b/src/v/config/config_store.h index 12fe7914f9eda..2dd7cf7299a77 100644 --- a/src/v/config/config_store.h +++ b/src/v/config/config_store.h @@ -94,9 +94,10 @@ class config_store { errors[name] = fmt::format( "Validation error: {}", validation_err.value().error_message()); + } else { + prop->set_value(node.second); + ok = true; } - prop->set_value(node.second); - ok = true; } catch (const YAML::InvalidNode& e) { errors[name] = fmt::format("Invalid syntax: {}", e); } catch (const YAML::ParserException& e) { diff --git a/src/v/config/tests/config_store_test.cc b/src/v/config/tests/config_store_test.cc index a4c8ef77ae45f..bc35c5cc0a86b 100644 --- a/src/v/config/tests/config_store_test.cc +++ b/src/v/config/tests/config_store_test.cc @@ -13,14 +13,12 @@ #include "json/writer.h" #include -#include #include #include #include #include #include -#include #include #include @@ -28,6 +26,20 @@ namespace { ss::logger lg("config_test"); // NOLINT +std::optional validate_magic_prefix(const ss::sstring& value) { + if (!value.starts_with("magic_")) { + return fmt::format("value must start with 'magic_', got: {}", value); + } + return std::nullopt; +} + +std::optional validate_throwing(const ss::sstring& value) { + if (value == "throw") { + throw std::runtime_error("validator threw an exception"); + } + return std::nullopt; +} + struct test_config : public config::config_store { config::property optional_int; config::property required_string; @@ -44,6 +56,8 @@ struct test_config : public config::config_store { config::property default_secret_string; config::property secret_string; config::property aliased_bool; + config::property validated_string; + config::property throwing_validator_string; test_config() : optional_int( @@ -113,11 +127,38 @@ struct test_config : public config::config_store { "aliased_bool", "Property with a compat alias", {.aliases = {"aliased_bool_legacy"}}, - true) {} + true) + , validated_string( + *this, + "validated_string", + "String that must start with magic_", + {}, + "magic_foo", + &validate_magic_prefix) + , throwing_validator_string( + *this, + "throwing_validator_string", + "String with a validator that throws", + {}, + "safe", + &validate_throwing) {} }; struct noop_config : public config::config_store {}; +struct required_validated_config : public config::config_store { + config::property required_validated_string; + + required_validated_config() + : required_validated_string( + *this, + "required_validated_string", + "Required string that must start with magic_", + {.required = config::required::yes}, + "magic_bar", + &validate_magic_prefix) {} +}; + YAML::Node minimal_valid_configuration() { return YAML::Load( "required_string: test_value_1\n" @@ -257,10 +298,53 @@ SEASTAR_THREAD_TEST_CASE(validate_valid_configuration) { BOOST_TEST(errors.size() == 0); } -SEASTAR_THREAD_TEST_CASE(validate_invalid_configuration) { +SEASTAR_THREAD_TEST_CASE(validate_with_validator_error) { auto cfg = test_config(); - auto errors = cfg.read_yaml(valid_configuration()); - BOOST_TEST(errors.size() == 0); + + auto invalid_yaml = YAML::Load("validated_string: invalid_value\n"); + + auto errors = cfg.read_yaml(invalid_yaml); + + // Should have error from validator + BOOST_TEST(errors.size() > 0); + + // Property should retain default value + BOOST_TEST(cfg.validated_string() == "magic_foo"); +} + +SEASTAR_THREAD_TEST_CASE(validate_with_type_mismatch) { + auto cfg = test_config(); + + // Provide string where int is expected + auto invalid_yaml = YAML::Load("optional_int: not_an_int\n"); + + auto errors = cfg.read_yaml(invalid_yaml); + + BOOST_REQUIRE(!errors.empty()); + auto& [key, msg] = *errors.begin(); + BOOST_TEST(key == "optional_int"); + BOOST_TEST_INFO(msg); + BOOST_TEST(msg.contains("bad conversion")); + + BOOST_TEST(cfg.optional_int() == 100); // expect default retained +} + +SEASTAR_THREAD_TEST_CASE(validate_with_throwing_validator) { + auto cfg = test_config(); + + auto yaml = YAML::Load("throwing_validator_string: throw\n"); + + BOOST_CHECK_THROW(cfg.read_yaml(yaml), std::runtime_error); +} + +SEASTAR_THREAD_TEST_CASE(validate_required_with_validator_error) { + auto cfg = required_validated_config(); + + auto invalid_yaml = YAML::Load( + "required_validated_string: invalid_value\n"); + + // Required property with validation error throws std::invalid_argument + BOOST_CHECK_THROW(cfg.read_yaml(invalid_yaml), std::invalid_argument); } SEASTAR_THREAD_TEST_CASE(config_json_serialization) { diff --git a/tests/rptest/tests/adjacent_segment_merging_test.py b/tests/rptest/tests/adjacent_segment_merging_test.py index 2047bf3ebec09..b5fc2329f791b 100644 --- a/tests/rptest/tests/adjacent_segment_merging_test.py +++ b/tests/rptest/tests/adjacent_segment_merging_test.py @@ -150,6 +150,10 @@ def __init__(self, test_context, *args, **kwargs): test_context, extra_rp_conf=xtra_conf, num_brokers=1, *args, **kwargs ) + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + @cluster(num_nodes=2) @matrix( acks=[ diff --git a/tests/rptest/tests/cluster_config_test.py b/tests/rptest/tests/cluster_config_test.py index d95f28eb07a85..94333cc16f18c 100644 --- a/tests/rptest/tests/cluster_config_test.py +++ b/tests/rptest/tests/cluster_config_test.py @@ -205,6 +205,38 @@ def test_unknown_redpanda_yaml(self): ) +class ClusterConfigBoundedPropertyTest(RedpandaTest): + """ + Test that bounded properties clamp out-of-range values from bootstrap config. + """ + + def __init__(self, *args, **kwargs): + # Set log_segment_size to 100 bytes (below minimum of 1MB) + super().__init__( + *args, + extra_rp_conf={"log_segment_size": 100}, + **kwargs, + ) + + @cluster(num_nodes=1) + def test_bounded_property_clamped_to_minimum(self): + """ + Verify that bounded properties are clamped to their minimum bound when + an out-of-range value is provided in bootstrap config. Setting + log_segment_size to 100 should result in the value being clamped to + 1MB (the minimum bound). + """ + admin = Admin(self.redpanda) + config = admin.get_cluster_config() + + # The value should be clamped to 1MB (1048576 bytes) + min_segment_size = 1024 * 1024 # 1 MiB + assert config["log_segment_size"] == min_segment_size, ( + f"Expected log_segment_size to be clamped to {min_segment_size} (1MB), " + f"but got {config['log_segment_size']}" + ) + + class HasRedpandaAndAdmin(Protocol): redpanda: RedpandaService admin: Admin diff --git a/tests/rptest/tests/e2e_shadow_indexing_test.py b/tests/rptest/tests/e2e_shadow_indexing_test.py index 302c7cccc6a16..2d0a9a898e058 100644 --- a/tests/rptest/tests/e2e_shadow_indexing_test.py +++ b/tests/rptest/tests/e2e_shadow_indexing_test.py @@ -1232,6 +1232,10 @@ def __init__(self, test_context): self.msg_size = 1024 * 256 self.msg_count = 3000 + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + def produce(self): topic_name = self.topics[0].name producer = KgoVerifierProducer( diff --git a/tests/rptest/tests/nodes_decommissioning_test.py b/tests/rptest/tests/nodes_decommissioning_test.py index 234f25c120bbb..68840680335f8 100644 --- a/tests/rptest/tests/nodes_decommissioning_test.py +++ b/tests/rptest/tests/nodes_decommissioning_test.py @@ -70,6 +70,10 @@ def __init__(self, test_context): extra_rp_conf=extra_rp_conf, ) + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + def setup(self): # defer starting redpanda to test body pass diff --git a/tests/rptest/tests/offset_for_leader_epoch_archival_test.py b/tests/rptest/tests/offset_for_leader_epoch_archival_test.py index e5622161a337f..2b95636566a7a 100644 --- a/tests/rptest/tests/offset_for_leader_epoch_archival_test.py +++ b/tests/rptest/tests/offset_for_leader_epoch_archival_test.py @@ -60,6 +60,10 @@ def __init__(self, test_context): si_settings=si_settings, ) + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + def _alter_topic_retention_with_retry(self, topic): rpk = RpkTool(self.redpanda) diff --git a/tests/rptest/tests/partition_movement_test.py b/tests/rptest/tests/partition_movement_test.py index f61cfb7efb512..a786e0f98e11f 100644 --- a/tests/rptest/tests/partition_movement_test.py +++ b/tests/rptest/tests/partition_movement_test.py @@ -1025,7 +1025,10 @@ def test_shadow_indexing( throughput, records, moves, partitions = self._get_scale_params() install_opts = InstallOptions(install_previous_version=test_mixed_versions) self.start_redpanda( - num_nodes=3, install_opts=install_opts, extra_rp_conf=extra_rp_conf + num_nodes=3, + install_opts=install_opts, + extra_rp_conf=extra_rp_conf, + environment={"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}, ) self.topic = "topic" @@ -1094,7 +1097,10 @@ def test_cross_shard( install_opts = InstallOptions(install_previous_version=test_mixed_versions) self.start_redpanda( - num_nodes=3, install_opts=install_opts, extra_rp_conf=extra_rp_conf + num_nodes=3, + install_opts=install_opts, + extra_rp_conf=extra_rp_conf, + environment={"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"}, ) self.topic = "topic" diff --git a/tests/rptest/tests/retention_policy_test.py b/tests/rptest/tests/retention_policy_test.py index 8bdbe5c1cceef..01c69f0b05a3f 100644 --- a/tests/rptest/tests/retention_policy_test.py +++ b/tests/rptest/tests/retention_policy_test.py @@ -254,6 +254,10 @@ def __init__(self, test_context): self.rpk = RpkTool(self.redpanda) self.s3_bucket_name = si_settings.cloud_storage_bucket + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + def query_segments(self): return self.redpanda.node_storage(self.redpanda.nodes[0]).segments( "kafka", self.topic_name, 0 @@ -457,6 +461,10 @@ def __init__(self, test_context): self.rpk = RpkTool(self.redpanda) self.s3_bucket_name = si_settings.cloud_storage_bucket + self.redpanda.set_environment( + {"__REDPANDA_TEST_DISABLE_BOUNDED_PROPERTY_CHECKS": "ON"} + ) + @cluster(num_nodes=3) @matrix(cloud_storage_type=get_cloud_storage_type()) def test_cloud_retention_deleted_segments_count(self, cloud_storage_type):