Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging to release-5.3.9: [TT-13021]Transfer encoding fix (#6770) #6776

Conversation

buger
Copy link
Member

@buger buger commented Dec 16, 2024

User description

TT-13021Transfer encoding fix (#6770)

User description

TASK: https://tyktech.atlassian.net/browse/TT-13021

Description

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

PR Type

Bug fix, Enhancement


Description

  • Fixed an issue with reading and resetting the request body in the URL
    rewrite middleware to ensure downstream handlers can process it
    correctly.
  • Enhanced regex matching logic in the URL rewrite middleware, including
    improved context data handling.
  • Extended the tracing functionality to support OAS definitions by
    adding a new field in the trace request structure and implementing
    extraction logic.
  • Improved error handling for malformed or incomplete trace requests.

Changes walkthrough 📝

Relevant files
Bug fix
mw_url_rewrite.go
Improve request body handling and regex matching in URL rewrite
middleware

gateway/mw_url_rewrite.go

  • Added error handling for reading the request body.
  • Reset the request body to allow downstream handlers to read it.
  • Enhanced regex matching logic and updated context data handling.
  • +20/-1   
    Enhancement
    tracing.go
    Extend tracing functionality to support OAS definitions   

    gateway/tracing.go

  • Extended trace request structure to include OAS definitions.
  • Added logic to extract and log OAS definitions into API definitions.
  • Improved error handling for missing or malformed trace requests.
  • +8/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull
    request to receive relevant information


    Co-authored-by: lghiur [email protected]


    PR Type

    Bug fix, Enhancement


    Description

    • Fixed an issue in the URL rewrite middleware to handle request body reading and resetting, ensuring downstream handlers can process it correctly.
    • Enhanced regex matching logic in the URL rewrite middleware, including improved context data handling and updates.
    • Added comprehensive test cases for payload triggers and ensured request body accessibility after checks in the URL rewrite middleware.
    • Updated JSON validation middleware to properly handle and reset the request body using io.ReadAll.

    Changes walkthrough 📝

    Relevant files
    Bug fix
    mw_url_rewrite.go
    Improve request body handling and regex matching in URL rewrite
    middleware

    gateway/mw_url_rewrite.go

  • Added error handling for reading the request body.
  • Reset the request body to allow downstream handlers to read it.
  • Enhanced regex matching logic and updated context data handling.
  • Improved context data updates with matches from the request body.
  • +18/-2   
    mw_validate_json.go
    Update JSON validation to handle request body correctly   

    gateway/mw_validate_json.go

  • Replaced ioutil.ReadAll with io.ReadAll for reading request body.
  • Ensured request body is properly reset for downstream processing.
  • +3/-2     
    Tests
    mw_url_rewrite_test.go
    Add tests for payload triggers and request body handling in URL
    rewrite

    gateway/mw_url_rewrite_test.go

  • Added test cases to validate payload triggers in URL rewrite
    middleware.
  • Ensured request body remains accessible after payload trigger checks.
  • Enhanced test coverage for various rewrite scenarios.
  • +48/-5   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    ### **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    TASK: https://tyktech.atlassian.net/browse/TT-13021
    
    ## Description
    
    <!-- Describe your changes in detail -->
    
    ## Related Issue
    
    <!-- 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. -->
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- 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. -->
    
    ## Screenshots (if appropriate)
    
    ## 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)
    - [ ] 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
    
    <!-- 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
    
    
    ___
    
    ### **PR Type**
    Bug fix, Enhancement
    
    
    ___
    
    ### **Description**
    - Fixed an issue with reading and resetting the request body in the URL
    rewrite middleware to ensure downstream handlers can process it
    correctly.
    - Enhanced regex matching logic in the URL rewrite middleware, including
    improved context data handling.
    - Extended the tracing functionality to support OAS definitions by
    adding a new field in the trace request structure and implementing
    extraction logic.
    - Improved error handling for malformed or incomplete trace requests.
    
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>mw_url_rewrite.go</strong><dd><code>Improve request
    body handling and regex matching in URL rewrite
    </code><br><code>middleware</code></dd></summary>
    <hr>
    
    gateway/mw_url_rewrite.go
    
    <li>Added error handling for reading the request body.<br> <li> Reset
    the request body to allow downstream handlers to read it.<br> <li>
    Enhanced regex matching logic and updated context data handling.<br>
    
    
    </details>
    
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6770/files#diff-84a6a5c810334aaa8702669f2aebf0284f116d83e8a55ec9d1d5b8bae87f1be6">+20/-1</a>&nbsp;
    &nbsp; </td>
    
    </tr>
    </table></td></tr><tr><td><strong>Enhancement</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>tracing.go</strong><dd><code>Extend tracing
    functionality to support OAS definitions</code>&nbsp; &nbsp;
    </dd></summary>
    <hr>
    
    gateway/tracing.go
    
    <li>Extended trace request structure to include OAS definitions.<br>
    <li> Added logic to extract and log OAS definitions into API
    definitions.<br> <li> Improved error handling for missing or malformed
    trace requests.<br>
    
    
    </details>
    
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6770/files#diff-0069987d730b02812808925a17e1434ca7558a4dfc8661beb27ccd11afb8c77d">+8/-2</a>&nbsp;
    &nbsp; &nbsp; </td>
    
    </tr>
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
    request to receive relevant information
    
    ---------
    
    Co-authored-by: lghiur <[email protected]>
    
    (cherry picked from commit 1b2df0e)
    @buger buger enabled auto-merge (squash) December 16, 2024 20:13
    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    6770 - Partially compliant

    Fully compliant requirements:

    • Fix the issue with reading and resetting the request body in the URL rewrite middleware.
    • Enhance regex matching logic in the URL rewrite middleware.
    • Improve context data handling in the URL rewrite middleware.

    Not compliant requirements:

    • Extend tracing functionality to support OAS definitions.
    • Add a new field in the trace request structure and implement extraction logic.
    • Improve error handling for malformed or incomplete trace requests.
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Bug
    The nopCloseRequestBody function is used to reset the request body, but there is no validation to ensure the body is properly reset for downstream handlers. This could lead to unexpected behavior.

    Error Handling
    Error handling for reading the request body (io.ReadAll) is added, but there is no fallback mechanism or retry logic in case of failure.

    Code Smell
    The nopCloseRequestBody function is used again without ensuring that the request body is properly reset for downstream handlers. This could lead to issues in other parts of the application.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Reset the request body after reading to prevent breaking subsequent reads

    Ensure that the r.Body is properly reset after being read by io.ReadAll to avoid
    breaking subsequent reads of the request body.

    gateway/mw_url_rewrite.go [696-701]

     nopCloseRequestBody(r)
     // Read the entire request body
     bodyBytes, err := io.ReadAll(r.Body)
     if err != nil {
         log.WithError(err).Error("error reading request body")
         return false
     }
    +r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical issue where the request body might not be reusable after being read, which could lead to errors in subsequent operations. The proposed change ensures the request body is reset, maintaining functionality and preventing potential bugs.

    9
    Reset the request body after reading to ensure it can be reused in subsequent operations

    Ensure that the r.Body is properly reset after being read by io.ReadAll to allow
    further processing of the request body.

    gateway/mw_validate_json.go [51-56]

     nopCloseRequestBody(r)
     // Load input body into gojsonschema
     bodyBytes, err := io.ReadAll(r.Body)
     if err != nil {
         return err, http.StatusBadRequest
     }
    +r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
     defer r.Body.Close()
    Suggestion importance[1-10]: 9

    Why: The suggestion resolves a potential issue where the request body might not be reusable after being read. By resetting the body, it ensures compatibility with further processing, which is crucial for maintaining the correctness of the application.

    9
    Reset the request body in the test case to ensure it can be read without errors

    Add a check to ensure that the req.Body is properly reset before reading it in the
    test case to avoid potential errors.

    gateway/mw_url_rewrite_test.go [1152-1157]

     //added check to ensure that reading the payload to check for the trigger does not break the request
     if tc.payloadTrigger {
    +    bodyBytes, _ := io.ReadAll(tc.req.Body)
    +    tc.req.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
         body, err := io.ReadAll(tc.req.Body)
         assert.NotEqual(t, "", string(body))
         assert.NoError(t, err)
     }
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the test case by ensuring the request body is reset before being read again. This prevents potential errors during testing and enhances the reliability of the test, though its impact is limited to the test environment.

    8

    Copy link

    Quality Gate Failed Quality Gate failed

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

    See analysis details on SonarQube Cloud

    @buger buger merged commit 014ce04 into release-5.3.9 Dec 16, 2024
    32 of 38 checks passed
    @buger buger deleted the merge/release-5.3.9/1b2df0e369a73ba01ad62af611b094f836349c95 branch December 16, 2024 20:27
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants