rule: Changes IPProto type to u8 - #1190
Conversation
|
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 due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an inline comment to ChangesFRA_IP_PROTO single-byte encoding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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.
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 `@rule.go`:
- Line 28: The public field Rule.IPProto was narrowed from int to uint8,
breaking callers that pass/read int values (and tests using unix.IPPROTO_UDP);
revert Rule.IPProto back to type int to preserve the API surface, and adjust
encoding/decoding in rule_linux.go (the FRA_IP_PROTO handling) to cast between
int and uint8 where necessary so internal netlink bytes remain a single byte
while the public type stays int; update any tests to use the int-typed field if
needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
coderabbit's suggestion is correct here, we don't want to change the public interface here as it may break callers. |
I considered that, but was of the opinion that it was an easily fixed breaking change and that it would be worse to silently change the semantics outside of the type system. If we need to narrow u32 to u8 before sending it to the kernel, we'll either have to return an error or silently discard upper bits. I suppose that given the kernel is already silently discarding larger values, doing it in userspace as well can't really change semantics. Do you have a preference on how to handle the narrowing? |
|
I agree it's unfortunate the Rule struct has |
netlink defines[1] the type of FRA_IP_PROTO to be a u8 but the definition in rule.go treats this field as a u32. This causes messages to appear in kernel logs with "attribute type 22 has an invalid length." This is exercised in TestRuleAddDel, although it does not cause the test to fail because netlink parses it permissively. The type of `IPProto` in `type Rule` is left as int to avoid a breaking API change. Instead, it is truncated in userspace before sending to the kernel. Truncation is how the kernel handles larger values, so we don't expect this to change semantics. [1]: https://elixir.bootlin.com/linux/v6.19.14/source/net/core/fib_rules.c#L862 Co-authored-by: conjones <connerj@cloudflare.com>
1d9df8c to
577f043
Compare
|
I amended the commit to not change the type of |
3 participants
netlink defines1 the type of FRA_IP_PROTO to be a u8 but the definition in rule.go treats this field as a u32. This causes messages to appear in kernel logs with "attribute type 22 has an invalid length." This is exercised in TestRuleAddDel, although it does not cause the test to fail because netlink parses it permissively.
Summary by CodeRabbit