From 944e50176c1a11dee40be9f3d032e9a4f1b53bde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miquel=20Sabat=C3=A9=20Sol=C3=A0?= Date: Tue, 19 Dec 2017 16:42:39 +0100 Subject: [PATCH] Merging network errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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à --- .../admin/registries/components/form.js | 3 +- app/controllers/passwords_controller.rb | 5 +- config/initializers/mail.rb | 54 ++++++++++--------- lib/portus/errors.rb | 20 +++++++ lib/portus/health_checks/clair.rb | 2 +- lib/portus/http_helpers.rb | 16 ++---- lib/portus/request_error.rb | 2 +- lib/portus/security_backends/clair.rb | 2 +- .../admin/registries_controller_spec.rb | 4 ++ spec/factories/registries.rb | 2 +- spec/features/admin/registries_spec.rb | 10 +++- spec/features/forgotten_password_spec.rb | 3 +- spec/helpers/repositories_helper_spec.rb | 2 + spec/jobs/catalog_job_spec.rb | 12 ++++- spec/lib/portus/request_error_spec.rb | 2 +- spec/models/registry_spec.rb | 10 ++-- 16 files changed, 91 insertions(+), 58 deletions(-) diff --git a/app/assets/javascripts/modules/admin/registries/components/form.js b/app/assets/javascripts/modules/admin/registries/components/form.js index 7bde82b7e..c6831bea1 100644 --- a/app/assets/javascripts/modules/admin/registries/components/form.js +++ b/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; diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 35bcaf624..b503a0a8c 100644 --- a/app/controllers/passwords_controller.rb +++ b/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 diff --git a/config/initializers/mail.rb b/config/initializers/mail.rb index ee9303e44..0ffc63411 100644 --- a/config/initializers/mail.rb +++ b/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 diff --git a/lib/portus/errors.rb b/lib/portus/errors.rb index 8e4a0a5ff..4e2f7d7b6 100644 --- a/lib/portus/errors.rb +++ b/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 diff --git a/lib/portus/health_checks/clair.rb b/lib/portus/health_checks/clair.rb index 7b09b3f29..9cc6bcd39 100644 --- a/lib/portus/health_checks/clair.rb +++ b/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 diff --git a/lib/portus/http_helpers.rb b/lib/portus/http_helpers.rb index 4515465f5..31b0c9e12 100644 --- a/lib/portus/http_helpers.rb +++ b/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 diff --git a/lib/portus/request_error.rb b/lib/portus/request_error.rb index 400bc4829..09057c8b8 100644 --- a/lib/portus/request_error.rb +++ b/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 diff --git a/lib/portus/security_backends/clair.rb b/lib/portus/security_backends/clair.rb index 1d99e40e8..c70b7cf4e 100644 --- a/lib/portus/security_backends/clair.rb +++ b/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 diff --git a/spec/controllers/admin/registries_controller_spec.rb b/spec/controllers/admin/registries_controller_spec.rb index 01c3ab71c..2777832c5 100644 --- a/spec/controllers/admin/registries_controller_spec.rb +++ b/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 diff --git a/spec/factories/registries.rb b/spec/factories/registries.rb index d8782a17a..22e62ea3a 100644 --- a/spec/factories/registries.rb +++ b/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| diff --git a/spec/features/admin/registries_spec.rb b/spec/features/admin/registries_spec.rb index 2876ea7ae..6c1eca5ff 100644 --- a/spec/features/admin/registries_spec.rb +++ b/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 diff --git a/spec/features/forgotten_password_spec.rb b/spec/features/forgotten_password_spec.rb index c9a4ff266..5cb5baa67 100644 --- a/spec/features/forgotten_password_spec.rb +++ b/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 diff --git a/spec/helpers/repositories_helper_spec.rb b/spec/helpers/repositories_helper_spec.rb index 41e86a021..f2f8e1bda 100644 --- a/spec/helpers/repositories_helper_spec.rb +++ b/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) diff --git a/spec/jobs/catalog_job_spec.rb b/spec/jobs/catalog_job_spec.rb index 150f17220..909d8a63f 100644 --- a/spec/jobs/catalog_job_spec.rb +++ b/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) diff --git a/spec/lib/portus/request_error_spec.rb b/spec/lib/portus/request_error_spec.rb index 0d68d65b8..0447a0c71 100644 --- a/spec/lib/portus/request_error_spec.rb +++ b/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 diff --git a/spec/models/registry_spec.rb b/spec/models/registry_spec.rb index 33998630c..4ab51c3f0 100644 --- a/spec/models/registry_spec.rb +++ b/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