[TT-12454] Extract ApplyPolicies into internal/policy scope

titpetric commented 4 months ago

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

github-actions[bot] commented 4 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	No
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The PR description mentions that removing an OrgID check against API spec might break CI, suggesting some tests will pass where they are expected to fail. This needs thorough investigation to ensure that the removal of this check does not introduce security or functional issues.
	Refactoring Scope: The PR involves significant refactoring in critical components like policy application and session management. It's crucial to ensure that these changes do not affect existing functionalities adversely. Each method's logic, especially in the new `policy` package, needs to be reviewed to ensure it aligns with the intended behaviors.
	Interface Implementation: The PR introduces a new `Repository` interface in `gateway.go`. It's important to review if all necessary methods are included and properly implemented in the `Gateway` struct to maintain compatibility and functionality.

github-actions[bot] commented 4 months ago

API Changes

--- prev.txt    2024-06-26 08:35:58.167328189 +0000
+++ current.txt 2024-06-26 08:35:55.231271963 +0000
@@ -8405,6 +8405,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
@@ -9693,6 +9699,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"`
@@ -12203,10 +12213,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] commented 4 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add error handling for policy store initialization and application ___ Refactor the `ApplyPolicies` method to include error handling for the new `policy.New` and `store.Apply` methods. This ensures that any errors during policy application are properly managed and reported. [gateway/middleware.go [353-354]](https://github.com/TykTechnologies/tyk/pull/6367/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR353-R354) ```diff -store := policy.New(t.Gw, log) +store, err := policy.New(t.Gw, log) +if err != nil { + return err // Properly handle initialization errors +} return store.Apply(session) ``` Suggestion importance[1-10]: 10 Why: Adding error handling for the initialization and application of the policy store is crucial for maintaining the stability and reliability of the system.	10
Possible issue	Improve error handling by returning a specific status when a policy is not found ___ Consider handling the case where the policy ID is not found in the `PolicyByID` method. This will ensure that the function returns a meaningful error or default value when the policy ID does not exist, improving error handling and robustness. [gateway/api.go [895-896]](https://github.com/TykTechnologies/tyk/pull/6367/files#diff-644cda3aeb4ac7f325359e85fcddb810f100dd5e6fa480b0d9f9363a743c4e05R895-R896) ```diff if pol, ok := gw.PolicyByID(polID); ok && pol.ID != "" { return pol, http.StatusOK +} else { + return nil, http.StatusNotFound // Handle the case where the policy is not found } ``` Suggestion importance[1-10]: 9 Why: This suggestion improves error handling by ensuring that a specific status is returned when a policy is not found, which enhances the robustness of the function.	9
Error handling	Add error handling for `session.CustomPolicies()` to manage potential invalid data issues ___ Consider handling the case where `session.CustomPolicies()` might return an error due to empty or invalid custom policies. Currently, the code assumes that if `session.CustomPolicies()` returns an error, it should fall back to using `session.PolicyIDs()`. However, this might not always be the correct behavior, especially if the error is due to invalid data rather than the absence of custom policies. It might be beneficial to log this error or handle it explicitly to avoid potential issues with policy application. [internal/policy/apply.go [80-85]](https://github.com/TykTechnologies/tyk/pull/6367/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R80-R85) ```diff customPolicies, err := session.CustomPolicies() if err != nil { - policyIDs = session.PolicyIDs() -} else { - storage = NewStore(customPolicies) - policyIDs = storage.PolicyIDs() + t.logger.Errorf("Error retrieving custom policies: %v", err) + return err } +storage = NewStore(customPolicies) +policyIDs = storage.PolicyIDs() ``` Suggestion importance[1-10]: 9 Why: This suggestion improves error handling by logging the error and returning it, which helps in diagnosing issues related to invalid custom policies. This is a significant improvement for robustness and debugging.	9
Performance	Optimize memory usage by preallocating slice capacity to its exact needed size ___ Use a more specific slice capacity in the `PolicyIDs` method to optimize memory usage. This is beneficial when the number of policies is known to be large, reducing overhead. [gateway/gateway.go [18-21]](https://github.com/TykTechnologies/tyk/pull/6367/files#diff-17cb8b37eda9018fe1c6cdb5f96b3fc948fc8ba49bc516987b8269576db9fcd4R18-R21) ```diff -result := make([]string, 0, len(gw.policiesByID)) +result := make([]string, len(gw.policiesByID)) +i := 0 for id := range gw.policiesByID { - result = append(result, id) + result[i] = id + i++ } ``` Suggestion importance[1-10]: 7 Why: Optimizing memory usage by preallocating the slice capacity to its exact needed size is a good practice, especially when dealing with a large number of policies.	7
Best practice	Use the `Logger()` method consistently for logging to ensure configurations are applied ___ The method `Logger()` is inconsistently accessed using `t.logger` and `t.Logger()`. It is recommended to consistently use the method `Logger()` to access the logger to ensure that any configurations or checks implemented in the method are consistently applied. [internal/policy/apply.go [111]](https://github.com/TykTechnologies/tyk/pull/6367/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R111-R111) ```diff -t.logger.Error(err) +t.Logger().Error(err) ``` Suggestion importance[1-10]: 7 Why: Consistently using the `Logger()` method ensures that any configurations or checks implemented in the method are applied uniformly. This is a good practice for maintainability and consistency.	7