[BACKPORT pg15-cherrypicks][#22950] DocDB: Add ValidateFlagValue RPC

Summary:
Original commit: 507d287 / D36157
Added `ValidateFlagValue` RPC that will check if the given flag value is valid for use, without setting it.

Make sure we do not log flag values that are sensitive, or send them back in Status messages.

Added `yb-ts-cli validate_flag_value <flag> <value>` command to validate the flag value using the cli tool.

Also fixed the `vmodule` flag validator.

**Upgrade/Rollback safety:**
Proto changes are new RPC which is only used by yb-ts-cli which is always run as the same yb server version, and YBA which will check db version numbers.

Fixes #22950
Jira: DB-11865

Test Plan:
FlagsTest.ValidateFlagValue

```
$ ./build/latest/bin/yb-ts-cli -server_address 127.0.0.1:9100 validate_flag_value v 3
Flag value is valid
```

```
$ ./build/latest/bin/yb-ts-cli -server_address 127.0.0.1:9100 validate_flag_value na_flag na_value
Invalid flag value: Remote error (yb/rpc/outbound_call.cc:561): Invalid argument (yb/util/flags/flags.cc:746): Unknown command line flag 'na_flag' : Bad value (rpc error 1)
```

```
$ ./build/latest/bin/yb-ts-cli -server_address 127.0.0.1:9100 validate_flag_value v hari
Invalid flag value: Remote error (yb/rpc/outbound_call.cc:561): Invalid argument (yb/util/flags/flags.cc:746): ERROR: illegal value 'hari' specified for int32 flag 'v'
 : Bad value (rpc error 1)
```

```
$ ./build/latest/bin/yb-ts-cli -server_address 127.0.0.1:9100 validate_flag_value rpc_throttle_threshold_bytes 1
Invalid flag value: Remote error (yb/rpc/outbound_call.cc:561): Invalid argument (yb/util/flags/flags.cc:746): ERROR: failed validation of new value '1' for flag 'rpc_throttle_threshold_bytes'
 : common_flags.cc:155] Invalid value '1' for flag 'rpc_throttle_threshold_bytes': Must be at least 16 (rpc error 1)
```

```
$ ./build/latest/bin/yb-ts-cli -server_address 127.0.0.1:9100 validate_flag_value vmodule asd-
Invalid flag value: Remote error (yb/rpc/outbound_call.cc:561): Invalid argument (yb/util/flags/flags.cc:753): ERROR: failed validation of new value 'asd-' for flag 'vmodule'
 : flags.cc:478] Invalid value 'asd-' for flag 'vmodule': 'asd-' is not valid. vmodule should be a comma list of <module_pattern>=<logging_level> (rpc error 1)
```

Reviewers: jason, tfoucher

Reviewed By: jason

Subscribers: ybase

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36223

src/yb/server/generic_service.cc

@@ -31,32 +31,15 @@
 //
 #include "yb/server/generic_service.h"
 
-#include <ctype.h>
-#include <functional>
-#include <map>
-#include <mutex>
-#include <set>
 #include <string>
-#include <unordered_map>
-#include <unordered_set>
 
 #include <boost/preprocessor/cat.hpp>
 #include <boost/preprocessor/stringize.hpp>
-#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) {

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,

src/yb/server/server_base.proto

@@ -155,13 +155,25 @@ 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);
 
   rpc GetFlag(GetFlagRequestPB)
     returns (GetFlagResponsePB);
 
+  rpc ValidateFlagValue(ValidateFlagValueRequestPB)
+      returns (ValidateFlagValueResponsePB);
+
   rpc RefreshFlags(RefreshFlagsRequestPB)
     returns (RefreshFlagsResponsePB);
 

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] <flag> <value>\n"
+      << "  " << kValidateFlagValueOp << " <flag> <value>\n"
       << "  " << kRefreshFlagsOp << "\n"
       << "  " << kTabletStateOp << " <tablet_id>\n"
       << "  " << kDumpTabletOp << " <tablet_id>\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);
 

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, <flag_type> 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 <typename T>
+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);

src/yb/util/flags/flags-test.cc

@@ -14,8 +14,9 @@
 #include <filesystem>
 
 #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