From e703a5cb0eb86343fe9812ebb2393807990a6351 Mon Sep 17 00:00:00 2001
From: James Harrison <00jamesh@gmail.com>
Date: Tue, 29 Mar 2022 13:30:05 +0100
Subject: [PATCH] MB-51612: Ensure GetClusterConfig/NMVB sends config in mixed
 clusters

In a mixed version cluster, ns_server may set a cluster config with
epoch -1.

get_cluster_config_executor tries to fetch the current config,
requesting anything "newer" than a default constructed config.

Similarly, Cookie::sendNotMyVBucket will only send configs "newer" than
any previously pushed config - or a default constucted config if
none has been pushed.

Unfortunately, a config with epoch -1 appears "older", and triggers
dedupe intended to avoid re-sending configs if the client already
has the current version.

Thus, in a mixed cluster, GetClusterConfig and sendNotMyVBucket may
erroneously consider the live config "older", and will refuse to send
it to clients which need it.

To resolve this, a default constructed config should start at epoch -1.
With this, any valid config set (and validated) by SetClusterConfig
will be "newer".

Change-Id: I470fe247c98c0c5e0d68667ddd40abe9691d8032
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/172962
Well-Formed: Restriction Checker
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
---
 daemon/cluster_config.h                 |  2 +-
 tests/testapp/testapp_cluster_config.cc | 29 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/daemon/cluster_config.h b/daemon/cluster_config.h
index 8b0cf70bb4..79ffd784c6 100644
--- a/daemon/cluster_config.h
+++ b/daemon/cluster_config.h
@@ -21,7 +21,7 @@ class Connection;
 
 class ClustermapVersion {
 public:
-    ClustermapVersion() : epoch(0), revno(0) {
+    ClustermapVersion() : epoch(-1), revno(0) {
     }
     ClustermapVersion(int64_t epoch, int64_t revno)
         : epoch(epoch), revno(revno){};
diff --git a/tests/testapp/testapp_cluster_config.cc b/tests/testapp/testapp_cluster_config.cc
index 9e01260f4b..e310f4e959 100644
--- a/tests/testapp/testapp_cluster_config.cc
+++ b/tests/testapp/testapp_cluster_config.cc
@@ -112,6 +112,35 @@ TEST_P(ClusterConfigTest, GetClusterConfig) {
                                    {value.data(), value.size()}));
 }
 
+TEST_P(ClusterConfigTest, GetClusterConfig_ClusterCompat) {
+    // MB-51612: Test that GetClusterConfig works in a mixed version cluster
+
+    // set a config with an epoch of -1 - used by ns_server when in a mixed
+    // version cluster.
+    int64_t epoch = -1;
+    const std::string config{R"({"rev":100})"};
+    {
+        auto& conn = getAdminConnection();
+        ASSERT_TRUE(
+                conn.execute(BinprotSetClusterConfigCommand{token,
+                                                            config,
+                                                            epoch,
+                                                            100 /*revision */,
+                                                            "default"})
+                        .isSuccess());
+    }
+
+    BinprotGenericCommand cmd{cb::mcbp::ClientOpcode::GetClusterConfig, "", ""};
+    auto& conn = getConnection();
+    const auto response = conn.execute(cmd);
+    EXPECT_TRUE(response.isSuccess()) << to_string(response.getStatus());
+    const auto value = response.getDataString();
+    EXPECT_EQ(config, value);
+    EXPECT_TRUE(hasCorrectDatatype(expectedJSONDatatype(),
+                                   cb::mcbp::Datatype(response.getDatatype()),
+                                   {value.data(), value.size()}));
+}
+
 TEST_P(ClusterConfigTest, test_MB_17506_no_dedupe) {
     test_MB_17506(false);
 }