Merging to release-5.4: [TT-12454] Extract ApplyPolicies into internal/policy scope (#6367)

[buger]

User description

TT-12454 Extract ApplyPolicies into internal/policy scope (#6367)

User description

This extracts a large problematic ApplyPolicies function into it's own package scope. It does this by:

• defining an interface for policy access and deleting inline mutex use
• updated *Gateway to implement storage interface
• implement storage interface in policy.Store as well (used inline)

On top of that:

• Fixes a bug in the Duration/Less() functions on user.APILimit, fixes tests
• Adds a test particular to ApplyRateLimits (decrease coginitive complexity)

The duration was calculated as rate/per, however, the correct way was per/rate; This fixes it so duration is calculated correctly, fixing the Less function comparison.

---

PR Type

Enhancement, Bug fix

---

Description

• Refactored handleGetPolicy to use the new PolicyByID method.
• Introduced a Repository interface and added methods PolicyIDs, PolicyByID, and PolicyCount to the Gateway struct.
• Refactored ApplyPolicies in BaseMiddleware to use the new policy package.
• Updated buildNodeInfo to use PolicyCount instead of policiesByIDLen.
• Removed redundant methods getPolicy and policiesByIDLen from Gateway.

---

Changes walkthrough 📝

Relevant files

Enhancement

api.go Refactor policy retrieval in `handleGetPolicy`.                    gateway/api.go - Replaced `getPolicy` with `PolicyByID` in `handleGetPolicy`. +1/-1      gateway.go Add policy-related methods and interface to Gateway.          gateway/gateway.go Introduced Repository interface. Added methods PolicyIDs, PolicyByID, and PolicyCount. +38/-0    middleware.go Refactor ApplyPolicies to use new policy store.                    gateway/middleware.go Removed clearSession method. Refactored ApplyPolicies to use policy.New and store.Apply. +3/-411  rpc_storage_handler.go Update policy count retrieval in buildNodeInfo.                    gateway/rpc_storage_handler.go - Replaced `policiesByIDLen` with `PolicyCount` in `buildNodeInfo`. +1/-1      server.go Remove redundant policy methods from Gateway.                        gateway/server.go - Removed `getPolicy` and `policiesByIDLen` methods. +0/-13

---

💡 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 tit@tyk.io

---

PR Type

Enhancement, Bug fix, Tests

---

Description

• Refactored handleGetPolicy to use the new PolicyByID method.
• Introduced a Repository interface and added methods PolicyIDs, PolicyByID, and PolicyCount to the Gateway struct.
• Refactored ApplyPolicies in BaseMiddleware to use the new policy package.
• Updated buildNodeInfo to use PolicyCount instead of policiesByIDLen.
• Removed redundant methods getPolicy and policiesByIDLen from Gateway.
• Fixed a bug in the Duration method of APILimit.
• Added tests for ApplyRateLimits and Apply methods in the policy package.
• Added Taskfile for running tests and coverage.

---

Changes walkthrough 📝

	Relevant files
Enhancement	10 files api.go Refactor `handleGetPolicy` to use `PolicyByID` method        gateway/api.go - Refactored `handleGetPolicy` to use `PolicyByID` method. +1/-1      gateway.go Introduce `Repository` interface and methods in `Gateway` gateway/gateway.go Introduced Repository interface. Added methods PolicyIDs, PolicyByID, and PolicyCount to Gateway. +38/-0    middleware.go Refactor `ApplyPolicies` to use new `policy` package          gateway/middleware.go Refactored ApplyPolicies to use the new policy package. Removed clearSession method. +7/-411  mw_rate_limiting.go Update error message in `handleRateLimitFailure`                  gateway/mw_rate_limiting.go - Updated error message in `handleRateLimitFailure`. +1/-1      rpc_storage_handler.go Refactor `buildNodeInfo` to use `PolicyCount` method          gateway/rpc_storage_handler.go - Refactored `buildNodeInfo` to use `PolicyCount` method. +1/-1      server.go Remove redundant methods from `Gateway`                                    gateway/server.go - Removed `getPolicy` and `policiesByIDLen` methods. +0/-13    event.go Remove `RateLimitExceeded` event from `eventMap`                  internal/event/event.go - Removed `RateLimitExceeded` event from `eventMap`. +0/-1      apply.go Extract `ApplyPolicies` logic into `policy` package            internal/policy/apply.go Extracted ApplyPolicies logic into policy package. Introduced Service struct and methods ClearSession and Apply. +478/-0  store.go Introduce `Store` struct implementing `Repository` interface internal/policy/store.go - Introduced `Store` struct implementing `Repository` interface. +35/-0    util.go Add utility functions for policy handling                                internal/policy/util.go - Added utility functions for policy handling. +129/-0
Tests	5 files policy_test.go Update tests to use `PolicyByID` method                                    gateway/policy_test.go - Updated tests to use `PolicyByID` method. +17/-4    server_test.go Update tests to use `PolicyCount` method                                  gateway/server_test.go - Updated tests to use `PolicyCount` method. +1/-1      event_test.go Update test to use `RateLimitSmoothingUp` event                    internal/event/event_test.go - Updated test to use `RateLimitSmoothingUp` event. +1/-1      apply_test.go Add tests for `ApplyRateLimits` and `Apply` methods            internal/policy/apply_test.go Added tests for ApplyRateLimits and Apply methods in policy package. +124/-0  session_test.go Update tests for `Duration` method and remove `Less` method tests user/session_test.go Updated tests for Duration method of APILimit. Removed tests for Less method. +1/-39
Bug fix	1 files session.go Fix bug in `Duration` method and remove `Less` method        user/session.go Fixed bug in Duration method of APILimit. Removed Less method from APILimit. +1/-6
Configuration changes	1 files Taskfile.yml Add Taskfile for running tests and coverage                            internal/policy/Taskfile.yml - Added Taskfile for running tests and coverage. +40/-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]]

API Changes

--- prev.txt    2024-06-26 09:20:42.370330174 +0000
+++ current.txt 2024-06-26 09:20:39.414303248 +0000
@@ -8234,6 +8234,12 @@

 func (gw *Gateway) NotifyCurrentServerStatus()

+func (gw *Gateway) PolicyByID(polID string) (user.Policy, bool)
+
+func (gw *Gateway) PolicyCount() int
+
+func (gw *Gateway) PolicyIDs() []string
+
 func (gw *Gateway) ProcessOauthClientsOps(clients map[string]string)
     ProcessOauthClientsOps performs the appropriate action for the received
     clients it can be any of the Create,Update and Delete operations
@@ -9522,6 +9528,10 @@
     TickOk triggers a reload and ensures a queue happened and a reload cycle
     happens. This will block until all the cases are met.

+type Repository interface {
+   policy.Repository
+}
+
 type RequestDefinition struct {
    Method      string            `json:"method"`
    Headers     map[string]string `json:"headers"`
@@ -12032,10 +12042,6 @@

 func (limit APILimit) IsEmpty() bool

-func (g *APILimit) Less(in APILimit) bool
-    Less will return true if the receiver has a smaller duration between
-    requests than `in`.
-
 type AccessDefinition struct {
    APIName              string                  `json:"api_name" msg:"api_name"`
    APIID                string                  `json:"api_id" msg:"api_id"`

[github-actions[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The refactoring of the ApplyPolicies function into the new policy package might introduce issues if not all edge cases are handled properly. The logic is complex and the changes are significant, so careful review of the new policy.Service implementation and its interaction with existing systems is crucial.
	Performance Concern: The new implementation locks and unlocks the mutex multiple times within loops in several functions such as PolicyIDs and PolicyByID. This could lead to performance degradation, especially under high load. Consider optimizing the locking strategy.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add nil checks to prevent potential runtime errors from nil pointer dereferences ___ **To prevent potential nil pointer dereferences, add checks for nil before dereferencing pointers such as orgID and logger in the Service struct methods.** [internal/policy/apply.go [112-116]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R112-R116) ```diff -if t.orgID != nil && policy.OrgID != *t.orgID { +if t.orgID == nil { + return errors.New("orgID is nil") +} +if policy.OrgID != *t.orgID { err := errors.New("attempting to apply policy from different organisation to key, skipping") t.Logger().Error(err) return err } ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential bug by adding nil checks, which is crucial for preventing runtime errors and ensuring the robustness of the code.	10
	Add nil check for t.Spec to prevent nil pointer dereference ___ **Consider checking if t.Spec is not nil before accessing t.Spec.OrgID to avoid potential nil pointer dereference.** [gateway/middleware.go [354-355]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR354-R355) ```diff -if t.Spec != nil { - orgID = &t.Spec.OrgID +if t.Spec == nil { + return errors.New("specification is nil"), http.StatusInternalServerError } +orgID = &t.Spec.OrgID ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential nil pointer dereference, which is a significant issue that can cause runtime errors. The proposed change ensures that the code handles the case where `t.Spec` is nil, improving robustness.	8
	Add nil checks for session and apiLimits to prevent potential runtime panics ___ **Consider handling the case where apiLimits or session might be nil to prevent potential nil pointer dereferences. This is crucial especially in a testing environment where mocks or incomplete data might be used.** [internal/policy/apply_test.go [26]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R26-R26) ```diff -svc.ApplyRateLimits(session, policy, &apiLimits) +if session == nil || apiLimits == nil { + return // or handle error appropriately +} +svc.ApplyRateLimits(session, policy, apiLimits) ``` Suggestion importance[1-10]: 8 Why: Adding nil checks is crucial to prevent potential runtime panics, especially in a testing environment where mocks or incomplete data might be used. This improves the robustness of the tests.	8
Maintainability	Refactor the ApplyPolicies method to improve readability and maintainability by separating concerns ___ **Refactor the ApplyPolicies method to separate concerns more clearly. Split the method into smaller, more focused methods, such as validatePolicy, applyPolicyToSession, and finalizeSessionUpdates. This will make the code easier to understand, maintain, and test.** [internal/policy/apply.go [70-432]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R70-R432) ```diff func (t *Service) Apply(session *user.SessionState) error { - ... + if err := t.validatePolicies(session); err != nil { + return err + } + if err := t.applyPoliciesToSession(session); err != nil { + return err + } + t.finalizeSessionUpdates(session) return nil } ``` Suggestion importance[1-10]: 9 Why: The suggestion significantly improves code readability and maintainability by breaking down a large method into smaller, focused methods. This makes the code easier to understand and test.	9
	Encapsulate the logic for merging AccessRights into a separate method to improve code maintainability ___ **Consider implementing a method to handle the merging of AccessRights to encapsulate the logic and improve maintainability. This method would handle the merging of quotas, rate limits, and other attributes of AccessRights.** [internal/policy/apply.go [177-235]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R177-R235) ```diff -if r, ok := rights[k]; ok { - // If GQL introspection is disabled, keep that configuration. - if v.DisableIntrospection { - r.DisableIntrospection = v.DisableIntrospection - } - r.Versions = appendIfMissing(rights[k].Versions, v.Versions...) - ... - ar = r -} +ar = mergeAccessRights(ar, v) ``` Suggestion importance[1-10]: 8 Why: This suggestion improves code maintainability by encapsulating complex merging logic into a separate method, making the code easier to read and maintain.	8
	Improve error handling in the policy application process for better control and testing ___ **Replace the direct error logging within the Apply method with a more structured error handling approach that allows for better control and testing. Instead of logging and returning errors immediately, accumulate errors in a slice and handle them collectively after the policy application loop.** [internal/policy/apply.go [102-108]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R102-R108) ```diff -err := fmt.Errorf("policy not found: %q", polID) -t.Logger().Error(err) -if len(policyIDs) > 1 { - continue +var errs []error +errs = append(errs, fmt.Errorf("policy not found: %q", polID)) +if len(policyIDs) == 1 { + return fmt.Errorf("errors encountered: %v", errs) } -return err ``` Suggestion importance[1-10]: 7 Why: The suggestion improves error handling by accumulating errors and handling them collectively, which enhances maintainability and testability. However, it introduces complexity that may not be necessary for all use cases.	7
	Simplify policy ID check by handling it inside the PolicyByID method ___ **Refactor the condition to check the policy ID directly in the PolicyByID method, simplifying the calling code.** [gateway/api.go [895-896]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-644cda3aeb4ac7f325359e85fcddb810f100dd5e6fa480b0d9f9363a743c4e05R895-R896) ```diff -if pol, ok := gw.PolicyByID(polID); ok && pol.ID != "" { +if pol, ok := gw.PolicyByID(polID); ok { return pol, http.StatusOK } ``` Suggestion importance[1-10]: 5 Why: Simplifying the condition check can make the code more readable and maintainable. However, this change is minor and does not address any critical issues.	5
Best practice	Add thread safety to the Store type by implementing a mutex ___ **Implement concurrency protection for the Store type to ensure thread safety, especially since the comment mentions the lack of concurrency protections. This can be achieved by using a mutex.** [internal/policy/store.go [10-12]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-13dec7bc453c9ff99550c83d2f86a017bbf7fb863584dc30603af15d29ef9d3dR10-R12) ```diff type Store struct { policies map[string]user.Policy + mu sync.Mutex +} +func (s *Store) PolicyByID(id string) (user.Policy, bool) { + s.mu.Lock() + defer s.mu.Unlock() + v, ok := s.policies[id] + return v, ok } ``` Suggestion importance[1-10]: 9 Why: Implementing concurrency protection is important for thread safety, especially since the comment mentions the lack of concurrency protections. This is a best practice for ensuring data integrity in concurrent environments.	9
Enhancement	Use dependency injection for the policy store ___ **Instead of creating a new policy store in each call to ApplyPolicies, consider using a dependency injection to provide the policy store, which can improve testability and reduce coupling.** [gateway/middleware.go [357-358]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR357-R358) ```diff -store := policy.New(orgID, t.Gw, log) -return store.Apply(session) +return t.policyStore.Apply(session) ``` Suggestion importance[1-10]: 7 Why: Using dependency injection can improve testability and reduce coupling, which are good practices for maintainable code. However, this change is more of an enhancement rather than a critical fix.	7
	Enhance the test assertions to cover changes to both Rate and Per fields ___ **To ensure that the ApplyRateLimits method is correctly updating the apiLimits and session, consider adding more granular assertions for the Per field as well, similar to what is done for the Rate field.** [internal/policy/apply_test.go [28-29]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R28-R29) ```diff assert.Equal(t, 10, int(apiLimits.Rate)) +assert.Equal(t, 10, int(apiLimits.Per)) assert.Equal(t, 5, int(session.Rate)) +assert.Equal(t, 10, int(session.Per)) ``` Suggestion importance[1-10]: 7 Why: Adding more granular assertions for the `Per` field ensures that the `ApplyRateLimits` method is thoroughly tested, improving test coverage and reliability.	7
	Enhance error message clarity in rate limit handling ___ **The error message in handleRateLimitFailure should provide more context about the failure. Consider including the rateLimitKey in the error message.** [gateway/middleware.go [525]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR525-R525) ```diff -return errors.New(message), http.StatusTooManyRequests +return errors.New(fmt.Sprintf("%s - Key: %s", message, rateLimitKey)), http.StatusTooManyRequests ``` Suggestion importance[1-10]: 6 Why: Providing more context in error messages can be helpful for debugging and understanding issues. This suggestion improves the clarity of the error message but is not crucial for functionality.	6
Performance	Optimize the appendIfMissing function to improve performance by reducing slice operations ___ **The appendIfMissing function can be optimized by directly appending new items to the result slice instead of recreating the src slice. This reduces unnecessary operations and improves performance.** [internal/policy/util.go [36-39]](https://github.com/TykTechnologies/tyk/pull/6372/files#diff-0323c3da13f08a9ccd340ac04208d680856354fd566dffcad925fa6645639955R36-R39) ```diff -src = uniqueSorted(src, srcMap) -in = uniqueSorted(in, srcMap) -return append(src, in...) +result := make([]string, 0, len(srcMap)) +for _, v := range src { + if srcMap[v] { + result = append(result, v) + delete(srcMap, v) + } +} +for _, v := range in { + if srcMap[v] { + result = append(result, v) + delete(srcMap, v) + } +} +return result ``` Suggestion importance[1-10]: 6 Why: The suggested optimization reduces unnecessary operations and improves performance, but the current implementation is already functional and clear.	6

[sonarcloud[bot]]

Quality Gate failed

Failed conditions
30.0% 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