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.

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

Add the ability to provide a list of hosts for a connection #223

Merged
merged 2 commits into from
Sep 29, 2015

Conversation

javanthropus
Copy link

This change adds the ability to provide a list of hosts to try when opening a connection while maintaining backward compatibility with existing code. If a list is provided, it overrides the individual host and port settings; otherwise, the individual settings are used as a single element list.

The host list is a simple object that responds to each and provides a hostname and port pair to a block. This allows for users to use any mechanism they need to provide a list of potential hosts. For instance, a randomized ordering for a static list could be used, or a list based on SRV record lookups could be provided.

In either case, failure of all servers is required in order to fail opening a connection. A Net::LDAP::Error instance that summarizes all of the server failures is raised in this case. If any server succeeds, usage proceeds as usual. This includes setting up encryption if requested.

@javanthropus
Copy link
Author

I forgot to mention that this PR provides a way to tackle #191 that avoids pulling in any additional dependencies into this project.

I also have another change ready to go that DRYs up some of the connection creation and error handling logic between when sockets are given as opposed to host-port pairs. I didn't include it here because it changes the messages of the error messages a bit, basically just wraps the original exception messages when raising Net::LDAP::Error and Net::LDAP::ConnectionRefusedError for them rather than include custom messages.

I can either add that commit here or apply it as a separate PR that builds on the result of accepting this PR so that it can be considered independently. Let me know.

]
flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil)
flexmock(TCPSocket).should_receive(:new).ordered.never
Net::LDAP::Connection.new(:hosts => hosts.to_enum(:each))
Copy link
Member

Choose a reason for hiding this comment

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

Why to_enum here? hosts is already an iterable object?

Copy link
Author

Choose a reason for hiding this comment

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

That was a late night edit, I think. I was trying to make sure that the
object passed in only responded to the each method so that the test
enforces the contract and doesn't allow the code to start using other
methods.

For now, I'll probably just chuck the to_enum calls unless you would
prefer the enforcement. It shouldn't too hard to make a simple wrapper
class to mask out the other methods though.

On Mon, Sep 28, 2015, 15:12 Jerry Cheung notifications@github.com wrote:

In test/test_ldap_connection.rb
#223 (comment)
:

@@ -9,6 +9,44 @@ def capture_stderr
$stderr = stderr
end

  • def test_list_of_hosts_with_first_host_successful
  • hosts = [
  •          ['test.mocked.com', 636],
    
  •          ['test2.mocked.com', 636],
    
  •          ['test3.mocked.com', 636],
    
  •        ]
    
  • flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil)
  • flexmock(TCPSocket).should_receive(:new).ordered.never
  • Net::LDAP::Connection.new(:hosts => hosts.to_enum(:each))

Why to_enum here? hosts is already an iterable object?


Reply to this email directly or view it on GitHub
https://github.com/ruby-ldap/ruby-net-ldap/pull/223/files#r40599218.

@jch
Copy link
Member

jch commented Sep 28, 2015

Looking good. Thanks for getting this started!

jch added a commit that referenced this pull request Sep 29, 2015
Add the ability to provide a list of hosts for a connection
@jch jch merged commit 5673f83 into ruby-ldap:master Sep 29, 2015
@jch
Copy link
Member

jch commented Sep 29, 2015

🎉

@javanthropus javanthropus deleted the support_host_list branch September 29, 2015 02:42
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 12, 2015
=== Net::LDAP 0.12.1

* Whitespace formatting cleanup
  {#236}[ruby-ldap/ruby-net-ldap#236]
* Set operation result if LDAP server is not accessible
  {#232}[ruby-ldap/ruby-net-ldap#232]

=== Net::LDAP 0.12.0

* DRY up connection handling logic
  {#224}[ruby-ldap/ruby-net-ldap#224]
* Define auth adapters
  {#226}[ruby-ldap/ruby-net-ldap#226]
* add slash to attribute value filter
  {#225}[ruby-ldap/ruby-net-ldap#225]
* Add the ability to provide a list of hosts for a connection
  {#223}[ruby-ldap/ruby-net-ldap#223]
* Specify the port of LDAP server by giving INTEGRATION_PORT
  {#221}[ruby-ldap/ruby-net-ldap#221]
* Correctly set BerIdentifiedString values to UTF-8
  {#212}[ruby-ldap/ruby-net-ldap#212]
* Raise Net::LDAP::ConnectionRefusedError when new connection is
  refused. {#213}[ruby-ldap/ruby-net-ldap#213]
* obscure auth password upon #inspect, added test, closes #216
  {#217}[ruby-ldap/ruby-net-ldap#217]
* Fixing incorrect error class name
  {#207}[ruby-ldap/ruby-net-ldap#207]
* Travis update {#205}[ruby-ldap/ruby-net-ldap#205]
* Remove obsolete rbx-19mode from Travis
  {#204}[ruby-ldap/ruby-net-ldap#204]
* mv "sudo" from script/install-openldap to .travis.yml
  {#199}[ruby-ldap/ruby-net-ldap#199]
* Remove meaningless shebang
  {#200}[ruby-ldap/ruby-net-ldap#200]
* Fix Travis CI build
  {#202}[ruby-ldap/ruby-net-ldap#202]
* README.rdoc: fix travis link
  {#195}[ruby-ldap/ruby-net-ldap#195]
astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
Add the ability to provide a list of hosts for a connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants