Merging to release-5.3: [TT-11806] Respect domain and listen path (#6289) #6290

buger · 2024-05-17T14:48:09Z

User description

TT-11806 Respect domain and listen path (#6289)

User description

PR reorders api specs with domains to be registered before fallback
(no-domain) specs.

Adds:

Ability to set the host header for tests,
Regression test testing gorilla mux expectations
Moves a domain based api test out of gateway scope
Tests loading apis with/without domain in different order

https://tyktech.atlassian.net/browse/TT-11806

PR Type

enhancement, bug_fix, tests

Description

Enhanced domain handling in API spec loading by prioritizing domain
length.
Fixed minor issues and typos in API loading logic.
Added support for setting the host header in HTTP tests.
Introduced comprehensive regression tests to ensure correct domain
routing.
Added new test API specifications for testing domain and non-domain
routes.

Changes walkthrough 📝

Relevant files

Enhancement

api_loader.go `Enhance API loading with domain prioritization and minor fixes` gateway/api_loader.go Improved domain handling in API spec counting. Fixed a typo in a comment. Modified the sorting logic to prioritize domain length before listen path length.	+7/-5
http.go `Support host header in HTTP tests` test/http.go Added support for setting the host header in HTTP test cases.	+5/-0

Tests

api_test.go `Remove redundant API loading order test` gateway/api_test.go Removed a redundant test case for API loading order.	+0/-50
issue_11806_test.go `Add regression tests for domain routing` tests/regression/issue_11806_test.go Added comprehensive regression tests for domain routing issues. Included tests for mux router and gateway router behaviors.	+165/-0
issue-11806-api-no-domain.json `Add test API spec without domain` tests/regression/testdata/issue-11806-api-no-domain.json Added a new test API spec without a domain.	+24/-0
issue-11806-api-with-domain.json `Add test API spec with domain` tests/regression/testdata/issue-11806-api-with-domain.json Added a new test API spec with a domain.	+24/-0

💡 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

Enhanced domain handling in API spec loading by prioritizing domain length.
Fixed minor issues and typos in API loading logic.
Added support for setting the host header in HTTP tests.
Introduced comprehensive regression tests to ensure correct domain routing.
Added new test API specifications for testing domain and non-domain routes.

Changes walkthrough 📝

Relevant files

Enhancement

api_loader.go `Enhance API loading with domain prioritization and minor fixes` gateway/api_loader.go Improved domain handling in API spec counting. Fixed a typo in a comment. Modified the sorting logic to prioritize domain length before listen path length.	+7/-5
http.go `Support host header in HTTP tests` test/http.go Added support for setting the host header in HTTP tests.	+5/-0

Tests

api_test.go `Remove outdated API loader path order test` gateway/api_test.go Removed a large block of test code related to API loader path ordering.	+0/-50
issue_11806_test.go `Comprehensive regression tests for domain routing` tests/regression/issue_11806_test.go Added a comprehensive regression test suite for domain routing issues. Tests both loading orders of APIs with and without domains. Includes direct testing with gorilla/mux to ensure correct subrouter behavior.	+176/-0
issue-11806-api-no-domain.json `New test API spec without domain` tests/regression/testdata/issue-11806-api-no-domain.json Added a new test API specification without a domain.	+24/-0
issue-11806-api-with-domain.json `New test API spec with domain` tests/regression/testdata/issue-11806-api-with-domain.json Added a new test API specification with a domain.	+24/-0

Configuration changes

ci-tests.yml `Update CI configuration for test exclusions` .github/workflows/ci-tests.yml Updated SonarQube test exclusions to include the 'test' directory.	+1/-1

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

### **User description** PR reorders api specs with domains to be registered before fallback (no-domain) specs. Adds: - Ability to set the host header for tests, - Regression test testing gorilla mux expectations - Moves a domain based api test out of gateway scope - Tests loading apis with/without domain in different order https://tyktech.atlassian.net/browse/TT-11806 ___ ### **PR Type** enhancement, bug_fix, tests ___ ### **Description** - Enhanced domain handling in API spec loading by prioritizing domain length. - Fixed minor issues and typos in API loading logic. - Added support for setting the host header in HTTP tests. - Introduced comprehensive regression tests to ensure correct domain routing. - Added new test API specifications for testing domain and non-domain routes. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement </strong></td><td><table> <tr> <td> <details> <summary><strong>api_loader.go</strong><dd><code>Enhance API loading with domain prioritization and minor fixes</code></dd></summary> <hr> gateway/api_loader.go <li>Improved domain handling in API spec counting.<br> <li> Fixed a typo in a comment.<br> <li> Modified the sorting logic to prioritize domain length before listen <br>path length.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68">+7/-5</a>      </td> </tr> <tr> <td> <details> <summary><strong>http.go</strong><dd><code>Support host header in HTTP tests</code>                                                </dd></summary> <hr> test/http.go - Added support for setting the host header in HTTP test cases. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-a5530e34c740ce6fe2efe8dda5a356463c450696b39b97b91228f1be2491e05e">+5/-0</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>api_test.go</strong><dd><code>Remove redundant API loading order test</code>                                    </dd></summary> <hr> gateway/api_test.go - Removed a redundant test case for API loading order. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-10b4a3d7bdd8d98e48b288d27fd46d9ee436617806c46913fdf7942c0e4a992e">+0/-50</a>    </td> </tr> <tr> <td> <details> <summary><strong>issue_11806_test.go</strong><dd><code>Add regression tests for domain routing</code>                                    </dd></summary> <hr> tests/regression/issue_11806_test.go <li>Added comprehensive regression tests for domain routing issues.<br> <li> Included tests for mux router and gateway router behaviors.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-6414917e8c31f924c3a423765e285a34d988e923bd0f10d5cc56bacad99195d8">+165/-0</a>  </td> </tr> <tr> <td> <details> <summary><strong>issue-11806-api-no-domain.json</strong><dd><code>Add test API spec without domain</code>                                                  </dd></summary> <hr> tests/regression/testdata/issue-11806-api-no-domain.json - Added a new test API spec without a domain. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-e8d8e335343405ef0e02562a51b4f8966cc03fe429e2c4b987504c6147bc00a7">+24/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>issue-11806-api-with-domain.json</strong><dd><code>Add test API spec with domain</code>                                                        </dd></summary> <hr> tests/regression/testdata/issue-11806-api-with-domain.json - Added a new test API spec with a domain. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6289/files#diff-f2b780fe118cacccbfe27e8784d7d2bae3cdc20f37c7bdccb4cf8146c9e6187b">+24/-0</a>    </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **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> (cherry picked from commit 55254bc)

github-actions · 2024-05-17T14:48:47Z

API Changes

--- prev.txt	2024-05-17 14:48:47.288352360 +0000
+++ current.txt	2024-05-17 14:48:44.456309062 +0000
@@ -11278,6 +11278,7 @@
 func (r TCPTestRunner) Run(t testing.TB, testCases ...TCPTestCase) error
 
 type TestCase struct {
+	Host    string `json:",omitempty"`
 	Method  string `json:",omitempty"`
 	Path    string `json:",omitempty"`
 	BaseURL string `json:",omitempty"`

github-actions · 2024-05-17T14:49:02Z

PR Description updated to latest commit (335b36e)

github-actions · 2024-05-17T14:49:21Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves changes across multiple files including Go source files and test files, which requires understanding of the domain logic and the impact of the changes on existing functionality.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The change in `gateway/api_loader.go` where `domain` is assigned "(no host)" directly might lead to unintended side effects if `domain` is used elsewhere after this assignment. It would be safer to use a new variable for the log context to avoid altering the `domain` variable's original content.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/api_loader.go
suggestion	Consider using a new variable for logging purposes instead of modifying the `domain` variable directly. This avoids potential side effects if `domain` is used later in the code. [important]
relevant line	domain = "(no host)"

relevant file	gateway/api_loader.go
suggestion	Add a check to ensure that the `Domain` field is not nil before comparing its length in the sorting logic to prevent potential panics. [important]
relevant line	if specs[i].Domain != specs[j].Domain {

relevant file	gateway/api_loader.go
suggestion	Consider adding a comment explaining the logic behind sorting by domain length before listen path length, as it might not be immediately clear to other developers or future maintainers. [medium]
relevant line	return len(specs[i].Domain) > len(specs[j].Domain)

relevant file	test/http.go
suggestion	Ensure that the `Host` field is properly documented in the struct definition to explain its usage and effects, especially since it's a new addition that might affect existing tests. [medium]
relevant line	Host string `json:",omitempty"`

github-actions · 2024-05-17T14:49:56Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Handle potential nil values in domain comparison It's recommended to handle the case where `specs[i].Domain` or `specs[j].Domain` could be nil to prevent potential runtime panics. gateway/api_loader.go [925-927] if specs[i].Domain != specs[j].Domain { + if specs[i].Domain == nil \|\| specs[j].Domain == nil { + return specs[i].Domain != nil + } return len(specs[i].Domain) > len(specs[j].Domain) } Suggestion importance[1-10]: 8 Why: Handling nil values in domain comparison is crucial to prevent runtime panics, addressing a potential bug.	8
Enhancement	Log when the default domain value is used Add a log entry for cases when the domain is set to the default value to trace how often this fallback is used. gateway/api_loader.go [92-94] if domain == "" { domain = "(no host)" + mainLog.WithFields(logrus.Fields{ + "api_name": spec.Name, + "event": "default_domain_set", + }).Info("Domain set to default due to empty value") } Suggestion importance[1-10]: 6 Why: Adding a log for when the default domain is used could help in debugging and is a good enhancement.	6
Enhancement	Use a more descriptive default domain value Consider using a more descriptive default value than "(no host)" for the domain when it is empty. This will improve clarity in logs and debugging sessions. gateway/api_loader.go [92-94] if domain == "" { - domain = "(no host)" + domain = "default-domain" } Suggestion importance[1-10]: 5 Why: The suggestion to use a more descriptive default value for 'domain' is valid for clarity, but not critical.	5
Maintainability	Improve readability of the count check Refactor the condition to improve readability by directly checking for non-zero count instead of using an equality check against zero. gateway/api_loader.go [91] +if count[domainHash] == 0 { - Suggestion importance[1-10]: 3 Why: The suggestion is minor and focuses on style rather than functionality or performance. The existing code is already clear enough.	3

github-actions · 2024-05-17T14:58:28Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

sonarcloud · 2024-05-20T08:13:40Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

buger requested a review from a team as a code owner May 17, 2024 14:48

buger enabled auto-merge (squash) May 17, 2024 14:48

github-actions bot added enhancement Tests Bug fix labels May 17, 2024

github-actions bot added the Review effort [1-5]: 3 label May 17, 2024

titpetric disabled auto-merge May 20, 2024 07:59

titpetric enabled auto-merge (squash) May 20, 2024 08:00

titpetric disabled auto-merge May 20, 2024 08:00

titpetric enabled auto-merge (squash) May 20, 2024 08:00

titpetric merged commit 7fef419 into release-5.3 May 20, 2024
34 checks passed

titpetric deleted the merge/release-5.3/55254bc8bfc6d71996c798891388721e54475ca7 branch May 20, 2024 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging to release-5.3: [TT-11806] Respect domain and listen path (#6289) #6290

Merging to release-5.3: [TT-11806] Respect domain and listen path (#6289) #6290

buger commented May 17, 2024 •

edited by github-actions bot

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

sonarcloud bot commented May 20, 2024

Merging to release-5.3: [TT-11806] Respect domain and listen path (#6289) #6290

Merging to release-5.3: [TT-11806] Respect domain and listen path (#6289) #6290

Conversation

buger commented May 17, 2024 • edited by github-actions bot

User description

User description

PR Type

Description

Changes walkthrough 📝

PR Type

Description

Changes walkthrough 📝

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

github-actions bot commented May 17, 2024

PR Review 🔍

github-actions bot commented May 17, 2024

PR Code Suggestions ✨

github-actions bot commented May 17, 2024

💥 CI tests failed 🙈

git-state

sonarcloud bot commented May 20, 2024

Quality Gate passed

buger commented May 17, 2024 •

edited by github-actions bot