From 82d06b1673a4ee8ceadca986733f007eccd89124 Mon Sep 17 00:00:00 2001 From: Ashique Saidalavi Date: Tue, 12 Apr 2022 19:26:11 +0530 Subject: [PATCH 1/3] Fixed the invocation of refresh! method on SharedCredentials Signed-off-by: Ashique Saidalavi --- gems/aws-sdk-core/lib/aws-sdk-core/plugins/retry_errors.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 index a9c3631665a..2e291c5300f 100644 --- 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 @@ -383,7 +383,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 should_refresh?(context, error) context.http_request.body.rewind context.http_response.reset call(context) @@ -399,6 +399,11 @@ def should_retry?(context, error) response_truncatable?(context) end + def should_refresh?(context, error) + error.expired_credentials? && + context.config.credentials.respond_to?(:refresh!) + end + def retry_limit(context) context.config.retry_limit end From adaed242e76661e9a8dded670ffa4fa20e9c0e9d Mon Sep 17 00:00:00 2001 From: Ashique Saidalavi Date: Tue, 12 Apr 2022 19:31:14 +0530 Subject: [PATCH 2/3] Updated the changelog Signed-off-by: Ashique Saidalavi --- gems/aws-sdk-core/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/gems/aws-sdk-core/CHANGELOG.md b/gems/aws-sdk-core/CHANGELOG.md index 0d05641ffe8..bfc17800459 100644 --- a/gems/aws-sdk-core/CHANGELOG.md +++ b/gems/aws-sdk-core/CHANGELOG.md @@ -1,5 +1,6 @@ Unreleased Changes ------------------ +* Issue - Fix for refresh! method is being called on Aws::SharedCredentials (#2685) 3.130.0 (2022-03-11) ------------------ From 9c6d2d1d005056ba302aa4ac9c1f3379481d10b2 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Tue, 12 Apr 2022 11:18:06 -0700 Subject: [PATCH 3/3] Add refresh! check to both handlers and add specs --- gems/aws-sdk-core/CHANGELOG.md | 3 ++- .../lib/aws-sdk-core/plugins/retry_errors.rb | 11 ++++++++--- .../spec/aws/plugins/retry_errors_legacy_spec.rb | 9 +++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/gems/aws-sdk-core/CHANGELOG.md b/gems/aws-sdk-core/CHANGELOG.md index bfc17800459..e8c2f05fbe4 100644 --- a/gems/aws-sdk-core/CHANGELOG.md +++ b/gems/aws-sdk-core/CHANGELOG.md @@ -1,6 +1,7 @@ Unreleased Changes ------------------ -* Issue - Fix for refresh! method is being called on Aws::SharedCredentials (#2685) + +* 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 index 2e291c5300f..e7bb4f5cd98 100644 --- 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 should_refresh?(context, error) + context.config.credentials.refresh! if refresh_credentials?(context, error) context.http_request.body.rewind context.http_response.reset call(context) @@ -399,7 +404,7 @@ def should_retry?(context, error) response_truncatable?(context) end - def should_refresh?(context, error) + def refresh_credentials?(context, error) error.expired_credentials? && context.config.credentials.respond_to?(:refresh!) 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 index 795868baf14..067231e8004 100644 --- 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(