[TT-11997] Backend logic for request_headers_rewrite by buraksezer · Pull Request #6306 · TykTechnologies/tyk

buraksezer · 2024-05-23T13:32:34Z

User description

See https://tyktech.atlassian.net/browse/TT-11997 for details of the request headers rewrite rules.

PR Type

Enhancement, Tests

Description

Added tests for request headers rewrite rules in GraphQL middleware.
Integrated request headers rewrite logic into the reverse proxy handler.
Added RequestHeadersRewrite to ReverseProxyParams struct.
Passed RequestHeadersRewrite to the GraphQL engine transport for both v1 and v2.
Implemented logic to apply request headers rewrite rules in the GraphQL engine transport, covering three different rewrite scenarios.

Changes walkthrough 📝

Relevant files

Tests

mw_graphql_test.go `Add tests for request headers rewrite rules in GraphQL middleware.` gateway/mw_graphql_test.go Added tests for request headers rewrite rules. Implemented three test cases for different rewrite scenarios.	+137/-0

Enhancement

reverse_proxy.go `Integrate request headers rewrite logic in reverse proxy handler.` gateway/reverse_proxy.go Integrated request headers rewrite logic into the reverse proxy handler.	+8/-7
engine.go `Add RequestHeadersRewrite to ReverseProxyParams struct.` internal/graphengine/engine.go Added `RequestHeadersRewrite` to `ReverseProxyParams` struct.	+8/-7
graphql_go_tools_v1.go `Pass RequestHeadersRewrite to GraphQL engine transport (v1).` internal/graphengine/graphql_go_tools_v1.go Passed `RequestHeadersRewrite` to the GraphQL engine transport.	+1/-0
graphql_go_tools_v2.go `Pass RequestHeadersRewrite to GraphQL engine transport (v2).` internal/graphengine/graphql_go_tools_v2.go Passed `RequestHeadersRewrite` to the GraphQL engine transport.	+1/-0
transport.go `Implement request headers rewrite logic in GraphQL engine transport.` internal/graphengine/transport.go Added logic to apply request headers rewrite rules in the GraphQL engine transport. Implemented three rewrite rules for request headers.	+96/-1

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

github-actions · 2024-05-23T13:33:22Z

API Changes

no api changes detected

github-actions · 2024-05-23T13:33:39Z

PR Description updated to latest commit (46b54fa)

github-actions · 2024-05-23T13:34:03Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple files and integrates new functionality across different components of the system. The complexity of the changes, especially around header rewriting logic, requires careful review to ensure correctness and efficiency.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The header rewrite logic in `GraphQLEngineTransport` might not correctly handle cases where headers are supposed to be removed or added based on complex conditions. The current implementation could lead to unintended header manipulations.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/mw_graphql_test.go
suggestion	Consider adding more comprehensive test cases to cover scenarios where multiple headers are manipulated simultaneously, ensuring that the rewrite rules do not interfere with each other. [important]
relevant line	_, _ = g.Run(t, []test.TestCase{

relevant file	internal/graphengine/transport.go
suggestion	Refactor the `applyRequestHeadersRewriteRules` function to separate the concerns of checking and applying rewrite rules into distinct methods. This will improve readability and maintainability of the code. [important]
relevant line	func (g GraphQLEngineTransport) applyRequestHeadersRewriteRules(r http.Request) {

relevant file	internal/graphengine/transport.go
suggestion	Implement unit tests specifically for the `applyRequestHeadersRewriteRules` function to ensure each rewrite rule behaves as expected under various scenarios. [important]
relevant line	func (g GraphQLEngineTransport) applyRequestHeadersRewriteRules(r http.Request) {

relevant file	internal/graphengine/transport.go
suggestion	Optimize the header rewrite logic to avoid unnecessary deletions and re-sets of headers which could lead to performance issues in high-load environments. Consider checking if the header actually needs changing before modifying it. [medium]
relevant line	r.Header.Del(key)

github-actions · 2024-05-23T13:34:29Z

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Add a test case to verify header addition when the header is not present but defined in the rewrite rules with `Remove` set to false Consider adding a test case to verify the behavior when the header key is not present in the request but is defined in the `RequestHeadersRewrite` with `Remove` set to false. This will ensure that the header is added as expected. gateway/mw_graphql_test.go [646-663] _, _ = g.Run(t, []test.TestCase{ { Data: request, Method: http.MethodPost, Headers: map[string]string{ "X-Tyk-Key": "tyk-value", "X-Other-Key": "other-value", - "X-Tyk-Test": "value-from-consumer", }, Code: http.StatusOK, BodyMatch: `{"data":{"hello":"World","httpMethod":"POST"}}`, HeadersMatch: map[string]string{ "X-Tyk-Key": "tyk-value", "X-Other-Key": "other-value", "X-Tyk-Test": "value-from-rewrite-config", }, }, }...) Suggestion importance[1-10]: 8 Why: The suggestion is valid and enhances the test coverage by ensuring that the header is added when it is defined in the rewrite rules with `Remove` set to false. This is a crucial test case to verify the correct behavior of the header rewrite functionality.	8

…aders_rewrite

sonarqubecloud · 2024-05-28T08:39:45Z

Quality Gate failed

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

### **User description** See https://tyktech.atlassian.net/browse/TT-11997 for details of the request headers rewrite rules. ___ ### **PR Type** Enhancement, Tests ___ ### **Description** - Added tests for request headers rewrite rules in GraphQL middleware. - Integrated request headers rewrite logic into the reverse proxy handler. - Added `RequestHeadersRewrite` to `ReverseProxyParams` struct. - Passed `RequestHeadersRewrite` to the GraphQL engine transport for both v1 and v2. - Implemented logic to apply request headers rewrite rules in the GraphQL engine transport, covering three different rewrite scenarios. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Tests </strong></td><td><table> <tr> <td> <details> <summary><strong>mw_graphql_test.go</strong><dd><code>Add tests for request headers rewrite rules in GraphQL middleware.</code></dd></summary> <hr> gateway/mw_graphql_test.go <li>Added tests for request headers rewrite rules.<br> <li> Implemented three test cases for different rewrite scenarios.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-72c11b8efcaceccfbbd87e75565353706443bf56420d3fdba6b5bf5f632f4b33">+137/-0</a>  </td> </tr> </table></td></tr><tr><td><strong>Enhancement </strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy.go</strong><dd><code>Integrate request headers rewrite logic in reverse proxy handler.</code></dd></summary> <hr> gateway/reverse_proxy.go <li>Integrated request headers rewrite logic into the reverse proxy <br>handler.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+8/-7</a>      </td> </tr> <tr> <td> <details> <summary><strong>engine.go</strong><dd><code>Add RequestHeadersRewrite to ReverseProxyParams struct.</code>    </dd></summary> <hr> internal/graphengine/engine.go - Added `RequestHeadersRewrite` to `ReverseProxyParams` struct. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-9d1ef4ec1b59d708ed86c7cc348b73bd0b619ef034094b5fbd4b138e11ccde6e">+8/-7</a>      </td> </tr> <tr> <td> <details> <summary><strong>graphql_go_tools_v1.go</strong><dd><code>Pass RequestHeadersRewrite to GraphQL engine transport (v1).</code></dd></summary> <hr> internal/graphengine/graphql_go_tools_v1.go - Passed `RequestHeadersRewrite` to the GraphQL engine transport. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-e592cc8ca6ac39e7574765d7f2bbf19193f173791a1b0930d4dde7f9412dc882">+1/-0</a>      </td> </tr> <tr> <td> <details> <summary><strong>graphql_go_tools_v2.go</strong><dd><code>Pass RequestHeadersRewrite to GraphQL engine transport (v2).</code></dd></summary> <hr> internal/graphengine/graphql_go_tools_v2.go - Passed `RequestHeadersRewrite` to the GraphQL engine transport. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-2e41b60f046c2947fbedb8fb841b5c3c962798e3ba8211c7144f326436ffabe3">+1/-0</a>      </td> </tr> <tr> <td> <details> <summary><strong>transport.go</strong><dd><code>Implement request headers rewrite logic in GraphQL engine transport.</code></dd></summary> <hr> internal/graphengine/transport.go <li>Added logic to apply request headers rewrite rules in the GraphQL <br>engine transport.<br> <li> Implemented three rewrite rules for request headers.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6306/files#diff-564061c9b29366529eb1f6f10fe39671d2ac738a4731ffd2c8b04dcc0a8cd610">+96/-1</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

[TT-11997] Backend logic for request_headers_rewrite

46b54fa

buraksezer marked this pull request as ready for review May 23, 2024 13:32

buraksezer requested review from kofoworola and pvormste May 23, 2024 13:33

buraksezer requested a review from rhianeKobar May 23, 2024 13:33

github-actions Bot added enhancement Tests labels May 23, 2024

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

pvormste approved these changes May 27, 2024

View reviewed changes

Merge branch 'master' into feat/TT-11997/Backend-logic-for-request_he…

6feea63

…aders_rewrite

buraksezer enabled auto-merge (squash) May 28, 2024 08:46

buraksezer merged commit 00a815f into master May 28, 2024

buraksezer deleted the feat/TT-11997/Backend-logic-for-request_headers_rewrite branch May 28, 2024 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-11997] Backend logic for request_headers_rewrite#6306

[TT-11997] Backend logic for request_headers_rewrite#6306
buraksezer merged 2 commits into
masterfrom
feat/TT-11997/Backend-logic-for-request_headers_rewrite

buraksezer commented May 23, 2024 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

buraksezer commented May 23, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

github-actions Bot commented May 23, 2024

Uh oh!

github-actions Bot commented May 23, 2024

PR Review 🔍

Uh oh!

github-actions Bot commented May 23, 2024

PR Code Suggestions ✨

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

buraksezer commented May 23, 2024 •

edited by github-actions Bot

Loading