Merging to release-5.7: [TT-13507] Fix for custom domains with substring listen path (#6705)

buger commented 1 week ago

User description

TT-13507 Fix for custom domains with substring listen path (#6705)

User description

Actual fix for domain matching edge case related to substring listen paths. Fixed the sorting function for the listen paths and added some unit tests.

Extra: httpbin service not working on macos with apple silicon; now it does

Description

Related Issue

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

Bug fix, Configuration changes

Description

Fixed the domain sorting logic in the API loader by changing the comparison from domain length to lexicographical order to ensure correct domain matching.
Updated the Docker configuration for the httpbin service to specify the platform as linux/amd64, addressing compatibility issues on macOS with Apple Silicon.

Changes walkthrough 📝

Relevant files

Bug fix

api_loader.go

Fix domain sorting
logic in API loader

gateway/api_loader.go

Modified sorting logic for API specs by domain.

Changed comparison from domain length to lexicographical order.

+1/-1

Configuration changes

httpbin.yml

Specify platform for
httpbin service in Docker configuration

docker/services/httpbin.yml - Added platform specification for httpbin service.

+1/-0

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

PR Type

Bug fix, Configuration changes, Tests

Description

Refactored the sorting logic for API specs in api_loader.go to ensure correct domain matching by prioritizing non-empty domains and sorting by listen path length.
Added platform specification (linux/amd64) for the httpbin service in the Docker configuration to address compatibility issues on macOS with Apple Silicon.
Introduced unit tests in api_loader_test.go to verify the correct order of API specs by listen path.

Changes walkthrough 📝

Relevant files

Bug fix

api_loader.go

Refactor domain sorting logic in API loader

gateway/api_loader.go

Refactored sorting logic for API specs by listen path.

Changed sorting criteria to prioritize non-empty domains.

Extracted sorting logic into a separate function.

+15/-8

Configuration changes

httpbin.yml

Specify platform for httpbin service in Docker

docker/services/httpbin.yml

Added platform specification for httpbin service in Docker
configuration.

+1/-0

Tests

api_loader_test.go

Add unit tests for domain sorting logic

gateway/api_loader_test.go

Added unit tests for domain sorting logic.

Verified correct order of API specs by listen path.

+8888/-1

Additional files (token-limit)

api_loader_test.go

...

gateway/api_loader_test.go ...

+8888/-1

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

github-actions[bot] commented 1 week ago

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

**🎫 Ticket compliance analysis 🔶** **[6705](https://github.com/TykTechnologies/tyk/issues/6705) - Partially compliant** Fully compliant requirements: - Fix domain matching edge case related to substring listen paths. - Update the sorting function for listen paths. - Address compatibility issues on macOS with Apple Silicon for httpbin service. Not compliant requirements: - Add unit tests.

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Sorting Logic
The new sorting logic for API specs should be thoroughly tested to ensure it handles edge cases correctly, especially when domains are empty.

github-actions[bot] commented 1 week ago

API Changes

no api changes detected

github-actions[bot] commented 1 week ago

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Improve the sorting logic to handle cases where both domains are empty ___ Ensure that the sorting logic in `sortSpecsByListenPath` correctly handles cases where `Domain` is empty for both compared elements. Currently, it only checks if they are different, which might lead to inconsistent sorting when both are empty. [gateway/api_loader.go [934-937]](https://github.com/TykTechnologies/tyk/pull/6723/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68R934-R937) ```diff +if specs[i].Domain == "" && specs[j].Domain == "" { + return len(specs[i].Proxy.ListenPath) > len(specs[j].Proxy.ListenPath) +} if (specs[i].Domain == "") != (specs[j].Domain == "") { return specs[i].Domain != "" } return len(specs[i].Proxy.ListenPath) > len(specs[j].Proxy.ListenPath) ``` Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a potential issue in the sorting logic where both domains are empty, which could lead to inconsistent sorting results. The improved code explicitly handles this scenario, ensuring more predictable and stable sorting behavior.	7

TykTechnologies / tyk