test(component): add Test_34 NetworkNeighbors CIDR collapse e2e to Matthias CIDR Collapse#861
Conversation
📝 WalkthroughWalkthroughAdded end-to-end component coverage for NetworkNeighborhood CIDR collapsing. The test configures collapse thresholds, generates IPv4 and IPv6 traffic with learner workloads, reads persisted results dynamically, and validates CIDR splitting and removal of covered host entries. ChangesNetworkNeighborhood CIDR collapse
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentTest
participant DynamicClient
participant LearnerDeployments
participant NetworkNeighborhood
ComponentTest->>DynamicClient: apply CollapseConfiguration
ComponentTest->>LearnerDeployments: deploy CIDR probe workloads
LearnerDeployments->>NetworkNeighborhood: generate observed egress IPs
ComponentTest->>NetworkNeighborhood: wait for completion and read object
NetworkNeighborhood-->>ComponentTest: collapsed CIDRs and remaining host entries
ComponentTest->>ComponentTest: assert expected IPv4 and IPv6 coverage
🚥 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 |
![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
🧹 Nitpick comments (1)
tests/component_test.go (1)
3325-3339: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCollect the collapsed payload after
Eventuallysucceeds.
require.Eventuallyruns the condition in a goroutine, whilefinalRawis later read in the test goroutine after passing the predicate. Capture the GET/Update result when the condition finally returnstrue, or re-GET once the poll succeeds, instead of letting a condition goroutine mutate captured test state.🤖 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 `@tests/component_test.go` around lines 3325 - 3339, Avoid mutating the captured finalRaw variable inside the require.Eventually predicate. Let the predicate only verify that the GET payload contains wantS3CIDR and wantBroadCIDR, then re-GET the resource after Eventually succeeds and marshal that result into finalRaw in the test goroutine.
🤖 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 `@tests/component_test.go`:
- Around line 3302-3321: Add cleanup to the test using NewRandomNamespace so
both the created NetworkNeighborhood and ns.Name namespace are deleted
automatically via t.Cleanup or defer. Register cleanup after each resource is
successfully created, using the appropriate storageClient NetworkNeighborhoods
and Namespaces operations, while preserving the existing test behavior.
---
Nitpick comments:
In `@tests/component_test.go`:
- Around line 3325-3339: Avoid mutating the captured finalRaw variable inside
the require.Eventually predicate. Let the predicate only verify that the GET
payload contains wantS3CIDR and wantBroadCIDR, then re-GET the resource after
Eventually succeeds and marshal that result into finalRaw in the test goroutine.
🪄 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: f0420f63-cb6e-4c21-b4a5-b2ab07e74ebc
📒 Files selected for processing (1)
tests/component_test.go
End-to-end component test for CIDR-based collapsing of NetworkNeighbor entries (storage-side PreSave path). Models a curl client with two external destinations — an S3-style endpoint clustered in a single /24 and a broader endpoint spanning a /16 — and asserts each group collapses to its covering CIDR block. Reads the result via the dynamic client so the test compiles against storage types lacking the plural ipAddresses field and passes only when storage implements the collapse. Signed-off-by: entlein <einentlein@gmail.com>
…+ dump on failure Storage wires SetCollapseSettings(collapseSettingsFromCRD), so with no CR present the effective network threshold isn't the assumed 50 default and the groups never collapsed. Apply an explicit CollapseConfiguration (default) with networkIPGroupThreshold=5, and replace the silent Eventually with a manual poll that logs the stored NetworkNeighborhood if collapse never appears, so a failing run shows whether the collapse ran with unexpected CIDRs or not at all. Signed-off-by: entlein <einentlein@gmail.com>
e4c55ae to
b63fc26
Compare
Signed-off-by: entlein <einentlein@gmail.com>
…pected NN as a fixutre Signed-off-by: entlein <einentlein@gmail.com>
…dpoints AFTER the collapsconfig was changed Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/resources/networkneighbors-cidr-fanout.yaml (1)
17-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRun all learner fixtures with a restricted security context.
These
ncloops do not need root or privilege escalation, yet every fixture defaults to both. They will also be rejected by Pod Security Restricted enforcement. AddrunAsNonRoot, a non-root UID,RuntimeDefaultseccomp,allowPrivilegeEscalation: false, and dropped capabilities consistently.
tests/resources/networkneighbors-cidr-fanout.yaml#L17-L25: add pod/container security contexts.tests/resources/networkneighbors-cidr-spread.yaml#L17-L26: add pod/container security contexts.tests/resources/networkneighbors-s3-27.yaml#L17-L23: add pod/container security contexts.tests/resources/networkneighbors-s3-28.yaml#L17-L23: add pod/container security contexts.tests/resources/networkneighbors-scattered.yaml#L17-L24: add pod/container security contexts.tests/resources/networkneighbors-v6-124.yaml#L17-L23: add pod/container security contexts.🤖 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 `@tests/resources/networkneighbors-cidr-fanout.yaml` around lines 17 - 25, Apply restricted pod and container security contexts to the learner fixtures in tests/resources/networkneighbors-cidr-fanout.yaml:17-25, tests/resources/networkneighbors-cidr-spread.yaml:17-26, tests/resources/networkneighbors-s3-27.yaml:17-23, tests/resources/networkneighbors-s3-28.yaml:17-23, tests/resources/networkneighbors-scattered.yaml:17-24, and tests/resources/networkneighbors-v6-124.yaml:17-23. Configure runAsNonRoot, a non-root UID, RuntimeDefault seccomp, allowPrivilegeEscalation false, and dropped capabilities consistently at the appropriate pod/container levels.Source: Linters/SAST tools
tests/resources/networkneighbors-cidr-spread.yaml (1)
17-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the remaining BusyBox learner fixtures.
tests/resources/networkneighbors-cidr-spread.yaml,tests/resources/networkneighbors-s3-27.yaml,tests/resources/networkneighbors-s3-28.yaml,tests/resources/networkneighbors-scattered.yaml, andtests/resources/networkneighbors-v6-124.yamlstill use mutableimage: busybox; align them with the existingbusybox:1.36fixture.🤖 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 `@tests/resources/networkneighbors-cidr-spread.yaml` around lines 17 - 26, Replace the mutable busybox image reference with busybox:1.36 in the container fixtures at tests/resources/networkneighbors-cidr-spread.yaml (17-26), tests/resources/networkneighbors-s3-27.yaml (17-23), tests/resources/networkneighbors-s3-28.yaml (17-23), tests/resources/networkneighbors-scattered.yaml (17-24), and tests/resources/networkneighbors-v6-124.yaml (17-23); make no other fixture changes.
🤖 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 `@tests/component_test.go`:
- Around line 3380-3382: Update the cleanup around applyCollapseFloor to
snapshot whether the default CollapseConfiguration exists and retain its
original spec before mutation. In the t.Cleanup callback, restore the saved spec
when the resource pre-existed, and delete it only when this test created it;
preserve the existing dynamic client and ccCollapseGVR usage.
---
Nitpick comments:
In `@tests/resources/networkneighbors-cidr-fanout.yaml`:
- Around line 17-25: Apply restricted pod and container security contexts to the
learner fixtures in tests/resources/networkneighbors-cidr-fanout.yaml:17-25,
tests/resources/networkneighbors-cidr-spread.yaml:17-26,
tests/resources/networkneighbors-s3-27.yaml:17-23,
tests/resources/networkneighbors-s3-28.yaml:17-23,
tests/resources/networkneighbors-scattered.yaml:17-24, and
tests/resources/networkneighbors-v6-124.yaml:17-23. Configure runAsNonRoot, a
non-root UID, RuntimeDefault seccomp, allowPrivilegeEscalation false, and
dropped capabilities consistently at the appropriate pod/container levels.
In `@tests/resources/networkneighbors-cidr-spread.yaml`:
- Around line 17-26: Replace the mutable busybox image reference with
busybox:1.36 in the container fixtures at
tests/resources/networkneighbors-cidr-spread.yaml (17-26),
tests/resources/networkneighbors-s3-27.yaml (17-23),
tests/resources/networkneighbors-s3-28.yaml (17-23),
tests/resources/networkneighbors-scattered.yaml (17-24), and
tests/resources/networkneighbors-v6-124.yaml (17-23); make no other fixture
changes.
🪄 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: 069c2cbd-97db-467c-bb35-0b5e85edfd2a
📒 Files selected for processing (7)
tests/component_test.gotests/resources/networkneighbors-cidr-fanout.yamltests/resources/networkneighbors-cidr-spread.yamltests/resources/networkneighbors-s3-27.yamltests/resources/networkneighbors-s3-28.yamltests/resources/networkneighbors-scattered.yamltests/resources/networkneighbors-v6-124.yaml
| k8sClient := k8sinterface.NewKubernetesApi() | ||
| dyn := dynamic.NewForConfigOrDie(k8sClient.K8SConfig) | ||
| t.Cleanup(func() { _ = dyn.Resource(ccCollapseGVR).Delete(context.Background(), "default", metav1.DeleteOptions{}) }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore a pre-existing CollapseConfiguration.
applyCollapseFloor updates default when it already exists, but Line 3382 always deletes it. Running this test can therefore remove cluster configuration owned by the environment or another test. Snapshot whether default existed before mutation; restore its previous spec on cleanup, and delete it only when this test created it.
🤖 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 `@tests/component_test.go` around lines 3380 - 3382, Update the cleanup around
applyCollapseFloor to snapshot whether the default CollapseConfiguration exists
and retain its original spec before mutation. In the t.Cleanup callback, restore
the saved spec when the resource pre-existed, and delete it only when this test
created it; preserve the existing dynamic client and ccCollapseGVR usage.
Collapse benchmarkThe component run above is green (
The |
1 participant
End-to-end component test for CIDR-based collapsing of NetworkNeighbor entries (storage-side PreSave path). Models a curl client with two external destinations — an S3-style endpoint clustered in a single /24 and a broader endpoint spanning a /16 — and asserts each group collapses to its covering CIDR block. Reads the result via the dynamic client so the test compiles against storage types lacking the plural ipAddresses field and passes only when storage implements the collapse.
How to Test
this is only a test file, it does not touch the prod code-base
Depends on
kubescape/storage#348 and kubescape/storage#349
Summary by CodeRabbit