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

[TT-11997] Backend logic for request_headers_rewrite #6306

Merged

Conversation

buraksezer
Copy link
Contributor

@buraksezer buraksezer commented May 23, 2024

User description

See https://tyktech.atlassian.net/browse/TT-11997 for details of the request headers rewrite rules.


PR Type

Enhancement, Tests


Description

  • Added tests for request headers rewrite rules in GraphQL middleware.
  • Integrated request headers rewrite logic into the reverse proxy handler.
  • Added RequestHeadersRewrite to ReverseProxyParams struct.
  • Passed RequestHeadersRewrite to the GraphQL engine transport for both v1 and v2.
  • Implemented logic to apply request headers rewrite rules in the GraphQL engine transport, covering three different rewrite scenarios.

Changes walkthrough 📝

Relevant files
Tests
mw_graphql_test.go
Add tests for request headers rewrite rules in GraphQL middleware.

gateway/mw_graphql_test.go

  • Added tests for request headers rewrite rules.
  • Implemented three test cases for different rewrite scenarios.
  • +137/-0 
    Enhancement
    reverse_proxy.go
    Integrate request headers rewrite logic in reverse proxy handler.

    gateway/reverse_proxy.go

  • Integrated request headers rewrite logic into the reverse proxy
    handler.
  • +8/-7     
    engine.go
    Add RequestHeadersRewrite to ReverseProxyParams struct.   

    internal/graphengine/engine.go

    • Added RequestHeadersRewrite to ReverseProxyParams struct.
    +8/-7     
    graphql_go_tools_v1.go
    Pass RequestHeadersRewrite to GraphQL engine transport (v1).

    internal/graphengine/graphql_go_tools_v1.go

    • Passed RequestHeadersRewrite to the GraphQL engine transport.
    +1/-0     
    graphql_go_tools_v2.go
    Pass RequestHeadersRewrite to GraphQL engine transport (v2).

    internal/graphengine/graphql_go_tools_v2.go

    • Passed RequestHeadersRewrite to the GraphQL engine transport.
    +1/-0     
    transport.go
    Implement request headers rewrite logic in GraphQL engine transport.

    internal/graphengine/transport.go

  • Added logic to apply request headers rewrite rules in the GraphQL
    engine transport.
  • Implemented three rewrite rules for request headers.
  • +96/-1   

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

    @buraksezer buraksezer marked this pull request as ready for review May 23, 2024 13:32
    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Description updated to latest commit (46b54fa)

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and integrates new functionality across different components of the system. The complexity of the changes, especially around header rewriting logic, requires careful review to ensure correctness and efficiency.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The header rewrite logic in GraphQLEngineTransport might not correctly handle cases where headers are supposed to be removed or added based on complex conditions. The current implementation could lead to unintended header manipulations.

    🔒 Security concerns

    No

    Code feedback:
    relevant filegateway/mw_graphql_test.go
    suggestion      

    Consider adding more comprehensive test cases to cover scenarios where multiple headers are manipulated simultaneously, ensuring that the rewrite rules do not interfere with each other. [important]

    relevant line_, _ = g.Run(t, []test.TestCase{

    relevant fileinternal/graphengine/transport.go
    suggestion      

    Refactor the applyRequestHeadersRewriteRules function to separate the concerns of checking and applying rewrite rules into distinct methods. This will improve readability and maintainability of the code. [important]

    relevant linefunc (g *GraphQLEngineTransport) applyRequestHeadersRewriteRules(r *http.Request) {

    relevant fileinternal/graphengine/transport.go
    suggestion      

    Implement unit tests specifically for the applyRequestHeadersRewriteRules function to ensure each rewrite rule behaves as expected under various scenarios. [important]

    relevant linefunc (g *GraphQLEngineTransport) applyRequestHeadersRewriteRules(r *http.Request) {

    relevant fileinternal/graphengine/transport.go
    suggestion      

    Optimize the header rewrite logic to avoid unnecessary deletions and re-sets of headers which could lead to performance issues in high-load environments. Consider checking if the header actually needs changing before modifying it. [medium]

    relevant liner.Header.Del(key)

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a test case to verify header addition when the header is not present but defined in the rewrite rules with Remove set to false

    Consider adding a test case to verify the behavior when the header key is not present in
    the request but is defined in the RequestHeadersRewrite with Remove set to false. This
    will ensure that the header is added as expected.

    gateway/mw_graphql_test.go [646-663]

     _, _ = g.Run(t, []test.TestCase{
         {
             Data:   request,
             Method: http.MethodPost,
             Headers: map[string]string{
                 "X-Tyk-Key":   "tyk-value",
                 "X-Other-Key": "other-value",
    -            "X-Tyk-Test":  "value-from-consumer",
             },
             Code:      http.StatusOK,
             BodyMatch: `{"data":{"hello":"World","httpMethod":"POST"}}`,
             HeadersMatch: map[string]string{
                 "X-Tyk-Key":   "tyk-value",
                 "X-Other-Key": "other-value",
                 "X-Tyk-Test":  "value-from-rewrite-config",
             },
         },
     }...)
     
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid and enhances the test coverage by ensuring that the header is added when it is defined in the rewrite rules with Remove set to false. This is a crucial test case to verify the correct behavior of the header rewrite functionality.

    8

    Copy link

    sonarcloud bot commented May 28, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    16.4% Coverage on New Code (required ≥ 80%)
    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

    @buraksezer buraksezer enabled auto-merge (squash) May 28, 2024 08:46
    @buraksezer buraksezer merged commit 00a815f into master May 28, 2024
    35 of 36 checks passed
    @buraksezer buraksezer deleted the feat/TT-11997/Backend-logic-for-request_headers_rewrite branch May 28, 2024 12:14
    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.

    None yet

    2 participants