Add CIDR-based collapsing for NetworkNeighbors#348
Merged
Conversation
matthyx
added
the
ai-assisted
📝 WalkthroughWalkthroughAdds configurable CIDR collapsing for high-cardinality ChangesNetworkNeighbor CIDR collapsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NetworkNeighborhoodProcessor
participant collapseIPGroups
participant generateEgressRule
participant buildIPAddressesPeers
NetworkNeighborhoodProcessor->>collapseIPGroups: Collapse deflated neighbor hosts
collapseIPGroups-->>NetworkNeighborhoodProcessor: Return CIDR-bearing IPAddresses
NetworkNeighborhoodProcessor->>generateEgressRule: Generate policy rule from neighbor
generateEgressRule->>buildIPAddressesPeers: Parse CIDRs, wildcard, or bare IPv4
buildIPAddressesPeers-->>generateEgressRule: Return peers and PolicyRefs
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (3)
pkg/registry/file/networkneighborhood_ipcollapse.go (1)
177-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
commonPrefixLenwithmath/bits.The manual byte/bit loop is correct (verified against the threshold/bucketing tests), but
math/bits.LeadingZeros8removes the hand-rolled bit-counting and the associated risk of a subtle off-by-one regression on future edits.♻️ Proposed simplification
+import "math/bits" + func commonPrefixLen(addrs []netip.Addr) int { base := addrs[0].As4() common := 32 for _, addr := range addrs[1:] { b := addr.As4() n := 0 for i := 0; i < 4 && n < common; i++ { x := base[i] ^ b[i] - if x == 0 { - n += 8 - continue - } - for bit := 7; bit >= 0; bit-- { - if x&(1<<uint(bit)) != 0 { - break - } - n++ - } - break + n += bits.LeadingZeros8(x) + if x != 0 { + break + } } if n < common { common = n } } return common }🤖 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 `@pkg/registry/file/networkneighborhood_ipcollapse.go` around lines 177 - 202, Update commonPrefixLen to use math/bits.LeadingZeros8 when calculating the first differing byte’s shared-bit count, replacing the manual inner bit loop while preserving the existing prefix-length and threshold behavior.pkg/registry/file/networkneighborhood_ipcollapse_test.go (1)
136-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest comment doesn't match generated data.
The comment says the second batch spans "16.15.183.x plus neighboring /24s", but with
icapped at 249,i/256is always0, so every address lands in16.15.180.xonly — no distinct /24s are actually exercised. The assertions still pass, but the test doesn't exercise the multi-subnet scenario it claims to.♻️ Suggested fix to actually span multiple /24s
- for i := 0; i < 250; i++ { - in = append(in, hostNeighbor(fmt.Sprintf("16.15.%d.%d", 180+i/256, i%256))) - } + for i := 0; i < 500; i++ { + in = append(in, hostNeighbor(fmt.Sprintf("16.15.%d.%d", 180+i/256, i%256))) + }🤖 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 `@pkg/registry/file/networkneighborhood_ipcollapse_test.go` around lines 136 - 157, Correct the second data-generation loop in TestCollapseIPGroups_RealWorldShapeOrdersOfMagnitude so its generated addresses actually span 16.15.183.x and neighboring /24 subnets as described, while retaining roughly 250 inputs and the existing assertions.pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go (1)
360-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
skipPortsblock between egress/ingress.The same 6-line
buildIPAddressesPeers+ skip-check block is repeated verbatim ingenerateEgressRuleandgenerateIngressRule. GivengenerateEgressRule/generateIngressRulealready largely mirror each other, extracting this into a small shared helper (e.g.resolvePeersAndSkip(neighbor, knownServers) (peers, refs, skip)) would remove the duplication introduced by this change and reduce the risk of the two copies drifting apart on a future edit.Also applies to: 451-459
🤖 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 `@pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go` around lines 360 - 368, The duplicated IP-address peer resolution and skip-check logic in generateEgressRule and generateIngressRule should be extracted into one shared helper, such as resolvePeersAndSkip. Have both rule generators use the helper to obtain peers, policy references, and the skipPorts result, preserving the existing behavior and removing both inline copies.
🤖 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 `@pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go`:
- Around line 360-368: The duplicated IP-address peer resolution and skip-check
logic in generateEgressRule and generateIngressRule should be extracted into one
shared helper, such as resolvePeersAndSkip. Have both rule generators use the
helper to obtain peers, policy references, and the skipPorts result, preserving
the existing behavior and removing both inline copies.
In `@pkg/registry/file/networkneighborhood_ipcollapse_test.go`:
- Around line 136-157: Correct the second data-generation loop in
TestCollapseIPGroups_RealWorldShapeOrdersOfMagnitude so its generated addresses
actually span 16.15.183.x and neighboring /24 subnets as described, while
retaining roughly 250 inputs and the existing assertions.
In `@pkg/registry/file/networkneighborhood_ipcollapse.go`:
- Around line 177-202: Update commonPrefixLen to use math/bits.LeadingZeros8
when calculating the first differing byte’s shared-bit count, replacing the
manual inner bit loop while preserving the existing prefix-length and threshold
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1e20614-f36d-44c5-86db-aa361d0b7dfb
⛔ Files ignored due to path filters (3)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (19)
docs/features/networkneighbors-collapsing.mdpkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy_test.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apiserver/apiserver.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/networkneighborhood_ipcollapse.gopkg/registry/file/networkneighborhood_ipcollapse_test.gopkg/registry/file/networkneighborhood_processor.gopkg/registry/file/networkneighborhood_processor_test.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
Contributor
Author
|
Good catch — confirmed. Added a third commit (2fee917) fixing the actual root cause: `hash()` gob-encoded rules/policyRefs for dedup, but gob's map encoding follows Go's randomized map iteration order, so a rule containing a `LabelSelector.MatchLabels` map could hash differently across otherwise-identical calls. That's real product nondeterminism (generated NetworkPolicies weren't byte-stable run-to-run), not just a test artifact, as you noted. Fix: switched `hash()` from `encoding/gob` to `encoding/json`, which sorts map keys deterministically. Verification:
|
Node-agent bakes the literal destination IP into the NetworkNeighbor identifier hash for external traffic, so every distinct observed IP produces a separate entry - a real profile hit 8,687 such entries and was flagged too-large. Extend the existing path-collapsing pattern (CollapseConfiguration CRD, TTL-cached provider, PreSave hook) to NetworkNeighbor: group entries by (Type, DNS, NamespaceSelector, PodSelector), aggregate IPv4 hosts into CIDR blocks once a configurable count threshold is exceeded, bounded by a configurable breadth floor. Held-stable CIDRs and a deterministic collapsed Identifier keep the pass idempotent across repeated saves. Policy generation now consumes the plural IPAddresses field so generated NetworkPolicies still reflect collapsed ranges, with known-server enrichment preserved for bare IPs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
…ison compareNP compared p1's Egress/Ingress against itself instead of p2, making every compareNP-based assertion in TestGenerateNetworkPolicy a no-op (pre-existing, unrelated to any recent feature work). Fixing the comparison surfaced a real latent bug: mergeIngressRulesByPorts and mergeEgressRulesByPorts appended every IPBlock peer sharing a port key without deduping, so the same IP declared across multiple distinct NetworkNeighbor entries (e.g. seen from different containers) ended up duplicated 2-3x in the merged rule. Harmless semantically but clearly unintended. Add containsIPBlockPeer and use it at both merge sites, and correct the 3 stale test fixtures that had assumed a merged-multi-port rule shape the merge functions never actually produce (confirmed by the existing, always-passing TestMergeIngressRulesByPorts/ TestMergeEgressRulesByPorts, which document the real one-rule-per-port design). A separate, unrelated flaky test (TestGenerateNetworkPolicy/ real_duplicate_bug_test) was found during investigation: hash() uses encoding/gob on a struct containing a map, so Go's randomized map iteration order occasionally produces two different hashes for the same logical rule. Not fixed here - left as a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
hash() gob-encoded rules/policyRefs to dedupe them, but gob's map encoding follows Go's randomized map iteration order. A rule containing a LabelSelector's MatchLabels map could therefore hash differently across otherwise-identical calls, so GenerateNetworkPolicy's dedup-by- hash sometimes kept a duplicate identical rule and sometimes didn't - reproduced reliably under `go test -run TestGenerateNetworkPolicy -count=40` (3/40 failures on TestGenerateNetworkPolicy/ real_duplicate_bug_test before this fix, 0/100 after). This was real product nondeterminism (generated NetworkPolicies weren't byte-stable run-to-run), not just test flakiness. Switch hash() to json.Marshal, which sorts map keys, giving a stable encoding regardless of map iteration order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
feat/networkneighbors-cidr-collapsing
branch
from
July 22, 2026 11:27
2fee917 to
a5b922e
Compare
|
Summary:
|
entlein
reviewed
Jul 23, 2026
/16 delivers the most aggressive entry-count reduction (the real 8,687-entry motivating case collapses to ~10 entries) but at the cost of much broader, less precise CIDR blocks. Measured against that same real-world IP set, a /24 floor still collapses it to ~341 entries (down from 8,687) while keeping blocks 256x tighter. 341 is an acceptable trade-off, so make /24 the default; /16 remains available for operators who want maximum compaction. Also fill in the two new fields in the sample CollapseConfiguration YAML, which was never updated when they were introduced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/features/networkneighbors-collapsing.md`:
- Line 83: Update the CIDR breadth statement near the NetworkNeighbor validation
guidance to clarify that emitted CIDR blocks must not be broader than the
configured floor, or equivalently that their prefix length must be greater than
or equal to floorBits; preserve the default /24 reference.
- Line 75: Update the configuration refresh statement in
networkneighbors-collapsing.md to clarify that changed CRD singleton values take
effect after the provider’s cache TTL expires, while preserving that no operator
restart is required.
- Line 19: Update the CIDR collapsing documentation to state that non-empty
buckets may over-approximate the observed hosts by allowing the entire CIDR
range, rather than claiming the same destination ranges are preserved. Clearly
describe this allowlist broadening for generated NetworkPolicies and retain the
existing efficiency explanation.
- Around line 30-33: The documentation uses inconsistent threshold semantics for
network-neighbor collapsing. Update the aggregation description and the “50+
hosts” example to consistently reflect the implementation’s boundary behavior,
explicitly stating whether exactly NetworkIPGroupThreshold aggregatable hosts
trigger collapsing.
In `@pkg/registry/file/networkneighborhood_processor_test.go`:
- Around line 166-168: Update the test assertion sequence around the collapsed
result `got` so the length check is fatal before accessing `got[0]`; replace the
non-fatal length assertion with the appropriate fatal assertion while preserving
the existing empty-IP and CIDR validations.
🪄 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 Plus
Run ID: dd85722f-9ea0-45d1-915f-0f57939c094f
⛔ Files ignored due to path filters (3)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.gopkg/generated/applyconfiguration/softwarecomposition/v1beta1/collapseconfigurationspec.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (20)
artifacts/collapseconfiguration-default-sample.yamldocs/features/networkneighbors-collapsing.mdpkg/apis/softwarecomposition/collapse_types.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.gopkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy_test.gopkg/apis/softwarecomposition/v1beta1/collapse_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apiserver/apiserver.gopkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_processor_test.gopkg/registry/file/dynamicpathdetector/collapse_config_from_crd.gopkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/networkneighborhood_ipcollapse.gopkg/registry/file/networkneighborhood_ipcollapse_test.gopkg/registry/file/networkneighborhood_processor.gopkg/registry/file/networkneighborhood_processor_test.gopkg/registry/softwarecomposition/collapseconfiguration/strategy.gopkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/registry/file/dynamicpathdetector/types.go
- pkg/apis/softwarecomposition/v1beta1/collapse_types.go
- pkg/registry/file/dynamicpathdetector/tests/collapse_config_crd_test.go
- pkg/registry/softwarecomposition/collapseconfiguration/strategy_test.go
- pkg/registry/file/containerprofile_processor.go
- pkg/registry/file/containerprofile_processor_test.go
- pkg/registry/file/networkneighborhood_ipcollapse.go
- pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
- pkg/registry/file/networkneighborhood_processor.go
- pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy.go
- pkg/registry/file/networkneighborhood_ipcollapse_test.go
- pkg/registry/file/dynamicpathdetector/collapse_config_from_crd.go
- pkg/apis/softwarecomposition/networkpolicy/v2/networkpolicy_test.go
- pkg/apis/softwarecomposition/v1beta1/generated.proto
|
|
||
| Before collapsing, external-traffic profiles with 8,687 individual IP entries consumed excessive storage, slowed queries, and produced NetworkPolicies with thousands of rules — most of which could be expressed as a handful of CIDR blocks. | ||
|
|
||
| Collapsing reduces entry count by orders of magnitude for traffic aimed at cloud IP blocks (e.g. `100.68.24.0/22`, `16.15.183.0/24`) while preserving policy correctness: generated NetworkPolicies still include the same destination ranges, just expressed more efficiently as CIDR blocks instead of `/32` host routes. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Document CIDR policy over-approximation.
A non-empty /24 bucket can contain only a few observed hosts, yet the generated IPBlock permits the entire /24. Therefore, the claim that policy correctness is preserved with the “same destination ranges” is not strictly true. Explicitly document this allowlist broadening, or only emit CIDRs whose address space is fully covered by observed hosts.
Also applies to: 30-33
🤖 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 `@docs/features/networkneighbors-collapsing.md` at line 19, Update the CIDR
collapsing documentation to state that non-empty buckets may over-approximate
the observed hosts by allowing the entire CIDR range, rather than claiming the
same destination ranges are preserved. Clearly describe this allowlist
broadening for generated NetworkPolicies and retain the existing efficiency
explanation.
Comment on lines
+30
to
+33
| **Aggregation**: If a group's count of aggregatable IPv4 host addresses exceeds `NetworkIPGroupThreshold`: | ||
| 1. Compute the smallest covering prefix (common leading bits across all hosts). | ||
| 2. If that prefix's length is at least `NetworkCIDRFloorBits` (e.g., `/24` or longer), emit it as-is. | ||
| 3. If it would be broader than `NetworkCIDRFloorBits`, split the group into floor-length buckets (e.g., `/24` buckets) and emit one CIDR entry per non-empty bucket. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make threshold semantics consistent.
The text says collapsing occurs when the count “exceeds” the threshold, which implies > 50, while the example says “50+ hosts,” which implies >= 50. Clarify whether exactly 50 aggregatable hosts trigger collapsing and align both descriptions with the implementation.
Also applies to: 71-72
🤖 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 `@docs/features/networkneighbors-collapsing.md` around lines 30 - 33, The
documentation uses inconsistent threshold semantics for network-neighbor
collapsing. Update the aggregation description and the “50+ hosts” example to
consistently reflect the implementation’s boundary behavior, explicitly stating
whether exactly NetworkIPGroupThreshold aggregatable hosts trigger collapsing.
| networkCIDRFloorBits: 24 # No block narrower than /24 | ||
| ``` | ||
|
|
||
| Zero or omitted values use the compiled-in defaults (50 and 24 respectively). No operator restart is required; the provider reads the singleton at each request. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the configuration refresh semantics.
The provider is TTL-cached, so it does not read the CRD singleton on every request. State that changes take effect after the cache TTL while still not requiring an operator restart.
🤖 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 `@docs/features/networkneighbors-collapsing.md` at line 75, Update the
configuration refresh statement in networkneighbors-collapsing.md to clarify
that changed CRD singleton values take effect after the provider’s cache TTL
expires, while preserving that no operator restart is required.
|
|
||
| **Entry count**: Compare entry counts before and after enabling the feature on a real profile with external traffic. A profile with 8,687 external-traffic entries should drop to a small number (typically hundreds or fewer CIDR entries, depending on traffic distribution). | ||
|
|
||
| **CIDR breadth**: Inspect emitted `NetworkNeighbor` entries; no single `IPAddresses` CIDR block should exceed the configured floor (default `/24`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the floor verification wording.
“Should not exceed the configured floor” is ambiguous: /28 numerically exceeds /24 but is valid. Use “must not be broader than the configured floor” or “prefix length must be greater than or equal to floorBits.”
🤖 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 `@docs/features/networkneighbors-collapsing.md` at line 83, Update the CIDR
breadth statement near the NetworkNeighbor validation guidance to clarify that
emitted CIDR blocks must not be broader than the configured floor, or
equivalently that their prefix length must be greater than or equal to
floorBits; preserve the default /24 reference.
Comment on lines
+166
to
+168
| assert.Len(t, got, 1, "expected all same-group host IPs to collapse into a single CIDR entry") | ||
| assert.Empty(t, got[0].IPAddress) | ||
| assert.Equal(t, []string{"10.0.0.0/26"}, got[0].IPAddresses) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid indexing after a non-fatal length assertion.
If collapsing returns zero entries, assert.Len records a failure but execution continues and got[0] panics. Use a fatal length check before indexing.
Proposed fix
- assert.Len(t, got, 1, "expected all same-group host IPs to collapse into a single CIDR entry")
+ if len(got) != 1 {
+ t.Fatalf("expected one collapsed entry, got %d", len(got))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert.Len(t, got, 1, "expected all same-group host IPs to collapse into a single CIDR entry") | |
| assert.Empty(t, got[0].IPAddress) | |
| assert.Equal(t, []string{"10.0.0.0/26"}, got[0].IPAddresses) | |
| if len(got) != 1 { | |
| t.Fatalf("expected one collapsed entry, got %d", len(got)) | |
| } | |
| assert.Empty(t, got[0].IPAddress) | |
| assert.Equal(t, []string{"10.0.0.0/26"}, got[0].IPAddresses) |
🤖 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 `@pkg/registry/file/networkneighborhood_processor_test.go` around lines 166 -
168, Update the test assertion sequence around the collapsed result `got` so the
length check is fatal before accessing `got[0]`; replace the non-fatal length
assertion with the appropriate fatal assertion while preserving the existing
empty-IP and CIDR validations.
|
Summary:
|
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
…apse-dedup collapse dedup (also known as "greedy bucket")
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/registry/file/containerprofile_processor_test.go (1)
28-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid indexing after a non-fatal length assertion.
If collapsing unexpectedly returns zero entries,
assert.Lenat line 54 records a failure but execution continues, andresult.Ingress[0]panics on the next line. Use a fatal check (require.Len) before indexing, mirroring the same fix needed in the sibling test inpkg/registry/file/networkneighborhood_processor_test.go.Proposed fix
- assert.Len(t, result.Ingress, 1, "expected all same-group host IPs to collapse into a single CIDR entry") + require.Len(t, result.Ingress, 1, "expected all same-group host IPs to collapse into a single CIDR entry")🤖 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 `@pkg/registry/file/containerprofile_processor_test.go` around lines 28 - 63, Replace the non-fatal assert.Len check in TestDeflateContainerProfileSpec_NetworkNeighborsCollapse with require.Len before accessing result.Ingress[0], and add or reuse the test package’s require assertion import as needed. Preserve the existing expected length and subsequent assertions, matching the sibling network-neighborhood test.
🧹 Nitpick comments (2)
pkg/registry/file/networkneighborhood_ipcollapse.go (2)
204-208: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent data loss on
IPSet()build error.If
b.IPSet()errors (or returns a nil set),coverPrefixesreturnsnil. Back incollapseIPGroups, that discards the whole group's aggregated values — sentinels are still appended, but if there are none, the group's ports/DNS/selectors data for every host/CIDR in it vanishes from the output entirely, with no error surfaced. Given this feeds NetworkPolicy generation, a silently dropped peer set means broken/incomplete policies rather than a visible failure.In practice this path is unlikely to trigger since all inputs are pre-validated
netip.Prefix/Addrvalues, but a defensive fallback (log + fall back to the originalg.entries, or to the un-covered host/CIDR strings) would avoid a full-group silent drop.🤖 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 `@pkg/registry/file/networkneighborhood_ipcollapse.go` around lines 204 - 208, Update the IPSet handling in coverPrefixes so an error or nil set is logged and falls back to the original group entries (or their un-covered host/CIDR strings) instead of returning nil. Ensure collapseIPGroups preserves the group’s ports, DNS, and selectors when coverage construction fails, while retaining the existing covered-prefix path for successful builds.
113-125: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider combining a group's values into a single
NetworkNeighborinstead of one per value.Each collapsed value (
v) currently produces its ownNetworkNeighbor, duplicatingdnsNames/ports/selectors across N entries.collapsedIdentifieralready accepts a[]string, which suggests batching the wholevaluesslice into one entry (IPAddresses: values) was a viable design.This matters because
NetworkNeighborhoodProcessor.PreSavemeasures profile size aslen(Ingress)/len(Egress)(entry count), not address count. For scattered-host groups (e.g.TestCollapseIPGroups_ScatteredHostsBucketedToFloor, 60 buckets from 60 hosts), this yields zero net reduction in the size metric that gatesObjectTooLargeError, partially defeating the collapsing goal for high-cardinality scattered traffic.Note: several existing tests (e.g.
ScatteredHostsBucketedToFloor,RealWorldShapeOrdersOfMagnitude) assert one-entry-per-value output today, so this would require updating those expectations alongside the code change.♻️ Sketch of combined-entry approach
- for _, v := range values { - out = append(out, softwarecomposition.NetworkNeighbor{ - Identifier: collapsedIdentifier(g.repType, g.repDNS, g.repNsSel, g.repPodSel, []string{v}), - Type: g.repType, - DNS: g.repDNS, - DNSNames: append([]string(nil), dnsNames...), - Ports: append([]softwarecomposition.NetworkPort(nil), ports...), - PodSelector: g.repPodSel, - NamespaceSelector: g.repNsSel, - IPAddress: "", - IPAddresses: []string{v}, - }) - } + out = append(out, softwarecomposition.NetworkNeighbor{ + Identifier: collapsedIdentifier(g.repType, g.repDNS, g.repNsSel, g.repPodSel, values), + Type: g.repType, + DNS: g.repDNS, + DNSNames: dnsNames, + Ports: ports, + PodSelector: g.repPodSel, + NamespaceSelector: g.repNsSel, + IPAddress: "", + IPAddresses: values, + })🤖 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 `@pkg/registry/file/networkneighborhood_ipcollapse.go` around lines 113 - 125, Update the value-collapsing loop in the relevant network-neighborhood collapse function to emit one NetworkNeighbor for the entire values slice, using collapsedIdentifier with values and assigning IPAddresses to that slice while preserving the shared DNS, ports, and selector fields. Update affected tests, including scattered-host and real-world shape cases, to expect a single combined entry rather than one entry per value.
🤖 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.
Outside diff comments:
In `@pkg/registry/file/containerprofile_processor_test.go`:
- Around line 28-63: Replace the non-fatal assert.Len check in
TestDeflateContainerProfileSpec_NetworkNeighborsCollapse with require.Len before
accessing result.Ingress[0], and add or reuse the test package’s require
assertion import as needed. Preserve the existing expected length and subsequent
assertions, matching the sibling network-neighborhood test.
---
Nitpick comments:
In `@pkg/registry/file/networkneighborhood_ipcollapse.go`:
- Around line 204-208: Update the IPSet handling in coverPrefixes so an error or
nil set is logged and falls back to the original group entries (or their
un-covered host/CIDR strings) instead of returning nil. Ensure collapseIPGroups
preserves the group’s ports, DNS, and selectors when coverage construction
fails, while retaining the existing covered-prefix path for successful builds.
- Around line 113-125: Update the value-collapsing loop in the relevant
network-neighborhood collapse function to emit one NetworkNeighbor for the
entire values slice, using collapsedIdentifier with values and assigning
IPAddresses to that slice while preserving the shared DNS, ports, and selector
fields. Update affected tests, including scattered-host and real-world shape
cases, to expect a single combined entry rather than one entry per value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 690873b4-055c-4fcc-a619-ce4c9c14e27d
📒 Files selected for processing (7)
go.modpkg/registry/file/containerprofile_processor_test.gopkg/registry/file/dynamicpathdetector/types.gopkg/registry/file/networkneighborhood_ipcollapse.gopkg/registry/file/networkneighborhood_ipcollapse_bench_test.gopkg/registry/file/networkneighborhood_ipcollapse_test.gopkg/registry/file/networkneighborhood_processor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/registry/file/dynamicpathdetector/types.go
|
Summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NetworkNeighborentries, extending the existing path-collapsing pattern (CollapseConfigurationCRD, TTL-cached provider,PreSavehook) to the network side. Real-world case: node-agent bakes the literal destination IP into theNetworkNeighboridentifier hash, so every distinct external IP produces a separate entry — a real profile hit 8,687 such entries and was flaggedtoo-large. Seedocs/features/networkneighbors-collapsing.mdfor the full mechanism, config knobs, and explicit accepted trade-offs.compareNP(test helper) comparedp1's Egress/Ingress against itself instead ofp2, silently making everycompareNP-based assertion inTestGenerateNetworkPolicya no-op for years. Fixing it surfaced a real (harmless) latent bug —mergeIngressRulesByPorts/mergeEgressRulesByPortscould duplicate the sameIPBlockpeer 2-3x when the same IP appeared across multiple distinctNetworkNeighborentries — now deduped.Test plan
go build ./...passesgo vet ./...passes (no new warnings; one pre-existing unrelated warning incallstack_test.go)go test ./...passes, including new unit tests for the collapse algorithm (idempotency, floor-bucketing, multi-bucket field replication), policy-generation tests for the newIPAddresses-consuming path, and the correctedTestGenerateNetworkPolicyfixtureshack/verify-codegen.shpasses (CRD field additions regenerated)Known follow-up (not fixed in this PR, flagged during investigation):
TestGenerateNetworkPolicy/real_duplicate_bug_testis flaky due tohash()usingencoding/gobover a struct containing a map — Go's randomized map iteration order occasionally produces different hashes for the same logical rule. Unrelated to this PR's changes.🤖 Generated with Claude Code
AI-skills: oh-my-claudecode:plan,oh-my-claudecode:team | cmds: /oh-my-claudecode:deep-interview
Summary by CodeRabbit
IPAddressesfield for ingress/egress peers (including CIDRs and wildcard).IPBlockpeers during rule merging and improves determinism of generated policies.