fix: normalize IPv4 addresses and fix byte alignment in Multipath Via attribute#1196
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesVia.Encode IPv4 normalization and test coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
route_test.go (2)
2513-2513: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider clarifying the error message.
The phrase "Bug still persists" may confuse future readers, as it suggests the test is expected to fail. Consider rephrasing to focus on the expected behavior:
- t.Fatalf("Failed to add route with 16-byte IPv4 Via address (Bug still persists): %v", err) + t.Fatalf("RouteAdd should handle 16-byte IPv4 Via addresses: %v", err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@route_test.go` at line 2513, The error message in the Fatalf call contains the phrase "Bug still persists" which is confusing and suggests the test is expected to fail. Replace this phrase with clearer language that describes the expected behavior and what failed. Instead of implying a bug persists, focus the message on what the test was attempting to do (add a route with a 16-byte IPv4 Via address) and that this operation failed unexpectedly.
2512-2522: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider verifying the Via attribute after listing.
The test confirms the route was added, but doesn't verify that the
Viaattribute round-trips correctly.TestRouteViaAddDel(lines 2457-2461) demonstrates this pattern:got := routes[0].Via want := route.MultiPath[0].Via if !want.Equal(got) { t.Fatalf("Route Via attribute does not match; got: %s, want: %s", got, want) }Adding this check would strengthen confidence in the encode/decode round-trip for 16-byte IPv4 addresses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@route_test.go` around lines 2512 - 2522, The test verifies that a route with a 16-byte IPv4 Via address was added to the kernel, but it does not verify that the Via attribute round-trips correctly when retrieved. After the existing length check that confirms len(routes) == 1, add a verification step that compares the Via attribute from the returned route (routes[0].Via) with the Via attribute from the original route object using the Equal method, following the same pattern as shown in TestRouteViaAddDel where it checks if want.Equal(got) for the Via attribute comparison.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@route_test.go`:
- Line 2513: The error message in the Fatalf call contains the phrase "Bug still
persists" which is confusing and suggests the test is expected to fail. Replace
this phrase with clearer language that describes the expected behavior and what
failed. Instead of implying a bug persists, focus the message on what the test
was attempting to do (add a route with a 16-byte IPv4 Via address) and that this
operation failed unexpectedly.
- Around line 2512-2522: The test verifies that a route with a 16-byte IPv4 Via
address was added to the kernel, but it does not verify that the Via attribute
round-trips correctly when retrieved. After the existing length check that
confirms len(routes) == 1, add a verification step that compares the Via
attribute from the returned route (routes[0].Via) with the Via attribute from
the original route object using the Equal method, following the same pattern as
shown in TestRouteViaAddDel where it checks if want.Equal(got) for the Via
attribute comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3bed55b7-1078-460f-bb9c-0511d24dfd57
📒 Files selected for processing (2)
route_linux.goroute_test.go
barisaydogdu
force-pushed
the
fix/multipath-via-ipv4-alignment
branch
2 times, most recently
from
a263e89 to
1101b9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@route_test.go`:
- Around line 2524-2527: After the RouteDel(route) function call completes
without error, add a post-delete assertion to verify that the route has actually
been removed. Use a filtered list check (similar to how routes were listed
earlier in the test) and verify that the result length is zero, ensuring the
test covers the complete regression lifecycle of add, list, and delete
operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4766201d-04da-482a-af2d-eac4a9dbab2b
📒 Files selected for processing (2)
route_linux.goroute_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- route_linux.go
barisaydogdu
force-pushed
the
fix/multipath-via-ipv4-alignment
branch
from
1101b9f to
3cddaf8
Compare
barisaydogdu
force-pushed
the
fix/multipath-via-ipv4-alignment
branch
from
3cddaf8 to
de69024
Compare
|
Hi @vishvananda, @aboch The PR is ready for review. All CI checks are passing, and full lifecycle regression tests are added for the Multipath Via attribute. Whenever you have a moment, I'd appreciate your feedback. cc: @alexandremahdhaoui (This should resolve the invalid argument error you encountered in #1064). |
|
LGTM |
2 participants
Fixes an issue where passing an IPv4 address generated by
net.ParseIP()to a MultipathViaattribute causes the kernel to reject the route with aninvalid gatewayerror.The problem is that
net.ParseIP()returns a 16-byte slice, andVia.Encode()was writing all 16 bytes to theRTA_VIAattribute. The kernel strictly expects 4 bytes forAF_INET.This PR simply adds a
.To4()conversion insideVia.Encode()forFAMILY_V4to ensure we only serialize the correct 4 bytes. I also included a test (TestRouteMultiPathViaIPv4Mapped) to cover this case.Fixes #1064
Summary by CodeRabbit
Bug Fixes
Viaaddress encoding by converting inputs to proper IPv4 form, returning an error when an IPv4 conversion isn’t possible to better match kernel behavior.Tests
Viaaddresses (add/verify/delete).Viaencoding with IPv4AddrFamily, validating the encoded family and IPv4 bytes for1.1.1.1.