[TT-13041] Adjust quota handling of missing or expired keys

[titpetric]

PR Type

Bug fix, Enhancement

https://tyktech.atlassian.net/browse/TT-13041

---

Description

• Refactored the RedisQuotaExceeded function to improve quota handling logic, including better management of key expiration and renewal.
• Enhanced logging with additional context to aid in debugging and monitoring of quota operations.
• Removed the distributed lock during quota increment and added it during quota reset to optimize performance.
• Updated test cases in middleware_test.go to use t.Run for better organization and added additional test cases for comprehensive coverage.

---

Changes walkthrough 📝

	Relevant files
Tests	middleware_test.go Refactor and extend test cases for session limiter              gateway/middleware_test.go Refactored test cases to use t.Run for better organization. Added additional test cases for API3 to check quota decrement. +18/-12
Enhancement	session_manager.go Refactor quota handling logic and improve logging                gateway/session_manager.go Improved logging for quota management with additional context. Refactored quota increment logic to handle key expiration and renewal. Removed distributed lock for quota increment and added it for quota reset. Ensured session quota is updated with new logic. +84/-51

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[github-actions]

API Changes

--- prev.txt	2024-09-10 20:24:17.276297472 +0000
+++ current.txt	2024-09-10 20:24:14.224295263 +0000
@@ -11926,6 +11926,9 @@
 func Cert(domain string) tls.Certificate
     Generate cert
 
+func Exclusive(t *testing.T)
+    Exclusive uses a lock to gate only a single test running.
+
 func Flaky(t *testing.T, fake ...func() (bool, func(...interface{})))
     Flaky skips a flaky test in a CI environment
 

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Complex Logic The refactoring in RedisQuotaExceeded introduces complex conditional logic and multiple nested operations which could be error-prone and hard to maintain. Consider simplifying or breaking down the function further. Error Handling The error handling strategy in the RedisQuotaExceeded function might lead to unexpected behavior or system state, especially around the locking and unlocking of quota keys. Ensure robust error handling and recovery paths.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Performance	Replace the background context with a timeout context to prevent potential blocking operations Instead of using context.Background() for Redis operations, consider using a timeout context to avoid potential blocking operations that could degrade performance. Here's how you can implement it: gateway/session_manager.go [346] -ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +defer cancel() Suggestion importance[1-10]: 9 Why: Using a timeout context instead of a background context is a significant improvement for preventing potential blocking operations, enhancing the performance and reliability of the Redis operations.	9
Maintainability	Refactor the increment function to separate concerns for better maintainability and testability The increment function currently handles multiple responsibilities, which can make the code harder to maintain and test. Consider refactoring this function to separate the concerns, such as moving the Redis pipeline operations into a separate function. gateway/session_manager.go [395-426] increment := func() bool { - var res *redis.IntCmd - conn.Pipelined(ctx, func(pipe redis.Pipeliner) error { - res = pipe.Incr(ctx, rawKey) - if quotaRenewalRate > 0 { - pipe.ExpireNX(ctx, rawKey, quotaRenewalRate) - } else { - pipe.ExpireNX(ctx, rawKey, 0) - } - return nil - }) - qInt, err := res.Result() + qInt, err := incrementQuotaKey(ctx, conn, rawKey, quotaRenewalRate) if err != nil { logger.WithError(err).Error("error incrementing quota key") return true } - blocked := qInt-1 >= limit.QuotaMax - remaining := limit.QuotaMax - qInt + return updateQuotaStatus(qInt, limit.QuotaMax) +} + +func incrementQuotaKey(ctx context.Context, conn storage.Handler, key string, renewalRate time.Duration) (int, error) { + var res *redis.IntCmd + err := conn.Pipelined(ctx, func(pipe redis.Pipeliner) error { + res = pipe.Incr(ctx, key) + if renewalRate > 0 { + pipe.ExpireNX(ctx, key, renewalRate) + } else { + pipe.ExpireNX(ctx, key, 0) + } + return nil + }) + if err != nil { + return 0, err + } + return res.Result() +} + +func updateQuotaStatus(currentQuota int, maxQuota int) bool { + blocked := currentQuota-1 >= maxQuota + remaining := maxQuota - currentQuota if blocked { remaining = 0 } - logger = logger.WithField("quota", qInt-1) - logger = logger.WithField("blocked", blocked) - logger = logger.WithField("remaining", remaining) + logger = logger.WithFields(logrus.Fields{ + "quota": currentQuota-1, + "blocked": blocked, + "remaining": remaining, + }) logger.Debug("[QUOTA] Update quota key") l.updateSessionQuota(session, scope, remaining, expiredAt.Unix()) return blocked } Suggestion importance[1-10]: 8 Why: Refactoring the increment function to separate concerns enhances code maintainability and testability, making it easier to manage and understand, which is beneficial for long-term code quality.	8
Possible issue	Handle lock acquisition failure explicitly to avoid unintended quota key increments The current implementation could lead to a potential issue where the quota key is incremented even if the lock acquisition fails. It's safer to handle the error explicitly and decide whether to proceed based on the specific requirements or error handling policies. gateway/session_manager.go [443-445] if err := locker.Lock(ctx); err != nil { - return increment() + logger.WithError(err).Error("error acquiring lock") + return false // or handle according to specific requirements } Suggestion importance[1-10]: 8 Why: Explicitly handling lock acquisition failures prevents unintended behavior, such as incrementing the quota key when the lock fails, which is crucial for ensuring correct application logic and error handling.	8
Best practice	Create a new logger instance within the function to avoid modifying the shared logger instance To avoid potential race conditions or unexpected behavior when multiple goroutines access the logger variable, consider creating a new logger instance within the increment function instead of reusing and modifying the outer logger. gateway/session_manager.go [419-422] -logger = logger.WithField("quota", qInt-1) -logger = logger.WithField("blocked", blocked) -logger = logger.WithField("remaining", remaining) -logger.Debug("[QUOTA] Update quota key") +funcLogger := logger.WithFields(logrus.Fields{ + "quota": qInt-1, + "blocked": blocked, + "remaining": remaining, +}) +funcLogger.Debug("[QUOTA] Update quota key") Suggestion importance[1-10]: 7 Why: This suggestion improves code safety by preventing potential race conditions when multiple goroutines access the shared logger instance, which is a good practice for concurrent programming.	7

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[titpetric]

/release to release-5.3

[tykbot]

Working on it! Note that it can take a few minutes.

[tykbot]

@titpetric Seems like there is conflict and it require manual merge.