MB-66628: SetClusterConfig CAS must be 0 [2/8]

ns_server never implemented the support for setting the control token
which means that it has always provided CAS == 0. Remove support for
the control token as the list of commands supporting it isn't very
consistent and a new task should be scheduled to do a full scrub over
all commands if we want to implement this.

Change-Id: I98a507f0ddd9d7125deb14064389d71d7e6ea207
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/227532
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Pavlos Georgiou <pavlos.georgiou@couchbase.com>

Author: trondn

cluster_framework/cluster.cc

@@ -142,7 +142,7 @@ ClusterImpl::ClusterImpl(std::vector<std::unique_ptr<Node>>& nod,
                                  cb::mcbp::Feature::JSON,
                                  cb::mcbp::Feature::SNAPPY});
         auto rsp = connection->execute(BinprotSetClusterConfigCommand{
-                0, globalmap, epoch, revno.load(), {}});
+                globalmap, epoch, revno.load(), {}});
         if (!rsp.isSuccess()) {
             fmt::print(stderr,
                        "Failed to set global CCCP on node {}: {}",
@@ -268,8 +268,7 @@ void ClusterImpl::createBucketOnNode(const Node& node,
     }
 
     rsp = connection->execute(
-            BinprotSetClusterConfigCommand{0,
-                                           bucket.getManifest().dump(2),
+            BinprotSetClusterConfigCommand{bucket.getManifest().dump(2),
                                            epoch,
                                            revno.load(),
                                            bucket.getName()});

daemon/mcbp_validators.cc

@@ -1171,7 +1171,7 @@ static Status set_cluster_config_validator(Cookie& cookie) {
             sizeof(SetClusterConfigPayload),
             ExpectedKeyLen::Any,
             ExpectedValueLen::NonZero,
-            ExpectedCas::Any,
+            ExpectedCas::NotSet,
             GeneratesDocKey::No,
             PROTOCOL_BINARY_RAW_BYTES | PROTOCOL_BINARY_DATATYPE_JSON);
 

daemon/protocol/mcbp/set_cluster_config_command_context.cc

@@ -10,7 +10,6 @@
 
 #include "set_cluster_config_command_context.h"
 #include "daemon/buckets.h"
-#include "daemon/session_cas.h"
 #include "mcbp/protocol/framebuilder.h"
 
 #include <cbsasl/mechanism.h>
@@ -94,15 +93,10 @@ cb::engine_errc SetClusterConfigCommandContext::doSetClusterConfig() {
     // session token.
     cb::engine_errc status;
     auto state = Bucket::State::None;
-    if (!session_cas.execute(
-                sessiontoken, [&status, &state, &configuration, this]() {
-                    auto [rv, st] = BucketManager::instance().setClusterConfig(
-                            bucketname, configuration);
-                    status = rv;
-                    state = st;
-                })) {
-        status = cb::engine_errc::key_already_exists;
-    }
+    auto [rv, st] = BucketManager::instance().setClusterConfig(bucketname,
+                                                               configuration);
+    status = rv;
+    state = st;
 
     if (status == cb::engine_errc::success) {
         return status;

protocol/connection/client_mcbp_commands.cc

@@ -1611,17 +1611,12 @@ void BinprotSetClusterConfigCommand::encode(std::vector<uint8_t>& buf) const {
 }
 
 BinprotSetClusterConfigCommand::BinprotSetClusterConfigCommand(
-        uint64_t token_,
-        std::string config,
-        int64_t epoch,
-        int64_t revision,
-        std::string bucket)
+        std::string config, int64_t epoch, int64_t revision, std::string bucket)
     : BinprotGenericCommand(cb::mcbp::ClientOpcode::SetClusterConfig,
                             std::move(bucket)),
       config(std::move(config)),
       epoch(epoch),
       revision(revision) {
-    setCas(token_);
 }
 
 BinprotObserveSeqnoCommand::BinprotObserveSeqnoCommand(Vbid vbid, uint64_t uuid)

protocol/connection/client_mcbp_commands.h

@@ -1028,8 +1028,7 @@ class BinprotSetControlTokenCommand : public BinprotGenericCommand {
 
 class BinprotSetClusterConfigCommand : public BinprotGenericCommand {
 public:
-    BinprotSetClusterConfigCommand(uint64_t token_,
-                                   std::string config,
+    BinprotSetClusterConfigCommand(std::string config,
                                    int64_t epoch,
                                    int64_t revision,
                                    std::string bucket);

tests/mcbp/mcbp_test_2.cc

@@ -190,9 +190,9 @@ TEST_P(SetClusterConfigValidatorTest, InvalidDatatype) {
     EXPECT_EQ(cb::mcbp::Status::Einval, validate());
 }
 
-TEST_P(SetClusterConfigValidatorTest, Cas) {
+TEST_P(SetClusterConfigValidatorTest, InvalidCas) {
     req.setCas(0xff);
-    EXPECT_EQ(cb::mcbp::Status::Success, validate());
+    EXPECT_EQ(cb::mcbp::Status::Einval, validate());
 }
 
 TEST_P(SetClusterConfigValidatorTest, InvalidBodylen) {

tests/testapp/testapp_cluster_config.cc

@@ -16,9 +16,6 @@ class ClusterConfigTest : public TestappXattrClientTest {
 protected:
     void SetUp() override {
         TestappXattrClientTest::SetUp();
-        // Make sure we've specified a session token
-        setClusterSessionToken(0xdeadbeef);
-
         protocol_binary_datatype_t expected = 0;
         if (hasSnappySupport() == ClientSnappySupport::Everywhere) {
             expected |= PROTOCOL_BINARY_DATATYPE_SNAPPY;
@@ -34,11 +31,10 @@ class ClusterConfigTest : public TestappXattrClientTest {
         userConnection.reset();
     }
 
-    BinprotResponse setClusterConfig(uint64_t token,
-                                     const std::string& config,
+    BinprotResponse setClusterConfig(const std::string& config,
                                      int64_t revision) {
         return adminConnection->execute(BinprotSetClusterConfigCommand{
-                token, config, 1, revision, bucketName});
+                config, 1, revision, bucketName});
     }
 
     void test_MB_17506(bool dedupe, bool client_setting);
@@ -85,7 +81,7 @@ void ClusterConfigTest::test_MB_17506(bool dedupe, bool client_setting) {
     const std::string clustermap{R"({"rev":100})"};
 
     // Make sure we have a cluster configuration installed
-    auto response = setClusterConfig(token, clustermap, 100);
+    auto response = setClusterConfig(clustermap, 100);
     EXPECT_TRUE(response.isSuccess());
 
     BinprotGetCommand command{"foo", Vbid{1}};
@@ -121,24 +117,9 @@ void ClusterConfigTest::test_MB_17506(bool dedupe, bool client_setting) {
     }
 }
 
-TEST_P(ClusterConfigTest, SetClusterConfigWithIncorrectSessionToken) {
-    auto response = setClusterConfig(0xcafebeef, R"({"rev":100})", 100);
-    EXPECT_FALSE(response.isSuccess()) << "Should not be allowed to set "
-                                          "cluster config with invalid session "
-                                          "token";
-    EXPECT_EQ(cb::mcbp::Status::KeyEexists, response.getStatus());
-}
-
-TEST_P(ClusterConfigTest, SetClusterConfigWithCorrectToken) {
-    auto response = setClusterConfig(token, R"({"rev":100})", 100);
-    EXPECT_TRUE(response.isSuccess()) << "Should be allowed to set cluster "
-                                         "config with the correct session "
-                                         "token";
-}
-
 TEST_P(ClusterConfigTest, GetClusterConfig) {
     const std::string config{R"({"rev":100})"};
-    ASSERT_TRUE(setClusterConfig(token, config, 100).isSuccess());
+    ASSERT_TRUE(setClusterConfig(config, 100).isSuccess());
 
     BinprotGenericCommand cmd{cb::mcbp::ClientOpcode::GetClusterConfig};
     const auto response = userConnection->execute(cmd);
@@ -155,11 +136,10 @@ TEST_P(ClusterConfigTest, GetClusterConfig_ClusterCompat) {
     // version cluster.
     int64_t epoch = -1;
     const std::string config{R"({"rev":100})"};
-    ASSERT_TRUE(
-            adminConnection
-                    ->execute(BinprotSetClusterConfigCommand{
-                            token, config, epoch, 100 /*revision */, "default"})
-                    .isSuccess());
+    ASSERT_TRUE(adminConnection
+                        ->execute(BinprotSetClusterConfigCommand{
+                                config, epoch, 100 /*revision */, "default"})
+                        .isSuccess());
 
     BinprotGenericCommand cmd{cb::mcbp::ClientOpcode::GetClusterConfig};
     const auto response = userConnection->execute(cmd);
@@ -235,16 +215,15 @@ void ClusterConfigTest::test_CccpPushNotification(bool global, bool brief) {
     if (global) {
         adminConnection->executeInBucket(bucketName, [&global_map](auto& c) {
             ASSERT_TRUE(c.execute(BinprotSetClusterConfigCommand{
-                                          token, global_map, 2, 532, {}})
+                                          global_map, 2, 532, {}})
 
                                 .isSuccess());
         });
     } else {
         adminConnection->executeInBucket(bucketName, [&bucket_map](auto& c) {
-            ASSERT_TRUE(
-                    c.execute(BinprotSetClusterConfigCommand{
-                                      token, bucket_map, 1, 666, bucketName})
-                            .isSuccess());
+            ASSERT_TRUE(c.execute(BinprotSetClusterConfigCommand{
+                                          bucket_map, 1, 666, bucketName})
+                                .isSuccess());
         });
     }
 
@@ -315,11 +294,11 @@ TEST_P(ClusterConfigTest, ClustermapChangeNotificationBrief_Global) {
 
 TEST_P(ClusterConfigTest, SetGlobalClusterConfig) {
     // Set one for the default bucket
-    setClusterConfig(token, R"({"rev":1000})", 1000);
+    setClusterConfig(R"({"rev":1000})", 1000);
 
     // Set the global config
-    auto rsp = adminConnection->execute(BinprotSetClusterConfigCommand{
-            token, R"({"foo" : "bar"})", 1, 100, ""});
+    auto rsp = adminConnection->execute(
+            BinprotSetClusterConfigCommand{R"({"foo" : "bar"})", 1, 100, ""});
     ASSERT_TRUE(rsp.isSuccess()) << rsp.getDataView();
 
     rsp = adminConnection->execute(
@@ -339,7 +318,7 @@ TEST_P(ClusterConfigTest, SetGlobalClusterConfig) {
  * The bucket configuration was not reset as part of bucket deletion
  */
 TEST_P(ClusterConfigTest, MB35395) {
-    setClusterConfig(token, R"({"rev":1000})", 1000);
+    setClusterConfig(R"({"rev":1000})", 1000);
 
     // Recreate the bucket, and the cluster config should be gone!
     DeleteTestBucket();
@@ -354,7 +333,7 @@ TEST_P(ClusterConfigTest, MB35395) {
 }
 
 TEST_P(ClusterConfigTest, MB57311_RequestWithVersion) {
-    setClusterConfig(token, R"({"rev":1000})", 1000);
+    setClusterConfig(R"({"rev":1000})", 1000);
     auto validatePushedRevno = [](int64_t epoch, int64_t revno) {
         nlohmann::json json;
         userConnection->stats(

tests/testapp/testapp_stats.cc

@@ -525,8 +525,8 @@ TEST_P(StatsTest, TestTasksAllInfo) {
     // Set up a config only bucket too to ensure that things work with that
     // present
     const std::string config = R"({"rev":1000})";
-    auto rsp = adminConnection->execute(BinprotSetClusterConfigCommand{
-            token, config, 1, 1000, "cluster-config"});
+    auto rsp = adminConnection->execute(
+            BinprotSetClusterConfigCommand{config, 1, 1000, "cluster-config"});
     ASSERT_TRUE(rsp.isSuccess()) << rsp.getStatus() << std::endl
                                  << rsp.getDataJson();