Address some minor bugs around establishing SSL connections#644
Conversation
A few minor bugs were discovered while investigating riak_test failures.
* The ssl application is explicitly started in
riak_core_connection:try_ssl/0. The statement in the function
expects the call to ssl:start/0 to always return ok, but in some
cases the ssl application is already started and the call returns
{error, {already_started, ssl}} instead. This should not represent
an error condition, but as written an exception is generated in this
case. This resulted in riak_test runs of replication tests that
exercise SSL to stall. Really there is no reason to attempt to start
the ssl application at this point in the code. The ssl application
is an application dependency of the riak_repl application and should
be started by the call to riak_core_util:start_app_deps in
riak_repl_app:start/2. Removing the attempt to start ssl in
riak_core_connection to avoid confusion.
* The first handle_info function clause in riak_core_connection that
handles a message received while in the wait_for_capabilities state
attempts to use SSL by calling the try_ssl/4 function. If it
succeeds a pair is returned whose elements are the name of the new
transport and a new socket for the SSL connection. However the new
socket was not being used for subsequent calls to send and setopts
and this caused failure of several riak_tests.
* The non-test function clause of
riak_core_cluster_conn:request_cluster_name/1 contained assumptions
about the transport in use and explicitly called
inet:setopts/2. This does not work when SSL is used and also caused
several test failures. The function has been changed so that the
specified transport is used for the call to setopts instead.
|
I uncovered these issues looking at the following failures from the 2.0.3 scorecard:
There are probably others that failed that may be resolved by these changes, but I have not had a chance to run them yet. In general, if the |
|
cc: @engelsanchez |
|
Thanks Kelly! I'll review and test this now. |
|
All code changes make sense (how the heck did it work before?). I found an issue with the realtime connection unit tests that exists before this PR and filed RIAK-1338 to resolve it. The SSL tests that I tried pass with this. Let's merge so we can start running the entire suite and get a sense of what else is lurking. 👍 2da227c |
|
👍 2da227c I said! |
Address some minor bugs around establishing SSL connections Reviewed-by: engelsanchez
Address some minor bugs around establishing SSL connections Reviewed-by: engelsanchez
|
@borshop merge |
A few minor bugs were discovered while investigating riak_test failures.
riak_core_connection:try_ssl/0. The statement in the function
expects the call to ssl:start/0 to always return ok, but in some
cases the ssl application is already started and the call returns
{error, {already_started, ssl}} instead. This should not represent
an error condition, but as written an exception is generated in this
case. This resulted in riak_test runs of replication tests that
exercise SSL to stall. Really there is no reason to attempt to start
the ssl application at this point in the code. The ssl application
is an application dependency of the riak_repl application and should
be started by the call to riak_core_util:start_app_deps in
riak_repl_app:start/2. Removing the attempt to start ssl in
riak_core_connection to avoid confusion.
handles a message received while in the wait_for_capabilities state
attempts to use SSL by calling the try_ssl/4 function. If it
succeeds a pair is returned whose elements are the name of the new
transport and a new socket for the SSL connection. However the new
socket was not being used for subsequent calls to send and setopts
and this caused failure of several riak_tests.
riak_core_cluster_conn:request_cluster_name/1 contained assumptions
about the transport in use and explicitly called
inet:setopts/2. This does not work when SSL is used and also caused
several test failures. The function has been changed so that the
specified transport is used for the call to setopts instead.