CORE-15436 delete topic enable#29365
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the delete_topic_enable cluster configuration property to provide a global safety mechanism for preventing topic deletion via the Kafka DeleteTopics API, similar to Apache Kafka's delete.topic.enable broker configuration.
Changes:
- Added a new boolean cluster configuration property
delete_topic_enable(default:true) that globally controls topic deletion - Implemented early rejection of DeleteTopics requests when the setting is disabled, returning
TOPIC_DELETION_DISABLED(error 73) for API v3+ orINVALID_REQUESTfor older API versions - Added comprehensive test coverage in both C++ unit tests and Python integration tests to verify the feature works correctly
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/v/config/configuration.h |
Declares the new delete_topic_enable property field in the configuration structure |
src/v/config/configuration.cc |
Defines the delete_topic_enable property with documentation, default value, and metadata |
src/v/kafka/server/server.cc |
Implements the early-rejection logic in the DeleteTopics handler to check the configuration and return appropriate error codes |
src/v/kafka/server/tests/delete_topics_test.cc |
Adds C++ unit tests verifying behavior with the setting disabled, for different API versions, and when re-enabled |
tests/rptest/tests/delete_topic_enable_test.py |
Adds Python integration tests covering disabled state, default behavior, interaction with kafka_nodelete_topics, and multiple topic deletion attempts |
5f865ff to
6d4a767
Compare
|
Force push:
|
18f3bdc to
d1bb385
Compare
|
Force push:
|
CI test resultstest results on build#79504
|
Add a new cluster configuration property `delete_topic_enable` that allows administrators to globally disable topic deletion via the Kafka DeleteTopics API, similar to Apache Kafka's `delete.topic.enable`. When set to false, all topic deletion requests are rejected with error code 73 (TOPIC_DELETION_DISABLED). This is a safety setting that cannot be overridden by superusers. Default is true for backward compatibility. Refs: CORE-15346 Signed-off-by: Michael Boquard <michael@redpanda.com>
Add topic_deletion_disabled to the license_required_feature enum and report it when delete_topic_enable is disabled. This allows license enforcement for clusters that have topic deletion disabled.
Check the delete_topic_enable config at the start of the DeleteTopics handler. When disabled, reject all deletion requests with error code 73 (TOPIC_DELETION_DISABLED) for API v3+, or INVALID_REQUEST for older clients that don't understand error 73. The check is placed before any validation or authorization to ensure fast rejection and consistent behavior regardless of topic existence or permissions. Also adds unit tests verifying: - Error code 73 returned for modern API versions - INVALID_REQUEST returned for old API versions (< v3) - Dynamic config toggle works correctly Refs: CORE-15346 Signed-off-by: Michael Boquard <michael@redpanda.com>
Add ducktape integration tests verifying the delete_topic_enable cluster configuration: - test_delete_topic_enable_disabled: Topics cannot be deleted when config is false, returns TOPIC_DELETION_DISABLED error - test_delete_topic_enable_default: Default value (true) allows deletion for backward compatibility - test_nodelete_topics_still_protected: kafka_nodelete_topics is still honored when delete_topic_enable is true - test_delete_topic_enable_multiple_topics: All topics rejected when config is false Refs: CORE-15346 Signed-off-by: Michael Boquard <michael@redpanda.com>
Add topic_deletion_disabled feature to the enterprise features license test. The feature is enabled when delete_topic_enable is set to false.
d1bb385 to
b71750e
Compare
|
Force push:
|
| 1_MiB) | ||
| , delete_topic_enable( | ||
| *this, | ||
| false, |
There was a problem hiding this comment.
The delete_topic_enable property is initialized with false as the second parameter to the constructor, but according to the PR description and line 1768, the default value should be true for backward compatibility. The second parameter appears to be the enterprise license requirement flag (not the default value), and the actual default value true is passed as the last parameter on line 1768. However, passing false for the license requirement means this feature does NOT require an enterprise license to use. According to the enterprise feature tracking in feature_manager.cc (lines 281-283) and the test enterprise_features_license_test.py, this feature should require an enterprise license when DISABLED (when set to false). The license requirement flag should be true to indicate that disabling topic deletion is an enterprise feature.
| false, | |
| true, |
There was a problem hiding this comment.
the second parameter is the restricted value. All good here.
| const auto ec = (ctx.header().version >= api_version(3)) | ||
| ? error_code::topic_deletion_disabled | ||
| : error_code::invalid_request; | ||
| const auto err_msg = "Topic deletion is disabled."; |
There was a problem hiding this comment.
The error message is constructed as a const auto and captured by value in the lambda expressions below. This results in the string literal being copied multiple times for each topic in the request. Consider using std::string_view or moving the message construction inside the response building logic to avoid unnecessary copies, or capture by reference if the lifetime is guaranteed.
There was a problem hiding this comment.
this is just a const char* so copilot is wrong. However, why it a const char* instead of const char[] is... very c++...
I would argue that auto should be replaced with an explicit type (probs string_view as the comment says... but for very different reasons)
| report.set( | ||
| features::license_required_feature::topic_deletion_disabled, | ||
| !cfg.delete_topic_enable()); |
There was a problem hiding this comment.
yeah, an extra inversion won't confuse anyone... it's fine 😅
(i see why it needs to be like that, but i had to call it out 🙄)
There was a problem hiding this comment.
yeah... i struggled with this one for that reason...
| const auto ec = (ctx.header().version >= api_version(3)) | ||
| ? error_code::topic_deletion_disabled | ||
| : error_code::invalid_request; | ||
| const auto err_msg = "Topic deletion is disabled."; |
There was a problem hiding this comment.
this is just a const char* so copilot is wrong. However, why it a const char* instead of const char[] is... very c++...
I would argue that auto should be replaced with an explicit type (probs string_view as the comment says... but for very different reasons)
| 1_MiB) | ||
| , delete_topic_enable( | ||
| *this, | ||
| false, |
There was a problem hiding this comment.
the second parameter is the restricted value. All good here.
Add
delete_topic_enablecluster configurationThis PR implements a new cluster configuration property
delete_topic_enablethat allows administrators to globally disable topic deletion via the Kafka DeleteTopics API, similar to Apache Kafka'sdelete.topic.enablebroker configuration.Motivation: Customers have requested the ability to prevent accidental topic deletion in production environments. While
kafka_nodelete_topicsprotects specific topics, there was no way to globally disable topic deletion as a safety measure.Implementation:
delete_topic_enableas a dynamic boolean cluster config (default:truefor backward compatibility)false, all DeleteTopics requests are rejected with error code 73 (TOPIC_DELETION_DISABLED) for API v3+, orINVALID_REQUESTfor older clientsFixes #29347
Backports Required
Release Notes
Features
delete_topic_enablecluster configuration property to globally enable or disable topic deletion via the Kafka API. When set tofalse, all topic deletion requests are rejected with error codeTOPIC_DELETION_DISABLED(73). Default istruefor backward compatibility. This setting works independently ofkafka_nodelete_topics, which continues to protect specific topics regardless of this setting.