[TT-11739] Clean up rate limiting area, decouple GlobalConfig in APISpec by titpetric · Pull Request #6262 · TykTechnologies/tyk

titpetric · 2024-05-08T14:34:36Z

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 · 2024-05-08T14:35:22Z

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 · 2024-05-08T14:36:34Z

PR Description updated to latest commit (c130a1b)

github-actions · 2024-05-08T14:37:19Z

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`.
⚡ Possible issues	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 · 2024-05-08T14:38:23Z

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 · 2024-05-08T14:43:57Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

github-actions · 2024-05-13T10:12:49Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

sonarqubecloud · 2024-05-13T10:13:55Z

Please retry analysis of this Pull-Request directly on SonarCloud

github-actions · 2024-05-13T11:03:35Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

sonarqubecloud · 2024-05-16T14:30:27Z

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

…pec (#6262) ### **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** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement </strong></td><td><table> <tr> <td> <details> <summary><strong>mw_api_rate_limit.go</strong><dd><code>Simplify API Rate Limiting Logic and Decouple Global Config</code></dd></summary> <hr> gateway/mw_api_rate_limit.go <li>Removed reference to <code>GlobalConfig</code> from rate limiting logic.<br> <li> Simplified function arguments by removing unnecessary parameters.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-46326b04f936c839922e970db5c2924156cc797070948f3dc9c589d04661d6d2">+0/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_organisation_activity.go</strong><dd><code>Refactor Organisation Activity Rate Limiting</code>                          </dd></summary> <hr> gateway/mw_organisation_activity.go <li>Removed <code>GlobalConfig</code> references in rate limiting checks.<br> <li> Streamlined function parameters to enhance clarity and <br>maintainability.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-26dd955903317b085be06642ae3e76fe41c8c53844d8758a1a1c8bd05b0110a2">+0/-2</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_rate_limiting.go</strong><dd><code>Refactor Rate Limiting and Quota Checks</code>                                    </dd></summary> <hr> gateway/mw_rate_limiting.go <li>Decoupled <code>GlobalConfig</code> from rate limiting and quota checks.<br> <li> Consolidated rate limiting key computation logic.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-4bf8ae01ccab67bb786468f793f6bb4324c8f6b950b0e98e203effebe763a630">+0/-2</a>      </td> </tr> <tr> <td> <details> <summary><strong>session_manager.go</strong><dd><code>Centralize Rate Limiter Key Prefix and Optimize Session Limiter</code></dd></summary> <hr> gateway/session_manager.go <li>Centralized rate limiter key prefix to <code>rate.LimiterKeyPrefix</code>.<br> <li> Optimized session limiter functions by removing redundant parameters <br>and simplifying logic.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-e6b40a285464cd86736e970c4c0b320b44c75b18b363d38c200e9a9d36cdabb6">+42/-74</a>  </td> </tr> <tr> <td> <details> <summary><strong>limiter.go</strong><dd><code>Enhance Rate Limiter Configuration and Key Generation</code>        </dd></summary> <hr> internal/rate/limiter.go <li>Added <code>LimiterKey</code> function to standardize rate limiter key generation.<br> <li> Enhanced modularity by separating limiter configuration logic.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-84e3fda0965028c6e464ae0916aaef0af63d0367fde6f910b513a610f2a34ee5">+16/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>rate.go</strong><dd><code>Update Constants and Error Handling for Rate Limiters</code>        </dd></summary> <hr> internal/rate/rate.go <li>Introduced <code>LimiterKeyPrefix</code> for consistent key naming.<br> <li> Updated constants and error handling for rate limiters.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6262/files#diff-666e0b0c32c831ededfd6c780dbf916d10e4b7d9c8e02f157bf1dca1d58a360d">+10/-3</a>    </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Tit Petric <[email protected]>

github-actions Bot added enhancement Bug fix labels May 8, 2024

github-actions Bot added the Review effort [1-5]: 3 label May 8, 2024

titpetric force-pushed the fix/tt-11739/fix-limiter-keys-and-pass-to-quota branch from c130a1b to 3b368cb Compare May 8, 2024 14:37

titpetric requested review from a team as code owners May 8, 2024 14:37

titpetric force-pushed the fix/tt-11739/fix-limiter-keys-and-pass-to-quota branch from 3b368cb to ca5cb60 Compare May 8, 2024 14:40

titpetric changed the title ~~Clean up rate limiting area, decouple GlobalConfig in APISpec~~ [TT-11739] Clean up rate limiting area, decouple GlobalConfig in APISpec May 8, 2024

titpetric changed the base branch from master to test/patch-sentinel-key-2 May 8, 2024 14:46

titpetric requested a review from jeffy-mathew May 8, 2024 15:08

jeffy-mathew approved these changes May 8, 2024

View reviewed changes

TykTechnologies deleted a comment from github-actions Bot May 8, 2024

titpetric changed the base branch from test/patch-sentinel-key-2 to master May 8, 2024 17:17

titpetric force-pushed the fix/tt-11739/fix-limiter-keys-and-pass-to-quota branch from 4e5170c to 8817cb9 Compare May 13, 2024 10:03

Tit Petric added 5 commits May 16, 2024 16:16

Don't count blocked requests during sentinel key lifetime

f2d873e

Get rid of DRL/RRL switch and use the quota mechanism for rate limit

3eb5673

Re-implement fixed-window from existing RRL

3ccd1fc

Revert POC changes, enable fixed window rate limiter

4921a32

Clean up rate limiting area, decouple GlobalConfig in APISpec

b13bf03

Tit Petric added 8 commits May 16, 2024 16:16

Decrease switch nesting

804a5c8

Disable DRL if fixed window is enabled

f90ac2a

Update TestJSVM_Auth to set HashKeys, better handling if no hash

dfc9c22

Revert quotaKey param type change for RedisQuotaExceeded

600112d

Make sonarcloud pass

e6915d5

Trim dashes from rate limiter key params

44c9fb1

Fix rate limit key apis to not bleed into rate limiter

1ed7269

Godoc fix

a12a2a6

titpetric force-pushed the fix/tt-11739/fix-limiter-keys-and-pass-to-quota branch from 62bf375 to a12a2a6 Compare May 16, 2024 14:16

titpetric enabled auto-merge (squash) May 16, 2024 14:17

titpetric merged commit 93ec00c into master May 16, 2024

titpetric deleted the fix/tt-11739/fix-limiter-keys-and-pass-to-quota branch May 16, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

[TT-11739] Clean up rate limiting area, decouple GlobalConfig in APISpec#6262
titpetric merged 13 commits into
masterfrom
fix/tt-11739/fix-limiter-keys-and-pass-to-quota

titpetric commented May 8, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented May 8, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

github-actions Bot commented May 13, 2024

Uh oh!

sonarqubecloud Bot commented May 13, 2024

Uh oh!

github-actions Bot commented May 13, 2024

Uh oh!

sonarqubecloud Bot commented May 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

titpetric commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions Bot commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 8, 2024

Uh oh!

github-actions Bot commented May 8, 2024

PR Review 🔍

Uh oh!

github-actions Bot commented May 8, 2024

PR Code Suggestions ✨

Uh oh!

github-actions Bot commented May 8, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

github-actions Bot commented May 13, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

sonarqubecloud Bot commented May 13, 2024

Uh oh!

github-actions Bot commented May 13, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

sonarqubecloud Bot commented May 16, 2024

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

titpetric commented May 8, 2024 •

edited

Loading

github-actions Bot commented May 8, 2024 •

edited

Loading