From dd9d2e32a0783e49f1e7beddd90e6a82bf3f21a0 Mon Sep 17 00:00:00 2001 From: Kofo Okesola Date: Thu, 21 Mar 2024 16:57:32 +0100 Subject: [PATCH] [TT-11683]: fixed header forwarding (#6174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ticket adds the header forwarding logic fix from [this PR](https://github.com/TykTechnologies/tyk/pull/6166) to 5-lts [TT-11683](https://tyktech.atlassian.net/browse/TT-11683) - [ ] 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) - [ ] 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-11683]: https://tyktech.atlassian.net/browse/TT-11683?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ ___ bug_fix, enhancement ___ - Enhanced GraphQL proxy handling with the introduction of `GraphQLProxyOnlyContextValues` for better context management. - Added a new function `selectContentEncodingToBeUsed` to determine the appropriate content encoding based on the `Accept-Encoding` header. - Improved header forwarding in GraphQL proxy-only mode, including a new test case to validate this behavior. - Updated a tracing test scenario for GraphQL to use the POST method. ___
Relevant files
Enhancement
reverse_proxy.go
Enhancements and Fixes in GraphQL Proxy Handling                 
gateway/reverse_proxy.go
  • Imported httpclient from graphql-go-tools for handling HTTP client
    operations.
  • Modified returnErrorsFromUpstream to use GraphQLProxyOnlyContextValues
    instead of GraphQLProxyOnlyContext.
  • Added selectContentEncodingToBeUsed function to select the appropriate
    content encoding based on the Accept-Encoding header.
  • Updated various functions to utilize the new
    GraphQLProxyOnlyContextValues structure for better header management.
    		• +35/-3   
    mw_graphql_transport.go
    Improved Context Management in GraphQL Transport Middleware
    gateway/mw_graphql_transport.go
  • Introduced GraphQLProxyOnlyContextValues struct for better management
    of proxy-only context values.
  • Added functions SetProxyOnlyContextValue and GetProxyOnlyContextValue
    for setting and retrieving proxy-only context values.
  • Updated handleProxyOnly and setProxyOnlyHeaders to use the new context
    values structure.
    		• +36/-5   
    Tests
    reverse_proxy_test.go
    New Test for Header Forwarding in GraphQL Proxy-Only Mode
    gateway/reverse_proxy_test.go
  • Added a new test TestGraphQL_ProxyOnlyPassHeadersWithOTel to ensure
    headers are correctly passed in proxy-only mode with OpenTelemetry
    enabled.
    		• +38/-0   
    tyk_test-graphql-tracing-invalid_404.yml
    Update Tracing Test Scenario for GraphQL                                 
    ci/tests/tracing/scenarios/tyk_test-graphql-tracing-invalid_404.yml
  • Changed the HTTP method from GET to POST in the tracing test scenario
    for GraphQL.
    		• +1/-1     
    ___ > ✨ **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --- gateway/mw_graphql_transport.go | 41 ++++++++++++++++++++++++---- gateway/mw_graphql_transport_test.go | 2 +- gateway/reverse_proxy.go | 39 ++++++++++++++++++++++++-- gateway/reverse_proxy_test.go | 38 ++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 9 deletions(-) diff --git a/gateway/mw_graphql_transport.go b/gateway/mw_graphql_transport.go index aa00f2c709f3..b93ab347c511 100644 --- a/gateway/mw_graphql_transport.go +++ b/gateway/mw_graphql_transport.go @@ -27,6 +27,37 @@ func DetermineGraphQLEngineTransportType(apiSpec *APISpec) GraphQLEngineTranspor return GraphQLEngineTransportTypeMultiUpstream } +type contextKey struct{} + +var graphqlProxyContextInfo = contextKey{} + +type GraphQLProxyOnlyContextValues struct { + forwardedRequest *http.Request + upstreamResponse *http.Response + ignoreForwardedHeaders map[string]bool +} + +func SetProxyOnlyContextValue(ctx context.Context, req *http.Request) context.Context { + value := &GraphQLProxyOnlyContextValues{ + forwardedRequest: req, + ignoreForwardedHeaders: map[string]bool{ + http.CanonicalHeaderKey("date"): true, + http.CanonicalHeaderKey("content-type"): true, + http.CanonicalHeaderKey("content-length"): true, + }, + } + + return context.WithValue(ctx, graphqlProxyContextInfo, value) +} + +func GetProxyOnlyContextValue(ctx context.Context) *GraphQLProxyOnlyContextValues { + val, ok := ctx.Value(graphqlProxyContextInfo).(*GraphQLProxyOnlyContextValues) + if !ok { + return nil + } + return val +} + type GraphQLProxyOnlyContext struct { context.Context forwardedRequest *http.Request @@ -65,16 +96,16 @@ func NewGraphQLEngineTransport(transportType GraphQLEngineTransportType, origina func (g *GraphQLEngineTransport) RoundTrip(request *http.Request) (res *http.Response, err error) { switch g.transportType { case GraphQLEngineTransportTypeProxyOnly: - proxyOnlyCtx, ok := request.Context().(*GraphQLProxyOnlyContext) - if ok { - return g.handleProxyOnly(proxyOnlyCtx, request) + val := GetProxyOnlyContextValue(request.Context()) + if val != nil { + return g.handleProxyOnly(val, request) } } return g.originalTransport.RoundTrip(request) } -func (g *GraphQLEngineTransport) handleProxyOnly(proxyOnlyCtx *GraphQLProxyOnlyContext, request *http.Request) (*http.Response, error) { +func (g *GraphQLEngineTransport) handleProxyOnly(proxyOnlyCtx *GraphQLProxyOnlyContextValues, request *http.Request) (*http.Response, error) { request.Method = proxyOnlyCtx.forwardedRequest.Method g.setProxyOnlyHeaders(proxyOnlyCtx, request) @@ -107,7 +138,7 @@ func (g *GraphQLEngineTransport) handleProxyOnly(proxyOnlyCtx *GraphQLProxyOnlyC return response, err } -func (g *GraphQLEngineTransport) setProxyOnlyHeaders(proxyOnlyCtx *GraphQLProxyOnlyContext, r *http.Request) { +func (g *GraphQLEngineTransport) setProxyOnlyHeaders(proxyOnlyCtx *GraphQLProxyOnlyContextValues, r *http.Request) { for forwardedHeaderKey, forwardedHeaderValues := range proxyOnlyCtx.forwardedRequest.Header { if proxyOnlyCtx.ignoreForwardedHeaders[forwardedHeaderKey] { continue diff --git a/gateway/mw_graphql_transport_test.go b/gateway/mw_graphql_transport_test.go index f7f1db91f8cc..54dc79fcf276 100644 --- a/gateway/mw_graphql_transport_test.go +++ b/gateway/mw_graphql_transport_test.go @@ -121,7 +121,7 @@ func TestGraphQLEngineTransport_RoundTrip(t *testing.T) { forwardedRequest.Header.Set("X-Custom-Key", "custom-value") forwardedRequest.Header.Set("X-Other-Value", "other-value") - ctx := NewGraphQLProxyOnlyContext(context.Background(), forwardedRequest) + ctx := SetProxyOnlyContextValue(context.Background(), forwardedRequest) httpClient := http.Client{ Transport: NewGraphQLEngineTransport(GraphQLEngineTransportTypeProxyOnly, http.DefaultTransport), diff --git a/gateway/reverse_proxy.go b/gateway/reverse_proxy.go index b5ea7ec1fccf..725a9ad62318 100644 --- a/gateway/reverse_proxy.go +++ b/gateway/reverse_proxy.go @@ -28,6 +28,8 @@ import ( "sync" "time" + "github.com/TykTechnologies/graphql-go-tools/pkg/engine/datasource/httpclient" + "github.com/buger/jsonparser" "github.com/gorilla/websocket" @@ -1077,7 +1079,7 @@ func (p *ReverseProxy) handleGraphQLEngineWebsocketUpgrade(roundTripper *TykRoun return nil, true, nil } -func returnErrorsFromUpstream(proxyOnlyCtx *GraphQLProxyOnlyContext, resultWriter *graphql.EngineResultWriter) error { +func returnErrorsFromUpstream(proxyOnlyCtx *GraphQLProxyOnlyContextValues, resultWriter *graphql.EngineResultWriter) error { body, ok := proxyOnlyCtx.upstreamResponse.Body.(*nopCloserBuffer) if !ok { // Response body already read by graphql-go-tools, and it's not re-readable. Quit silently. @@ -1132,7 +1134,7 @@ func (p *ReverseProxy) handoverRequestToGraphQLExecutionEngine(roundTripper *Tyk isProxyOnly := isGraphQLProxyOnly(p.TykAPISpec) reqCtx := context.Background() if isProxyOnly { - reqCtx = NewGraphQLProxyOnlyContext(context.Background(), outreq) + reqCtx = SetProxyOnlyContextValue(reqCtx, outreq) } resultWriter := graphql.NewEngineResultWriter() @@ -1154,7 +1156,7 @@ func (p *ReverseProxy) handoverRequestToGraphQLExecutionEngine(roundTripper *Tyk header.Set("Content-Type", "application/json") if isProxyOnly { - proxyOnlyCtx := reqCtx.(*GraphQLProxyOnlyContext) + proxyOnlyCtx := GetProxyOnlyContextValue(reqCtx) // There is a case in the proxy-only mode where the request can be handled // by the library without calling the upstream. // This is a valid query for proxy-only mode: query { __typename } @@ -1163,6 +1165,9 @@ func (p *ReverseProxy) handoverRequestToGraphQLExecutionEngine(roundTripper *Tyk if proxyOnlyCtx.upstreamResponse != nil { header = proxyOnlyCtx.upstreamResponse.Header httpStatus = proxyOnlyCtx.upstreamResponse.StatusCode + // change the value of the header's content encoding to use the content encoding defined by the accept encoding + contentEncoding := selectContentEncodingToBeUsed(proxyOnlyCtx.forwardedRequest.Header.Get(httpclient.AcceptEncodingHeader)) + header.Set(httpclient.ContentEncodingHeader, contentEncoding) if p.TykAPISpec.GraphQL.Proxy.UseResponseExtensions.OnErrorForwarding && httpStatus >= http.StatusBadRequest { err = returnErrorsFromUpstream(proxyOnlyCtx, &resultWriter) if err != nil { @@ -1179,6 +1184,34 @@ func (p *ReverseProxy) handoverRequestToGraphQLExecutionEngine(roundTripper *Tyk return nil, false, errors.New("graphql configuration is invalid") } +// selectContentEncodingToBeUsed selects the encoding value to be returned based on the IETF standards +// if acceptedEncoding is a list of comma separated strings br,gzip, deflate; then it selects the first supported one +// if it is a single value then it returns that value +// if no supported encoding is found, it returns the last value +func selectContentEncodingToBeUsed(acceptedEncoding string) string { + supportedHeaders := map[string]struct{}{ + "gzip": {}, + "deflate": {}, + "br": {}, + } + + values := strings.Split(acceptedEncoding, ",") + if len(values) < 2 { + return values[0] + } + + for i, e := range values { + enc := strings.TrimSpace(e) + if _, ok := supportedHeaders[enc]; ok { + return enc + } + if i == len(values)-1 { + return enc + } + } + return "" +} + func (p *ReverseProxy) handoverWebSocketConnectionToGraphQLExecutionEngine(roundTripper *TykRoundTripper, conn net.Conn, req *http.Request) { p.TykAPISpec.GraphQLExecutor.Client.Transport = NewGraphQLEngineTransport(DetermineGraphQLEngineTransportType(p.TykAPISpec), roundTripper) diff --git a/gateway/reverse_proxy_test.go b/gateway/reverse_proxy_test.go index ff61bc1dd7ca..b633fffbba99 100644 --- a/gateway/reverse_proxy_test.go +++ b/gateway/reverse_proxy_test.go @@ -984,6 +984,44 @@ func TestGraphQL_ProxyOnlyHeaders(t *testing.T) { }) } +func TestGraphQL_ProxyOnlyPassHeadersWithOTel(t *testing.T) { + g := StartTest(func(globalConf *config.Config) { + globalConf.OpenTelemetry.Enabled = true + }) + defer g.Close() + + spec := BuildAPI(func(spec *APISpec) { + spec.Name = "tyk-api" + spec.APIID = "tyk-api" + spec.GraphQL.Enabled = true + spec.GraphQL.ExecutionMode = apidef.GraphQLExecutionModeProxyOnly + spec.GraphQL.Schema = gqlCountriesSchema + spec.GraphQL.Version = apidef.GraphQLConfigVersion2 + spec.Proxy.TargetURL = TestHttpAny + "/dynamic" + spec.Proxy.ListenPath = "/" + })[0] + + g.Gw.LoadAPI(spec) + g.AddDynamicHandler("/dynamic", func(writer http.ResponseWriter, r *http.Request) { + 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) + } + }) + + _, err := g.Run(t, test.TestCase{ + Path: "/", + Headers: map[string]string{ + "custom-client-header": "custom-value", + }, + Method: http.MethodPost, + Data: graphql.Request{ + Query: gqlContinentQuery, + }, + }) + + assert.NoError(t, err) +} + func TestGraphQL_InternalDataSource(t *testing.T) { g := StartTest(nil) defer g.Close()