[TT-12964] ignore endpoint rate limit configurations when rate limit partition is disabled

jeffy-mathew commented 1 month ago

User description

ignore endpoint rate limit configurations when rate limit partition is disabled

Description

This PR makes apply policies ignore endpoint rate limits when rate limit partition is disabled

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

Enhanced the Apply function to ignore endpoint rate limits when the rate limit partition is disabled, ensuring that endpoints are set to nil in such cases.
Added new test cases in policy_test.go to verify the behavior of endpoint rate limits when partitions are disabled or combined.
Updated policies.json with new policy configurations to support the new test cases, including specific endpoint paths and methods.

Changes walkthrough 📝

Relevant files

Tests

policy_test.go `Add test cases for endpoint rate limits with partitioning` gateway/policy_test.go Added new test cases for endpoint rate limits when rate limit partition is disabled. Ensured session state access rights are correctly validated in tests.	+21/-0
policies.json `Add policy configurations for endpoint rate limit tests` gateway/testdata/policies.json Added new policy configurations for testing endpoint rate limits with ACL and quota partitions. Defined specific endpoint paths and methods for new policies.	+53/-0

Enhancement

apply.go

Ignore endpoint rate limits when partition is disabled

internal/policy/apply.go

Modified the Apply function to ignore endpoint rate limits when rate
limit partition is disabled.

Set Endpoints to nil if rate limit is not applied.

+1/-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

no api changes detected

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Logic Check
Ensure that setting `Endpoints` to nil under certain conditions does not unintentionally affect other parts of the system that might expect this property to be present.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a check to ensure the key exists in the map before accessing it ___ Consider checking for the presence of the key `"d"` in `s.AccessRights` before accessing it to avoid potential panics due to missing keys. [gateway/policy_test.go [904-905]](https://github.com/TykTechnologies/tyk/pull/6491/files#diff-40d701767204255c38c7dd64939d6bb8df621640c4bddfe5f56080380476a18aR904-R905) ```diff assert.NotEmpty(t, s.AccessRights) -assert.Empty(t, s.AccessRights["d"].Endpoints) +if _, ok := s.AccessRights["d"]; ok { + assert.Empty(t, s.AccessRights["d"].Endpoints) +} ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug by adding a check for the presence of the key "d" in the map before accessing it, which prevents possible panics due to missing keys.	9
Possible bug	Add a nil check before setting `v.Endpoints` to nil ___ Ensure that `v.Endpoints` is only set to nil when `v.Limit` is not nil to avoid potential nil pointer dereference. [internal/policy/apply.go [209]](https://github.com/TykTechnologies/tyk/pull/6491/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R209-R209) ```diff -v.Endpoints = nil +if v.Limit != nil { + v.Endpoints = nil +} ``` Suggestion importance[1-10]: 8 Why: Adding a nil check before setting `v.Endpoints` to nil is a good practice to avoid potential nil pointer dereference, which could lead to runtime errors.	8
Enhancement	Use unique IDs for each policy configuration ___ Ensure unique IDs for different policy configurations to avoid conflicts or unintended behavior due to ID collisions. [gateway/testdata/policies.json [731]](https://github.com/TykTechnologies/tyk/pull/6491/files#diff-d20906fb23674ef58fee794635f5c1f668eb36f9dc4ce0f4b7d3dbd816e13eb5R731-R731) ```diff -"id": "endpoint_rate_limits_on_rate_limit_partition_disabled", +"id": "unique_policy_id_for_each_configuration", ``` Suggestion importance[1-10]: 7 Why: Ensuring unique IDs for different policy configurations is important to prevent conflicts or unintended behavior due to ID collisions, enhancing the robustness of the system.	7
Best practice	Use non-negative values for `quota_max` to avoid misinterpretation ___ Consider using positive values for `quota_max` or a specific flag to indicate no quota, as negative values might be misinterpreted by other parts of the system. [gateway/testdata/policies.json [734]](https://github.com/TykTechnologies/tyk/pull/6491/files#diff-d20906fb23674ef58fee794635f5c1f668eb36f9dc4ce0f4b7d3dbd816e13eb5R734-R734) ```diff -"quota_max": -1, +"quota_max": 0, # Assuming 0 is treated as 'no quota' by the system ``` Suggestion importance[1-10]: 6 Why: Using non-negative values for `quota_max` is a best practice to avoid misinterpretation by other parts of the system, although it may not be crucial if the system already handles negative values correctly.	6