-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merging to release-5.3.0: [TT-10909]: fix issue with missing upstream headers in graphql proxy only (#6166) #6187
base: release-5.3.0
Are you sure you want to change the base?
Conversation
…only (#6166) ## **User description** <!-- Provide a general summary of your changes in the Title above --> ## Description This PR fixes an issue where the requests from the client were not sent upstream. This was due to an edge case cause by the open telemetry context modification [TT-10909](https://tyktech.atlassian.net/browse/TT-10909) This PR also fixes a situation where the requested content-encoding by the client is ignored <!-- Describe your changes in detail --> ## Related Issue <!-- This project only accepts pull requests related to open issues. --> <!-- If suggesting a new feature or change, please discuss it in an issue first. --> <!-- If fixing a bug, there should be an issue describing it with steps to reproduce. --> <!-- OSS: Please link to the issue here. Tyk: please create/link the JIRA ticket. --> ## Motivation and Context <!-- Why is this change required? What problem does it solve? --> ## How This Has Been Tested <!-- Please describe in detail how you tested your changes --> <!-- Include details of your testing environment, and the tests --> <!-- you ran to see how your change affects other areas of the code, etc. --> <!-- This information is helpful for reviewers and QA. --> ## Screenshots (if appropriate) ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] 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 <!-- Go over all the following points, and put an `x` in all the boxes that apply --> <!-- If there are no documentation updates required, mark the item as checked. --> <!-- Raise up any additional concerns not covered by the 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 [TT-10909]: https://tyktech.atlassian.net/browse/TT-10909?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ ___ ## **Type** bug_fix, enhancement ___ ## **Description** - Added a new test to ensure headers are correctly passed to the upstream when OpenTelemetry is enabled. - Introduced a new context management strategy for GraphQL proxy-only mode to correctly forward headers. - Updated various components within the internal GraphQL engine to utilize the new context structure for header forwarding. ___ ## **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>reverse_proxy_test.go</strong><dd><code>Add Test for GraphQL Proxy with OpenTelemetry</code> </dd></summary> <hr> gateway/reverse_proxy_test.go <li>Added a new test <code>TestGraphQL_ProxyOnlyPassHeadersWithOTel</code> to ensure <br>headers are passed upstream when OpenTelemetry is enabled. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6166/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3f">+38/-0</a> </td> </tr> </table></td></tr><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>context.go</strong><dd><code>Manage GraphQL Proxy Context for Headers Forwarding</code> </dd></summary> <hr> internal/graphengine/context.go <li>Introduced <code>GraphQLProxyOnlyContextValues</code> struct to store request and <br>response details.<br> <li> Added functions <code>SetProxyOnlyContextValue</code> and <code>GetProxyOnlyContextValue</code> <br>for managing GraphQL proxy context. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6166/files#diff-59c1392237cf0565e56acd0f46f7500043ec66fff078bf211ceefbb983baaf94">+31/-0</a> </td> </tr> <tr> <td> <details> <summary><strong>engine_v2.go</strong><dd><code>Integrate New Context Management in Engine V2</code> </dd></summary> <hr> internal/graphengine/engine_v2.go <li>Modified to use <code>SetProxyOnlyContextValue</code> for setting proxy context.<br> <li> Updated to retrieve proxy context using <code>GetProxyOnlyContextValue</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6166/files#diff-b1eaa954c9836f395e1d49090e85c739e3878747c8bd748f556fc5a53ff7b191">+2/-2</a> </td> </tr> <tr> <td> <details> <summary><strong>graphql_go_tools_v1.go</strong><dd><code>Update Error Handling with New Context Structure</code> </dd></summary> <hr> internal/graphengine/graphql_go_tools_v1.go <li>Updated <code>returnErrorsFromUpstream</code> to use <code>GraphQLProxyOnlyContextValues</code>. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6166/files#diff-e592cc8ca6ac39e7574765d7f2bbf19193f173791a1b0930d4dde7f9412dc882">+1/-1</a> </td> </tr> <tr> <td> <details> <summary><strong>transport.go</strong><dd><code>Refactor Transport Logic for GraphQL Proxy</code> </dd></summary> <hr> internal/graphengine/transport.go <li>Refactored to use <code>GraphQLProxyOnlyContextValues</code> for handling <br>proxy-only requests.<br> <li> Adjusted header forwarding logic to accommodate new context structure. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6166/files#diff-564061c9b29366529eb1f6f10fe39671d2ac738a4731ffd2c8b04dcc0a8cd610">+10/-10</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 (cherry picked from commit 6ab2b56)
API Changes no api changes detected |
PR Description updated to latest commit (72eb16e) |
PR Review
Code feedback:
✨ Review tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions
✨ Improve tool usage guide:Overview:
See the improve usage page for a comprehensive guide on using this tool. |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
User description
TT-10909: fix issue with missing upstream headers in graphql proxy only (#6166)
User description
Description
This PR fixes an issue where the requests from the client were not sent
upstream. This was due to an edge case cause by the open telemetry
context modification
TT-10909
This PR also fixes a situation where the requested content-encoding by
the client is ignored
Related Issue
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
functionality to change)
coverage to functionality)
Checklist
why it's required
explained why
Type
bug_fix, enhancement
Description
upstream when OpenTelemetry is enabled.
mode to correctly forward headers.
utilize the new context structure for header forwarding.
Changes walkthrough
reverse_proxy_test.go
Add Test for GraphQL Proxy with OpenTelemetry
gateway/reverse_proxy_test.go
TestGraphQL_ProxyOnlyPassHeadersWithOTel
to ensureheaders are passed upstream when OpenTelemetry is enabled.
context.go
Manage GraphQL Proxy Context for Headers Forwarding
internal/graphengine/context.go
GraphQLProxyOnlyContextValues
struct to store request andresponse details.
SetProxyOnlyContextValue
andGetProxyOnlyContextValue
for managing GraphQL proxy context.
engine_v2.go
Integrate New Context Management in Engine V2
internal/graphengine/engine_v2.go
SetProxyOnlyContextValue
for setting proxy context.GetProxyOnlyContextValue
.graphql_go_tools_v1.go
Update Error Handling with New Context Structure
internal/graphengine/graphql_go_tools_v1.go
returnErrorsFromUpstream
to useGraphQLProxyOnlyContextValues
.transport.go
Refactor Transport Logic for GraphQL Proxy
internal/graphengine/transport.go
GraphQLProxyOnlyContextValues
for handlingproxy-only requests.
Type
bug_fix, enhancement
Description
GraphQLProxyOnlyContextValues
for better context handling in GraphQL proxy-only mode.Accept-Encoding
header.Changes walkthrough
handler_success_test.go
Add Headers to Test Cases for Empty Accept-Encoding
gateway/handler_success_test.go
Accept-Encoding
during tests.context.go
Add Context Handling for GraphQL Proxy-Only Mode
internal/graphengine/context.go
GraphQLProxyOnlyContextValues
struct to store request andresponse details for GraphQL proxy-only mode.
GraphQLProxyOnlyContextValues
incontext.
engine_v2.go
Enhance GraphQL Engine V2 for Proxy-Only Mode and Content Encoding
internal/graphengine/engine_v2.go
Accept-Encoding
header.
graphql_go_tools_v1.go
Adjust to Use New Context Values Structure
internal/graphengine/graphql_go_tools_v1.go
GraphQLProxyOnlyContextValues
struct.transport.go
Update Transport Logic for GraphQL Proxy-Only Mode
internal/graphengine/transport.go
reverse_proxy_test.go
Ensure Custom Headers Forwarding in Proxy-Only Mode with OpenTelemetry
gateway/reverse_proxy_test.go
TestGraphQL_ProxyOnlyPassHeadersWithOTel
to ensurecustom headers are forwarded in proxy-only mode with OpenTelemetry
enabled.
tyk_test-graphql-tracing-invalid_404.yml
Change Method to POST in Tracing Test Scenario
ci/tests/tracing/scenarios/tyk_test-graphql-tracing-invalid_404.yml