ci(component): DO NOT MERGE - temporary CI run of Test_34 for #861 - #867
Closed
matthyx wants to merge 8 commits into
Closed
ci(component): DO NOT MERGE - temporary CI run of Test_34 for #861#867matthyx wants to merge 8 commits into
matthyx wants to merge 8 commits into
Conversation
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>
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>
Signed-off-by: entlein <einentlein@gmail.com>
…ture Test_34_NetworkNeighborsCIDRCollapse was never added to the component-tests matrix, so nothing ran it. The storage-side collapse it exercises (kubescape/storage#348 + #349) shipped in storage v0.0.298, and kubescape/helm-charts#887 bumped the chart to that tag — which tests/scripts/storage-tag.sh reads from helm-charts main at CI time, so the deployed storage now carries the collapse and the test can pass. Also drops tests/resources/networkneighbors-cidr-fanout.yaml: it was added alongside the test but is not referenced by it (the test uses the s3-28, s3-27, scattered, cidr-spread and v6-124 fixtures). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Docs-exempt: CI matrix entry plus removal of an unreferenced test fixture; no production code or behavioral change Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
added
the
ai-assisted
📝 WalkthroughWalkthroughAdds ChangesCIDR collapse component testing
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 3
🤖 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 @.github/workflows/component-tests.yaml:
- Around line 76-77: Update the component-test matrix entry containing
Test_34_NetworkNeighborsCIDRCollapse to explicitly select the PR#348 storage
image/tag used by tests/scripts/storage-tag.sh, rather than relying on the
generic upstream Kubernetes Operator values.yaml; keep
Test_32_NetworkNeighborsCIDRCollapse unchanged.
In `@tests/component_test.go`:
- Line 3387: Update the test cleanup around applyCollapseFloor to snapshot the
pre-existing default collapse configuration before modification, then restore
that object during cleanup; only delete the resource when the test created it.
- Around line 3429-3435: Update the IPv6 assertion around collectLearntCollapse
and withPrefixes so the single-stack skip occurs only when v6CIDRs contains
neither collapsed CIDRs nor bare IPv6 host entries. Preserve the existing
expected /124 assertion when any IPv6 data was learned, including uncollapsed
hosts, rather than discarding those entries before deciding to skip.
🪄 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: 23cc3c93-fd58-4916-9cc9-90057dcba8cc
📒 Files selected for processing (7)
.github/workflows/component-tests.yamltests/component_test.gotests/resources/networkneighbors-cidr-spread.yamltests/resources/networkneighbors-s3-27.yamltests/resources/networkneighbors-s3-28.yamltests/resources/networkneighbors-scattered.yamltests/resources/networkneighbors-v6-124.yaml
Comment on lines
+76
to
+77
| Test_32_UnexpectedProcessArguments, | ||
| Test_34_NetworkNeighborsCIDRCollapse |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a -t f '^storage-tag\.sh$' tests
rg -n -C 3 'storage-tag|STORAGE_TAG|storage.image.tag|348|ipAddresses' tests .githubRepository: kubescape/node-agent
Length of output: 25982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow matrix and component flags =="
sed -n '1,130p' .github/workflows/component-tests.yaml | cat -n
echo
echo "== storage tag script =="
cat -n tests/scripts/storage-tag.sh
python3 - <<'PY'
from urllib.request import urlopen
base = "https://raw.githubusercontent.com/kubescape/helm-charts/main/charts/kubescape-operator"
for path in ["main/values.yaml", "main/charts/kubescape-operator/values.yaml"]:
url = f"{base}/{path}"
try:
data = urlopen(url, timeout=20).read().decode()
except Exception as e:
print(f"{url}: fetch failed {e}")
continue
print(f"--- {url} ---")
for i, line in enumerate(data.splitlines(), 1):
if "storage:" in line or "image:" in line or "kubescape-node-agent-apiserver" in line or "sha256:" in line or "ipAddresses" in line:
print(f"{i:4}: {line}")
PYRepository: kubescape/node-agent
Length of output: 7797
Pin Test_34_NetworkNeighborsCIDRCollapse to a PR#348 storage build.
The component-test matrix installs storage via tests/scripts/storage-tag.sh, which just reads .storage.image.tag from the generic upstream Kubernetes Operator values.yaml. That matrix entry needs an explicit PR#348 storage image/tag, otherwise it can run against upstream storage without the required ipAddresses schema.
🤖 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 @.github/workflows/component-tests.yaml around lines 76 - 77, Update the
component-test matrix entry containing Test_34_NetworkNeighborsCIDRCollapse to
explicitly select the PR#348 storage image/tag used by
tests/scripts/storage-tag.sh, rather than relying on the generic upstream
Kubernetes Operator values.yaml; keep Test_32_NetworkNeighborsCIDRCollapse
unchanged.
|
|
||
| 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 collapse configuration instead of deleting it.
When default already exists, applyCollapseFloor updates it, but cleanup unconditionally deletes it. Snapshot and restore the prior object; delete only if 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` at line 3387, Update the test cleanup around
applyCollapseFloor to snapshot the pre-existing default collapse configuration
before modification, then restore that object during cleanup; only delete the
resource when the test created it.
Comment on lines
+3429
to
+3435
| v6CIDRs, _ := collectLearntCollapse(t, dyn, v6) | ||
| if v6got := withPrefixes(v6CIDRs, "2606:4700:0:1:"); len(v6got) == 0 { | ||
| t.Log("no IPv6 egress learnt (single-stack cluster) — skipping the v6 assertion") | ||
| } else { | ||
| assert.Equal(t, []string{"2606:4700:0:1::/124"}, v6got, | ||
| "a fully-observed Cloudflare v6 /124 must collapse to exactly that /124") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat uncollapsed IPv6 hosts as a single-stack skip.
A failed IPv6 collapse can leave ipAddress entries, but they are discarded here and the test passes as “single-stack.” Skip only when neither CIDRs nor bare IPv6 hosts were learnt.
Proposed fix
- v6CIDRs, _ := collectLearntCollapse(t, dyn, v6)
- if v6got := withPrefixes(v6CIDRs, "2606:4700:0:1:"); len(v6got) == 0 {
+ v6CIDRs, v6Bare := collectLearntCollapse(t, dyn, v6)
+ v6got := withPrefixes(v6CIDRs, "2606:4700:0:1:")
+ v6bare := withPrefixes(v6Bare, "2606:4700:0:1:")
+ if len(v6got) == 0 && len(v6bare) == 0 {
t.Log("no IPv6 egress learnt (single-stack cluster) — skipping the v6 assertion")
} else {
+ assert.Empty(t, v6bare, "no bare IPv6 hosts may remain after collapse")
assert.Equal(t, []string{"2606:4700:0:1::/124"}, v6got,📝 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
| v6CIDRs, _ := collectLearntCollapse(t, dyn, v6) | |
| if v6got := withPrefixes(v6CIDRs, "2606:4700:0:1:"); len(v6got) == 0 { | |
| t.Log("no IPv6 egress learnt (single-stack cluster) — skipping the v6 assertion") | |
| } else { | |
| assert.Equal(t, []string{"2606:4700:0:1::/124"}, v6got, | |
| "a fully-observed Cloudflare v6 /124 must collapse to exactly that /124") | |
| } | |
| v6CIDRs, v6Bare := collectLearntCollapse(t, dyn, v6) | |
| v6got := withPrefixes(v6CIDRs, "2606:4700:0:1:") | |
| v6bare := withPrefixes(v6Bare, "2606:4700:0:1:") | |
| if len(v6got) == 0 && len(v6bare) == 0 { | |
| t.Log("no IPv6 egress learnt (single-stack cluster) — skipping the v6 assertion") | |
| } else { | |
| assert.Empty(t, v6bare, "no bare IPv6 hosts may remain after collapse") | |
| assert.Equal(t, []string{"2606:4700:0:1::/124"}, v6got, | |
| "a fully-observed Cloudflare v6 /124 must collapse to exactly that /124") | |
| } |
🤖 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 3429 - 3435, Update the IPv6 assertion
around collectLearntCollapse and withPrefixes so the single-stack skip occurs
only when v6CIDRs contains neither collapsed CIDRs nor bare IPv6 host entries.
Preserve the existing expected /124 assertion when any IPv6 data was learned,
including uncollapsed hosts, rather than discarding those entries before
deciding to skip.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Contributor
Author
|
CI run complete and green: all 23 component-tests legs passed, including |
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
Throwaway PR opened solely to run the component-test CI for #861, which cannot run it itself. Nothing here is intended for merge or review; it will be closed as soon as the run reports, and #861 remains the PR of record. This branch is a byte-identical mirror of the head of #861 (7ab815f).
DO NOT MERGE, DO NOT REVIEW
This does not supersede #861.
Why this exists
#861 is opened from a fork (k8sstormcenter/node-agent). GitHub withholds repository secrets from pull_request runs originating from forks, so build-and-push-image fails at "Login to Quay.io" with "Username and password required". Because component-tests declares needs: build-and-push-image, the entire matrix is skipped. That has happened on every run of #861 since 2026-07-23, so Test_34_NetworkNeighborsCIDRCollapse has never executed in CI. Pushing the same commits to a base-repo branch makes secrets available, so the matrix can run.
What is being validated
Test_34_NetworkNeighborsCIDRCollapse exercises the storage-side CIDR collapse (kubescape/storage#348 and #349), released in storage v0.0.298. kubescape/helm-charts#887 bumped the chart to that tag, and tests/scripts/storage-tag.sh reads it from helm-charts main at CI time, so the deployed storage now carries the collapse code.
Notes
Refs #861
PR-structure-exempt: temporary CI-only PR, closed as soon as the run reports; no code is proposed for merge
AI-skills: none
Summary by CodeRabbit
Tests
Test Resources