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/11/17 06:19:26 UTC
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5120
to review the following change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll return an error on first usage of SASL. Similarly, if
they disable initialization but didn't do a proper job of
initialization, we detect it and return an error on usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 179 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5120/1
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:
PS3, Line 132: SASL
> Only if the client is also using the Cyrus SASL library, right? In theory t
Yes in theory. In practice the overlap between people using custom sasl libraries and kudu is probably non-existent.
Would you like me to change SASL to Cyrus SASL everywhere it's used, or something else to make it clearer?
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/1//COMMIT_MSG
Commit Message:
PS1, Line 29: - we try to auto-detect the case in which the client has fully
: initialized SASL including providing a mutex implementation. In that
: case, we skip initialization but log a warning that the caller should
: disable it explicitly.
: - if we detect that it's initialized but no mutex implementation is
: provided, we'll return an error on first usage of SASL. Similarly, if
: they disable initialization but didn't do a proper job of
: initialization, we detect it and return an error on usage.
> what about also offering a way to explicitly enable as well, then you also
I started writing the code for this, and when I got to documenting the API I couldn't really explain why you'd want to force initialization on even if we detect that it's already initialized. If we run into such a problem I think I'd rather have the user hit an issue and report it and get it fixed rather than work around it (and us continue to ship flaky auto-detection code)
Do you feel strongly?
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 6: Code-Review+2
I think that given the high priority of this patch those comments I left on a couple of nits are not crucial.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(2 comments)
Mostly just wanted to look at the API changes.
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:
PS3, Line 132: SASL
Only if the client is also using the Cyrus SASL library, right? In theory there are other SASL implementations for which there'd be no overlap at all?
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS3, Line 511: executable_path,
: "test",
: filter_flag,
: "--is_test_child"},
Nit: indentation is strangely unaligned here.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5120
to look at the new patch set (#5).
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
own, we'll return an error on first usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 200 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5120/5
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 2:
(2 comments)
Here's a patch to make this work with OS X: https://gist.github.com/anonymous/bc2d7fe7769cd50239ce1a27bec7d497
http://gerrit.cloudera.org:8080/#/c/5120/2//COMMIT_MSG
Commit Message:
PS2, Line 19: Impala has initialized the SASL library with no
: call to sasl_set_mutex(), and then during Kudu initialization we set the
: mutex functions to our own.
Has Impala been fixed? If not, this patch still won't work correctly since it will fail when the mutex check is done.
http://gerrit.cloudera.org:8080/#/c/5120/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
Line 203: LOG(WARNING) << "SASL was initialized prior to Kudu's initialization. Skipping "
I think I would be in favor of making this a hard error.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:
PS3, Line 132: SASL
> Yes in theory. In practice the overlap between people using custom sasl li
I think changing just this one instance would be enough to precisely indicate the distinction.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 1:
(1 comment)
Thanks, Todd!
http://gerrit.cloudera.org:8080/#/c/5120/1//COMMIT_MSG
Commit Message:
PS1, Line 29: - we try to auto-detect the case in which the client has fully
: initialized SASL including providing a mutex implementation. In that
: case, we skip initialization but log a warning that the caller should
: disable it explicitly.
: - if we detect that it's initialized but no mutex implementation is
: provided, we'll return an error on first usage of SASL. Similarly, if
: they disable initialization but didn't do a proper job of
: initialization, we detect it and return an error on usage.
what about also offering a way to explicitly enable as well, then you also won't have to auto detect unless it's not specified either way. It's also odd, but at least if the auto detection turns out to be flaky in some cases (who knows), then this is a way to avoid it. Obviously it won't matter for Impala.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
own, we'll return an error on first usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Reviewed-on: http://gerrit.cloudera.org:8080/5120
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 212 insertions(+), 10 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/4/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS4, Line 561: rpc::internal::SaslSetMutex();
Does it make sense to add a test where this is skipped by the client? I.e., the same, but this line is omitted.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
PS3, Line 218:
extra space
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/6/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
PS6, Line 28: #include <sasl/saslplug.h>
nit: does it make sense to include only for non-macOS builds?
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5120
to look at the new patch set (#6).
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
own, we'll return an error on first usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 212 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5120/6
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
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/5120
to look at the new patch set (#3).
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
own, we'll return an error on first usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 183 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5120/3
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/1//COMMIT_MSG
Commit Message:
PS1, Line 29: - we try to auto-detect the case in which the client has fully
: initialized SASL including providing a mutex implementation. In that
: case, we skip initialization but log a warning that the caller should
: disable it explicitly.
: - if we detect that it's initialized but no mutex implementation is
: provided, we'll return an error on first usage of SASL. Similarly, if
: they disable initialization but didn't do a proper job of
: initialization, we detect it and return an error on usage.
> I started writing the code for this, and when I got to documenting the API
Nope I don't feel strongly, this won't affect Impala, I just thought it'd be a reasonable risk mitigation option for other clients.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS3, Line 511: executable_path,
: "test",
: filter_flag,
: "--is_test_child"},
> Nit: indentation is strangely unaligned here.
Done
http://gerrit.cloudera.org:8080/#/c/5120/4/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS4, Line 561: rpc::internal::SaslSetMutex();
> Does it make sense to add a test where this is skipped by the client? I.e.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:
PS3, Line 218:
> extra space
Done
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
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/5120
to look at the new patch set (#4).
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
KUDU-1749. Allow callers to disable SASL initialization
The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.
The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.
IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.
The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
SASL.
- we try to auto-detect the case in which the client has fully
initialized SASL including providing a mutex implementation. In that
case, we skip initialization but log a warning that the caller should
disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
own, we'll return an error on first usage.
The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.
Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
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, 183 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/5120/4
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
Chatted offline with MJ from the Impala team. He asked me to make it a WARNING instead of returning a bad Status in the case that Impala inits SASL but doesn't initialize the SASL mutexes. Both Impala and Kudu through 1.0 neglected to set up mutexes and have never seen a problem, so we don't think it's 100% critical. And there's some fear it could cause an unexpected perf issue to enable it now. So, we want to stage the change -- this patch will just make it a WARNING and unbreak the integration, and then they'll separately test the sasl_set_mutex() addition separately.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 6: Code-Review+2
Looks good to me, though Alexey/Todd may want to have another look.
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/6/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:
PS6, Line 570: // Test a client which inits SASL itself but doesn't remember to disable Kudu's
: // SASL initialization
nit: is it worth mentioning in the comment that the client does not do proper mutex initialization?
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1749. Allow callers to disable SASL initialization
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:
PS3, Line 132: SASL
> I think changing just this one instance would be enough to precisely indica
Done
--
To view, visit http://gerrit.cloudera.org:8080/5120
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes