From c5dc30f57cfbb8d2471c05c6c68c99ab0a526ee2 Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Mon, 11 Jul 2022 16:16:23 +0100 Subject: [PATCH 1/3] Move RegistryClient from maven to common --- common/lib/dependabot/registry_client.rb | 55 ++++++++++++++++++ .../spec/dependabot}/registry_client_spec.rb | 4 +- maven/lib/dependabot/maven.rb | 1 - .../file_parser/property_value_finder.rb | 3 +- .../maven/file_parser/repositories_finder.rb | 3 +- maven/lib/dependabot/maven/metadata_finder.rb | 5 +- maven/lib/dependabot/maven/registry_client.rb | 57 ------------------- .../maven/update_checker/version_finder.rb | 5 +- 8 files changed, 67 insertions(+), 66 deletions(-) create mode 100644 common/lib/dependabot/registry_client.rb rename {maven/spec/dependabot/maven => common/spec/dependabot}/registry_client_spec.rb (98%) delete mode 100644 maven/lib/dependabot/maven/registry_client.rb diff --git a/common/lib/dependabot/registry_client.rb b/common/lib/dependabot/registry_client.rb new file mode 100644 index 00000000000..a1fa7bad4e6 --- /dev/null +++ b/common/lib/dependabot/registry_client.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require "dependabot/shared_helpers" + +# This class provides a thin wrapper around our normal usage of Excon as a simple HTTP client in order to +# provide some minor caching functionality. +# +# This is not used to support full response caching currently, we just use it to ensure we detect unreachable +# hosts and fast-fail on any subsequent requests to them to avoid excessive use of retries and connect- or +# read-timeouts as Maven jobs tend to be sensitive to exceeding our overall 45 minute timeout. +module Dependabot + class RegistryClient + @cached_errors = {} + + def self.get(url:, headers: {}, options: {}) + raise cached_error_for(url) if cached_error_for(url) + + Excon.get( + url, + idempotent: true, + **SharedHelpers.excon_defaults({ headers: headers }.merge(options)) + ) + rescue Excon::Error::Timeout => e + cache_error(url, e) + raise e + end + + def self.head(url:, headers: {}, options: {}) + raise cached_error_for(url) if cached_error_for(url) + + Excon.head( + url, + idempotent: true, + **SharedHelpers.excon_defaults({ headers: headers }.merge(options)) + ) + rescue Excon::Error::Timeout => e + cache_error(url, e) + raise e + end + + def self.clear_cache! + @cached_errors = {} + end + + private_class_method def self.cache_error(url, error) + host = URI(url).host + @cached_errors[host] = error + end + + private_class_method def self.cached_error_for(url) + host = URI(url).host + @cached_errors.fetch(host, nil) + end + end +end diff --git a/maven/spec/dependabot/maven/registry_client_spec.rb b/common/spec/dependabot/registry_client_spec.rb similarity index 98% rename from maven/spec/dependabot/maven/registry_client_spec.rb rename to common/spec/dependabot/registry_client_spec.rb index 6f12984f18a..5d578346b82 100644 --- a/maven/spec/dependabot/maven/registry_client_spec.rb +++ b/common/spec/dependabot/registry_client_spec.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true require "spec_helper" -require "dependabot/maven/registry_client" +require "dependabot/registry_client" -RSpec.describe Dependabot::Maven::RegistryClient do +RSpec.describe Dependabot::RegistryClient do let(:url) { "https://example.com" } let(:maven_defaults) do { idempotent: true } diff --git a/maven/lib/dependabot/maven.rb b/maven/lib/dependabot/maven.rb index 90dbc23f1ed..cf9f2a23901 100644 --- a/maven/lib/dependabot/maven.rb +++ b/maven/lib/dependabot/maven.rb @@ -9,7 +9,6 @@ require "dependabot/maven/metadata_finder" require "dependabot/maven/requirement" require "dependabot/maven/version" -require "dependabot/maven/registry_client" require "dependabot/pull_request_creator/labeler" Dependabot::PullRequestCreator::Labeler. diff --git a/maven/lib/dependabot/maven/file_parser/property_value_finder.rb b/maven/lib/dependabot/maven/file_parser/property_value_finder.rb index bfe03352373..e165262a347 100644 --- a/maven/lib/dependabot/maven/file_parser/property_value_finder.rb +++ b/maven/lib/dependabot/maven/file_parser/property_value_finder.rb @@ -4,6 +4,7 @@ require "dependabot/dependency_file" require "dependabot/maven/file_parser" +require "dependabot/registry_client" # For documentation, see: # - http://maven.apache.org/guides/introduction/introduction-to-the-pom.html @@ -127,7 +128,7 @@ def fetch_remote_parent_pom(group_id, artifact_id, version, pom) url = remote_pom_url(group_id, artifact_id, version, base_url) @maven_responses ||= {} - @maven_responses[url] ||= RegistryClient.get(url: url) + @maven_responses[url] ||= Dependabot::RegistryClient.get(url: url) next unless @maven_responses[url].status == 200 next unless pom?(@maven_responses[url].body) diff --git a/maven/lib/dependabot/maven/file_parser/repositories_finder.rb b/maven/lib/dependabot/maven/file_parser/repositories_finder.rb index 316cb4e9e9c..20b5b3e7d1d 100644 --- a/maven/lib/dependabot/maven/file_parser/repositories_finder.rb +++ b/maven/lib/dependabot/maven/file_parser/repositories_finder.rb @@ -4,6 +4,7 @@ require "dependabot/dependency_file" require "dependabot/maven/file_parser" +require "dependabot/registry_client" require "dependabot/errors" # For documentation, see: @@ -109,7 +110,7 @@ def fetch_remote_parent_pom(group_id, artifact_id, version, repo_urls) url = remote_pom_url(group_id, artifact_id, version, base_url) @maven_responses ||= {} - @maven_responses[url] ||= RegistryClient.get( + @maven_responses[url] ||= Dependabot::RegistryClient.get( url: url, # We attempt to find dependencies in private repos before failing over to the CENTRAL_REPO_URL, # but this can burn a lot of a job's time against slow servers due to our `read_timeout` being 20 seconds. diff --git a/maven/lib/dependabot/maven/metadata_finder.rb b/maven/lib/dependabot/maven/metadata_finder.rb index 7844a2ff0a7..efc7f58b0b0 100644 --- a/maven/lib/dependabot/maven/metadata_finder.rb +++ b/maven/lib/dependabot/maven/metadata_finder.rb @@ -7,6 +7,7 @@ require "dependabot/maven/file_parser" require "dependabot/maven/file_parser/repositories_finder" require "dependabot/maven/utils/auth_headers_finder" +require "dependabot/registry_client" module Dependabot module Maven @@ -104,7 +105,7 @@ def source_from_anywhere_in_pom(pom) def dependency_pom_file return @dependency_pom_file unless @dependency_pom_file.nil? - response = RegistryClient.get( + response = Dependabot::RegistryClient.get( url: "#{maven_repo_dependency_url}/#{dependency.version}/#{dependency_artifact_id}-#{dependency.version}.pom", headers: auth_headers ) @@ -134,7 +135,7 @@ def parent_pom_file(pom) "#{version}/"\ "#{artifact_id}-#{version}.pom" - response = RegistryClient.get( + response = Dependabot::RegistryClient.get( url: substitute_properties_in_source_url(url, pom), headers: auth_headers ) diff --git a/maven/lib/dependabot/maven/registry_client.rb b/maven/lib/dependabot/maven/registry_client.rb deleted file mode 100644 index a96b3be5cf1..00000000000 --- a/maven/lib/dependabot/maven/registry_client.rb +++ /dev/null @@ -1,57 +0,0 @@ -# frozen_string_literal: true - -require "dependabot/shared_helpers" - -# This class provides a thin wrapper around our normal usage of Excon as a simple HTTP client in order to -# provide some minor caching functionality. -# -# This is not used to support full response caching currently, we just use it to ensure we detect unreachable -# hosts and fast-fail on any subsequent requests to them to avoid excessive use of retries and connect- or -# read-timeouts as Maven jobs tend to be sensitive to exceeding our overall 45 minute timeout. -module Dependabot - module Maven - class RegistryClient - @cached_errors = {} - - def self.get(url:, headers: {}, options: {}) - raise cached_error_for(url) if cached_error_for(url) - - Excon.get( - url, - idempotent: true, - **SharedHelpers.excon_defaults({ headers: headers }.merge(options)) - ) - rescue Excon::Error::Timeout => e - cache_error(url, e) - raise e - end - - def self.head(url:, headers: {}, options: {}) - raise cached_error_for(url) if cached_error_for(url) - - Excon.head( - url, - idempotent: true, - **SharedHelpers.excon_defaults({ headers: headers }.merge(options)) - ) - rescue Excon::Error::Timeout => e - cache_error(url, e) - raise e - end - - def self.clear_cache! - @cached_errors = {} - end - - private_class_method def self.cache_error(url, error) - host = URI(url).host - @cached_errors[host] = error - end - - private_class_method def self.cached_error_for(url) - host = URI(url).host - @cached_errors.fetch(host, nil) - end - end - end -end diff --git a/maven/lib/dependabot/maven/update_checker/version_finder.rb b/maven/lib/dependabot/maven/update_checker/version_finder.rb index 40197c238ed..d72f4accead 100644 --- a/maven/lib/dependabot/maven/update_checker/version_finder.rb +++ b/maven/lib/dependabot/maven/update_checker/version_finder.rb @@ -7,6 +7,7 @@ require "dependabot/maven/version" require "dependabot/maven/requirement" require "dependabot/maven/utils/auth_headers_finder" +require "dependabot/registry_client" module Dependabot module Maven @@ -138,7 +139,7 @@ def released?(version) @released_check[version] = repositories.any? do |repository_details| url = repository_details.fetch("url") - response = RegistryClient.head( + response = Dependabot::RegistryClient.head( url: dependency_files_url(url, version), headers: repository_details.fetch("auth_headers") ) @@ -160,7 +161,7 @@ def dependency_metadata(repository_details) end def fetch_dependency_metadata(repository_details) - response = RegistryClient.get( + response = Dependabot::RegistryClient.get( url: dependency_metadata_url(repository_details.fetch("url")), headers: repository_details.fetch("auth_headers") ) From f815e99f3c59cce7ac66b4444004ef7622b381eb Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 13 Jul 2022 11:26:10 +0100 Subject: [PATCH 2/3] Clear the RegistryClient after each test --- common/spec/spec_helper.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/common/spec/spec_helper.rb b/common/spec/spec_helper.rb index 1e8d99c9452..7fbeacdd511 100644 --- a/common/spec/spec_helper.rb +++ b/common/spec/spec_helper.rb @@ -10,6 +10,7 @@ require "uri" require "dependabot/dependency_file" +require "dependabot/registry_client" require_relative "dummy_package_manager/dummy" require_relative "warning_monkey_patch" @@ -38,6 +39,11 @@ config.mock_with(:rspec) { |mocks| mocks.verify_partial_doubles = true } config.raise_errors_for_deprecations! + config.after do + # Ensure we clear any cached timeouts between tests + Dependabot::RegistryClient.clear_cache! + end + config.around do |example| if example.metadata[:profile] example_name = example.metadata[:full_description].strip.gsub(/[\s#\.-]/, "_").gsub("::", "_").downcase From 005705a07c73f4dd8ce832ddbd8d79cb35738a7e Mon Sep 17 00:00:00 2001 From: Barry Gordon Date: Wed, 13 Jul 2022 20:19:26 +0100 Subject: [PATCH 3/3] [bundler] Cache unreachable registry hosts --- .../lib/dependabot/bundler/metadata_finder.rb | 21 +++++++------------ .../dependency_source.rb | 7 +++---- .../update_checker/shared_bundler_helpers.rb | 7 +++---- .../update_checker/version_resolver.rb | 7 +++---- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/bundler/lib/dependabot/bundler/metadata_finder.rb b/bundler/lib/dependabot/bundler/metadata_finder.rb index f9caddd8d04..dd08c5177c3 100644 --- a/bundler/lib/dependabot/bundler/metadata_finder.rb +++ b/bundler/lib/dependabot/bundler/metadata_finder.rb @@ -3,6 +3,7 @@ require "excon" require "dependabot/metadata_finders" require "dependabot/metadata_finders/base" +require "dependabot/registry_client" module Dependabot module Bundler @@ -127,10 +128,9 @@ def rubygems_marshalled_gemspec_response "#{dependency.name}-#{dependency.version}.gemspec.rz" response = - Excon.get( - gemspec_uri, - idempotent: true, - **SharedHelpers.excon_defaults(headers: registry_auth_headers) + Dependabot::RegistryClient.get( + url: gemspec_uri, + headers: registry_auth_headers ) return @rubygems_marshalled_gemspec_response = nil if response.status >= 400 @@ -145,10 +145,9 @@ def rubygems_api_response return @rubygems_api_response if defined?(@rubygems_api_response) response = - Excon.get( - "#{registry_url}api/v1/gems/#{dependency.name}.json", - idempotent: true, - **SharedHelpers.excon_defaults(headers: registry_auth_headers) + Dependabot::RegistryClient.get( + url: "#{registry_url}api/v1/gems/#{dependency.name}.json", + headers: registry_auth_headers ) return @rubygems_api_response = {} if response.status >= 400 @@ -186,11 +185,7 @@ def augment_private_response_if_appropriate(response_body) return response_body if source_url rubygems_response = - Excon.get( - "https://rubygems.org/api/v1/gems/#{dependency.name}.json", - idempotent: true, - **SharedHelpers.excon_defaults - ) + Dependabot::RegistryClient.get(url: "https://rubygems.org/api/v1/gems/#{dependency.name}.json") parsed_rubygems_body = JSON.parse(rubygems_response.body) rubygems_digest = parsed_rubygems_body.values_at("version", "authors", "info").hash diff --git a/bundler/lib/dependabot/bundler/update_checker/latest_version_finder/dependency_source.rb b/bundler/lib/dependabot/bundler/update_checker/latest_version_finder/dependency_source.rb index 4a15ec02749..9ee33310020 100644 --- a/bundler/lib/dependabot/bundler/update_checker/latest_version_finder/dependency_source.rb +++ b/bundler/lib/dependabot/bundler/update_checker/latest_version_finder/dependency_source.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require "dependabot/registry_client" require "dependabot/bundler/native_helpers" require "dependabot/bundler/helpers" @@ -84,10 +85,8 @@ def git? def rubygems_versions @rubygems_versions ||= begin - response = Excon.get( - dependency_rubygems_uri, - idempotent: true, - **SharedHelpers.excon_defaults + response = Dependabot::RegistryClient.get( + url: dependency_rubygems_uri ) JSON.parse(response.body). diff --git a/bundler/lib/dependabot/bundler/update_checker/shared_bundler_helpers.rb b/bundler/lib/dependabot/bundler/update_checker/shared_bundler_helpers.rb index bb9008562b1..d70edd00db9 100644 --- a/bundler/lib/dependabot/bundler/update_checker/shared_bundler_helpers.rb +++ b/bundler/lib/dependabot/bundler/update_checker/shared_bundler_helpers.rb @@ -6,6 +6,7 @@ require "dependabot/bundler/update_checker" require "dependabot/bundler/native_helpers" require "dependabot/bundler/helpers" +require "dependabot/registry_client" require "dependabot/shared_helpers" require "dependabot/errors" @@ -182,10 +183,8 @@ def inaccessible_git_dependencies uri = URI.parse(spec.fetch("auth_uri")) next false unless %w(http https).include?(uri.scheme) - Excon.get( - uri.to_s, - idempotent: true, - **SharedHelpers.excon_defaults + Dependabot::RegistryClient.get( + url: uri.to_s ).status == 200 rescue Excon::Error::Socket, Excon::Error::Timeout false diff --git a/bundler/lib/dependabot/bundler/update_checker/version_resolver.rb b/bundler/lib/dependabot/bundler/update_checker/version_resolver.rb index 97da3664795..283be50d497 100644 --- a/bundler/lib/dependabot/bundler/update_checker/version_resolver.rb +++ b/bundler/lib/dependabot/bundler/update_checker/version_resolver.rb @@ -6,6 +6,7 @@ require "dependabot/bundler/update_checker" require "dependabot/bundler/file_updater/lockfile_updater" require "dependabot/bundler/requirement" +require "dependabot/registry_client" require "dependabot/shared_helpers" require "dependabot/errors" @@ -180,10 +181,8 @@ def ruby_version_incompatible?(details) # If no Ruby version is specified, we don't have a problem return false unless details[:ruby_version] - versions = Excon.get( - "https://rubygems.org/api/v1/versions/#{dependency.name}.json", - idempotent: true, - **SharedHelpers.excon_defaults + versions = Dependabot::RegistryClient.get( + url: "https://rubygems.org/api/v1/versions/#{dependency.name}.json" ) # Give the benefit of the doubt if something goes wrong fetching