feat(middleware/proxy): Add DialDualStack option for upstream IPv6 support #2900
Conversation
negrel
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThis update introduces a new Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/proxy.md (1 hunks)
- middleware/proxy/config.go (1 hunks)
- middleware/proxy/proxy.go (1 hunks)
- middleware/proxy/proxy_test.go (18 hunks)
Additional comments: 7
middleware/proxy/config.go (1)
- 54-64: The addition of the
DialDualStackfield to theConfigstruct is a significant enhancement for supporting dual-stack (IPv4 and IPv6) network environments. This change aligns with the PR's objective to improve IPv6 compatibility in proxy operations. However, it's crucial to ensure that the default value offalseforDialDualStackis explicitly documented in the API documentation to inform users about the default behavior.Additionally, the comment update on lines 54-55 is clear and informative, correctly indicating that the
DialDualStackoption, along with other specified fields, will not be utilized if a custom client is set. This is an important detail that helps prevent confusion about the precedence of configuration options.middleware/proxy/proxy.go (1)
- 48-49: The implementation of the
DialDualStackoption within theBalancerfunction is correctly applied to eachfasthttp.HostClientinstance. This ensures that whenDialDualStackis enabled in the configuration, the proxy middleware will attempt connections to both IPv4 and IPv6 addresses, optimizing network path selection. This change is crucial for applications operating in dual-stack environments and aligns with the PR's objectives.It's important to verify that this behavior is thoroughly tested, especially in environments where IPv6 connectivity might be inconsistent or unavailable, to ensure that the fallback to IPv4 does not introduce unexpected delays or issues.
docs/api/middleware/proxy.md (1)
- 154-154: The documentation update to include the
DialDualStackconfiguration option is clear and informative, providing users with guidance on enabling dual-stack connectivity for the proxy middleware. It's crucial that the documentation emphasizes the default value offalseforDialDualStackand explains the potential impact of enabling this option on network path selection and application versatility in dual-stack environments.One minor suggestion for improvement is to ensure consistency in the documentation's formatting and to check for any typographical errors or grammatical issues that might detract from the clarity of the information provided.
middleware/proxy/proxy_test.go (4)
- 18-30: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-40]
The implementation of
createProxyTestServerlooks correct and aligns with the PR objectives. However, consider using a more reliable synchronization mechanism thantime.Sleepfor waiting for the server to start, if applicable. This is not a blocker but could improve test reliability in some scenarios.
- 42-50: The introduction of
createProxyTestServerIPv4andcreateProxyTestServerIPv6functions is a good approach to support both IPv4 and IPv6 in tests. These changes align well with the PR's objective to enhance IPv6 support in the proxy middleware.- 161-182: The test
Test_Proxy_Balancer_IPv6_Upstreamcorrectly sets up an IPv6 test server and tests proxy balancing to it. However, the assertion forfiber.StatusInternalServerErroras the expected status code is unusual for a positive test case. Could you clarify if this is the intended behavior or if a different status code was expected?- 185-208: The introduction of
Test_Proxy_Balancer_IPv6_Upstream_With_DialDualStackis a valuable addition, effectively validating the newDialDualStackfeature with IPv6 upstreams. This test aligns well with the PR's objective to enhance IPv6 support and demonstrates the functionality of theDialDualStackoption.
negrel
force-pushed
the
fix-proxy-ipv6-upstream
branch
from
cbb4a09 to
9abeea9
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/proxy.md (1 hunks)
- middleware/proxy/config.go (1 hunks)
- middleware/proxy/proxy.go (1 hunks)
- middleware/proxy/proxy_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/proxy/config.go
- middleware/proxy/proxy.go
- middleware/proxy/proxy_test.go
docs/api/middleware/proxy.md
Outdated
| @@ -151,6 +151,7 @@ app.Use(proxy.BalancerForward([]string{ | |||
| | ReadBufferSize | `int` | Per-connection buffer size for requests' reading. This also limits the maximum header size. Increase this buffer if your clients send multi-KB RequestURIs and/or multi-KB headers (for example, BIG cookies). | (Not specified) | | |||
| | WriteBufferSize | `int` | Per-connection buffer size for responses' writing. | (Not specified) | | |||
| | TlsConfig | `*tls.Config` (or `*fasthttp.TLSConfig` in v3) | TLS config for the HTTP client. | `nil` | | |||
| | DialDualStack | `bool` | Client will attempt to connect to both ipv4 and ipv6 host addresses if set to true. | `false` | | |||
There was a problem hiding this comment.
The addition of the DialDualStack configuration option is well-documented. However, it would be beneficial to include an example or more detailed explanation on how enabling this option affects proxy behavior, especially for users unfamiliar with dual-stack environments.
Consider adding a practical example or a more detailed explanation of the DialDualStack option's impact on proxy behavior to aid user understanding.
There was a problem hiding this comment.
Consider adding a practical example or a more detailed explanation of the DialDualStack option's impact on proxy behavior to aid user understanding.
is a good hint, could you do this
only a feature that is understandable and well documented will be found and used
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@ReneWerner87 I've added an example. Let me know if that suits you!
negrel
force-pushed
the
fix-proxy-ipv6-upstream
branch
from
9abeea9 to
dc8bdcd
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/proxy.md (5 hunks)
- middleware/proxy/config.go (1 hunks)
- middleware/proxy/proxy.go (1 hunks)
- middleware/proxy/proxy_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/proxy/config.go
- middleware/proxy/proxy.go
- middleware/proxy/proxy_test.go
Additional comments: 1
docs/api/middleware/proxy.md (1)
- 156-156: The introduction of the
DialDualStackconfiguration option is a crucial enhancement for supporting dual-stack (IPv4 and IPv6) environments. This change aligns well with the PR objectives and modern network requirements. However, it's essential to ensure that the default value (false) is the intended behavior, as it requires explicit opt-in by the developer. This approach is sensible for backward compatibility but consider highlighting this in the documentation to ensure developers are aware they need to enable it for IPv6 support.
docs/api/middleware/proxy.md
Outdated
| c.Response().Header.Del(fiber.HeaderServer) | ||
| return nil | ||
| }, | ||
| // Enable support for IPv6 upstreams. | ||
| DialDualStack: true, | ||
| })) | ||
|
|
||
| // Or this way if the balancer is using https and the destination server is only using http. |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [122-133]
Adding the IPv6 address in the Servers list and introducing the DialDualStack option are significant improvements for IPv6 support. However, ensure that the example provided is clear to users about when and why to use DialDualStack: true. It might be beneficial to add a brief explanation or comment in the code snippet about the specific scenarios where enabling DialDualStack is advantageous.
Consider adding a comment in the code snippet explaining the use case for DialDualStack: true to enhance clarity for developers.
negrel
force-pushed
the
fix-proxy-ipv6-upstream
branch
2 times, most recently
from
472bd21 to
0700c40
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/proxy.md (4 hunks)
- middleware/proxy/config.go (1 hunks)
- middleware/proxy/proxy.go (1 hunks)
- middleware/proxy/proxy_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/proxy/config.go
- middleware/proxy/proxy.go
- middleware/proxy/proxy_test.go
Additional comments: 2
docs/api/middleware/proxy.md (2)
- 143-150: The example provided for using the
DialDualStackoption is clear and demonstrates how to enable IPv6 support in a round-robin balancer setup. This addition is crucial for applications that need to operate in dual-stack (IPv4 and IPv6) network environments. The example correctly shows the configuration of IPv6 and IPv4 addresses in theServerslist and the enabling ofDialDualStack.- 165-165: The documentation for the
DialDualStackconfiguration option is well-explained, indicating that setting this option totrueenables the client to attempt connections to both IPv4 and IPv6 host addresses. This is a significant enhancement for the proxy middleware, ensuring better network path selection and application versatility in dual-stack environments.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2900 +/- ##
==========================================
+ Coverage 82.66% 82.73% +0.07%
==========================================
Files 116 116
Lines 8368 8369 +1
==========================================
+ Hits 6917 6924 +7
+ Misses 1107 1103 -4
+ Partials 344 342 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@negrel Can you take a look at the merge conflict so we can move this forward? Thanks! |
negrel
force-pushed
the
fix-proxy-ipv6-upstream
branch
from
0700c40 to
6ad2db4
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- docs/api/middleware/proxy.md (4 hunks)
- middleware/proxy/config.go (1 hunks)
- middleware/proxy/proxy.go (1 hunks)
- middleware/proxy/proxy_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (3)
- middleware/proxy/config.go
- middleware/proxy/proxy.go
- middleware/proxy/proxy_test.go
Additional comments: 2
docs/api/middleware/proxy.md (2)
- 142-151: The example provided for making a round-robin balancer with IPv6 support using the
DialDualStackoption is clear and demonstrates the feature well. This addition should help users understand how to enable both IPv4 and IPv6 support in their proxy configurations.- 166-166: The addition of the
DialDualStackoption to the configuration section is well-documented, clearly explaining that setting this option to true enables the client to attempt connections to both IPv4 and IPv6 host addresses. The default value offalseis also correctly noted.
|
@gaby I've fix all conflicts. Let me know if this is good for you! |
Running CI now :-) thanks! |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- docs/api/middleware/proxy.md (4 hunks)
Additional comments: 3
docs/api/middleware/proxy.md (3)
- 10-10: The documentation correctly updates the signature comment for
BalancerForwardto include "round-robin" hyphenated, as previously suggested. This change improves grammatical correctness and consistency in the documentation.- 143-152: This section introduces the
DialDualStackoption in a practical example, demonstrating how to enable dual-stack (IPv4 and IPv6) support for proxy connections. The example is clear and directly illustrates the use of the new feature. However, it's essential to ensure that theDialDualStackfield is properly documented in theConfigstruct section to maintain consistency and provide complete information to the users.- 167-167: The documentation has been updated to include the
DialDualStackoption in theConfigstruct, specifying its purpose and default value. This addition is crucial for informing users about the new feature and how to utilize it. The description is concise and informative, aligning with the PR's objectives to enhance IPv6 support. It's recommended to ensure that all related documentation and examples are updated to reflect this new option for consistency and completeness.
sixcolors
changed the title
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
…pport (gofiber#2900) * 🔥 Add DialDualStack option to proxy middleware for upstream IPv6 support * Update docs/api/middleware/proxy.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: Juan Calderon-Perez <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Jason McNeil <[email protected]>
Description
This PR adds a
DialDualStackoptions github.com/gofiber/fiber/v2/middleware/proxy.Configin order to enable proxy middleware to forward to IPv6 upstreams.Fixes #2890
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md
Summary by CodeRabbit
DialDualStackfor proxy settings, allowing clients to attempt connections to both IPv4 and IPv6 host addresses.DialDualStackconfiguration option.