SERVER-25602 Make split/mergeChunks commands check the validity of input

6 files changed, 82 insertions, 46 deletions

diff --git a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
index fdbaedebda1..02d4bc050f1 100644
--- a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
+++ b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
@@ -43,6 +43,7 @@ selector:
     - jstests/sharding/cursor_timeout.js
     # Bug in v3.2 shards, fixed in v3.4.
     - jstests/sharding/auth_sharding_cmd_metadata.js
+    - jstests/sharding/split_against_shard_with_invalid_split_points.js
     # TODO Requires mongos to have fix for SERVER-25258. Enable when v3.2.9+ is 'last-stable'.
     - jstests/sharding/kill_op_overflow.js
     # TODO Assumes shardCollection can handle the collation option; enable when 3.4 becomes
diff --git a/jstests/sharding/split_against_shard_with_invalid_split_points.js b/jstests/sharding/split_against_shard_with_invalid_split_points.js
new file mode 100644
index 00000000000..0ae5424983f
--- /dev/null
+++ b/jstests/sharding/split_against_shard_with_invalid_split_points.js
@@ -0,0 +1,42 @@
+// Tests that executing splitChunk directly against a shard, with an invalid split point will not
+// corrupt the chunks metadata
+(function() {
+    'use strict';
+
+    var st = new ShardingTest({shards: 1});
+
+    var testDB = st.s.getDB('TestSplitDB');
+    assert.commandWorked(testDB.adminCommand({enableSharding: 'TestSplitDB'}));
+    st.ensurePrimaryShard('TestSplitDB', st.shard0.shardName);
+
+    assert.commandWorked(testDB.adminCommand({shardCollection: 'TestSplitDB.Coll', key: {x: 1}}));
+    assert.commandWorked(testDB.adminCommand({split: 'TestSplitDB.Coll', middle: {x: 0}}));
+
+    var chunksBefore = st.s.getDB('config').chunks.find().toArray();
+
+    // Try to do a split with invalid parameters through mongod
+    var callSplit = function(db, minKey, maxKey, splitPoints) {
+        var res = assert.commandWorked(st.s.adminCommand({getShardVersion: 'TestSplitDB.Coll'}));
+        var shardVersion = [res.version, res.versionEpoch];
+
+        return db.runCommand({
+            splitChunk: 'TestSplitDB.Coll',
+            from: st.shard0.shardName,
+            min: minKey,
+            max: maxKey,
+            keyPattern: {x: 1},
+            splitKeys: splitPoints,
+            shardVersion: shardVersion,
+        });
+    };
+
+    assert.commandFailedWithCode(callSplit(st.d0.getDB('admin'), {x: MinKey}, {x: 0}, [{x: 2}]),
+                                 ErrorCodes.IllegalOperation);
+
+    var chunksAfter = st.s.getDB('config').chunks.find().toArray();
+    assert.eq(chunksBefore,
+              chunksAfter,
+              'Split chunks failed, but the chunks were updated in the config database');
+
+    st.stop();
+})();
diff --git a/src/mongo/db/s/collection_metadata.cpp b/src/mongo/db/s/collection_metadata.cpp
index ac902c3feb2..e3a576d88d3 100644
--- a/src/mongo/db/s/collection_metadata.cpp
+++ b/src/mongo/db/s/collection_metadata.cpp
@@ -201,7 +201,7 @@ StatusWith<std::unique_ptr<CollectionMetadata>> CollectionMetadata::cloneSplit(
         // Check that the split key is valid
         if (!rangeContains(minKey, maxKey, split)) {
             return {ErrorCodes::IllegalOperation,
-                    stream() << "cannot split chunk " << rangeToString(minKey, maxKey) << " at key "
+                    stream() << "Cannot split chunk " << rangeToString(minKey, maxKey) << " at key "
                              << split};
         }
 
diff --git a/src/mongo/db/s/merge_chunks_command.cpp b/src/mongo/db/s/merge_chunks_command.cpp
index a309f77cb47..6081f342926 100644
--- a/src/mongo/db/s/merge_chunks_command.cpp
+++ b/src/mongo/db/s/merge_chunks_command.cpp
@@ -324,6 +324,9 @@ bool mergeChunks(OperationContext* txn,
     ChunkVersion mergeVersion = metadata->getCollVersion();
     mergeVersion.incMinor();
 
+    // Ensure that the newly applied chunks would result in a correct metadata state
+    auto metadataAfterMerge = uassertStatusOK(metadata->cloneMerge(minKey, maxKey, mergeVersion));
+
     Status applyOpsStatus = runApplyOpsCmd(txn, chunksToMerge, shardVersion, mergeVersion);
     if (!applyOpsStatus.isOK()) {
         warning() << applyOpsStatus;
@@ -339,10 +342,7 @@ bool mergeChunks(OperationContext* txn,
         AutoGetCollection autoColl(txn, nss, MODE_IX, MODE_X);
 
         auto css = CollectionShardingState::get(txn, nss);
-
-        std::unique_ptr<CollectionMetadata> cloned(
-            fassertStatusOK(40222, css->getMetadata()->cloneMerge(minKey, maxKey, mergeVersion)));
-        css->refreshMetadata(txn, std::move(cloned));
+        css->refreshMetadata(txn, std::move(metadataAfterMerge));
     }
 
     //
diff --git a/src/mongo/db/s/operation_sharding_state.h b/src/mongo/db/s/operation_sharding_state.h
index 0ef397b7e92..d83822d5696 100644
--- a/src/mongo/db/s/operation_sharding_state.h
+++ b/src/mongo/db/s/operation_sharding_state.h
@@ -116,7 +116,7 @@ private:
     void _clear();
 
     bool _hasVersion = false;
-    ChunkVersion _shardVersion;
+    ChunkVersion _shardVersion{ChunkVersion::UNSHARDED()};
     NamespaceString _ns;
 
     // This value will only be non-null if version check during the operation execution failed due
diff --git a/src/mongo/db/s/split_chunk_command.cpp b/src/mongo/db/s/split_chunk_command.cpp
index 059f1634168..b57170b5464 100644
--- a/src/mongo/db/s/split_chunk_command.cpp
+++ b/src/mongo/db/s/split_chunk_command.cpp
@@ -255,31 +255,22 @@ public:
             return false;
         }
 
-        ChunkVersion cmdVersion;
-        {
-            // Mongos >= v3.2 sends the full version, v3.0 only sends the epoch.
-            // TODO(SERVER-20742): Stop parsing epoch separately after 3.2.
-            OID cmdEpoch;
-            auto& oss = OperationShardingState::get(txn);
-            if (oss.hasShardVersion()) {
-                cmdVersion = oss.getShardVersion(nss);
-                cmdEpoch = cmdVersion.epoch();
-            } else {
-                BSONElement epochElem(cmdObj["epoch"]);
-                if (epochElem.type() == jstOID) {
-                    cmdEpoch = epochElem.OID();
-                }
-            }
-
-            if (cmdEpoch != shardVersion.epoch()) {
-                std::string msg = str::stream() << "splitChunk cannot split chunk "
-                                                << "[" << min << "," << max << "), "
-                                                << "collection may have been dropped. "
-                                                << "current epoch: " << shardVersion.epoch()
-                                                << ", cmd epoch: " << cmdEpoch;
-                warning() << msg;
-                throw SendStaleConfigException(nss.toString(), msg, cmdVersion, shardVersion);
-            }
+        const auto& oss = OperationShardingState::get(txn);
+        uassert(ErrorCodes::InvalidOptions, "collection version is missing", oss.hasShardVersion());
+
+        // Even though the splitChunk command transmits a value in the operation's shardVersion
+        // field, this value does not actually contain the shard version, but the global collection
+        // version.
+        ChunkVersion expectedCollectionVersion = oss.getShardVersion(nss);
+        if (expectedCollectionVersion.epoch() != shardVersion.epoch()) {
+            std::string msg = str::stream() << "splitChunk cannot split chunk "
+                                            << "[" << min << "," << max << "), "
+                                            << "collection may have been dropped. "
+                                            << "current epoch: " << shardVersion.epoch()
+                                            << ", cmd epoch: " << expectedCollectionVersion.epoch();
+            warning() << msg;
+            throw SendStaleConfigException(
+                nss.toString(), msg, expectedCollectionVersion, shardVersion);
         }
 
         ScopedCollectionMetadata collMetadata;
@@ -305,7 +296,8 @@ public:
                                             << "[" << min << "," << max << ")"
                                             << " to split, the chunk boundaries may be stale";
             warning() << msg;
-            throw SendStaleConfigException(nss.toString(), msg, cmdVersion, shardVersion);
+            throw SendStaleConfigException(
+                nss.toString(), msg, expectedCollectionVersion, shardVersion);
         }
 
         log() << "splitChunk accepted at version " << shardVersion;
@@ -402,6 +394,20 @@ public:
             preCond.append(b.obj());
         }
 
+        // NOTE: The newShardVersion resulting from this split is higher than any other chunk
+        // version, so it's also implicitly the newCollVersion
+        ChunkVersion newShardVersion = collVersion;
+
+        // Increment the minor version once, splitChunk increments once per split point
+        // (resulting in the correct final shard/collection version)
+        //
+        // TODO: Revisit this interface, it's a bit clunky
+        newShardVersion.incMinor();
+
+        // Ensure that the newly applied chunks would result in a correct metadata state
+        auto metadataAfterSplit =
+            uassertStatusOK(collMetadata->cloneSplit(min, max, splitKeys, newShardVersion));
+
         //
         // 4. apply the batch of updates to remote and local metadata
         //
@@ -421,20 +427,7 @@ public:
             AutoGetCollection autoColl(txn, nss, MODE_IX, MODE_X);
 
             auto css = CollectionShardingState::get(txn, nss);
-
-            // NOTE: The newShardVersion resulting from this split is higher than any other chunk
-            // version, so it's also implicitly the newCollVersion
-            ChunkVersion newShardVersion = collVersion;
-
-            // Increment the minor version once, splitChunk increments once per split point
-            // (resulting in the correct final shard/collection version)
-            //
-            // TODO: Revisit this interface, it's a bit clunky
-            newShardVersion.incMinor();
-
-            std::unique_ptr<CollectionMetadata> cloned(fassertStatusOK(
-                40221, css->getMetadata()->cloneSplit(min, max, splitKeys, newShardVersion)));
-            css->refreshMetadata(txn, std::move(cloned));
+            css->refreshMetadata(txn, std::move(metadataAfterSplit));
         }
 
         //