Merging to release-5.2: [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

• Introduces enhancements for GraphQL analytics recording, including a new test to verify correct behavior.
• Refactors GraphQL proxy handling to use a new context values structure, improving code readability and maintainability.
• Adds support for OpenTelemetry in GraphQL proxy handling, including a specific test to ensure headers are correctly passed.
• Implements a new version of the GraphQL engine with OpenTelemetry configuration, supporting both proxy only and multi-upstream transport types.
• Adds utility functions for common GraphQL engine operations, facilitating the creation of execution engines, request processing, and complexity checking.
• Modifies a tracing test scenario to use POST method and include a GraphQL query, ensuring accurate testing of GraphQL functionalities.

---

Changes walkthrough

	Relevant files
Enhancement	handler_success_test.go Add Test for GraphQL Analytics Recording                                  gateway/handler_success_test.go Adds new test TestAnalyticRecord_GraphStats to verify GraphQL analytics recording. Modifies existing tests to accommodate new changes. +131/-0  mw_graphql_transport.go Refactor GraphQL Proxy Only Handling                                          gateway/mw_graphql_transport.go Refactors handleProxyOnly to use GraphQLProxyOnlyContextValues. Updates setProxyOnlyHeaders to work with the new context values structure. +10/-10  reverse_proxy_test.go Add Test for GraphQL Proxy with OpenTelemetry                        gateway/reverse_proxy_test.go Adds new test TestGraphQL_ProxyOnlyPassHeadersWithOTel to ensure headers pass with OpenTelemetry. Adjusts existing tests for new GraphQL handling. +38/-0    context.go Implement Context Management for GraphQL Engine                    internal/graphengine/context.go Introduces new context management for GraphQL engine transport types. Adds utility functions for setting and getting proxy only context values. +80/-0    engine_v2.go Implement New Version of GraphQL Engine with OpenTelemetry Support internal/graphengine/engine_v2.go Implements a new version of the GraphQL engine with OpenTelemetry configuration. Adds handling for proxy only and multi-upstream transport types. +391/-0  graphql_go_tools_v1.go Add Utilities for GraphQL Engine Operations                            internal/graphengine/graphql_go_tools_v1.go Adds utility functions for GraphQL engine operations. Implements execution engine creation, request processing, and complexity checking. +528/-0
Tests	tyk_test-graphql-tracing-invalid_404.yml Modify Tracing Test Scenario for GraphQL                                  ci/tests/tracing/scenarios/tyk_test-graphql-tracing-invalid_404.yml Modifies HTTP method from GET to POST for a tracing test scenario. Updates request body to include a GraphQL query. +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 (4a6748b)

[github-actions]

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

[github-actions]

PR Review

⏱️ Estimated effort to review [1-5]	4, because the PR introduces significant changes across multiple files, including new tests, modifications to existing logic, and the introduction of new context management for GraphQL proxy handling. The complexity of the changes, especially around the GraphQL proxy handling and telemetry, requires careful review to ensure correctness, performance, and maintainability.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: Merge conflict markers present in 'gateway/handler_success_test.go'. This indicates that a merge conflict was not resolved properly before committing the changes.
	Performance Concern: The introduction of new context management for GraphQL proxy handling in 'internal/graphengine/context.go' and its usage in 'gateway/mw_graphql_transport.go' could potentially impact performance. It's important to ensure that the context manipulation is efficient and does not introduce unnecessary overhead.
	Code Clarity: The changes introduce complex logic for handling GraphQL requests, especially with the introduction of OpenTelemetry tracing in 'internal/graphengine/engine_v2.go'. It might be beneficial to simplify this logic or add more comments to improve readability and maintainability.
🔒 Security concerns	No

Code feedback:

---

relevant file	gateway/handler_success_test.go
suggestion	Resolve the merge conflict markers present in the file. This is critical to ensure that the intended changes are correctly applied and that the code compiles without errors. [important]
relevant line	<<<<<<< HEAD

---

relevant file	internal/graphengine/context.go
suggestion	Consider benchmarking the context manipulation logic introduced for GraphQL proxy handling to ensure it does not introduce significant performance overhead. Optimizing context usage could be crucial for high-throughput scenarios. [important]
relevant line	func SetProxyOnlyContextValue(ctx context.Context, req *http.Request) context.Context {

---

relevant file	internal/graphengine/engine_v2.go
suggestion	Simplify the logic for handling OpenTelemetry tracing or add detailed comments explaining the steps involved. This will improve code readability and maintainability, making it easier for future contributors to understand and modify the tracing logic. [medium]
relevant line	if engineV2.OpenTelemetry.Enabled {

---

relevant file	gateway/mw_graphql_transport.go
suggestion	Ensure that the new context values used for GraphQL proxy handling are properly documented and tested, especially for edge cases. This will help prevent potential bugs related to context management. [medium]
relevant line	val := GetProxyOnlyContextValue(request.Context())

---

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review. 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=...

Utilizing extra instructions The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize. Examples for extra instructions: [pr_reviewer] # /review # extra_instructions=""" In the 'possible issues' section, emphasize the following: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

How to enable\disable automation When you first install PR-Agent app, the default mode for the review tool is: pr_commands = ["/review", ...] meaning the review tool will run automatically on every PR, with the default configuration. Edit this field to enable/disable the tool, or to change the used configurations

Auto-labels The review tool can auto-generate two specific types of labels for a PR: a possible security issue label, that detects possible security issues (enable_review_labels_security flag) a Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)

Extra sub-tools The review tool provides a collection of possible feedbacks about a PR. It is recommended to review the possible options, and choose the ones relevant for your use case. Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example: require_score_review, require_soc2_ticket, require_can_be_split_review, and more.

Auto-approve PRs By invoking: /review auto_approve The tool will automatically approve the PR, and add a comment with the approval. To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following: [pr_reviewer] enable_auto_approval = true (this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository) You can also enable auto-approval only if the PR meets certain requirements, such as that the estimated_review_effort is equal or below a certain threshold, by adjusting the flag: [pr_reviewer] maximal_review_effort = 5

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

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

[github-actions]

PR Code Suggestions

Category	Suggestions
Possible issue	Remove merge conflict markers and resolve the conflict. It seems there's a merge conflict marker left in the code. Please resolve the merge conflict by choosing the appropriate code block or merging the changes from both blocks if necessary. gateway/handler_success_test.go [116-244] -+<<<<<<< HEAD -+======= -+>>>>>>> 6ab2b5693... [TT-10909]: fix issue with missing upstream headers in graphql proxy only (#6166) ++// Correctly merged or chosen code block here
	Check for nil before dereferencing upstreamResponse to avoid runtime panics. In the returnErrorsFromUpstream function, consider checking if proxyOnlyCtx.upstreamResponse is nil before accessing its Body to avoid potential nil pointer dereference. internal/graphengine/graphql_go_tools_v1.go [91] +if proxyOnlyCtx.upstreamResponse == nil { + return errors.New("upstream response is nil") +} body, err := seekReadCloser(proxyOnlyCtx.upstreamResponse.Body)
Best practice	Add nil check for proxyOnlyValues to prevent potential nil pointer dereference. Consider checking if proxyOnlyValues is not nil before accessing its fields in setProxyOnlyHeaders to avoid potential nil pointer dereference. gateway/mw_graphql_transport.go [117-118] -+ for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyValues.forwardedRequest.Header { -+ if proxyOnlyValues.ignoreForwardedHeaders[forwardedHeaderKey] { ++ if proxyOnlyValues != nil { ++ for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyValues.forwardedRequest.Header { ++ if proxyOnlyValues.ignoreForwardedHeaders[forwardedHeaderKey] {
	Use a more specific context key type to avoid potential collisions. Consider using a more specific context key type instead of the generic struct{} to avoid potential collisions with other context values in the future. internal/graphengine/context.go [17-19] -+type contextKey struct{} ++type contextKey string + -+var graphqlProxyContextInfo = contextKey{} ++var graphqlProxyContextInfo = contextKey("graphqlProxyContextInfo")
	Add nil check for forwardedRequest to prevent potential nil pointer dereference. Ensure that proxyOnlyValues.forwardedRequest is not nil before iterating over its headers in setProxyOnlyHeaders to prevent a nil pointer dereference. gateway/mw_graphql_transport.go [117] -+ for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyValues.forwardedRequest.Header { ++ if proxyOnlyValues.forwardedRequest != nil { ++ for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyValues.forwardedRequest.Header {
	Use http.NewRequestWithContext for proper context management. Consider using http.NewRequestWithContext instead of r.WithContext to ensure that the request context is correctly initialized with the context containing the span. This approach is more idiomatic and ensures that the context's lifecycle is correctly managed from the creation of the request. internal/graphengine/engine_v2.go [218-220] ctx, span := e.OpenTelemetry.TracerProvider.Tracer().Start(r.Context(), "GraphqlMiddleware Validation") defer span.End() -*r = *r.WithContext(ctx) +r = r.Clone(ctx)
	Use early return for error handling to improve readability. To improve error handling and readability, consider returning early when an error is encountered instead of using an if statement. This approach reduces the nesting level and makes the code easier to understand. internal/graphengine/engine_v2.go [171-184] if err != nil { options.Logger.WithError(err).Error("error creating custom execution engine v2") cancel() return nil, err } +otelRequestProcessor := &graphqlRequestProcessorWithOTelV1{ + logger: logger, + schema: parsedSchema, + otelExecutor: executor, + ctxRetrieveRequest: options.Injections.ContextRetrieveRequest, +} +engineV2.graphqlRequestProcessor = otelRequestProcessor +engineV2.OpenTelemetry.Executor = executor
	Use defer for resource cleanup to ensure proper resource release. To ensure that resources are properly released and to improve the readability of the code, consider using a defer statement to close channels and handle cleanup operations. This approach ensures that resources are released in case of errors or early returns. internal/graphengine/engine_v2.go [373-385] -go gqlwebsocket.Handle( - done, - errChan, - conn, - executorPool, - gqlwebsocket.WithLogger(e.logger), - gqlwebsocket.WithProtocolFromRequestHeaders(params.OutRequest), -) +go func() { + defer close(done) + defer close(errChan) + gqlwebsocket.Handle( + done, + errChan, + conn, + executorPool, + gqlwebsocket.WithLogger(e.logger), + gqlwebsocket.WithProtocolFromRequestHeaders(params.OutRequest), + ) +}() select { case err := <-errChan: e.logger.Error("could not start graphql websocket handler: ", abstractlogger.Error(err)) case <-done: }
	Handle the error returned by json.Marshal to ensure safe execution. Consider handling the error returned by json.Marshal in the createExecutionEngine function to avoid potential runtime panics or incorrect behavior if marshaling fails. internal/graphengine/graphql_go_tools_v1.go [122-123] -res, _ := json.Marshal(datasource.SchemaDataSourcePlannerConfig{}) +res, err := json.Marshal(datasource.SchemaDataSourcePlannerConfig{}) +if err != nil { + return nil, fmt.Errorf("failed to marshal SchemaDataSourcePlannerConfig: %w", err) +}
Enhancement	Use require instead of assert for critical test conditions. To ensure the test fails for the right reason, consider using require instead of assert for critical test conditions that must pass for the test to continue meaningfully. gateway/reverse_proxy_test.go [1022-1023] + 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) ++ require.Equal(t, "custom-value", gotten, "expected upstream to receive header `custom-client-header` with value of `custom-value`")
	Use more specific error types for better error context. Consider using a more specific error type than errors.New for ProxyingRequestFailedErr to provide more context about the error, especially in the ProcessRequest methods of graphqlRequestProcessorV1 and graphqlRequestProcessorWithOTelV1. internal/graphengine/graphql_go_tools_v1.go [204-205] -return ProxyingRequestFailedErr, http.StatusInternalServerError +return fmt.Errorf("proxying request failed: %w", someMoreSpecificError), http.StatusInternalServerError
Maintainability	Extract logic into separate functions for better maintainability. To enhance the maintainability and readability of the code, consider extracting the logic for handling different reverseProxyType cases into separate functions. This modular approach makes the code easier to understand and test. internal/graphengine/engine_v2.go [243-252] -switch reverseProxyType { -case ReverseProxyTypeIntrospection: - return e.gqlTools.handleIntrospection(e.Schema) -case ReverseProxyTypeWebsocketUpgrade: - return e.handoverWebSocketConnectionToGraphQLExecutionEngine(&params) -case ReverseProxyTypeGraphEngine: - return e.handoverRequestToGraphQLExecutionEngine(gqlRequest, params.OutRequest) -case ReverseProxyTypeNone: - return nil, false, nil +return e.handleReverseProxyType(reverseProxyType, params, gqlRequest) +// New function to handle the logic +func (e *EngineV2) handleReverseProxyType(reverseProxyType ReverseProxyType, params ReverseProxyParams, gqlRequest *graphql.Request) (res *http.Response, hijacked bool, err error) { + switch reverseProxyType { + case ReverseProxyTypeIntrospection: + return e.gqlTools.handleIntrospection(e.Schema) + case ReverseProxyTypeWebsocketUpgrade: + return e.handoverWebSocketConnectionToGraphQLExecutionEngine(&params) + case ReverseProxyTypeGraphEngine: + return e.handoverRequestToGraphQLExecutionEngine(gqlRequest, params.OutRequest) + case ReverseProxyTypeNone: + return nil, false, nil + } + e.logger.Error("unknown reverse proxy type", abstractlogger.Int("reverseProxyType", int(reverseProxyType))) + return nil, false, ErrUnknownReverseProxyType }
	Use specific error messages for different error contexts. Avoid using a generic error message in multiple places. Instead, use specific error messages to make debugging easier. This suggestion applies to the error handling in the createExecutionEngine function. internal/graphengine/graphql_go_tools_v1.go [161] -errMsgFormat := "%s couldn't be added" +errMsgFormatHTTPJSON := "HTTPJSONDataSource couldn't be added: %w" +errMsgFormatGraphQL := "GraphQLDataSource couldn't be added: %w" +errMsgFormatSchema := "SchemaDataSource couldn't be added: %w"
	Simplify the return statement in depthLimitEnabled for better readability. In the depthLimitEnabled method of the complexityCheckerV1 struct, simplify the return statement to improve readability. internal/graphengine/graphql_go_tools_v1.go [389-397] -if accessDefinition.Limit.MaxQueryDepth == -1 && len(accessDefinition.FieldAccessRights) == 0 { - return false -} -return accessDefinition.Limit.MaxQueryDepth != -1 || len(accessDefinition.FieldAccessRights) != 0 +return !(accessDefinition.Limit.MaxQueryDepth == -1 && len(accessDefinition.FieldAccessRights) == 0)
Performance	Optimize string operations for performance. To improve the performance and reduce the overhead of string operations, consider using a more efficient method for checking if the acceptedEncoding contains only one value or if it's a list. This can be achieved by using strings.Index to check for the presence of a comma, which is more efficient than splitting the string. internal/graphengine/engine_v2.go [332-334] +if !strings.Contains(acceptedEncoding, ",") { + return acceptedEncoding +} values := strings.Split(acceptedEncoding, ",") -if len(values) < 2 { - return values[0] -}

---

✨ 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=...

Enabling\disabling automation When you first install the app, the default mode for the improve tool is: pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...] meaning the improve tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.

Utilizing extra instructions Extra instructions are very important for the improve tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project. Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on. Examples for extra instructions: [pr_code_suggestions] # /improve # extra_instructions=""" Emphasize the following aspects: - Does the code logic cover relevant edge cases? - Is the code logic clear and easy to understand? - Is the code logic efficient? ... """ Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.

A note on code suggestions quality While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically. Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base. Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions 💎 tool With large PRs, best quality will be obtained by using 'improve --extended' mode.

More PR-Agent commands To invoke the PR-Agent, add a comment using one of the following commands: /review: Request a review of your Pull Request. /describe: Update the PR title and description based on the contents of the PR. /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback. /ask <QUESTION>: Ask a question about the PR. /update_changelog: Update the changelog based on the PR's contents. /add_docs 💎: Generate docstring for new components introduced in the PR. /generate_labels 💎: Generate labels for the PR based on the PR's contents. /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component. See the tools guide for more details. To list the possible configuration parameters, add a /config comment.

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

[buger]

API tests result: failure 🚫
Branch used: refs/pull/6171/merge
Commit:
Triggered by: pull_request (@buger)
Execution page