(maint) Changes discussed w/ Josh

Author: melissa

lib/puppet/http/client.rb

@@ -147,10 +147,10 @@ def build_resolvers
     resolvers = []
 
     if Puppet[:use_srv_records]
-      resolvers << Puppet::HTTP::Resolver::SRV.new(domain: Puppet[:srv_domain])
+      resolvers << Puppet::HTTP::Resolver::SRV.new(self, domain: Puppet[:srv_domain])
     end
 
-    resolvers << Puppet::HTTP::Resolver::Settings.new
+    resolvers << Puppet::HTTP::Resolver::Settings.new(self)
     resolvers.freeze
   end
 end

lib/puppet/http/resolver.rb

@@ -1,5 +1,18 @@
 class Puppet::HTTP::Resolver
-  def resolve(session, name, &block)
+  def initialize(client)
+    @client = client
+  end
+
+  def resolve(session, name, ssl_context: nil, &block)
     raise NotImplementedError
   end
+
+  def check_connection?(session, service, ssl_context: nil)
+    service.connect(ssl_context: ssl_context)
+    return true
+  rescue Puppet::HTTP::ConnectionError => e
+    session.add_exception(e)
+    Puppet.debug("Connection to #{service.url} failed, trying next route: #{e.message}")
+    return false
+  end
 end

lib/puppet/http/resolver/serverlist.rb

@@ -5,26 +5,33 @@ def initialize(client, server_list: Puppet[:server_list], default_port: Puppet[:
     @default_port = default_port
   end
 
-  def resolve(session, name, &block)
+  def resolve(session, name, ssl_context: nil, &block)
     success = false
     @server_list.each do |server|
       host = server[0]
       port = server[1] || @default_port
-      begin
-        response = @client.get(URI("https://#{host}:#{port}/status/v1/simple/master"))
-        if response.success?
-          success = true
-          yield session.create_service(name, host, port)
-        end
-
-        Puppet.debug(_("Puppet server %{host}:%{port} is unavailable: %{code} %{reason}") %
-                     { host: host, port: port, code: response.code, reason: response.message })
-      rescue => detail
-        #TRANSLATORS 'server_list' is the name of a setting and should not be translated
-        Puppet.debug _("Unable to connect to server from server_list setting: %{detail}") % {detail: detail}
+      uri = URI("https://#{host}:#{port}/status/v1/simple/master")
+      service = Puppet::HTTP::Service.create_service(@client, name, host, port)
+      if check_connection?(uri, session, service, ssl_context: ssl_context)
+        success = true
+        return service
       end
     end
 
     raise Puppet::Error, _("Could not select a functional puppet master from server_list: '%{server_list}'") % { server_list: Puppet.settings.value(:server_list, Puppet[:environment].to_sym, true) } unless success
   end
+
+  def check_connection?(uri, session, service, ssl_context: nil)
+    response = @client.get(uri, ssl_context: ssl_context)
+    return true if response.success?
+
+    Puppet.debug(_("Puppet server %{host}:%{port} is unavailable: %{code} %{reason}") %
+                 { host: host, port: port, code: response.code, reason: response.message })
+    return false
+  rescue => detail
+    session.add_exception(detail)
+    #TRANSLATORS 'server_list' is the name of a setting and should not be translated
+    Puppet.debug _("Unable to connect to server from server_list setting: %{detail}") % {detail: detail}
+    return false
+  end
 end

lib/puppet/http/resolver/settings.rb

@@ -1,5 +1,6 @@
 class Puppet::HTTP::Resolver::Settings < Puppet::HTTP::Resolver
-  def resolve(session, name, &block)
-    yield session.create_service(name)
+  def resolve(session, name, ssl_context: nil, &block)
+    service = Puppet::HTTP::Service.create_service(@client, name)
+    return service if check_connection?(session, service, ssl_context: ssl_context)
   end
 end

lib/puppet/http/resolver/srv.rb

@@ -1,15 +1,17 @@
 class Puppet::HTTP::Resolver::SRV < Puppet::HTTP::Resolver
-  def initialize(domain: srv_domain, dns: Resolv::DNS.new)
+  def initialize(client, domain:, dns: Resolv::DNS.new)
+    @client = client
     @srv_domain = domain
     @delegate = Puppet::Network::Resolver.new(dns)
   end
 
-  def resolve(session, name, &block)
+  def resolve(session, name, ssl_context: nil, &block)
     # Here we pass our HTTP service name as the DNS SRV service name
     # This is fine for :ca, but note that :puppet and :file are handled
     # specially in `each_srv_record`.
     @delegate.each_srv_record(@srv_domain, name) do |server, port|
-      yield session.create_service(name, server, port)
+      service = Puppet::HTTP::Service.create_service(@client, name, server, port)
+      return service if check_connection?(session, service, ssl_context: ssl_context)
     end
   end
 end

lib/puppet/http/session.rb

@@ -3,6 +3,7 @@ def initialize(client, resolvers)
     @client = client
     @resolvers = resolvers
     @resolved_services = {}
+    @resolution_exceptions = []
   end
 
   def route_to(name, ssl_context: nil)
@@ -15,25 +16,19 @@ def route_to(name, ssl_context: nil)
 
     @resolvers.each do |resolver|
       Puppet.debug("Resolving service '#{name}' using #{resolver.class}")
-      resolver.resolve(self, name) do |service|
-        begin
-          service.connect(ssl_context: ssl_context)
-          @resolved_services[name] = service
-          Puppet.debug("Resolved service '#{name}' to #{service.url}")
-          return service
-        rescue Puppet::HTTP::ConnectionError => e
-          errors << e
-          Puppet.debug("Connection to #{service.url} failed, trying next route: #{e.message}")
-        end
+      service = resolver.resolve(self, name, ssl_context: ssl_context)
+      if service
+        @resolved_services[name] = service
+        Puppet.debug("Resolved service '#{name}' to #{service.url}")
+        return service
       end
     end
 
-    errors.each { |e| Puppet.log_exception(e) }
-
+    @resolution_exceptions.each { |e| Puppet.log_exception(e) }
     raise Puppet::HTTP::RouteError, "No more routes to #{name}"
   end
 
-  def create_service(name, server = nil, port = nil)
-    Puppet::HTTP::Service.create_service(@client, name, server, port)
+  def add_exception(exception)
+    @resolution_exceptions << exception
   end
 end

spec/unit/http/resolver_spec.rb

@@ -9,7 +9,7 @@
   let(:uri) { URI.parse('https://www.example.com') }
 
   context 'when resolving using settings' do
-    let(:subject) { Puppet::HTTP::Resolver::Settings.new }
+    let(:subject) { Puppet::HTTP::Resolver::Settings.new(client) }
 
     it 'yields a service based on the current ca_server and ca_port settings' do
       Puppet[:ca_server] = 'ca.example.com'
@@ -82,7 +82,7 @@
 
   context 'when resolving using SRV' do
     let(:dns) { double('dns') }
-    let(:subject) { Puppet::HTTP::Resolver::SRV.new(domain: 'example.com', dns: dns) }
+    let(:subject) { Puppet::HTTP::Resolver::SRV.new(client, domain: 'example.com', dns: dns) }
 
     def stub_srv(host, port)
       srv = Resolv::DNS::Resource::IN::SRV.new(0, 0, port, host)

spec/unit/http/session_spec.rb

@@ -10,7 +10,7 @@
     double('good', url: uri, connect: nil)
   }
   let(:bad_service) {
-    service = double('good', url: uri)
+    service = double('bad', url: uri)
     allow(service).to receive(:connect).and_raise(Puppet::HTTP::ConnectionError, 'whoops')
     service
   }
@@ -23,9 +23,18 @@ def initialize(service)
       @count = 0
     end
 
-    def resolve(session, name, &block)
+    def resolve(session, name, ssl_context: nil, &block)
       @count += 1
-      yield @service
+      return @service if check_connection?(session, @service, ssl_context: ssl_context)
+    end
+
+    def check_connection?(session, service, ssl_context: nil)
+      service.connect(ssl_context: ssl_context)
+      return true
+    rescue Puppet::HTTP::ConnectionError => e
+      session.add_exception(e)
+      Puppet.debug("Connection to #{service.url} failed, trying next route: #{e.message}")
+      return false
     end
   end