From 841de0c1431530903cac29bab34bebfd65f6284d Mon Sep 17 00:00:00 2001 From: Gabriel Marks Date: Fri, 6 Jan 2023 04:08:54 +0000 Subject: SERVER-72220 Separate set from validate for cluster parameters --- jstests/libs/cluster_server_parameter_utils.js | 2 +- src/mongo/db/change_stream_options_manager.cpp | 5 ----- .../commands/set_cluster_parameter_invocation.cpp | 21 +++++++++++++-------- .../db/commands/set_cluster_parameter_invocation.h | 3 ++- .../configsvr_set_cluster_parameter_command.cpp | 3 ++- .../db/s/shardsvr_set_cluster_parameter_command.cpp | 3 ++- src/mongo/db/server_parameter_with_storage.h | 7 +++++-- .../idl/server_parameter_with_storage_test.cpp | 1 - 8 files changed, 25 insertions(+), 20 deletions(-) diff --git a/jstests/libs/cluster_server_parameter_utils.js b/jstests/libs/cluster_server_parameter_utils.js index 89e22e079eb..363b3005c21 100644 --- a/jstests/libs/cluster_server_parameter_utils.js +++ b/jstests/libs/cluster_server_parameter_utils.js @@ -395,7 +395,7 @@ function testDisabledClusterParameters(conn, tenantId) { assert.commandFailedWithCode( adminDB.runCommand(tenantCommand( {setClusterParameter: {testIntClusterParameter: {intData: 5}}}, tenantId)), - ErrorCodes.BadValue); + ErrorCodes.IllegalOperation); // Assert that explicitly getting a disabled cluster server parameter fails on the primary. testExplicitDisabledGetClusterParameter(conn.getPrimary(), tenantId); diff --git a/src/mongo/db/change_stream_options_manager.cpp b/src/mongo/db/change_stream_options_manager.cpp index 4832b1dc2e4..ab7d98c8506 100644 --- a/src/mongo/db/change_stream_options_manager.cpp +++ b/src/mongo/db/change_stream_options_manager.cpp @@ -82,11 +82,6 @@ void ChangeStreamOptionsParameter::append(OperationContext* opCtx, Status ChangeStreamOptionsParameter::set(const BSONElement& newValueElement, const boost::optional& tenantId) { try { - Status validateStatus = validate(newValueElement, tenantId); - if (!validateStatus.isOK()) { - return validateStatus; - } - ChangeStreamOptionsManager& changeStreamOptionsManager = ChangeStreamOptionsManager::get(getGlobalServiceContext()); ChangeStreamOptions newOptions = ChangeStreamOptions::parse( diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp index b8d5a4e424b..bcac612de1c 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp +++ b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp @@ -55,12 +55,14 @@ bool SetClusterParameterInvocation::invoke(OperationContext* opCtx, ServerParameter* serverParameter = _sps->get(parameterName); auto tenantId = cmd.getDbName().tenantId(); - uassert(ErrorCodes::BadValue, - str::stream() << "Server parameter: '" << serverParameter->name() << "' is disabled", - serverParameter->isEnabled()); - auto [query, update] = - normalizeParameter(opCtx, cmdParamObj, paramTime, serverParameter, parameterName, tenantId); + normalizeParameter(opCtx, + cmdParamObj, + paramTime, + serverParameter, + parameterName, + tenantId, + serverGlobalParams.clusterRole.isExclusivelyShardRole()); BSONObjBuilder oldValueBob; serverParameter->append(opCtx, &oldValueBob, parameterName.toString(), tenantId); @@ -79,7 +81,8 @@ std::pair SetClusterParameterInvocation::normalizeParameter( const boost::optional& paramTime, ServerParameter* sp, StringData parameterName, - const boost::optional& tenantId) { + const boost::optional& tenantId, + bool skipValidation) { BSONElement commandElement = cmdParamObj.firstElement(); uassert(ErrorCodes::IllegalOperation, "Cluster parameter value must be an object", @@ -87,7 +90,7 @@ std::pair SetClusterParameterInvocation::normalizeParameter( uassert(ErrorCodes::IllegalOperation, str::stream() << "Server parameter: '" << sp->name() << "' is disabled", - sp->isEnabled()); + skipValidation || sp->isEnabled()); Timestamp clusterTime = paramTime ? *paramTime : _dbService.getUpdateClusterTime(opCtx); @@ -98,7 +101,9 @@ std::pair SetClusterParameterInvocation::normalizeParameter( BSONObj query = BSON("_id" << parameterName); BSONObj update = updateBuilder.obj(); - uassertStatusOK(sp->validate(update, tenantId)); + if (!skipValidation) { + uassertStatusOK(sp->validate(update, tenantId)); + } return {query, update}; } diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation.h b/src/mongo/db/commands/set_cluster_parameter_invocation.h index c712669a272..d44de4274b2 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation.h +++ b/src/mongo/db/commands/set_cluster_parameter_invocation.h @@ -89,7 +89,8 @@ public: const boost::optional& paramTime, ServerParameter* sp, StringData parameterName, - const boost::optional& tenantId); + const boost::optional& tenantId, + bool skipValidation); private: std::unique_ptr _sps; diff --git a/src/mongo/db/s/config/configsvr_set_cluster_parameter_command.cpp b/src/mongo/db/s/config/configsvr_set_cluster_parameter_command.cpp index a4a451c1bb4..08834bf9f67 100644 --- a/src/mongo/db/s/config/configsvr_set_cluster_parameter_command.cpp +++ b/src/mongo/db/s/config/configsvr_set_cluster_parameter_command.cpp @@ -77,7 +77,8 @@ public: boost::none, serverParameter, parameterName, - request().getDbName().tenantId()); + request().getDbName().tenantId(), + false /* skipValidation */); auto tenantId = request().getDbName().tenantId(); diff --git a/src/mongo/db/s/shardsvr_set_cluster_parameter_command.cpp b/src/mongo/db/s/shardsvr_set_cluster_parameter_command.cpp index e8ed9e14277..204888c3834 100644 --- a/src/mongo/db/s/shardsvr_set_cluster_parameter_command.cpp +++ b/src/mongo/db/s/shardsvr_set_cluster_parameter_command.cpp @@ -79,7 +79,8 @@ public: SetClusterParameterInvocation invocation{std::move(parameterService), dbService}; // Use local write concern for setClusterParameter, the idea is that the command is // being called with majority write concern, so, we'll wait for majority after checking - // out the session. + // out the session. Note that we use the force option for invoke -- the config server + // should already have checked isEnabled + validate for us. bool writePerformed = invocation.invoke(opCtx, setClusterParameterRequest, request().getClusterParameterTime(), diff --git a/src/mongo/db/server_parameter_with_storage.h b/src/mongo/db/server_parameter_with_storage.h index 423509ae476..357e58e89a8 100644 --- a/src/mongo/db/server_parameter_with_storage.h +++ b/src/mongo/db/server_parameter_with_storage.h @@ -331,8 +331,11 @@ public: * Convenience wrapper for storing a value. */ Status setValue(const element_type& newValue, const boost::optional& tenantId) { - if (auto status = validateValue(newValue, tenantId); !status.isOK()) { - return status; + // For cluster parameters, validation must be separated from setting. + if constexpr (paramType != SPT::kClusterWide) { + if (auto status = validateValue(newValue, tenantId); !status.isOK()) { + return status; + } } _storage.store(newValue, tenantId); diff --git a/src/mongo/idl/server_parameter_with_storage_test.cpp b/src/mongo/idl/server_parameter_with_storage_test.cpp index 3944c9e0ce0..b95f8bb16f3 100644 --- a/src/mongo/idl/server_parameter_with_storage_test.cpp +++ b/src/mongo/idl/server_parameter_with_storage_test.cpp @@ -412,7 +412,6 @@ TEST(IDLServerParameterWithStorage, CSPStorageTest) { updatedParam.setTestStringField("newTestString"); updateTime = LogicalTime(Timestamp(Date_t::now())); ASSERT_NOT_OK(clusterParam->ServerParameter::validate(updatedParam.toBSON(), boost::none)); - ASSERT_NOT_OK(clusterParam->ServerParameter::set(updatedParam.toBSON(), boost::none)); retrievedParam = clusterParam->getValue(boost::none); ASSERT_EQ(retrievedParam.getPreAndPostImages().getExpireAfterSeconds(), 35); ASSERT_EQ(retrievedParam.getTestStringField(), "default"); -- cgit v1.2.1