[TT-13011] implement combining endpoint rate limits from non partitioned policies.

jeffy-mathew commented 2 months ago

Description

This PR would allow combining endpoint rate limits by combining multiple non partitioned policies.

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

Tests, Enhancement

Description

Enhanced the policy application logic to support combining endpoint rate limits from non-partitioned policies.
Added comprehensive tests to verify the correct application of endpoint rate limits when combining multiple policies.
Introduced new test data and updated existing test policies to cover various scenarios of endpoint rate limits.
Modified the applyAPILevelLimits function to ensure correct updating of SetBy and AllowanceScope attributes.

Changes walkthrough 📝

Relevant files

Tests

policy_test.go `Enhance policy tests to include endpoint rate limits` gateway/policy_test.go Added `reverseOrder` field to `testApplyPoliciesData` struct. Implemented tests for combining non-partitioned policies with endpoint rate limits. Modified test logic to handle reversed policy order.	+235/-54
apply_test.go `Add tests for endpoint level limits application` internal/policy/apply_test.go Added new test `TestApplyEndpointLevelLimits`. Embedded test data for endpoint rate limits.	+28/-0
policies.json `Update test policies with endpoint rate limits` gateway/testdata/policies.json Added new policies for testing endpoint rate limits. Updated existing policy identifiers.	+146/-2
apply_endpoint_rl.json `Add test data for endpoint rate limits application` internal/policy/testdata/apply_endpoint_rl.json - Added test cases for applying endpoint rate limits.	+264/-0

Enhancement

apply.go

Implement endpoint level limits application in policies

internal/policy/apply.go

Added logic to apply endpoint level limits when combining policies.

Modified applyAPILevelLimits to update SetBy and AllowanceScope.

Renamed applyEndpointLevelLimits to ApplyEndpointLevelLimits.

+40/-10

💡 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 2 months ago

API Changes

no api changes detected

github-actions[bot] commented 2 months ago

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Endpoint Limits Logic
The logic for applying endpoint level limits (`ApplyEndpointLevelLimits`) needs careful review to ensure that it correctly handles overlapping endpoints and respects the most restrictive limits. The merging strategy should be verified against expected behaviors, especially in complex scenarios with multiple overlapping policies. Test Coverage
Ensure that the new test cases added for handling combined endpoint rate limits (`combinedEndpointRLTCs`) are comprehensive and cover all edge cases, particularly for scenarios where policies might conflict or require complex merging logic.

github-actions[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Ensure updates are tracked by setting the `updated` flag ___ Ensure that the `updated` flag is correctly set to `true` when any changes are made to the `policyAD.Limit` fields to avoid potential bugs where updates are not tracked. [internal/policy/apply.go [542-545]](https://github.com/TykTechnologies/tyk/pull/6494/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R542-R545) ```diff if policyAD.Limit.Duration() > currAD.Limit.Duration() { policyAD.Limit.Per = currAD.Limit.Per policyAD.Limit.Rate = currAD.Limit.Rate policyAD.Limit.Smoothing = currAD.Limit.Smoothing + updated = true } ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by ensuring that changes to `policyAD.Limit` are tracked, which is crucial for maintaining correct state and avoiding subtle bugs.	9
Enhancement	Simplify slice cloning and reversing using `slices.Clone` and `slices.Reverse` ___ Replace the manual copying and reversing of slices with the `slices.Clone` and `slices.Reverse` functions for clarity and efficiency. [internal/policy/apply.go [1090-1092]](https://github.com/TykTechnologies/tyk/pull/6494/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R1090-R1092) ```diff -var copyPols = make([]string, len(tc.policies)) -copy(copyPols, tc.policies) +copyPols := slices.Clone(tc.policies) slices.Reverse(copyPols) ``` Suggestion importance[1-10]: 8 Why: The suggestion improves code clarity and efficiency by using built-in functions for cloning and reversing slices, which is a cleaner and more idiomatic approach in Go.	8
Maintainability	Refactor to handle empty current endpoints at the start of the function ___ Refactor the `ApplyEndpointLevelLimits` function to avoid redundant checks and improve readability by handling the case where `currEPMap` is empty at the start of the function. [internal/policy/apply.go [573-576]](https://github.com/TykTechnologies/tyk/pull/6494/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R573-R576) ```diff -currEPMap := currEndpoints.Map() -if len(currEPMap) == 0 { +if len(currEndpoints) == 0 { return policyEndpoints } +currEPMap := currEndpoints.Map() ``` Suggestion importance[1-10]: 7 Why: The refactor improves code readability and efficiency by handling the empty case early, reducing unnecessary operations and making the function logic clearer.	7
Best practice	Improve variable naming for clarity ___ Use a more descriptive variable name than `ok` in the `ApplyEndpointLevelLimits` function to enhance code readability and maintainability. [internal/policy/apply.go [584-585]](https://github.com/TykTechnologies/tyk/pull/6494/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R584-R585) ```diff -policyRL, ok := result[currEP] -if !ok { +policyRL, exists := result[currEP] +if !exists { ``` Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name enhances code readability and maintainability, which is a good practice, although it is a minor improvement.	6