From cfac5644196b1e7d69642ea69378cbbc29e41320 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Mon, 10 Sep 2018 17:09:38 -0400
Subject: [PATCH 01/11] Openstack Swift DB Backups

In support of the RFE described in BZ https://bugzilla.redhat.com/show_bug.cgi?id=1615488.

Add a new FileDepot subclass for Swift.
Since we need to pass info in to the Mount Session (in gems-pending) regarding the Openstack
connection, utilize the URI with Query Parameters to do so.
---
 app/models/database_backup.rb         |  7 +--
 app/models/file_depot.rb              |  4 ++
 app/models/file_depot_swift.rb        | 74 +++++++++++++++++++++++++++
 app/models/miq_schedule.rb            | 19 ++++---
 app/models/mixins/file_depot_mixin.rb |  7 +--
 lib/evm_database_ops.rb               |  7 ++-
 locale/en.yml                         |  1 +
 7 files changed, 106 insertions(+), 13 deletions(-)
 create mode 100644 app/models/file_depot_swift.rb

diff --git a/app/models/database_backup.rb b/app/models/database_backup.rb
index 7733b5666fe..cf606c312e0 100644
--- a/app/models/database_backup.rb
+++ b/app/models/database_backup.rb
@@ -1,8 +1,9 @@
 class DatabaseBackup < ApplicationRecord
   SUPPORTED_DEPOTS = {
-    'smb' => 'Samba',
-    'nfs' => 'Network File System',
-    's3'  => 'AWS S3'
+    'smb'   => 'Samba',
+    'nfs'   => 'Network File System',
+    's3'    => 'AWS S3',
+    'swift' => 'Openstack Swift'
   }.freeze
 
   def self.supported_depots
diff --git a/app/models/file_depot.rb b/app/models/file_depot.rb
index e1a1326da8d..0373f5d3de3 100644
--- a/app/models/file_depot.rb
+++ b/app/models/file_depot.rb
@@ -32,4 +32,8 @@ def requires_support_case?
   def upload_file(file)
     @file = file
   end
+
+  def merged_uri
+    uri
+  end
 end
diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
new file mode 100644
index 00000000000..8aaef2892bc
--- /dev/null
+++ b/app/models/file_depot_swift.rb
@@ -0,0 +1,74 @@
+class FileDepotSwift < FileDepot
+  attr_accessor :swift
+
+  def self.uri_prefix
+    "swift"
+  end
+
+  def self.validate_settings(settings)
+    new(:uri => settings[:uri]).verify_credentials(nil, settings.slice(:username, :password))
+  end
+
+  def connect(options = {})
+    uri  = options[:uri]
+    host = URI(uri).host
+    openstack_handle(options).connect(options)
+  rescue Excon::Errors::Unauthorized => err
+    logger.error("Access to Swift host #{host} failed due to a bad username or password. #{err}")
+    nil
+  rescue => err
+    logger.error("Error connecting to Swift host #{host}. #{err}")
+    msg = "Error connecting to Swift host #{host}. #{err}"
+    raise err, msg, err.backtrace
+  end
+
+  def openstack_handle(options = {})
+    require 'manageiq/providers/openstack/legacy/openstack_handle'
+    @openstack_handle ||= begin
+
+      username = options[:username] || authentication_userid(options[:auth_type])
+      password = options[:password] || authentication_password(options[:auth_type])
+      uri      = options[:uri]
+      address  = URI(uri).host
+      port     = URI(uri).port
+
+      extra_options = {
+        :ssl_ca_file    => ::Settings.ssl.ssl_ca_file,
+        :ssl_ca_path    => ::Settings.ssl.ssl_ca_path,
+        :ssl_cert_store => OpenSSL::X509::Store.new
+      }
+      extra_options[:domain_id]         = v3_domain_ident
+      extra_options[:service]           = "Compute"
+      extra_options[:omit_default_port] = ::Settings.ems.ems_openstack.excon.omit_default_port
+      extra_options[:read_timeout]      = ::Settings.ems.ems_openstack.excon.read_timeout
+      begin
+        OpenstackHandle::Handle.new(username, password, address, port, keystone_api_version, security_protocol, extra_options)
+      rescue => err
+        logger.error("Error connecting to Swift host #{address}. #{err}")
+        msg = "Error connecting to Swift host #{address}. #{err}"
+        raise err, msg, err.backtrace
+      end
+    end
+  end
+
+  def verify_credentials(auth_type = nil, options = {})
+    auth_type ||= 'default'
+
+    options[:auth_type] = auth_type
+    connect(options.merge(:auth_type => auth_type))
+  rescue => err
+    logger.error("Error connecting to Swift host #{host}. #{err}")
+    msg = "Error connecting to Swift host #{host}. #{err}"
+    raise err, msg, err.backtrace
+  end
+
+  def merged_uri(uri, api_port)
+    port      = api_port.blank? ? 5000 : api_port
+    uri       = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")
+    uri.query = [uri.query, "region=#{openstack_region}"].compact.join('&') unless openstack_region.blank?
+    uri.query = [uri.query, "api_version=#{keystone_api_version}"].compact.join('&') unless keystone_api_version.blank?
+    uri.query = [uri.query, "domain_id=#{v3_domain_ident}"].compact.join('&') unless v3_domain_ident.blank?
+    uri.query = [uri.query, "security_protocol=#{security_protocol}"].compact.join('&') unless security_protocol.blank?
+    uri
+  end
+end
diff --git a/app/models/miq_schedule.rb b/app/models/miq_schedule.rb
index 9324d64a014..aa1f9d7fb2c 100644
--- a/app/models/miq_schedule.rb
+++ b/app/models/miq_schedule.rb
@@ -331,16 +331,23 @@ def validate_file_depot  # TODO: Do we need this if the validations are on the F
   end
 
   def verify_file_depot(params)  # TODO: This logic belongs in the UI, not sure where
-    depot_class      = FileDepot.supported_protocols[params[:uri_prefix]]
-    depot            = file_depot.class.name == depot_class ? file_depot : build_file_depot(:type => depot_class)
-    depot.name       = params[:name]
-    depot.uri        = params[:uri]
-    depot.aws_region = params[:aws_region]
+    depot_class                = FileDepot.supported_protocols[params[:uri_prefix]]
+    depot                      = file_depot.class.name == depot_class ? file_depot : build_file_depot(:type => depot_class)
+    depot.name                 = params[:name]
+    uri                        = params[:uri]
+    api_port                   = params[:swift_api_port]
+    depot.aws_region           = params[:aws_region]
+    depot.openstack_region     = params[:openstack_region]
+    depot.keystone_api_version = params[:keystone_api_version]
+    depot.v3_domain_ident      = params[:v3_domain_ident]
+    depot.security_protocol    = params[:security_protocol]
+    depot.uri                  = api_port.blank? ?  uri : depot.merged_uri(uri, api_port)
+    _log.debug("uri is [#{uri}]")
     if params[:save]
       file_depot.save!
       file_depot.update_authentication(:default => {:userid => params[:username], :password => params[:password]}) if (params[:username] || params[:password]) && depot.class.requires_credentials?
     else
-      depot.verify_credentials(nil, params.slice(:username, :password)) if depot.class.requires_credentials?
+      depot.verify_credentials(nil, params) if depot.class.requires_credentials?
     end
   end
 
diff --git a/app/models/mixins/file_depot_mixin.rb b/app/models/mixins/file_depot_mixin.rb
index 4070a1b48a5..ab43966f593 100644
--- a/app/models/mixins/file_depot_mixin.rb
+++ b/app/models/mixins/file_depot_mixin.rb
@@ -4,9 +4,10 @@
 module FileDepotMixin
   extend ActiveSupport::Concern
   SUPPORTED_DEPOTS = {
-    'smb' => 'Samba',
-    'nfs' => 'Network File System',
-    's3'  => 'Amazon Web Services'
+    'smb'   => 'Samba',
+    'nfs'   => 'Network File System',
+    's3'    => 'Amazon Web Services',
+    'swift' => 'Openstack Swift'
   }.freeze
 
   included do
diff --git a/lib/evm_database_ops.rb b/lib/evm_database_ops.rb
index 30b31eb827b..fdcd68eb83d 100644
--- a/lib/evm_database_ops.rb
+++ b/lib/evm_database_ops.rb
@@ -115,7 +115,12 @@ def self.restore(db_opts, connect_opts = {})
       else
         connect_opts[:remote_file_name] ||= File.basename(backup_file_name(action))
         backup_folder = action == :dump ? "db_dump" : "db_backup"
-        uri = File.join(connect_opts[:uri], backup_folder, connect_opts[:remote_file_name])
+        #
+        # If the passed in URI contains query parameters, ignore them
+        # when creating the dump file name. They'll be used in the session object.
+        #
+        connect_opts_uri = connect_opts[:uri].split('?')[0]
+        uri = File.join(connect_opts_uri, backup_folder, connect_opts[:remote_file_name])
       end
     else
       uri = db_opts[:local_file]
diff --git a/locale/en.yml b/locale/en.yml
index d776d7ac88b..27ad8332941 100644
--- a/locale/en.yml
+++ b/locale/en.yml
@@ -730,6 +730,7 @@ en:
       FileDepotFtpAnonymous:    Anonymous FTP
       FileDepotNfs:             NFS
       FileDepotS3:              AWS S3
+      FileDepotSwift:           Openstack Swift
       FileDepotSmb:             Samba
       Filesystem:               File
       Flavor:                   Flavor

From 6f363169402c802f31eaf12c4e40b5a1a9dce85f Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 11 Sep 2018 09:17:35 -0400
Subject: [PATCH 02/11] Rubocop Warnings

---
 app/models/file_depot_swift.rb | 11 +++++------
 app/models/miq_schedule.rb     |  6 +++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index 8aaef2892bc..82732bdd92a 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -25,7 +25,6 @@ def connect(options = {})
   def openstack_handle(options = {})
     require 'manageiq/providers/openstack/legacy/openstack_handle'
     @openstack_handle ||= begin
-
       username = options[:username] || authentication_userid(options[:auth_type])
       password = options[:password] || authentication_password(options[:auth_type])
       uri      = options[:uri]
@@ -63,12 +62,12 @@ def verify_credentials(auth_type = nil, options = {})
   end
 
   def merged_uri(uri, api_port)
-    port      = api_port.blank? ? 5000 : api_port
+    port      = api_port.presence || 5000
     uri       = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")
-    uri.query = [uri.query, "region=#{openstack_region}"].compact.join('&') unless openstack_region.blank?
-    uri.query = [uri.query, "api_version=#{keystone_api_version}"].compact.join('&') unless keystone_api_version.blank?
-    uri.query = [uri.query, "domain_id=#{v3_domain_ident}"].compact.join('&') unless v3_domain_ident.blank?
-    uri.query = [uri.query, "security_protocol=#{security_protocol}"].compact.join('&') unless security_protocol.blank?
+    uri.query = [uri.query, "region=#{openstack_region}"].compact.join('&') if openstack_region.present?
+    uri.query = [uri.query, "api_version=#{keystone_api_version}"].compact.join('&') if keystone_api_version.present?
+    uri.query = [uri.query, "domain_id=#{v3_domain_ident}"].compact.join('&') if v3_domain_ident.present?
+    uri.query = [uri.query, "security_protocol=#{security_protocol}"].compact.join('&') if security_protocol.present?
     uri
   end
 end
diff --git a/app/models/miq_schedule.rb b/app/models/miq_schedule.rb
index aa1f9d7fb2c..e9fa899024c 100644
--- a/app/models/miq_schedule.rb
+++ b/app/models/miq_schedule.rb
@@ -341,13 +341,13 @@ def verify_file_depot(params)  # TODO: This logic belongs in the UI, not sure wh
     depot.keystone_api_version = params[:keystone_api_version]
     depot.v3_domain_ident      = params[:v3_domain_ident]
     depot.security_protocol    = params[:security_protocol]
-    depot.uri                  = api_port.blank? ?  uri : depot.merged_uri(uri, api_port)
+    depot.uri                  = api_port.blank? ? uri : depot.merged_uri(uri, api_port)
     _log.debug("uri is [#{uri}]")
     if params[:save]
       file_depot.save!
       file_depot.update_authentication(:default => {:userid => params[:username], :password => params[:password]}) if (params[:username] || params[:password]) && depot.class.requires_credentials?
-    else
-      depot.verify_credentials(nil, params) if depot.class.requires_credentials?
+    elsif depot.class.requires_credentials?
+      depot.verify_credentials(nil, params)
     end
   end
 

From 431ecb39139e52ab3c63bc47cdb8cb27150a72fb Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 18 Sep 2018 10:31:17 -0400
Subject: [PATCH 03/11] Review Comments

Several changes based on PR review comments -
1) Change "Openstack Swift" to "OpenStack Swift"
2) Fix default argument handling
3) Refactor Query String processing.
---
 app/models/database_backup.rb         |  2 +-
 app/models/file_depot_swift.rb        | 18 +++++++++---------
 app/models/mixins/file_depot_mixin.rb |  2 +-
 locale/en.yml                         |  2 +-
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/app/models/database_backup.rb b/app/models/database_backup.rb
index cf606c312e0..e2cbe1b2926 100644
--- a/app/models/database_backup.rb
+++ b/app/models/database_backup.rb
@@ -3,7 +3,7 @@ class DatabaseBackup < ApplicationRecord
     'smb'   => 'Samba',
     'nfs'   => 'Network File System',
     's3'    => 'AWS S3',
-    'swift' => 'Openstack Swift'
+    'swift' => 'OpenStack Swift'
   }.freeze
 
   def self.supported_depots
diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index 82732bdd92a..2c91adb90c5 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -50,9 +50,7 @@ def openstack_handle(options = {})
     end
   end
 
-  def verify_credentials(auth_type = nil, options = {})
-    auth_type ||= 'default'
-
+  def verify_credentials(auth_type = 'default', options = {})
     options[:auth_type] = auth_type
     connect(options.merge(:auth_type => auth_type))
   rescue => err
@@ -62,12 +60,14 @@ def verify_credentials(auth_type = nil, options = {})
   end
 
   def merged_uri(uri, api_port)
-    port      = api_port.presence || 5000
-    uri       = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")
-    uri.query = [uri.query, "region=#{openstack_region}"].compact.join('&') if openstack_region.present?
-    uri.query = [uri.query, "api_version=#{keystone_api_version}"].compact.join('&') if keystone_api_version.present?
-    uri.query = [uri.query, "domain_id=#{v3_domain_ident}"].compact.join('&') if v3_domain_ident.present?
-    uri.query = [uri.query, "security_protocol=#{security_protocol}"].compact.join('&') if security_protocol.present?
+    port           = api_port.presence || 5000
+    uri            = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")
+    query_elements = []
+    query_elements << "region=#{openstack_region}"              if openstack_region.present?
+    query_elements << "api_version=#{keystone_api_version}"     if keystone_api_version.present?
+    query_elements << "domain_id=#{v3_domain_ident}"            if v3_domain_ident.present?
+    query_elements << "security_protocol=#{security_protocol}"  if security_protocol.present?
+    uri.query = query_elements.join('&').presence
     uri
   end
 end
diff --git a/app/models/mixins/file_depot_mixin.rb b/app/models/mixins/file_depot_mixin.rb
index ab43966f593..e8bed8693e5 100644
--- a/app/models/mixins/file_depot_mixin.rb
+++ b/app/models/mixins/file_depot_mixin.rb
@@ -7,7 +7,7 @@ module FileDepotMixin
     'smb'   => 'Samba',
     'nfs'   => 'Network File System',
     's3'    => 'Amazon Web Services',
-    'swift' => 'Openstack Swift'
+    'swift' => 'OpenStack Swift'
   }.freeze
 
   included do
diff --git a/locale/en.yml b/locale/en.yml
index 27ad8332941..45cfa3aa12c 100644
--- a/locale/en.yml
+++ b/locale/en.yml
@@ -730,7 +730,7 @@ en:
       FileDepotFtpAnonymous:    Anonymous FTP
       FileDepotNfs:             NFS
       FileDepotS3:              AWS S3
-      FileDepotSwift:           Openstack Swift
+      FileDepotSwift:           OpenStack Swift
       FileDepotSmb:             Samba
       Filesystem:               File
       Flavor:                   Flavor

From 12d0c202f55fc9813f3feaaf4b8e72fa049b52d4 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 9 Oct 2018 14:30:29 -0400
Subject: [PATCH 04/11] Credential Verification and Cleanup

Fix credentials verification.  Cleanup extra log messages.
---
 app/models/file_depot_swift.rb | 14 +++++---------
 app/models/miq_schedule.rb     |  1 -
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index 2c91adb90c5..e0b1bbdbbff 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -10,16 +10,7 @@ def self.validate_settings(settings)
   end
 
   def connect(options = {})
-    uri  = options[:uri]
-    host = URI(uri).host
     openstack_handle(options).connect(options)
-  rescue Excon::Errors::Unauthorized => err
-    logger.error("Access to Swift host #{host} failed due to a bad username or password. #{err}")
-    nil
-  rescue => err
-    logger.error("Error connecting to Swift host #{host}. #{err}")
-    msg = "Error connecting to Swift host #{host}. #{err}"
-    raise err, msg, err.backtrace
   end
 
   def openstack_handle(options = {})
@@ -51,8 +42,13 @@ def openstack_handle(options = {})
   end
 
   def verify_credentials(auth_type = 'default', options = {})
+    host = URI(options[:uri]).host
     options[:auth_type] = auth_type
     connect(options.merge(:auth_type => auth_type))
+  rescue Excon::Errors::Unauthorized => err
+    msg = "Access to Swift host #{host} failed due to a bad username or password."
+    logger.error("Access to Swift host #{host} failed due to a bad username or password. #{err}")
+    raise msg
   rescue => err
     logger.error("Error connecting to Swift host #{host}. #{err}")
     msg = "Error connecting to Swift host #{host}. #{err}"
diff --git a/app/models/miq_schedule.rb b/app/models/miq_schedule.rb
index e9fa899024c..b89b7f40ca8 100644
--- a/app/models/miq_schedule.rb
+++ b/app/models/miq_schedule.rb
@@ -342,7 +342,6 @@ def verify_file_depot(params)  # TODO: This logic belongs in the UI, not sure wh
     depot.v3_domain_ident      = params[:v3_domain_ident]
     depot.security_protocol    = params[:security_protocol]
     depot.uri                  = api_port.blank? ? uri : depot.merged_uri(uri, api_port)
-    _log.debug("uri is [#{uri}]")
     if params[:save]
       file_depot.save!
       file_depot.update_authentication(:default => {:userid => params[:username], :password => params[:password]}) if (params[:username] || params[:password]) && depot.class.requires_credentials?

From 2b2c246004ea914614a05edfa38110799beb11a6 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 9 Oct 2018 15:54:21 -0400
Subject: [PATCH 05/11] Assign URI Port More Simply

Nuff said.
---
 app/models/file_depot_swift.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index e0b1bbdbbff..023505a84e2 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -56,8 +56,7 @@ def verify_credentials(auth_type = 'default', options = {})
   end
 
   def merged_uri(uri, api_port)
-    port           = api_port.presence || 5000
-    uri            = URI.parse("#{URI(uri).scheme}://#{URI(uri).host}:#{port}#{URI(uri).path}")
+    uri.port       = api_port.presence || 5000
     query_elements = []
     query_elements << "region=#{openstack_region}"              if openstack_region.present?
     query_elements << "api_version=#{keystone_api_version}"     if keystone_api_version.present?

From f2a9640838a0a2c1f3b93ec88c18e65f6586b5ed Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 9 Oct 2018 16:27:18 -0400
Subject: [PATCH 06/11] Add FileDepotSwift specs

Add some spec tests for the class including the merged_uri method.
---
 spec/models/file_depot_swift_spec.rb | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 spec/models/file_depot_swift_spec.rb

diff --git a/spec/models/file_depot_swift_spec.rb b/spec/models/file_depot_swift_spec.rb
new file mode 100644
index 00000000000..c9b9ac0ea64
--- /dev/null
+++ b/spec/models/file_depot_swift_spec.rb
@@ -0,0 +1,28 @@
+describe FileDepotSwift do
+  let(:uri) { URI("swift://server.example.com/bucket") }
+  let(:merged_uri) { URI("swift://server.example.com:5000/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
+  let(:file_depot_swift) { FileDepotSwift.new(:uri => uri) }
+  it "should require credentials" do
+    expect(FileDepotSwift.requires_credentials?).to eq true
+  end
+
+  it "should return a valid prefix" do
+    expect(FileDepotSwift.uri_prefix).to eq "swift"
+  end
+
+  context "valid merged uri" do
+    before do
+      file_depot_swift.openstack_region = "test_openstack_region"
+      file_depot_swift.keystone_api_version = "v3"
+      file_depot_swift.v3_domain_ident = "default"
+    end
+
+    it "should return a merged uri with query strings given an empty port" do
+      expect(file_depot_swift.merged_uri(uri, nil)).to eq merged_uri
+    end 
+
+    it "should return a merged uri with query strings when given a valid port" do
+      expect(file_depot_swift.merged_uri(uri, "5000")).to eq merged_uri
+    end 
+  end
+end

From 24e1197df035736011e680151b79560ed5f37cef Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 9 Oct 2018 16:28:09 -0400
Subject: [PATCH 07/11] Another FileDepotSwift Test

---
 spec/models/file_depot_spec.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/spec/models/file_depot_spec.rb b/spec/models/file_depot_spec.rb
index 9441c899eeb..66557594819 100644
--- a/spec/models/file_depot_spec.rb
+++ b/spec/models/file_depot_spec.rb
@@ -4,5 +4,6 @@
     expect(described_class.depot_description_to_class("NFS")).to eq(FileDepotNfs)
     expect(described_class.depot_description_to_class("Samba")).to eq(FileDepotSmb)
     expect(described_class.depot_description_to_class("Anonymous FTP")).to eq(FileDepotFtpAnonymous)
+    expect(described_class.depot_description_to_class("OpenStack Swift")).to eq(FileDepotSwift)
   end
 end

From bc76d2ceca7a35413e35b02cfad5db96359db0b6 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Tue, 9 Oct 2018 16:37:00 -0400
Subject: [PATCH 08/11] remove unused attr_accessor for swift variable

---
 app/models/file_depot_swift.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index 023505a84e2..dda267e0714 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -1,6 +1,4 @@
 class FileDepotSwift < FileDepot
-  attr_accessor :swift
-
   def self.uri_prefix
     "swift"
   end

From 871b514ea950bb425710c5f513ecd590a520b639 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Wed, 10 Oct 2018 13:34:46 -0400
Subject: [PATCH 09/11] Review comments for some cleanup

Fixed one test so that it doesn't use the default port for the URI.
Cleaned up some msg logging to be more efficient.
---
 app/models/file_depot_swift.rb       | 6 +++---
 spec/models/file_depot_swift_spec.rb | 7 ++++---
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index dda267e0714..3d845020653 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -32,8 +32,8 @@ def openstack_handle(options = {})
       begin
         OpenstackHandle::Handle.new(username, password, address, port, keystone_api_version, security_protocol, extra_options)
       rescue => err
-        logger.error("Error connecting to Swift host #{address}. #{err}")
         msg = "Error connecting to Swift host #{address}. #{err}"
+        logger.error(msg)
         raise err, msg, err.backtrace
       end
     end
@@ -45,11 +45,11 @@ def verify_credentials(auth_type = 'default', options = {})
     connect(options.merge(:auth_type => auth_type))
   rescue Excon::Errors::Unauthorized => err
     msg = "Access to Swift host #{host} failed due to a bad username or password."
-    logger.error("Access to Swift host #{host} failed due to a bad username or password. #{err}")
+    logger.error("#{msg} #{err}")
     raise msg
   rescue => err
-    logger.error("Error connecting to Swift host #{host}. #{err}")
     msg = "Error connecting to Swift host #{host}. #{err}"
+    logger.error(msg)
     raise err, msg, err.backtrace
   end
 
diff --git a/spec/models/file_depot_swift_spec.rb b/spec/models/file_depot_swift_spec.rb
index c9b9ac0ea64..06a31e51fc8 100644
--- a/spec/models/file_depot_swift_spec.rb
+++ b/spec/models/file_depot_swift_spec.rb
@@ -1,6 +1,7 @@
 describe FileDepotSwift do
   let(:uri) { URI("swift://server.example.com/bucket") }
-  let(:merged_uri) { URI("swift://server.example.com:5000/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
+  let(:merged_uri) { URI("swift://server.example.com:5678/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
+  let(:merged_default_uri) { URI("swift://server.example.com:5000/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
   let(:file_depot_swift) { FileDepotSwift.new(:uri => uri) }
   it "should require credentials" do
     expect(FileDepotSwift.requires_credentials?).to eq true
@@ -18,11 +19,11 @@
     end
 
     it "should return a merged uri with query strings given an empty port" do
-      expect(file_depot_swift.merged_uri(uri, nil)).to eq merged_uri
+      expect(file_depot_swift.merged_uri(uri, nil)).to eq merged_default_uri
     end 
 
     it "should return a merged uri with query strings when given a valid port" do
-      expect(file_depot_swift.merged_uri(uri, "5000")).to eq merged_uri
+      expect(file_depot_swift.merged_uri(uri, "5678")).to eq merged_uri
     end 
   end
 end

From d42bd4e0a3b93fc73813b99b2516a9d1e821cdd8 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Wed, 10 Oct 2018 17:14:54 -0400
Subject: [PATCH 10/11] Fix Failing Test and Merged URI Handling

The merged_uri method was working incorrectly - coded to let the
spec tests succeed but the validation of the Swift connection to fail.
---
 app/models/file_depot_swift.rb       | 3 ++-
 app/models/miq_schedule.rb           | 2 +-
 spec/models/file_depot_swift_spec.rb | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/app/models/file_depot_swift.rb b/app/models/file_depot_swift.rb
index 3d845020653..f7be3fe5cbf 100644
--- a/app/models/file_depot_swift.rb
+++ b/app/models/file_depot_swift.rb
@@ -54,6 +54,7 @@ def verify_credentials(auth_type = 'default', options = {})
   end
 
   def merged_uri(uri, api_port)
+    uri            = URI(uri)
     uri.port       = api_port.presence || 5000
     query_elements = []
     query_elements << "region=#{openstack_region}"              if openstack_region.present?
@@ -61,6 +62,6 @@ def merged_uri(uri, api_port)
     query_elements << "domain_id=#{v3_domain_ident}"            if v3_domain_ident.present?
     query_elements << "security_protocol=#{security_protocol}"  if security_protocol.present?
     uri.query = query_elements.join('&').presence
-    uri
+    uri.to_s
   end
 end
diff --git a/app/models/miq_schedule.rb b/app/models/miq_schedule.rb
index b89b7f40ca8..09b0d4149b5 100644
--- a/app/models/miq_schedule.rb
+++ b/app/models/miq_schedule.rb
@@ -341,7 +341,7 @@ def verify_file_depot(params)  # TODO: This logic belongs in the UI, not sure wh
     depot.keystone_api_version = params[:keystone_api_version]
     depot.v3_domain_ident      = params[:v3_domain_ident]
     depot.security_protocol    = params[:security_protocol]
-    depot.uri                  = api_port.blank? ? uri : depot.merged_uri(uri, api_port)
+    depot.uri                  = api_port.blank? ? uri : depot.merged_uri(URI(uri), api_port)
     if params[:save]
       file_depot.save!
       file_depot.update_authentication(:default => {:userid => params[:username], :password => params[:password]}) if (params[:username] || params[:password]) && depot.class.requires_credentials?
diff --git a/spec/models/file_depot_swift_spec.rb b/spec/models/file_depot_swift_spec.rb
index 06a31e51fc8..7a0970bcbba 100644
--- a/spec/models/file_depot_swift_spec.rb
+++ b/spec/models/file_depot_swift_spec.rb
@@ -1,7 +1,7 @@
 describe FileDepotSwift do
-  let(:uri) { URI("swift://server.example.com/bucket") }
-  let(:merged_uri) { URI("swift://server.example.com:5678/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
-  let(:merged_default_uri) { URI("swift://server.example.com:5000/bucket?region=test_openstack_region&api_version=v3&domain_id=default") }
+  let(:uri) { "swift://server.example.com/bucket" }
+  let(:merged_uri) { "swift://server.example.com:5678/bucket?region=test_openstack_region&api_version=v3&domain_id=default" }
+  let(:merged_default_uri) { "swift://server.example.com:5000/bucket?region=test_openstack_region&api_version=v3&domain_id=default" }
   let(:file_depot_swift) { FileDepotSwift.new(:uri => uri) }
   it "should require credentials" do
     expect(FileDepotSwift.requires_credentials?).to eq true

From 3700e44888d776ea16c2c1369bf0e210cefc1680 Mon Sep 17 00:00:00 2001
From: Jerry Keselman <jkeselma@redhat.com>
Date: Thu, 11 Oct 2018 11:22:14 -0400
Subject: [PATCH 11/11] Fix Spec Tests Based on Comments

Some formatting changes based on review comments.
---
 spec/models/file_depot_swift_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/file_depot_swift_spec.rb b/spec/models/file_depot_swift_spec.rb
index 7a0970bcbba..a82ff877486 100644
--- a/spec/models/file_depot_swift_spec.rb
+++ b/spec/models/file_depot_swift_spec.rb
@@ -11,7 +11,7 @@
     expect(FileDepotSwift.uri_prefix).to eq "swift"
   end
 
-  context "valid merged uri" do
+  describe "#merged_uri" do
     before do
       file_depot_swift.openstack_region = "test_openstack_region"
       file_depot_swift.keystone_api_version = "v3"