diff --git a/modules/registration/registration_api.rb b/modules/registration/registration_api.rb index c4ad22273..b26837ae1 100644 --- a/modules/registration/registration_api.rb +++ b/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 diff --git a/test/registration/registration_api_test.rb b/test/registration/registration_api_test.rb index bd9ff7b1b..71ff6f828 100644 --- a/test/registration/registration_api_test.rb +++ b/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