Skip to content

add logic for use_immutable_headers (TT-12190)#6315

Merged
pvormste merged 1 commit into
masterfrom
feat/TT-12190/add-logic-for-immutable-headers
May 29, 2024
Merged

add logic for use_immutable_headers (TT-12190)#6315
pvormste merged 1 commit into
masterfrom
feat/TT-12190/add-logic-for-immutable-headers

Conversation

@pvormste

@pvormste pvormste commented May 29, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Collaborator

User description

This PR adds the logic for use_immutable_headers for GraphQL proxy-only

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)

PR Type

Enhancement, Tests

Description

  • Added use_immutable_headers feature to GraphQL proxy-only mode.
  • Updated reverse proxy and transport logic to handle use_immutable_headers flag.
  • Refactored header configuration to use new structs.
  • Added comprehensive tests for new feature in middleware and transport.

Changes walkthrough 📝

Relevant files
Tests
mw_graphql_test.go
Add tests for use_immutable_headers feature in GraphQL middleware

gateway/mw_graphql_test.go

  • Added tests for use_immutable_headers feature.
  • Differentiated behavior based on use_immutable_headers flag.
    		•  +79/-34 
    transport_test.go
    Add tests for use_immutable_headers feature in transport 

    internal/graphengine/transport_test.go

    • Added tests for use_immutable_headers feature in transport.
    		 +114/-0 
    Enhancement
    reverse_proxy.go
    Update reverse proxy to support use_immutable_headers feature

    gateway/reverse_proxy.go

  • Updated reverse proxy handling to include use_immutable_headers flag.
  • Refactored header configuration handling.
    		•  +13/-8   
    engine.go
    Extend ReverseProxyParams with header configuration structs

    internal/graphengine/engine.go

  • Added HeadersConfig to ReverseProxyParams.
  • Introduced ReverseProxyHeadersConfig and ProxyOnlyHeadersConfig
    structs.
    		•  +16/-7   
    graphql_go_tools_v1.go
    Modify GraphQL transport initialization for header configuration

    internal/graphengine/graphql_go_tools_v1.go

    • Updated transport initialization to use HeadersConfig.
    		 +1/-1     
    graphql_go_tools_v2.go
    Modify GraphQL transport initialization for header configuration

    internal/graphengine/graphql_go_tools_v2.go

    • Updated transport initialization to use HeadersConfig.
    		 +2/-1     
    transport.go
    Refactor transport to use HeadersConfig and support
    use_immutable_headers

    internal/graphengine/transport.go

  • Refactored transport to use HeadersConfig for header rewrites.
  • Added logic to respect use_immutable_headers flag.
    		•  +10/-12 

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

    @pvormste
    pvormste requested review from buraksezer and kofoworola May 29, 2024 09:32
    @github-actions

    github-actions Bot commented May 29, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @github-actions

    github-actions Bot commented May 29, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (9c0a100)

    @github-actions

    github-actions Bot commented May 29, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and complex changes related to handling headers in a proxy environment. The logic for immutable and mutable headers needs careful review to ensure it behaves as expected under different configurations.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The implementation of header handling in GraphQLEngineTransport might not correctly prioritize consumer's header values when use_immutable_headers is true. This could lead to unexpected behavior where headers intended to be immutable can still be overwritten.

    🔒 Security concerns

    No
    Code feedback:
    relevant filegateway/mw_graphql_test.go
    suggestion      

    Consider adding a test case to verify that headers are not overwritten when use_immutable_headers is set to true, and that they are correctly overwritten when set to false. This will help ensure the feature works as expected across updates. [important]
    relevant linespec.GraphQL.Proxy.Features.UseImmutableHeaders = true
    relevant fileinternal/graphengine/transport.go
    suggestion      

    Ensure that the header rewrite logic checks the UseImmutableHeaders flag before applying any changes to the headers. This is crucial to maintain the integrity of immutable headers. [important]
    relevant lineheadersConfig: headersConfig,
    relevant fileinternal/graphengine/transport_test.go
    suggestion      

    Expand the test coverage to include scenarios where headers are supposed to be removed according to the rewrite rules, ensuring that the Remove flag in header rewrite configurations is respected. [medium]
    relevant lineReverseProxyHeadersConfig{
    relevant filegateway/reverse_proxy.go
    suggestion      

    Add logging around the decision points for immutable headers to aid in debugging and operational monitoring. This can help trace how headers are manipulated through the system. [medium]
    relevant lineUseImmutableHeaders: p.TykAPISpec.GraphQL.Proxy.Features.UseImmutableHeaders,

    @github-actions

    github-actions Bot commented May 29, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add error handling for the g.Run function to improve test reliability

    Consider adding error handling for the g.Run function to ensure that any runtime errors
    during the test execution are caught and handled appropriately.

    gateway/mw_graphql_test.go [600-618]

    -_, _ = g.Run(t, []test.TestCase{...})
+_, err := g.Run(t, []test.TestCase{...})
+if err != nil {
+    t.Fatalf("Error running test: %v", err)
+}
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the g.Run function is a best practice that ensures any runtime errors during the test execution are caught and handled appropriately, improving the reliability of the tests.

    		9
    Possible bug
    Add a check to ensure header key exists before modifying it

    To prevent potential issues with header manipulation, consider adding a check to ensure
    that the header key exists before attempting to delete or modify it in the request
    headers.

    internal/graphengine/transport.go [175-177]

    -if g.headersConfig.ProxyOnly.UseImmutableHeaders && exitingHeaderValue != "" {
+if g.headersConfig.ProxyOnly.UseImmutableHeaders && exitingHeaderValue != "" && r.Header.Has(forwardedHeaderKey) {
     r.Header.Del(forwardedHeaderKey)
 }
    Suggestion importance[1-10]: 8

    Why: Adding a check to ensure the header key exists before attempting to delete or modify it prevents potential issues with header manipulation, making the code more robust and less prone to runtime errors.

    		8
    Enhancement
    Add assertions to verify the final state of request headers after middleware processing

    To ensure that the headers are being set correctly regardless of the use_immutable_headers
    flag, consider adding assertions in your test cases to check the final state of the
    request headers after they have been potentially modified by the middleware.

    gateway/mw_graphql_test.go [611-616]

     HeadersMatch: map[string]string{
     "Authorization": "123abc",
     "X-Tyk-Key":     "tyk-value",
     "X-Other-Key":   "other-value",
     "X-Tyk-Test":    "value-from-consumer",
 }
+assert.Equal(t, "123abc", request.Header.Get("Authorization"))
+assert.Equal(t, "tyk-value", request.Header.Get("X-Tyk-Key"))
+assert.Equal(t, "other-value", request.Header.Get("X-Other-Key"))
+assert.Equal(t, "value-from-consumer", request.Header.Get("X-Tyk-Test"))
    Suggestion importance[1-10]: 7

    Why: Adding assertions to verify the final state of request headers enhances the test cases by ensuring that the headers are being set correctly regardless of the use_immutable_headers flag. This improves test coverage and reliability.

    		7
    Maintainability
    Refactor header manipulation into a separate method to improve code maintainability

    To enhance the readability and maintainability of the code, consider refactoring the
    header manipulation logic into a separate method. This method could handle the logic of
    checking and applying header rules based on the configuration.

    internal/graphengine/transport.go [175-178]

    -if g.headersConfig.ProxyOnly.UseImmutableHeaders && exitingHeaderValue != "" {
-    r.Header.Del(forwardedHeaderKey)
+applyHeaderRules(r, forwardedHeaderKey, forwardedHeaderValue, g.headersConfig)
+
+func applyHeaderRules(r *http.Request, key, value string, config ReverseProxyHeadersConfig) {
+    if config.ProxyOnly.UseImmutableHeaders && r.Header.Get(key) != "" {
+        r.Header.Del(key)
+    }
+    r.Header.Add(key, value)
 }
-r.Header.Add(forwardedHeaderKey, forwardedHeaderValue)
    Suggestion importance[1-10]: 6

    Why: Refactoring the header manipulation logic into a separate method enhances the readability and maintainability of the code. While this is a good practice, it is not crucial for functionality, hence a moderate score.

    		6

    @sonarqubecloud

    sonarqubecloud Bot commented May 29, 2024

    Copy link
    Copy Markdown

    Quality Gate Failed Quality Gate failed

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

    @pvormste
    pvormste enabled auto-merge (squash) May 29, 2024 10:38
    @pvormste
    pvormste merged commit 08a7fde into master May 29, 2024
    @pvormste
    pvormste deleted the feat/TT-12190/add-logic-for-immutable-headers branch May 29, 2024 11:54
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    @pvormste @nerdydread
    add logic for use_immutable_headers (TT-12190) (#6315)
    8d48ddd 
    ### **User description**
This PR adds the logic for `use_immutable_headers` for GraphQL
proxy-only

## 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)
- [x] 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)


___

### **PR Type**
Enhancement, Tests


___

### **Description**
- Added `use_immutable_headers` feature to GraphQL proxy-only mode.
- Updated reverse proxy and transport logic to handle
`use_immutable_headers` flag.
- Refactored header configuration to use new structs.
- Added comprehensive tests for new feature in middleware and transport.


___



### **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>mw_graphql_test.go</strong><dd><code>Add tests for
use_immutable_headers feature in GraphQL
middleware</code></dd></summary>
<hr>

gateway/mw_graphql_test.go
<li>Added tests for <code>use_immutable_headers</code> feature.<br> <li>
Differentiated behavior based on <code>use_immutable_headers</code>
flag.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-72c11b8efcaceccfbbd87e75565353706443bf56420d3fdba6b5bf5f632f4b33">+79/-34</a>&nbsp;
</td>
</tr>                    

<tr>
  <td>
    <details>
<summary><strong>transport_test.go</strong><dd><code>Add tests for
use_immutable_headers feature in transport</code>&nbsp; </dd></summary>
<hr>

internal/graphengine/transport_test.go
- Added tests for `use_immutable_headers` feature in transport.



</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-14490e6a204ab6724dd57f0f7f840b6d9b200f68bccf8d8df87c8d41321551f6">+114/-0</a>&nbsp;
</td>
</tr>                    
</table></td></tr><tr><td><strong>Enhancement
</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>reverse_proxy.go</strong><dd><code>Update reverse proxy
to support use_immutable_headers feature</code></dd></summary>
<hr>

gateway/reverse_proxy.go
<li>Updated reverse proxy handling to include
<code>use_immutable_headers</code> flag.<br> <li> Refactored header
configuration handling.<br>


</details>
    

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

<tr>
  <td>
    <details>
<summary><strong>engine.go</strong><dd><code>Extend ReverseProxyParams
with header configuration structs</code></dd></summary>
<hr>

internal/graphengine/engine.go
<li>Added <code>HeadersConfig</code> to
<code>ReverseProxyParams</code>.<br> <li> Introduced
<code>ReverseProxyHeadersConfig</code> and
<code>ProxyOnlyHeadersConfig</code> <br>structs.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-9d1ef4ec1b59d708ed86c7cc348b73bd0b619ef034094b5fbd4b138e11ccde6e">+16/-7</a>&nbsp;
&nbsp; </td>
</tr>                    

<tr>
  <td>
    <details>
<summary><strong>graphql_go_tools_v1.go</strong><dd><code>Modify GraphQL
transport initialization for header configuration</code></dd></summary>
<hr>

internal/graphengine/graphql_go_tools_v1.go
- Updated transport initialization to use `HeadersConfig`.



</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-e592cc8ca6ac39e7574765d7f2bbf19193f173791a1b0930d4dde7f9412dc882">+1/-1</a>&nbsp;
&nbsp; &nbsp; </td>
</tr>                    

<tr>
  <td>
    <details>
<summary><strong>graphql_go_tools_v2.go</strong><dd><code>Modify GraphQL
transport initialization for header configuration</code></dd></summary>
<hr>

internal/graphengine/graphql_go_tools_v2.go
- Updated transport initialization to use `HeadersConfig`.



</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-2e41b60f046c2947fbedb8fb841b5c3c962798e3ba8211c7144f326436ffabe3">+2/-1</a>&nbsp;
&nbsp; &nbsp; </td>
</tr>                    

<tr>
  <td>
    <details>
<summary><strong>transport.go</strong><dd><code>Refactor transport to
use HeadersConfig and support
</code><br><code>use_immutable_headers</code></dd></summary>
<hr>

internal/graphengine/transport.go
<li>Refactored transport to use <code>HeadersConfig</code> for header
rewrites.<br> <li> Added logic to respect
<code>use_immutable_headers</code> flag.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6315/files#diff-564061c9b29366529eb1f6f10fe39671d2ac738a4731ffd2c8b04dcc0a8cd610">+10/-12</a>&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
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @kofoworola kofoworola kofoworola approved these changes
    @buraksezer buraksezer Awaiting requested review from buraksezer

    Assignees

    No one assigned

    Labels

    enhancement Review effort [1-5]: 3 Tests

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @pvormste @kofoworola