From 30bf6a128a42b11813f383abf04861900111f25e Mon Sep 17 00:00:00 2001 From: Juan Hernandez Date: Mon, 6 Nov 2017 15:36:06 +0100 Subject: [PATCH] Try both API versions in `raw_connect` Currently the `raw_connect` method needs to receive the version of the API in order to work correctly. If it doesn't receive it it fails because the method that determines it just doesn't exist (it is an instance method, but `raw_connect` is a class method). To avoid that this patch changes the `raw_connect` method so that it tries with both versions of the API. This patch fixes partially the following bug: Failed validation when adding RHV provider https://bugzilla.redhat.com/1509432 --- .../redhat/infra_manager/api_integration.rb | 152 ++++++++++++------ .../redhat/infra_manager/event_parser_spec.rb | 16 +- .../redhat/infra_manager/provision_spec.rb | 8 +- .../refresh/refresher_3_1_spec.rb | 8 +- .../refresh/refresher_target_host_4_spec.rb | 10 +- .../refresh/refresher_target_vm_spec.rb | 18 ++- .../providers/redhat/infra_manager/vm_spec.rb | 9 +- .../providers/redhat/infra_manager_spec.rb | 38 +++-- 8 files changed, 191 insertions(+), 68 deletions(-) diff --git a/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb b/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb index e18ff9497..c1ef0af07 100644 --- a/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb +++ b/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb @@ -41,8 +41,8 @@ def connect(options = {}) # Starting with version 4 of oVirt authentication doesn't work when using directly the IP address, it requires # the fully qualified host name, so if we received an IP address we try to convert it into the corresponding # host name: - if resolve_ip_addresses? - resolved = resolve_ip_address(connect_options[:server]) + if self.class.resolve_ip_addresses? + resolved = self.class.resolve_ip_address(connect_options[:server]) if resolved != connect_options[:server] _log.info("IP address '#{connect_options[:server]}' has been resolved to host name '#{resolved}'.") default_endpoint.hostname = resolved @@ -232,17 +232,78 @@ def process_api_features_support end end - def raw_connect(options) - version = options[:version] || highest_allowed_api_version - options[:password] = MiqPassword.try_decrypt(options[:password]) - connect_method = "raw_connect_v#{version}".to_sym + # + # This method is called only when the UI button to verify the connection details is clicked. It isn't used create + # the connections actually used by the provider. + # + # Note that the protocol (HTTP or HTTPS) and the version of the API are *not* options of this method, if they are + # provided they will be silently ignored. + # + # @param opts [Hash] A hash containing the connection details and the credentials. + # @option opts [String] :username The name of the user. + # @option opts [String] :password The password of the user. + # @option opts [String] :server The host name or IP address of the server. + # @option opts [Integer] :port ('443') The port number of the server. + # @option opts [Integer] :verify_ssl ('1') A numeric flag indicating if TLS certificates should be checked. Value + # `0` indicates that the should not be checked, value `1` indicates that they should be checked. + # @option opts [String] :ca_certs The custom trusted CA certificates, in PEM format. A blank or nil value means + # that no custom CA certificates should be used. + # @return [Boolean] Returns `true` if the connection details and credentials are valid, or `false` otherwise. + # + def raw_connect(opts = {}) + # Get options and assign default values: + username = opts[:username] + password = opts[:password] + server = opts[:server] + port = opts[:port] || 443 + verify_ssl = opts[:verify_ssl] || 1 + ca_certs = opts[:ca_certs] + + # Decrypt the password: + password = MiqPassword.try_decrypt(password) + + # Starting with version 4 of oVirt authentication doesn't work when using directly the IP address, it requires + # the fully qualified host name, so if we received an IP address we try to convert it into the corresponding + # host name: + resolved = server + if resolve_ip_addresses? + resolved = resolve_ip_address(server) + if resolved != server + _log.info("IP address '#{server}' has been resolved to host name '#{resolved}'.") + end + end + + # Build the options that will be used to call the methods that create the connection with specific versions + # of the API: + opts = { + :username => username, + :password => password, + :server => resolved, + :port => port, + :verify_ssl => verify_ssl, + :ca_certs => ca_certs, + :service => 'Inventory' # This is needed only for version 3 of the API. + } + # Try to verify the details using version 4 of the API. If this succeeds or fails with an authentication + # exception, then we don't need to do anything else. Note that the connection should not be closed, because + # that is handled by the `ConnectionManager` class. begin - connection = public_send(connect_method, options) - connection.test(true) + connection = raw_connect_v4(opts) + connection.test(:raise_exception => true) + return true + rescue OvirtSDK4::Error => error + raise error if /error.*sso/i =~ error.message + end + + # Try to verify the details using version 3 of the API. + begin + connection = raw_connect_v3(opts) + connection.api ensure disconnect(connection) end + true end # Connect to the engine using version 4 of the API and the `ovirt-engine-sdk` gem. @@ -260,10 +321,10 @@ def raw_connect_v4(options = {}) ca_certs = [ca_certs] if ca_certs url = URI::Generic.build( - :scheme => options[:scheme], + :scheme => 'https', :host => options[:server], :port => options[:port], - :path => options[:path] || '/ovirt-engine/api' + :path => '/ovirt-engine/api' ) ManageIQ::Providers::Redhat::ConnectionManager.instance.get( @@ -295,7 +356,7 @@ def raw_connect_v3(options = {}) params = { :server => options[:server], :port => options[:port].presence && options[:port].to_i, - :path => options[:path], + :path => '/ovirt-engine/api', :username => options[:username], :password => options[:password], :verify_ssl => options[:verify_ssl], @@ -333,42 +394,43 @@ def make_ems_ref(href) def extract_ems_ref_id(href) href && href.split("/").last end - end + # + # Checks if IP address to host name resolving is enabled. + # + # @return [Boolean] `true` if host name resolving is enabled in the configuration, `false` otherwise. + # + # @api private + # + def resolve_ip_addresses? + ::Settings.ems.ems_redhat.resolve_ip_addresses + end - private - - # - # Checks if IP address to host name resolving is enabled. - # - # @return [Boolean] `true` if host name resolving is enabled in the configuration, `false` otherwise. - # - def resolve_ip_addresses? - ::Settings.ems.ems_redhat.resolve_ip_addresses - end - - # - # Tries to convert the given IP address into a host name, doing a reverse DNS lookup if needed. If it - # isn't possible to find the host name the original IP address will be returned, and a warning will be - # written to the log. - # - # @param address [String] The IP address. - # @return [String] The host name. - # - def resolve_ip_address(address) - # Don't try to resolve unless the string is really an IP address and not a host name: - return address unless address =~ Resolv::IPv4::Regex || address =~ Resolv::IPv6::Regex - - # Try to do a reverse resolve of the address to find the host name, using the default resolver, which - # means first using the local hosts file and then DNS: - begin - Resolv.getname(address) - rescue Resolv::ResolvError - _log.warn( - "Can't find fully qualified host name for IP address '#{address}', will use the IP address " \ - "directly." - ) - address + # + # Tries to convert the given IP address into a host name, doing a reverse DNS lookup if needed. If it + # isn't possible to find the host name the original IP address will be returned, and a warning will be + # written to the log. + # + # @param address [String] The IP address. + # @return [String] The host name. + # + # @api private + # + def resolve_ip_address(address) + # Don't try to resolve unless the string is really an IP address and not a host name: + return address unless address =~ Resolv::IPv4::Regex || address =~ Resolv::IPv6::Regex + + # Try to do a reverse resolve of the address to find the host name, using the default resolver, which + # means first using the local hosts file and then DNS: + begin + Resolv.getname(address) + rescue Resolv::ResolvError + _log.warn( + "Can't find fully qualified host name for IP address '#{address}', will use the IP address " \ + "directly." + ) + address + end end end end diff --git a/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb index ca9897c22..6f6713021 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb @@ -9,7 +9,13 @@ @ems.update_authentication(:default => {:userid => "admin@internal", :password => "engine"}) @ems.default_endpoint.verify_ssl = OpenSSL::SSL::VERIFY_NONE allow(@ems).to receive(:supported_api_versions).and_return([3]) - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) end it "should parse event" do @@ -153,7 +159,13 @@ @ems.update_authentication(:default => {:userid => "admin@internal", :password => "engine"}) @ems.default_endpoint.path = "/ovirt-engine/api" allow(@ems).to receive(:supported_api_versions).and_return([3, 4]) - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) end require 'yaml' diff --git a/spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb index 2c86cd1d0..4bec26fa9 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb @@ -110,7 +110,13 @@ context "with a destination vm" do let(:destination_vm) { FactoryGirl.create(:vm_redhat, :ext_management_system => @ems) } before do - allow(@ems).to receive(:resolve_ip_address).with(@ems.hostname).and_return("1.2.3.4") + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) rhevm_vm = double(:attributes => {:status => {:state => "down"}}) allow(@vm_prov).to receive(:with_provider_destination).and_yield(rhevm_vm) allow(@vm_prov).to receive(:destination).and_return(destination_vm) diff --git a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_3_1_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_3_1_spec.rb index 2ae9f0bfc..c1e10aea4 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_3_1_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_3_1_spec.rb @@ -7,7 +7,13 @@ @ems.update_authentication(:default => {:userid => "evm@manageiq.com", :password => "password"}) allow(@ems).to receive(:supported_api_versions).and_return([3]) allow(@ems).to receive(:highest_allowed_api_version).and_return('3') - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) end it ".ems_type" do diff --git a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_host_4_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_host_4_spec.rb index 363dc6436..2723a8f6c 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_host_4_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_host_4_spec.rb @@ -13,8 +13,14 @@ @ems.update_authentication(:default => {:userid => "admin@internal", :password => "engine"}) @ems.default_endpoint.path = "/ovirt-engine/api" allow(@ems).to receive(:supported_api_versions).and_return([3, 4]) - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) - stub_settings_merge(:ems => { :ems_redhat => { :use_ovirt_engine_sdk => true } }) + stub_settings_merge( + :ems => { + :ems_redhat => { + :use_ovirt_engine_sdk => true, + :resolve_ip_addresses => false + } + } + ) allow(Spec::Support::OvirtSDK::ConnectionVCR).to receive(:new).and_call_original allow(Spec::Support::OvirtSDK::ConnectionVCR).to receive(:new).with(kind_of(Hash)) do |opts| Spec::Support::OvirtSDK::ConnectionVCR.new(opts, 'spec/vcr_cassettes/manageiq/providers/redhat/infra_manager/refresh/refresher_target_host.yml') diff --git a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_spec.rb index 0e3ca1524..51bd0ca2e 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_spec.rb @@ -46,8 +46,14 @@ :name => "Default") allow(@ems).to receive(:supported_api_versions).and_return([3]) - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) - stub_settings_merge(:ems => { :ems_redhat => { :use_ovirt_engine_sdk => false } }) + stub_settings_merge( + :ems => { + :ems_redhat => { + :use_ovirt_engine_sdk => false, + :resolve_ip_addresses => false + } + } + ) end it "should refresh a vm" do @@ -120,7 +126,13 @@ :hostname => ip_address, :ipaddress => ip_address, :port => 443) @ems.update_authentication(:default => {:userid => "admin@internal", :password => "password"}) allow(@ems).to receive(:supported_api_versions).and_return(['3']) - allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) end it 'should save the vms new host' do diff --git a/spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb index a0a2566d1..d9c4784ac 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb @@ -167,8 +167,13 @@ ems.update_authentication(:default => {:userid => "admin@internal", :password => "engine"}) # TODO: (inventory) resvisit this test and write one for V4 allow(ems).to receive(:supported_api_versions).and_return([3]) - allow(ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address) - + stub_settings_merge( + :ems => { + :ems_redhat => { + :resolve_ip_addresses => false + } + } + ) @storage = FactoryGirl.create(:storage, :ems_ref => "/api/storagedomains/ee745353-c069-4de8-8d76-ec2e155e2ca0") disk = FactoryGirl.create(:disk, :storage => @storage, :filename => "da123bb9-095a-4933-95f2-8032dfa332e1") hardware = FactoryGirl.create(:hardware, :disks => [disk]) diff --git a/spec/models/manageiq/providers/redhat/infra_manager_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager_spec.rb index 9d5c7d9d5..02559ba24 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager_spec.rb +++ b/spec/models/manageiq/providers/redhat/infra_manager_spec.rb @@ -371,35 +371,49 @@ let(:options) do { :username => 'user', - :password => 'pword', - :version => '4' + :password => 'pword' } end - let(:connection) { double(OvirtSDK4::Connection) } + let(:v4_connection) { double(OvirtSDK4::Connection) } + let(:v3_connection) { double(Ovirt::Service) } before do - allow(connection).to receive(:test).and_return(true) + allow(v3_connection).to receive(:disconnect) end - it "works with different versions" do - expect(described_class).to receive(:raw_connect_v4).and_return(connection) + it "works with version 4" do + expect(described_class).to receive(:raw_connect_v4).and_return(v4_connection) + expect(v4_connection).to receive(:test).with(hash_including(:raise_exception => true)) + .and_return(true) described_class.raw_connect(options) + end - expect(described_class).to receive(:raw_connect_v3).and_return(connection) + it "works with version 3" do + expect(described_class).to receive(:raw_connect_v4).and_return(v4_connection) + expect(v4_connection).to receive(:test).with(hash_including(:raise_exception => true)) + .and_raise(OvirtSDK4::Error.new('Something failed')) + expect(described_class).to receive(:raw_connect_v3).and_return(v3_connection) + expect(v3_connection).to receive(:api).and_return(nil) - described_class.raw_connect(options.merge(:version => '3')) + described_class.raw_connect(options) end - it "always closes the connection" do - expect(described_class).to receive(:raw_connect_v4).and_return(connection) - expect(connection).to receive(:disconnect) + it "always closes the V3 connection" do + expect(described_class).to receive(:raw_connect_v4).and_return(v4_connection) + expect(v4_connection).to receive(:test).with(hash_including(:raise_exception => true)) + .and_raise(OvirtSDK4::Error.new('Something failed')) + expect(described_class).to receive(:raw_connect_v3).and_return(v3_connection) + expect(v3_connection).to receive(:api).and_return(nil) + expect(v3_connection).to receive(:disconnect) described_class.raw_connect(options) end it "decrypts the password" do - allow(described_class).to receive(:raw_connect_v4).and_return(connection) + allow(described_class).to receive(:raw_connect_v4).and_return(v4_connection) + expect(v4_connection).to receive(:test).with(hash_including(:raise_exception => true)) + .and_return(true) expect(MiqPassword).to receive(:try_decrypt).with(options[:password])