-
Notifications
You must be signed in to change notification settings - Fork 11
Replace SQS singleton with per-pipeline client cache with TTL eviction #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,25 +19,93 @@ package awsclienthandler | |||||||||||||||||||
| import ( | ||||||||||||||||||||
| "context" | ||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||
| "errors" | ||||||||||||||||||||
| "strings" | ||||||||||||||||||||
| "sync" | ||||||||||||||||||||
| "time" | ||||||||||||||||||||
|
|
||||||||||||||||||||
| "github.com/aws/aws-sdk-go-v2/aws" | ||||||||||||||||||||
| "github.com/aws/aws-sdk-go-v2/service/sqs" | ||||||||||||||||||||
| "sigs.k8s.io/controller-runtime/pkg/log" | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| var ( | ||||||||||||||||||||
| SQSClient *sqs.Client | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| type SQSClientCache struct { | ||||||||||||||||||||
| mu sync.Mutex | ||||||||||||||||||||
| clients map[string]*sqsEntry | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| type sqsEntry struct { | ||||||||||||||||||||
| client *sqs.Client | ||||||||||||||||||||
| awsConfig AWSConfig | ||||||||||||||||||||
| lastUsed time.Time | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+36
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To support credential rotation and updates, we should store the
Suggested change
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| var sqsClientCache = &SQSClientCache{ | ||||||||||||||||||||
| clients: make(map[string]*sqsEntry), | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func InitSQSCache(ctx context.Context, ttl time.Duration) { | ||||||||||||||||||||
| sqsClientCache.startCleanup(ctx, ttl) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func (c *SQSClientCache) getClient(pipelineName string) (*sqsEntry, bool) { | ||||||||||||||||||||
| c.mu.Lock() | ||||||||||||||||||||
| defer c.mu.Unlock() | ||||||||||||||||||||
| entry, ok := c.clients[pipelineName] | ||||||||||||||||||||
| if ok { | ||||||||||||||||||||
| entry.lastUsed = time.Now() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return entry, ok | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func (c *SQSClientCache) setClient(pipelineName string, entry *sqsEntry) { | ||||||||||||||||||||
| c.mu.Lock() | ||||||||||||||||||||
| defer c.mu.Unlock() | ||||||||||||||||||||
| if c.clients == nil { | ||||||||||||||||||||
| c.clients = make(map[string]*sqsEntry) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| c.clients[pipelineName] = entry | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func (c *SQSClientCache) startCleanup(ctx context.Context, ttl time.Duration) { | ||||||||||||||||||||
| if ttl <= 0 { | ||||||||||||||||||||
| return | ||||||||||||||||||||
| } | ||||||||||||||||||||
| ticker := time.NewTicker(ttl / 2) | ||||||||||||||||||||
|
Comment on lines
+69
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||
| go func() { | ||||||||||||||||||||
| defer ticker.Stop() | ||||||||||||||||||||
| for { | ||||||||||||||||||||
| select { | ||||||||||||||||||||
| case <-ctx.Done(): | ||||||||||||||||||||
| return | ||||||||||||||||||||
| case <-ticker.C: | ||||||||||||||||||||
| c.evictStale(ttl) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }() | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func (c *SQSClientCache) evictStale(ttl time.Duration) { | ||||||||||||||||||||
| c.mu.Lock() | ||||||||||||||||||||
| defer c.mu.Unlock() | ||||||||||||||||||||
| now := time.Now() | ||||||||||||||||||||
| for key, entry := range c.clients { | ||||||||||||||||||||
| if now.Sub(entry.lastUsed) > ttl { | ||||||||||||||||||||
| delete(c.clients, key) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // NewSQSClientFromConfig creates and returns an Amazon SQS client using the provided context and AWS configuration. | ||||||||||||||||||||
| func NewSQSClientFromConfig(ctx context.Context, awsConfig *AWSConfig) (*sqs.Client, error) { | ||||||||||||||||||||
| func NewSQSClientFromConfig(ctx context.Context, awsConfig *AWSConfig, pipelineName string) (*sqs.Client, error) { | ||||||||||||||||||||
| logger := log.FromContext(ctx) | ||||||||||||||||||||
| if SQSClient != nil { | ||||||||||||||||||||
| return SQSClient, nil | ||||||||||||||||||||
|
|
||||||||||||||||||||
| entry, ok := sqsClientCache.getClient(pipelineName) | ||||||||||||||||||||
| if ok && awsConfig != nil && entry.awsConfig == *awsConfig { | ||||||||||||||||||||
| logger.Info("Using existing SQS client for the pipeline", "pipelineName", pipelineName) | ||||||||||||||||||||
| return entry.client, nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| logger.Info("Creating new SQS client for the pipeline", "pipelineName", pipelineName) | ||||||||||||||||||||
| cfg, err := getAWSConfig(ctx, awsConfig) | ||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||
| return nil, err | ||||||||||||||||||||
|
|
@@ -48,17 +116,32 @@ func NewSQSClientFromConfig(ctx context.Context, awsConfig *AWSConfig) (*sqs.Cli | |||||||||||||||||||
| o.BaseEndpoint = aws.String(awsConfig.Endpoint) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| SQSClient = sqs.NewFromConfig(cfg, sqsOptions) | ||||||||||||||||||||
| logger.Info("SQS client initialized ...") | ||||||||||||||||||||
| return SQSClient, nil | ||||||||||||||||||||
|
|
||||||||||||||||||||
| sqsClient := sqs.NewFromConfig(cfg, sqsOptions) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| var cfgVal AWSConfig | ||||||||||||||||||||
| if awsConfig != nil { | ||||||||||||||||||||
| cfgVal = *awsConfig | ||||||||||||||||||||
| } | ||||||||||||||||||||
| newEntry := &sqsEntry{client: sqsClient, awsConfig: cfgVal, lastUsed: time.Now()} | ||||||||||||||||||||
| sqsClientCache.setClient(pipelineName, newEntry) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return sqsClient, nil | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| func DeleteSQSClient(pipelineName string) { | ||||||||||||||||||||
| sqsClientCache.mu.Lock() | ||||||||||||||||||||
| defer sqsClientCache.mu.Unlock() | ||||||||||||||||||||
| delete(sqsClientCache.clients, pipelineName) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // GetSQSClient returns the initialized Amazon SQS client instance. | ||||||||||||||||||||
| func GetSQSClient() (*sqs.Client, error) { | ||||||||||||||||||||
| if SQSClient == nil { | ||||||||||||||||||||
| return nil, errors.New("SQS client not initialized yet") | ||||||||||||||||||||
| func GetSQSClient(pipelineName string) (*sqs.Client, bool) { | ||||||||||||||||||||
| entry, ok := sqsClientCache.getClient(pipelineName) | ||||||||||||||||||||
| if !ok { | ||||||||||||||||||||
| return nil, false | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return SQSClient, nil | ||||||||||||||||||||
| return entry.client, true | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const sqsLongPollSeconds = 20 | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -62,7 +62,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| var kubeClient klient.Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var sourceS3Client *s3.Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var destS3Client *s3.Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var sqsClient *sqs.Client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| feature.Setup( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -87,11 +87,18 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| err = awsclienthandler.NewDestinationS3ClientFromConfig(ctx, e2eAWS) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // create SQS client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, err = awsclienthandler.NewSQSClientFromConfig(ctx, &awsclienthandler.AWSConfig{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Region: "us-east-1", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AccessKeyID: "test", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SecretAccessKey: "test", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Endpoint: localstackURL, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, "e2e-test") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.Fatal(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
89
to
99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win Newly inserted code swallows the error from Line 89 assigns 🐛 Proposed fix err = awsclienthandler.NewDestinationS3ClientFromConfig(ctx, e2eAWS)
+ if err != nil {
+ t.Fatal(err)
+ }
+
// create SQS client
_, err = awsclienthandler.NewSQSClientFromConfig(ctx, &awsclienthandler.AWSConfig{
Region: "us-east-1",
AccessKeyID: "test",
SecretAccessKey: "test",
Endpoint: localstackURL,
}, "e2e-test")
if err != nil {
t.Fatal(err)
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sqsClient, err = awsclienthandler.NewSQSClientFromConfig(ctx, e2eAWS) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.Fatal(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -131,6 +138,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // create SQS queue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sqsClient, ok := awsclienthandler.GetSQSClient("e2e-test") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !ok { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.Fatal("SQS client not found for e2e-test") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _, err = sqsClient.CreateQueue(ctx, &sqs.CreateQueueInput{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| QueueName: aws.String(unstructuredQueueName), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using only
req.Nameas the cache key can lead to cache collisions if multipleSourceCrawlerresources with the same name exist in different namespaces. Since this operator can watch multiple namespaces, we should use the namespaced name (e.g.,req.NamespacedName.String()) as the cache key to prevent cross-namespace client leakage.