🐛 fix: Parsing of IPv6 addresses#3466
Conversation
|
""" WalkthroughThe test suite for address parsing utilities was expanded with additional cases to cover more address formats, including various IPv6, IPv4, hostname, Unix socket, and edge-case scenarios. Error messages in assertions were also improved to include the original input for clearer diagnostics. The address parsing function was enhanced to correctly handle bracketed IPv6 addresses and other complex address formats. No changes were made to exported or public entities. Changes
Poem
📜 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 (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3466 +/- ##
==========================================
+ Coverage 84.36% 84.45% +0.08%
==========================================
Files 120 120
Lines 12140 12157 +17
==========================================
+ Hits 10242 10267 +25
+ Misses 1468 1462 -6
+ Partials 430 428 -2
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helpers.go (1)
569-600: Suggest adding RFC reference in function documentationThe function implementation now properly follows RFC 3986 for handling IPv6 addresses, but this isn't mentioned in the function documentation.
-func parseAddr(raw string) (string, string) { //nolint:revive // Returns (host, port) +func parseAddr(raw string) (string, string) { //nolint:revive // Returns (host, port) according to RFC 3986
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helpers.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Compare
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (5)
helpers.go (5)
570-572: LGTM - Good defensive programming!Adding a check for empty strings at the beginning of the function is a solid defensive programming practice. This prevents potential issues with empty inputs and makes the function more robust.
574-583: IPv6 bracketed address handling looks goodThe implementation correctly handles IPv6 addresses enclosed in brackets according to RFC 3986. This properly extracts both the host (with brackets) and port from addresses like
[2001:db8::1]:8080.
585-596: Robust handling of unbracketed IPv6 addressesThis enhancement correctly handles the case where an IPv6 address isn't properly bracketed but still needs to be parsed. By checking if the host part still contains colons after splitting at the last colon, the function can detect an unbracketed IPv6 address and treat the entire string as the host.
598-599: Comment adds clarityThe additional comment explains the fallback case when no colon is present in the input string, improving code readability.
569-600:Details
✅ Verification successful
Verify IPv6 parsing with zone identifiers
The function should be able to handle IPv6 addresses with zone identifiers (e.g.,
[fe80::1%eth0]:8080), but it would be good to verify this with test cases.
🏁 Script executed:
#!/bin/bash # Description: Search for IPv6 zone identifier test cases in the test file # Look for tests with zone identifiers (%) in IPv6 addresses rg -A 2 -B 2 "%.*\]" --glob "**/*_test.go"Length of output: 2974
IPv6 zone identifier support validated
Existing tests in helpers_test.go cover zone-identifier scenarios:
{addr: "[fe80::1%lo0]:1234", host: "[fe80::1%lo0]", port: "1234"}{addr: "[fe80::1%lo0]", host: "[fe80::1%lo0]", port: ""}The
parseAddrlogic correctly handles bracketed IPv6 literals with zone identifiers, so no additional changes are needed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: e70f6a0 | Previous: 273df5f | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels/Brotli_LevelBestCompression - B/op |
5 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helpers_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
helpers_test.go
[failure] 528-528:
File is not properly formatted (gofmt)
🪛 GitHub Actions: golangci-lint
helpers_test.go
[error] 528-528: File is not properly formatted (gofmt)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: Compare
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: repeated
🔇 Additional comments (2)
helpers_test.go (2)
519-532: Great enhancement to test coverage!The additional test cases for the
parseAddrfunction significantly improve coverage for various address formats, including IPv6 addresses without ports, IPv4 addresses without ports, hostnames, IPv6 link-local addresses with zone identifiers, and edge cases. This comprehensive set of test cases will help ensure the function correctly handles all these scenarios.The improved error messages in the assertions also add clarity by including the original address that caused the test failure.
🧰 Tools
🪛 GitHub Check: lint
[failure] 528-528:
File is not properly formatted (gofmt)🪛 GitHub Actions: golangci-lint
[error] 528-528: File is not properly formatted (gofmt)
536-537: Good improvement to error messagesAdding the original address string to the error messages makes test failures much more diagnostic-friendly.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the IPv6 address parsing logic and expands the associated test coverage for the parseAddr() function.
- Added multiple test cases for IPv6 (with/without ports, with zone identifiers), IPv4, hostname, and bare port or empty cases.
- Refined the parseAddr() function to handle empty input and bracketed IPv6 addresses more robustly.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| helpers_test.go | New test cases added to thoroughly validate IPv6, IPv4, and socket addresses. |
| helpers.go | Updated parseAddr() implements more precise handling of IPv6 and colon splitting. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
gaby
added
the
📜 RFC Compliance
Description
parseAddr()