SERVER-42114 Allow single RS transactions to run on the config database in sharded clusters

BlakeIsBlake · BlakeIsBlake · commit 927883be3ec0 · 2019-08-01T16:26:43.000-04:00
diff --git a/buildscripts/resmokeconfig/suites/sharded_causally_consistent_jscore_txns_passthrough.yml b/buildscripts/resmokeconfig/suites/sharded_causally_consistent_jscore_txns_passthrough.yml
@@ -28,10 +28,6 @@ selector:
   - jstests/core/txns/abort_transaction_thread_does_not_block_on_locks.js
   - jstests/core/txns/kill_op_on_txn_expiry.js
 
-  # Writes to the local database are not allowed through mongos.
-  # TODO SERVER-28756: Mongos CSRS write retry logic drops txnNumbers.
-  - jstests/core/txns/banned_txn_dbs.js
-
   # Uses hangAfterCollectionInserts failpoint not available on mongos.
   - jstests/core/txns/speculative_snapshot_includes_all_writes.js
 
@@ -40,6 +36,7 @@ selector:
   - jstests/core/txns/non_transactional_operations_on_session_with_transaction.js
 
   exclude_with_any_tags:
+  - assumes_against_mongod_not_mongos
   - does_not_support_causal_consistency
   # Transactions are not allowed to operate on capped collections.
   - requires_capped
diff --git a/buildscripts/resmokeconfig/suites/sharded_collections_causally_consistent_jscore_txns_passthrough.yml b/buildscripts/resmokeconfig/suites/sharded_collections_causally_consistent_jscore_txns_passthrough.yml
@@ -24,10 +24,6 @@ selector:
   - jstests/core/txns/abort_transaction_thread_does_not_block_on_locks.js
   - jstests/core/txns/kill_op_on_txn_expiry.js
 
-  # Writes to the local database are not allowed through mongos.
-  # TODO SERVER-28756: Mongos CSRS write retry logic drops txnNumbers.
-  - jstests/core/txns/banned_txn_dbs.js
-
   # Uses hangAfterCollectionInserts failpoint not available on mongos.
   - jstests/core/txns/speculative_snapshot_includes_all_writes.js
 
diff --git a/buildscripts/resmokeconfig/suites/sharded_jscore_txns.yml b/buildscripts/resmokeconfig/suites/sharded_jscore_txns.yml
@@ -28,14 +28,11 @@ selector:
   - jstests/core/txns/abort_transaction_thread_does_not_block_on_locks.js
   - jstests/core/txns/kill_op_on_txn_expiry.js
 
-  # Writes to the local database are not allowed through mongos.
-  # TODO SERVER-28756: Mongos CSRS write retry logic drops txnNumbers.
-  - jstests/core/txns/banned_txn_dbs.js
-
   # Uses hangAfterCollectionInserts failpoint not available on mongos.
   - jstests/core/txns/speculative_snapshot_includes_all_writes.js
 
   exclude_with_any_tags:
+  - assumes_against_mongod_not_mongos
   # Transactions are not allowed to operate on capped collections.
   - requires_capped
   # Prepare is not a command on mongos.
diff --git a/buildscripts/resmokeconfig/suites/sharded_jscore_txns_sharded_collections.yml b/buildscripts/resmokeconfig/suites/sharded_jscore_txns_sharded_collections.yml
@@ -24,10 +24,6 @@ selector:
   - jstests/core/txns/abort_transaction_thread_does_not_block_on_locks.js
   - jstests/core/txns/kill_op_on_txn_expiry.js
 
-  # Writes to the local database are not allowed through mongos.
-  # TODO SERVER-28756: Mongos CSRS write retry logic drops txnNumbers.
-  - jstests/core/txns/banned_txn_dbs.js
-
   # Uses hangAfterCollectionInserts failpoint not available on mongos.
   - jstests/core/txns/speculative_snapshot_includes_all_writes.js
 
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
@@ -149,6 +149,7 @@ selector:
   - jstests/sharding/database_and_shard_versioning_all_commands.js
   - jstests/sharding/shard1.js
   - jstests/sharding/track_unsharded_collections_create_collection.js
+  - jstests/sharding/banned_txn_databases_sharded.js
   # Enable if SERVER-41813 is backported or 4.4 becomes last-stable
   - jstests/sharding/invalid_system_views_sharded_collection.js
 
diff --git a/jstests/core/txns/banned_txn_dbs_unsharded.js b/jstests/core/txns/banned_txn_dbs_unsharded.js
@@ -1,6 +1,6 @@
 // Tests that reads and writes to the config, admin, and local databases are forbidden within
-// transactions.
-// @tags: [uses_transactions]
+// transactions on non-sharded clusters. Behavior on sharded clusters is tested separately.
+// @tags: [assumes_against_mongod_not_mongos, assumes_unsharded_collection, uses_transactions]
 (function() {
 "use strict";
 
diff --git a/jstests/sharding/banned_txn_databases_sharded.js b/jstests/sharding/banned_txn_databases_sharded.js
@@ -0,0 +1,78 @@
+/**
+ * Tests that:
+ * 1. Reads and writes to the admin database are forbidden within single replica set transactions
+ *    on sharded clusters.
+ * 2. Read and writes to the config database are forbidden from mongos within single replica set
+ *    transactions on sharded clusters.
+ * 3. Reads and writes to the config.transactions namespace are forbidden within single replica set
+ *    transactions on sharded clusters, BUT read and writes to other namespaces in the config
+ *    database are allowed.
+ *
+ * @tags: [requires_find_command]
+ */
+
+(function() {
+"use strict";
+
+load("jstests/sharding/libs/sharded_transactions_helpers.js");
+
+const st = new ShardingTest({shards: 1});
+const mongosSession = st.s.startSession();
+const shardSession = st.shard0.getDB("test").getMongo().startSession();
+const collName = "banned_txn_dbs";
+
+jsTest.log("Verify that read and write operations within transactions are forbidden for the " +
+           "admin database within a transaction.");
+
+const adminColl = shardSession.getDatabase("admin")[collName];
+
+shardSession.startTransaction();
+let error = assert.throws(() => adminColl.find().itcount());
+assert.commandFailedWithCode(error, ErrorCodes.OperationNotSupportedInTransaction);
+assert.commandFailedWithCode(shardSession.abortTransaction_forTesting(),
+                             ErrorCodes.NoSuchTransaction);
+
+shardSession.startTransaction();
+assert.commandFailedWithCode(adminColl.insert({}), ErrorCodes.OperationNotSupportedInTransaction);
+assert.commandFailedWithCode(shardSession.abortTransaction_forTesting(),
+                             ErrorCodes.NoSuchTransaction);
+
+jsTestLog("Verify that read and write operations within transactions are forbidden for the " +
+          "config database when accessed through mongos.");
+
+const mongosConfigDB = mongosSession.getDatabase("config");
+const clusterColls = [
+    mongosConfigDB["test"],
+    mongosConfigDB["actionlog"],
+    mongosConfigDB["transaction_coords"],
+    mongosConfigDB["transactions"]
+];
+
+mongosSession.startTransaction();
+clusterColls.forEach((coll) => {
+    error = assert.throws(() => coll.find().itcount());
+    assert.commandFailedWithCode(error, ErrorCodes.OperationNotSupportedInTransaction);
+});
+
+mongosSession.endSession();
+
+jsTestLog("Verify that read operations within transactions work fine for the config database " +
+          "when not config.transactions (and directly accessed through the shard).");
+
+const configDB = shardSession.getDatabase("config");
+const shardColls = [configDB["test"], configDB["actionlog"], configDB["transaction_coords"]];
+
+shardSession.startTransaction();
+shardColls.forEach((coll) => {
+    coll.find().itcount();
+});
+
+jsTestLog("Verify that read operations will not work for the config.transactions namespace.");
+
+const shardCollTransactions = configDB["transactions"];
+error = assert.throws(() => shardCollTransactions.find().itcount());
+assert.commandFailedWithCode(error, ErrorCodes.OperationNotSupportedInTransaction);
+
+shardSession.endSession();
+st.stop();
+}());
diff --git a/src/mongo/db/commands.cpp b/src/mongo/db/commands.cpp
@@ -431,26 +431,38 @@ bool CommandHelpers::uassertShouldAttemptParse(OperationContext* opCtx,
 }
 
 
-Status CommandHelpers::canUseTransactions(StringData dbName, StringData cmdName) {
-    if (cmdName == "count"_sd) {
-        return {ErrorCodes::OperationNotSupportedInTransaction,
-                "Cannot run 'count' in a multi-document transaction. Please see "
-                "http://dochub.mongodb.org/core/transaction-count for a recommended alternative."};
-    }
-
-    if (txnCmdWhitelist.find(cmdName) == txnCmdWhitelist.cend()) {
-        return {ErrorCodes::OperationNotSupportedInTransaction,
-                str::stream() << "Cannot run '" << cmdName << "' in a multi-document transaction."};
-    }
-
-    if (dbName == "config"_sd || dbName == "local"_sd ||
-        (dbName == "admin"_sd && txnAdminCommands.find(cmdName) == txnAdminCommands.cend())) {
-        return {ErrorCodes::OperationNotSupportedInTransaction,
-                str::stream() << "Cannot run command against the '" << dbName
-                              << "' database in a transaction"};
+void CommandHelpers::canUseTransactions(const NamespaceString& nss,
+                                        StringData cmdName,
+                                        bool allowTransactionsOnConfigDatabase) {
+
+    uassert(ErrorCodes::OperationNotSupportedInTransaction,
+            "Cannot run 'count' in a multi-document transaction. Please see "
+            "http://dochub.mongodb.org/core/transaction-count for a recommended alternative.",
+            cmdName != "count"_sd);
+
+    uassert(ErrorCodes::OperationNotSupportedInTransaction,
+            str::stream() << "Cannot run '" << cmdName << "' in a multi-document transaction.",
+            txnCmdWhitelist.find(cmdName) != txnCmdWhitelist.cend());
+
+    const auto dbName = nss.db();
+
+    uassert(ErrorCodes::OperationNotSupportedInTransaction,
+            str::stream() << "Cannot run command against the '" << dbName
+                          << "' database in a transaction.",
+            dbName != NamespaceString::kLocalDb &&
+                (dbName != NamespaceString::kAdminDb ||
+                 txnAdminCommands.find(cmdName) != txnAdminCommands.cend()));
+
+    if (allowTransactionsOnConfigDatabase) {
+        uassert(ErrorCodes::OperationNotSupportedInTransaction,
+                "Cannot run command against the config.transactions namespace in a transaction"
+                "on a sharded cluster.",
+                nss != NamespaceString::kSessionTransactionsTableNamespace);
+    } else {
+        uassert(ErrorCodes::OperationNotSupportedInTransaction,
+                "Cannot run command against the config database in a transaction.",
+                dbName != "config"_sd);
     }
-
-    return Status::OK();
 }
 
 constexpr StringData CommandHelpers::kHelpFieldName;
diff --git a/src/mongo/db/commands.h b/src/mongo/db/commands.h
@@ -211,9 +211,12 @@ struct CommandHelpers {
                                           const OpMsgRequest& request);
 
     /**
-     * Returns OK if command is allowed to run under a transaction in the given database.
+     * Verifies that command is allowed to run under a transaction in the given database or
+     * namespace, and throws if that verification doesn't pass.
      */
-    static Status canUseTransactions(StringData dbName, StringData cmdName);
+    static void canUseTransactions(const NamespaceString& nss,
+                                   StringData cmdName,
+                                   bool allowTransactionsOnConfigDatabase);
 
     static constexpr StringData kHelpFieldName = "help"_sd;
 
diff --git a/src/mongo/db/service_entry_point_common.cpp b/src/mongo/db/service_entry_point_common.cpp
@@ -664,7 +664,15 @@ void execCommandDatabase(OperationContext* opCtx,
             str::stream() << "Invalid database name: '" << dbname << "'",
             NamespaceString::validDBName(dbname, NamespaceString::DollarInDbNameBehavior::Allow));
 
-        validateSessionOptions(sessionOptions, command->getName(), dbname);
+
+        const auto allowTransactionsOnConfigDatabase =
+            (serverGlobalParams.clusterRole == ClusterRole::ConfigServer ||
+             serverGlobalParams.clusterRole == ClusterRole::ShardServer);
+
+        validateSessionOptions(sessionOptions,
+                               command->getName(),
+                               invocation->ns(),
+                               allowTransactionsOnConfigDatabase);
 
         std::unique_ptr<MaintenanceModeSetter> mmSetter;
 
diff --git a/src/mongo/db/transaction_validation.cpp b/src/mongo/db/transaction_validation.cpp
@@ -70,9 +70,10 @@ bool shouldCommandSkipSessionCheckout(StringData cmdName) {
 
 void validateSessionOptions(const OperationSessionInfoFromClient& sessionOptions,
                             StringData cmdName,
-                            StringData dbname) {
+                            const NamespaceString& nss,
+                            bool allowTransactionsOnConfigDatabase) {
     if (sessionOptions.getAutocommit()) {
-        uassertStatusOK(CommandHelpers::canUseTransactions(dbname, cmdName));
+        CommandHelpers::canUseTransactions(nss, cmdName, allowTransactionsOnConfigDatabase);
     }
 
     if (!sessionOptions.getAutocommit() && sessionOptions.getTxnNumber()) {
diff --git a/src/mongo/db/transaction_validation.h b/src/mongo/db/transaction_validation.h
@@ -46,10 +46,11 @@ void validateWriteConcernForTransaction(const WriteConcernOptions& wcResult, Str
 bool shouldCommandSkipSessionCheckout(StringData cmdName);
 
 /**
- * Throws if the given session options are invalid for the given command and target database.
+ * Throws if the given session options are invalid for the given command and target namespace.
  */
 void validateSessionOptions(const OperationSessionInfoFromClient& sessionOptions,
                             StringData cmdName,
-                            StringData dbname);
+                            const NamespaceString& nss,
+                            bool allowTransactionsOnConfigDatabase);
 
 }  // namespace mongo
diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp
@@ -382,7 +382,11 @@ void runCommand(OperationContext* opCtx,
                                               command->attachLogicalSessionsToOpCtx(),
                                               true,
                                               true);
-    validateSessionOptions(osi, command->getName(), nss.db());
+
+    // TODO SERVER-28756: Change allowTransactionsOnConfigDatabase to true once we fix the bug
+    // where the mongos custom write path incorrectly drops the client's txnNumber.
+    auto allowTransactionsOnConfigDatabase = false;
+    validateSessionOptions(osi, command->getName(), nss, allowTransactionsOnConfigDatabase);
 
     auto& readConcernArgs = repl::ReadConcernArgs::get(opCtx);
     auto readConcernParseStatus = [&]() {
