From 079028d66dbd7f357d3085eb0662dd957221409d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maksymilian=20Bogu=C5=84?= Date: Wed, 4 Sep 2024 14:31:50 +0200 Subject: [PATCH] Introduce aws region into the AWC config cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maksymilian BoguĊ„ --- pkg/scalers/aws/aws_common.go | 4 +-- pkg/scalers/aws/aws_config_cache.go | 14 ++++---- pkg/scalers/aws/aws_config_cache_test.go | 35 +++++++++++++++---- pkg/scalers/aws_cloudwatch_scaler.go | 2 +- pkg/scalers/aws_dynamodb_scaler.go | 2 +- pkg/scalers/aws_dynamodb_streams_scaler.go | 2 +- pkg/scalers/aws_kinesis_stream_scaler.go | 2 +- pkg/scalers/aws_sqs_queue_scaler.go | 2 +- .../resolver/aws_secretmanager_handler.go | 2 +- 9 files changed, 44 insertions(+), 21 deletions(-) diff --git a/pkg/scalers/aws/aws_common.go b/pkg/scalers/aws/aws_common.go index ed98e2b219d..721ff5bcb18 100644 --- a/pkg/scalers/aws/aws_common.go +++ b/pkg/scalers/aws/aws_common.go @@ -137,6 +137,6 @@ func GetAwsAuthorization(uniqueKey string, podIdentity kedav1alpha1.AuthPodIdent } // ClearAwsConfig wraps the removal of the config from the cache -func ClearAwsConfig(awsAuthorization AuthorizationMetadata) { - awsSharedCredentialsCache.RemoveCachedEntry(awsAuthorization) +func ClearAwsConfig(awsRegion string, awsAuthorization AuthorizationMetadata) { + awsSharedCredentialsCache.RemoveCachedEntry(awsRegion, awsAuthorization) } diff --git a/pkg/scalers/aws/aws_config_cache.go b/pkg/scalers/aws/aws_config_cache.go index 684e45c743b..67b0d664d3a 100644 --- a/pkg/scalers/aws/aws_config_cache.go +++ b/pkg/scalers/aws/aws_config_cache.go @@ -68,12 +68,12 @@ func newSharedConfigsCache() sharedConfigCache { // getCacheKey returns a unique key based on given AuthorizationMetadata. // As it can contain sensitive data, the key is hashed to not expose secrets -func (a *sharedConfigCache) getCacheKey(awsAuthorization AuthorizationMetadata) string { - key := "keda" +func (a *sharedConfigCache) getCacheKey(awsRegion string, awsAuthorization AuthorizationMetadata) string { + key := "keda-" + awsRegion if awsAuthorization.AwsAccessKeyID != "" { - key = fmt.Sprintf("%s-%s-%s", awsAuthorization.AwsAccessKeyID, awsAuthorization.AwsSecretAccessKey, awsAuthorization.AwsSessionToken) + key = fmt.Sprintf("%s-%s-%s-%s", awsAuthorization.AwsAccessKeyID, awsAuthorization.AwsSecretAccessKey, awsAuthorization.AwsSessionToken, awsRegion) } else if awsAuthorization.AwsRoleArn != "" { - key = awsAuthorization.AwsRoleArn + key = fmt.Sprintf("%s-%s", awsAuthorization.AwsRoleArn, awsRegion) } // to avoid sensitive data as key and to use a constant key size, // we hash the key with sha3 @@ -89,7 +89,7 @@ func (a *sharedConfigCache) getCacheKey(awsAuthorization AuthorizationMetadata) func (a *sharedConfigCache) GetCredentials(ctx context.Context, awsRegion string, awsAuthorization AuthorizationMetadata) (*aws.Config, error) { a.Lock() defer a.Unlock() - key := a.getCacheKey(awsAuthorization) + key := a.getCacheKey(awsRegion, awsAuthorization) if cachedEntry, exists := a.items[key]; exists { cachedEntry.usages[awsAuthorization.TriggerUniqueKey] = true a.items[key] = cachedEntry @@ -125,10 +125,10 @@ func (a *sharedConfigCache) GetCredentials(ctx context.Context, awsRegion string // RemoveCachedEntry removes the usage of an AuthorizationMetadata from the cached item. // If there isn't any usage of a given cached item (because there isn't any trigger using the aws.Config), // we also remove it from the cache -func (a *sharedConfigCache) RemoveCachedEntry(awsAuthorization AuthorizationMetadata) { +func (a *sharedConfigCache) RemoveCachedEntry(awsRegion string, awsAuthorization AuthorizationMetadata) { a.Lock() defer a.Unlock() - key := a.getCacheKey(awsAuthorization) + key := a.getCacheKey(awsRegion, awsAuthorization) if cachedEntry, exists := a.items[key]; exists { // Delete the TriggerUniqueKey from usages delete(cachedEntry.usages, awsAuthorization.TriggerUniqueKey) diff --git a/pkg/scalers/aws/aws_config_cache_test.go b/pkg/scalers/aws/aws_config_cache_test.go index d94247a6fee..39566815071 100644 --- a/pkg/scalers/aws/aws_config_cache_test.go +++ b/pkg/scalers/aws/aws_config_cache_test.go @@ -34,7 +34,7 @@ func TestGetCredentialsReturnNewItemAndStoreItIfNotExist(t *testing.T) { TriggerUniqueKey: "test-key", }, } - cacheKey := cache.getCacheKey(config.awsAuthorization) + cacheKey := cache.getCacheKey(config.awsRegion, config.awsAuthorization) _, err := cache.GetCredentials(context.Background(), config.awsRegion, config.awsAuthorization) assert.NoError(t, err) assert.Contains(t, cache.items, cacheKey) @@ -52,7 +52,7 @@ func TestGetCredentialsReturnCachedItemIfExist(t *testing.T) { } cfg := aws.Config{} cfg.AppID = "test1-app" - cacheKey := cache.getCacheKey(config.awsAuthorization) + cacheKey := cache.getCacheKey(config.awsRegion, config.awsAuthorization) cache.items[cacheKey] = cacheEntry{ config: &cfg, usages: map[string]bool{ @@ -76,14 +76,14 @@ func TestRemoveCachedEntryRemovesCachedItemIfNotUsages(t *testing.T) { } cfg := aws.Config{} cfg.AppID = "test2-app" - cacheKey := cache.getCacheKey(config.awsAuthorization) + cacheKey := cache.getCacheKey(config.awsRegion, config.awsAuthorization) cache.items[cacheKey] = cacheEntry{ config: &cfg, usages: map[string]bool{ config.awsAuthorization.TriggerUniqueKey: true, }, } - cache.RemoveCachedEntry(config.awsAuthorization) + cache.RemoveCachedEntry(config.awsRegion, config.awsAuthorization) assert.NotContains(t, cache.items, cacheKey) } @@ -98,7 +98,7 @@ func TestRemoveCachedEntryNotRemoveCachedItemIfUsages(t *testing.T) { } cfg := aws.Config{} cfg.AppID = "test3-app" - cacheKey := cache.getCacheKey(config.awsAuthorization) + cacheKey := cache.getCacheKey(config.awsRegion, config.awsAuthorization) cache.items[cacheKey] = cacheEntry{ config: &cfg, usages: map[string]bool{ @@ -106,6 +106,29 @@ func TestRemoveCachedEntryNotRemoveCachedItemIfUsages(t *testing.T) { "other-usage": true, }, } - cache.RemoveCachedEntry(config.awsAuthorization) + cache.RemoveCachedEntry(config.awsRegion, config.awsAuthorization) assert.Contains(t, cache.items, cacheKey) } + +func TestCredentialsShouldBeCachedPerRegion(t *testing.T) { + cache := newSharedConfigsCache() + cache.logger = logr.Discard() + config1 := awsConfigMetadata{ + awsRegion: "test4-region1", + awsAuthorization: AuthorizationMetadata{ + TriggerUniqueKey: "test4-key1", + }, + } + config2 := awsConfigMetadata{ + awsRegion: "test4-region2", + awsAuthorization: AuthorizationMetadata{ + TriggerUniqueKey: "test4-key2", + }, + } + cred1, err1 := cache.GetCredentials(context.Background(), config1.awsRegion, config1.awsAuthorization) + cred2, err2 := cache.GetCredentials(context.Background(), config2.awsRegion, config2.awsAuthorization) + + assert.NoError(t, err1) + assert.NoError(t, err2) + assert.NotEqual(t, cred1, cred2, "Credentials should be stored per region") +} diff --git a/pkg/scalers/aws_cloudwatch_scaler.go b/pkg/scalers/aws_cloudwatch_scaler.go index a07db246f80..c5bfb96d72d 100644 --- a/pkg/scalers/aws_cloudwatch_scaler.go +++ b/pkg/scalers/aws_cloudwatch_scaler.go @@ -214,7 +214,7 @@ func (s *awsCloudwatchScaler) GetMetricSpecForScaling(context.Context) []v2.Metr } func (s *awsCloudwatchScaler) Close(context.Context) error { - awsutils.ClearAwsConfig(s.metadata.awsAuthorization) + awsutils.ClearAwsConfig(s.metadata.AwsRegion, s.metadata.awsAuthorization) return nil } diff --git a/pkg/scalers/aws_dynamodb_scaler.go b/pkg/scalers/aws_dynamodb_scaler.go index f3c9bcac1f6..43f1f5dbc82 100644 --- a/pkg/scalers/aws_dynamodb_scaler.go +++ b/pkg/scalers/aws_dynamodb_scaler.go @@ -164,7 +164,7 @@ func (s *awsDynamoDBScaler) GetMetricSpecForScaling(context.Context) []v2.Metric } func (s *awsDynamoDBScaler) Close(context.Context) error { - awsutils.ClearAwsConfig(s.metadata.awsAuthorization) + awsutils.ClearAwsConfig(s.metadata.awsRegion, s.metadata.awsAuthorization) return nil } diff --git a/pkg/scalers/aws_dynamodb_streams_scaler.go b/pkg/scalers/aws_dynamodb_streams_scaler.go index a8448a46408..2e75fb8c3d7 100644 --- a/pkg/scalers/aws_dynamodb_streams_scaler.go +++ b/pkg/scalers/aws_dynamodb_streams_scaler.go @@ -169,7 +169,7 @@ func getDynamoDBStreamsArn(ctx context.Context, db dynamodb.DescribeTableAPIClie } func (s *awsDynamoDBStreamsScaler) Close(_ context.Context) error { - awsutils.ClearAwsConfig(s.metadata.awsAuthorization) + awsutils.ClearAwsConfig(s.metadata.awsRegion, s.metadata.awsAuthorization) return nil } diff --git a/pkg/scalers/aws_kinesis_stream_scaler.go b/pkg/scalers/aws_kinesis_stream_scaler.go index 65fb90a4e23..acf0e73aeac 100644 --- a/pkg/scalers/aws_kinesis_stream_scaler.go +++ b/pkg/scalers/aws_kinesis_stream_scaler.go @@ -143,7 +143,7 @@ func createKinesisClient(ctx context.Context, metadata *awsKinesisStreamMetadata } func (s *awsKinesisStreamScaler) Close(context.Context) error { - awsutils.ClearAwsConfig(s.metadata.awsAuthorization) + awsutils.ClearAwsConfig(s.metadata.awsRegion, s.metadata.awsAuthorization) return nil } diff --git a/pkg/scalers/aws_sqs_queue_scaler.go b/pkg/scalers/aws_sqs_queue_scaler.go index 1c5976685fb..d6ad5c122b7 100644 --- a/pkg/scalers/aws_sqs_queue_scaler.go +++ b/pkg/scalers/aws_sqs_queue_scaler.go @@ -202,7 +202,7 @@ func createSqsClient(ctx context.Context, metadata *awsSqsQueueMetadata) (*sqs.C } func (s *awsSqsQueueScaler) Close(context.Context) error { - awsutils.ClearAwsConfig(s.metadata.awsAuthorization) + awsutils.ClearAwsConfig(s.metadata.awsRegion, s.metadata.awsAuthorization) return nil } diff --git a/pkg/scaling/resolver/aws_secretmanager_handler.go b/pkg/scaling/resolver/aws_secretmanager_handler.go index 3d10ba2e1e5..1054459fafe 100644 --- a/pkg/scaling/resolver/aws_secretmanager_handler.go +++ b/pkg/scaling/resolver/aws_secretmanager_handler.go @@ -109,5 +109,5 @@ func (ash *AwsSecretManagerHandler) Initialize(ctx context.Context, client clien } func (ash *AwsSecretManagerHandler) Stop() { - awsutils.ClearAwsConfig(ash.awsMetadata) + awsutils.ClearAwsConfig(ash.secretManager.Region, ash.awsMetadata) }