Fixes #39359 - Reuse persistent Foreman connections and retry stale POST /register

config/settings.yml.example

@@ -42,6 +42,21 @@
 #:foreman_ssl_cert: ssl/certs/fqdn.pem
 #:foreman_ssl_key: ssl/private_keys/fqdn.pem
 
+# Timeout in seconds for HTTP requests from smart-proxy to Foreman (e.g. POST /register).
+# Defaults to 200s to stay above subscription-manager's default server_timeout of 180s,
+# ensuring the proxy never times out before the client does.
+#:foreman_request_timeout: 200
+
+# Timeout in seconds for establishing TCP/TLS connections from smart-proxy to Foreman.
+# Leave unset to use Ruby's default. Consider raising this if TLS handshakes (including
+# future post-quantum profiles) need more time.
+#:foreman_connect_timeout: 30
+
+# Seconds to keep idle outbound connections to Foreman alive for reuse.
+# Must be lower than the server-side KeepAliveTimeout (15s on RHEL).
+# Higher values reuse more TLS connections; lower values reduce stale socket risk.
+#:foreman_keep_alive_timeout: 10
+
 # host and ports configuration
 # an array of interfaces to bind ports to (possible values: *, localhost, 0.0.0.0)
 #:bind_host: ['*']

lib/proxy/request.rb

@@ -44,8 +44,99 @@ def create_post(path, body, headers = {}, query = {})
   end
 
   class ForemanRequest
+    DEFAULT_KEEP_ALIVE_TIMEOUT = 10
+    DEFAULT_READ_TIMEOUT = 200
+
+    RETRY_EXCEPTIONS = [
+      EOFError,
+      Errno::ECONNRESET,
+      Errno::EPIPE,
+      IOError,
+      Net::OpenTimeout,
+      Net::ReadTimeout,
+      OpenSSL::SSL::SSLError,
+    ].freeze
+
+    FOREMAN_MUTEX = Mutex.new
+    REGISTRATION_GET_MUTEX = Mutex.new
+
+    class << self
+      def reset!
+        FOREMAN_MUTEX.synchronize do
+          finish_connection(:@foreman_http)
+          ForemanRequest.instance_variable_set(:@connection_uri, nil)
+        end
+        REGISTRATION_GET_MUTEX.synchronize do
+          finish_connection(:@registration_get_http)
+        end
+      end
+
+      private
+
+      def foreman_http
+        ForemanRequest.instance_variable_get(:@foreman_http) ||
+          ForemanRequest.instance_variable_set(:@foreman_http, http_init)
+      end
+
+      def registration_get_http
+        ForemanRequest.instance_variable_get(:@registration_get_http) ||
+          ForemanRequest.instance_variable_set(:@registration_get_http, http_init)
+      end
+
+      def finish_connection(ivar)
+        old = ForemanRequest.instance_variable_get(ivar)
+        ForemanRequest.instance_variable_set(ivar, nil)
+        old&.finish
+      rescue IOError, SystemCallError
+        nil
+      end
+
+      def setting(name, default)
+        value = Proxy::SETTINGS.respond_to?(name) ? Proxy::SETTINGS.public_send(name).to_i : 0
+        (value > 0) ? value : default
+      end
+
+      def connection_uri
+        ForemanRequest.instance_variable_get(:@connection_uri) ||
+          ForemanRequest.instance_variable_set(:@connection_uri, URI.parse(Proxy::SETTINGS.foreman_url.to_s))
+      end
+
+      def http_init
+        uri = connection_uri
+        http             = Net::HTTP.new(uri.host, uri.port)
+        http.use_ssl     = uri.scheme == 'https'
+        http.verify_mode = OpenSSL::SSL::VERIFY_NONE
+        http.keep_alive_timeout = setting(:foreman_keep_alive_timeout, DEFAULT_KEEP_ALIVE_TIMEOUT)
+        http.read_timeout = setting(:foreman_request_timeout, DEFAULT_READ_TIMEOUT)
+        timeout = setting(:foreman_connect_timeout, 0)
+        http.open_timeout = timeout if timeout > 0
+
+        if http.use_ssl?
+          ca_file = Proxy::SETTINGS.foreman_ssl_ca || Proxy::SETTINGS.ssl_ca_file
+          certificate = Proxy::SETTINGS.foreman_ssl_cert || Proxy::SETTINGS.ssl_certificate
+          private_key = Proxy::SETTINGS.foreman_ssl_key || Proxy::SETTINGS.ssl_private_key
+
+          if ca_file && !ca_file.to_s.empty?
+            http.ca_file     = ca_file
+            http.verify_mode = OpenSSL::SSL::VERIFY_PEER
+          end
+
+          if certificate && !certificate.to_s.empty? && private_key && !private_key.to_s.empty?
+            http.cert = OpenSSL::X509::Certificate.new(File.read(certificate))
+            http.key  = OpenSSL::PKey.read(File.read(private_key), nil)
+          end
+        end
+
+        http
+      end
+    end
+
     def send_request(request)
-      http.request(request)
+      execute_with(FOREMAN_MUTEX, :@foreman_http, request)
+    end
+
+    def send_request_direct(request)
+      execute_with(REGISTRATION_GET_MUTEX, :@registration_get_http, request)
     end
 
     def request_factory
@@ -57,32 +148,26 @@ def uri
     end
 
     def http
-      @http ||= http_init
+      self.class.send(:foreman_http)
     end
 
     private
 
-    def http_init
-      http             = Net::HTTP.new(uri.host, uri.port)
-      http.use_ssl     = uri.scheme == 'https'
-      http.verify_mode = OpenSSL::SSL::VERIFY_NONE
-
-      if http.use_ssl?
-        ca_file = Proxy::SETTINGS.foreman_ssl_ca || Proxy::SETTINGS.ssl_ca_file
-        certificate = Proxy::SETTINGS.foreman_ssl_cert || Proxy::SETTINGS.ssl_certificate
-        private_key = Proxy::SETTINGS.foreman_ssl_key || Proxy::SETTINGS.ssl_private_key
-
-        if ca_file && !ca_file.to_s.empty?
-          http.ca_file     = ca_file
-          http.verify_mode = OpenSSL::SSL::VERIFY_PEER
-        end
-
-        if certificate && !certificate.to_s.empty? && private_key && !private_key.to_s.empty?
-          http.cert = OpenSSL::X509::Certificate.new(File.read(certificate))
-          http.key  = OpenSSL::PKey.read(File.read(private_key), nil)
+    def execute_with(mutex, ivar, request)
+      mutex.synchronize do
+        http = ForemanRequest.instance_variable_get(ivar) || ForemanRequest.instance_variable_set(ivar, self.class.send(:http_init))
+        begin
+          http.start unless http.started?
+          http.request(request)
+        rescue *RETRY_EXCEPTIONS
+          begin
+            http.finish
+          rescue IOError, SystemCallError # rubocop:disable Lint/SuppressedException
+          end
+          ForemanRequest.instance_variable_set(ivar, self.class.send(:http_init))
+          raise
         end
       end
-      http
     end
   end
 end

modules/registration/proxy_request.rb

@@ -7,30 +7,41 @@ def global_register(request)
                                              request_params(request),
                                              headers(request)
 
-      send_request(proxy_req)
+      send_request_direct(proxy_req)
     end
 
-    # we support two way of sending data - either a JSON or url encoded data
+    # Retries once on transport errors. Foreman's POST /register uses
+    # find_or_initialize_by for the host record, making a single replay
+    # safe after a stale keep-alive connection failure.
+    REGISTER_RETRY_EXCEPTIONS = [
+      EOFError,
+      Errno::ECONNRESET,
+      Errno::EPIPE,
+      IOError,
+      OpenSSL::SSL::SSLError,
+    ].freeze
+
     def host_register(request)
+      body = request.body.read
+      hdrs = headers(request)
+
       if request.content_type == 'application/x-www-form-urlencoded'
-        # the request has a different content type, ForemanRequestFactory sets content type to json, unless
-        # specified explicitly
-        # also request.params contain the same data that is in request.body, just parsed to hash,
-        # in case they are nested (e.g. host hash) we need this causes problem during CGI escaping
-        # therefore we only add url, everything else should be in body in this type of request
-        proxy_req = request_factory.create_post '/register',
-                                                request.body.read,
-                                                headers(request).merge("Content-Type" => request.content_type),
-                                                { url: register_url(request) }
+        content_type = request.content_type
+        query = { url: register_url(request) }
+        build_req = -> { request_factory.create_post('/register', body, hdrs.merge("Content-Type" => content_type), query) }
       else
-        # the application/json request body contains the data - JSON as a string, query contains only the URL
-        proxy_req = request_factory.create_post '/register',
-                                                request.body.read,
-                                                headers(request),
-                                                request_params(request)
+        params = request_params(request)
+        build_req = -> { request_factory.create_post('/register', body, hdrs, params) }
       end
 
-      send_request(proxy_req)
+      retried = false
+      begin
+        send_request(build_req.call)
+      rescue *REGISTER_RETRY_EXCEPTIONS
+        raise if retried
+        retried = true
+        retry
+      end
     end
 
     private

test/request_test.rb

@@ -1,8 +1,12 @@
 require 'test_helper'
 require 'uri'
 require 'net/http'
+require 'openssl'
+require 'tempfile'
 require 'mocha'
 require 'templates/templates_plugin'
+require 'templates/proxy_request'
+require 'registration/proxy_request'
 require "proxy/util"
 require 'proxy/request'
 require 'webmock/test_unit'
@@ -11,6 +15,16 @@ class RequestTest < Test::Unit::TestCase
   def setup
     @foreman_url = 'https://foreman.example.com'
     Proxy::SETTINGS.stubs(:foreman_url).returns(@foreman_url)
+    Proxy::SETTINGS.stubs(:foreman_ssl_ca).returns(nil)
+    Proxy::SETTINGS.stubs(:ssl_ca_file).returns(nil)
+    Proxy::SETTINGS.stubs(:foreman_ssl_cert).returns(nil)
+    Proxy::SETTINGS.stubs(:ssl_certificate).returns(nil)
+    Proxy::SETTINGS.stubs(:foreman_ssl_key).returns(nil)
+    Proxy::SETTINGS.stubs(:ssl_private_key).returns(nil)
+    Proxy::SETTINGS.stubs(:foreman_request_timeout).returns(nil)
+    Proxy::SETTINGS.stubs(:foreman_connect_timeout).returns(nil)
+    Proxy::SETTINGS.stubs(:foreman_keep_alive_timeout).returns(nil)
+    Proxy::HttpRequest::ForemanRequest.reset!
     @template_url = 'http://proxy.lan:8443'
     Proxy::Templates::Plugin.load_test_settings(:template_url => @template_url)
     @request = Proxy::HttpRequest::ForemanRequest.new
@@ -54,6 +68,110 @@ def test_post
     assert_equal("body", result.body)
   end
 
+  def test_connection_is_shared_across_request_instances
+    request_a = Proxy::HttpRequest::ForemanRequest.new
+    request_b = Proxy::HttpRequest::ForemanRequest.new
+
+    assert_same request_a.http, request_b.http
+    assert_kind_of Net::HTTP, request_a.http
+  end
+
+  def test_connection_is_shared_across_foreman_request_subclasses
+    registration_request = Proxy::Registration::ProxyRequest.new
+    template_request = Proxy::Templates::ProxyRequest.new
+
+    assert_same registration_request.http, template_request.http
+  end
+
+  def test_ssl_verify_none_when_no_ca_configured
+    assert_equal OpenSSL::SSL::VERIFY_NONE, @request.http.verify_mode
+  end
+
+  def test_ssl_verify_peer_when_ca_configured
+    ca_file = Tempfile.new(['foreman-ca', '.pem'])
+    ca_file.write("ca-content")
+    ca_file.flush
+
+    Proxy::SETTINGS.stubs(:foreman_ssl_ca).returns(ca_file.path)
+    Proxy::HttpRequest::ForemanRequest.reset!
+
+    connection = Proxy::HttpRequest::ForemanRequest.new.http
+
+    assert_equal OpenSSL::SSL::VERIFY_PEER, connection.verify_mode
+    assert_equal ca_file.path, connection.ca_file
+  ensure
+    ca_file&.close
+    ca_file&.unlink
+  end
+
+  def test_read_timeout_uses_default
+    assert_equal 200, @request.http.read_timeout
+  end
+
+  def test_read_timeout_configurable
+    Proxy::SETTINGS.stubs(:foreman_request_timeout).returns(120)
+    Proxy::HttpRequest::ForemanRequest.reset!
+    request = Proxy::HttpRequest::ForemanRequest.new
+    assert_equal 120, request.http.read_timeout
+  end
+
+  def test_keep_alive_timeout_uses_default
+    assert_equal 10, @request.http.keep_alive_timeout
+  end
+
+  def test_keep_alive_timeout_configurable
+    Proxy::SETTINGS.stubs(:foreman_keep_alive_timeout).returns(8)
+    Proxy::HttpRequest::ForemanRequest.reset!
+    request = Proxy::HttpRequest::ForemanRequest.new
+    assert_equal 8, request.http.keep_alive_timeout
+  end
+
+  def test_connect_timeout_not_set_by_default
+    default = Net::HTTP.new('example.com').open_timeout
+    assert_equal default, @request.http.open_timeout
+  end
+
+  def test_connect_timeout_configurable
+    Proxy::SETTINGS.stubs(:foreman_connect_timeout).returns(30)
+    Proxy::HttpRequest::ForemanRequest.reset!
+    request = Proxy::HttpRequest::ForemanRequest.new
+    assert_equal 30, request.http.open_timeout
+  end
+
+  def test_send_request_and_send_request_direct_use_different_connections
+    stub_request(:get, @foreman_url + '/path').to_return(:status => [200, 'OK'], :body => "body")
+    proxy_req = @request.request_factory.create_get("/path")
+
+    @request.send_request(proxy_req)
+    foreman_http = Proxy::HttpRequest::ForemanRequest.instance_variable_get(:@foreman_http)
+
+    @request.send_request_direct(proxy_req)
+    registration_get_http = Proxy::HttpRequest::ForemanRequest.instance_variable_get(:@registration_get_http)
+
+    assert_not_same foreman_http, registration_get_http
+  end
+
+  def test_transport_error_replaces_connection_and_raises
+    stub_request(:get, @foreman_url + '/path').to_raise(EOFError)
+    proxy_req = @request.request_factory.create_get("/path")
+
+    assert_raises(EOFError) { @request.send_request(proxy_req) }
+    http_before = Proxy::HttpRequest::ForemanRequest.instance_variable_get(:@foreman_http)
+
+    stub_request(:get, @foreman_url + '/path').to_raise(EOFError)
+    assert_raises(EOFError) { @request.send_request(proxy_req) }
+
+    http_after = Proxy::HttpRequest::ForemanRequest.instance_variable_get(:@foreman_http)
+    assert_not_same http_before, http_after
+  end
+
+  def test_send_request_direct_succeeds
+    stub_request(:get, @foreman_url + '/path').to_return(:status => [200, 'OK'], :body => "body")
+    proxy_req = @request.request_factory.create_get("/path")
+    result = @request.send_request_direct(proxy_req)
+    assert_equal("body", result.body)
+  end
+
   def test_post_with_nested_params
     stub_request(:post, @foreman_url + '/register?activation_keys%5B%5D=ac_AlmaLinux8&location_id=2&organization_id=1&repo_data%5B%5D%5Brepo%5D=repo1&repo_data%5B%5D%5Brepo_gpg_key_url%5D=key1&repo_data%5B%5D%5Brepo%5D=repo2&repo_data%5B%5D%5Brepo_gpg_key_url%5D=key2&update_packages=false')
       .to_return(status: 200, body: "body", headers: {h1: "header"})