Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/6755 )
Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help ...................................................................... Patch Set 7: (8 comments) http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@385 PS7, Line 385: ScopedDisableRedaction s; : if (kudu::g_should_redact_web) s.EnableRedaction(); This seems a little strange; how about just heap allocating the ScopedDisableRedaction if necessary and storing it in a unique_ptr? It's an extra allocation per web request, but it's very small and is allocated/deallocated by the same thread, so it should be fairly cheap. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@468 PS7, Line 468: handler.callback()(req, &resp); Would it be sufficient to disable redaction around L468? http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h File src/kudu/util/flag_tags.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h@84 PS7, Line 84: The values of these flags are considered sensitive and will be redacted : // in the web UI and glog messages if --redact is set with 'all', or only in : // glog messages if --redact is set with 'log'. Let's be a little bit vague about this, to allow the redaction implementation to evolve: "The values of these flags are considered sensitive and will be redacted if redaction is enabled". http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h File src/kudu/util/flags.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h@68 PS7, Line 68: // Get all the flags different from their defaults. The output is a nicely : // formatted string with --flag=value pairs per line. Redact any flags that : // are tagged as sensitive, if --redact is set with 'flag'. Update. http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc File src/kudu/util/flags.cc: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc@545 PS7, Line 545: // Redact the flags tagged as sensitive, if --redact indicates : // that log messages should be redacted. Seems like this call-site ought to check g_should_redact_log and the one in CommandlineFlagsIntoString should check g_should_redact_web. What's confusing about all of this is that there's no guarantee that GetNonDefaultFlags is only used for logging or error messages, and that CommandlineFlagsIntoString is only used by the web UI. Maybe those functions should be renamed to indicate that purpose, so that if someone tries to repurpose them, they'll see that the redaction behavior should also be adjusted? http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@63 PS7, Line 63: #define KUDU_SHOULD_REDACT() (kudu::g_should_redact_log && kudu::tls_redact_user_data) Shouldn't this check g_should_redact_log || g_should_redact_web? http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@91 PS7, Line 91: extern bool g_should_redact_log; It might be clearer if the tri-state --redact values were mapped to a tri-state enum instead of two bools. With two bools there's one state that doesn't make sense (g_should_redact_log=false && g_should_redact_web=true). http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@93 PS7, Line 93: the Nit: don't need the -- To view, visit http://gerrit.cloudera.org:8080/6755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f Gerrit-Change-Number: 6755 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 20 Oct 2017 19:22:55 +0000 Gerrit-HasComments: Yes
