From 1854c66a7dc76d891912356cf1646ccd4b146e8c Mon Sep 17 00:00:00 2001 From: Allison Easton Date: Mon, 14 Nov 2022 11:14:54 +0000 Subject: SERVER-68852 Investigate handling of incorrect values for balancer settings --- jstests/sharding/config_settings_schema.js | 52 +++++++++++++++++ .../config_settings_schema_upgrade_downgrade.js | 31 ++++++++++ src/mongo/db/catalog/collection_impl.cpp | 4 +- src/mongo/db/commands/create_command.cpp | 9 +++ src/mongo/db/commands/dbcommands.cpp | 9 +++ .../set_feature_compatibility_version_command.cpp | 29 ++++++++- src/mongo/db/s/SConscript | 1 + src/mongo/db/s/config/sharding_catalog_manager.cpp | 68 ++++++++++++++++++++++ src/mongo/db/s/config/sharding_catalog_manager.h | 10 ++++ .../s/commands/cluster_collection_mod_cmd.cpp | 8 +++ src/mongo/s/sharding_feature_flags.idl | 6 ++ 11 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 jstests/sharding/config_settings_schema.js create mode 100644 jstests/sharding/config_settings_schema_upgrade_downgrade.js 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 #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 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 @@ -545,6 +550,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 */ 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 -- cgit v1.2.1