From 2cfc78ff27a7ed95e7632f7749f8d09383be308b Mon Sep 17 00:00:00 2001 From: Sara Golemon Date: Fri, 9 Sep 2022 12:03:07 -0500 Subject: SERVER-69060 Guard server parameters on feature flags --- buildscripts/idl/idl/ast.py | 1 + buildscripts/idl/idl/binder.py | 12 +- buildscripts/idl/idl/generator.py | 9 + buildscripts/idl/idl/parser.py | 1 + buildscripts/idl/idl/syntax.py | 1 + jstests/libs/cluster_server_parameter_utils.js | 285 +++++++++------------ .../noPassthrough/server_parameter_feature_flag.js | 84 ++++++ src/mongo/db/SConscript | 2 + .../commands/get_cluster_parameter_invocation.cpp | 4 +- src/mongo/db/commands/parameters.cpp | 8 +- .../commands/set_cluster_parameter_invocation.cpp | 8 + src/mongo/db/feature_flag_test.idl.tpl | 11 + src/mongo/db/server_parameter.cpp | 15 ++ src/mongo/db/server_parameter.h | 10 +- src/mongo/idl/cluster_server_parameter.idl | 13 + 15 files changed, 297 insertions(+), 167 deletions(-) create mode 100644 jstests/noPassthrough/server_parameter_feature_flag.js diff --git a/buildscripts/idl/idl/ast.py b/buildscripts/idl/idl/ast.py index c49bfee0be5..da791000a07 100644 --- a/buildscripts/idl/idl/ast.py +++ b/buildscripts/idl/idl/ast.py @@ -367,6 +367,7 @@ class Condition(common.SourceLocation): self.expr = None # type: str self.constexpr = None # type: str self.preprocessor = None # type: str + self.feature_flag = None # type: str super(Condition, self).__init__(file_name, line, column) diff --git a/buildscripts/idl/idl/binder.py b/buildscripts/idl/idl/binder.py index 6ffaf2d5108..38b646e162f 100644 --- a/buildscripts/idl/idl/binder.py +++ b/buildscripts/idl/idl/binder.py @@ -942,8 +942,8 @@ def _bind_validator(ctxt, validator): return ast_validator -def _bind_condition(condition): - # type: (syntax.Condition) -> ast.Condition +def _bind_condition(condition, condition_for): + # type: (syntax.Condition, str) -> ast.Condition """Bind a condition from the idl.syntax tree.""" if not condition: @@ -954,6 +954,10 @@ def _bind_condition(condition): ast_condition.constexpr = condition.constexpr ast_condition.preprocessor = condition.preprocessor + if condition.feature_flag: + assert condition_for == 'server_parameter' + ast_condition.feature_flag = condition.feature_flag + return ast_condition @@ -1374,7 +1378,7 @@ def _bind_server_parameter(ctxt, param): ast_param = ast.ServerParameter(param.file_name, param.line, param.column) ast_param.name = param.name ast_param.description = param.description - ast_param.condition = _bind_condition(param.condition) + ast_param.condition = _bind_condition(param.condition, condition_for='server_parameter') ast_param.redact = param.redact ast_param.test_only = param.test_only ast_param.deprecated_name = param.deprecated_name @@ -1500,7 +1504,7 @@ def _bind_config_option(ctxt, globals_spec, option): node.arg_vartype = option.arg_vartype node.cpp_vartype = option.cpp_vartype node.cpp_varname = option.cpp_varname - node.condition = _bind_condition(option.condition) + node.condition = _bind_condition(option.condition, condition_for='config') node.requires = option.requires node.conflicts = option.conflicts diff --git a/buildscripts/idl/idl/generator.py b/buildscripts/idl/idl/generator.py index 3cb145c09eb..64c7f504146 100644 --- a/buildscripts/idl/idl/generator.py +++ b/buildscripts/idl/idl/generator.py @@ -2504,6 +2504,15 @@ class _CppSourceFileWriter(_CppFileWriterBase): if param.test_only: self._writer.write_line('scp_%d->setTestOnly();' % (param_no)) + if param.condition and param.condition.feature_flag: + ffs = [sp for sp in params if sp.name == param.condition.feature_flag] + if len(ffs) == 0: + raise ValueError("Unable to find feature flag named %s" % + (param.condition.feature_flag)) + assert len(ffs) == 1 + self._writer.write_line( + 'scp_%d->setFeatureFlag(&%s);' % (param_no, ffs[0].cpp_varname)) + self._gen_server_parameter_deprecated_aliases(param_no, param) self.write_empty_line() diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py index 5f4d0cbec19..b89c1724273 100644 --- a/buildscripts/idl/idl/parser.py +++ b/buildscripts/idl/idl/parser.py @@ -302,6 +302,7 @@ def _parse_condition(ctxt, node): "preprocessor": _RuleDesc("scalar"), "constexpr": _RuleDesc("scalar"), "expr": _RuleDesc("scalar"), + "feature_flag": _RuleDesc("scalar"), }) return condition diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py index 90d53bebeae..77453397478 100644 --- a/buildscripts/idl/idl/syntax.py +++ b/buildscripts/idl/idl/syntax.py @@ -734,6 +734,7 @@ class Condition(common.SourceLocation): self.expr = None # type: str self.constexpr = None # type: str self.preprocessor = None # type: str + self.feature_flag = None # type: str super(Condition, self).__init__(file_name, line, column) diff --git a/jstests/libs/cluster_server_parameter_utils.js b/jstests/libs/cluster_server_parameter_utils.js index 9ea692e9649..be242dca010 100644 --- a/jstests/libs/cluster_server_parameter_utils.js +++ b/jstests/libs/cluster_server_parameter_utils.js @@ -2,129 +2,88 @@ * Util functions used by cluster server parameter tests. * * When adding new cluster server parameter, do the following: - * 1. If it's test-only, add its name to the end of testOnlyClusterParameterNames. Otherwise, add it - * it to the end of nonTestClusterParameterNames. - * 2. Add the clusterParameter document that's expected as default to the end of - * testOnlyClusterParametersDefault if it's test-only. Otherwise, add it to the end of - * nonTestClusterParametersDefault. - * 3. Add the clusterParameter document that setClusterParameter is expected to insert after its - * first invocation to the end of testOnlyClusterParametersInsert if it's test-only. Otherwise, - * add it to the end of nonTestClusterParametersInsert. - * 4. Add the clusterParameter document that setClusterParameter is expected to update to after its - * second invocation to the end of testOnlyClusterParametersUpdate if it's test-only. Otherwise, - * add it to the end of nonTestClusterParametersUpdate. - * + * If it's test-only, add its definition to kTestOnlyClusterParameters. + * Otherwise, add to kNonTestOnlyClusterParameters. + * The keyname will be the name of the cluster-wide server parameter, + * it's value will be an object with at least three keys named: + * * 'default': Properties to expect on an unset CWSP + * * 'insert': Values to set on the CWSP on first write + * * 'update': Values to set on the CWSP on second write + * A fourth property 'featureFlag' may also be set if the + * parameter depends on a featureFlag. + * Use the name of the featureFlag if it is required in + * order to consider the parameter. + * Prefix the name with a bang '!' if it is only considered + * when the featureFlag is disabled. */ load("jstests/libs/feature_flag_util.js"); -const testOnlyClusterParameterNames = [ - "testStrClusterParameter", - "testIntClusterParameter", - "testBoolClusterParameter", -]; -const nonTestClusterParameterNames = ["changeStreamOptions", "changeStreams"]; -const clusterParameterNames = testOnlyClusterParameterNames.concat(nonTestClusterParameterNames); - -// A dictionary to ignore the cluster parameters based on specific criteria. The key is the name of -// a cluster parameter and the value is a boolean function returning 'true' in cases when the -// parameter should be ignored. -const ignoreParametersDict = { - changeStreamOptions: function(conn) { - return FeatureFlagUtil.isEnabled(conn, "ServerlessChangeStreams"); +const kNonTestOnlyClusterParameters = { + changeStreamOptions: { + default: {preAndPostImages: {expireAfterSeconds: 'off'}}, + insert: {preAndPostImages: {expireAfterSeconds: 30}}, + update: {preAndPostImages: {expireAfterSeconds: 'off'}}, + featureFlag: '!ServerlessChangeStreams', + }, + changeStreams: { + default: {expireAfterSeconds: NumberLong(3600)}, + insert: {expireAfterSeconds: 30}, + update: {expireAfterSeconds: 10}, + featureFlag: 'ServerlessChangeStreams', }, - changeStreams: function(conn) { - return !FeatureFlagUtil.isEnabled(conn, "ServerlessChangeStreams"); - } }; -function ignoreParameter(paramName, conn) { - if (ignoreParametersDict[paramName]) { - return ignoreParametersDict[paramName](conn); - } - return false; -} - -const testOnlyClusterParametersDefault = [ - { - _id: "testStrClusterParameter", - strData: "off", - }, - { - _id: "testIntClusterParameter", - intData: 16, +const kTestOnlyClusterParameters = { + cwspTestNeedsFeatureFlagClusterWideToaster: { + default: {intData: 16}, + insert: {intData: 17}, + update: {intData: 18}, + featureFlag: 'ClusterWideToaster', }, - { - _id: "testBoolClusterParameter", - boolData: false, + testStrClusterParameter: { + default: {strData: 'off'}, + insert: {strData: 'on'}, + update: {strData: 'sleep'}, }, -]; -const nonTestClusterParametersDefault = [ - { - _id: "changeStreamOptions", - preAndPostImages: { - expireAfterSeconds: "off", - }, + testIntClusterParameter: { + default: {intData: 16}, + insert: {intData: 17}, + update: {intData: 18}, }, - {_id: "changeStreams", expireAfterSeconds: NumberLong(3600)} -]; -const clusterParametersDefault = - testOnlyClusterParametersDefault.concat(nonTestClusterParametersDefault); - -const testOnlyClusterParametersInsert = [ - { - _id: "testStrClusterParameter", - strData: "on", + testBoolClusterParameter: { + default: {boolData: false}, + insert: {boolData: true}, + update: {boolData: false}, }, - { - _id: "testIntClusterParameter", - intData: 17, - }, - { - _id: "testBoolClusterParameter", - boolData: true, - }, -]; -const nonTestClusterParametersInsert = [ - { - _id: "changeStreamOptions", - preAndPostImages: { - expireAfterSeconds: 30, - }, - }, - { - _id: "changeStreams", - expireAfterSeconds: 30, +}; + +const kAllClusterParameters = + Object.assign({}, kNonTestOnlyClusterParameters, kTestOnlyClusterParameters); +const kAllClusterParameterNames = Object.keys(kAllClusterParameters); +const kAllClusterParameterDefaults = kAllClusterParameterNames.map( + (name) => Object.assign({_id: name}, kAllClusterParameters[name].default)); +const kAllClusterParameterInserts = kAllClusterParameterNames.map( + (name) => Object.assign({_id: name}, kAllClusterParameters[name].insert)); +const kAllClusterParameterUpdates = kAllClusterParameterNames.map( + (name) => Object.assign({_id: name}, kAllClusterParameters[name].update)); + +const kNonTestOnlyClusterParameterDefaults = + Object.keys(kNonTestOnlyClusterParameters) + .map((name) => Object.assign({_id: name}, kAllClusterParameters[name].default)); + +function considerParameter(paramName, conn) { + // { featureFlag: 'name' } indicates that the CWSP should only be considered with the FF + // enabled. { featureFlag: '!name' } indicates that the CWSP should only be considered with the + // FF disabled. + const cp = kAllClusterParameters[paramName] || {}; + if (cp.featureFlag) { + const considerWhenFFEnabled = cp.featureFlag[0] !== '!'; + const ff = cp.featureFlag.substr(considerWhenFFEnabled ? 0 : 1); + return FeatureFlagUtil.isEnabled(conn, ff) === considerWhenFFEnabled; } -]; -const clusterParametersInsert = - testOnlyClusterParametersInsert.concat(nonTestClusterParametersInsert); - -const testOnlyClusterParametersUpdate = [ - { - _id: "testStrClusterParameter", - strData: "sleep", - }, - { - _id: "testIntClusterParameter", - intData: 18, - }, - { - _id: "testBoolClusterParameter", - boolData: false, - }, -]; -const nonTestClusterParametersUpdate = [ - { - _id: "changeStreamOptions", - preAndPostImages: { - expireAfterSeconds: "off", - }, - }, - {_id: "changeStreams", expireAfterSeconds: NumberLong(10)} -]; -const clusterParametersUpdate = - testOnlyClusterParametersUpdate.concat(nonTestClusterParametersUpdate); + return true; +} // Set the log level for get/setClusterParameter logging to appear. function setupNode(conn) { @@ -152,7 +111,7 @@ function setupSharded(st) { // Upserts config.clusterParameters document with w:majority via setClusterParameter. function runSetClusterParameter(conn, update) { const paramName = update._id; - if (ignoreParameter(paramName, conn)) { + if (!considerParameter(paramName, conn)) { return; } let updateCopy = Object.assign({}, update); @@ -170,37 +129,42 @@ function runSetClusterParameter(conn, update) { // on whether the expected values were returned. function runGetClusterParameterNode(conn, getClusterParameterArgs, expectedClusterParameters) { const adminDB = conn.getDB('admin'); + + // Filter out parameters that we don't care about. + if (Array.isArray(getClusterParameterArgs)) { + getClusterParameterArgs = + getClusterParameterArgs.filter((name) => considerParameter(name, conn)); + } else if ((typeof getClusterParameterArgs === 'string') && + !considerParameter(getClusterParameterArgs, conn)) { + return true; + } + const actualClusterParameters = assert.commandWorked(adminDB.runCommand({getClusterParameter: getClusterParameterArgs})) .clusterParameters; - // Sort the returned clusterParameters and the expected clusterParameters by _id. - actualClusterParameters.sort((a, b) => a._id.localeCompare(b._id)); - expectedClusterParameters.sort((a, b) => a._id.localeCompare(b._id)); + // Reindex actual based on name, and remove irrelevant field. + let actual = {}; + actualClusterParameters.forEach(function(acp) { + actual[acp._id] = acp; + delete actual[acp._id].clusterParameterTime; + }); + for (let i = 0; i < expectedClusterParameters.length; i++) { - const expectedClusterParameter = expectedClusterParameters[i]; - const actualClusterParameter = actualClusterParameters[i]; - - // Sort both expectedClusterParameter and actualClusterParameter into alphabetical order - // by key. - const sortedExpectedClusterParameter = - Object.keys(expectedClusterParameter).sort().reduce(function(sorted, key) { - sorted[key] = expectedClusterParameter[key]; - return sorted; - }, {}); - const sortedActualClusterParameter = - Object.keys(actualClusterParameter).sort().reduce(function(sorted, key) { - if (key !== 'clusterParameterTime') { - sorted[key] = actualClusterParameter[key]; - } - return sorted; - }, {}); - if (ignoreParameter(expectedClusterParameter._id, conn)) { + if (!considerParameter(expectedClusterParameters[i]._id, conn)) { continue; } - if (bsonWoCompare(sortedExpectedClusterParameter, sortedActualClusterParameter) !== 0) { - print('expected: ' + tojson(sortedExpectedClusterParameter) + - '\nactual: ' + tojson(sortedActualClusterParameter)); + + const id = expectedClusterParameters[i]._id; + if (actual[id] === undefined) { + jsTest.log('Expected to retreive ' + id + ' but it was not returned'); + return false; + } + + if (bsonWoCompare(expectedClusterParameters[i], actual[id]) !== 0) { + jsTest.log('Server parameter mismatch on node: ' + conn.host + '\n' + + 'Expected: ' + tojson(expectedClusterParameters[i]) + '\n' + + 'Actual: ' + tojson(actual[id])); return false; } } @@ -246,53 +210,56 @@ function testValidClusterParameterCommands(conn) { if (conn instanceof ReplSetTest) { // Run getClusterParameter in list format and '*' and ensure it returns all default values // on all nodes in the replica set. - runGetClusterParameterReplicaSet(conn, clusterParameterNames, clusterParametersDefault); - runGetClusterParameterReplicaSet(conn, '*', clusterParametersDefault); + runGetClusterParameterReplicaSet( + conn, kAllClusterParameterNames, kAllClusterParameterDefaults); + runGetClusterParameterReplicaSet(conn, '*', kAllClusterParameterDefaults); // For each parameter, run setClusterParameter and verify that getClusterParameter // returns the updated value on all nodes in the replica set. - for (let i = 0; i < clusterParameterNames.length; i++) { - runSetClusterParameter(conn.getPrimary(), clusterParametersInsert[i]); + for (let i = 0; i < kAllClusterParameterNames.length; i++) { + runSetClusterParameter(conn.getPrimary(), kAllClusterParameterInserts[i]); runGetClusterParameterReplicaSet( - conn, clusterParameterNames[i], [clusterParametersInsert[i]]); + conn, kAllClusterParameterNames[i], [kAllClusterParameterInserts[i]]); } // Do the above again to verify that document updates are also handled properly. - for (let i = 0; i < clusterParameterNames.length; i++) { - runSetClusterParameter(conn.getPrimary(), clusterParametersUpdate[i]); + for (let i = 0; i < kAllClusterParameterNames.length; i++) { + runSetClusterParameter(conn.getPrimary(), kAllClusterParameterUpdates[i]); runGetClusterParameterReplicaSet( - conn, clusterParameterNames[i], [clusterParametersUpdate[i]]); + conn, kAllClusterParameterNames[i], [kAllClusterParameterUpdates[i]]); } // Finally, run getClusterParameter in list format and '*' and ensure that they now all // return updated values. - runGetClusterParameterReplicaSet(conn, clusterParameterNames, clusterParametersUpdate); - runGetClusterParameterReplicaSet(conn, '*', clusterParametersUpdate); + runGetClusterParameterReplicaSet( + conn, kAllClusterParameterNames, kAllClusterParameterUpdates); + runGetClusterParameterReplicaSet(conn, '*', kAllClusterParameterUpdates); } else { // Run getClusterParameter in list format and '*' and ensure it returns all default values // on all nodes in the sharded cluster. - runGetClusterParameterSharded(conn, clusterParameterNames, clusterParametersDefault); - runGetClusterParameterSharded(conn, '*', clusterParametersDefault); + runGetClusterParameterSharded( + conn, kAllClusterParameterNames, kAllClusterParameterDefaults); + runGetClusterParameterSharded(conn, '*', kAllClusterParameterDefaults); // For each parameter, simulate setClusterParameter and verify that getClusterParameter // returns the updated value on all nodes in the sharded cluster. - for (let i = 0; i < clusterParameterNames.length; i++) { - runSetClusterParameter(conn.s0, clusterParametersInsert[i]); + for (let i = 0; i < kAllClusterParameterNames.length; i++) { + runSetClusterParameter(conn.s0, kAllClusterParameterInserts[i]); runGetClusterParameterSharded( - conn, clusterParameterNames[i], [clusterParametersInsert[i]]); + conn, kAllClusterParameterNames[i], [kAllClusterParameterInserts[i]]); } // Do the above again to verify that document updates are also handled properly. - for (let i = 0; i < clusterParameterNames.length; i++) { - runSetClusterParameter(conn.s0, clusterParametersUpdate[i]); + for (let i = 0; i < kAllClusterParameterNames.length; i++) { + runSetClusterParameter(conn.s0, kAllClusterParameterUpdates[i]); runGetClusterParameterSharded( - conn, clusterParameterNames[i], [clusterParametersUpdate[i]]); + conn, kAllClusterParameterNames[i], [kAllClusterParameterUpdates[i]]); } // Finally, run getClusterParameter in list format and '*' and ensure that they now all // return updated values. - runGetClusterParameterSharded(conn, clusterParameterNames, clusterParametersUpdate); - runGetClusterParameterSharded(conn, '*', clusterParametersUpdate); + runGetClusterParameterSharded(conn, kAllClusterParameterNames, kAllClusterParameterUpdates); + runGetClusterParameterSharded(conn, '*', kAllClusterParameterUpdates); } } @@ -327,13 +294,13 @@ function testDisabledClusterParameters(conn) { // Assert that getClusterParameter: '*' succeeds but only returns enabled cluster // parameters. - runGetClusterParameterReplicaSet(conn, '*', nonTestClusterParametersDefault); + runGetClusterParameterReplicaSet(conn, '*', kNonTestOnlyClusterParameterDefaults); } else { // Assert that explicitly setting a disabled cluster server parameter fails. const adminDB = conn.s0.getDB('admin'); assert.commandFailedWithCode( adminDB.runCommand({setClusterParameter: {testIntClusterParameter: {intData: 5}}}), - ErrorCodes.BadValue); + ErrorCodes.IllegalOperation); // Assert that explicitly getting a disabled cluster server parameter fails on mongos. testExplicitDisabledGetClusterParameter(conn.s0); @@ -356,7 +323,7 @@ function testDisabledClusterParameters(conn) { // Assert that getClusterParameter: '*' succeeds but only returns enabled cluster // parameters. - runGetClusterParameterSharded(conn, '*', nonTestClusterParametersDefault); + runGetClusterParameterSharded(conn, '*', kNonTestOnlyClusterParameterDefaults); } } diff --git a/jstests/noPassthrough/server_parameter_feature_flag.js b/jstests/noPassthrough/server_parameter_feature_flag.js new file mode 100644 index 00000000000..539428f9b88 --- /dev/null +++ b/jstests/noPassthrough/server_parameter_feature_flag.js @@ -0,0 +1,84 @@ +// Test featureFlag enable/disable based on feature flag. +// This test will run whether or not featureFlagToaster is enabled. +// If it is, then we expect the serverParameter to be available and settable. +// If it is not, then we expect no such serverParameter. + +(function() { +'use strict'; + +function runTestForSP(conn) { + const kFF = 'featureFlagToaster'; + const kSP = 'spTestNeedsFeatureFlagToaster'; + const ff = TestData.setParameters[kFF]; + const assertion = ff ? assert.commandWorked : assert.commandFailed; + jsTest.log('ServerParameter: ' + kSP); + jsTest.log(kFF + ': ' + tojson(ff)); + + const admin = conn.getDB('admin'); + const initial = assertion(admin.runCommand({getParameter: 1, [kSP]: 1})); + jsTest.log('Initial: ' + tojson(initial)); + const newval = initial[kSP] + 1; + const set = assertion(admin.runCommand({setParameter: 1, [kSP]: newval})); + jsTest.log('Set: ' + tojson(set)); + const changed = assertion(admin.runCommand({getParameter: 1, [kSP]: 1})); + jsTest.log('Changed: ' + tojson(changed)); + if (ff) { + assert.neq(initial[kSP], changed[kSP]); + } else { + assert.eq(initial.errmsg, 'no option found to get'); + assert.eq(set.errmsg, "Server parameter: '" + kSP + "' is disabled"); + assert.eq(changed.errmsg, 'no option found to get'); + } +} + +function runTestForCWSP(conn) { + const kFF = 'featureFlagClusterWideToaster'; + const kSP = 'cwspTestNeedsFeatureFlagClusterWideToaster'; + const ff = TestData.setParameters[kFF]; + const assertion = ff ? assert.commandWorked : assert.commandFailed; + jsTest.log('ClusterWideServerParameter: ' + kSP); + jsTest.log(kFF + ': ' + tojson(ff)); + + function val(res) { + if (!ff) { + return 0; + } + const obj = res.clusterParameters.filter((cwsp) => cwsp._id === kSP)[0]; + return (obj === undefined) ? 0 : obj.intData; + } + + const admin = conn.getDB('admin'); + const initial = assertion(admin.runCommand({getClusterParameter: kSP})); + jsTest.log('Initial: ' + tojson(initial)); + const set = + assertion(admin.runCommand({setClusterParameter: {[kSP]: {intData: val(initial) + 1}}})); + jsTest.log('Set: ' + tojson(set)); + const changed = assertion(admin.runCommand({getClusterParameter: kSP})); + jsTest.log('Changed: ' + tojson(changed)); + if (ff) { + assert.neq(val(initial), val(changed)); + } else { + const msg = "Server parameter: '" + kSP + "' is disabled"; + assert.eq(initial.errmsg, msg); + assert.eq(set.errmsg, msg); + assert.eq(changed.errmsg, msg); + } +} + +{ + jsTest.log('START standalone'); + const standalone = MongoRunner.runMongod({}); + runTestForSP(standalone); + MongoRunner.stopMongod(standalone); + jsTest.log('END standalone'); +} + +{ + jsTest.log('BEGIN sharding'); + const s = new ShardingTest({mongos: 1, config: 1, shards: 3}); + runTestForSP(s.s0); + runTestForCWSP(s.s0); + s.stop(); + jsTest.log('END sharding'); +} +})(); diff --git a/src/mongo/db/SConscript b/src/mongo/db/SConscript index be4f755fac0..535af49bb6d 100644 --- a/src/mongo/db/SConscript +++ b/src/mongo/db/SConscript @@ -79,6 +79,8 @@ env.Library( '$BUILD_DIR/mongo/base', '$BUILD_DIR/mongo/idl/idl_parser', '$BUILD_DIR/mongo/util/options_parser/options_parser', + ], LIBDEPS_PRIVATE=[ + 'server_options_core', ]) # diff --git a/src/mongo/db/commands/get_cluster_parameter_invocation.cpp b/src/mongo/db/commands/get_cluster_parameter_invocation.cpp index 64e269ea20b..b931c9c4db5 100644 --- a/src/mongo/db/commands/get_cluster_parameter_invocation.cpp +++ b/src/mongo/db/commands/get_cluster_parameter_invocation.cpp @@ -77,7 +77,7 @@ GetClusterParameterInvocation::retrieveRequestedParameters( ServerParameter* sp = clusterParameters->get(strParameterName); uassert(ErrorCodes::BadValue, str::stream() << "Server parameter: '" << strParameterName - << "' is currently disabled", + << "' is disabled", sp->isEnabled()); makeBSON(sp); } @@ -95,7 +95,7 @@ GetClusterParameterInvocation::retrieveRequestedParameters( uassert(ErrorCodes::BadValue, str::stream() << "Server parameter: '" << requestedParameterName - << "' is currently disabled'", + << "' is disabled'", sp->isEnabled()); makeBSON(sp); } diff --git a/src/mongo/db/commands/parameters.cpp b/src/mongo/db/commands/parameters.cpp index 997b910de8c..574f47e91bb 100644 --- a/src/mongo/db/commands/parameters.cpp +++ b/src/mongo/db/commands/parameters.cpp @@ -240,7 +240,7 @@ public: const ServerParameter::Map& m = ServerParameterSet::getNodeParameterSet()->getMap(); for (ServerParameter::Map::const_iterator i = m.begin(); i != m.end(); ++i) { - if (all || cmdObj.hasElement(i->first.c_str())) { + if (i->second->isEnabled() && (all || cmdObj.hasElement(i->first.c_str()))) { if (options.getShowDetails()) { BSONObjBuilder detailBob(result.subobjStart(i->second->name())); i->second->append(opCtx, &detailBob, "value", boost::none); @@ -322,6 +322,12 @@ public: << "], use help:true to see options ", foundParameter != parameterMap.end()); + // Is the parameter disabled? + uassert(ErrorCodes::InvalidOptions, + str::stream() << "Server parameter: '" << foundParameter->second->name() + << "' is disabled", + foundParameter->second->isEnabled()); + // Make sure we are allowed to change this parameter uassert(ErrorCodes::IllegalOperation, str::stream() << "not allowed to change [" << parameterName << "] at runtime", diff --git a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp index 4b04c491894..fb563b6ae6f 100644 --- a/src/mongo/db/commands/set_cluster_parameter_invocation.cpp +++ b/src/mongo/db/commands/set_cluster_parameter_invocation.cpp @@ -54,6 +54,10 @@ bool SetClusterParameterInvocation::invoke(OperationContext* opCtx, StringData parameterName = cmdParamObj.firstElement().fieldName(); ServerParameter* serverParameter = _sps->get(parameterName); + uassert(ErrorCodes::BadValue, + str::stream() << "Server parameter: '" << serverParameter->name() << "' is disabled", + serverParameter->isEnabled()); + auto [query, update] = normalizeParameter( opCtx, cmdParamObj, paramTime, serverParameter, parameterName, cmd.getDbName().tenantId()); @@ -80,6 +84,10 @@ std::pair SetClusterParameterInvocation::normalizeParameter( "Cluster parameter value must be an object", BSONType::Object == commandElement.type()); + uassert(ErrorCodes::IllegalOperation, + str::stream() << "Server parameter: '" << sp->name() << "' is disabled", + sp->isEnabled()); + Timestamp clusterTime = paramTime ? *paramTime : _dbService.getUpdateClusterTime(opCtx); BSONObjBuilder updateBuilder; diff --git a/src/mongo/db/feature_flag_test.idl.tpl b/src/mongo/db/feature_flag_test.idl.tpl index 86a2734dfa3..ec51b9f6419 100644 --- a/src/mongo/db/feature_flag_test.idl.tpl +++ b/src/mongo/db/feature_flag_test.idl.tpl @@ -46,3 +46,14 @@ feature_flags: default: true # The version should match GenericFCV::kLastLTS in the generated 'releases.h' file. version: $ver_str(last_lts) + +server_parameters: + spTestNeedsFeatureFlagToaster: + description: "Server Parameter gated on featureFlagToaster" + set_at: runtime + cpp_varname: gSPTestFeatureFlagToaster + cpp_vartype: bool + test_only: true + default: false + condition: + feature_flag: featureFlagToaster diff --git a/src/mongo/db/server_parameter.cpp b/src/mongo/db/server_parameter.cpp index 9e70ab3aac1..50a34796fb1 100644 --- a/src/mongo/db/server_parameter.cpp +++ b/src/mongo/db/server_parameter.cpp @@ -31,6 +31,7 @@ #include +#include "mongo/db/feature_flag.h" #include "mongo/logv2/log.h" #include "mongo/util/static_immortal.h" @@ -76,6 +77,20 @@ ServerParameterSet* ServerParameterSet::getNodeParameterSet() { return &*obj; } +bool ServerParameter::featureFlagIsDisabled() const { + if (!_featureFlag) { + return false; + } + + if (!serverGlobalParams.featureCompatibility.isVersionInitialized()) { + // Minor race-condition at startup. + // Pretend it's not enabled until we know for certain that it is. + return true; + } + + return !_featureFlag->isEnabled(serverGlobalParams.featureCompatibility); +} + ServerParameterSet* ServerParameterSet::getClusterParameterSet() { static StaticImmortal obj = [] { ServerParameterSet sps; diff --git a/src/mongo/db/server_parameter.h b/src/mongo/db/server_parameter.h index 4d583096c33..29b3347ec7b 100644 --- a/src/mongo/db/server_parameter.h +++ b/src/mongo/db/server_parameter.h @@ -88,6 +88,7 @@ enum class ServerParameterType { kClusterWide, }; +class FeatureFlag; class ServerParameterSet; class OperationContext; @@ -217,15 +218,22 @@ public: } virtual bool isEnabled() const { - return true; + return !featureFlagIsDisabled(); + } + + void setFeatureFlag(FeatureFlag* featureFlag) { + _featureFlag = featureFlag; } protected: + bool featureFlagIsDisabled() const; + // Helper for translating setParameter values from BSON to string. StatusWith _coerceToString(const BSONElement&); private: std::string _name; + FeatureFlag* _featureFlag = nullptr; ServerParameterType _type; bool _testOnly = false; bool _redact = false; diff --git a/src/mongo/idl/cluster_server_parameter.idl b/src/mongo/idl/cluster_server_parameter.idl index 7c8485e9374..52771ed4cd7 100644 --- a/src/mongo/idl/cluster_server_parameter.idl +++ b/src/mongo/idl/cluster_server_parameter.idl @@ -92,6 +92,10 @@ feature_flags: cpp_varname: gFeatureFlagClusterWideConfigM2 default: true version: 6.1 + featureFlagClusterWideToaster: + description: Feature flag for testing use of feature flags with CW server params + cpp_varname: gFeatureFlagClusterWideToaster + default: false server_parameters: testIntClusterParameter: @@ -114,3 +118,12 @@ server_parameters: cpp_vartype: TestBoolClusterParameterStorage cpp_varname: boolStorage test_only: true + + cwspTestNeedsFeatureFlagClusterWideToaster: + description: "Server Parameter gated on featureFlagClusterWideToaster" + set_at: cluster + cpp_varname: gCWSPTestFeatureFlagClusterWideToaster + cpp_vartype: TestIntClusterParameterStorage + test_only: true + condition: + feature_flag: featureFlagClusterWideToaster -- cgit v1.2.1