From 67061690e9c9ec0e68e76e996a72f75e9f40f5f1 Mon Sep 17 00:00:00 2001 From: Mael Regnery Date: Tue, 5 Dec 2023 15:43:53 +0100 Subject: [PATCH 1/3] fix: renew the session token when the token expires --- storage/s3.go | 28 ++++++++++++++++++++++------ storage/s3_test.go | 16 ++++++++++------ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/storage/s3.go b/storage/s3.go index f6ebcffd3..5676c0e57 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -1235,9 +1235,9 @@ func (sc *SessionCache) newSession(ctx context.Context, opts Options) (*session. WithLogger(sdkLogger{}) } - awsCfg.Retryer = newCustomRetryer(opts.MaxRetries) + awsCfg.Retryer = newCustomRetryer(sc, opts.MaxRetries) - useSharedConfig := session.SharedConfigEnable + useSharedConfig := session.SharedConfigDisable { // Reverse of what the SDK does: if AWS_SDK_LOAD_CONFIG is 0 (or a // falsy value) disable shared configs @@ -1276,7 +1276,7 @@ func (sc *SessionCache) newSession(ctx context.Context, opts Options) (*session. return sess, nil } -func (sc *SessionCache) clear() { +func (sc *SessionCache) Clear() { sc.Lock() defer sc.Unlock() sc.sessions = map[Options]*session.Session{} @@ -1324,10 +1324,12 @@ func setSessionRegion(ctx context.Context, sess *session.Session, bucket string) // error codes. Such as, retry for S3 InternalError code. type customRetryer struct { client.DefaultRetryer + sc *SessionCache } -func newCustomRetryer(maxRetries int) *customRetryer { +func newCustomRetryer(sc *SessionCache, maxRetries int) *customRetryer { return &customRetryer{ + sc: sc, DefaultRetryer: client.DefaultRetryer{ NumMaxRetries: maxRetries, }, @@ -1337,13 +1339,27 @@ func newCustomRetryer(maxRetries int) *customRetryer { // ShouldRetry overrides SDK's built in DefaultRetryer, adding custom retry // logics that are not included in the SDK. func (c *customRetryer) ShouldRetry(req *request.Request) bool { - shouldRetry := errHasCode(req.Error, "InternalError") || errHasCode(req.Error, "RequestTimeTooSkewed") || errHasCode(req.Error, "SlowDown") || strings.Contains(req.Error.Error(), "connection reset") || strings.Contains(req.Error.Error(), "connection timed out") + log.Error(log.ErrorMessage{ + Command: "retrier", + Err: req.Error.Error(), + }) + + shouldRetry := errHasCode(req.Error, "InternalError") || errHasCode(req.Error, "RequestTimeTooSkewed") || errHasCode(req.Error, "SlowDown") || strings.Contains(req.Error.Error(), "connection reset") || strings.Contains(req.Error.Error(), "connection timed out") || errHasCode(req.Error, "ExpiredToken") || errHasCode(req.Error, "ExpiredTokenException") + + if errHasCode(req.Error, "ExpiredToken") || errHasCode(req.Error, "ExpiredTokenException") { + log.Debug(log.DebugMessage{ + Err: "Clearing the token", + }) + + c.sc.Clear() + } + if !shouldRetry { shouldRetry = c.DefaultRetryer.ShouldRetry(req) } // Errors related to tokens - if errHasCode(req.Error, "ExpiredToken") || errHasCode(req.Error, "ExpiredTokenException") || errHasCode(req.Error, "InvalidToken") { + if errHasCode(req.Error, "InvalidToken") { return false } diff --git a/storage/s3_test.go b/storage/s3_test.go index aee355b1c..36a7d9f51 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -97,7 +97,7 @@ func TestNewSessionPathStyle(t *testing.T) { } func TestNewSessionWithRegionSetViaEnv(t *testing.T) { - globalSessionCache.clear() + globalSessionCache.Clear() const expectedRegion = "us-west-2" @@ -116,7 +116,7 @@ func TestNewSessionWithRegionSetViaEnv(t *testing.T) { } func TestNewSessionWithNoSignRequest(t *testing.T) { - globalSessionCache.clear() + globalSessionCache.Clear() sess, err := globalSessionCache.newSession(context.Background(), Options{ NoSignRequest: true, @@ -190,7 +190,7 @@ aws_secret_access_key = p2_profile_access_key` } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - globalSessionCache.clear() + globalSessionCache.Clear() sess, err := globalSessionCache.newSession(context.Background(), Options{ Profile: tc.profileName, CredentialFile: tc.fileName, @@ -538,8 +538,12 @@ func TestS3Retry(t *testing.T) { for _, tc := range testcases { tc := tc t.Run(tc.name, func(t *testing.T) { + sessionCache := &SessionCache{ + sessions: map[Options]*session.Session{}, + } + sess := unit.Session - sess.Config.Retryer = newCustomRetryer(expectedRetry) + sess.Config.Retryer = newCustomRetryer(sessionCache, expectedRetry) mockAPI := s3.New(sess) mockS3 := &S3{ @@ -1041,7 +1045,7 @@ func TestSessionRegionDetection(t *testing.T) { opts.bucket = tc.bucket } - globalSessionCache.clear() + globalSessionCache.Clear() sess, err := globalSessionCache.newSession(context.Background(), opts) if err != nil { @@ -1241,7 +1245,7 @@ func TestAWSLogLevel(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { - globalSessionCache.clear() + globalSessionCache.Clear() sess, err := globalSessionCache.newSession(context.Background(), Options{ LogLevel: log.LevelFromString(tc.level), }) From 7b370e3224e766f04db1479322e23a14af380888 Mon Sep 17 00:00:00 2001 From: Mael Regnery Date: Wed, 6 Dec 2023 14:10:25 +0100 Subject: [PATCH 2/3] fix test --- storage/s3.go | 5 ----- storage/s3_test.go | 4 ++-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/storage/s3.go b/storage/s3.go index 5676c0e57..94bbdffd0 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -1339,11 +1339,6 @@ func newCustomRetryer(sc *SessionCache, maxRetries int) *customRetryer { // ShouldRetry overrides SDK's built in DefaultRetryer, adding custom retry // logics that are not included in the SDK. func (c *customRetryer) ShouldRetry(req *request.Request) bool { - log.Error(log.ErrorMessage{ - Command: "retrier", - Err: req.Error.Error(), - }) - shouldRetry := errHasCode(req.Error, "InternalError") || errHasCode(req.Error, "RequestTimeTooSkewed") || errHasCode(req.Error, "SlowDown") || strings.Contains(req.Error.Error(), "connection reset") || strings.Contains(req.Error.Error(), "connection timed out") || errHasCode(req.Error, "ExpiredToken") || errHasCode(req.Error, "ExpiredTokenException") if errHasCode(req.Error, "ExpiredToken") || errHasCode(req.Error, "ExpiredTokenException") { diff --git a/storage/s3_test.go b/storage/s3_test.go index 36a7d9f51..6dafa628a 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -489,12 +489,12 @@ func TestS3Retry(t *testing.T) { { name: "ExpiredToken", err: awserr.New("ExpiredToken", "expired token", nil), - expectedRetry: 0, + expectedRetry: 5, }, { name: "ExpiredTokenException", err: awserr.New("ExpiredTokenException", "expired token exception", nil), - expectedRetry: 0, + expectedRetry: 5, }, // Invalid Token errors From d70a2b5bff34d7a02265aef2f1a7f648ea70156b Mon Sep 17 00:00:00 2001 From: Mael Regnery Date: Wed, 24 Jan 2024 11:59:02 +0100 Subject: [PATCH 3/3] revert change --- storage/s3.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/s3.go b/storage/s3.go index 94bbdffd0..849a0a51e 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -1237,7 +1237,7 @@ func (sc *SessionCache) newSession(ctx context.Context, opts Options) (*session. awsCfg.Retryer = newCustomRetryer(sc, opts.MaxRetries) - useSharedConfig := session.SharedConfigDisable + useSharedConfig := session.SharedConfigEnable { // Reverse of what the SDK does: if AWS_SDK_LOAD_CONFIG is 0 (or a // falsy value) disable shared configs