Merging to release-5.3: [TT-12865] Rename config parameter, update usage, support mux params on legacy (#6506)

buger commented 1 month ago

User description

TT-12865 Rename config parameter, update usage, support mux params on legacy (#6506)

PR Type

enhancement

Description

Renamed configuration fields EnablePrefixMatching and EnableSuffixMatching to EnablePathPrefixMatching and EnablePathSuffixMatching respectively, across multiple files.
Updated comments and documentation to reflect the new naming conventions.
Modified test cases to use the updated configuration fields.
Refactored middleware and session manager logic to accommodate the new path matching configuration.

Changes walkthrough 📝

Relevant files

Enhancement

config.go `Rename configuration fields for path matching options` config/config.go Renamed `EnablePrefixMatching` to `EnablePathPrefixMatching`. Renamed `EnableSuffixMatching` to `EnablePathSuffixMatching`. Updated comments to reflect the new naming conventions.	+12/-12
api_definition.go `Update API definition to use new path matching config` gateway/api_definition.go Updated variable names to use new path matching configuration fields.	+2/-2
mw_granular_access.go `Refactor middleware to use updated path matching config` gateway/mw_granular_access.go Updated variable names to use new path matching configuration fields. Refactored pattern preparation using `PreparePathRegexp`.	+3/-9
session_manager.go `Update session manager for new path matching config` gateway/session_manager.go Updated variable names to use new path matching configuration fields.	+2/-2
schema.json `Update JSON schema for path matching configuration` cli/linter/schema.json - Renamed JSON schema fields for path matching options.	+2/-2

Tests

api_definition_test.go `Update test configuration for path prefix matching` gateway/api_definition_test.go - Modified test configuration to use `EnablePathPrefixMatching`.	+1/-1
mw_granular_access_test.go `Update middleware test for path prefix matching` gateway/mw_granular_access_test.go - Modified test configuration to use `EnablePathPrefixMatching`.	+1/-1

💡 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, Tests

Description

Introduced new configuration options EnablePathPrefixMatching and EnablePathSuffixMatching to enhance URL path matching capabilities.
Refactored URLSpec and related logic to support the new path matching configuration.
Updated middleware and session management to utilize the new path matching options.
Added comprehensive tests for the new path matching logic, including regression tests for issue 12865.
Improved logging and error handling in path matching processes.

Changes walkthrough 📝

Relevant files

Enhancement

6 files

config.go `Add configuration for path prefix and suffix matching` config/config.go Added new configuration fields `EnablePathPrefixMatching` and `EnablePathSuffixMatching`. Updated comments to explain the new path matching configuration.	+43/-0
api_definition.go `Refactor URLSpec and update path matching logic` gateway/api_definition.go Refactored URLSpec to use private fields and methods. Updated path matching logic to support new configuration. Renamed several functions for clarity.	+47/-142
model_apispec.go `Refactor and deprecate CheckSpecMatchesStatus` gateway/model_apispec.go Moved `CheckSpecMatchesStatus` to a separate file and deprecated it. Added `FindSpecMatchesStatus` for improved status checking.	+75/-0
model_urlspec.go `Add path and method matching methods to URLSpec` gateway/model_urlspec.go Added new methods to URLSpec for matching paths and methods. Deprecated `modeSpecificSpec` method.	+120/-0
mw_granular_access.go `Update GranularAccessMiddleware for new path matching` gateway/mw_granular_access.go Updated middleware to use new path matching configuration. Improved logging and error handling in path matching.	+74/-20
mux.go `Refactor path regex preparation and matching` internal/httputil/mux.go Refactored path regex preparation and matching functions. Improved caching and handling of mux-style parameters.	+78/-18

Tests

4 files

api_definition_test.go `Update and add test cases for path matching logic` gateway/api_definition_test.go Updated test cases to reflect changes in path matching logic. Added new test cases for path prefix matching.	+12/-12
mw_granular_access_test.go `Add tests for GranularAccessMiddleware path matching` gateway/mw_granular_access_test.go Added tests for new path matching logic in GranularAccessMiddleware. Updated existing tests to use new configuration.	+50/-28
mux_test.go `Add tests for path regex preparation and matching` internal/httputil/mux_test.go Added tests for new path matching functions. Updated existing tests to reflect changes in regex preparation.	+112/-14
issue_12865_test.go `Add regression tests for issue 12865` tests/regression/issue_12865_test.go Added regression tests for issue 12865. Tested various configurations of path prefix and suffix matching.	+171/-0

Configuration changes

2 files

test.yml `Update test task configuration` .taskfiles/test.yml Updated test task to exclude the first package in the list. Added a task to merge coverage reports.	+2/-1
schema.json `Update schema for new path matching configuration` cli/linter/schema.json - Added new schema fields for path prefix and suffix matching.	+6/-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

--- prev.txt    2024-09-12 17:32:03.415738583 +0000
+++ current.txt 2024-09-12 17:31:59.983726905 +0000
@@ -5514,6 +5514,49 @@
    // Regular expressions and parameterized routes will be left alone regardless of this setting.
    EnableStrictRoutes bool `json:"enable_strict_routes"`

+   // EnablePathPrefixMatching changes the URL matching from wildcard mode to prefix mode.
+   // For example, `/json` matches `*/json*` by current default behaviour.
+   // If prefix matching is enabled, the match will be performed as a prefix match (`/json*`).
+   //
+   // The `/json` url would be matched as `^/json` against the following paths:
+   //
+   // - Full listen path and versioning URL (`/listen-path/v4/json`)
+   // - Stripped listen path URL (`/v4/json`)
+   // - Stripped version information (`/json`) - match.
+   //
+   // If versioning is disabled then the following URLs are considered:
+   //
+   // - Full listen path and endpoint (`/listen-path/json`)
+   // - Stripped listen path (`/json`) - match.
+   //
+   // For inputs that start with `/`, a prefix match is ensured by
+   // prepending the start of string `^` caret.
+   //
+   // For all other cases, the pattern remains unmodified.
+   //
+   // Combine this option with `enable_path_suffix_matching` to achieve
+   // exact url matching with `/json` being evaluated as `^/json$`.
+   EnablePathPrefixMatching bool `json:"enable_path_prefix_matching"`
+
+   // EnablePathSuffixMatching changes the URL matching to match as a suffix.
+   // For example: `/json` is matched as `/json$` against the following paths:
+   //
+   // - Full listen path and versioning URL (`/listen-path/v4/json`)
+   // - Stripped listen path URL (`/v4/json`)
+   // - Stripped version information (`/json`) - match.
+   //
+   // If versioning is disabled then the following URLs are considered:
+   //
+   // - Full listen path and endpoint (`/listen-path/json`)
+   // - Stripped listen path (`/json`) - match.
+   //
+   // If the input pattern already ends with a `$` (`/json$`)
+   // then the pattern remains unmodified.
+   //
+   // Combine this option with `enable_path_prefix_matching` to achieve
+   // exact url matching with `/json` being evaluated as `^/json$`.
+   EnablePathSuffixMatching bool `json:"enable_path_suffix_matching"`
+
    // Disable TLS verification. Required if you are using self-signed certificates.
    SSLInsecureSkipVerify bool `json:"ssl_insecure_skip_verify"`

@@ -7424,10 +7467,16 @@
 func CloneAPI(a *APISpec) *APISpec

 func (a *APISpec) CheckSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (bool, interface{})
-    CheckSpecMatchesStatus checks if a url spec has a specific status
+    CheckSpecMatchesStatus checks if a URL spec has a specific status.
+    Deprecated: The function doesn't follow go return conventions (T, ok);
+    use FindSpecMatchesStatus;

 func (a *APISpec) Expired() bool

+func (a *APISpec) FindSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (*URLSpec, bool)
+    FindSpecMatchesStatus checks if a URL spec has a specific status and returns
+    the URLSpec for it.
+
 func (s *APISpec) FireEvent(name apidef.TykEvent, meta interface{})

 func (a *APISpec) GetSessionLifetimeRespectsKeyExpiration() bool
@@ -7450,6 +7499,12 @@
 func (a *APISpec) StopSessionManagerPool()

 func (a *APISpec) StripListenPath(reqPath string) string
+    StripListenPath will strip the listen path from the URL, keeping version in
+    tact.
+
+func (a *APISpec) StripVersionPath(reqPath string) string
+    StripVersionPath will strip the version from the URL. The input URL should
+    already have listen path stripped.

 func (a *APISpec) URLAllowedAndIgnored(r *http.Request, rxPaths []URLSpec, whiteListStatus bool) (RequestStatus, interface{})
     URLAllowedAndIgnored checks if a url is allowed and ignored.
@@ -10074,7 +10129,6 @@
     system, return an error to have the chain fail

 type URLSpec struct {
-   Spec                      *regexp.Regexp
    Status                    URLStatus
    MethodActions             map[string]apidef.EndpointMethodMeta
    Whitelist                 apidef.EndPointMeta
@@ -10102,6 +10156,7 @@
    PersistGraphQL            apidef.PersistGraphQLMeta

    IgnoreCase bool
+   // Has unexported fields.
 }
     URLSpec represents a flattened specification for URLs, used to check if
     a proxy URL path is on any of the white, black or ignored lists. This is

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Performance Concern
The implementation of `GranularAccessMiddleware.ProcessRequest` may lead to performance issues due to the repeated compilation of regex patterns within a loop (lines 100-105). Consider caching compiled regex patterns to avoid recompilation on every request. Error Handling
The function `MatchPaths` does not properly handle or log errors when regex matching fails. This could lead to missed debugging information or unhandled exceptions. Consider improving error handling and logging in this function.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Handle errors from regex compilation explicitly to avoid security risks ___ Ensure that the error from `regexp.Compile` is not just logged but also handled appropriately to prevent potential security issues or incorrect behavior if the regex is not compiled correctly. [gateway/mw_granular_access.go [104-109]](https://github.com/TykTechnologies/tyk/pull/6507/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R104-R109) ```diff asRegex, err := regexp.Compile(pattern) if err != nil { logger.WithError(err).Error("error compiling regex") - continue + return errors.New("regex compilation failed"), http.StatusInternalServerError } ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential security issue by ensuring that errors from regex compilation are handled explicitly, preventing incorrect behavior or vulnerabilities. It is a significant improvement for robustness.	9
Performance	Precompile regex patterns during initialization to enhance performance ___ Use a more efficient approach for matching URL paths by compiling regex patterns once during initialization instead of recompiling them for every request, which can significantly improve performance. [gateway/mw_granular_access.go [100-111]](https://github.com/TykTechnologies/tyk/pull/6507/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R100-R111) ```diff -pattern := httputil.PreparePathRegexp(accessSpec.URL, isPrefixMatch, isSuffixMatch) -asRegex, err := regexp.Compile(pattern) +// Assume regex patterns are compiled and stored in accessSpec during initialization +match := accessSpec.CompiledRegex.MatchString(urlPath) ``` Suggestion importance[1-10]: 8 Why: Precompiling regex patterns can significantly improve performance by avoiding repeated compilation during each request. This is a valuable optimization, especially for high-traffic applications.	8
Maintainability	Replace `slices.Contains` with a custom method existence check function ___ Replace the use of `slices.Contains` with a custom function to check for method existence in `accessSpec.Methods`. This avoids importing the `slices` package which is not necessary for such a simple operation, improving the maintainability and reducing external dependencies. [gateway/mw_granular_access.go [56-58]](https://github.com/TykTechnologies/tyk/pull/6507/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R56-R58) ```diff -if !slices.Contains(accessSpec.Methods, r.Method) { +if !methodExists(accessSpec.Methods, r.Method) { continue } ``` Suggestion importance[1-10]: 7 Why: This suggestion improves maintainability by reducing external dependencies, which is beneficial for a simple operation like checking method existence. However, the performance gain is minimal, so it is not crucial.	7
Maintainability	Refactor logging logic into a separate function to enhance readability and reduce redundancy ___ Refactor the logging logic to a separate function to reduce redundancy and improve code readability. This function can handle both debug and error logging based on parameters. [gateway/mw_granular_access.go [69-77]](https://github.com/TykTechnologies/tyk/pull/6507/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436R69-R77) ```diff -if gwConfig.LogLevel == "debug" \|\| err != nil { - logger = logger.WithError(err).WithField("pattern", pattern).WithField("match", match) - if err != nil { - logger.Error("error matching endpoint") - } else { - logger.Debug("matching endpoint") - } -} +logEndpointMatching(gwConfig.LogLevel, logger, err, pattern, match) ``` Suggestion importance[1-10]: 6 Why: Refactoring the logging logic into a separate function improves code readability and maintainability by reducing redundancy. This is a minor improvement and not critical to the functionality.	6
Enhancement	Optimize conditional logic for pattern manipulation ___ Refactor the `PreparePathRegexp` function to avoid redundant checks and concatenations by structuring the conditional logic more efficiently. [internal/httputil/mux.go [48-57]](https://github.com/TykTechnologies/tyk/pull/6507/files#diff-3d9ee5f5e946d72e6f2ae662ff03ee5253bbdc15203d2e4f6e9f46c13011ebf8R48-R57) ```diff -if prefix && strings.HasPrefix(pattern, "/") { - pattern = "^" + pattern +if prefix { + pattern = "^" + strings.TrimLeft(pattern, "^") } -if suffix && !strings.HasSuffix(pattern, "$") { - pattern = pattern + "$" +if suffix { + pattern = strings.TrimRight(pattern, "$") + "$" } ``` Suggestion importance[1-10]: 7 Why: The suggestion improves the code by reducing redundancy and making the logic for adding prefix and suffix more efficient. This enhances code readability and maintainability without altering functionality. However, it is not a critical change, hence a moderate score.	7