SERVER-48411 Increase maximum allowed replica set memberId on disk from 255 to INT_MAX.

7 files changed, 130 insertions, 9 deletions

diff --git a/src/mongo/db/repl/member_config.h b/src/mongo/db/repl/member_config.h
index 87077caf650..00d0b3a8e60 100644
--- a/src/mongo/db/repl/member_config.h
+++ b/src/mongo/db/repl/member_config.h
@@ -70,6 +70,14 @@ public:
     static const std::string kConfigVoterTagName;
 
     /**
+     * Inline `kMaxUserMemberId` to allow others use
+     * the constant without linking to `member_config.cpp`.
+     *
+     * Maximum allowed member id for new nodes added by user.
+     */
+    inline static const int kMaxUserMemberId = 255;
+
+    /**
      * Construct a MemberConfig from the contents of "mcfg".
      *
      * If "mcfg" describes any tags, builds ReplSetTags for this
diff --git a/src/mongo/db/repl/member_config_test.cpp b/src/mongo/db/repl/member_config_test.cpp
index 82f0790bf7d..61f18e2369b 100644
--- a/src/mongo/db/repl/member_config_test.cpp
+++ b/src/mongo/db/repl/member_config_test.cpp
@@ -73,19 +73,26 @@ TEST(MemberConfig, ParseFailsWithMissingIdField) {
                   ExceptionFor<ErrorCodes::NoSuchKey>);
 }
 
-TEST(MemberConfig, ParseFailsWithIdOutOfRange) {
+TEST(MemberConfig, ParseMemberConfigId) {
     ReplSetTagConfig tagConfig;
     {
+        // Fail case
         ASSERT_THROWS(MemberConfig(BSON("_id" << -1 << "host"
                                               << "localhost:12345"),
                                    &tagConfig),
                       ExceptionFor<ErrorCodes::BadValue>);
     }
     {
-        ASSERT_THROWS(MemberConfig(BSON("_id" << -1 << "host"
-                                              << "localhost:12345"),
-                                   &tagConfig),
-                      ExceptionFor<ErrorCodes::BadValue>);
+        // Pass cases
+        MemberConfig(BSON("_id" << 0 << "host"
+                                << "localhost:12345"),
+                     &tagConfig);
+        MemberConfig(BSON("_id" << MemberConfig::kMaxUserMemberId << "host"
+                                << "localhost:12345"),
+                     &tagConfig);
+        MemberConfig(BSON("_id" << (MemberConfig::kMaxUserMemberId + 1) << "host"
+                                << "localhost:12345"),
+                     &tagConfig);
     }
 }
 
diff --git a/src/mongo/db/repl/member_id.h b/src/mongo/db/repl/member_id.h
index 4f2db2b342c..a9cdca09a11 100644
--- a/src/mongo/db/repl/member_id.h
+++ b/src/mongo/db/repl/member_id.h
@@ -45,9 +45,10 @@ public:
     MemberId() : _id(kUninitializedMemberId) {}
 
     explicit MemberId(int id) {
-        if (id < 0 || id > 255) {
+        if (id < 0) {
             uasserted(ErrorCodes::BadValue,
-                      str::stream() << "_id field value of " << id << " is out of range.");
+                      str::stream()
+                          << "_id field value of " << id << " can't be a negative number");
         }
         _id = id;
     }
diff --git a/src/mongo/db/repl/repl_set_config.cpp b/src/mongo/db/repl/repl_set_config.cpp
index 1f703cd1615..90b52b7818d 100644
--- a/src/mongo/db/repl/repl_set_config.cpp
+++ b/src/mongo/db/repl/repl_set_config.cpp
@@ -754,7 +754,7 @@ int ReplSetConfig::findMemberIndexByHostAndPort(const HostAndPort& hap) const {
     return -1;
 }
 
-int ReplSetConfig::findMemberIndexByConfigId(long long configId) const {
+int ReplSetConfig::findMemberIndexByConfigId(int configId) const {
     int x = 0;
     for (const auto& member : _members) {
         if (member.getId() == MemberId(configId)) {
diff --git a/src/mongo/db/repl/repl_set_config.h b/src/mongo/db/repl/repl_set_config.h
index ce93134f3e9..257fb9b11f2 100644
--- a/src/mongo/db/repl/repl_set_config.h
+++ b/src/mongo/db/repl/repl_set_config.h
@@ -302,7 +302,7 @@ public:
      * Returns a MemberConfig index position corresponding to the member with the given
      * _id in the config, or -1 if there is no member with that address.
      */
-    int findMemberIndexByConfigId(long long configId) const;
+    int findMemberIndexByConfigId(int configId) const;
 
     /**
      * Gets the default write concern for the replica set described by this configuration.
diff --git a/src/mongo/db/repl/repl_set_config_checks.cpp b/src/mongo/db/repl/repl_set_config_checks.cpp
index b8927bc9d8d..0462c840fbc 100644
--- a/src/mongo/db/repl/repl_set_config_checks.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks.cpp
@@ -124,6 +124,35 @@ Status validateSingleNodeChange(const ReplSetConfig& oldConfig, const ReplSetCon
 }
 
 /**
+ * Returns ErrorCodes::BadValue if the user tries to add new nodes with member id greater than
+ * MemberConfig::kMaxUserMemberId(255).
+ *
+ * validateMemberId() will be called only for replSetReconfig and replSetInitiate commands.
+ *
+ * TODO SERVER-48345: Remove this method on MongoDB v4.6 as we allow users to add new nodes
+ * with member id > MemberConfig::kMaxUserMemberId(255).
+ */
+Status validateMemberId(const ReplSetConfig& newConfig,
+                        const ReplSetConfig& oldConfig = ReplSetConfig()) {
+    for (ReplSetConfig::MemberIterator mNew = newConfig.membersBegin();
+         mNew != newConfig.membersEnd();
+         ++mNew) {
+        auto mNewConfigId = mNew->getId().getData();
+        // Existing members are allowed to have member id greater than kMaxUserMemberId. And,
+        // it can happen only if this node was previously running on MongoDB version greater
+        // than v4.4.
+        if (oldConfig.findMemberIndexByConfigId(mNewConfigId) == -1 &&
+            mNewConfigId > MemberConfig::kMaxUserMemberId) {
+            return Status(ErrorCodes::BadValue,
+                          str::stream()
+                              << "Replica set configuration contains new members with "
+                              << "member id greater than " << MemberConfig::kMaxUserMemberId);
+        }
+    }
+    return Status::OK();
+}
+
+/**
  * Compares two initialized and validated replica set configurations, and checks to
  * see if "newConfig" is a legal successor configuration to "oldConfig".
  *
@@ -313,6 +342,11 @@ StatusWith<int> validateConfigForInitiate(ReplicationCoordinatorExternalState* e
         return StatusWith<int>(status);
     }
 
+    status = validateMemberId(newConfig);
+    if (!status.isOK()) {
+        return status;
+    }
+
     status = newConfig.checkIfWriteConcernCanBeSatisfied(newConfig.getDefaultWriteConcern());
     if (!status.isOK()) {
         return status.withContext(
@@ -359,6 +393,11 @@ Status validateConfigForReconfig(const ReplSetConfig& oldConfig,
         return status;
     }
 
+    status = validateMemberId(newConfig, oldConfig);
+    if (!status.isOK()) {
+        return status;
+    }
+
     // For non-force reconfigs, verify that the reconfig only adds or removes a single node. This
     // ensures that all quorums of the new config overlap with all quorums of the old config.
     if (!force &&
diff --git a/src/mongo/db/repl/repl_set_config_checks_test.cpp b/src/mongo/db/repl/repl_set_config_checks_test.cpp
index f7ee995a638..dc90eb6d115 100644
--- a/src/mongo/db/repl/repl_set_config_checks_test.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks_test.cpp
@@ -78,6 +78,23 @@ TEST_F(ServiceContextTest, ValidateConfigForInitiate_TermIsAlwaysInitialTerm) {
     ASSERT_EQUALS(config.getConfigTerm(), OpTime::kInitialTerm);
 }
 
+TEST_F(ServiceContextTest, ValidateConfigForInitiate_memberId) {
+    ReplicationCoordinatorExternalStateMock rses;
+    rses.addSelf(HostAndPort("h1"));
+
+    // Config with Member id > 255.
+    ReplSetConfig invalidConfig;
+    ASSERT_OK(invalidConfig.initializeForInitiate(
+        BSON("_id"
+             << "rs0"
+             << "version" << 1 << "protocolVersion" << 1 << "members"
+             << BSON_ARRAY(BSON("_id" << (MemberConfig::kMaxUserMemberId + 1) << "host"
+                                      << "h1")))));
+    ASSERT_EQUALS(
+        ErrorCodes::BadValue,
+        validateConfigForInitiate(&rses, invalidConfig, getGlobalServiceContext()).getStatus());
+}
+
 TEST_F(ServiceContextTest, ValidateConfigForInitiate_MustFindSelf) {
     ReplSetConfig config;
     ASSERT_OK(
@@ -249,6 +266,55 @@ TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigVersionNumberMustB
                   validateConfigForReconfig(newConfig, oldConfig, false));
 }
 
+TEST_F(ServiceContextTest, ValidateConfigForReconfig_memberId) {
+    ReplicationCoordinatorExternalStateMock externalState;
+    externalState.addSelf(HostAndPort("h1"));
+
+    ReplSetConfig oldConfig;
+    ReplSetConfig newConfig;
+
+    // Case 1: Add a new node with member id > 255.
+    ASSERT_OK(oldConfig.initialize(BSON("_id"
+                                        << "rs0"
+                                        << "version" << 1 << "protocolVersion" << 1 << "members"
+                                        << BSON_ARRAY(BSON("_id" << 1 << "host"
+                                                                 << "h1")))));
+    ASSERT_OK(newConfig.initialize(
+        BSON("_id"
+             << "rs0"
+             << "version" << 2 << "protocolVersion" << 1 << "members"
+             << BSON_ARRAY(BSON("_id" << 1 << "host"
+                                      << "h1")
+                           << BSON("_id" << (MemberConfig::kMaxUserMemberId + 1) << "host"
+                                         << "h2")))));
+    ASSERT_OK(oldConfig.validate());
+    ASSERT_OK(newConfig.validate());
+    ASSERT_EQUALS(ErrorCodes::BadValue, validateConfigForReconfig(oldConfig, newConfig, false));
+
+    // Case 2: Change the member config setting for the existing member with member id > 255.
+    ASSERT_OK(oldConfig.initialize(
+        BSON("_id"
+             << "rs0"
+             << "version" << 1 << "protocolVersion" << 1 << "members"
+             << BSON_ARRAY(BSON("_id" << 1 << "host"
+                                      << "h1")
+                           << BSON("_id" << (MemberConfig::kMaxUserMemberId + 1) << "host"
+                                         << "h2")))));
+    ASSERT_OK(newConfig.initialize(
+        BSON("_id"
+             << "rs0"
+             << "version" << 2 << "protocolVersion" << 1 << "members"
+             << BSON_ARRAY(BSON("_id" << 1 << "host"
+                                      << "h1")
+                           << BSON("_id" << (MemberConfig::kMaxUserMemberId + 1) << "host"
+                                         << "h2"
+                                         << "priority" << 0)))));
+    ASSERT_OK(oldConfig.validate());
+    ASSERT_OK(newConfig.validate());
+    ASSERT_OK(validateConfigForReconfig(oldConfig, newConfig, false));
+}
+
+
 TEST_F(ServiceContextTest, ValidateConfigForReconfig_NewConfigMustNotChangeSetName) {
     ReplicationCoordinatorExternalStateMock externalState;
     externalState.addSelf(HostAndPort("h1"));