collapse dedup (also known as "greedy bucket")

[entlein]

has a corresponding CT in nodeagent kubescape/node-agent#861
perf overhead was negligible but added bench

original PR was resulting in duplicate outputs: e.g.
learnt fan-out collapsed to CIDR(s) [52.216.0.0/26 52.216.0.0/26 52.216.0.0/27]

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a6c112fd-48a6-470f-ace8-e917aab798a6

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[entlein]

Validation

Unit tests — pkg/registry/file

The collapse suite passes:

• No over-approximation — hosts .1 .2 .3 cover to exactly [.1/32, .2/31], never a /30 that would admit the unobserved .0.
• Adjacent siblings merge — two /25 halves → /24; IPv6 2606:4700::/33 + 2606:4700:8000::/33 → 2606:4700::/32.
• Messy real CIDR lists reduce — subsumed ranges dropped and adjacent siblings merged, across IPv4 and IPv6.
• Floor is a breadth cap — a /22 under a /24 floor splits into its four /24 children.
• Fixpoint — re-collapsing already-collapsed input is idempotent; no duplicate or nested blocks accumulate across incremental saves.
• IPv6 hosts aggregate (lone host → /128, contiguous → the covering prefix); the floor cap is IPv4-only.

Component test — Test_34 (kubescape/node-agent#861)

Real learn → collapse path on a cluster, egressing to real cloud-provider address space:

workload	floor	result
fully-observed AWS S3 /28 (52.216.1.0/28)	/16	52.216.1.0/28
scattered S3 / Cloudflare / Azure / GCP IPs	/16	their exact /32s — no covering block
fully-observed S3 /27 (52.216.2.0/27)	/28	52.216.2.0/28 + 52.216.2.16/28

--- PASS: Test_34_NetworkNeighborsCIDRCollapse (380.97s)

[matthyx]

Thanks for tracking down the duplicate-CIDR bug — the root cause diagnosis is correct and the netipx-based dedupe/merge approach is the right idea. But I tested this against the actual 8,687-IP dataset from the real profile that originally motivated this feature (see #348), and the fix as written introduces a much larger regression that undoes the feature's core purpose. Requesting changes before merge.

The bug you're fixing is real

aggregateHosts computed a cover of fresh hosts only, then appended already-held pass-through CIDRs from prior saves verbatim, with no dedup or subsumption check. A held /26 + a held /27 (nested) + a freshly-recomputed /26 could all land in the same output — exactly the [52.216.0.0/26, 52.216.0.0/26, 52.216.0.0/27] garbage you found. Feeding hosts + held CIDRs into one netipx.IPSetBuilder and taking the exact minimal cover correctly fixes this (TestCollapseIPGroups_IncrementalReCollapseDeduplicatesAndAbsorbs demonstrates convergence to one /26).

Critical regression: floor-bucketing effectively stops working for realistic scattered traffic

coverPrefixes only runs splitToFloor on a prefix that netipx's exact cover already produced as broader than the floor. But netipx's exact cover only merges strictly adjacent, byte-aligned sibling prefixes. For genuinely scattered individual hosts — the real-world case — it just returns each host as its own /32 (occasionally a /31 pair), and a /32 is never "broader than the floor," so it's never bucketed at all.

I checked this out locally and ran it against the actual 8,687-IP dataset from the original motivating profile:

Implementation	floor /16	floor /24
Current on this branch (pre-#349)	10 entries	341 entries
This PR	8,614 entries	8,614 entries

Of the 8,614 output entries: 8,545 are still bare /32 hosts, 67 are /31 pairs, 2 are /30s. This is essentially no collapsing — the exact profile that was flagged too-large and motivated this feature would remain flagged too-large after this change.

Notably, TestCollapseIPGroups_ScatteredHostsStayGranularAndCappedAtFloor (renamed from TestCollapseIPGroups_AboveThresholdBroaderThanFloorBuckets) now asserts this as the intended behavior — 60 hosts each in a different /16 stay as 60 separate /32s, where the old test asserted they got bucketed. That's the previous bucketing guarantee inverted into its opposite; worth calling out explicitly as an intentional behavior change if it is one, but I don't think it should be.

Secondary issue: silent scope expansion to IPv6, not consumed by policy generation

classifyGroupAddresses dropped the addr.Is4() gate, so bare IPv6 hosts are now aggregated into collapsed IPv6 CIDR entries in IPAddresses (previously pass-through only). networkpolicy.go (untouched here) explicitly skips non-IPv4 entries in buildIPAddressesPeers, so a group of 50+ external IPv6 peers would now collapse into an IPv6 CIDR that silently produces zero peers in the generated NetworkPolicy — and if that neighbor has no selector, the skipPorts guard drops ports too, so the whole rule vanishes. Same failure class (collapsed entries silently missing from generated policy) that took a few review rounds to catch for IPv4 in #348.

Minor: reverses a deliberated design choice

The original design explicitly rejected netipx as the primary production aggregation mechanism for this exact failure mode, and restricted any use of it to test-only code specifically to avoid promoting it from an indirect to a direct dependency. This PR makes it the core production mechanism and promotes the dependency. Not necessarily wrong, but flagging it since that design history isn't visible anywhere in the repo (it lived in local planning notes), so there was no way to know about that constraint going in.

Suggested direction

Keep the dedupe/merge-via-netipx idea, but run it within floor-length buckets rather than before bucketing: bucket everything (fresh hosts + held CIDRs) by floor-length network first, then run netipx per-bucket just to dedupe/merge/absorb overlaps within that bucket. That preserves the original bucketing guarantee (bounded output size regardless of scatter) while still fixing the real duplicate/nested-CIDR bug.

Happy to help iterate on this if useful.

[entlein]

Re-verified against the current head

Test_34 (kubescape/node-agent — real learn→collapse path on a cluster, egressing to real cloud-provider space) is green with the storage image built from this branch's current head and deployed for the run:

--- PASS: Test_34_NetworkNeighborsCIDRCollapse (396.84s)

Learnt egress collapsed to:

workload	floor	result
fully-observed AWS S3 /28 (52.216.1.0/28)	/16	52.216.1.0/28
S3 spread across the /16	/16	52.216.0.0/16
scattered S3 / Cloudflare / Azure / GCP IPs	/16	one /16 per distinct block: 104.16.0.0/16, 172.64.0.0/16, 20.150.0.0/16, 13.107.0.0/16, 34.120.0.0/16, 35.190.0.0/16, 52.216.0.0/16, 52.217.0.0/16
fully-observed S3 /27 (52.216.2.0/27)	/28	52.216.2.0/28 + 52.216.2.16/28

[entlein]

Ranges tested → how they collapsed

Each workload egresses to the listed IPs; node-agent learns them, storage collapses:

tested IPs (egress)	floor	collapsed to
52.216.1.0–52.216.1.15 (a full /28)	/16	52.216.1.0/28
52.216.2.0–52.216.2.31 (a full /27)	/28	52.216.2.0/28, 52.216.2.16/28
52.216.{0,20,40,64,96,128,160,192,224,255}.{1,2,3} (~30 hosts across the /16)	/16	52.216.0.0/16
52.216.10.20, 52.217.50.100, 104.16.100.50, 172.64.200.10, 20.150.10.5, 13.107.6.152, 34.120.50.10, 35.190.20.30 (scattered across 8 distinct /16s)	/16	104.16.0.0/16, 13.107.0.0/16, 172.64.0.0/16, 20.150.0.0/16, 34.120.0.0/16, 35.190.0.0/16, 52.216.0.0/16, 52.217.0.0/16

Every scattered host is bucketed to its enclosing /16 (the floor); no bare host /32s remain.

[matthyx]

Re-tested the rewrite (ac8721e) — both issues from the previous review are resolved.

Floor-bucketing regression: fixed correctly, and via exactly the structure I'd suggested — aggregateHostsToFloor buckets first (common-prefix collapse when tighter than the floor, else one block per distinct floor-length network), and coverPrefixes only runs the netipx dedupe/merge/absorb step on those buckets + held CIDRs afterward, rather than running netipx's exact cover across raw scattered hosts. Re-ran against the same real 8,687-IP dataset:

	floor /16	floor /24
Before (this PR, previous commit)	8,614	8,614
Now	10	341

Matches the pre-#349 baseline exactly.

IPv6 scope creep: fixed — classifyGroupAddresses has the Is4() gate back, so IPv6 hosts are held pass-through rather than aggregated into CIDRs that buildIPAddressesPeers would silently drop. TestCollapseIPGroups_IPv6NotAggregatedHeldPassThrough covers it directly and explains why in the comment.

Also re-confirmed: the original duplicate-CIDR bug this PR set out to fix is still fixed (TestCollapseIPGroups_IncrementalReCollapseDeduplicatesAndAbsorbs passes), full go build/go vet/go test across the repo are clean, and the benchmark runs fine with no red flags.

Nice fix — the two-stage bucket-then-merge structure is the right shape for this. Approving.