SERVER-72220 Separate set from validate for cluster parameters

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>& 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<BSONObj, BSONObj> SetClusterParameterInvocation::normalizeParameter(
     const boost::optional<Timestamp>& paramTime,
     ServerParameter* sp,
     StringData parameterName,
-    const boost::optional<TenantId>& tenantId) {
+    const boost::optional<TenantId>& tenantId,
+    bool skipValidation) {
     BSONElement commandElement = cmdParamObj.firstElement();
     uassert(ErrorCodes::IllegalOperation,
             "Cluster parameter value must be an object",
@@ -87,7 +90,7 @@ std::pair<BSONObj, BSONObj> 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<BSONObj, BSONObj> 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<Timestamp>& paramTime,
                                                    ServerParameter* sp,
                                                    StringData parameterName,
-                                                   const boost::optional<TenantId>& tenantId);
+                                                   const boost::optional<TenantId>& tenantId,
+                                                   bool skipValidation);
 
 private:
     std::unique_ptr<ServerParameterService> _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>& 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");