Fixed the invocation of refresh! method on SharedCredentials (#2686)

aws · Apr 12, 2022 · 2c58c2a · 2c58c2a
1 parent 3e79476
commit 2c58c2a
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 2 deletions.
diff --git a/gems/aws-sdk-core/CHANGELOG.md b/gems/aws-sdk-core/CHANGELOG.md
@@ -1,6 +1,8 @@
 Unreleased Changes
 ------------------
 
+* Issue - Don't call `refresh!` on non-refreshable `Credentials` when retrying errors (#2685).
+
 3.130.0 (2022-03-11)
 ------------------
 

diff --git a/gems/aws-sdk-core/lib/aws-sdk-core/plugins/retry_errors.rb b/gems/aws-sdk-core/lib/aws-sdk-core/plugins/retry_errors.rb
@@ -313,12 +313,17 @@ def exponential_backoff(retries)
 
         def retry_request(context, error)
           context.retries += 1
-          context.config.credentials.refresh! if error.expired_credentials?
+          context.config.credentials.refresh! if refresh_credentials?(context, error)
           context.http_request.body.rewind
           context.http_response.reset
           call(context)
         end
 
+        def refresh_credentials?(context, error)
+          error.expired_credentials? &&
+            context.config.credentials.respond_to?(:refresh!)
+        end
+
         def add_retry_headers(context)
           request_pairs = {
             'attempt' => context.retries,
@@ -383,7 +388,7 @@ def retry_if_possible(response, error_inspector)
         def retry_request(context, error)
           delay_retry(context)
           context.retries += 1
-          context.config.credentials.refresh! if error.expired_credentials?
+          context.config.credentials.refresh! if refresh_credentials?(context, error)
           context.http_request.body.rewind
           context.http_response.reset
           call(context)
@@ -399,6 +404,11 @@ def should_retry?(context, error)
             response_truncatable?(context)
         end
 
+        def refresh_credentials?(context, error)
+          error.expired_credentials? &&
+            context.config.credentials.respond_to?(:refresh!)
+        end
+
         def retry_limit(context)
           context.config.retry_limit
         end

diff --git a/gems/aws-sdk-core/spec/aws/plugins/retry_errors_legacy_spec.rb b/gems/aws-sdk-core/spec/aws/plugins/retry_errors_legacy_spec.rb
@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require_relative '../../spec_helper'
+require_relative '../../retry_errors_helper'
 
 module Aws
   module Plugins
@@ -207,12 +208,20 @@ module Plugins
       end
 
       it 'retries if creds expire and are refreshable' do
+        # Note: this adds the refresh! method to credentials
         expect(credentials).to receive(:refresh!).exactly(3).times
         resp.error = RetryErrorsSvc::Errors::AuthFailure.new(nil, nil)
         handle { |_context| resp }
         expect(resp.context.retries).to eq(3)
       end
 
+      it 'does not call refresh! when error is expired credentials and clock skew' do
+        resp.error = RetryErrorsSvc::Errors::RequestExpired.new(nil, nil)
+        resp.context.http_response.headers['date'] = (Time.now + 10*60).iso8601
+        handle { |_context| resp }
+        expect(resp.context.retries).to eq(1)
+      end
+
       it 'retries if endpoint discovery error is detected' do
         config.api.endpoint_operation = :describe_endpoints
         DescribeEndpointsRequest = Seahorse::Model::Shapes::StructureShape.new(