Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy initialize Net::LDAP::Connection's internal socket #235

Merged
merged 12 commits into from
Jan 11, 2016
Merged

Lazy initialize Net::LDAP::Connection's internal socket #235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d5e6afd
lazy init Net::LDAP::Connection's internal sock
jch Nov 10, 2015
4a415bc
preserve existing socket init code
jch Nov 10, 2015
b856806
#socket internal for easier testing
jch Nov 10, 2015
76dde7b
parameterize socket_class for testing
jch Nov 10, 2015
53cc6b5
remove tcpsocket stubbing
jch Nov 10, 2015
e9a1bf1
add initialize docs
jch Nov 10, 2015
9a2e26e
preserve existing behavior
jch Nov 10, 2015
259f18a
update tests
jch Nov 10, 2015
f6ad189
use fake for auth adapter test
jch Nov 11, 2015
e7cc5ae
replace ldap tests with fake connection object
jch Nov 11, 2015
d35d3bf
Merge remote-tracking branch 'origin/master' into lazy-socket-initial…
jch Jan 8, 2016
1aab8c9
set socket_class in initialize
jch Jan 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/net/ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,6 +1275,11 @@ def inspect
inspected
end

# Internal: Set @open_connection for testing
def connection=(connection)
@open_connection = connection
end

private

# Yields an open connection if there is one, otherwise establishes a new
Expand All @@ -1300,13 +1305,17 @@ def use_connection(args)

# Establish a new connection to the LDAP server
def new_connection
Net::LDAP::Connection.new \
connection = Net::LDAP::Connection.new \
:host => @host,
:port => @port,
:hosts => @hosts,
:encryption => @encryption,
:instrumentation_service => @instrumentation_service,
:connect_timeout => @connect_timeout

# Force connect to see if there's a connection error
connection.socket
connection
rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::LDAP::ConnectionRefusedError => e
@result = {
:resultCode => 52,
Expand Down
58 changes: 48 additions & 10 deletions lib/net/ldap/connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,28 @@ class Net::LDAP::Connection #:nodoc:
LdapVersion = 3
MaxSaslChallenges = 10

def initialize(server)
# Initialize a connection to an LDAP server
#
# :server
# :hosts Array of tuples specifying host, port
# :host host
# :port port
# :socket prepared socket
#
def initialize(server = {})
@server = server
@instrumentation_service = server[:instrumentation_service]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add the following here, you can simplify overriding the socket class:

@socket_class = server.fetch(:socket_class, TCPSocket)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1aab8c9


if server[:socket]
prepare_socket(server)
else
server[:hosts] = [[server[:host], server[:port]]] if server[:hosts].nil?
open_connection(server)
end
# Allows tests to parameterize what socket class to use
@socket_class = server.fetch(:socket_class, DefaultSocket)

yield self if block_given?
end

def socket_class=(socket_class)
@socket_class = socket_class
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods could be removed if you allow the socket class to be set in the initializer.

def prepare_socket(server)
socket = server[:socket]
encryption = server[:encryption]
Expand All @@ -41,7 +50,7 @@ def open_connection(server)
errors = []
hosts.each do |host, port|
begin
prepare_socket(server.merge(socket: Socket.tcp(host, port, socket_opts)))
prepare_socket(server.merge(socket: @socket_class.new(host, port, socket_opts)))
return
rescue Net::LDAP::Error, SocketError, SystemCallError,
OpenSSL::SSL::SSLError => e
Expand Down Expand Up @@ -202,7 +211,7 @@ def message_queue
def read(syntax = Net::LDAP::AsnSyntax)
ber_object =
instrument "read.net_ldap_connection", :syntax => syntax do |payload|
@conn.read_ber(syntax) do |id, content_length|
socket.read_ber(syntax) do |id, content_length|
payload[:object_type_id] = id
payload[:content_length] = content_length
end
Expand Down Expand Up @@ -232,7 +241,7 @@ def read(syntax = Net::LDAP::AsnSyntax)
def write(request, controls = nil, message_id = next_msgid)
instrument "write.net_ldap_connection" do |payload|
packet = [message_id.to_ber, request, controls].compact.to_ber_sequence
payload[:content_length] = @conn.write(packet)
payload[:content_length] = socket.write(packet)
end
end
private :write
Expand Down Expand Up @@ -652,4 +661,33 @@ def delete(args)

pdu
end

# Internal: Returns a Socket like object used internally to communicate with
# LDAP server.
#
# Typically a TCPSocket, but can be a OpenSSL::SSL::SSLSocket
def socket
return @conn if defined? @conn
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, I have a question.
Why is defined? used? it looks working as if @conn. but there are a little bit difference between the both.

@foo = 1 # <= defined instance variable
puts 'ok' if @foo # => 'ok'
puts 'ok' if defined? @foo # => 'ok'   so the both are same.

@bar = nil # defined but its value is nil
puts 'ok' if @bar # => nothing
puts 'ok' if defined @bar # => 'ok'  so different behavior

I believe that this difference doesn't affect on the behavior of Net::LDAP.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satoryu This is likely done to avoid warnings from the interpreter for using uninitialized instance variables. The defined? method is exempted from the warnings as I recall.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


# First refactoring uses the existing methods open_connection and
# prepare_socket to set @conn. Next cleanup would centralize connection
# handling here.
if @server[:socket]
prepare_socket(@server)
else
@server[:hosts] = [[@server[:host], @server[:port]]] if @server[:hosts].nil?
open_connection(@server)
end

@conn
end

private

# Wrap around Socket.tcp to normalize with other Socket initializers
class DefaultSocket
def self.new(host, port, socket_opts = {})
Socket.tcp(host, port, socket_opts)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at a somewhat old definition of Socket.tcp, I'm not sure this invocation will work for setting socket options.

http://ruby-doc.org/stdlib-1.9.3/libdoc/socket/rdoc/Socket.html#method-c-tcp

The newer version of that method has an ability to take the socket options, but you need to pass something along to the local_host and local_port parameters first.

http://ruby-doc.org/stdlib-2.3.0/libdoc/socket/rdoc/Socket.html#method-c-tcp

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanthropus this is possible because we dropped support for 1.9.x in 0.13.0

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch Check the documentation I linked for version 2.3.0. Unless I misunderstood something there, you'll need to set the local_host and local_port parameters to at least nil in order to pass socket_opts. Maybe there is some parameter hacking going on behind the scenes though.

end
end
end # class Connection
9 changes: 6 additions & 3 deletions test/test_auth_adapter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
require 'test_helper'

class TestAuthAdapter < Test::Unit::TestCase
def test_undefined_auth_adapter
flexmock(Socket).should_receive(:tcp).ordered.with('ldap.example.com', 379, { connect_timeout: 5 }).once.and_return(nil)
class FakeSocket
def initialize(*args)
end
end

conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379)
def test_undefined_auth_adapter
conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379, :socket_class => FakeSocket)
assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do
conn.bind(method: :foo)
end
Expand Down
41 changes: 31 additions & 10 deletions test/test_ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,28 @@
require 'test_helper'

class TestLDAPInstrumentation < Test::Unit::TestCase
# Fake Net::LDAP::Connection for testing
class FakeConnection
# It's difficult to instantiate Net::LDAP::PDU objects. Faking out what we
# need here until that object is brought under test and has it's constructor
# cleaned up.
class Result < Struct.new(:success?, :result_code); end

def initialize
@bind_success = Result.new(true, Net::LDAP::ResultCodeSuccess)
@search_success = Result.new(true, Net::LDAP::ResultCodeSizeLimitExceeded)
end

def bind(args = {})
@bind_success
end

def search(*args)
yield @search_success if block_given?
@search_success
end
end

def setup
@connection = flexmock(:connection, :close => true)
flexmock(Net::LDAP::Connection).should_receive(:new).and_return(@connection)
Expand All @@ -15,8 +37,9 @@ def setup
def test_instrument_bind
events = @service.subscribe "bind.net_ldap"

bind_result = flexmock(:bind_result, :success? => true)
flexmock(@connection).should_receive(:bind).with(Hash).and_return(bind_result)
fake_connection = FakeConnection.new
@subject.connection = fake_connection
bind_result = fake_connection.bind

assert @subject.bind

Expand All @@ -28,10 +51,9 @@ def test_instrument_bind
def test_instrument_search
events = @service.subscribe "search.net_ldap"

flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess))
flexmock(@connection).should_receive(:search).with(Hash, Proc).
yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")).
and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSuccess))
fake_connection = FakeConnection.new
@subject.connection = fake_connection
entry = fake_connection.search

refute_nil @subject.search(:filter => "(uid=user1)")

Expand All @@ -44,10 +66,9 @@ def test_instrument_search
def test_instrument_search_with_size
events = @service.subscribe "search.net_ldap"

flexmock(@connection).should_receive(:bind).and_return(flexmock(:bind_result, :result_code => Net::LDAP::ResultCodeSuccess))
flexmock(@connection).should_receive(:search).with(Hash, Proc).
yields(entry = Net::LDAP::Entry.new("uid=user1,ou=users,dc=example,dc=com")).
and_return(flexmock(:search_result, :success? => true, :result_code => Net::LDAP::ResultCodeSizeLimitExceeded))
fake_connection = FakeConnection.new
@subject.connection = fake_connection
entry = fake_connection.search

refute_nil @subject.search(:filter => "(uid=user1)", :size => 1)

Expand Down
77 changes: 44 additions & 33 deletions test/test_ldap_connection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,44 +9,55 @@ def capture_stderr
$stderr = stderr
end

# Fake socket for testing
#
# FakeTCPSocket.new("success", 636)
# FakeTCPSocket.new("fail.SocketError", 636) # raises SocketError
class FakeTCPSocket
def initialize(host, port, socket_opts = {})
status, error = host.split(".")
if status == "fail"
raise Object.const_get(error)
end
end
end

def test_list_of_hosts_with_first_host_successful
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
["success.host", 636],
["fail.SocketError", 636],
["fail.SocketError", 636],
]
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_return(nil)
flexmock(Socket).should_receive(:tcp).ordered.never
Net::LDAP::Connection.new(:hosts => hosts)

connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
connection.socket
end

def test_list_of_hosts_with_first_host_failure
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
["fail.SocketError", 636],
["success.host", 636],
["fail.SocketError", 636],
]
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_return(nil)
flexmock(Socket).should_receive(:tcp).ordered.never
Net::LDAP::Connection.new(:hosts => hosts)

connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
connection.socket
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch Uses such as these 3 lines are what I'm proposing to simplify a bit. I think it would look better and be easier to use like this:

connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)

The idea is for this single call to create the connection and arrange for the opening of the socket using the specified socket class when needed all in a single go rather than requiring the socket method to be called to actually open the socket. If the socket class is not specified, it should default to TCPSocket.

This is a fairly common way to handle dependency injection as you're trying to enable here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, with this change in place, you wouldn't need to add the socket_class and socket_class= attributes to the class definition of Connection at all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanthropus ++ your idea sounds good 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanthropus ah, I totally misunderstood! Parameterizing the socket class rather than the socket instance is a good idea. I'll roll with that.

end

def test_list_of_hosts_with_all_hosts_failure
hosts = [
['test.mocked.com', 636],
['test2.mocked.com', 636],
['test3.mocked.com', 636],
["fail.SocketError", 636],
["fail.SocketError", 636],
["fail.SocketError", 636],
]
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_raise(SocketError)
flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[2], { connect_timeout: 5 }).once.and_raise(SocketError)
flexmock(Socket).should_receive(:tcp).ordered.never

connection = Net::LDAP::Connection.new(:hosts => hosts, :socket_class => FakeTCPSocket)
assert_raise Net::LDAP::ConnectionError do
Net::LDAP::Connection.new(:hosts => hosts)
connection.socket
end
end

# This belongs in test_ldap, not test_ldap_connection
def test_result_for_connection_failed_is_set
flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)

Expand All @@ -61,42 +72,42 @@ def test_result_for_connection_failed_is_set
end

def test_unresponsive_host
connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket)
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
connection.socket
end
end

def test_blocked_port
flexmock(Socket).should_receive(:tcp).and_raise(SocketError)
connection = Net::LDAP::Connection.new(:host => "fail.SocketError", :port => 636, :socket_class => FakeTCPSocket)
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
connection.socket
end
end

def test_connection_refused
flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
connection = Net::LDAP::Connection.new(:host => "fail.Errno::ECONNREFUSED", :port => 636, :socket_class => FakeTCPSocket)
stderr = capture_stderr do
assert_raise Net::LDAP::ConnectionRefusedError do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
connection.socket
end
end
assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n", stderr)
end

def test_connection_timedout
flexmock(Socket).should_receive(:tcp).and_raise(Errno::ETIMEDOUT)
def test_connection_timeout
connection = Net::LDAP::Connection.new(:host => "fail.Errno::ETIMEDOUT", :port => 636, :socket_class => FakeTCPSocket)
stderr = capture_stderr do
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
connection.socket
end
end
end

def test_raises_unknown_exceptions
error = Class.new(StandardError)
flexmock(Socket).should_receive(:tcp).and_raise(error)
assert_raise error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
connection = Net::LDAP::Connection.new(:host => "fail.StandardError", :port => 636, :socket_class => FakeTCPSocket)
assert_raise StandardError do
connection.socket
end
end

Expand Down