You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/04/28 00:44:07 UTC

[kudu-CR] KUDU-1957 Clarify web UI redaction in --redact flag help 

Hao Hao has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6755

Change subject: KUDU-1957 Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957 Clarify web UI redaction in --redact flag help

Currently,row data in web UI will be redacted if --redact
flag is set to 'log'. It would be better to have a seperate
option to enable web UI redaction.

This adds a new option 'web' to specify web UI redaction via
--redact flag. And updates help description properly.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/client/batcher.cc
M src/kudu/client/session-internal.cc
M src/kudu/common/encoded_key.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/partition.cc
M src/kudu/common/row_changelist.cc
M src/kudu/common/types.cc
M src/kudu/consensus/log_util.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/delta_key.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/lock_manager.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/flags.cc
M src/kudu/util/hexdump.cc
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
31 files changed, 103 insertions(+), 88 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#8).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 52 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/8
-- 
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: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

> (19 comments)
 > 
 > I don't understand why we need per-context redaction macros if
 > redaction is now controlled on a per context rather than per data
 > basis. I would have expected more prolific use of ScopedDisableRedaction
 > and/or KUDU_DISABLE_REDACTION instead. For example, if web UI
 > redaction is disabled, I would expect a web UI thread to
 > temporarily create a ScopedDisabledRedaction when processing _any_
 > user request, which would temporarily disable all instances of
 > KUDU_REDACT in that thread.
 > 
 > Disabling redaction for glogs is a little trickier though, since
 > LOG messages are peppered everywhere. I'm curious to hear Todd or
 > Dan's take on this.

Yeah, I am not sure if there is a central place to disable redaction for LOG or we need to go into each redacted log message to check the flag?


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:48:33 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

> state it clear in help that 'log' also contains error messages.

SGTM


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:25:20 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:

(19 comments)

I don't understand why we need per-context redaction macros if redaction is now controlled on a per context rather than per data basis. I would have expected more prolific use of ScopedDisableRedaction and/or KUDU_DISABLE_REDACTION instead. For example, if web UI redaction is disabled, I would expect a web UI thread to temporarily create a ScopedDisabledRedaction when processing _any_ user request, which would temporarily disable all instances of KUDU_REDACT in that thread.

Disabling redaction for glogs is a little trickier though, since LOG messages are peppered everywhere. I'm curious to hear Todd or Dan's take on this.

http://gerrit.cloudera.org:8080/#/c/6755/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/6755/6//COMMIT_MSG@15
PS6, Line 15: controls
control


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc
File src/kudu/common/encoded_key.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc@190
PS6, Line 190: string EncodedKey::RangeToString(const EncodedKey* lower, const EncodedKey* upper) {
There's no guarantee that RangeToString has to be called only from the web UI. Maybe rename it RangeToStringForWebUI to make that more clear?


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc
File src/kudu/server/default-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc@129
PS6, Line 129: 'web'
or 'all' (generalize)


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h
File src/kudu/tablet/rowset_info.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h@35
PS6, Line 35:   enum class RedactMode {
Can't you reuse RedactMode from flags.h?


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h
File src/kudu/util/flag_tags.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85
PS6, Line 85: in web UI
in the web UI


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85
PS6, Line 85: the log message
glog messages


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h
File src/kudu/util/flags.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@60
PS6, Line 60: enum class RedactMode {
I think RedactContext is a little more clear.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@65
PS6, Line 65: is set with 'web'
Or 'all', so just generalize this.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@133
PS6, Line 133: from
from the


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@148
PS6, Line 148: ''
What is this referring to?


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@528
PS6, Line 528:     ret_value += CheckFlagAndRedact(f, escape_mode, RedactMode::WEB);
See the comment I left in GetNonDefaultFlags about passing through the redaction context).


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@553
PS6, Line 553: 'log'
Or 'all', so I would just say 'if --redact indicates that log messages should be redacted'.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@554
PS6, Line 554:         string flag_value = CheckFlagAndRedact(flag, EscapeMode::NONE, RedactMode::LOG);
It feels a little strange to use RedactMode::LOG when there's no requirement that callers use this function's output in a log message. I understand that currently all callers do pass the output into LOG(INFO), but maybe it'd be cleaner if RedactMode were also an argument to the function, and if RedactMode had a third option for NONE? Then the caller, knowing how the output will be used, can set the appropriate redaction context.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h
File src/kudu/util/logging.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@61
PS6, Line 61: for log redaction
from glogs


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@66
PS6, Line 66: for web UI redaction
from the web UI


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@70
PS6, Line 70: log redaction
for glogs


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@74
PS6, Line 74: for web UI redaction
for the web UI


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@78
PS6, Line 78: For web UI redaction
How about "Like KUDU_REDACT_WEBUI and with..."


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc@87
PS6, Line 87:     {"redact", "web"},
Shouldn't this be log?



-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:21:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

> > Yah, this is basically what I'm thinking.  It seems highly
 > suspicious to return an error containing user data across the wire.
 >  DDL ops are a special case; we don't consider table schemas or
 > even partition schemas to be sensitive.  Given that, I'm personally
 > fine ignoring the edge-case of error messages in the flag name.
 > 
 > Alright, I can live with this.
 > 
 > Hao, you may want to solicit Todd's opinion too so there's even
 > less chance of someone asking for a rework of your patch again (the
 > way I did, sorry about that!).

No problem at all! Thanks for giving all the suggestions.

@Dan, I am not quite sure, but look at partition.cc, we are redacting key value in the return error messages. Will that eventually be part of the error messages displayed?


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:16:47 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#5).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'web', 'log',
'all', and 'none'. If 'web' is specified, all sensitive data types
(configuration flags and row data) will be redacted from web UI. While
'log' is specified, all sensitive data will be redacted from log and
error messages. And the option 'flag' is removed from --redact flag to
let the flag only controls redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/client/batcher.cc
M src/kudu/client/session-internal.cc
M src/kudu/common/encoded_key.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/partition.cc
M src/kudu/common/row_changelist.cc
M src/kudu/common/types.cc
M src/kudu/consensus/log_util.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/delta_key.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/lock_manager.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/hexdump.cc
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/test_util.cc
39 files changed, 164 insertions(+), 132 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/5
-- 
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: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 9: Code-Review+2


-- 
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: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 25 Oct 2017 19:09:34 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc@382
PS8, Line 382: 
> Nit: revert this empty line addition.
Done



-- 
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: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Oct 2017 00:10:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

> Yeah, I think you're right.  I'd suggest we not redact that, but to
 > be honest it's probably not worth changing.  That error can only
 > happen in corruption-type scenarios, or a client bug, so it doesn't
 > seem like making a special case for.

Makes sense to me, I also prefer to keep it. Thanks!


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:26:35 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc
File src/kudu/common/encoded_key.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/common/encoded_key.cc@190
PS6, Line 190: string EncodedKey::RangeToString(const EncodedKey* lower, const EncodedKey* upper) {
> There's no guarantee that RangeToString has to be called only from the web 
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc
File src/kudu/server/default-path-handlers.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/server/default-path-handlers.cc@129
PS6, Line 129: 
> or 'all' (generalize)
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h
File src/kudu/tablet/rowset_info.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/tablet/rowset_info.h@35
PS6, Line 35: 
> Can't you reuse RedactMode from flags.h?
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h
File src/kudu/util/flag_tags.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85
PS6, Line 85: l', or only in
> glog messages
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flag_tags.h@85
PS6, Line 85: in the we
> in the web UI
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h
File src/kudu/util/flags.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@60
PS6, Line 60: // Stick the flags into a string. If --redact indicates that redaction
> I think RedactContext is a little more clear.
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.h@65
PS6, Line 65: 
> Or 'all', so just generalize this.
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@133
PS6, Line 133: "
> from the
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@148
PS6, Line 148: ''
> What is this referring to?
I think it refers to empty setting.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@528
PS6, Line 528:   ostringstream args;
> See the comment I left in GetNonDefaultFlags about passing through the reda
Removed.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@553
PS6, Line 553: 
> Or 'all', so I would just say 'if --redact indicates that log messages shou
Done


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/flags.cc@554
PS6, Line 554: 
> It feels a little strange to use RedactMode::LOG when there's no requiremen
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h
File src/kudu/util/logging.h:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@61
PS6, Line 61: Most callers shou
> from glogs
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@66
PS6, Line 66: redaction is enabled
> from the web UI
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@70
PS6, Line 70: ion that reda
> for glogs
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@74
PS6, Line 74: 
> for the web UI
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/logging.h@78
PS6, Line 78: 
> How about "Like KUDU_REDACT_WEBUI and with..."
Removed the change.


http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc
File src/kudu/util/test_util.cc:

http://gerrit.cloudera.org:8080/#/c/6755/6/src/kudu/util/test_util.cc@87
PS6, Line 87:     {"redact", "none"},
> Shouldn't this be log?
As comment above, we need to disable log redaction here for the test cases that issue checks without redaction to pass.



-- 
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Oct 2017 07:04:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

Yeah, I think you're right.  I'd suggest we not redact that, but to be honest it's probably not worth changing.  That error can only happen in corruption-type scenarios, or a client bug, so it doesn't seem like making a special case for.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:23:38 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has removed Jean-Daniel Cryans from this change.  ( http://gerrit.cloudera.org:8080/6755 )

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................


Removed reviewer Jean-Daniel Cryans.
-- 
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: deleteReviewer
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:

> Proposed this on slack, but would like to do it again here. Given 1) it is hard to tell which data will be displayed on web UI or logged, 2) it is straightforward to disable webUI redaction by using ScopedDisableRedaction 3) it is not straightforward to disable redaction in LOG and error messages. Can we redefine the options of --redact to  'all', 'log', and 'none'? So that 'all' includes web UI and log redaction, 'log' includes log redaction only, and 'none' turn off all redactions.

I think this is a good approach, but 'log' doesn't precisely identify the redaction context it affects: it's not just glog messages, but also error messages.

Since it's tough to find a word that precisely captures both "glog and error messages", perhaps we should invert it? That is, change --redact to --disable-redaction with possible values 'none', 'webui', and 'all'? The default value would be 'none'.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 21:23:18 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

The low-level printing routines in places like types.cc can be called in the context of the web UI or logging, so I don't think it's correct to just label it with one or the other.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:58:17 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has uploaded a new patch set (#2).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

Currently,row data in web UI will be redacted if --redact
flag is set to 'log'. It would be better to have a seperate
option to enable web UI redaction.

This adds a new option 'web' to specify web UI redaction via
--redact flag. And updates help description properly.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/client/batcher.cc
M src/kudu/client/session-internal.cc
M src/kudu/common/encoded_key.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/partition.cc
M src/kudu/common/row_changelist.cc
M src/kudu/common/types.cc
M src/kudu/consensus/log_util.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/delta_key.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/lock_manager.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/flags.cc
M src/kudu/util/hexdump.cc
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
31 files changed, 114 insertions(+), 94 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

> > > Yah, this is basically what I'm thinking.  It seems highly
 > > suspicious to return an error containing user data across the
 > wire.
 > >  DDL ops are a special case; we don't consider table schemas or
 > > even partition schemas to be sensitive.  Given that, I'm
 > personally
 > > fine ignoring the edge-case of error messages in the flag name.
 > >
 > > Alright, I can live with this.
 > >
 > > Hao, you may want to solicit Todd's opinion too so there's even
 > > less chance of someone asking for a rework of your patch again
 > (the
 > > way I did, sorry about that!).
 > 
 > No problem at all! Thanks for giving all the suggestions.
 > 
 > @Dan, I am not quite sure, but look at partition.cc, we are
 > redacting key value in the return error messages. Will that
 > eventually be part of the error messages displayed?

Though I want to state that I am good with not reverting the definition of --redact and just state it clear in help that 'log' also contains error messages.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:23:27 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:

> Do we have any concrete examples of error messages which are redacted?  I'm not aware of any, which may be why I'm fine downplaying error messages.

There are a bunch in this patch; look at block_compression.cc and cfile_reader.cc for example.

If you're referring specifically to error messages _sent to the client_, I'm not sure. I'd bet that most redacted error messages terminate in glogs, but there's programmatic guarantee here. Nor is there a guarantee that future code changes won't send a previously-glogged-only message to the client. I'm especially nervous about the stuff in common/ which I know is used on DDL paths.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 00:58:31 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

The failed test FlexPartitioningITest seems to be flaky and should not relate to this change.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 17 Oct 2017 18:45:10 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Oct 2017 19:22:55 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#7).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 47 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/7
-- 
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: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

I think it's confusing to invert the flag.  I'd keep it at '--redact=log' and maybe add a note in the help text about errors. 

Do we have any concrete examples of error messages which are redacted?  I'm not aware of any, which may be why I'm fine downplaying error messages.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 00:27:51 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

> state it clear in help that 'log' also contains error messages.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:25:09 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 8: Code-Review+1

(1 comment)

Would like Dan or Todd to also review.

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/8/src/kudu/server/webserver.cc@382
PS8, Line 382: 
Nit: revert this empty line addition.



-- 
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: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:53:10 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#3).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

Currently,row data in web UI will be redacted if --redact
flag is set to 'log'. It would be better to have a seperate
option to enable web UI redaction.

This adds a new option 'web' to specify web UI redaction via
--redact flag. And updates help description properly.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/client/batcher.cc
M src/kudu/client/session-internal.cc
M src/kudu/common/encoded_key.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/partition.cc
M src/kudu/common/row_changelist.cc
M src/kudu/common/types.cc
M src/kudu/consensus/log_util.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/delta_key.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/lock_manager.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/flags.cc
M src/kudu/util/hexdump.cc
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
31 files changed, 113 insertions(+), 93 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#9).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 51 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/9
-- 
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: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

> > Proposed this on slack, but would like to do it again here. Given
 > 1) it is hard to tell which data will be displayed on web UI or
 > logged, 2) it is straightforward to disable webUI redaction by
 > using ScopedDisableRedaction 3) it is not straightforward to
 > disable redaction in LOG and error messages. Can we redefine the
 > options of --redact to  'all', 'log', and 'none'? So that 'all'
 > includes web UI and log redaction, 'log' includes log redaction
 > only, and 'none' turn off all redactions.
 > 
 > I think this is a good approach, but 'log' doesn't precisely
 > identify the redaction context it affects: it's not just glog
 > messages, but also error messages.
 > 
 > Since it's tough to find a word that precisely captures both "glog
 > and error messages", perhaps we should invert it? That is, change
 > --redact to --disable-redaction with possible values 'none',
 > 'webui', and 'all'? The default value would be 'none'.

+1 on this suggestion.  If there is no objections here, then I will go ahead with it and update the patch.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 23:54:54 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 6:

Proposed this on slack, but would like to do it again here. Given 1) it is hard to tell which data will be displayed on web UI or logged, 2) it is straightforward to disable webUI redaction by using ScopedDisableRedaction 3) it is not straightforward to disable redaction in LOG and error messages. Can we redefine the options of --redact to  'all', 'log', and 'none'? So that 'all' includes web UI and log redaction, 'log' includes log redaction only, and 'none' turn off all redactions.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 18 Oct 2017 05:52:24 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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 6:

> If you're referring specifically to error messages _sent to the client_, I'm not sure.

Yah, this is basically what I'm thinking.  It seems highly suspicious to return an error containing user data across the wire.  DDL ops are a special case; we don't consider table schemas or even partition schemas to be sensitive.  Given that, I'm personally fine ignoring the edge-case of error messages in the flag name.


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:02:06 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6755/2/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

Line 116: DEFINE_string(redact, "all",
Dan, David, myself, and Hao talked about this in person, and I wanted to summarize the discussion for the benefit of others.

I raised the concern that with the addition of 'flag' --redact seems to be controlling two separate axes: context (i.e. "Should I redact in the web UI? Should I redact while logging?") and data type (i.e. "Should I redact sensitive gflag values? Should I redact user data?"). I found this comingling to be confusing and suggested that we either:
1. Split this gflag into two, one for controlling redaction context and the other for redaction data types.
2. Choose a hardcoded policy for one of the two axes and restrict --redact to controlling the other axis.

In our discussion, we reached a rough consensus that redaction context is far more interesting than redaction data types. Thus, we think option #2 is preferable, and that redaction data types should always be "all" while redaction context should be controllable.

As far as what this means for this patch, Hao will:
1. Retain the addition of 'web'.
2. Remove the 'flag' option.
3. Wherever 'flag' was used to decide whether to redact sensitive gflag values, instead consider just the context (are we logging? am I in the web UI? etc).


-- 
To view, visit http://gerrit.cloudera.org:8080/6755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao 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 8:

(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:     shared_lock<RWMutex> l(lock_);
             :     PathHandlerMap::const_iterator it = path_handlers
> This seems a little strange; how about just heap allocating the ScopedDisab
Moved the disable logic to L468 as you suggested.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@468
PS7, Line 468:     ScopedDisableRedaction s;
> Would it be sufficient to disable redaction around L468?
Done


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
            : //         if redaction is enabled.
            : //
> Let's be a little bit vague about this, to allow the redaction implementati
Done


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 redaction is enabled.
> Update.
Done


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:         args << "--" << flag.name << '=' << flag_value;
             :       }
> Seems like this call-site ought to check g_should_redact_log and the one in
On a second thought, I am not sure if it would be better to leave it more vague.  As now there is not much differentiation in GetNonDefaultFlags() and CommandlineFlagsIntoString(). The enabling/disabling of web UI redaction logic is mainly controlled by ScopedDisableRedaction.


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 == kudu::RedactContext::ALL ||    \
> Shouldn't this check g_should_redact_log || g_should_redact_web?
Updated.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@91
PS7, Line 91: enum class RedactContext {
> It might be clearer if the tri-state --redact values were mapped to a tri-s
Updated to tri-state.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@93
PS7, Line 93: 
> Nit: don't need the
Done



-- 
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: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:39:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/6755

to look at the new patch set (#6).

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'web', 'log',
'all', and 'none'. If 'web' is specified, all sensitive data types
(configuration flags and row data) will be redacted from web UI. While
'log' is specified, all sensitive data will be redacted from log and
error messages. And the option 'flag' is removed from --redact flag to
let the flag only controls redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/client/batcher.cc
M src/kudu/client/session-internal.cc
M src/kudu/common/encoded_key.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/partition.cc
M src/kudu/common/row_changelist.cc
M src/kudu/common/types.cc
M src/kudu/consensus/log_util.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/server/default-path-handlers.cc
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/concurrent_btree.h
M src/kudu/tablet/delta_key.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/lock_manager.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/util/compression/compression_codec.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/hexdump.cc
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/logging-test.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/pb_util.cc
M src/kudu/util/slice.cc
M src/kudu/util/test_util.cc
40 files changed, 165 insertions(+), 132 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/6
-- 
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: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/6755 )

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
......................................................................

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Reviewed-on: http://gerrit.cloudera.org:8080/6755
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 51 insertions(+), 41 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

-- 
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: merged
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 6:

> Yah, this is basically what I'm thinking.  It seems highly suspicious to return an error containing user data across the wire.  DDL ops are a special case; we don't consider table schemas or even partition schemas to be sensitive.  Given that, I'm personally fine ignoring the edge-case of error messages in the flag name.

Alright, I can live with this.

Hao, you may want to solicit Todd's opinion too so there's even less chance of someone asking for a rework of your patch again (the way I did, sorry about that!).


-- 
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: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 19 Oct 2017 01:07:58 +0000
Gerrit-HasComments: No