fix: use /128 CIDR for single IPv6 peers instead of /32 - #353
Conversation
getSingleIP always appended /32 to the address it was given, no matter what kind of address it was. That is correct for IPv4 but wrong for IPv6, where a single host needs a /128 prefix. A single IPv6 peer such as 2001:db8::1 was ending up as 2001:db8::1/32, which is not a valid single-host CIDR for IPv6 and corrupts the generated NetworkPolicy. This checks the address family and picks /128 for IPv6 and /32 for everything else, including IPv4-mapped addresses and unparseable input, which both keep the old /32 behavior. Converted the existing single-case test into a table test covering IPv4, IPv6, IPv4-mapped IPv6, and malformed input. Fixes AC-2 in kubescape#334 Signed-off-by: Arnesh Banerjee <linkrinku13@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesNetwork policy IP CIDR handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed at 1fb702d. Checked out locally: go build ./... and go test ./pkg/apis/softwarecomposition/networkpolicy/... both pass.
Approving. The change is exactly AC-2 of #334, minimal, and can't regress the IPv4/hostname paths: net.ParseIP returns nil for hostnames, malformed input and zone-bearing addresses, and every To4() != nil address keeps /32 — so only true IPv6 changes. Nice that one fix covers all three call sites (networkpolicy.go:392, :482, :583), and the table test conversion reads well.
Two notes, neither blocking merge.
1. ::ffff:1.2.3.4 → /32 is a valid string that means ::/32 — networkpolicy.go:601, pinned by the test row at networkpolicy_test.go:2296.
This is pre-existing behavior and what the ticket asked for, so it's not a regression from this PR. But the new test now records it as intended, and it isn't benign: Go parses that string as an IPv6 prefix, not an IPv4 one, and masks it down to a /32 of the IPv6 space.
net.ParseCIDR("::ffff:1.2.3.4/32") -> ip=1.2.3.4 net=::/32 err=<nil>
netip.ParsePrefix(...).Masked() -> ::/32
It parses cleanly, so nothing rejects it as malformed — the peer just silently widens from one host to 2^96 addresses. Reachability is low (NetworkNeighbor.IPAddress is filled by node-agent, and Go's IP.String() renders 4-in-6 as a dotted quad), which is why I'm not holding the PR for it. If you'd like it closed off, unmapping first collapses both families into one rule:
func getSingleIP(ipAddress string) *softwarecomposition.IPBlock {
if addr, err := netip.ParseAddr(ipAddress); err == nil {
addr = addr.Unmap().WithZone("") // zones are not valid inside a CIDR
if addr.Is4() {
return &softwarecomposition.IPBlock{CIDR: addr.String() + "/32"}
}
return &softwarecomposition.IPBlock{CIDR: addr.String() + "/128"}
}
return &softwarecomposition.IPBlock{CIDR: ipAddress + "/32"} // unparseable: unchanged
}The mapped test row would then expect 1.2.3.4/32. Note netip.ParseAddr is more permissive than net.ParseIP — it accepts fe80::1%eth0, hence the WithZone(""); today that input yields fe80::1%eth0/32, which is invalid either way. Follow-up PR is fine too; the minimum I'd ask for is an // intentional comment on the mapped test row so the next reader doesn't take ::ffff:…/32 for a host route.
2. Consistency, for tracking rather than for this PR: the plural path still drops every non-IPv4 entry (networkpolicy.go:561, continue // IPv6 or unparseable, out of scope (AC9), same at :528). After this merges, a single IPv6 peer gets a correct /128 while a batched one is silently discarded. Deliberate per AC-9, but worth keeping visible so IPv6 doesn't settle at half-done.
CI: the pull_request_created workflow is parked in action_required (run 31058071159) — it needs a maintainer to approve the run before build/test reports here. Only CodeRabbit, DCO and GitGuardian have posted so far; I ran the build and the package tests locally in the meantime.
|
Thanks! |
2 participants
Part of #334 (AC-2)
getSingleIP always added /32 to the address it was given, no matter what kind of address it was. That is correct for IPv4 but wrong for IPv6, where a single host needs a /128 prefix. A single IPv6 peer such as 2001:db8::1 was ending up as 2001:db8::1/32, which is not a valid single host CIDR for IPv6 and corrupts the generated NetworkPolicy.
This checks the address family and uses /128 for IPv6 and /32 for everything else, including IPv4-mapped addresses and input that fails to parse, which both keep the old /32 behavior.
I converted the existing single-case test into a table test covering IPv4, IPv6, IPv4-mapped IPv6, and malformed input.
Ran go test on the package and gofmt, both clean.
Summary by CodeRabbit
Bug Fixes
/128CIDRs./32CIDRs for IPv4 addresses and other supported inputs.Tests