diff --git a/src/yb/server/generic_service.cc b/src/yb/server/generic_service.cc index 8b307ee96356..0d162193a5e0 100644 --- a/src/yb/server/generic_service.cc +++ b/src/yb/server/generic_service.cc @@ -31,32 +31,15 @@ // #include "yb/server/generic_service.h" -#include -#include -#include -#include -#include #include -#include -#include #include #include -#include "yb/util/logging.h" - -#include "yb/gutil/atomicops.h" -#include "yb/gutil/callback_forward.h" -#include "yb/gutil/dynamic_annotations.h" -#include "yb/gutil/macros.h" -#include "yb/gutil/map-util.h" -#include "yb/gutil/port.h" -#include "yb/gutil/strings/split.h" + #include "yb/rpc/rpc_context.h" #include "yb/server/clock.h" #include "yb/server/server_base.h" #include "yb/util/flags.h" -#include "yb/util/metrics_fwd.h" -#include "yb/util/monotime.h" #include "yb/util/status.h" #include "yb/util/status_format.h" #include "yb/util/status_fwd.h" @@ -137,6 +120,18 @@ void GenericServiceImpl::GetFlag(const GetFlagRequestPB* req, rpc.RespondSuccess(); } +void GenericServiceImpl::ValidateFlagValue( + const ValidateFlagValueRequestPB* req, ValidateFlagValueResponsePB* resp, rpc::RpcContext rpc) { + auto status = flags_internal::ValidateFlagValue(req->flag_name(), req->flag_value()); + if (status.ok()) { + rpc.RespondSuccess(); + return; + } + + LOG(WARNING) << "Flag validation failed: " << status; + rpc.RespondFailure(status); +} + void GenericServiceImpl::FlushCoverage(const FlushCoverageRequestPB* req, FlushCoverageResponsePB* resp, rpc::RpcContext rpc) { diff --git a/src/yb/server/generic_service.h b/src/yb/server/generic_service.h index 0638ad005005..823fa28809f2 100644 --- a/src/yb/server/generic_service.h +++ b/src/yb/server/generic_service.h @@ -56,6 +56,10 @@ class GenericServiceImpl : public GenericServiceIf { GetFlagResponsePB* resp, rpc::RpcContext rpc) override; + void ValidateFlagValue( + const ValidateFlagValueRequestPB* req, ValidateFlagValueResponsePB* resp, + rpc::RpcContext rpc) override; + void GetAutoFlagsConfigVersion( const GetAutoFlagsConfigVersionRequestPB* req, GetAutoFlagsConfigVersionResponsePB* resp, diff --git a/src/yb/server/server_base.proto b/src/yb/server/server_base.proto index a4defbccabc2..8ed29f299fa6 100644 --- a/src/yb/server/server_base.proto +++ b/src/yb/server/server_base.proto @@ -155,6 +155,15 @@ message GetAutoFlagsConfigVersionResponsePB { required uint32 config_version = 1; } +message ValidateFlagValueRequestPB { + required string flag_name = 1; + required string flag_value = 2; +} + +message ValidateFlagValueResponsePB { + // RPC will fail on errors. +} + service GenericService { rpc SetFlag(SetFlagRequestPB) returns (SetFlagResponsePB); @@ -162,6 +171,9 @@ service GenericService { rpc GetFlag(GetFlagRequestPB) returns (GetFlagResponsePB); + rpc ValidateFlagValue(ValidateFlagValueRequestPB) + returns (ValidateFlagValueResponsePB); + rpc RefreshFlags(RefreshFlagsRequestPB) returns (RefreshFlagsResponsePB); diff --git a/src/yb/tools/ts-cli.cc b/src/yb/tools/ts-cli.cc index 98a8f4e29c27..e0e893cb12fb 100644 --- a/src/yb/tools/ts-cli.cc +++ b/src/yb/tools/ts-cli.cc @@ -102,6 +102,7 @@ const char* const kVerifyTabletOp = "verify_tablet"; const char* const kAreTabletsRunningOp = "are_tablets_running"; const char* const kIsServerReadyOp = "is_server_ready"; const char* const kSetFlagOp = "set_flag"; +const char* const kValidateFlagValueOp = "validate_flag_value"; const char* const kRefreshFlagsOp = "refresh_flags"; const char* const kDumpTabletOp = "dump_tablet"; const char* const kTabletStateOp = "get_tablet_state"; @@ -200,6 +201,9 @@ class TsAdminClient { Status SetFlag(const string& flag, const string& val, bool force); + // Validates the value of a flag without actually setting it. + Status ValidateFlagValue(const string& flag, const string& val); + // Refreshes all gflags on the remote server to the flagfile, via RPC. Status RefreshFlags(); @@ -383,6 +387,18 @@ Status TsAdminClient::SetFlag(const string& flag, const string& val, } } +Status TsAdminClient::ValidateFlagValue(const string& flag, const string& val) { + server::ValidateFlagValueRequestPB req; + server::ValidateFlagValueResponsePB resp; + RpcController rpc; + + rpc.set_timeout(timeout_); + req.set_flag_name(flag); + req.set_flag_value(val); + + return generic_proxy_->ValidateFlagValue(req, &resp, &rpc); +} + Status TsAdminClient::RefreshFlags() { server::RefreshFlagsRequestPB req; server::RefreshFlagsResponsePB resp; @@ -727,6 +743,7 @@ void SetUsage(const char* argv0) { << " " << kAreTabletsRunningOp << "\n" << " " << kIsServerReadyOp << "\n" << " " << kSetFlagOp << " [-force] \n" + << " " << kValidateFlagValueOp << " \n" << " " << kRefreshFlagsOp << "\n" << " " << kTabletStateOp << " \n" << " " << kDumpTabletOp << " \n" @@ -860,6 +877,13 @@ static int TsCliMain(int argc, char** argv) { RETURN_NOT_OK_PREPEND_FROM_MAIN(client.SetFlag(argv[2], argv[3], FLAGS_force), "Unable to set flag"); + } else if (op == kValidateFlagValueOp) { + CHECK_ARGC_OR_RETURN_WITH_USAGE(op, 4); + + RETURN_NOT_OK_PREPEND_FROM_MAIN( + client.ValidateFlagValue(argv[2], argv[3]), "Invalid flag value"); + std::cout << "Flag value is valid" << std::endl; + } else if (op == kRefreshFlagsOp) { CHECK_ARGC_OR_RETURN_WITH_USAGE(op, 2); diff --git a/src/yb/util/flags.h b/src/yb/util/flags.h index ef89ceeb785d..ec558ddac0fc 100644 --- a/src/yb/util/flags.h +++ b/src/yb/util/flags.h @@ -38,15 +38,29 @@ #include "yb/util/flags/flags_callback.h" #include "yb/util/flags/auto_flags.h" -// Redefine the macro from gflags.h with an unused attribute. -// Note: This macro should be used in the same file that DEFINEs the flag. Using it any other file -// can result in segfault due to indeterminate order of static initialization. +// Macro for the registration of a flag validator. +// The validation function should return true if the flag value is valid, and false otherwise. If +// the function returns false for the new value of the flag, the flag will retain its current value. +// If it returns false for the default value, the process will die. Use LOG_FLAG_VALIDATION_ERROR to +// log error messages when returning false. +// +// Validator fuction should be of the form: +// bool ValidatorFunc(const char* flag_name, value); +// for strings the second argument should be `const std::string&`. +// +// Note: +// If the validation depends on the value of other flags then make sure to call +// DELAY_FLAG_VALIDATION_ON_STARTUP macro, so that we do not enforce a restriction on the order of +// flags in command line and flags file. +// +// This macro should be used in the same file that DEFINEs the flag. Using it any other file can +// result in segfault due to indeterminate order of static initialization. #ifdef DEFINE_validator #undef DEFINE_validator #endif #define DEFINE_validator(name, validator) \ static_assert( \ - sizeof(_DEFINE_FLAG_IN_FILE(name)), "validator must be DEFINED in the same file as the flag"); \ + sizeof(_DEFINE_FLAG_IN_FILE(name)), "validator must be DEFINED in the same file as the flag"); \ static const bool BOOST_PP_CAT(name, _validator_registered) __attribute__((unused)) = \ google::RegisterFlagValidator(&BOOST_PP_CAT(FLAGS_, name), (validator)) @@ -121,6 +135,17 @@ SetFlagResult SetFlag( const std::string& flag_name, const std::string& new_value, const SetFlagForce force, std::string* old_value, std::string* output_msg); +// If the flag is tagged as sensitive_info then returns a masked value('***'). +// Only string values are sensitive. +std::string GetMaskedValueIfSensitive(const std::string& flag_name, const std::string& value); +template +T GetMaskedValueIfSensitive(const std::string& flag_name, T value) { + return value; +} + +// Check if the flag can be set to the new value. Does not actually set the flag. +Status ValidateFlagValue(const std::string& flag_name, const std::string& value); + } // namespace flags_internal // In order to mark a flag as deprecated, use this macro: @@ -166,9 +191,12 @@ class FlagValidatorSink : public google::LogSink { FlagValidatorSink& GetFlagValidatorSink(); +// Log error message to the error log and the flag validator sink, which will ensure it is sent +// back to the user. Also masks any sensitive values. #define LOG_FLAG_VALIDATION_ERROR(flag_name, value) \ LOG_TO_SINK(&yb::GetFlagValidatorSink(), ERROR) \ - << "Invalid value '" << value << "' for flag '" << flag_name << "': " + << "Invalid value '" << yb::flags_internal::GetMaskedValueIfSensitive(flag_name, value) \ + << "' for flag '" << flag_name << "': " // Returns true if the flag was recorded for delayed validation, and the validation can be skipped. bool RecordFlagForDelayedValidation(const std::string& flag_name); diff --git a/src/yb/util/flags/flags-test.cc b/src/yb/util/flags/flags-test.cc index 2ae327fed9da..5c822be791f3 100644 --- a/src/yb/util/flags/flags-test.cc +++ b/src/yb/util/flags/flags-test.cc @@ -14,8 +14,9 @@ #include #include "yb/util/flags.h" -#include "yb/util/status.h" +#include "yb/util/logging_test_util.h" #include "yb/util/status_log.h" +#include "yb/util/status.h" #include "yb/util/test_util.h" using std::string; @@ -35,6 +36,19 @@ DECLARE_string(vmodule); DECLARE_string(allowed_preview_flags_csv); DEFINE_RUNTIME_PREVIEW_bool(preview_flag, false, "preview flag"); +DEFINE_NON_RUNTIME_string(flagstest_secret_flag, "", "This is a secret"); +TAG_FLAG(flagstest_secret_flag, sensitive_info); + +const auto kBadSecretValue = "yb_rox"; +bool ValidateSecretFlag(const char* flag_name, const std::string& new_val) { + if (new_val == kBadSecretValue) { + LOG_FLAG_VALIDATION_ERROR(flag_name, new_val) << "This is no secret. Everyone knows it!"; + return false; + } + return true; +} +DEFINE_validator(flagstest_secret_flag, &ValidateSecretFlag); + namespace yb { class FlagsTest : public YBTest { @@ -97,17 +111,23 @@ TEST_F(FlagsTest, TestVmodule) { // Set to invalid value string old_value, output_msg; - ASSERT_DEATH( + ASSERT_EQ( SetFlag("vmodule", "BadValue", SetFlagForce::kFalse, &old_value, &output_msg), - "'BadValue' is not valid"); + SetFlagResult::BAD_VALUE); + ASSERT_STR_CONTAINS(output_msg, "'BadValue' is not valid"); - ASSERT_DEATH( + ASSERT_EQ( SetFlag("vmodule", "files=", SetFlagForce::kFalse, &old_value, &output_msg), - "'files=' is not valid"); + SetFlagResult::BAD_VALUE); + ASSERT_STR_CONTAINS(output_msg, "'files=' is not valid"); - ASSERT_DEATH( + ASSERT_EQ( SetFlag("vmodule", "biggerThanInt=2147483648", SetFlagForce::kFalse, &old_value, &output_msg), - "'2147483648' is not a valid integer number"); + SetFlagResult::BAD_VALUE); + ASSERT_STR_CONTAINS( + output_msg, + "Invalid logging level '2147483648' for module 'biggerThanInt'. Only integer values between " + "-2147483648 and 2147483647 are allowed"); ASSERT_EQ(FLAGS_vmodule, expected_old); ASSERT_FALSE(VLOG_IS_ON(1)); @@ -218,4 +238,46 @@ TEST_F(FlagsTest, TestPreviewFlagsAllowList) { ASSERT_EQ(FLAGS_allowed_preview_flags_csv, ""); expected_old_allow_list = FLAGS_allowed_preview_flags_csv; } + +TEST_F(FlagsTest, ValidateFlagValue) { + StringVectorSink sink; + ScopedRegisterSink srs(&sink); + + // Test a valid value and make sure the flag is not changed. + ASSERT_OK(flags_internal::ValidateFlagValue("flagstest_testflag", "1")); + ASSERT_EQ(FLAGS_flagstest_testflag, 0); + + // Test an invalid value. + const auto kBadValueMessage = "Must be >= 0"; + ASSERT_NOK_STR_CONTAINS( + flags_internal::ValidateFlagValue("flagstest_testflag", "-1"), kBadValueMessage); + ASSERT_EQ(FLAGS_flagstest_testflag, 0); + + // We should have logged the error to the logs as well. + ASSERT_STR_CONTAINS(AsString(sink.logged_msgs()), kBadValueMessage); + + // Test validating a flag with tag sensitive_info. + const auto kSecretValue = "$secr3t#"; + ASSERT_OK(flags_internal::ValidateFlagValue("flagstest_secret_flag", kSecretValue)); + ASSERT_EQ(FLAGS_flagstest_secret_flag, ""); + + // Test an invalid value and make sure the status does not contain the secret value. + auto status = flags_internal::ValidateFlagValue("flagstest_secret_flag", kBadSecretValue); + ASSERT_NOK(status); + ASSERT_STR_NOT_CONTAINS(status.ToString(), kBadSecretValue); + ASSERT_EQ(FLAGS_flagstest_secret_flag, ""); + + // We should not have logged the secret values to the logs. + ASSERT_STR_NOT_CONTAINS(AsString(sink.logged_msgs()), kSecretValue); + ASSERT_STR_NOT_CONTAINS(AsString(sink.logged_msgs()), kBadSecretValue); + + // Test the vmodule flag. + const auto kVmoduleValue = "file=1"; + ASSERT_OK(SET_FLAG(vmodule, kVmoduleValue)); + ASSERT_EQ(FLAGS_vmodule, kVmoduleValue); + ASSERT_OK(flags_internal::ValidateFlagValue("vmodule", "files=1")); + ASSERT_NOK(flags_internal::ValidateFlagValue("vmodule", "files=")); + ASSERT_EQ(FLAGS_vmodule, kVmoduleValue); +} + } // namespace yb diff --git a/src/yb/util/flags/flags.cc b/src/yb/util/flags/flags.cc index 3226da4a43c1..4573c0564cf7 100644 --- a/src/yb/util/flags/flags.cc +++ b/src/yb/util/flags/flags.cc @@ -340,8 +340,6 @@ static string DescribeOneFlagInXML( return r; } -namespace { - struct sort_flags_by_name { inline bool operator()(const CommandLineFlagInfo& flag1, const CommandLineFlagInfo& flag2) { const auto& a = flag1.name; @@ -364,41 +362,19 @@ bool& CommandLineFlagsParsed() { return parsed; } -// Check if the flag can be set to the new value. Does not actually set the flag. -Status ValidateFlagValue(const std::string& flag_name, const std::string& value) { - auto flag_info = google::GetCommandLineFlagInfoOrDie(flag_name.c_str()); - - // Clear previous errors if any. - GetFlagValidatorSink().GetMessagesAndClear(); - - std::string error_msg; - if (google::ValidateCommandLineOption( - flag_name.c_str(), flag_info.current_value.c_str(), &error_msg)) { - return Status::OK(); - } - - auto validation_msgs = GetFlagValidatorSink().GetMessagesAndClear(); - - return STATUS_FORMAT( - InvalidArgument, "$0 : $1", error_msg, - validation_msgs.empty() ? "Bad value" : JoinStrings(validation_msgs, ";")); -} - Status ValidateFlagsRequiringDelayedValidation() { CommandLineFlagsParsed() = true; for (const auto& flag_name : FlagsWithDelayedValidation()) { auto flag_info = google::GetCommandLineFlagInfoOrDie(flag_name.c_str()); // Flag was already set without any validation. Check if the current value is valid. - RETURN_NOT_OK(ValidateFlagValue(flag_name, flag_info.current_value)); + RETURN_NOT_OK(flags_internal::ValidateFlagValue(flag_name, flag_info.current_value)); } FlagsWithDelayedValidation().clear(); return Status::OK(); } -} // namespace - void DumpFlagsXMLAndExit(OnlyDisplayDefaultFlagValue only_display_default_values) { vector flags; GetAllFlags(&flags); @@ -491,33 +467,35 @@ bool IsPreviewFlagAllowed(const CommandLineFlagInfo& flag_info, const string& ne } // Validates that the requested updates to vmodule can be made. -void ValidateVmodule() { - auto requested_settings = strings::Split(FLAGS_vmodule, ","); +bool ValidateVmodule(const char* flag_name, const std::string& value) { + auto requested_settings = strings::Split(value, ","); for (const auto& module_value : requested_settings) { if (module_value.empty()) { continue; } vector kv = strings::Split(module_value, "="); if (kv.size() != 2 || kv[0].empty() || kv[1].empty()) { - LOG(FATAL) << Format( - "'$0' is not valid. vmodule should be a comma list of =", - module_value); + LOG_FLAG_VALIDATION_ERROR(flag_name, value) + << "'" << module_value + << "' is not valid. vmodule should be a comma list of ="; + return false; } char* end; errno = 0; const int64 value = strtol(kv[1].c_str(), &end, 10); if (*end != '\0' || errno == ERANGE || value > INT_MAX || value < INT_MIN) { - LOG(FATAL) << Format( - "'$0' is not a valid integer number. Cannot update vmodule setting for module '$1'", - kv[1], kv[0]); + LOG_FLAG_VALIDATION_ERROR(flag_name, value) + << "Invalid logging level '" << kv[1] << "' for module '" << kv[0] + << "'. Only integer values between " << INT_MIN << " and " << INT_MAX << " are allowed"; + return false; } } -} -void ValidateAndUpdateVmodule() { - ValidateVmodule(); + return true; +} +void UpdateVmodule() { // glog behavior: The first time VLOG is invoked for a file it tries to find a matching pattern in // vmodule list. If found it links to that pattern for the rest of the program lifetime. If not // found it uses the default logging level from FLAGS_v and gets added to a list of files that @@ -580,9 +558,14 @@ void ValidateAndUpdateVmodule() { void ParseCommandLineFlags(int* argc, char*** argv, bool remove_flags) { static GoogleOnceType once_register_vmodule_callback = GOOGLE_ONCE_INIT; + // We cannot use DEFINE_validator and REGISTER_CALLBACK for vmodule since it is not DEFINED in any + // yb file, and we cannot guarantee the static initialization order. Instead we register them + // before we parse flags by which time static initialization is guaranteed to be complete. GoogleOnceInit(&once_register_vmodule_callback, []() { + CHECK(google::RegisterFlagValidator(&FLAGS_vmodule, &ValidateVmodule)); // NOLINT + flags_callback_internal::RegisterGlobalFlagUpdateCallback( - &FLAGS_vmodule, "ValidateAndUpdateVmodule", &ValidateAndUpdateVmodule); + &FLAGS_vmodule, "ValidateAndUpdateVmodule", &UpdateVmodule); }); { @@ -720,6 +703,55 @@ void WarnFlagDeprecated(const std::string& flagname, const std::string& date_mm_ } } +static const std::string kMaskedFlagValue = "***"; + +bool IsFlagSensitive(const unordered_set& tags) { + return ContainsKey(tags, FlagTag::kSensitive_info); +} + +bool IsFlagSensitive(const std::string& flag_name) { + unordered_set tags; + GetFlagTags(flag_name, &tags); + return IsFlagSensitive(tags); +} + +std::string GetMaskedValueIfSensitive( + const unordered_set& tags, const std::string& value) { + if (IsFlagSensitive(tags)) { + return kMaskedFlagValue; + } + return value; +} + +std::string GetMaskedValueIfSensitive(const std::string& flag_name, const std::string& value) { + unordered_set tags; + GetFlagTags(flag_name, &tags); + return GetMaskedValueIfSensitive(tags, value); +} + +Status ValidateFlagValue(const std::string& flag_name, const std::string& value) { + // Clear previous errors if any. + GetFlagValidatorSink().GetMessagesAndClear(); + + std::string error_msg; + if (google::ValidateCommandLineOption(flag_name.c_str(), value.c_str(), &error_msg)) { + return Status::OK(); + } + + auto validation_msgs = GetFlagValidatorSink().GetMessagesAndClear(); + + // error_msg originates from gflags, which may contain the value. Therefore, if it is sensitive, + // mask it. + // Ex: ERROR: failed validation of new value '1000' for flag 'ysql_conn_mgr_port' + if (!value.empty() && IsFlagSensitive(flag_name)) { + boost::replace_all(error_msg, Format("'$0'", value), Format("'$0'", kMaskedFlagValue)); + } + + return STATUS_FORMAT( + InvalidArgument, "$0 : $1", error_msg, + validation_msgs.empty() ? "Bad value" : JoinStrings(validation_msgs, ";")); +} + SetFlagResult SetFlag( const string& flag_name, const string& new_value, const SetFlagForce force, string* old_value, string* output_msg) { @@ -779,9 +811,9 @@ SetFlagResult SetFlag( // We have already validated the flag_name with GetCommandLineFlagInfo, so this should not fail. CHECK(google::GetCommandLineOption(flag_name.c_str(), &final_value)); - bool is_sensitive = ContainsKey(tags, FlagTag::kSensitive_info); - LOG(INFO) << "Changed flag '" << flag_name << "' from '" << (is_sensitive ? "***" : old_val) - << "' to '" << (is_sensitive ? "***" : final_value) << "'"; + LOG(INFO) << "Changed flag '" << flag_name << "' from '" + << GetMaskedValueIfSensitive(tags, old_val) << "' to '" + << GetMaskedValueIfSensitive(tags, final_value) << "'"; *output_msg = ret; *old_value = old_val;