Adar Dembo has posted comments on this change. Change subject: KUDU-1844: /varz should not expose potentially sensitive configs ......................................................................
Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/6043/1//COMMIT_MSG Commit Message: PS1, Line 12: redact Nit: "<redacted>" right? Not "<redact>"? http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/server/default-path-handlers.cc File src/kudu/server/default-path-handlers.cc: PS1, Line 115: If --redact_sensitive_flags is true for a flag with sensitive tag Nit: the way this is worded, it sounds like --redact_sensitive_flags is something that would be true/false on a per-flag basis instead of globally. How about reword as "If --redact_sensitive_flags is true, the values of flags tagged as sensitive will be redacted." http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flag_tags-test.cc File src/kudu/util/flag_tags-test.cc: Line 27: #include "logging.h" Should be kudu/util/logging.h PS1, Line 125: ASSERT_STR_CONTAINS Can we be more specific and assert that the output contains "--test_sensitive_flag=<redacted>" rather than just kRedactionMessage anywhere in the output? http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flag_tags.h File src/kudu/util/flag_tags.h: PS1, Line 84: These flags are considering sensitive and their value would be redacted if : // --redact_sensitive_flags is enabled. Nit: how about "The values of these flags are considered sensitive and will be redacted if --redact_sensitive_flags is true."? http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flags.cc File src/kudu/util/flags.cc: Line 40: #include "kudu/util/logging.h" Keep this list sorted alphabetically. Line 111: DEFINE_bool(redact_sensitive_flags, false, Shouldn't this default to true? That's what we did for --log_redact_user_data. Speaking of, I think we should find a way to work this into that existing flag. What do you think about combining them into a single flag? Something like "--redact", which could take a comma-separated list of values, like "flags", "data", etc. Line 115: TAG_FLAG(redact_sensitive_flags, stable); I would be hesitant to introduce any new flag as "stable". Let's use "evolving" for the time being. Line 318: // Stick the flags into a string. During the process, check that Apart from the redaction, does this produce the exact same output as google::CommandlineFlagsIntoString()? PS1, Line 322: const string& tag, What's the value of allowing this to be customized if it's only ever called with 'sensitive'? Line 440: return CheckFlagsAndRedact("sensitive", FLAGS_redact_sensitive_flags); Why not embed CheckFlagsAndRedact() into this function? Since it's only being called here, I don't really see the point of splitting them up like this. http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flags.h File src/kudu/util/flags.h: PS1, Line 53: If any flags tagged with : // 'sensitive' and --redact_sensitive_flags is enabled, : // redact the flag How about "If --redact_sensitive_flags is true, the values of flags tagged as sensitive will be redacted. Otherwise, the values will be written to the string as-is." http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/logging.h File src/kudu/util/logging.h: PS1, Line 284: is set to true, the flags with sensitive tag, would be logged with redacted value. "...is set to true, the values of flags tagged as sensitive will be redacted." -- To view, visit http://gerrit.cloudera.org:8080/6043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7661e0409c79559f22b07e7c12737e804544186b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
