Merging network errors

Networking errors have been listed into a common array that then can be
used in `rescue` statements. This is useful to catch as many possible
networking issues in an easy way. Moreover, I've also written a helper
method to generate a proper message for each different error.

After doing this, I discovered that rescuing StandardError on
`safe_request` hid lots of badly written tests. Thus, I had to re-adjust
quite a lot of them.

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

app/assets/javascripts/modules/admin/registries/components/form.js

@@ -70,7 +70,8 @@ export default {
 
   methods: {
     isReachableError(error) {
-      return error.indexOf('Error: ') !== -1 ||
+      return error.indexOf('Error') !== -1 ||
+             error.indexOf('connection') !== -1 ||
              error.indexOf('SSLError') !== -1 ||
              error.indexOf('OpenTimeout') !== -1 ||
              error.indexOf('SSLError') !== -1;

app/controllers/passwords_controller.rb

@@ -17,8 +17,9 @@ def create
     else
       redirect_to new_user_password_path, alert: resource.errors.full_messages, float: true
     end
-  rescue SocketError, Errno::ECONNREFUSED => e
-    Rails.logger.tagged("Mailer") { Rails.logger.info "Exception: #{e.message}" }
+  rescue *::Portus::Errors::NET => e
+    msg = "#{e}: #{::Portus::Errors.message_from_exception(e)}"
+    Rails.logger.tagged("Mailer") { Rails.logger.info msg }
     redirect_to new_user_password_path,
                 alert: "Something went wrong. Check the configuration of Portus",
                 float: true

config/initializers/mail.rb

@@ -6,34 +6,36 @@ def check_email!(key)
   raise "Mail: bad config value for '#{key}'. '#{value}' is not a proper email..."
 end
 
-check_email!("from")
-check_email!("reply_to")
+unless Rails.env.test?
+  check_email!("from")
+  check_email!("reply_to")
 
-# If SMTP was set, then use it as the delivery method and configure it with the
-# given config.
+  # If SMTP was set, then use it as the delivery method and configure it with the
+  # given config.
 
-if defined?(APP_CONFIG) && APP_CONFIG["email"]["smtp"]["enabled"]
-  Portus::Application.config.action_mailer.delivery_method = :smtp
-  smtp = APP_CONFIG["email"]["smtp"]
-  smtp_settings = {
-    address:              smtp["address"],
-    port:                 smtp["port"],
-    domain:               smtp["domain"],
-    enable_starttls_auto: false
-  }
-  if smtp["user_name"].blank?
-    Rails.logger.info "No smtp username supplied, not using smtp authentication"
-  else
-    auth_settings = {
-      user_name:            smtp["user_name"],
-      password:             smtp["password"],
-      authentication:       :login,
-      enable_starttls_auto: true
+  if defined?(APP_CONFIG) && APP_CONFIG["email"]["smtp"]["enabled"]
+    Portus::Application.config.action_mailer.delivery_method = :smtp
+    smtp = APP_CONFIG["email"]["smtp"]
+    smtp_settings = {
+      address:              smtp["address"],
+      port:                 smtp["port"],
+      domain:               smtp["domain"],
+      enable_starttls_auto: false
     }
-    smtp_settings = smtp_settings.merge(auth_settings)
+    if smtp["user_name"].blank?
+      Rails.logger.info "No smtp username supplied, not using smtp authentication"
+    else
+      auth_settings = {
+        user_name:            smtp["user_name"],
+        password:             smtp["password"],
+        authentication:       :login,
+        enable_starttls_auto: true
+      }
+      smtp_settings = smtp_settings.merge(auth_settings)
+    end
+    ActionMailer::Base.smtp_settings = smtp_settings
+  else
+    # If SMTP is not enabled, then go for sendmail.
+    Portus::Application.config.action_mailer.delivery_method = :sendmail
   end
-  ActionMailer::Base.smtp_settings = smtp_settings
-else
-  # If SMTP is not enabled, then go for sendmail.
-  Portus::Application.config.action_mailer.delivery_method = :sendmail
 end

lib/portus/errors.rb

@@ -3,6 +3,26 @@
 module Portus
   # Errors contain registry-specific errors that have no real implementation.
   module Errors
+    # Networking errors given usually on this application. This is useful to
+    # catch a set of common networking issues on a single rescue statement.
+    NET = [SocketError, OpenSSL::SSL::SSLError, Net::HTTPBadResponse,
+           Errno::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH,
+           Errno::ETIMEDOUT, Net::OpenTimeout, Net::ReadTimeout].freeze
+
+    # Returns a string with a message representing the given exception.
+    def self.message_from_exception(klass)
+      case klass
+      when SocketError, Errno::ECONNREFUSED, Errno::EHOSTUNREACH
+        "connection refused"
+      when Errno::ECONNRESET
+        "connection reset"
+      when OpenSSL::SSL::SSLError, Net::HTTPBadResponse
+        "could not stablish connection: SSL error"
+      when Errno::ETIMEDOUT, Net::OpenTimeout
+        "connection timed out"
+      end
+    end
+
     # As specified in the token specification of distribution, the client will
     # get a 401 on the first attempt of logging in, but in there should be the
     # "WWW-Authenticate" header. This exception will be raised when there's no

lib/portus/health_checks/clair.rb

@@ -21,7 +21,7 @@ def self.ready
         res     = get_response_token(uri, req)
         success = res.code.to_i == 200
         ["clair is#{success ? "" : " not"} reachable", success]
-      rescue SocketError, Errno::ECONNREFUSED => e
+      rescue *::Portus::Errors::NET => e
         ["clair is not reachable: #{e.message}", false]
       end
 

lib/portus/http_helpers.rb

@@ -61,19 +61,9 @@ def perform_request(path, method = "get", request_auth_token = true)
     # of exceptions that an HTTP request might entail.
     def safe_request(path, method = "get", request_auth_token = true)
       perform_request(path, method, request_auth_token)
-    rescue Errno::ECONNREFUSED, SocketError => e
-      raise ::Portus::RequestError.new(exception: e, message: "connection refused")
-    rescue Errno::ETIMEDOUT, Net::OpenTimeout => e
-      raise ::Portus::RequestError.new(exception: e, message: "connection timed out")
-    rescue Net::HTTPBadResponse => e
-      raise ::Portus::RequestError.new(exception: e,
-                                       message:   "there's something wrong with your SSL config")
-    rescue OpenSSL::SSL::SSLError => e
-      raise ::Portus::RequestError.new(exception: e,
-                                       message:   "SSL error while communicating with the registry")
-    rescue StandardError => e
-      raise ::Portus::RequestError.new(exception: e,
-                                       message:   "something went wrong. Check your configuration")
+    rescue *::Portus::Errors::NET => e
+      message = ::Portus::Errors.message_from_exception(e)
+      raise ::Portus::RequestError.new(exception: e, message: message)
     end
 
     # Handle a known error from Docker distribution. Typically these are

lib/portus/request_error.rb

@@ -8,7 +8,7 @@ class RequestError < StandardError
     # Given an inner exception and a message, it builds up a common error
     # message.
     def initialize(exception:, message:)
-      @msg = "#{exception.class.name}: #{message}"
+      @msg = "#{exception.class.name}: #{message}."
       Rails.logger.error @msg
     end
 

lib/portus/security_backends/clair.rb

@@ -70,7 +70,7 @@ def fetch_layer(digest)
         req["Accept"] = "application/json"
         begin
           res = get_response_token(uri, req)
-        rescue SocketError, Errno::ECONNREFUSED => e
+        rescue *::Portus::Errors::NET => e
           Rails.logger.tagged("clair.get") { Rails.logger.debug e.message }
           return
         end

spec/controllers/admin/registries_controller_spec.rb

@@ -39,6 +39,7 @@
     context "not using the Force" do
       it "renders 'new' with unprocessable entity status (422)
           when there's something wrong with the reachability of the registry" do
+        allow_any_instance_of(Registry).to receive(:reachable?).and_return("Error")
         expect do
           post :create, registry: attributes_for(:registry)
         end.to change(Registry, :count).by(0)
@@ -109,6 +110,7 @@
 
     it "renders 'edit' with unprocessable entity status (422)
         when there's something wrong with the reachability of the registry" do
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return("Error")
       expect do
         put :update, id: registry.id, registry: { hostname: "lala" }
       end.to change(Registry, :count).by(0)
@@ -126,12 +128,14 @@
 
     it "does not allow to update hostname if there are repos" do
       create(:repository)
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return(nil)
       old = registry.hostname
       put :update, id: registry.id, registry: { hostname: "lala" }
       expect(Registry.first.hostname).to eq old
     end
 
     it "does not allow to update name if empty" do
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return(nil)
       put :update, id: registry.id, registry: { name: "" }
       expect(response).to have_http_status(:unprocessable_entity)
     end

spec/factories/registries.rb

@@ -23,7 +23,7 @@
     before(:create) { create(:admin) }
 
     sequence :hostname do |n|
-      "registry hostname #{n}"
+      "registry.hostname:#{n}"
     end
 
     sequence :name do |n|

spec/features/admin/registries_spec.rb

@@ -38,6 +38,8 @@
     end
 
     it "shows an error if hostname is not reachable" do
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return("connection refused")
+
       visit new_admin_registry_path
 
       expect(page).not_to have_content("Skip remote checks")
@@ -46,11 +48,13 @@
       fill_in "registry_hostname", with: "url_not_known:1234"
 
       expect(page).to have_content("Skip remote checks")
-      expect(page).to have_content("something went wrong")
+      expect(page).to have_content("connection refused")
       expect(page).to have_button("Create", disabled: true)
     end
 
     it "shows an error (hostname), but you can force it afterwards" do
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return("Error")
+
       visit new_admin_registry_path
 
       fill_in "registry_name", with: "registry"
@@ -122,11 +126,13 @@
     end
 
     it "shows an error if hostname is not reachable" do
+      allow_any_instance_of(Registry).to receive(:reachable?).and_return("connection refused")
+
       fill_in "registry_name", with: "registry"
       fill_in "registry_hostname", with: "url_not_known:1234"
 
       expect(page).to have_content("Skip remote checks")
-      expect(page).to have_content("something went wrong")
+      expect(page).to have_content("connection refused")
       expect(page).to have_button("Update", disabled: true)
     end
 

spec/features/forgotten_password_spec.rb

@@ -10,7 +10,8 @@
     APP_CONFIG["email"]  = {
       "from"     => "test@example.com",
       "name"     => "Portus",
-      "reply_to" => "no-reply@example.com"
+      "reply_to" => "no-reply@example.com",
+      "smtp"     => { "enabled" => false }
     }
     ActionMailer::Base.deliveries.clear
   end

spec/helpers/repositories_helper_spec.rb

@@ -72,6 +72,8 @@ def update_registry!(catalog)
     let!(:tag4)       { create(:tag, name: "0.4", author: owner, repository: repo3) }
 
     it "creates the proper HTML for each kind of activity" do
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
+
       repo.create_activity(:push, owner: owner, recipient: tag, created_at: 1.hour.ago)
       repo.create_activity(:push, owner: owner, recipient: tag1, created_at: 2.hours.ago)
       repo1.create_activity(:push, owner: owner, recipient: tag2, created_at: 3.hours.ago)

spec/jobs/catalog_job_spec.rb

@@ -12,6 +12,8 @@ def update_registry!(catalog)
 describe CatalogJob do
   describe "Empty database" do
     it "updates the registry" do
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
+
       registry  = create(:registry)
       namespace = create(:namespace, registry: registry)
 
@@ -58,6 +60,7 @@ def update_registry!(catalog)
     it "performs the job as expected" do
       VCR.turn_on!
 
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
       registry = create(:registry, "hostname" => "registry.test.lan")
 
       VCR.use_cassette("registry/get_registry_catalog", record: :none) do
@@ -66,7 +69,7 @@ def update_registry!(catalog)
       end
 
       repos = Repository.all
-      expect(repos.count).to be 1
+      expect(repos.count).to eq 1
       repo = repos[0]
       expect(repo.name).to eq "busybox"
       expect(repo.namespace.id).to eq registry.namespaces.first.id
@@ -77,6 +80,7 @@ def update_registry!(catalog)
     it "handles registries even if there some namespaces missing" do
       VCR.turn_on!
 
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
       registry = create(:registry, "hostname" => "registry.test.lan")
 
       VCR.use_cassette("registry/get_registry_catalog_namespace_missing", record: :none) do
@@ -85,7 +89,7 @@ def update_registry!(catalog)
       end
 
       repos = Repository.all
-      expect(repos.count).to be 1
+      expect(repos.count).to eq 1
       repo = repos[0]
       expect(repo.name).to eq "busybox"
       expect(repo.namespace.id).to eq registry.namespaces.first.id
@@ -105,6 +109,8 @@ def update_registry!(catalog)
     let!(:tag3)        { create(:tag, name: "tag3", repository: repo2) }
 
     it "updates the registry" do
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
+
       job = CatalogJobMock.new
       job.update_registry!([
                              { "name" => "busybox",                  "tags" => ["latest", "0.1"] },
@@ -151,6 +157,8 @@ def update_registry!(catalog)
     let!(:tag)        { create(:tag, name: "latest", author: owner, repository: repo) }
 
     it "removes activities from dangling tags" do
+      allow_any_instance_of(::Portus::RegistryClient).to receive(:manifest).and_return(["", ""])
+
       repo.create_activity(:push, owner: owner, recipient: tag)
 
       activities = PublicActivity::Activity.order(:updated_at)

spec/lib/portus/request_error_spec.rb

@@ -6,6 +6,6 @@
 
     expect do
       raise ::Portus::RequestError.new(exception: e, message: "something")
-    end.to raise_error(::Portus::RequestError, "StandardError: something")
+    end.to raise_error(::Portus::RequestError, "StandardError: something.")
   end
 end

spec/models/registry_spec.rb

@@ -107,7 +107,6 @@ def create_empty_namespace
     end
   end
 
-  # rubocop: disable Metrics/LineLength
   describe "#reachable" do
     after :each do
       allow_any_instance_of(::Portus::RegistryClient).to receive(:perform_request).and_call_original
@@ -123,10 +122,10 @@ def create_empty_namespace
         [Errno::ECONNREFUSED, GOOD_RESPONSE, true, /connection refused/],
         [Errno::ETIMEDOUT, GOOD_RESPONSE, true, /connection timed out/],
         [Net::OpenTimeout, GOOD_RESPONSE, true, /connection timed out/],
-        [Net::HTTPBadResponse, GOOD_RESPONSE, true, /wrong with your SSL config/],
-        [OpenSSL::SSL::SSLError, GOOD_RESPONSE, true, /SSL error while communicating with the registry/],
-        [OpenSSL::SSL::SSLError, GOOD_RESPONSE, false, /SSL error while communicating with the registry/],
-        [StandardError, GOOD_RESPONSE, true, /something went wrong/]
+        [Net::HTTPBadResponse, GOOD_RESPONSE, true, /could not stablish connection: SSL error/],
+        [OpenSSL::SSL::SSLError, GOOD_RESPONSE, true, /could not stablish connection: SSL error/],
+        [OpenSSL::SSL::SSLError, GOOD_RESPONSE, false, /could not stablish connection: SSL error/],
+        [Errno::ECONNRESET, GOOD_RESPONSE, false, /connection reset/]
       ].each do |cs|
         allow_any_instance_of(::Portus::RegistryClient).to receive(:perform_request) do
           raise cs.first if cs.first
@@ -137,7 +136,6 @@ def create_empty_namespace
       end
     end
   end
-  # rubocop: enable Metrics/LineLength
 
   describe "#get_tag_from_manifest" do
     it "returns a tag on success" do