🔥 feat: Add KeepConnectionHeader option to Proxy middleware#3662
🔥 feat: Add KeepConnectionHeader option to Proxy middleware#3662ReneWerner87 merged 6 commits intomainfrom
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA new boolean option, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant Backend
Client->>Proxy: HTTP request (with Connection header)
alt KeepConnectionHeader = true
Proxy->>Backend: Forward request (preserve Connection header)
Backend->>Proxy: Response (may include Connection header)
Proxy->>Client: Forward response (preserve Connection header)
else KeepConnectionHeader = false
Proxy->>Backend: Forward request (remove Connection header)
Backend->>Proxy: Response (may include Connection header)
Proxy->>Client: Forward response (remove Connection header)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've introduced a new configuration option, KeepConnectionHeader, to the proxy middleware. This option provides users with explicit control over whether the Connection header is retained or dropped when proxying requests and responses, offering greater flexibility for various proxying scenarios.
Highlights
- New Configuration Option: Introduced KeepConnectionHeader (boolean, defaults to false) in the proxy.Config struct, allowing explicit control over the Connection header.
- Conditional Header Handling: Modified the proxy logic to conditionally remove the Connection header from both incoming requests and outgoing responses only if KeepConnectionHeader is false.
- Comprehensive Testing: Added new unit tests to validate both scenarios: when the Connection header is kept and when it is dropped, ensuring the new functionality works as expected.
- Documentation Updates: Updated the proxy.md middleware documentation to include the new option and added an entry to whats_new.md for changelog purposes.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new KeepConnectionHeader configuration option to the proxy middleware that allows retaining the Connection header instead of dropping it by default (which is the current behavior).
- Adds
KeepConnectionHeaderboolean field to proxy config (defaults to false) - Conditionally removes Connection header based on this setting in both request and response processing
- Includes comprehensive test coverage for both keeping and dropping the Connection header
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| middleware/proxy/config.go | Adds the new KeepConnectionHeader field to Config struct with default value |
| middleware/proxy/proxy.go | Implements conditional Connection header removal based on the new config option |
| middleware/proxy/proxy_test.go | Adds test cases to verify Connection header handling behavior |
| docs/middleware/proxy.md | Documents the new configuration option in the middleware documentation |
| docs/whats_new.md | Notes the new feature in the changelog |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3662 +/- ##
=======================================
Coverage 91.84% 91.84%
=======================================
Files 113 113
Lines 11443 11444 +1
=======================================
+ Hits 10510 10511 +1
Misses 669 669
Partials 264 264
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces a KeepConnectionHeader option to the proxy middleware, allowing the Connection header to be preserved. The implementation for the Balancer is correct and well-tested. My feedback focuses on improving the documentation for this new option, addressing a potential inconsistency with other proxy functions like Forward and Do, and suggesting a refactoring of the new tests for better maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a KeepConnectionHeader option to the proxy middleware, allowing users to preserve the Connection header, which is stripped by default. The implementation is sound, with updates to both the config-based Balancer and the global helpers like Do and Forward. The documentation and changelog have been updated accordingly, and the feature is well-tested. My main feedback is regarding the use of parallel tests on shared global state, which can lead to flaky test runs.
middleware/proxy/proxy_test.go
Outdated
| } | ||
|
|
||
| func Test_Proxy_Do_KeepConnectionHeader(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
This test modifies global state (keepConnectionHeader) and is marked to run in parallel. This can create race conditions with other tests that also modify this state (e.g., Test_Proxy_Do_DropConnectionHeader), leading to flaky results. Tests that modify shared global state should not be run in parallel. Please remove t.Parallel() to ensure this test runs serially.
middleware/proxy/proxy_test.go
Outdated
| } | ||
|
|
||
| func Test_Proxy_Do_DropConnectionHeader(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
This test modifies global state (keepConnectionHeader) and is marked to run in parallel. This can create race conditions with other tests that also modify this state (e.g., Test_Proxy_Do_KeepConnectionHeader), leading to flaky results. Tests that modify shared global state should not be run in parallel. Please remove t.Parallel() to ensure this test runs serially.
|
@gaby is this test flaky |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/middleware/proxy.md (1)
113-119: Optional: Slim down the second Balancer example or give it a subtitleBack-to-back full balancer snippets (Lines 104-112 and 113-119) differ by just one flag. Consider either:
- Adding a short heading/comment (e.g. “// Example with Connection header preserved”) so readers immediately see the purpose, or
- Merging the flag into the first example with an inline comment to reduce duplication.
This keeps the docs concise without losing information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/proxy.md(4 hunks)docs/whats_new.md(1 hunks)middleware/proxy/proxy.go(2 hunks)middleware/proxy/proxy_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- middleware/proxy/proxy.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/whats_new.md
- middleware/proxy/proxy_test.go
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/middleware/proxy.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.
🪛 markdownlint-cli2 (0.17.2)
docs/middleware/proxy.md
169-169: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
169-169: Table column count
Expected: 4; Actual: 3; Too few cells, row will be missing data
(MD056, table-column-count)
169-169: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
170-170: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🪛 GitHub Check: markdownlint
docs/middleware/proxy.md
[failure] 170-170: Code block style
docs/middleware/proxy.md:170 MD046/code-block-style Code block style [Expected: fenced; Actual: indented] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md046.md
[failure] 169-169: Tables should be surrounded by blank lines
docs/middleware/proxy.md:169 MD058/blanks-around-tables Tables should be surrounded by blank lines [Context: "| KeepConnectionHeader | `bool..."] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md058.md
[failure] 169-169: Table column count
docs/middleware/proxy.md:169:118 MD056/table-column-count Table column count [Expected: 4; Actual: 3; Too few cells, row will be missing data] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md056.md
[failure] 169-169: Table pipe style
docs/middleware/proxy.md:169:118 MD055/table-pipe-style Table pipe style [Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md055.md
🪛 GitHub Actions: markdownlint
docs/middleware/proxy.md
[error] 169-169: markdownlint MD055/table-pipe-style: Table pipe style [Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe]
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Fixes #2700