SERVER-68852 Investigate handling of incorrect values for balancer settings

11 files changed, 223 insertions, 4 deletions

diff --git a/jstests/sharding/config_settings_schema.js b/jstests/sharding/config_settings_schema.js
new file mode 100644
index 00000000000..8039bdd2206
--- /dev/null
+++ b/jstests/sharding/config_settings_schema.js
@@ -0,0 +1,52 @@
+/**
+ * Tests that the schema on config.settings works as intended.
+ *
+ * @tags: [featureFlagConfigSettingsSchema, requires_fcv_62, does_not_support_stepdowns]
+ */
+(function() {
+'use strict';
+
+load("jstests/libs/feature_flag_util.js");
+
+var st = new ShardingTest({shards: 1, config: 2});
+
+let coll = st.config.settings;
+
+// Updates that violate schema are rejected
+// Chunk size too small
+assert.commandFailed(coll.update({_id: "chunksize"}, {$set: {value: -1}}, {upsert: true}));
+// Chunk size must be a number
+assert.commandFailed(coll.update({_id: "chunksize"}, {$set: {value: "string"}}, {upsert: true}));
+// Chunk size too big
+assert.commandFailed(coll.update({_id: "chunksize"}, {$set: {value: 5000}}, {upsert: true}));
+// Extra field in chunk size doc
+assert.commandFailed(
+    coll.update({_id: "chunksize"}, {$set: {value: 100, extraField: 10}}, {upsert: true}));
+// Not a valid setting _id
+assert.commandFailed(coll.update({_id: "notARealSetting"}, {$set: {value: 10}}, {upsert: true}));
+
+// Updates that match the schema are accepted
+// No schema is enforced for balancer, autosplit, and ReadWriteConcernDefaults
+assert.commandWorked(coll.update({_id: "balancer"}, {$set: {anything: true}}, {upsert: true}));
+assert.commandWorked(coll.update({_id: "autosplit"}, {$set: {anything: true}}, {upsert: true}));
+assert.commandWorked(
+    coll.update({_id: "ReadWriteConcernDefaults"}, {$set: {anything: true}}, {upsert: true}));
+// Schema enforces chunksize to be a number (not an int), so doubles will be accepted and the
+// balancer will fail until a correct value is set
+assert.commandWorked(coll.update({_id: "chunksize"}, {$set: {value: 3.5}}, {upsert: true}));
+// Valid integer value
+assert.commandWorked(coll.update({_id: "chunksize"}, {$set: {value: 5}}, {upsert: true}));
+
+// User cannot change schema on config.settings
+assert.commandFailedWithCode(
+    st.s.getDB("config").runCommand({"collMod": "settings", "validator": {}}),
+    ErrorCodes.InvalidOptions);
+assert.commandFailedWithCode(
+    st.s.getDB("config").runCommand({"collMod": "settings", "validationLevel": "off"}),
+    ErrorCodes.InvalidOptions);
+assert.commandFailedWithCode(
+    st.s.getDB("config").runCommand({"collMod": "settings", "validationAction": "warn"}),
+    ErrorCodes.InvalidOptions);
+
+st.stop();
+})();
diff --git a/jstests/sharding/config_settings_schema_upgrade_downgrade.js b/jstests/sharding/config_settings_schema_upgrade_downgrade.js
new file mode 100644
index 00000000000..6562cc9b94b
--- /dev/null
+++ b/jstests/sharding/config_settings_schema_upgrade_downgrade.js
@@ -0,0 +1,31 @@
+/**
+ * TODO (SERVER-70763) remove this test after 7.0 becomes lastLTS
+ *
+ * Tests that a schema is added to the config.settings collection on upgrade and removed on
+ * downgrade.
+ *
+ * @tags: [multiversion_incompatible, featureFlagConfigSettingsSchema, does_not_support_stepdowns]
+ */
+(function() {
+'use strict';
+
+load("jstests/libs/feature_flag_util.js");
+
+var st = new ShardingTest({shards: 1, config: 2});
+
+// Validator should be created for new clusters in 6.2
+let validatorDoc = st.config.getCollectionInfos({name: "settings"})[0].options.validator;
+assert(validatorDoc);
+
+// Validator should be removed on downgrade
+st.s.adminCommand({setFeatureCompatibilityVersion: lastLTSFCV});
+validatorDoc = st.config.getCollectionInfos({name: "settings"})[0].options.validator;
+assert(!validatorDoc);
+
+// Validator should be added in on upgrade
+st.s.adminCommand({setFeatureCompatibilityVersion: latestFCV});
+validatorDoc = st.config.getCollectionInfos({name: "settings"})[0].options.validator;
+assert(validatorDoc);
+
+st.stop();
+})();
diff --git a/src/mongo/db/catalog/collection_impl.cpp b/src/mongo/db/catalog/collection_impl.cpp
index 36d3ea653fa..f93ae664f29 100644
--- a/src/mongo/db/catalog/collection_impl.cpp
+++ b/src/mongo/db/catalog/collection_impl.cpp
@@ -100,7 +100,9 @@ Status checkValidatorCanBeUsedOnNs(const BSONObj& validator,
                               << " with UUID " << uuid};
     }
 
-    if (nss.isOnInternalDb()) {
+    // Allow schema on config.settings. This is created internally, and user changes to this
+    // validator are disallowed in the createCollection and collMod commands.
+    if (nss.isOnInternalDb() && nss != NamespaceString::kConfigSettingsNamespace) {
         return {ErrorCodes::InvalidOptions,
                 str::stream() << "Document validators are not allowed on collection " << nss.ns()
                               << " with UUID " << uuid << " in the " << nss.db()
diff --git a/src/mongo/db/commands/create_command.cpp b/src/mongo/db/commands/create_command.cpp
index bcefed128b2..e3a0e5cbc92 100644
--- a/src/mongo/db/commands/create_command.cpp
+++ b/src/mongo/db/commands/create_command.cpp
@@ -323,6 +323,15 @@ public:
                 cmd.setIdIndex(idIndexSpec);
             }
 
+            if (cmd.getValidator() || cmd.getValidationLevel() || cmd.getValidationAction()) {
+                // Check for config.settings in the user command since a validator is allowed
+                // internally on this collection but the user may not modify the validator.
+                uassert(ErrorCodes::InvalidOptions,
+                        str::stream()
+                            << "Document validators not allowed on system collection " << ns(),
+                        ns() != NamespaceString::kConfigSettingsNamespace);
+            }
+
             OperationShardingState::ScopedAllowImplicitCollectionCreate_UNSAFE
                 unsafeCreateCollection(opCtx);
             uassertStatusOK(createCollection(opCtx, cmd));
diff --git a/src/mongo/db/commands/dbcommands.cpp b/src/mongo/db/commands/dbcommands.cpp
index 0b6f90d0203..b02534cf98c 100644
--- a/src/mongo/db/commands/dbcommands.cpp
+++ b/src/mongo/db/commands/dbcommands.cpp
@@ -569,6 +569,15 @@ public:
             }
         }
 
+        if (cmd->getValidator() || cmd->getValidationLevel() || cmd->getValidationAction()) {
+            // Check for config.settings in the user command since a validator is allowed
+            // internally on this collection but the user may not modify the validator.
+            uassert(ErrorCodes::InvalidOptions,
+                    str::stream() << "Document validators not allowed on system collection "
+                                  << cmd->getNamespace(),
+                    cmd->getNamespace() != NamespaceString::kConfigSettingsNamespace);
+        }
+
         uassertStatusOK(timeseries::processCollModCommandWithTimeSeriesTranslation(
             opCtx, cmd->getNamespace(), *cmd, true, &result));
         return true;
diff --git a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
index a0672dd0d84..b7908899c83 100644
--- a/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
+++ b/src/mongo/db/commands/set_feature_compatibility_version_command.cpp
@@ -495,6 +495,7 @@ private:
         if (serverGlobalParams.clusterRole == ClusterRole::ConfigServer) {
             _createGlobalIndexesIndexes(opCtx, requestedVersion);
             _cleanupConfigVersionOnUpgrade(opCtx, requestedVersion);
+            _createSchemaOnConfigSettings(opCtx, requestedVersion);
         } else if (serverGlobalParams.clusterRole == ClusterRole::ShardServer) {
             _createGlobalIndexesIndexes(opCtx, requestedVersion);
         } else {
@@ -580,6 +581,15 @@ private:
         }
     }
 
+    // TODO (SERVER-70763): Remove once FCV 7.0 becomes last-lts.
+    void _createSchemaOnConfigSettings(
+        OperationContext* opCtx, const multiversion::FeatureCompatibilityVersion requestedVersion) {
+        if (feature_flags::gConfigSettingsSchema.isEnabledOnVersion(requestedVersion)) {
+            LOGV2(6885200, "Creating schema on config.settings");
+            uassertStatusOK(ShardingCatalogManager::get(opCtx)->upgradeConfigSettings(opCtx));
+        }
+    }
+
     // _runUpgrade performs all the upgrade specific code for setFCV. Any new feature specific
     // upgrade code should be placed in the _runUpgrade helper functions:
     //  * _prepareForUpgrade: for any upgrade actions that should be done before taking the FCV full
@@ -587,8 +597,7 @@ private:
     //  * _userCollectionsWorkForUpgrade: for any user collections uasserts, creations, or deletions
     //    that need to happen during the upgrade. This happens after the FCV full transition lock.
     //  * _completeUpgrade: for updating metadata to make sure the new features in the upgraded
-    //  version
-    //    work for sharded and non-sharded clusters
+    //    version work for sharded and non-sharded clusters
     // Please read the comments on those helper functions for more details on what should be placed
     // in each function.
     void _runUpgrade(OperationContext* opCtx,
@@ -822,7 +831,7 @@ private:
         OperationContext* opCtx, const multiversion::FeatureCompatibilityVersion requestedVersion) {
         if (serverGlobalParams.clusterRole == ClusterRole::ConfigServer) {
             _dropInternalGlobalIndexesCollection(opCtx, requestedVersion);
-
+            _removeSchemaOnConfigSettings(opCtx, requestedVersion);
             // Always abort the reshardCollection regardless of version to ensure that it will
             // run on a consistent version from start to finish. This will ensure that it will
             // be able to apply the oplog entries correctly.
@@ -902,6 +911,20 @@ private:
         }
     }
 
+    // TODO (SERVER-70763): Remove once FCV 7.0 becomes last-lts.
+    void _removeSchemaOnConfigSettings(
+        OperationContext* opCtx, const multiversion::FeatureCompatibilityVersion requestedVersion) {
+        if (!feature_flags::gGlobalIndexesShardingCatalog.isEnabledOnVersion(requestedVersion)) {
+            LOGV2(6885201, "Removing schema on config.settings");
+            CollMod collModCmd{NamespaceString::kConfigSettingsNamespace};
+            collModCmd.getCollModRequest().setValidator(BSONObj());
+            collModCmd.getCollModRequest().setValidationLevel(ValidationLevelEnum::off);
+            BSONObjBuilder builder;
+            uassertStatusOKIgnoreNSNotFound(processCollModCommand(
+                opCtx, {NamespaceString::kConfigSettingsNamespace}, collModCmd, &builder));
+        }
+    }
+
     // _runDowngrade performs all the downgrade specific code for setFCV. Any new feature specific
     // downgrade code should be placed in the _runDowngrade helper functions:
     // * _prepareForDowngrade: Any downgrade actions that should be done before taking the FCV full
diff --git a/src/mongo/db/s/SConscript b/src/mongo/db/s/SConscript
index fa95c943bcd..3fee5b331b0 100644
--- a/src/mongo/db/s/SConscript
+++ b/src/mongo/db/s/SConscript
@@ -362,6 +362,7 @@ env.Library(
         '$BUILD_DIR/mongo/bson/util/bson_extract',
         '$BUILD_DIR/mongo/client/fetcher',
         '$BUILD_DIR/mongo/db/audit',
+        '$BUILD_DIR/mongo/db/catalog/catalog_helpers',
         '$BUILD_DIR/mongo/db/catalog/collection_options',
         '$BUILD_DIR/mongo/db/commands/cluster_server_parameter_commands_invocation',
         '$BUILD_DIR/mongo/db/commands/mongod_fcv',
diff --git a/src/mongo/db/s/config/sharding_catalog_manager.cpp b/src/mongo/db/s/config/sharding_catalog_manager.cpp
index 77e387a3081..a4de1d835a5 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager.cpp
+++ b/src/mongo/db/s/config/sharding_catalog_manager.cpp
@@ -34,18 +34,22 @@
 #include <vector>
 
 #include "mongo/db/auth/authorization_session_impl.h"
+#include "mongo/db/catalog/coll_mod.h"
+#include "mongo/db/coll_mod_gen.h"
 #include "mongo/db/dbdirectclient.h"
 #include "mongo/db/error_labels.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/ops/write_ops.h"
 #include "mongo/db/ops/write_ops_parsers.h"
 #include "mongo/db/query/query_request_helper.h"
+#include "mongo/db/read_write_concern_defaults.h"
 #include "mongo/db/repl/repl_client_info.h"
 #include "mongo/db/s/balancer/type_migration.h"
 #include "mongo/db/s/config/index_on_config.h"
 #include "mongo/db/s/sharding_util.h"
 #include "mongo/db/vector_clock.h"
 #include "mongo/logv2/log.h"
+#include "mongo/s/balancer_configuration.h"
 #include "mongo/s/catalog/sharding_catalog_client.h"
 #include "mongo/s/catalog/type_chunk.h"
 #include "mongo/s/catalog/type_collection.h"
@@ -345,6 +349,13 @@ Status ShardingCatalogManager::initializeConfigDatabaseIfNeeded(OperationContext
         return status;
     }
 
+    if (feature_flags::gConfigSettingsSchema.isEnabled(serverGlobalParams.featureCompatibility)) {
+        status = _initConfigSettings(opCtx);
+        if (!status.isOK()) {
+            return status;
+        }
+    }
+
     // Make sure to write config.version last since we detect rollbacks of config.version and
     // will re-run initializeConfigDatabaseIfNeeded if that happens, but we don't detect rollback
     // of the index builds.
@@ -359,6 +370,10 @@ Status ShardingCatalogManager::initializeConfigDatabaseIfNeeded(OperationContext
     return Status::OK();
 }
 
+Status ShardingCatalogManager::upgradeConfigSettings(OperationContext* opCtx) {
+    return _initConfigSettings(opCtx);
+}
+
 void ShardingCatalogManager::discardCachedConfigDatabaseInitializationState() {
     stdx::lock_guard<Latch> lk(_mutex);
     _configInitialized = false;
@@ -466,6 +481,59 @@ Status ShardingCatalogManager::_initConfigCollections(OperationContext* opCtx) {
     return Status::OK();
 }
 
+Status ShardingCatalogManager::_initConfigSettings(OperationContext* opCtx) {
+    DBDirectClient client(opCtx);
+
+    /**
+     * $jsonSchema: {
+     *   oneOf: [
+     *       {"properties": {_id: {enum: ["chunksize"]}},
+     *                      {value: {bsonType: "number", minimum: 1, maximum: 1024}}},
+     *       {"properties": {_id: {enum: ["balancer", "autosplit", "ReadWriteConcernDefaults",
+     *                                    "audit"]}}}
+     *   ]
+     * }
+     *
+     * Note: the schema uses "number" for the chunksize instead of "int" because "int" requires the
+     * user to pass NumberInt(x) as the value rather than x (as all of our docs recommend). Non-
+     * integer values will be handled as they were before the schema, by the balancer failing until
+     * a new value is set.
+     */
+    const auto chunkSizeValidator =
+        BSON("properties" << BSON("_id" << BSON("enum" << BSON_ARRAY(ChunkSizeSettingsType::kKey))
+                                        << "value"
+                                        << BSON("bsonType"
+                                                << "number"
+                                                << "minimum" << 1 << "maximum" << 1024))
+                          << "additionalProperties" << false);
+    const auto noopValidator =
+        BSON("properties" << BSON(
+                 "_id" << BSON("enum" << BSON_ARRAY(
+                                   BalancerSettingsType::kKey
+                                   << AutoSplitSettingsType::kKey
+                                   << ReadWriteConcernDefaults::kPersistedDocumentId << "audit"))));
+    const auto fullValidator =
+        BSON("$jsonSchema" << BSON("oneOf" << BSON_ARRAY(chunkSizeValidator << noopValidator)));
+
+    BSONObj cmd = BSON("create" << NamespaceString::kConfigSettingsNamespace.coll());
+    BSONObj result;
+    const bool ok =
+        client.runCommand(NamespaceString::kConfigSettingsNamespace.db().toString(), cmd, result);
+    if (!ok) {  // create returns error NamespaceExists if collection already exists
+        Status status = getStatusFromCommandResult(result);
+        if (status != ErrorCodes::NamespaceExists) {
+            return status.withContext("Could not create config.settings");
+        }
+    }
+
+    // Collection already exists, create validator on that collection
+    CollMod collModCmd{NamespaceString::kConfigSettingsNamespace};
+    collModCmd.getCollModRequest().setValidator(fullValidator);
+    BSONObjBuilder builder;
+    return processCollModCommand(
+        opCtx, {NamespaceString::kConfigSettingsNamespace}, collModCmd, &builder);
+}
+
 Status ShardingCatalogManager::setFeatureCompatibilityVersionOnShards(OperationContext* opCtx,
                                                                       const BSONObj& cmdObj) {
 
diff --git a/src/mongo/db/s/config/sharding_catalog_manager.h b/src/mongo/db/s/config/sharding_catalog_manager.h
index 77630acaab2..bb0bc6e6b40 100644
--- a/src/mongo/db/s/config/sharding_catalog_manager.h
+++ b/src/mongo/db/s/config/sharding_catalog_manager.h
@@ -533,6 +533,11 @@ public:
                               bool force,
                               const Timestamp& validAfter);
 
+    /**
+     * Creates config.settings (if needed) and adds a schema to the collection.
+     */
+    Status upgradeConfigSettings(OperationContext* opCtx);
+
 private:
     /**
      * Performs the necessary checks for version compatibility and creates a new config.version
@@ -546,6 +551,11 @@ private:
     Status _initConfigIndexes(OperationContext* opCtx);
 
     /**
+     * Creates config.settings (if needed) and adds a schema to the collection.
+     */
+    Status _initConfigSettings(OperationContext* opCtx);
+
+    /**
      * Ensure that config.collections exists upon configsvr startup
      */
     Status _initConfigCollections(OperationContext* opCtx);
diff --git a/src/mongo/s/commands/cluster_collection_mod_cmd.cpp b/src/mongo/s/commands/cluster_collection_mod_cmd.cpp
index 777f30bf5f9..03afdbe6feb 100644
--- a/src/mongo/s/commands/cluster_collection_mod_cmd.cpp
+++ b/src/mongo/s/commands/cluster_collection_mod_cmd.cpp
@@ -109,6 +109,14 @@ public:
         }
         const auto dbInfo = uassertStatusOK(swDbInfo);
 
+        if (cmd.getValidator() || cmd.getValidationLevel() || cmd.getValidationAction()) {
+            // Check for config.settings in the user command since a validator is allowed
+            // internally on this collection but the user may not modify the validator.
+            uassert(ErrorCodes::InvalidOptions,
+                    str::stream() << "Document validators not allowed on system collection " << nss,
+                    nss != NamespaceString::kConfigSettingsNamespace);
+        }
+
         ShardsvrCollMod collModCommand(nss);
         collModCommand.setCollModRequest(cmd.getCollModRequest());
         // TODO SERVER-67411 change executeCommandAgainstDatabasePrimary to take in DatabaseName
diff --git a/src/mongo/s/sharding_feature_flags.idl b/src/mongo/s/sharding_feature_flags.idl
index cbd558771a7..9d2c5392b51 100644
--- a/src/mongo/s/sharding_feature_flags.idl
+++ b/src/mongo/s/sharding_feature_flags.idl
@@ -98,3 +98,9 @@ feature_flags:
                   the tolerance in case of a failure on donor and recipient nodes"
     cpp_varname: feature_flags::gResilientMovePrimary
     default: false
+  featureFlagConfigSettingsSchema:
+    description: "Feature flag for adding schema to config.settings collection"
+    cpp_varname: feature_flags::gConfigSettingsSchema
+    default: true
+    version: 6.2
+    
\ No newline at end of file