support a connection callback for proxies

[ccutrer]

also refactors connection establishment to only go through one method

[jch]

Could you leave this out of the change log for now? We usually update this when we prepare for a release. For example, see #174

[jch]

@ccutrer thanks for the pull. I like refactoring for new_connection and use_connection. Can you elaborate more on your use case for the connect callback? What decorated socket object would you return there?

[ccutrer]

Not a problem. I was just following the instructions from https://github.com/ruby-ldap/ruby-net-ldap/blob/master/Hacking.rdoc

I'm planning on establishing the connection with https://github.com/samuelkadolph/ruby-proxifier.

[ccutrer]

Sorry for the terse comment. I was writing it, then realized I was late for a short appointment.

I updated the commit to remove the history entry. I plan on using proxifier because in our environment we need to proxy LDAP requests (and LDAP requests only) through a specific server so that clients can whitelist a specific IP address. The application makes many other network requests that need to take the general route over the network for performance reasons, so I need to be able to control LDAP specifically (and without monkey patching all of TCPSocket).

[mynameisrufus]

Nice low hanging fruit refactor, I was about to embark on it myself. Well done.

[jch]

Not a problem. I was just following the instructions from https://github.com/ruby-ldap/ruby-net-ldap/blob/master/Hacking.rdoc

Hah, oops. Sorry about that, I didn't see that previously.

Great explanation of your reasoning behind the callback. How do you feel about parameterizing the socket passed into connection instead? For example:

Net::LDAP.new(:socket => @proxy_socket, ...)

@mtodd @schaary could you chime in about allowing callers to customize the socket class? It feels pretty low level, but I think the above use case is reasonable. We can also implement this and maintain backwards compatibility.

[ccutrer]

I had originally planned on just passing the socket through, until I realized that there may be 0 or more connections created by a Net::LDAP - i.e. if open is not used, and you call multiple other methods, a new connection is established each time. Re-using the cached @socket would cause errors on the second connection attempt (or waste resources if a connection is never opened and you just always create a Net::LDAP object before you know if you're going to use it).

[jch]

@ccutrer good point, but your refactoring of new_connection is now the only entry point for opening a new socket. Wouldn't it be the same as the callback if we saved the passed in socket in an instance variable and use that?

module Net
  class LDAP
    def initialize(args = {})
      @socket = args[:socket]
      # snip other ivars
    end

    def new_connection
      socket = @socket || TCPSocket.new(...)
      Connection.new(:socket => socket, ....)
    end
  end
end

[mtodd]

I explored a similar implementation in #135 which allowed users to set a connection_class (to wrap Net::LDAP::Connection) and socket_class (to wrap/replace TCPSocket). We decided to scrap that, for the time being.

I'm 👎 to adding a callback, but absolutely support the need for more control over the socket.

The API that comes to mind looks something like this:

ldap = Net::LDAP.new(options)

ldap.open(:socket => TCPSocket.new(ip, port)) do |conn|
  conn.search
end

This would require new_connection to accept a socket instead of requiring a callback to get that value, but it forces users to go through Net::LDAP#open.

I think this is more adaptable, since different socket classes may not have compatible instantiation with TCPSocket (which would be a problem if socket_class were the only configurable option).

Huge 👍 to consolidating connection handling through the new_connection and use_connection APIs.

[ccutrer]

@jch - the callback can return a new socket each time it is called

@mtodd - shall I separate out the use_connection refactor into a separate pull request and get that in, and then we can continue the discussion on how to offer better control of the connection process?

[jch]

shall I separate out the use_connection refactor into a separate pull request and get that in

👍 to a separate PR

@mtodd I'm skeptical of duplicating all or any of the initialization options in #open. It does address @ccutrer's point about having fine grained control of whether you want a new socket or not.

Taking a step back, I dislike the implicit connection handling in Net::LDAP. I'd prefer Net::LDAP::Connection to be the public interface rather than what's currently in Net::LDAP. But that's outside the scope of this PR.

[ccutrer]

#180 for the refactor; I'll rebase this PR on top of that

[mtodd]

@mtodd I'm skeptical of duplicating all or any of the initialization options in #open.

It shouldn't be necessary since Net::LDAP#open should defer to new_connection or use_connection appropriately. It should just merge nicely with the options already set and override only for the scope of that block. Net::LDAP#open feels like a very natural place for this kind of behavior, too.

[colmexdev]

Sorry to comment over this old pull request, but I've been having troubles to connect to LDAP through proxy (Nginx + Puma) with ECONNREFUSED error. I don't know why it's taking localhost as default socket, but I want to authenticate to a remote LDAP server and I can't find how is that possible given that it seems impossible to change host and port for connection.

Any advice will be truly appreciated.