SERVER-66735: New addShard scenarios

2 files changed, 140 insertions, 32 deletions

diff --git a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp
index 9b4e340e508..a71478246f9 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager_add_shard_test.cpp
@@ -1288,7 +1288,6 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) {
                                                     existingShard.toBSON(),
                                                     ShardingCatalogClient::kMajorityWriteConcern));
     assertShardExists(existingShard);
-
     // Adding the same connection string with a different shard name should fail.
     std::string differentName = "anotherShardName";
     auto future1 = launchAsync([&] {
@@ -1303,6 +1302,18 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) {
     // Ensure that the shard document was unchanged.
     assertShardExists(existingShard);
 
+    // Adding a different connection string with the same shard name should fail.
+    ConnectionString otherHostConnString2 =
+        assertGet(ConnectionString::parse("mySet1/host2:12345"));
+    auto future2 = launchAsync([&] {
+        ThreadClient tc(getServiceContext());
+        auto opCtx = Client::getCurrent()->makeOperationContext();
+        ASSERT_EQUALS(ErrorCodes::IllegalOperation,
+                      ShardingCatalogManager::get(opCtx.get())
+                          ->addShard(opCtx.get(), &existingShardName, otherHostConnString2));
+    });
+    future2.timed_get(kLongFutureTimeout);
+
     // Ensure that the shard document was unchanged.
     assertShardExists(existingShard);
 
@@ -1367,7 +1378,7 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) {
     assertShardExists(existingShard);
 
     // Adding the same replica set but different host membership (but otherwise the same options)
-    // should succeed
+    // should fail.
     auto otherHost = connString.getServers().back();
     ConnectionString otherHostConnString = assertGet(ConnectionString::parse("mySet/host2:12345"));
     {
@@ -1381,9 +1392,9 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) {
     auto future7 = launchAsync([&] {
         ThreadClient tc(getServiceContext());
         auto opCtx = Client::getCurrent()->makeOperationContext();
-        auto shardName = assertGet(ShardingCatalogManager::get(opCtx.get())
-                                       ->addShard(opCtx.get(), nullptr, otherHostConnString));
-        ASSERT_EQUALS(existingShardName, shardName);
+        ASSERT_EQUALS(ErrorCodes::IllegalOperation,
+                      ShardingCatalogManager::get(opCtx.get())
+                          ->addShard(opCtx.get(), nullptr, otherHostConnString));
     });
     future7.timed_get(kLongFutureTimeout);
 
@@ -1391,5 +1402,77 @@ TEST_F(AddShardTest, AddExistingShardReplicaSet) {
     assertShardExists(existingShard);
 }
 
+// Tests both that trying to add a shard with a different replica set as an existing shard but with
+// overlapping hosts fails, and that adding a shard with the same replica set as an existing shard
+// with overlapping hosts succeeds.
+TEST_F(AddShardTest, AddShardWithOverlappingHosts) {
+    std::unique_ptr<RemoteCommandTargeterMock> replsetTargeter(
+        std::make_unique<RemoteCommandTargeterMock>());
+    ConnectionString connString =
+        assertGet(ConnectionString::parse("mySet/host1:12345,host2:12345,host3:12345"));
+    replsetTargeter->setConnectionStringReturnValue(connString);
+    HostAndPort shardTarget = connString.getServers().front();
+    replsetTargeter->setFindHostReturnValue(shardTarget);
+    targeterFactory()->addTargeterToReturn(connString, std::move(replsetTargeter));
+
+    std::string existingShardName = "myShard";
+    ShardType existingShard;
+    existingShard.setName(existingShardName);
+    existingShard.setHost(connString.toString());
+    existingShard.setState(ShardType::ShardState::kShardAware);
+
+    // Make sure the shard already exists.
+    ASSERT_OK(catalogClient()->insertConfigDocument(operationContext(),
+                                                    NamespaceString::kConfigsvrShardsNamespace,
+                                                    existingShard.toBSON(),
+                                                    ShardingCatalogClient::kMajorityWriteConcern));
+    assertShardExists(existingShard);
+
+    // Adding a shard with a different replica set name but with some common hosts should fail.
+    auto otherHost = connString.getServers().front();
+    ConnectionString otherHostConnString =
+        assertGet(ConnectionString::parse("mySet1/host1:12345,host2:12345,host4:12345"));
+    {
+        // Add a targeter for the different seed string this addShard request will use.
+        std::unique_ptr<RemoteCommandTargeterMock> otherHostTargeter(
+            std::make_unique<RemoteCommandTargeterMock>());
+        otherHostTargeter->setConnectionStringReturnValue(otherHostConnString);
+        otherHostTargeter->setFindHostReturnValue(otherHost);
+        targeterFactory()->addTargeterToReturn(otherHostConnString, std::move(otherHostTargeter));
+    }
+    auto future1 = launchAsync([&] {
+        ThreadClient tc(getServiceContext());
+        auto opCtx = Client::getCurrent()->makeOperationContext();
+        ASSERT_EQUALS(ErrorCodes::IllegalOperation,
+                      ShardingCatalogManager::get(opCtx.get())
+                          ->addShard(opCtx.get(), nullptr, otherHostConnString));
+    });
+    future1.timed_get(kLongFutureTimeout);
+    // Ensure that the shard document was unchanged.
+    assertShardExists(existingShard);
+
+    // Adding a shard with the same replica set name and some common hosts should pass.
+    ConnectionString otherHostConnString1 =
+        assertGet(ConnectionString::parse("mySet/host1:12345,host2:12345,host4:12345"));
+    {
+        // Add a targeter for the different seed string this addShard request will use.
+        std::unique_ptr<RemoteCommandTargeterMock> otherHostTargeter(
+            std::make_unique<RemoteCommandTargeterMock>());
+        otherHostTargeter->setConnectionStringReturnValue(otherHostConnString1);
+        otherHostTargeter->setFindHostReturnValue(otherHost);
+        targeterFactory()->addTargeterToReturn(otherHostConnString1, std::move(otherHostTargeter));
+    }
+    auto future2 = launchAsync([&] {
+        ThreadClient tc(getServiceContext());
+        auto opCtx = Client::getCurrent()->makeOperationContext();
+        auto shardName =
+            assertGet(ShardingCatalogManager::get(opCtx.get())
+                          ->addShard(opCtx.get(), &existingShardName, otherHostConnString1));
+        ASSERT_EQUALS(existingShardName, shardName);
+    });
+    future2.timed_get(kLongFutureTimeout);
+    // Ensure that the shard document was unchanged.
+    assertShardExists(existingShard);
+}
 }  // namespace
 }  // namespace mongo
diff --git a/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp b/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp
index 274ca03fdc5..c8cf7edde7f 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager_shard_operations.cpp
@@ -262,15 +262,55 @@ StatusWith<boost::optional<ShardType>> ShardingCatalogManager::_checkIfShardExis
             return true;
         };
 
+        // Function for determining if there is an overlap amongst the hosts of the existing shard
+        // and the propsed shard.
+        auto checkIfHostsAreEquivalent =
+            [&](bool checkShardEquivalency) -> StatusWith<boost::optional<ShardType>> {
+            for (const auto& existingHost : existingShardConnStr.getServers()) {
+                for (const auto& addingHost : proposedShardConnectionString.getServers()) {
+                    if (existingHost == addingHost) {
+                        if (checkShardEquivalency) {
+                            // At least one of the hosts in the shard being added already exists in
+                            // an existing shard. If the options aren't the same, then this is an
+                            // error, but if the options match then the addShard operation should be
+                            // immediately considered a success and terminated.
+                            if (shardsAreEquivalent()) {
+                                return {existingShard};
+                            } else {
+                                return {ErrorCodes::IllegalOperation,
+                                        str::stream()
+                                            << "'" << addingHost.toString() << "' "
+                                            << "is already a member of the existing shard '"
+                                            << existingShard.getHost() << "' ("
+                                            << existingShard.getName() << ")."};
+                            }
+                        } else {
+                            return {existingShard};
+                        }
+                    }
+                }
+            }
+            return {boost::none};
+        };
+
         if (existingShardConnStr.type() == ConnectionString::ConnectionType::kReplicaSet &&
             proposedShardConnectionString.type() == ConnectionString::ConnectionType::kReplicaSet &&
             existingShardConnStr.getSetName() == proposedShardConnectionString.getSetName()) {
             // An existing shard has the same replica set name as the shard being added.
-            // If the options aren't the same, then this is an error,
-            // but if the options match then the addShard operation should be immediately
-            // considered a success and terminated.
+            // If the options aren't the same, then this is an error.
+            // If the shards are equivalent, there must be some overlap amongst their hosts, if not
+            // then addShard results in an error.
             if (shardsAreEquivalent()) {
-                return {existingShard};
+                auto hostsAreEquivalent = checkIfHostsAreEquivalent(false);
+                if (hostsAreEquivalent.isOK() && hostsAreEquivalent.getValue() != boost::none) {
+                    return {existingShard};
+                } else {
+                    return {ErrorCodes::IllegalOperation,
+                            str::stream()
+                                << "A shard named " << existingShardConnStr.getSetName()
+                                << " containing the replica set '"
+                                << existingShardConnStr.getSetName() << "' already exists"};
+                }
             } else {
                 return {ErrorCodes::IllegalOperation,
                         str::stream() << "A shard already exists containing the replica set '"
@@ -278,32 +318,17 @@ StatusWith<boost::optional<ShardType>> ShardingCatalogManager::_checkIfShardExis
             }
         }
 
-        for (const auto& existingHost : existingShardConnStr.getServers()) {
-            // Look if any of the hosts in the existing shard are present within the shard trying
-            // to be added.
-            for (const auto& addingHost : proposedShardConnectionString.getServers()) {
-                if (existingHost == addingHost) {
-                    // At least one of the hosts in the shard being added already exists in an
-                    // existing shard.  If the options aren't the same, then this is an error,
-                    // but if the options match then the addShard operation should be immediately
-                    // considered a success and terminated.
-                    if (shardsAreEquivalent()) {
-                        return {existingShard};
-                    } else {
-                        return {ErrorCodes::IllegalOperation,
-                                str::stream() << "'" << addingHost.toString() << "' "
-                                              << "is already a member of the existing shard '"
-                                              << existingShard.getHost() << "' ("
-                                              << existingShard.getName() << ")."};
-                    }
-                }
-            }
+        // Look if any of the hosts in the existing shard are present within the shard trying
+        // to be added.
+        auto hostsAreEquivalent = checkIfHostsAreEquivalent(true);
+        if (!hostsAreEquivalent.isOK() || hostsAreEquivalent.getValue() != boost::none) {
+            return hostsAreEquivalent;
         }
 
         if (proposedShardName && *proposedShardName == existingShard.getName()) {
-            // If we get here then we're trying to add a shard with the same name as an existing
-            // shard, but there was no overlap in the hosts between the existing shard and the
-            // proposed connection string for the new shard.
+            // If we get here then we're trying to add a shard with the same name as an
+            // existing shard, but there was no overlap in the hosts between the existing
+            // shard and the proposed connection string for the new shard.
             return {ErrorCodes::IllegalOperation,
                     str::stream() << "A shard named " << *proposedShardName << " already exists"};
         }