Fixes #39208 - Cache global registration script in smart-proxy

modules/registration/registration_api.rb

@@ -1,9 +1,51 @@
 require 'registration/proxy_request'
 
 class Proxy::Registration::Api < ::Sinatra::Base
+  # Cache for the global registration script (GET /register).
+  #
+  # The script is identical for all hosts sharing the same registration
+  # parameters (org, location, hostgroup, activation keys), making it safe
+  # to serve from an in-memory cache during concurrent bulk registration.
+  #
+  # Per-key double-checked locking prevents thundering herd while allowing
+  # genuinely independent cache keys (e.g. different activation keys) to
+  # fetch from Foreman in parallel.
+  #
+  # Only HTTP 200 responses are cached — errors are raised out of the cache
+  # block so they never poison the cache. The per-key mutex is evicted
+  # immediately after caching so KEY_MUTEXES stays bounded to keys currently
+  # being fetched for the first time (zero under steady state).
+  REGISTRATION_SCRIPT_CACHE_TTL = 5 * 60 # seconds
+  KEY_MUTEXES                   = Concurrent::Map.new
+  SCRIPT_CACHE                  = Concurrent::Map.new
+
+  class ScriptFetchError < StandardError
+    attr_reader :response
+
+    def initialize(response)
+      super()
+      @response = response
+    end
+  end
+
+  class << self
+    def registration_script_cache
+      SCRIPT_CACHE
+    end
+
+    def key_mutex(cache_key)
+      KEY_MUTEXES.compute_if_absent(cache_key) { Mutex.new }
+    end
+
+    def evict_key_mutex(cache_key)
+      KEY_MUTEXES.delete(cache_key)
+    end
+  end
+
   get '/' do
-    response = Proxy::Registration::ProxyRequest.new.global_register(request)
-    handle_response(response)
+    registration_script
+  rescue ScriptFetchError => e
+    handle_response(e.response)
   rescue StandardError => e
     logger.exception "Error when rendering Global Registration Template", e
     render_error(default_error_msg)
@@ -19,11 +61,41 @@ class Proxy::Registration::Api < ::Sinatra::Base
 
   private
 
+  def registration_script
+    cache_key = Rack::Utils.build_query(
+      Rack::Utils.parse_nested_query(request.query_string).sort_by { |k, _| k }
+    )
+    cache(cache_key) do
+      response = Proxy::Registration::ProxyRequest.new.global_register(request)
+      raise ScriptFetchError, response unless response.code == '200'
+      response.body
+    end
+  end
+
+  def cache(key, &block)
+    value = read_registration_cache(key)
+    return value if value
+
+    self.class.key_mutex(key).synchronize do
+      value = read_registration_cache(key)
+      return value if value
+
+      result = yield
+      self.class.registration_script_cache[key] = { body: result, at: Time.now }
+      self.class.evict_key_mutex(key)
+      result
+    end
+  end
+
+  def read_registration_cache(cache_key)
+    entry = self.class.registration_script_cache[cache_key]
+    entry[:body] if entry && (Time.now - entry[:at]) < REGISTRATION_SCRIPT_CACHE_TTL
+  end
+
   def handle_response(response)
-    if response.code.start_with? '2'
+    if response.code.start_with?('2')
       response.body
     else
-      # Return error message only if it is not HTML.
       message = response["content-type"].include?('text/plain') ? response.body : default_error_msg
       render_error(message, code: response.code)
     end

test/registration/registration_api_test.rb

@@ -11,6 +11,9 @@ def app
   def setup
     @foreman_url = 'http://foreman.example.com'
     Proxy::SETTINGS.stubs(:foreman_url).returns(@foreman_url)
+    # Clear class-level state between tests to prevent cross-test contamination
+    Proxy::Registration::Api.registration_script_cache.clear
+    Proxy::Registration::Api::KEY_MUTEXES.clear
   end
 
   def test_global_register_template
@@ -102,6 +105,86 @@ def test_global_500
     assert_match("echo \"Internal Server Error\"\nexit 1\n", last_response.body)
   end
 
+  def test_global_register_caches_response
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template')
+
+    2.times do
+      get '/'
+      assert last_response.ok?
+      assert_match('template', last_response.body)
+    end
+
+    assert_requested stub, times: 1
+  end
+
+  def test_global_register_cache_key_is_parameter_order_independent
+    # Cache key is normalised (params sorted alphabetically), so both orderings
+    # produce activation_keys=rhel9&owner=Default_Organization and share one entry.
+    stub_request(:get, "#{@foreman_url}/register?activation_keys=rhel9&owner=Default_Organization")
+      .to_return(body: 'template')
+
+    get '/', { owner: 'Default_Organization', activation_keys: 'rhel9' }
+    assert last_response.ok?
+
+    # Different parameter order — must hit cache, not Foreman again
+    get '/', { activation_keys: 'rhel9', owner: 'Default_Organization' }
+    assert last_response.ok?
+
+    assert_requested :get, "#{@foreman_url}/register?activation_keys=rhel9&owner=Default_Organization", times: 1
+  end
+
+  def test_global_register_caches_per_key
+    stub_a = stub_request(:get, "#{@foreman_url}/register?key=a").to_return(body: 'template_a')
+    stub_b = stub_request(:get, "#{@foreman_url}/register?key=b").to_return(body: 'template_b')
+
+    get '/', { key: 'a' }
+    assert_match('template_a', last_response.body)
+    get '/', { key: 'b' }
+    assert_match('template_b', last_response.body)
+    # second requests — must be served from cache
+    get '/', { key: 'a' }
+    assert_match('template_a', last_response.body)
+    get '/', { key: 'b' }
+    assert_match('template_b', last_response.body)
+
+    assert_requested stub_a, times: 1
+    assert_requested stub_b, times: 1
+  end
+
+  def test_global_register_evicts_mutex_after_caching
+    stub_request(:get, "#{@foreman_url}/register").to_return(body: 'template')
+
+    get '/'
+    assert last_response.ok?
+    assert_empty Proxy::Registration::Api::KEY_MUTEXES
+  end
+
+  def test_global_register_cache_entry_expires_after_ttl
+    Proxy::Registration::Api.registration_script_cache[''] = {
+      body: 'stale-template',
+      at: Time.now - Proxy::Registration::Api::REGISTRATION_SCRIPT_CACHE_TTL - 1,
+    }
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(body: 'fresh-template')
+
+    get '/'
+    assert last_response.ok?
+    assert_match('fresh-template', last_response.body)
+    assert_requested stub, times: 1
+  end
+
+  def test_global_register_does_not_cache_errors
+    stub = stub_request(:get, "#{@foreman_url}/register").to_return(
+      body: 'error', status: 500, headers: { "Content-Type" => 'text/plain' }
+    )
+
+    2.times do
+      get '/'
+      assert last_response.server_error?
+    end
+
+    assert_requested stub, times: 2
+  end
+
   def test_host_500
     Rack::NullLogger.any_instance.stubs(:exception)
     stub_request(:post, "#{@foreman_url}/register").to_timeout