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 2017/02/14 02:18:58 UTC
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Hello Matthew Jacobs, Sailesh Mukil, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5992
to review the following change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
5 files changed, 95 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/1
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL 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/5992
to look at the new patch set (#9).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_context.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
15 files changed, 159 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/9
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
Gerrit-PatchSet: 9
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5992/5/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS5, Line 88: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
Call CheckOpenSSLInitialized() instead?
Line 94: // Continue anyway rather than crashing the process in release builds.
Why not do something similar to what DoSaslInit() does here?
i.e. if SSL already set up, set g_disable_ssl_init = true;
And
if (g_disable_ssl_init) {
check SSLMutexImplementationProvided();
check locking_callback();
return;
}
Instead of overwriting everything?
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL 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/5992
to look at the new patch set (#8).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_context.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
14 files changed, 152 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/8
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
Gerrit-PatchSet: 8
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 4:
(1 comment)
I think this API should work, though Impala will have to make some changes as it looks like our OpenSSL configuration is a mess right now.
We'll need to:
1) update squeasel to get your change to disable ssl init, and disable the squeasel init
2) change our process startup to force thrift to do its ssl init early
3) potentially add a follow up call to set the function ID callback, though I'm curious to see what you say to my comment in openssl_util.cc.
4) add a call to the kudu disable ssl fn.
Does that smell right to you?
Thanks
3) potentially
http://gerrit.cloudera.org:8080/#/c/5992/4/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS4, Line 75: if (!CRYPTO_THREADID_get_callback()) {
: return Status::RuntimeError("Thread-ID callback not initialized");
: }
Does this need to be a requirement? Impala relies on thrift to initialize SSL currently, and that code [1] won't end up setting this fn. However, according to the OpenSSL doc, the address of errno is used if this isn't called and that should work fine as long as errno itself is threadsafe, which should be true [2].
1: https://github.com/apache/thrift/blob/0.9.0/lib/cpp/src/thrift/transport/TSSLSocket.cpp#L527
2: http://stackoverflow.com/questions/1694164/is-errno-thread-safe
It's fine if you say you don't feel comfortable relying on this, though it seems like it could be up to the consumer to determine if this is good enough for MT OpenSSL or not.
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5992/4/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS4, Line 75: if (!CRYPTO_THREADID_get_callback()) {
: return Status::RuntimeError("Thread-ID callback not initialized");
: }
> Does this need to be a requirement? Impala relies on thrift to initialize S
ah, you're right.. it looks like this has been the case for all of the OpenSSL >= 1.0.0, and that's our minimum version. Let me check with Dan and Alexey to see what they think, but I think you're probably right that we can stop setting and checking the threadid callback.
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Sailesh Mukil, Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5992
to look at the new patch set (#2).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
5 files changed, 95 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/2
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5992/6/src/kudu/client/client.h
File src/kudu/client/client.h:
PS6, Line 154: NOTE
> doxygen nit: please use @note instead to enable special formatting in the g
Done
http://gerrit.cloudera.org:8080/#/c/5992/6/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
Line 46: bool g_ssl_is_initialized = false;
> Could you add some notes on the thread safety of these booleans? My first
Done
Line 70: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
> This would be simpler as
Done
PS6, Line 80: return;
> Is it worth adding LOG() statement to inform on this control path branch?
LOG(INFO) is too noisy since that would show up in clients, and we want clients to be quiet in the normal case. (and this is the normal path for something like python which has external openssl init). I'll add a VLOG(2)
Line 88: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
> Same.
Done
Line 130: g_disable_ssl_init = true;
> Consider adding LOG(INFO) about disabling OpenSSL initialization.
see above. Added the VLOG(2) in the "once" site
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5992/4/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS4, Line 75: if (!CRYPTO_THREADID_get_callback()) {
: return Status::RuntimeError("Thread-ID callback not initialized");
: }
> ah, you're right.. it looks like this has been the case for all of the Open
This looks good to me -- I took a look at cryptolib.c and indeed, CRYPTO_THREADID_set_pointer(id, (void *)&errno);
If not setting the callback and relying on the default, there might be some case when OpenSSL library compiled without OPENSSL_NO_DEPRECATED flag and the other application set id_callback to something like getpid() (check crypto/cryptlib.c CRYPTO_THREADID_current() function). However, I don't thinkg it's feasible to catch every case when other application messed up with something in OpenSSL initialization.
I'm voting to remove our custom CRYPTO_THREADID callback.
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5992/6/src/kudu/client/client.h
File src/kudu/client/client.h:
PS6, Line 154: NOTE
doxygen nit: please use @note instead to enable special formatting in the generated docs.
http://gerrit.cloudera.org:8080/#/c/5992/6/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS6, Line 80: return;
Is it worth adding LOG() statement to inform on this control path branch? Consider at least LOG(INFO)
Line 130: g_disable_ssl_init = true;
Consider adding LOG(INFO) about disabling OpenSSL initialization.
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5992/6/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
Line 46: bool g_ssl_is_initialized = false;
Could you add some notes on the thread safety of these booleans? My first reaction is that they don't look protected.
Line 70: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
This would be simpler as
auto ctx = ssl_make_unique(SSLv23_method());
using the helpers from openssl_utils.
Line 88: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
Same.
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
Gerrit-PatchSet: 9
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
Reviewed-on: http://gerrit.cloudera.org:8080/5992
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_context.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
15 files changed, 159 insertions(+), 22 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
Gerrit-PatchSet: 10
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL 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/5992
to look at the new patch set (#6).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
13 files changed, 141 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/6
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Sailesh Mukil, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5992
to look at the new patch set (#4).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
12 files changed, 132 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/4
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Matthew Jacobs, Sailesh Mukil, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5992
to look at the new patch set (#3).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
10 files changed, 120 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/3
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL 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/5992
to look at the new patch set (#5).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
13 files changed, 127 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/5
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL 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/5992
to look at the new patch set (#7).
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
KUDU-1738. Allow users of the client to disable OpenSSL initialization
OpenSSL's initialization sequence is not thread-safe, and many
applications that embed Kudu may also be initializing OpenSSL by some
other means. This provides an API so that such applications can disable
Kudu's initialization sequence.
This patch exposed a couple cases where we previously had conflicting
OpenSSL initializations:
* EasyCurl was initializing curl, which initted OpenSSL.
The fix for this one was to explicitly disable curl's OpenSSL init
sequence.
* Python tests failed because _ssl was getting imported by setuptools.
The fix for this one was to explicitly disable Kudu's init sequence.
Additionally, Matt Jacobs noticed that the THREADID callback is actually
not necessary on Linux -- if it isn't set, OpenSSL since 1.0.0 (our min
version) defaults to using &errno, which is a different address in each
thread. So, this patch removes the setting of this callback.
Change-Id: I43eab5c848b30362356422d0380a336f16587562
---
M CMakeLists.txt
M python/kudu/client.pyx
M python/kudu/libkudu_client.pxd
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/tls_context.cc
M src/kudu/server/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/curl_util.cc
14 files changed, 152 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5992/7
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1738. Allow users of the client to disable OpenSSL initialization
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1738. Allow users of the client to disable OpenSSL initialization
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5992/5/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS5, Line 88: SSL_CTX* ctx = SSL_CTX_new(SSLv23_method());
> Call CheckOpenSSLInitialized() instead?
In the case that it's partially initialized, I'm worried that would still return !ok, but we'd still have a conflict. I think this is the most "basic" check we can do to see if anyone else tried to init the library.
Line 94: // Continue anyway rather than crashing the process in release builds.
> Why not do something similar to what DoSaslInit() does here?
I couldn't find a reliable way to check whether or not each of the individual things below was enabled. They also are all idempotent except for the locking callback. Let me add a check around the locking callback so that, if one is already installed, we don't overwrite it. The others should be idempotent (though they aren't thread-safe).
--
To view, visit http://gerrit.cloudera.org:8080/5992
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I43eab5c848b30362356422d0380a336f16587562
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: Jordan Birdsell <jt...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes