Skip to content

[TT-11990] Change default behaviour of request_headers#6277

Merged
buraksezer merged 3 commits into
masterfrom
feat/TT-11990/Change-default-behaviour-of-request_headers
May 15, 2024
Merged

[TT-11990] Change default behaviour of request_headers#6277
buraksezer merged 3 commits into
masterfrom
feat/TT-11990/Change-default-behaviour-of-request_headers

Conversation

@buraksezer

@buraksezer buraksezer commented May 14, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

User description

See https://tyktech.atlassian.net/browse/TT-11990

PR Type

enhancement, tests

Description

  • Added a new integration test in gateway/mw_graphql_test.go to verify that consumer's header values are prioritized over the default request headers in GraphQL middleware.
  • Updated internal/graphengine/transport.go to enhance header management by prioritizing consumer's header values, deleting the header from request headers if it exists before adding the consumer's value.

Changes walkthrough 📝

Relevant files
Tests
mw_graphql_test.go
Add test for prioritizing consumer header values in GraphQL middleware

gateway/mw_graphql_test.go

  • Added a new test case to ensure consumer's header values are
    prioritized over default request headers.
    		•  +41/-0   
    Enhancement
    transport.go
    Modify header handling to prioritize consumer's values     

    internal/graphengine/transport.go

  • Modified header handling to prioritize consumer's header values by
    deleting existing header from request headers if present.
    		•  +6/-0     

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

    @github-actions

    github-actions Bot commented May 14, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @buraksezer
    buraksezer marked this pull request as ready for review May 15, 2024 07:31
    @github-actions

    github-actions Bot commented May 15, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (c5f98d6)

    @github-actions

    github-actions Bot commented May 15, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a moderate amount of changes with clear intent and well-documented code. The changes are localized to specific functionalities, making it easier to review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: In internal/graphengine/transport.go, the header is deleted and then added which might lead to unexpected behavior if multiple values for the same header are expected.

    🔒 Security concerns

    No
    Code feedback:
    relevant fileinternal/graphengine/transport.go
    suggestion      

    Consider using r.Header.Set instead of r.Header.Del followed by r.Header.Add to replace the header value directly. This ensures that the header value is replaced in a single operation, reducing the complexity and potential for errors. [important]
    relevant liner.Header.Del(forwardedHeaderKey)

    @github-actions

    github-actions Bot commented May 15, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Modify the header handling to avoid deleting existing headers unnecessarily

    The current implementation deletes the existing header and then adds the new one, which
    could lead to unexpected behavior if multiple values for the same header are expected.
    Instead, you should check if the header exists and, if so, not add the new value.

    internal/graphengine/transport.go [79-85]

    -exitingHeaderValue := r.Header.Get(forwardedHeaderKey)
-// Prioritize consumer's header value. Delete the header from request_headers
-// and add the consumer's value. See TT-11990.
-if exitingHeaderValue != "" {
-    r.Header.Del(forwardedHeaderKey)
+if r.Header.Get(forwardedHeaderKey) == "" {
+    r.Header.Add(forwardedHeaderKey, forwardedHeaderValue)
 }
-r.Header.Add(forwardedHeaderKey, forwardedHeaderValue)
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a potential issue with the handling of headers that could lead to unexpected behavior. The proposed change ensures that headers are only added if they do not already exist, which is a safer approach.

    		7
    Enhancement
    Add a test case for conflicting headers to ensure prioritization works as expected

    The test case should include a scenario where conflicting headers are provided by both the
    API specification and the consumer to ensure that the consumer's headers are prioritized
    correctly according to the new feature.

    gateway/mw_graphql_test.go [602-606]

     Headers: map[string]string{
     "X-Tyk-Key":   "tyk-value",
     "X-Other-Key": "other-value",
     "X-Tyk-Test":  "value-from-consumer",
+    "Authorization": "new-consumer-value",  // Adding conflicting header
 }
    Suggestion importance[1-10]: 6

    Why: This suggestion is useful as it proposes adding a test case to handle scenarios where API specification and consumer headers conflict, ensuring the new feature works as intended. This enhances the test coverage for the new functionality.

    		6
    Ensure the 'Authorization' header value is correctly validated in the test

    The test case does not verify if the 'Authorization' header retains its value from the
    request headers when a consumer's value is also present. It's important to add an
    assertion to check that the 'Authorization' header is not overwritten by consumer-specific
    headers.

    gateway/mw_graphql_test.go [609-614]

     HeadersMatch: map[string]string{
-    "Authorization": "123abc",
+    "Authorization": "123abc",  // Ensure this value is not overwritten
     "X-Tyk-Key":     "tyk-value",
     "X-Other-Key":   "other-value",
     "X-Tyk-Test":    "value-from-consumer",
 }
    Suggestion importance[1-10]: 5

    Why: The suggestion is valid as it emphasizes the importance of verifying that the 'Authorization' header is not overwritten. However, the existing test already checks the 'Authorization' header value, so the suggestion does not add significant new checks but rather clarifies the existing ones.

    		5
    Maintainability
    Correct the typo in the variable name for clarity

    The variable name 'exitingHeaderValue' might be a typo and could be confusing. Consider
    renaming it to 'existingHeaderValue' to improve code readability.

    internal/graphengine/transport.go [79]

    -exitingHeaderValue := r.Header.Get(forwardedHeaderKey)
+existingHeaderValue := r.Header.Get(forwardedHeaderKey)
    Suggestion importance[1-10]: 4

    Why: Correcting the typo from 'exitingHeaderValue' to 'existingHeaderValue' improves code readability. Although this is a minor change, it helps in maintaining the clarity and professionalism of the code.

    		4

    @github-actions

    github-actions Bot commented May 15, 2024

    Copy link
    Copy Markdown
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @sonarqubecloud

    sonarqubecloud Bot commented May 15, 2024

    Copy link
    Copy Markdown

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required ≥ 80%)

    See analysis details on SonarCloud

    @buraksezer
    buraksezer merged commit 0e82520 into master May 15, 2024
    @buraksezer
    buraksezer deleted the feat/TT-11990/Change-default-behaviour-of-request_headers branch May 15, 2024 14:38
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    @buraksezer @nerdydread
    [TT-11990] Change default behaviour of request_headers (#6277)
    cacea9f 
    ### **User description**
See https://tyktech.atlassian.net/browse/TT-11990


___

### **PR Type**
enhancement, tests


___

### **Description**
- Added a new integration test in `gateway/mw_graphql_test.go` to verify
that consumer's header values are prioritized over the default request
headers in GraphQL middleware.
- Updated `internal/graphengine/transport.go` to enhance header
management by prioritizing consumer's header values, deleting the header
from request headers if it exists before adding the consumer's value.


___



### **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 test for
prioritizing consumer header values in GraphQL
middleware</code></dd></summary>
<hr>

gateway/mw_graphql_test.go
<li>Added a new test case to ensure consumer's header values are
<br>prioritized over default request headers.<br>


</details>
    

  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6277/files#diff-72c11b8efcaceccfbbd87e75565353706443bf56420d3fdba6b5bf5f632f4b33">+41/-0</a>&nbsp;
&nbsp; </td>
</tr>                    
</table></td></tr><tr><td><strong>Enhancement
</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>transport.go</strong><dd><code>Modify header handling
to prioritize consumer's values</code>&nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

internal/graphengine/transport.go
<li>Modified header handling to prioritize consumer's header values by
<br>deleting existing header from request headers if present.<br>


</details>
    

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

    Reviewers

    @pvormste pvormste pvormste approved these changes
    @kofoworola kofoworola kofoworola approved these changes
    @rhianeKobar rhianeKobar Awaiting requested review from rhianeKobar

    Assignees

    No one assigned

    Labels

    enhancement Review effort [1-5]: 2 Tests

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @buraksezer @pvormste @kofoworola