lib: be explicit on the exceptions to be rescued

In the past we have had some problems when it comes to unhandled
exceptions. This commit fixes the opposite problem: having too broad
exceptions. That is, this commit clarifies which exact exceptions might
be raised from the different registry methods and updates the places
where these methods are used.

Some tests had to be corrected because they were abusing some code
paths that were rescuing StandardError.

Signed-off-by: Miquel Sabaté Solà <[email protected]>

app/models/registry.rb

@@ -107,6 +107,16 @@ def get_namespace_from_event(event, fetch_tag = true)
     [namespace, repo, tag_name]
   end
 
+  # Returns a Repository object for the given event. It returns nil if no
+  # repository could be found from the given event.
+  def get_repository_from_event(event, fetch_tag = true)
+    ns, repo_name, = get_namespace_from_event(event, fetch_tag)
+    return if ns.nil?
+    repo = ns.repositories.find_by(name: repo_name)
+    return if repo.nil? || repo.marked?
+    repo
+  end
+
   # Checks whether this registry is reachable. If it is, then an empty string
   # is returned. Otherwise a string will be returned containing the reasoning
   # of the reachability failure.
@@ -140,9 +150,12 @@ def get_tag_from_target(namespace, repo, target)
       "application/vnd.docker.distribution.manifest.list.v2+json"
       get_tag_from_list(namespace, repo)
     else
-      raise "unsupported media type \"#{target["mediaType"]}\""
+      raise ::Portus::RegistryClient::UnsupportedMediaType,
+            "unsupported media type \"#{target["mediaType"]}\""
     end
-  rescue StandardError => e
+  rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+         ::Portus::RegistryClient::UnsupportedMediaType,
+         ::Portus::RegistryClient::ManifestError => e
     logger.info("Could not fetch the tag for target #{target}")
     logger.info("Reason: #{e.message}")
     nil
@@ -172,6 +185,9 @@ def get_tag_from_list(namespace, repository)
   # is used to fetch it, thus the repo name and the digest are needed (and
   # they are contained inside the event's target).
   #
+  # This method calls `::Portus::RegistryClient#manifest` but does not rescue
+  # the possible exceptions. It's up to the called to rescue them.
+  #
   # Returns the name of the tag if found, nil otherwise.
   def get_tag_from_manifest(target)
     _, _, manifest = client.manifest(target["repository"], target["digest"])

app/models/repository.rb

@@ -107,13 +107,11 @@ def self.handle_push_event(event)
 
   # Handle a delete event.
   def self.handle_delete_event(event)
+    # Fetch the repo.
     registry = Registry.find_from_event(event)
     return if registry.nil?
-
-    # Fetch the repo.
-    ns, repo_name, = registry.get_namespace_from_event(event, false)
-    repo = ns.repositories.find_by(name: repo_name)
-    return if repo.nil? || repo.marked?
+    repo = registry.get_repository_from_event(event, false)
+    return if repo.nil?
 
     # Destroy tags and the repository if it's empty now.
     user = User.find_from_event(event)
@@ -162,8 +160,9 @@ def self.id_and_digest_from_event(event, repo)
     if digest.present?
       begin
         id, = Registry.get.client.manifest(repo, digest)
-      rescue StandardError => e
-        logger.warn "Could not fetch manifest for '#{repo}' with digest '#{digest}': " + e.message
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::ManifestError => e
+        logger.warn "Could not fetch manifest for '#{repo}' with digest '#{digest}': " + e.to_s
       end
     end
 
@@ -214,10 +213,11 @@ def self.update_tags(client, repository, tags)
       # Try to fetch the manifest digest of the tag.
       begin
         _, digest, = client.manifest(repository.full_name, tag)
-      rescue StandardError => e
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::ManifestError => e
         logger.tagged("catalog") do
           logger.warn "Could not fetch manifest for '#{repository.full_name}' " \
-            "with tag '#{tag}': " + e.message
+            "with tag '#{tag}': " + e.to_s
         end
         next
       end
@@ -239,7 +239,8 @@ def self.create_tags(client, repository, author, tags)
       # Try to fetch the manifest digest of the tag.
       begin
         id, digest, = client.manifest(repository.full_name, tag)
-      rescue ::Portus::RegistryClient::ManifestError, ::Portus::RequestError => e
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::ManifestError => e
         Rails.logger.info e.to_s
         id = ""
         digest = ""

app/models/tag.rb

@@ -67,7 +67,8 @@ def delete_by_digest!(actor)
 
     begin
       Registry.get.client.delete(repository.full_name, dig, "manifests")
-    rescue StandardError => e
+    rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+           ::Portus::RegistryClient::RegistryError => e
       Rails.logger.error "Could not delete tag on the registry: #{e.message}"
       return false
     end
@@ -127,8 +128,9 @@ def fetch_digest
         _, dig, = client.manifest(repository.full_name, name)
         update_column(:digest, dig)
         dig
-      rescue StandardError => e
-        Rails.logger.error "Could not fetch manifest digest: #{e.message}"
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::ManifestError => e
+        Rails.logger.error "Could not fetch manifest digest: #{e}"
         nil
       end
     else

lib/portus/background/sync.rb

@@ -59,9 +59,10 @@ def execute!
           # the DB in an unknown state because of an update failure.
           ActiveRecord::Base.transaction { update_registry!(cat) }
           @executed = true
-        rescue EOFError, *::Portus::Errors::NET,
+        rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+               ::Portus::RegistryClient::ManifestError,
                ::Portus::Errors::NoBearerRealmException, ::Portus::Errors::AuthorizationError,
-               ::Portus::Errors::NotFoundError, ::Portus::Errors::CredentialsMissingError => e
+               ::Portus::Errors::CredentialsMissingError => e
           Rails.logger.warn "Exception: #{e.message}"
         end
       end

lib/portus/errors.rb

@@ -6,7 +6,7 @@ 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::ECONNRESET, Errno::ECONNREFUSED, Errno::EHOSTUNREACH, EOFError,
            Errno::ETIMEDOUT, Net::OpenTimeout, Net::ReadTimeout].freeze
 
     # Returns a string with a message representing the given exception.

lib/portus/registry_client.rb

@@ -15,6 +15,10 @@ class RegistryClient
     # fetch has given a bad HTTP status code.
     class ManifestError < StandardError; end
 
+    # UnsupportedMediaType is the exception to be raised when the target
+    # mediaType given by the Registry is unsupported.
+    class UnsupportedMediaType < StandardError; end
+
     # Exception being raised when we get an error from the Registry API that we
     # don't know how to handle.
     class RegistryError < StandardError; end
@@ -84,21 +88,44 @@ def manifest(repository, tag = "latest")
     # Returns an array of hashes which contain two keys:
     #   - name: a string containing the name of the repository.
     #   - tags: an array containing the available tags for the repository.
+    #
+    # Three different exceptions might be raised:
+    #
+    #   - ::Portus::RequestError: there was a request error with the registry
+    #     (e.g. a timeout).
+    #   - ::Portus::Errors::NotFoundError: the given manifest was not found.
+    #   - ::Portus::RegistryClient::RegistryError: there was an unknown problem
+    #     with the request.
     def catalog
       res = paged_response("_catalog", "repositories")
       add_tags(res)
     end
 
     # Returns an array containing the list of tags. If something goes wrong,
     # then it raises an exception.
+    #
+    # Three different exceptions might be raised:
+    #
+    #   - ::Portus::RequestError: there was a request error with the registry
+    #     (e.g. a timeout).
+    #   - ::Portus::Errors::NotFoundError: the given manifest was not found.
+    #   - ::Portus::RegistryClient::RegistryError: there was an unknown problem
+    #     with the request.
     def tags(repository)
       paged_response("#{repository}/tags/list", "tags")
     end
 
     # Deletes a blob/manifest of the specified image. Returns true if the
-    # request was successful, otherwise it raises an exception.
+    # request was successful, otherwise it raises an exception. Three different
+    # exceptions might be raised:
+    #
+    #   - ::Portus::RequestError: there was a request error with the registry
+    #     (e.g. a timeout).
+    #   - ::Portus::Errors::NotFoundError: the given manifest was not found.
+    #   - ::Portus::RegistryClient::RegistryError: there was an unknown problem
+    #     with the request.
     def delete(name, digest, object = "blobs")
-      res = perform_request("#{name}/#{object}/#{digest}", "delete")
+      res = safe_request("#{name}/#{object}/#{digest}", "delete")
       if res.code.to_i == 202
         true
       elsif res.code.to_i == 404 || res.code.to_i == 405
@@ -112,9 +139,12 @@ def delete(name, digest, object = "blobs")
 
     protected
 
-    # Returns all the items that could be extracted from the given link that
-    # are indexed by the given field in a successful response. If anything goes
-    # wrong, it raises an exception.
+    # Returns all the items that could be extracted from the given link that are
+    # indexed by the given field in a successful response.
+    #
+    # If anything goes wrong, it raises an exception: ::Portus::RequestError,
+    # ::Portus::Errors::NotFoundError or
+    # ::Portus::RegistryClient::RegistryError.
     def paged_response(link, field)
       res = []
       link += "?n=#{APP_CONFIG["registry"]["catalog_page"]["value"]}"
@@ -131,9 +161,12 @@ def paged_response(link, field)
     # an array of the items:
     #   - The parsed response body.
     #   - The link to the next page.
-    # On error it will raise the proper exception.
+    #
+    # On error it will raise the proper exception: ::Portus::RequestError,
+    # ::Portus::Errors::NotFoundError or
+    # ::Portus::RegistryClient::RegistryError.
     def get_page(link)
-      res = perform_request(link)
+      res = safe_request(link)
       if res.code.to_i == 200
         [JSON.parse(res.body), fetch_link(res["link"])]
       elsif res.code.to_i == 404
@@ -156,14 +189,19 @@ def fetch_link(header)
     # problem while fetching a repository's tag, it will return an empty array.
     # Otherwise it will return an array with the results as specified in the
     # documentation of the `catalog` method.
+    #
+    # It rescues the exceptions that might be raised by `#tags`, so if a fetch
+    # fails for a particular repository, this method tries to fetch the tags for
+    # other methods.
     def add_tags(repositories)
       return [] if repositories.nil?
 
       result = []
       repositories.each do |repo|
         ts = tags(repo)
         result << { "name" => repo, "tags" => ts } if ts.present?
-      rescue StandardError => e
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::RegistryError => e
         Rails.logger.debug "Could not get tags for repo: #{repo}: #{e.message}."
       end
       result

lib/tasks/portus/tags.rake

@@ -43,7 +43,7 @@ namespace :portus do
   task :update_tags, [:update] => [:environment] do |_, args|
     warn_user!
 
-    tags = tags_tp_update(args[:update])
+    tags = tags_to_update(args[:update])
 
     # And for each tag fetch its digest and update the DB.
     client = Registry.get.client
@@ -53,9 +53,10 @@ namespace :portus do
 
       begin
         id, digest, = client.manifest(t.repository.full_name, t.name)
-        t.update_attributes(digest: digest, image_id: id)
-      rescue StandardError => e
-        puts "Could not get the manifest for #{repo_name}: #{e.message}"
+        t.update(digest: digest, image_id: id)
+      rescue ::Portus::RequestError, ::Portus::Errors::NotFoundError,
+             ::Portus::RegistryClient::ManifestError => e
+        puts "Could not get the manifest for #{repo_name}: #{e}"
       end
     end
     puts

spec/models/registry_spec.rb

@@ -36,11 +36,11 @@ def client
     o = nil
     if @should_fail
       def o.manifest(*_)
-        raise StandardError, "Some message"
+        raise ::Portus::RegistryClient::ManifestError, "Some message"
       end
 
       def o.tags(*_)
-        raise StandardError, "Some message"
+        raise ::Portus::RegistryClient::ManifestError, "Some message"
       end
     else
       def o.manifest(*_)