[TT-12618] Add RateLimit per-path settings to apidefinition classic, add test case

titpetric commented 1 month ago

User description

This adds the per-path configuration for rate limits into apidef classic.

Task: https://tyktech.atlassian.net/browse/TT-12565 Subtask: https://tyktech.atlassian.net/browse/TT-12618

It creates a ./tests/rate location for tests, testing per-endpoint and global rate limits

[x] data model change
[x] new tests added
[x] implementation done

PR Type

Enhancement, Tests

Description

Added RateLimitMeta struct to support per-path rate limit settings in API definitions.
Implemented validation methods for RateLimitMeta to ensure proper configuration.
Integrated rate limit paths into the extended path specifications in the API definition loader.
Enhanced RateLimitForAPI middleware to handle per-endpoint rate limits, including new methods shouldEnable and getSession.
Added comprehensive tests for the new rate limit configurations and their integration into the API definitions.

Changes walkthrough 📝

Relevant files

Enhancement

api_definitions.go `Add RateLimitMeta struct and validation methods` apidef/api_definitions.go Added `RateLimitMeta` struct for per-path rate limit settings. Implemented validation methods for `RateLimitMeta`. Included `RateLimitMeta` in `ExtendedPathsSet`.	+34/-0
api_definition.go `Integrate rate limit paths into API definition` gateway/api_definition.go Added `RateLimit` to constants and URLSpec. Implemented `compileRateLimitPathsSpec` function. Integrated rate limit paths into extended path specs.	+41/-12
mw_api_rate_limit.go `Enhance RateLimitForAPI middleware for per-endpoint limits` gateway/mw_api_rate_limit.go Added `shouldEnable` method to check rate limit activation. Implemented `getSession` method to handle per-endpoint rate limits. Modified `EnabledForSpec` and `ProcessRequest` to support per-endpoint rate limits.	+54/-2

Tests

api_definitions_test.go `Add tests for API definitions and rate limits` apidef/api_definitions_test.go Added tests for schema validation, string regex map, routing trigger options, and encoding/decoding. Included tests for rate limit configurations.	+327/-1
per_api_limit_test.go `Add tests for per-endpoint rate limits` tests/rate/per_api_limit_test.go Added test cases for per-endpoint rate limits. Implemented helper function to build API with rate limits.	+121/-0

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

github-actions[bot] commented 1 month ago

API Changes

--- prev.txt    2024-07-15 12:22:53.420437846 +0000
+++ current.txt 2024-07-15 12:22:50.392452152 +0000
@@ -1210,6 +1210,7 @@
    Internal                []InternalMeta        `bson:"internal" json:"internal,omitempty"`
    GoPlugin                []GoPluginMeta        `bson:"go_plugin" json:"go_plugin,omitempty"`
    PersistGraphQL          []PersistGraphQLMeta  `bson:"persist_graphql" json:"persist_graphql"`
+   RateLimit               []RateLimitMeta       `bson:"rate_limit" json:"rate_limit"`
 }

 func (e *ExtendedPathsSet) Clear()
@@ -1666,6 +1667,24 @@
    Value string `bson:"value" json:"value"`
 }

+type RateLimitMeta struct {
+   Disabled bool   `bson:"disabled" json:"disabled"`
+   Path     string `bson:"path" json:"path"`
+   Method   string `bson:"method" json:"method"`
+
+   Rate float64 `bson:"rate" json:"rate"`
+   Per  float64 `bson:"per" json:"per"`
+}
+    RateLimitMeta configures rate limits per API path.
+
+func (r *RateLimitMeta) Err() error
+    Err checks the rate limit configuration for validity and returns an error
+    if it is not valid. It checks for a nil value, the enabled flag and valid
+    values for each setting.
+
+func (r *RateLimitMeta) Valid() bool
+    Valid will return true if the rate limit should be applied.
+
 type RateLimitSmoothing struct {
    // Enabled indicates if rate limit smoothing is active.
    Enabled bool `json:"enabled" bson:"enabled"`
@@ -9887,6 +9906,7 @@
    StatusInternal                 RequestStatus = "Internal path"
    StatusGoPlugin                 RequestStatus = "Go plugin"
    StatusPersistGraphQL           RequestStatus = "Persist GraphQL"
+   StatusRateLimit                RequestStatus = "Rate Limited"
 )
     Statuses of the request, all are false-y except StatusOk and
     StatusOkAndIgnore
@@ -10474,6 +10494,7 @@
    Internal                  apidef.InternalMeta
    GoPluginMeta              GoPluginMiddleware
    PersistGraphQL            apidef.PersistGraphQLMeta
+   RateLimit                 apidef.RateLimitMeta

    IgnoreCase bool
 }
@@ -10508,6 +10529,7 @@
    Internal
    GoPlugin
    PersistGraphQL
+   RateLimit
 )
     Enums representing the various statuses for a VersionInfo Path match during
     a proxy request
@@ -12057,6 +12079,8 @@
     manipulate

 func MyPluginReturningError(rw http.ResponseWriter, r *http.Request)
+# Package: ./tests/rate
+
 # Package: ./tests/regression

 package regression // import "github.com/TykTechnologies/tyk/tests/regression"

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

**Possible Bug:** The error message "rate limit disabled: per invalid" might be misleading as it suggests the rate limit is disabled when actually the 'per' value is invalid. Consider rephrasing to "invalid 'per' value for rate limit". **Error Handling:** The method `getSession` in `gateway/mw_api_rate_limit.go` uses `log.Fatalf` which will cause the program to exit if the type assertion fails. This could be handled more gracefully to avoid crashing the service.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Remove nil check on struct method to prevent runtime panic ___ The `Err` method in `RateLimitMeta` struct should not check for `r == nil` as it is a method on the struct itself and will cause a panic if called on a nil pointer. Instead, ensure that `r` is not nil before calling this method. [apidef/api_definitions.go [237-238]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67R237-R238) ```diff -if r == nil \|\| r.Disabled { +if r.Disabled { return errors.New("rate limit disabled") } ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential runtime panic by removing an invalid nil check on a struct method, which is a critical bug fix.	10
Possible bug	Handle errors when retrieving version information to ensure stability ___ Consider handling the error returned by `k.Spec.Version(r)` in `getSession` method to prevent potential runtime panics or unexpected behaviors if the version cannot be determined. [gateway/mw_api_rate_limit.go [51]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-46326b04f936c839922e970db5c2924156cc797070948f3dc9c589d04661d6d2R51-R51) ```diff -versionInfo, _ := k.Spec.Version(r) +versionInfo, err := k.Spec.Version(r) +if err != nil { + log.Fatalf("Error retrieving version info: %v", err) +} ``` Suggestion importance[1-10]: 9 Why: Handling the error returned by `k.Spec.Version(r)` is crucial for preventing potential runtime panics or unexpected behaviors, thus improving the stability of the application.	9
Enhancement	Add validation for negative values in 'rate' configuration ___ Consider adding a check for `r.Rate <= 0` instead of `r.Rate == 0` to handle negative values, which are likely invalid for rate limits. [apidef/api_definitions.go [243-244]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67R243-R244) ```diff -if r.Rate == 0 { - return fmt.Errorf("rate limit disabled: rate zero") +if r.Rate <= 0 { + return fmt.Errorf("invalid configuration: 'rate' must be positive") } ``` Suggestion importance[1-10]: 9 Why: This suggestion enhances the validation logic by preventing negative values for rate limits, which is a significant improvement for ensuring correct configuration.	9
Enhancement	Improve the error message for invalid 'per' configuration ___ The error message for `r.Per <= 0` should be more descriptive about the nature of the error, specifying that the 'per' value must be greater than zero. [apidef/api_definitions.go [240-241]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67R240-R241) ```diff if r.Per <= 0 { - return fmt.Errorf("rate limit disabled: per invalid") + return fmt.Errorf("invalid configuration: 'per' must be greater than 0") } ``` Suggestion importance[1-10]: 7 Why: The suggestion improves the clarity of the error message, enhancing code readability and debugging, though it is a minor enhancement.	7
Maintainability	Simplify the return statement in the `Valid` method ___ The `Valid` method should directly return the negation of the error check for simplicity and to avoid unnecessary branching. [apidef/api_definitions.go [228-231]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67R228-R231) ```diff -if err := r.Err(); err != nil { - return false -} -return true +return r.Err() == nil ``` Suggestion importance[1-10]: 8 Why: Simplifying the return statement improves code readability and maintainability without altering functionality.	8
	Use less severe error handling to avoid unnecessary termination of the program ___ Replace the use of `log.Fatalf` in `getSession` method with a less severe error handling mechanism that does not terminate the program, such as logging the error and returning a default session or nil. [gateway/mw_api_rate_limit.go [58]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-46326b04f936c839922e970db5c2924156cc797070948f3dc9c589d04661d6d2R58-R58) ```diff -log.Fatalf("Expected RateLimitMeta, got %T", meta) +log.Printf("Expected RateRateLimitMeta, got %T", meta) +return nil ``` Suggestion importance[1-10]: 8 Why: Using a less severe error handling mechanism avoids unnecessary termination of the program, which enhances maintainability and user experience.	8
	Refactor nested loops into a separate method to improve code readability ___ In the `shouldEnable` method, refactor the nested loops checking for rate limits into a separate method to improve readability and maintainability. [gateway/mw_api_rate_limit.go [34-40]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-46326b04f936c839922e970db5c2924156cc797070948f3dc9c589d04661d6d2R34-R40) ```diff -for _, version := range k.Spec.VersionData.Versions { - for _, v := range version.ExtendedPaths.RateLimit { - if !v.Disabled { - return true +if hasActiveRateLimits(k.Spec.VersionData.Versions) { + return true +} + +func hasActiveRateLimits(versions map[string]VersionInfo) bool { + for _, version := range versions { + for _, v := range version.ExtendedPaths.RateLimit { + if !v.Disabled { + return true + } } } + return false } ``` Suggestion importance[1-10]: 7 Why: Refactoring the nested loops into a separate method improves code readability and maintainability, making it easier for future developers to understand and modify the code.	7
Performance	Optimize hashing mechanism for potential performance gains ___ Use a more efficient hashing mechanism in `getSession` method to potentially improve performance, especially under high load. [gateway/mw_api_rate_limit.go [63]](https://github.com/TykTechnologies/tyk/pull/6413/files#diff-46326b04f936c839922e970db5c2924156cc797070948f3dc9c589d04661d6d2R63-R63) ```diff -keyname := k.keyName + "-" + storage.HashStr(limits.Path) +keyname := k.keyName + "-" + storage.FastHash(limits.Path) ``` Suggestion importance[1-10]: 6 Why: While optimizing the hashing mechanism can lead to performance gains, the actual impact may vary depending on the context and load, making this a minor improvement.	6

sonarcloud[bot] commented 1 month ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
89.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

TykTechnologies / tyk