[TT-11739] Clean up rate limiting area, decouple GlobalConfig in APISpec

[titpetric]

User description

Clean up refactor of rate limits area:

• use rate limiter config object (apispec.GlobalConfig dropped)
• moved rate limiter key computation to happen before rate limiter logic, deduplicate
• minimized passed arguments

Fixing bugs:

• fixes new rate limiter not proceeding to quota to maintain existing behaviour
• key computation on new rate limiter now consistent with others (rate- prefix)

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

---

PR Type

Bug fix, Enhancement

---

Description

• Decoupled GlobalConfig from various rate limiting and quota management functions across multiple files to enhance modularity.
• Centralized rate limiter key prefix usage and standardized key generation logic.
• Removed redundant parameters and streamlined logic in session management and rate limiting functions.
• Added new utility functions to manage rate limiter configurations and key generation more effectively.

---

Changes walkthrough 📝

Relevant files

Enhancement

mw_api_rate_limit.go Simplify API Rate Limiting Logic and Decouple Global Config gateway/mw_api_rate_limit.go Removed reference to GlobalConfig from rate limiting logic. Simplified function arguments by removing unnecessary parameters. +0/-1      mw_organisation_activity.go Refactor Organisation Activity Rate Limiting                          gateway/mw_organisation_activity.go Removed GlobalConfig references in rate limiting checks. Streamlined function parameters to enhance clarity and maintainability. +0/-2      mw_rate_limiting.go Refactor Rate Limiting and Quota Checks                                    gateway/mw_rate_limiting.go Decoupled GlobalConfig from rate limiting and quota checks. Consolidated rate limiting key computation logic. +0/-2      session_manager.go Centralize Rate Limiter Key Prefix and Optimize Session Limiter gateway/session_manager.go Centralized rate limiter key prefix to rate.LimiterKeyPrefix. Optimized session limiter functions by removing redundant parameters and simplifying logic. +42/-74  limiter.go Enhance Rate Limiter Configuration and Key Generation        internal/rate/limiter.go Added LimiterKey function to standardize rate limiter key generation. Enhanced modularity by separating limiter configuration logic. +16/-0    rate.go Update Constants and Error Handling for Rate Limiters        internal/rate/rate.go Introduced LimiterKeyPrefix for consistent key naming. Updated constants and error handling for rate limiters. +10/-3

---

💡 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-05-16 14:17:40.992509241 +0000
+++ current.txt	2024-05-16 14:17:36.968433493 +0000
@@ -5815,7 +5815,7 @@
 	// EnableFixedWindow enables fixed window rate limiting.
 	EnableFixedWindowRateLimiter bool `json:"enable_fixed_window_rate_limiter"`
 
-	// Redis based rate limiter with fixed window. Provides 100% rate limiting accuracy, but require two additional Redis roundtrip for each request.
+	// Redis based rate limiter with sliding log. Provides 100% rate limiting accuracy, but require two additional Redis roundtrip for each request.
 	EnableRedisRollingLimiter bool `json:"enable_redis_rolling_limiter"`
 
 	// To enable, set to `true`. The sentinel-based rate limiter delivers a smoother performance curve as rate-limit calculations happen off-thread, but a stricter time-out based cool-down for clients. For example, when a throttling action is triggered, they are required to cool-down for the period of the rate limit.
@@ -7090,8 +7090,11 @@
 const (
 	// QuotaKeyPrefix serves as a standard prefix for generating quota keys.
 	QuotaKeyPrefix = "quota-"
-	// RateLimitKeyPrefix serves as a standard prefix for generating rate limit keys.
-	RateLimitKeyPrefix          = "rate-limit-"
+
+	// RateLimitKeyPrefix serves as a standard prefix for generating rate limiter keys.
+	RateLimitKeyPrefix = rate.LimiterKeyPrefix
+
+	// SentinelRateLimitKeyPostfix is appended to the rate limiting key to combine into a sentinel key.
 	SentinelRateLimitKeyPostfix = ".BLOCKED"
 )
 const (
@@ -9690,7 +9693,7 @@
 
 func (l *SessionLimiter) Context() context.Context
 
-func (l *SessionLimiter) ForwardMessage(r *http.Request, currentSession *user.SessionState, rateLimitKey string, quotaKey string, store storage.Handler, enableRL, enableQ bool, globalConf *config.Config, api *APISpec, dryRun bool) sessionFailReason
+func (l *SessionLimiter) ForwardMessage(r *http.Request, currentSession *user.SessionState, rateLimitKey string, quotaKey string, store storage.Handler, enableRL, enableQ bool, api *APISpec, dryRun bool) sessionFailReason
     ForwardMessage will enforce rate limiting, returning a non-zero
     sessionFailReason if session limits have been exceeded. Key values to manage
     rate are Rate and Per, e.g. Rate of 10 messages Per 10 seconds

[github-actions]

PR Description updated to latest commit (c130a1b)

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across several files with modifications in logic and configuration handling. The changes impact core functionality such as rate limiting and session management, requiring a thorough understanding of the existing system and the changes to ensure they integrate smoothly without introducing new issues.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The removal of GlobalConfig references and its replacement with Spec or local configurations might lead to unexpected behavior if the new configurations are not set up correctly or if they do not fully replicate the functionality of the GlobalConfig.
	Performance Concern: The changes in how rate limiting keys are generated and used could impact performance, especially if the new methods introduce more complexity or inefficiencies in key computation.
🔒 Security concerns	No

Code feedback:

---

relevant file	gateway/mw_api_rate_limit.go
suggestion	Consider handling potential nil pointer dereference when accessing k.Spec after removing GlobalConfig. Ensure that k.Spec is always non-nil before accessing its properties. [important]
relevant line	k.Spec,

---

relevant file	gateway/session_manager.go
suggestion	Refactor the doRollingWindowWrite function to remove the unused apiLimit parameter if it is no longer needed after the refactor. This will clean up the function signature and improve code clarity. [medium]
relevant line	func (l *SessionLimiter) doRollingWindowWrite(rateLimiterKey, rateLimiterSentinelKey string,

---

relevant file	gateway/session_manager.go
suggestion	Verify the removal of logging for key and rateLimiterKey does not reduce the ability to debug issues. If these logs are crucial, consider adding them back or ensuring adequate logging at a different part of the process. [medium]
relevant line	- log.Debug("[RATELIMIT] Inbound raw key is: ", key)

---

relevant file	gateway/session_manager.go
suggestion	Ensure that the new configuration flags EnableSentinelRateLimiter and DRLEnableSentinelRateLimiter are properly migrated and tested, as their usage replaces the old GlobalConfig based flags. This is crucial to maintain existing functionality. [important]
relevant line	if l.config.EnableSentinelRateLimiter || l.config.DRLEnableSentinelRateLimiter {

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestions
Maintainability	Improve encapsulation by using a method to access the Spec object. Replace the direct use of k.Spec with a method or property that ensures encapsulation and potentially handles more complex logic or default values. This change would improve maintainability and encapsulation in your codebase. gateway/mw_api_rate_limit.go [80] -k.Spec +k.GetSpec() // Assuming GetSpec() is a method that returns the Spec object
Error handling	Add error handling for the store operation. Consider checking the return value of k.Spec.OrgSessionManager.Store() for errors. This ensures that any issues with the store operation are handled gracefully. gateway/mw_organisation_activity.go [113] -k.Spec.OrgSessionManager.Store() +store, err := k.Spec.OrgSessionManager.Store() +if err != nil { + // handle error +}
Best practice	Define sentinel rate limit key postfix as a constant. Instead of using a hardcoded string for the sentinel rate limit key postfix, consider defining it as a constant to avoid magic strings and improve code readability. gateway/session_manager.go [39] -SentinelRateLimitKeyPostfix = ".BLOCKED" +const SentinelRateLimitKeyPostfix = ".BLOCKED"
Enhancement	Improve logging by using structured logging. Use structured logging instead of basic logging to provide more context in your log messages, making them more useful for debugging and monitoring. gateway/session_manager.go [231] -log.Debug("[RATELIMIT] Inbound raw key is: ", rateLimiterKey) +log.Debug("Inbound raw key", zap.String("rateLimiterKey", rateLimiterKey))
Bug	Add nil check for apiLimit to prevent potential crashes. Refactor the limitDRL function to handle the case where apiLimit is nil to prevent potential nil pointer dereferences. gateway/session_manager.go [166] +if apiLimit == nil { + return false +} currRate := apiLimit.Rate

[github-actions]

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

[github-actions]

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

[sonarcloud]

Please retry analysis of this Pull-Request directly on SonarCloud

[github-actions]

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

[sonarcloud]

Quality Gate failed

Failed conditions
48.2% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint