Try both API versions in raw_connect

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

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'

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)

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_3_1_spec.rb

@@ -7,7 +7,13 @@
     @ems.update_authentication(:default => {:userid => "[email protected]", :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

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')

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

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])

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])