You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/10/20 01:01:44 UTC

[kudu-CR] rpc: improve error messages and logging for bad authentication 

Hello Dan Burkert, Alexey Serbin,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................

rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.
* Improve the error messages for GSSAPI failures to only include the
  interesting bit.
* Don't log all authentication failures at ERROR level, since they're
  already passed back to the caller via a Status.

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 35 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_client.h
File src/kudu/rpc/sasl_client.h:

PS4, Line 135: Incopmlete
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS4, Line 34: #include "kudu/util/scoped_cleanup.h"
> nit: not sure whether this header is necessary here.
Done


PS4, Line 198: re
> What about calling regfree()?  Is LSAN happy without that?
yea, LSAN appears to not be complaining. If it does at some point later we can add a LeakCheckDisabler, but I think it's happy because there is static storage pointing at the allocated object


PS4, Line 249: Status::NotAuthorized
> Does it make sense to return something else here instead?  I.e., there migh
I broke out the list of expected "authorization" failures here, but unfortunately missing krb5 credentials shows up as SASL_FAIL, which is pretty generic. But at least now something like memory errors will show up as RuntimeError.


http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_server.cc
File src/kudu/rpc/sasl_server.cc:

PS4, Line 450: SaslMessagePB msg
> That might be not relevant to the scope of this patch, but it seems this va
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_client.cc
File src/kudu/rpc/sasl_client.cc:

PS2, Line 382: NotAuthorized
Do we want to distinguish between 'not authorized' and 'not authenticated'?


http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS2, Line 214:   regex_t re;
             :   CHECK_EQ(0, regcomp(&re,
             :                       "generic failure: GSSAPI Error: "
             :                       "Unspecified GSS failure. +"
             :                       "Minor code may provide more information +"
             :                       "\\((.+)\\)", REG_EXTENDED));
             :   auto c = MakeScopedCleanup([&]() { regfree(&re); });
Is it possible to make it only once and store the result for future invocations of this method?  E.g., using GoogleOnceInit()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 6: Code-Review+2

Just rebased

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

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

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

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................

rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.

* Improve the error messages for GSSAPI failures to only include the
  interesting bit.

* Don't log all authentication failures at ERROR level. Instead, capture
  the error log message and pass it back to the caller of the SASL API
  through a thread-local hack.

  Both clients and servers have provisions to log failed negotiations
  elsewhere, so the log message coming from this context is redundant;
  however, the error text that is passed here is more descriptive than
  the error returned by sasl_errstring().

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
7 files changed, 186 insertions(+), 102 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/4764/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4764
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_client.h
File src/kudu/rpc/sasl_client.h:

PS4, Line 135: Incopmlete
nit: typo


http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS4, Line 34: #include "kudu/util/scoped_cleanup.h"
nit: not sure whether this header is necessary here.


PS4, Line 198: re
What about calling regfree()?  Is LSAN happy without that?


PS4, Line 249: Status::NotAuthorized
Does it make sense to return something else here instead?  I.e., there might be SASL_FAILURE or SASL_BADPARAM errors, but why do we want to always map them into NotAuthorized()?  Or it's not possible in this context?

I think it's worth consider mapping the rest of error codes into something else at least to distinguish from the previous switch case (i.e., something like RunTimeError() or alike).


http://gerrit.cloudera.org:8080/#/c/4764/4/src/kudu/rpc/sasl_server.cc
File src/kudu/rpc/sasl_server.cc:

PS4, Line 450: SaslMessagePB msg
That might be not relevant to the scope of this patch, but it seems this variable is not used.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_client.cc
File src/kudu/rpc/sasl_client.cc:

PS2, Line 382: NotAuthorized
> Do we want to distinguish between 'not authorized' and 'not authenticated'?
Unfortunately adding new codes to Status is difficult because that would add new enums to the wire protocol. So, we more or less have to keep the ones we've got.


http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS2, Line 214:   regex_t re;
             :   CHECK_EQ(0, regcomp(&re,
             :                       "generic failure: GSSAPI Error: "
             :                       "Unspecified GSS failure. +"
             :                       "Minor code may provide more information +"
             :                       "\\((.+)\\)", REG_EXTENDED));
             :   auto c = MakeScopedCleanup([&]() { regfree(&re); });
> Is it possible to make it only once and store the result for future invocat
will give it a try and see if it doesn't make things more messy. I figured this is a rare enough path that it wasn't a perf concern, but if it doesn't add too many LOC I'll do it


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................

rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.

* Improve the error messages for GSSAPI failures to only include the
  interesting bit.

* Don't log all authentication failures at ERROR level. Instead, capture
  the error log message and pass it back to the caller of the SASL API
  through a thread-local hack.

  Both clients and servers have provisions to log failed negotiations
  elsewhere, so the log message coming from this context is redundant;
  however, the error text that is passed here is more descriptive than
  the error returned by sasl_errstring().

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
7 files changed, 190 insertions(+), 103 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/4764/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4764
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4764/2//COMMIT_MSG
Commit Message:

PS2, Line 13: caller
is the caller here the client or server or both?


http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

Line 215:   CHECK_EQ(0, regcomp(&re,
I'm a little worried about CHECK here.  The server could get send a message that happens to fail with an uncommon SASL error, and crash the server.  Could this early return the whole err on failure instead?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4764/3/src/kudu/rpc/negotiation.cc
File src/kudu/rpc/negotiation.cc:

PS3, Line 246: (s.IsNotAuthorized())
nit: unneeded extra braces.

Besides, I think it could be more readable if written as:

bool is_bad = !s.ok() && !(
      (s.IsNetworkError() && s.posix_code() == ECONNREFUSED) ||
      s.IsNotAuthorized());

Or:

bool is_good = s.ok() || s.IsNotAuthorized() ||
  (s.IsNetworkError() && s.posix_code() == ECONNREFUSED);


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 3:

fairly substantial changes in the new revision, based on my experiencing some more unintelligible messages

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: improve error messages and logging for bad authentication

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.

* Improve the error messages for GSSAPI failures to only include the
  interesting bit.

* Don't log all authentication failures at ERROR level. Instead, capture
  the error log message and pass it back to the caller of the SASL API
  through a thread-local hack.

  Both clients and servers have provisions to log failed negotiations
  elsewhere, so the log message coming from this context is redundant;
  however, the error text that is passed here is more descriptive than
  the error returned by sasl_errstring().

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Reviewed-on: http://gerrit.cloudera.org:8080/4764
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
M src/kudu/rpc/sasl_server.cc
7 files changed, 190 insertions(+), 103 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

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

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

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................

rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.
* Improve the error messages for GSSAPI failures to only include the
  interesting bit.
* Don't log all authentication failures at ERROR level, since they're
  already passed back to the caller of Negotiate() via a Status.
  Both clients and servers have provisions to log failed negotiations
  elsewhere, so the log message coming from SASL is redundant.

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 41 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4764/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

Line 215:   CHECK_EQ(0, regcomp(&re,
> I'm a little worried about CHECK here.  The server could get send a message
but this is the compilation of the constant regex, not the _matching_ of the regex. So, it would only fail with a regex with invalid syntax, or OOM or something.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

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

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

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................

rpc: improve error messages and logging for bad authentication

* Don't dump the trace for a failed authentication exchange.
* Improve the error messages for GSSAPI failures to only include the
  interesting bit.
* Don't log all authentication failures at ERROR level, since they're
  already passed back to the caller via a Status.

Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
---
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/sasl_client.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 37 insertions(+), 15 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] rpc: improve error messages and logging for bad authentication

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

Change subject: rpc: improve error messages and logging for bad authentication
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3a5156b09fa4f8c7591f4e399ce8cc450c089e88
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No