You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
3, because the PR involves changes across multiple files with modifications in test cases and core logic handling headers. Understanding the context and ensuring the changes meet the requirements needs a careful review.
🧪 Relevant tests
Yes
⚡ Possible issues
Possible Bug: The test case in mw_graphql_test.go assumes headers are case-insensitive, but the actual implementation in reverse_proxy.go might not handle all edge cases if headers are manipulated elsewhere in the code.
🔒 Security concerns
No
Code feedback:
relevant file
gateway/reverse_proxy.go
suggestion
Consider adding error handling for the textproto.CanonicalMIMEHeaderKey function to catch any potential issues with header key transformations. This is important to ensure robustness in header processing. [important]
Ensure that the removal of r.Header.Del(key) does not introduce any side effects where headers might need explicit deletion to prevent processing old values in subsequent requests. This is important for maintaining correct header states. [important]
Add a test case to ensure header case insensitivity is handled correctly
Consider adding a test case to verify the behavior when headers are provided in non-canonical form, as the implementation now explicitly handles case insensitivity using textproto.CanonicalMIMEHeaderKey.
Why: The suggestion is relevant and useful as it ensures that the new functionality handling case insensitivity in headers is properly tested. This enhances the robustness of the test suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
This PR addresses the problem explained here https://tyktech.atlassian.net/browse/TT-11997?focusedCommentId=54161
PR Type
Bug fix, Tests
Description
request_headers_rewriteconfiguration to ensure consistent handling.Header.Deloperation in the header rewrite logic.Changes walkthrough 📝
mw_graphql_test.go
Add test for request headers rewrite case insensitivitygateway/mw_graphql_test.go
insensitivity.
hitting the upstream.
reverse_proxy.go
Canonicalize header names in request headers rewrite configurationgateway/reverse_proxy.go
request_headers_rewriteconfiguration.
format.
transport.go
Remove unnecessary header deletion in rewrite rulesinternal/graphengine/transport.go
Header.Deloperation.