Skip to content
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

[TT-12566/TT-12851] Add client endpoint rate limiter #6462

Merged
merged 32 commits into from
Aug 21, 2024

Conversation

jeffy-mathew
Copy link
Contributor

@jeffy-mathew jeffy-mathew commented Aug 14, 2024

User description

Description

This PR adds endpoint level rate limiting to keys.
Merging this in policies would be added in a follow up PR.
Counter usage and increments would be as follows.

  • Endpoint rate limits for /API1/Endpoint1, /API1/Endpoint2 and /API2/Endpoint1 (using API1Endpoint1_Counter, API1Endpoint2_Counter, API2Endpoint1_Counter),
  • API rate limits for /API1 and /API3 (using API1_Counter, API3_Counter), no per API limits specified for API2
  • Global rate limit for all other requests (using Global_Counter)
Client request Global_Counter API1_Counter API3_Counter API1Endpoint1_Counter API1Endpoint2_Counter API2Endpoint1_Counter
/API1/Endpoint1 ++
/API1/Endpoint2 ++
/API1/Endpoint3 ++
/API2/Endpoint1 ++
/API2/Endpoint2 ++
/API2/Endpoint3 ++
/API3/Endpoint1 ++
/API3/Endpoint2 ++
/API3/Endpoint3 ++

Related Issue

parent: https://tyktech.atlassian.net/browse/TT-12566
subtask: https://tyktech.atlassian.net/browse/TT-12851

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement, Tests


Description

  • Implemented endpoint-specific rate limiting logic in session_manager.go.
  • Added new test cases and refactored existing ones to support endpoint rate limiting.
  • Introduced new structures in session.go to handle endpoint-specific rate limits.
  • Enhanced test framework to support setup functions in test cases.

Changes walkthrough 📝

Relevant files
Tests
mw_rate_limiting_test.go
Add and refactor tests for endpoint rate limiting               

gateway/mw_rate_limiting_test.go

  • Added new test cases for endpoint rate limiting.
  • Refactored existing rate limit test cases into reusable structures.
  • Introduced endpointRateLimitTestHelper function for testing endpoint
    rate limits.
  • +196/-51
    session_manager_test.go
    Add tests for endpoint rate limit info retrieval                 

    gateway/session_manager_test.go

  • Added tests for getEndpointRateLimitInfo function.
  • Verified correct rate limit info retrieval based on endpoint and
    method.
  • +105/-0 
    Enhancement
    session_manager.go
    Implement endpoint-specific rate limiting logic                   

    gateway/session_manager.go

  • Implemented endpoint-specific rate limiting logic.
  • Added support for endpoint rate limit key suffixes.
  • Integrated endpoint rate limit checks into existing rate limiting
    flow.
  • +62/-9   
    http.go
    Add support for setup functions in test cases                       

    test/http.go

  • Added support for BeforeFn in test cases to execute setup functions
    before tests.
  • +4/-0     
    session.go
    Add endpoint-specific rate limit structures and clone method

    user/session.go

  • Added Clone method to APILimit for deep copying.
  • Introduced Endpoint, EndpointMethod, and EndpointMethodRateLimit
    structs for endpoint-specific rate limits.
  • Updated AccessDefinition to include Endpoints.
  • +50/-5   

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

    Copy link
    Contributor

    github-actions bot commented Aug 14, 2024

    API Changes

    --- prev.txt	2024-08-20 11:12:09.682883050 +0000
    +++ current.txt	2024-08-20 11:12:06.410806673 +0000
    @@ -8193,6 +8193,9 @@
     	DisableIntrospection bool                         `json:"disable_introspection"`
     	FieldAccessRights    []user.FieldAccessDefinition `json:"field_access_rights"`
     	Limit                *user.APILimit               `json:"limit"`
    +
    +	// Endpoints contains endpoint rate limit settings.
    +	Endpoints user.Endpoints `json:"endpoints,omitempty"`
     }
     
     func (d *DBAccessDefinition) ToRegularAD() user.AccessDefinition
    @@ -12345,8 +12348,7 @@
     TYPES
     
     type APILimit struct {
    -	Rate               float64 `json:"rate" msg:"rate"`
    -	Per                float64 `json:"per" msg:"per"`
    +	RateLimit
     	ThrottleInterval   float64 `json:"throttle_interval" msg:"throttle_interval"`
     	ThrottleRetryLimit int     `json:"throttle_retry_limit" msg:"throttle_retry_limit"`
     	MaxQueryDepth      int     `json:"max_query_depth" msg:"max_query_depth"`
    @@ -12355,17 +12357,14 @@
     	QuotaRemaining     int64   `json:"quota_remaining" msg:"quota_remaining"`
     	QuotaRenewalRate   int64   `json:"quota_renewal_rate" msg:"quota_renewal_rate"`
     	SetBy              string  `json:"-" msg:"-"`
    -
    -	// Smoothing contains rate limit smoothing settings.
    -	Smoothing *apidef.RateLimitSmoothing `json:"smoothing" bson:"smoothing"`
     }
         APILimit stores quota and rate limit on ACL level (per API)
     
    -func (g *APILimit) Duration() time.Duration
    -    Duration returns the time between two allowed requests at the defined rate.
    -    It's used to decide which rate limit has a bigger allowance.
    +func (a APILimit) Clone() *APILimit
    +    Clone does a deepcopy of APILimit.
     
    -func (limit APILimit) IsEmpty() bool
    +func (a APILimit) IsEmpty() bool
    +    IsEmpty checks if APILimit is empty.
     
     type AccessDefinition struct {
     	APIName              string                  `json:"api_name" msg:"api_name"`
    @@ -12379,6 +12378,8 @@
     	DisableIntrospection bool                    `json:"disable_introspection" msg:"disable_introspection"`
     
     	AllowanceScope string `json:"allowance_scope" msg:"allowance_scope"`
    +
    +	Endpoints Endpoints `json:"endpoints,omitempty" msg:"endpoints,omitempty"`
     }
         AccessDefinition defines which versions of an API a key has access to NOTE:
         when adding new fields it is required to map them from DBAccessDefinition in
    @@ -12396,6 +12397,37 @@
     	Hash     HashType `json:"hash_type" msg:"hash_type"`
     }
     
    +type Endpoint struct {
    +	Path    string          `json:"path,omitempty" msg:"path"`
    +	Methods EndpointMethods `json:"methods,omitempty" msg:"methods"`
    +}
    +    Endpoint holds the configuration for endpoint rate limiting.
    +
    +type EndpointMethod struct {
    +	Name  string    `json:"name,omitempty" msg:"name,omitempty"`
    +	Limit RateLimit `json:"limit,omitempty" msg:"limit,omitempty"`
    +}
    +    EndpointMethod holds the configuration on endpoint method level.
    +
    +type EndpointMethods []EndpointMethod
    +    EndpointMethods is a collection of EndpointMethod.
    +
    +type EndpointRateLimitInfo struct {
    +	// KeySuffix is the suffix to use for the storage key.
    +	KeySuffix string
    +	// Rate is the allowance.
    +	Rate float64
    +	// Per is the rate limiting interval.
    +	Per float64
    +}
    +    EndpointRateLimitInfo holds the information to process endpoint rate limits.
    +
    +type Endpoints []Endpoint
    +    Endpoints is a collection of Endpoint.
    +
    +func (es Endpoints) RateLimitInfo(method string, path string) (*EndpointRateLimitInfo, bool)
    +    RateLimitInfo returns EndpointRateLimitInfo for endpoint rate limiting.
    +
     type FieldAccessDefinition struct {
     	TypeName  string      `json:"type_name" msg:"type_name"`
     	FieldName string      `json:"field_name" msg:"field_name"`
    @@ -12458,6 +12490,21 @@
     	PerAPI     bool `bson:"per_api" json:"per_api"`
     }
     
    +type RateLimit struct {
    +	// Rate is the allowed number of requests per interval.
    +	Rate float64 `json:"rate" msg:"rate"`
    +	// Per is the interval at which rate limit is enforced.
    +	Per float64 `json:"per" msg:"per"`
    +
    +	// Smoothing contains rate limit smoothing settings.
    +	Smoothing *apidef.RateLimitSmoothing `json:"smoothing,omitempty" bson:"smoothing,omitempty"`
    +}
    +    RateLimit holds rate limit configuration.
    +
    +func (r RateLimit) Duration() time.Duration
    +    Duration returns the time between two allowed requests at the defined rate.
    +    It's used to decide which rate limit has a bigger allowance.
    +
     type SessionState struct {
     	LastCheck                     int64                       `json:"last_check" msg:"last_check"`
     	Allowance                     float64                     `json:"allowance" msg:"allowance"`

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The function getEndpointRateLimitInfo does not handle the error from regexp.Compile other than returning nil, false. Consider logging the error or handling it more explicitly to aid in debugging and to avoid silent failures in regex compilation.

    Performance Concern
    The regex compilation in getEndpointRateLimitInfo is done within a loop which can lead to performance issues if the number of endpoints is large and the function is called frequently. Consider compiling the regex patterns once and reusing them.

    Possible Bug
    In ForwardMessage, the rate limiting key suffix is applied without checking if the endpoint rate limit should actually override the API rate limit. This might lead to incorrect rate limiting if the endpoint does not define a specific rate but the path matches.

    Copy link
    Contributor

    github-actions bot commented Aug 14, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Improve error handling in regex compilation by logging the error

    Ensure that the getEndpointRateLimitInfo function handles the potential error from
    regex compilation by logging the error or returning it to the caller, instead of
    silently failing.

    gateway/session_manager.go [453-455]

     asRegex, err := regexp.Compile(endpoint.Path)
     if err != nil {
    +    log.Errorf("Failed to compile regex for endpoint path: %s, error: %v", endpoint.Path, err)
         return nil, false
     }
     
    Suggestion importance[1-10]: 8

    Why: Logging the error provides better visibility into issues during regex compilation, which is crucial for debugging and maintaining the code.

    8
    Maintainability
    Refactor repeated rate limiting logic into a separate function for clarity and maintainability

    Refactor the rate limiting logic to avoid duplication and enhance readability by
    extracting the repeated logic into a separate function.

    gateway/session_manager.go [280-287]

    -if l.config.EnableSentinelRateLimiter {
    -    if l.limitSentinel(r, session, limiterKey, &apiLimit, dryRun) {
    -        return sessionFailRateLimit
    -    }
    -}
    -if l.config.EnableRedisRollingLimiter {
    -    if l.limitRedis(r, session, limiterKey, &apiLimit, dryRun) {
    -        return sessionFailRateLimit
    -    }
    +if l.applyRateLimit(r, session, limiterKey, &apiLimit, dryRun) {
    +    return sessionFailRateLimit
     }
     
    +// New function
    +func (l *SessionLimiter) applyRateLimit(r *http.Request, session *user.SessionState, limiterKey string, apiLimit *APILimit, dryRun bool) bool {
    +    if l.config.EnableSentinelRateLimiter {
    +        return l.limitSentinel(r, session, limiterKey, apiLimit, dryRun)
    +    }
    +    if l.config.EnableRedisRollingLimiter {
    +        return l.limitRedis(r, session, limiterKey, apiLimit, dryRun)
    +    }
    +    return false
    +}
    +
    Suggestion importance[1-10]: 7

    Why: This refactoring enhances code readability and maintainability by reducing duplication. It makes the code easier to understand and modify in the future.

    7
    Encapsulation
    Encapsulate the setting of rate limits into a method to enhance code maintainability

    Replace the direct manipulation of apiLimit.Rate and apiLimit.Per with a method that
    encapsulates this logic, enhancing encapsulation and allowing for easier adjustments
    and validations in the future.

    gateway/session_manager.go [247-248]

     if doEndpointRL {
    -    apiLimit.Rate = float64(endpointRLInfo.rate)
    -    apiLimit.Per = float64(endpointRLInfo.per)
    +    apiLimit.SetRateLimit(endpointRLInfo.rate, endpointRLInfo.per)
     }
     
    +// Method to be added in APILimit
    +func (a *APILimit) SetRateLimit(rate, per int64) {
    +    a.Rate = float64(rate)
    +    a.Per = float64(per)
    +}
    +
    Suggestion importance[1-10]: 6

    Why: Encapsulating the rate limit setting logic into a method improves code maintainability and readability. However, the improvement is relatively minor as it only affects a small part of the code.

    6
    Best practice
    Add a default case to handle unexpected limiter values in the switch statement

    Consider adding a default case in the switch statement inside the
    rlTestRunnerProvider function to handle unexpected limiter values gracefully. This
    will ensure that the function behaves predictably even with incorrect input.

    gateway/mw_rate_limiting_test.go [228-238]

     switch limiter {
         case "Redis":
             globalConf.RateLimit.EnableRedisRollingLimiter = true
         case "Sentinel":
             globalConf.RateLimit.EnableSentinelRateLimiter = true
         case "DRL":
             globalConf.RateLimit.DRLEnableSentinelRateLimiter = true
         case "NonTransactional":
             globalConf.RateLimit.EnableNonTransactionalRateLimiter = true
         default:
    -        t.Fatal("There is no such a rate limiter:", limiter)
    +        log.Errorf("Unsupported rate limiter: %s", limiter)
    +        return nil, func() {}
     }
     
    Suggestion importance[1-10]: 5

    Why: The suggestion improves the handling of unexpected limiter values by logging an error and returning a no-op function. However, the existing code already has a default case that calls t.Fatal, which stops the test execution, so the improvement is minor.

    5

    @jeffy-mathew jeffy-mathew changed the title add endpoint rate limiter [TT-12566] Add client endpoint rate limiter Aug 14, 2024
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from 60fc512 to 1333903 Compare August 14, 2024 16:48
    @jeffy-mathew jeffy-mathew changed the title [TT-12566] Add client endpoint rate limiter [TT-12566/TT-12851] Add client endpoint rate limiter Aug 16, 2024
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from e68f55d to d768ba4 Compare August 16, 2024 10:16
    @jeffy-mathew jeffy-mathew requested a review from a team as a code owner August 16, 2024 10:16
    user/session.go Outdated Show resolved Hide resolved
    user/session.go Outdated Show resolved Hide resolved
    user/session.go Outdated Show resolved Hide resolved
    user/session_test.go Outdated Show resolved Hide resolved
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from e1c722e to d94223d Compare August 19, 2024 08:39
    globalConf.RateLimit.EnableSentinelRateLimiter = true
    case "DRL":
    globalConf.RateLimit.DRLEnableSentinelRateLimiter = true
    case "NonTransactional":
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    this block is missing fixed window rl, should be able to use the internal/rate api to:

    1. get a symbol that has a list of all these limiters (there's an enum around already, needs a slice if there isn't one already)
    2. provide something like rate.Enable(*config.Config, RateLimiter/string) error

    try to be mindful what test code is useful, should some tests be shorter, should some logic be part of a particular internal package rather than a test helper

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    user/session_test.go Outdated Show resolved Hide resolved
    user/session_test.go Outdated Show resolved Hide resolved
    swagger.yml Outdated Show resolved Hide resolved
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch 2 times, most recently from 2541759 to 6fd3c38 Compare August 20, 2024 07:50
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from 20f049d to adcae95 Compare August 20, 2024 08:16
    gateway/policy.go Outdated Show resolved Hide resolved
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from 6db944d to 95866ca Compare August 20, 2024 09:39
    @jeffy-mathew jeffy-mathew force-pushed the feat/TT-12566/endpoint-clientside-ratelimits branch from 7c03309 to 7151951 Compare August 20, 2024 10:08
    Copy link

    @lghiur lghiur merged commit c639e35 into master Aug 21, 2024
    23 of 27 checks passed
    @lghiur lghiur deleted the feat/TT-12566/endpoint-clientside-ratelimits branch August 21, 2024 12:32
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants