From 04b4a90d1634eafb03c86f4375df63bbc99f40d7 Mon Sep 17 00:00:00 2001 From: Adam Ruzicka Date: Mon, 20 May 2024 10:32:10 +0200 Subject: [PATCH] Fixes #37487 - Refactor proxy selection The primary reason for this is to avoid unnecessary proxy calls and version comparisons. All currently supported versions of Foreman already ship with smart_proxy_openscap >= 0.5 so we can skip the check altogether. --- app/lib/proxy_api/available_proxy.rb | 44 ------------------- app/lib/proxy_api/openscap.rb | 11 +++++ .../foreman_openscap/data_stream_content.rb | 6 +-- .../foreman_openscap/data_stream_validator.rb | 2 +- lib/foreman_openscap/data_migration.rb | 13 +++--- test/unit/scap_content_test.rb | 5 +-- 6 files changed, 22 insertions(+), 59 deletions(-) delete mode 100644 app/lib/proxy_api/available_proxy.rb diff --git a/app/lib/proxy_api/available_proxy.rb b/app/lib/proxy_api/available_proxy.rb deleted file mode 100644 index b73d7940f..000000000 --- a/app/lib/proxy_api/available_proxy.rb +++ /dev/null @@ -1,44 +0,0 @@ -module ::ProxyAPI - class AvailableProxy - HTTP_ERRORS = [ - EOFError, - Errno::ECONNRESET, - Errno::EINVAL, - Net::HTTPBadResponse, - Net::HTTPHeaderSyntaxError, - Net::ProtocolError, - Timeout::Error, - ProxyAPI::ProxyException - ].freeze - - def initialize(args) - @args = args - end - - def available? - begin - return true if has_scap_feature? && minimum_version - rescue *HTTP_ERRORS - return false - end - false - end - - private - - def has_scap_feature? - @features ||= ::ProxyAPI::Features.new(@args).features - @features.include?('openscap') - end - - def openscap_proxy_version - @versions ||= ::ProxyAPI::Version.new(@args).proxy_versions['modules'] - @versions['openscap'] if @versions && @versions['openscap'] - end - - def minimum_version - return false unless openscap_proxy_version - openscap_proxy_version.to_f >= 0.5 - end - end -end diff --git a/app/lib/proxy_api/openscap.rb b/app/lib/proxy_api/openscap.rb index 876b21ad5..cf3004ab0 100644 --- a/app/lib/proxy_api/openscap.rb +++ b/app/lib/proxy_api/openscap.rb @@ -1,5 +1,16 @@ module ::ProxyAPI class Openscap < ::ProxyAPI::Resource + HTTP_ERRORS = [ + EOFError, + Errno::ECONNRESET, + Errno::EINVAL, + Net::HTTPBadResponse, + Net::HTTPHeaderSyntaxError, + Net::ProtocolError, + Timeout::Error, + ProxyAPI::ProxyException + ].freeze + def initialize(args) @url = args[:url] + '/compliance/' super args diff --git a/app/models/concerns/foreman_openscap/data_stream_content.rb b/app/models/concerns/foreman_openscap/data_stream_content.rb index c6dbf9b2d..cd08039ae 100644 --- a/app/models/concerns/foreman_openscap/data_stream_content.rb +++ b/app/models/concerns/foreman_openscap/data_stream_content.rb @@ -10,11 +10,7 @@ module DataStreamContent end def proxy_url - @proxy_url ||= SmartProxy.with_features('Openscap').find do |proxy| - available = ProxyAPI::AvailableProxy.new(:url => proxy.url) - available.available? - end.try(:url) - @proxy_url + @proxy_url ||= SmartProxy.with_features('Openscap').find(&:ping)&.url end def create_profiles diff --git a/app/validators/foreman_openscap/data_stream_validator.rb b/app/validators/foreman_openscap/data_stream_validator.rb index d0b16b75c..719fea162 100644 --- a/app/validators/foreman_openscap/data_stream_validator.rb +++ b/app/validators/foreman_openscap/data_stream_validator.rb @@ -22,7 +22,7 @@ def validate(data_stream_content) errors['errors'].each { |error| data_stream_content.errors.add(:scap_file, _(error)) } return false end - rescue *ProxyAPI::AvailableProxy::HTTP_ERRORS => e + rescue *ProxyAPI::Openscap::HTTP_ERRORS => e data_stream_content.errors.add(:base, _('No available proxy to validate. Returned with error: %s') % e) return false end diff --git a/lib/foreman_openscap/data_migration.rb b/lib/foreman_openscap/data_migration.rb index 37601142d..45d0e6762 100644 --- a/lib/foreman_openscap/data_migration.rb +++ b/lib/foreman_openscap/data_migration.rb @@ -6,14 +6,15 @@ module ForemanOpenscap class DataMigration def initialize(proxy_id) - @proxy = ::SmartProxy.find(proxy_id) - puts "Found proxy #{@proxy.to_label}" - @url = @proxy.url + @proxy = ::SmartProxy.with_features('Openscap').where(id: proxy_id).first + if @proxy + puts "Found proxy #{@proxy.to_label}" + @url = @proxy.url + end end def available? - return false unless @proxy && @url - ::ProxyAPI::AvailableProxy.new(:url => @url).available? && foreman_available? + @proxy&.ping && foreman_available? end def migrate @@ -47,7 +48,7 @@ def foreman_available? foreman_status_url = Setting[:foreman_url] + '/status' response = JSON.parse(RestClient.get(foreman_status_url)) return true if response["status"] == "ok" - rescue *::ProxyAPI::AvailableProxy::HTTP_ERRORS + rescue *::ProxyAPI::Openscap::HTTP_ERRORS return false end diff --git a/test/unit/scap_content_test.rb b/test/unit/scap_content_test.rb index 2146fe501..dafc8d4dd 100644 --- a/test/unit/scap_content_test.rb +++ b/test/unit/scap_content_test.rb @@ -17,7 +17,6 @@ class ScapContentTest < ActiveSupport::TestCase test 'scap content should fail if no openscap proxy' do SmartProxy.stubs(:with_features).returns([]) - ProxyAPI::AvailableProxy.any_instance.stubs(:available?).returns(false) scap_content = ForemanOpenscap::ScapContent.new(:title => 'Fedora', :scap_file => @scap_file) refute(scap_content.save) assert_includes(scap_content.errors.messages[:base], 'No proxy with OpenSCAP feature was found.') @@ -26,8 +25,8 @@ class ScapContentTest < ActiveSupport::TestCase test 'proxy_url should return the first available proxy it finds' do available_proxy = SmartProxy.with_features('Openscap').first unavailable_proxy = FactoryBot.create(:smart_proxy, :url => 'http://proxy.example.com:8443', :features => [FactoryBot.create(:feature, :name => 'Openscap')]) - available_proxy.stubs(:proxy_url).returns(available_proxy.url) - unavailable_proxy.stubs(:proxy_url).returns(nil) + SmartProxy.expects(:with_features).with('Openscap').returns([unavailable_proxy, available_proxy]) + SmartProxy.any_instance.expects(:ping).twice.returns(false).then.returns(true) scap_content = ForemanOpenscap::ScapContent.new(:title => 'Fedora', :scap_file => @scap_file) assert_equal(available_proxy.url, scap_content.proxy_url) end