Skip to content

Commit

Permalink
[TT-11683]: fixed header forwarding (#6174)
Browse files Browse the repository at this point in the history
<!-- Provide a general summary of your changes in the Title above -->

This ticket adds the header forwarding logic fix from [this
PR](#6166) to 5-lts
<!-- Describe your changes in detail -->

[TT-11683](https://tyktech.atlassian.net/browse/TT-11683)

<!-- 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. -->

<!-- Why is this change required? What problem does it solve? -->

<!-- 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. -->

<!-- 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)

<!-- 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-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.

___

<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>reverse_proxy.go</strong><dd><code>Enhancements and
Fixes in GraphQL Proxy Handling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

gateway/reverse_proxy.go
<li>Imported <code>httpclient</code> from <code>graphql-go-tools</code>
for handling HTTP client <br>operations.<br> <li> Modified
<code>returnErrorsFromUpstream</code> to use
<code>GraphQLProxyOnlyContextValues</code> <br>instead of
<code>GraphQLProxyOnlyContext</code>.<br> <li> Added
<code>selectContentEncodingToBeUsed</code> function to select the
appropriate <br>content encoding based on the
<code>Accept-Encoding</code> header.<br> <li> Updated various functions
to utilize the new <br><code>GraphQLProxyOnlyContextValues</code>
structure for better header management.

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6174/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+35/-3</a>&nbsp;
&nbsp; </td>
</tr>

<tr>
  <td>
    <details>
<summary><strong>mw_graphql_transport.go</strong><dd><code>Improved
Context Management in GraphQL Transport Middleware</code></dd></summary>
<hr>

gateway/mw_graphql_transport.go
<li>Introduced <code>GraphQLProxyOnlyContextValues</code> struct for
better management <br>of proxy-only context values.<br> <li> Added
functions <code>SetProxyOnlyContextValue</code> and
<code>GetProxyOnlyContextValue</code> <br>for setting and retrieving
proxy-only context values.<br> <li> Updated <code>handleProxyOnly</code>
and <code>setProxyOnlyHeaders</code> to use the new context <br>values
structure.

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6174/files#diff-f75d622cc8e7e488b4c7795f381baa5177c568dbfcfbb4422bedf7b4b31c31ba">+36/-5</a>&nbsp;
&nbsp; </td>
</tr>
</table></td></tr><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>reverse_proxy_test.go</strong><dd><code>New Test for
Header Forwarding in GraphQL Proxy-Only Mode</code></dd></summary>
<hr>

gateway/reverse_proxy_test.go
<li>Added a new test
<code>TestGraphQL_ProxyOnlyPassHeadersWithOTel</code> to ensure
<br>headers are correctly passed in proxy-only mode with OpenTelemetry
<br>enabled.

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6174/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3f">+38/-0</a>&nbsp;
&nbsp; </td>
</tr>

<tr>
  <td>
    <details>

<summary><strong>tyk_test-graphql-tracing-invalid_404.yml</strong><dd><code>Update
Tracing Test Scenario for GraphQL</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

ci/tests/tracing/scenarios/tyk_test-graphql-tracing-invalid_404.yml
<li>Changed the HTTP method from GET to POST in the tracing test
scenario <br>for GraphQL.

</details>

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6174/files#diff-59e055d305e2edd7dc55944593332b42baa0a5b2a08d3a2c3592ec115223d459">+1/-1</a>&nbsp;
&nbsp; &nbsp; </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
  • Loading branch information
kofoworola committed Mar 22, 2024
1 parent 78c891c commit dd9d2e3
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 9 deletions.
41 changes: 36 additions & 5 deletions gateway/mw_graphql_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion gateway/mw_graphql_transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
39 changes: 36 additions & 3 deletions gateway/reverse_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand All @@ -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 }
Expand All @@ -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 {
Expand All @@ -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)

Expand Down
38 changes: 38 additions & 0 deletions gateway/reverse_proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Check failure on line 989 in gateway/reverse_proxy_test.go

View workflow job for this annotation

GitHub Actions / Go 1.16 Redis 5

globalConf.OpenTelemetry undefined (type *config.Config has no field or method OpenTelemetry)
})
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()
Expand Down

0 comments on commit dd9d2e3

Please sign in to comment.