Merging to release-5.3.0: [TT-10909]: fix issue with missing upstream headers in graphql proxy only (#6166)

[buger]

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

• 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

---

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

	Relevant files
Tests	reverse_proxy_test.go Add Test for GraphQL Proxy with OpenTelemetry                        gateway/reverse_proxy_test.go Added a new test TestGraphQL_ProxyOnlyPassHeadersWithOTel to ensure headers are passed upstream when OpenTelemetry is enabled. +38/-0
Enhancement	context.go Manage GraphQL Proxy Context for Headers Forwarding            internal/graphengine/context.go Introduced GraphQLProxyOnlyContextValues struct to store request and response details. Added functions SetProxyOnlyContextValue and GetProxyOnlyContextValue for managing GraphQL proxy context. +31/-0    engine_v2.go Integrate New Context Management in Engine V2                        internal/graphengine/engine_v2.go Modified to use SetProxyOnlyContextValue for setting proxy context. Updated to retrieve proxy context using GetProxyOnlyContextValue. +2/-2      graphql_go_tools_v1.go Update Error Handling with New Context Structure                  internal/graphengine/graphql_go_tools_v1.go Updated returnErrorsFromUpstream to use GraphQLProxyOnlyContextValues. +1/-1      transport.go Refactor Transport Logic for GraphQL Proxy                              internal/graphengine/transport.go Refactored to use GraphQLProxyOnlyContextValues for handling proxy-only requests. Adjusted header forwarding logic to accommodate new context structure. +10/-10

---

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

---

Type

bug_fix, enhancement

---

Description

• Added a new test to ensure custom headers are forwarded in proxy-only mode with OpenTelemetry enabled.
• Introduced and utilized GraphQLProxyOnlyContextValues for better context handling in GraphQL proxy-only mode.
• Enhanced GraphQL engine V2 to handle content encoding based on Accept-Encoding header.
• Updated transport logic to accommodate the new context structure for GraphQL proxy-only mode.
• Adjusted tracing test scenario to use POST method.

---

Changes walkthrough

	Relevant files
Enhancement	handler_success_test.go Add Headers to Test Cases for Empty Accept-Encoding            gateway/handler_success_test.go Added headers to ensure empty Accept-Encoding during tests. +5/-0      context.go Add Context Handling for GraphQL Proxy-Only Mode                  internal/graphengine/context.go Introduced GraphQLProxyOnlyContextValues struct to store request and response details for GraphQL proxy-only mode. Added functions to set and get GraphQLProxyOnlyContextValues in context. +31/-0    engine_v2.go Enhance GraphQL Engine V2 for Proxy-Only Mode and Content Encoding internal/graphengine/engine_v2.go Modified to use the new context handling for GraphQL proxy-only mode. Added logic to select content encoding based on the Accept-Encoding header. +37/-3    graphql_go_tools_v1.go Adjust to Use New Context Values Structure                              internal/graphengine/graphql_go_tools_v1.go Adjusted to use the new GraphQLProxyOnlyContextValues struct. +1/-1      transport.go Update Transport Logic for GraphQL Proxy-Only Mode              internal/graphengine/transport.go Updated to use the new context values for GraphQL proxy-only mode. Adjusted header forwarding logic based on the new context structure. +10/-10
Bug_fix	reverse_proxy_test.go Ensure Custom Headers Forwarding in Proxy-Only Mode with OpenTelemetry gateway/reverse_proxy_test.go Added a new test TestGraphQL_ProxyOnlyPassHeadersWithOTel to ensure custom headers are forwarded in proxy-only mode with OpenTelemetry enabled. +38/-0
Tests	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 Changed method from GET to POST in the test scenario. +1/-1

---

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

[github-actions]

API Changes

no api changes detected

[github-actions]

PR Description updated to latest commit (72eb16e)

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	4, due to the complexity and the number of files involved in the PR. The changes impact several core functionalities, including handling of headers, context management, and proxy behavior. Understanding the full implications of these changes requires a deep understanding of the existing codebase and the specific issues being addressed.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: The implementation of selectContentEncodingToBeUsed in internal/graphengine/engine_v2.go assumes the first supported encoding should be chosen or the last one if none is supported. This might not align with the client's preferences if multiple encodings are supported.
	Performance Concern: The method selectContentEncodingToBeUsed iterates through all accepted encodings for every request. For high-throughput systems, this could introduce unnecessary overhead.
🔒 Security concerns	No

Code feedback:

---

relevant file	internal/graphengine/engine_v2.go
suggestion	Consider caching the result of selectContentEncodingToBeUsed for repeated Accept-Encoding values to improve performance. This can be particularly beneficial in high-throughput scenarios where the overhead of parsing and selecting encodings for each request can accumulate. [important]
relevant line	contentEncoding := selectContentEncodingToBeUsed(proxyOnlyCtx.forwardedRequest.Header.Get(httpclient.AcceptEncodingHeader))

---

relevant file	internal/graphengine/engine_v2.go
suggestion	Implement a more sophisticated algorithm for selecting the content encoding to use, taking into account the client's preferences (e.g., by considering the quality values associated with each encoding in the Accept-Encoding header). This ensures that the chosen encoding aligns more closely with the client's capabilities and preferences. [important]
relevant line	func selectContentEncodingToBeUsed(acceptedEncoding string) string {

---

relevant file	internal/graphengine/context.go
suggestion	Add documentation to the SetProxyOnlyContextValue and GetProxyOnlyContextValue functions explaining their purpose and usage within the system. This can help future developers understand the context management strategy employed for GraphQL proxy-only requests. [medium]
relevant line	func SetProxyOnlyContextValue(ctx context.Context, req *http.Request) context.Context {

---

relevant file	gateway/reverse_proxy_test.go
suggestion	Extend the TestGraphQL_ProxyOnlyPassHeadersWithOTel test to cover more edge cases, such as handling of multiple headers, absence of the custom header, and scenarios where OpenTelemetry is disabled. This ensures the robustness of the header forwarding logic under various conditions. [medium]
relevant line	func TestGraphQL_ProxyOnlyPassHeadersWithOTel(t *testing.T) {

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[github-actions]

PR Code Suggestions

Category	Suggestions
Enhancement	Validate the acceptedEncoding parameter to ensure it is not empty. Consider validating the acceptedEncoding parameter in the selectContentEncodingToBeUsed function to ensure it is not empty before proceeding with the logic. This can prevent unexpected behavior or errors when the acceptedEncoding string is empty. internal/graphengine/engine_v2.go [325] func selectContentEncodingToBeUsed(acceptedEncoding string) string { + if acceptedEncoding == "" { + return "" + }
	Use more specific assertions for header value checks in tests. Consider using a more specific assertion than assert.NoError to validate the presence and value of the custom-client-header in the request. This could provide clearer feedback in case of test failures. gateway/reverse_proxy_test.go [1017-1018] -if gotten := r.Header.Get("custom-client-header"); gotten != "custom-value" { - t.Errorf("expected upstream to recieve header `custom-client-header` with value of `custom-value`, instead got %s", gotten) -} +assert.Equal(t, "custom-value", r.Header.Get("custom-client-header"), "expected upstream to receive header `custom-client-header` with value of `custom-value`")
Performance	Optimize the loop in selectContentEncodingToBeUsed for efficiency. To improve the efficiency of header selection in selectContentEncodingToBeUsed, consider using a loop that breaks once a supported encoding is found instead of iterating through all values even after finding a match. internal/graphengine/engine_v2.go [337-344] -for i, e := range values { +for _, e := range values { enc := strings.TrimSpace(e) if _, ok := supportedHeaders[enc]; ok { return enc } - if i == len(values)-1 { - return enc - } } +return ""
Bug	Ensure all header values are copied in setProxyOnlyHeaders. In the setProxyOnlyHeaders function, ensure to copy all header values, not just the first one, from the forwarded request to the new request. This is important for headers that can have multiple values. internal/graphengine/transport.go [73-74] for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyValues.forwardedRequest.Header { - if proxyOnlyValues.ignoreForwardedHeaders[forwardedHeaderKey] { + if proxyOnlyValues.ignoreForwardedHeaders[http.CanonicalHeaderKey(forwardedHeaderKey)] { continue } + for _, value := range forwardedHeaderValues { + r.Header.Add(forwardedHeaderKey, value) + } +}
Best practice	Encapsulate header ignore list manipulation in GraphQLProxyOnlyContextValues. Consider adding a method to safely add headers to the ignoreForwardedHeaders map in GraphQLProxyOnlyContextValues. This encapsulation can prevent direct manipulation of the map and ensure that header keys are always canonicalized. internal/graphengine/context.go [29-33] -ignoreForwardedHeaders: map[string]bool{ - http.CanonicalHeaderKey("date"): true, - http.CanonicalHeaderKey("content-type"): true, - http.CanonicalHeaderKey("content-length"): true, -}, +ignoreForwardedHeaders: make(map[string]bool), +// Then, in the constructor or initialization method: +func (v *GraphQLProxyOnlyContextValues) AddIgnoreHeader(header string) { + v.ignoreForwardedHeaders[http.CanonicalHeaderKey(header)] = true +} +// Usage: +value.AddIgnoreHeader("date") +value.AddIgnoreHeader("content-type") +value.AddIgnoreHeader("content-length")

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[sonarcloud]

Quality Gate failed

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