Skip to content

Commit

Permalink
MB-43205: Introduce the allow_sanitize_value_in_deletion config param
Browse files Browse the repository at this point in the history
This is a renaming of the previous allow_del_with_meta_prune_user_data
config param.

As per its old name, that was a switch for enabling/disabling
del_with_meta operating in "sanitizer" mode rather than enforcing a
strict validation and failing the operation in the case of invalid
payload.
Now we want to extend the same behaviour to DCP_DELETE/DCP_PREPARE.

That is because in 6.6.0 we have introduced some stricter validation
on deletion payloads that may fail the operation. That may happen
mainly in the case of pre-6.6 to 6.6 offline upgrade.

Under this MB we want to introduce the possibility to set a replica
in "sanitizer" mode, the same as we already do at del_with_meta. That
way, DCP_DELETE/DCP_PREPARE will just remove any invalid body in the
payload rather that failing.

Change-Id: Ia9faff48de3a51a77d367961b45c41ed45c609d1
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/143656
Well-Formed: Build Bot <build@couchbase.com>
Tested-by: Paolo Cocchi <paolo.cocchi@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
  • Loading branch information
paolococchi committed Jan 20, 2021
1 parent 51684ab commit 8d13b01
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 19 deletions.
4 changes: 2 additions & 2 deletions engines/ep/configuration.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"params": {
"allow_del_with_meta_prune_user_data" : {
"allow_sanitize_value_in_deletion" : {
"default" : "true",
"descr": "Let del_with_meta prune user data provided from the user instead of failing",
"descr": "Let EPE delete/prepare/del_with_meta prune any invalid body in the payload instead of failing",
"dynamic" : true,
"type" : "bool"
},
Expand Down
16 changes: 8 additions & 8 deletions engines/ep/src/ep_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -744,8 +744,8 @@ cb::mcbp::Status EventuallyPersistentEngine::setFlushParam(
getConfiguration().setCouchstoreWriteValidation(cb_stob(val));
} else if (key == "couchstore_mprotect") {
getConfiguration().setCouchstoreMprotect(cb_stob(val));
} else if (key == "allow_del_with_meta_prune_user_data") {
getConfiguration().setAllowDelWithMetaPruneUserData(cb_stob(val));
} else if (key == "allow_sanitize_value_in_deletion") {
getConfiguration().setAllowSanitizeValueInDeletion(cb_stob(val));
} else {
msg = "Unknown config param";
rv = cb::mcbp::Status::KeyEnoent;
Expand Down Expand Up @@ -2051,8 +2051,8 @@ class EpEngineValueChangeListener : public ValueChangedListener {
}

void booleanValueChanged(const std::string& key, bool b) override {
if (key == "allow_del_with_meta_prune_user_data") {
engine.allowDelWithMetaPruneUserData.store(b);
if (key == "allow_sanitize_value_in_deletion") {
engine.allowSanitizeValueInDeletion.store(b);
}
}

Expand Down Expand Up @@ -2139,10 +2139,10 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::initialize(const char* config) {
"getl_max_timeout",
std::make_unique<EpEngineValueChangeListener>(*this));

allowDelWithMetaPruneUserData.store(
configuration.isAllowDelWithMetaPruneUserData());
allowSanitizeValueInDeletion.store(
configuration.isAllowSanitizeValueInDeletion());
configuration.addValueChangedListener(
"allow_del_with_meta_prune_user_data",
"allow_sanitize_value_in_deletion",
std::make_unique<EpEngineValueChangeListener>(*this));

auto numShards = configuration.getMaxNumShards();
Expand Down Expand Up @@ -5554,7 +5554,7 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::deleteWithMeta(
datatype &= ~PROTOCOL_BINARY_DATATYPE_SNAPPY;
}

if (allowDelWithMetaPruneUserData) {
if (allowSanitizeValueInDeletion) {
if (mcbp::datatype::is_xattr(datatype)) {
// Whatever we have in the value, just keep Xattrs
const auto valBuffer = cb::const_char_buffer{
Expand Down
9 changes: 8 additions & 1 deletion engines/ep/src/ep_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -1161,5 +1161,12 @@ class EventuallyPersistentEngine : public EngineIface, public DcpIface {
EpEngineTaskable taskable;
std::atomic<BucketCompressionMode> compressionMode;
std::atomic<float> minCompressionRatio;
std::atomic_bool allowDelWithMetaPruneUserData;

/**
* Whether del-operations at EPE level (currently only DelWithMeta) should
* just sanitize invalid payloads or fail the operation if an invalid
* payload is detected.
* Non-const as the related configuration param is dynamic.
*/
std::atomic_bool allowSanitizeValueInDeletion;
};
4 changes: 2 additions & 2 deletions engines/ep/tests/ep_testsuite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6961,7 +6961,7 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
{"info", {"info"}},
{"allocator", {"detailed"}},
{"config",
{"ep_allow_del_with_meta_prune_user_data",
{"ep_allow_sanitize_value_in_deletion",
"ep_backend",
"ep_backfill_mem_threshold",
"ep_bfilter_enabled",
Expand Down Expand Up @@ -7131,7 +7131,7 @@ static enum test_result test_mb19687_fixed(EngineIface* h) {
"vb_0:num_entries",
"vb_0:num_erroneous_entries_erased"}},
{"", // Note: we convert empty to a null to get engine stats
{"ep_allow_del_with_meta_prune_user_data",
{"ep_allow_sanitize_value_in_deletion",
"bytes",
"curr_items",
"curr_items_tot",
Expand Down
4 changes: 2 additions & 2 deletions engines/ep/tests/mock/mock_synchronous_ep_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ SynchronousEPEngine::SynchronousEPEngine(std::string extra_config)
maxItemSize = configuration.getMaxItemSize();

setCompressionMode(configuration.getCompressionMode());
allowDelWithMetaPruneUserData =
configuration.isAllowDelWithMetaPruneUserData();
allowSanitizeValueInDeletion =
configuration.isAllowSanitizeValueInDeletion();
}

void SynchronousEPEngine::setKVBucket(std::unique_ptr<KVBucket> store) {
Expand Down
2 changes: 1 addition & 1 deletion engines/ep/tests/module_tests/evp_store_with_meta.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class WithMetaTest : public SingleThreadedEPBucketTest {
if (!config_string.empty()) {
config_string += ";";
}
config_string += "allow_del_with_meta_prune_user_data=true";
config_string += "allow_sanitize_value_in_deletion=true";
SingleThreadedEPBucketTest::SetUp();
store->setVBucketState(vbid, vbucket_state_active);
expiry = ep_real_time() + 31557600; // +1 year in seconds
Expand Down
6 changes: 3 additions & 3 deletions tests/testapp/testapp_withmeta.cc
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ TEST_P(WithMetaTest, MB36321_DeleteWithMetaRefuseUserXattrs) {
conn.selectBucket(bucketName);
const auto setParam = BinprotSetParamCommand(
cb::mcbp::request::SetParamPayload::Type::Flush,
"allow_del_with_meta_prune_user_data",
"allow_sanitize_value_in_deletion",
"false");
const auto resp = BinprotMutationResponse(conn.execute(setParam));
ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
Expand Down Expand Up @@ -292,7 +292,7 @@ void WithMetaTest::testDeleteWithMetaAcceptsUserXattrs(bool allowValuePruning,
conn.selectBucket(bucketName);
const auto setParam = BinprotSetParamCommand(
cb::mcbp::request::SetParamPayload::Type::Flush,
"allow_del_with_meta_prune_user_data",
"allow_sanitize_value_in_deletion",
allowValuePruning ? "true" : "false");
const auto resp = BinprotMutationResponse(conn.execute(setParam));
ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
Expand Down Expand Up @@ -356,7 +356,7 @@ void WithMetaTest::testDeleteWithMetaRejectsBody(bool allowValuePruning,
conn.selectBucket(bucketName);
const auto setParam = BinprotSetParamCommand(
cb::mcbp::request::SetParamPayload::Type::Flush,
"allow_del_with_meta_prune_user_data",
"allow_sanitize_value_in_deletion",
allowValuePruning ? "true" : "false");
const auto resp = BinprotMutationResponse(conn.execute(setParam));
ASSERT_EQ(cb::mcbp::Status::Success, resp.getStatus());
Expand Down

0 comments on commit 8d13b01

Please sign in to comment.