Merge pull request #15345 from vespa-engine/vekterli/reject-operations-with-too-few-bucket-bits-used

Reject incoming operations with too few bucket bits set

3 files changed, 51 insertions, 2 deletions

diff --git a/storage/src/tests/storageserver/bouncertest.cpp b/storage/src/tests/storageserver/bouncertest.cpp
index bcfe2bd0830..097ea118f0c 100644
--- a/storage/src/tests/storageserver/bouncertest.cpp
+++ b/storage/src/tests/storageserver/bouncertest.cpp
@@ -12,6 +12,7 @@
 #include <vespa/document/test/make_document_bucket.h>
 #include <vespa/document/fieldset/fieldsets.h>
 #include <vespa/storageapi/message/persistence.h>
+#include <vespa/persistence/spi/bucket_limits.h>
 #include <vespa/config/common/exceptions.h>
 #include <vespa/vespalib/gtest/gtest.h>
 
@@ -336,5 +337,29 @@ TEST_F(BouncerTest, allow_get_operations_when_node_is_in_maintenance_mode) {
     EXPECT_EQ(0, _manager->metrics().unavailable_node_aborts.getValue());
 }
 
+namespace {
+
+std::shared_ptr<api::RemoveCommand> make_remove_with_used_bits(uint8_t n_bits) {
+    auto id = document::DocumentId("id:ns:foo::bar");
+    auto raw_bucket = id.getGlobalId().convertToBucketId();
+    return std::make_shared<api::RemoveCommand>(
+            makeDocumentBucket(document::BucketId(n_bits, raw_bucket.getRawId())),
+            id, api::Timestamp(1000));
+}
+
+}
+
+TEST_F(BouncerTest, operation_with_too_few_bucket_bits_is_rejected) {
+    auto cmd = make_remove_with_used_bits(spi::BucketLimits::MinUsedBits - 1);
+    _upper->sendDown(std::move(cmd));
+    expectMessageBouncedWithRejection();
+}
+
+TEST_F(BouncerTest, operation_with_sufficient_bucket_bits_is_not_rejected) {
+    auto cmd = make_remove_with_used_bits(spi::BucketLimits::MinUsedBits);
+    _upper->sendDown(std::move(cmd));
+    expectMessageNotBounced();
+}
+
 } // storage
 
diff --git a/storage/src/vespa/storage/storageserver/bouncer.cpp b/storage/src/vespa/storage/storageserver/bouncer.cpp
index 8a0cffedd05..9ef8a02b74e 100644
--- a/storage/src/vespa/storage/storageserver/bouncer.cpp
+++ b/storage/src/vespa/storage/storageserver/bouncer.cpp
@@ -4,10 +4,12 @@
 #include "bouncer_metrics.h"
 #include "config_logging.h"
 #include <vespa/vdslib/state/cluster_state_bundle.h>
+#include <vespa/persistence/spi/bucket_limits.h>
 #include <vespa/storageapi/message/state.h>
 #include <vespa/storageapi/message/persistence.h>
 #include <vespa/config/subscription/configuri.h>
 #include <vespa/config/common/exceptions.h>
+#include <vespa/vespalib/util/stringfmt.h>
 #include <sstream>
 
 #include <vespa/log/log.h>
@@ -224,12 +226,26 @@ Bouncer::rejectDueToInsufficientPriority(
     sendUp(reply);
 }
 
+void
+Bouncer::reject_due_to_too_few_bucket_bits(api::StorageMessage& msg) {
+    std::shared_ptr<api::StorageReply> reply(
+            dynamic_cast<api::StorageCommand&>(msg).makeReply());
+    reply->setResult(api::ReturnCode(api::ReturnCode::REJECTED,
+                                     vespalib::make_string("Operation bucket %s has too few bits used (%u < minimum of %u)",
+                                                           msg.getBucketId().toString().c_str(),
+                                                           msg.getBucketId().getUsedBits(),
+                                                           spi::BucketLimits::MinUsedBits)));
+    sendUp(reply);
+}
+
 bool
 Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg)
 {
     const api::MessageType& type(msg->getType());
-        // All replies can come in.
-    if (type.isReply()) return false;
+    // All replies can come in.
+    if (type.isReply()) {
+        return false;
+    }
 
     switch (type.getId()) {
         case api::MessageType::SETNODESTATE_ID:
@@ -303,6 +319,12 @@ Bouncer::onDown(const std::shared_ptr<api::StorageMessage>& msg)
         abortCommandDueToClusterDown(*msg);
         return true;
     }
+
+    const auto bucket_id = msg->getBucketId();
+    if ((bucket_id.getId() != 0) && (bucket_id.getUsedBits() < spi::BucketLimits::MinUsedBits)) {
+        reject_due_to_too_few_bucket_bits(*msg);
+        return true;
+    }
     return false;
 }
 
diff --git a/storage/src/vespa/storage/storageserver/bouncer.h b/storage/src/vespa/storage/storageserver/bouncer.h
index ee51114fcbe..7e1d6eaa4ba 100644
--- a/storage/src/vespa/storage/storageserver/bouncer.h
+++ b/storage/src/vespa/storage/storageserver/bouncer.h
@@ -67,6 +67,8 @@ private:
     void rejectDueToInsufficientPriority(api::StorageMessage&,
                                          api::StorageMessage::Priority);
 
+    void reject_due_to_too_few_bucket_bits(api::StorageMessage&);
+
     bool clusterIsUp() const;
 
     bool isDistributor() const;