allowing CIDRs, wildcards and Plural in IP and DNS#324
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds list-form network neighbor IP fields, introduces IP and DNS matcher and validation helpers with tests and benchmarks, and applies the new validation rules during ChangesNetwork neighbor matching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
entlein
changed the title
🧩 Analysis chain
🏁 Script executed: Length of output: 21242
Atomicity Confirmation
Cross-PR Coupling Flags🔴 File-Level Conflicts (merge order matters)1. node-agent#808 ↔ node-agent#809 — 2. node-agent#808 ↔ node-agent#812 — 3. node-agent#807 ↔ node-agent#811 — 🟡 Ordering Dependencies (no file conflicts, but merge sequence required)4. storage#323 → storage#325 5. storage#324 → node-agent#806 6. storage#325 → node-agent#808 Recommended Merge Sequencegraph TD
A[storage#322] --> E[storage#326 umbrella]
B[storage#323] --> C[storage#325] --> E
D[storage#324] --> F[node-agent#806] --> K[node-agent#810 umbrella]
C --> G[node-agent#808]
H[node-agent#811] --> I[node-agent#807] --> K
J[node-agent#812] --> G
G --> L[node-agent#809] --> K
A --> E
D --> E
H --> K
J --> K
F --> K
In short: the three intra-node-agent file conflicts ( ✅ Actions performedReview triggered.
|
| // IPAddresses is the v0.0.2 list-form replacement for IPAddress. | ||
| // Each entry MAY be a literal IP, a CIDR (a.b.c.d/n), or the "*" sentinel. | ||
| // See pkg/registry/file/networkmatch for matcher semantics. | ||
| IPAddresses []string `json:"ipAddresses,omitempty" protobuf:"bytes,9,rep,name=ipAddresses"` |
There was a problem hiding this comment.
Blocking: IPAddresses is added here, but the generated protobuf/conversion/deepcopy code is unchanged, so the field is silently dropped on real storage paths. The new TestNetworkNeighbor_IPAddresses_ProtobufRoundtrip already fails (go test ./pkg/apis/softwarecomposition/v1beta1). Please regenerate generated.pb.go, conversion code, and deepcopy code for this field before merge.
There was a problem hiding this comment.
This has now been addressed and the PR compiles standalone
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Blocking: IPAddresses does not survive marshal/unmarshal today. The new protobuf roundtrip test fails because generated.pb.go, zz_generated.conversion.go, and zz_generated.deepcopy.go were not regenerated for the new field, so the API loses data on storage/conversion paths. Please regenerate the generated artifacts (and ideally add conversion/deepcopy regression coverage) before merging.
entlein
force-pushed
the
upstream-pr/sbob-network
branch
2 times, most recently
from
6473d36 to
46f37d3
Compare
|
the failed test -> can we rerun- it passed on my arm64 3x in a row 🤷🏻♀️ |
Three matthyx blockers (2026-05-27): (1) tamper_alert.go:28 — imports pkg/signature and pkg/signature/profiles which ship in node-agent#809 (not yet merged). Adds minimum-surface stubs (pkg/signature/stub.go, pkg/signature/profiles/stub.go) so the PR compiles standalone. With IsSigned returning false, the tamper path short-circuits and never invokes Verify — the no-op stub is behavior- safe: signed-profile detection is dormant until kubescape#809 lands and replaces these files with the real implementation. (2) projection_apply.go:265 — NetworkNeighbor.IPAddresses missing. (3) mock.go:202 — same IPAddresses issue. Both (2) and (3) resolved by replacing kubescape/storage with k8sstormcenter/storage's upstream-pr/sbob-network tip 46f37d32 (sibling of kubescape/storage#324). Removed when kubescape#324 merges and a release ships IPAddresses.
|
Rechecked on current head The original protobuf/codegen blocker is fixed - Still blocking: |
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
…nal state Signed-off-by: entlein <einentlein@gmail.com>
NetworkNeighbor.IPAddresses (added on this branch) was missing hand-edited entries in the Marshal/Size/String/Unmarshal stanzas of generated.pb.go, the proto declaration, both conversion functions, and the internal+v1beta1 deepcopy. Result: TestNetworkNeighbor_IPAddresses_ ProtobufRoundtrip failed and the field was silently dropped on every real storage write. This patch adds the missing codec for field 9 (repeated string, wire tag 0x4a), the .proto declaration, the conversion stanzas in both directions, and the deepcopy slice copy in both type-system layers. Codegen pipeline is not run as part of build on this fork (the protoc image is x86_64-only); hand-edit follows the proven recipe from commit 0d83e2b / ad60a5b. Resolves matthyx review on network_types.go:68 (2026-05-27). Signed-off-by: entlein <einentlein@gmail.com>
entlein
force-pushed
the
upstream-pr/sbob-network
branch
from
46f37d3 to
f4e5f53
Compare
![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)
pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go (1)
356-423: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd ingress-path cases to pin the full admission contract.
Line 365 builds only
Egressneighbors, so this suite never asserts ingress field paths. Add malformed ingress IP/DNS cases (e.g.,spec.containers[0].ingress[0].ipAddresses[0],...dnsNames[0]) to protect the “ingress + egress” validation guarantee.🤖 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/softwarecomposition/networkneighborhood/strategy_test.go` around lines 356 - 423, The test cases in the NetworkNeighborhood validation suite currently only cover egress neighbors and their corresponding field paths. Add test cases that validate ingress neighbors to complete the admission contract coverage. Create additional test cases (similar to the existing ones like "single malformed IP" and "single malformed CIDR") that populate the Ingress field of NetworkNeighborhoodContainer instead of Egress, and update their wantPaths to reference the ingress paths (e.g., spec.containers[0].ingress[0].ipAddresses[0] and spec.containers[0].ingress[0].dnsNames[0]). This may require modifying the makeNN function to accept a parameter indicating whether to use ingress or egress, or creating a separate makeNN variant for ingress validation.
🤖 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 `@pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go`:
- Around line 464-480: In the ValidateUpdate test, the code currently passes the
same `bad` object as both the old and new parameters to the ValidateUpdate
method call, which prevents the test from properly validating that the method is
checking the correct operand. Instead, create two distinct fixtures: use a valid
NetworkNeighborhood object for the old parameter and the invalid `bad` object
for the new parameter when calling ValidateUpdate, ensuring the test explicitly
validates that the new object is being checked rather than masking potential
bugs in validation logic.
---
Nitpick comments:
In `@pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go`:
- Around line 356-423: The test cases in the NetworkNeighborhood validation
suite currently only cover egress neighbors and their corresponding field paths.
Add test cases that validate ingress neighbors to complete the admission
contract coverage. Create additional test cases (similar to the existing ones
like "single malformed IP" and "single malformed CIDR") that populate the
Ingress field of NetworkNeighborhoodContainer instead of Egress, and update
their wantPaths to reference the ingress paths (e.g.,
spec.containers[0].ingress[0].ipAddresses[0] and
spec.containers[0].ingress[0].dnsNames[0]). This may require modifying the
makeNN function to accept a parameter indicating whether to use ingress or
egress, or creating a separate makeNN variant for ingress validation.
🪄 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
Run ID: ebc0e04c-fd41-4f5a-a2e8-4716c9ca8593
⛔ Files ignored due to path filters (1)
pkg/apis/softwarecomposition/v1beta1/generated.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
pkg/apis/softwarecomposition/network_types.gopkg/apis/softwarecomposition/v1beta1/generated.protopkg/apis/softwarecomposition/v1beta1/network_types.gopkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.gopkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.gopkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.gopkg/apis/softwarecomposition/zz_generated.deepcopy.gopkg/registry/file/networkmatch/README.mdpkg/registry/file/networkmatch/bench_test.gopkg/registry/file/networkmatch/doc.gopkg/registry/file/networkmatch/match_dns.gopkg/registry/file/networkmatch/match_dns_test.gopkg/registry/file/networkmatch/match_ip.gopkg/registry/file/networkmatch/match_ip_test.gopkg/registry/file/networkmatch/validate.gopkg/registry/file/networkmatch/validate_test.gopkg/registry/softwarecomposition/networkneighborhood/strategy.gopkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
| bad := &softwarecomposition.NetworkNeighborhood{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-nn", | ||
| Namespace: "default", | ||
| Annotations: map[string]string{helpers.CompletionMetadataKey: "complete", helpers.StatusMetadataKey: "ready"}, | ||
| }, | ||
| Spec: softwarecomposition.NetworkNeighborhoodSpec{ | ||
| Containers: []softwarecomposition.NetworkNeighborhoodContainer{{ | ||
| Name: "c", | ||
| Egress: []softwarecomposition.NetworkNeighbor{ | ||
| {IPAddresses: []string{"not-an-ip"}, DNSNames: []string{"**"}}, | ||
| }, | ||
| }}, | ||
| }, | ||
| } | ||
| s := NetworkNeighborhoodStrategy{} | ||
| errs := s.ValidateUpdate(context.TODO(), bad, bad) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use distinct old/new objects in ValidateUpdate test.
Line 480 passes bad as both new and old objects, which can mask bugs where ValidateUpdate validates the wrong operand. Use oldValid and newInvalid fixtures so the test pins new-object validation explicitly.
Suggested test adjustment
- bad := &softwarecomposition.NetworkNeighborhood{
+ newBad := &softwarecomposition.NetworkNeighborhood{
ObjectMeta: metav1.ObjectMeta{
Name: "test-nn",
Namespace: "default",
Annotations: map[string]string{helpers.CompletionMetadataKey: "complete", helpers.StatusMetadataKey: "ready"},
},
Spec: softwarecomposition.NetworkNeighborhoodSpec{
Containers: []softwarecomposition.NetworkNeighborhoodContainer{{
Name: "c",
Egress: []softwarecomposition.NetworkNeighbor{
{IPAddresses: []string{"not-an-ip"}, DNSNames: []string{"**"}},
},
}},
},
}
+ oldGood := &softwarecomposition.NetworkNeighborhood{
+ ObjectMeta: newBad.ObjectMeta,
+ Spec: softwarecomposition.NetworkNeighborhoodSpec{
+ Containers: []softwarecomposition.NetworkNeighborhoodContainer{{
+ Name: "c",
+ Egress: []softwarecomposition.NetworkNeighbor{
+ {IPAddresses: []string{"10.0.0.0/8"}, DNSNames: []string{"api.example.com."}},
+ },
+ }},
+ },
+ }
s := NetworkNeighborhoodStrategy{}
- errs := s.ValidateUpdate(context.TODO(), bad, bad)
+ errs := s.ValidateUpdate(context.TODO(), newBad, oldGood)📝 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.
| bad := &softwarecomposition.NetworkNeighborhood{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-nn", | |
| Namespace: "default", | |
| Annotations: map[string]string{helpers.CompletionMetadataKey: "complete", helpers.StatusMetadataKey: "ready"}, | |
| }, | |
| Spec: softwarecomposition.NetworkNeighborhoodSpec{ | |
| Containers: []softwarecomposition.NetworkNeighborhoodContainer{{ | |
| Name: "c", | |
| Egress: []softwarecomposition.NetworkNeighbor{ | |
| {IPAddresses: []string{"not-an-ip"}, DNSNames: []string{"**"}}, | |
| }, | |
| }}, | |
| }, | |
| } | |
| s := NetworkNeighborhoodStrategy{} | |
| errs := s.ValidateUpdate(context.TODO(), bad, bad) | |
| newBad := &softwarecomposition.NetworkNeighborhood{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-nn", | |
| Namespace: "default", | |
| Annotations: map[string]string{helpers.CompletionMetadataKey: "complete", helpers.StatusMetadataKey: "ready"}, | |
| }, | |
| Spec: softwarecomposition.NetworkNeighborhoodSpec{ | |
| Containers: []softwarecomposition.NetworkNeighborhoodContainer{{ | |
| Name: "c", | |
| Egress: []softwarecomposition.NetworkNeighbor{ | |
| {IPAddresses: []string{"not-an-ip"}, DNSNames: []string{"**"}}, | |
| }, | |
| }}, | |
| }, | |
| } | |
| oldGood := &softwarecomposition.NetworkNeighborhood{ | |
| ObjectMeta: newBad.ObjectMeta, | |
| Spec: softwarecomposition.NetworkNeighborhoodSpec{ | |
| Containers: []softwarecomposition.NetworkNeighborhoodContainer{{ | |
| Name: "c", | |
| Egress: []softwarecomposition.NetworkNeighbor{ | |
| {IPAddresses: []string{"10.0.0.0/8"}, DNSNames: []string{"api.example.com."}}, | |
| }, | |
| }}, | |
| }, | |
| } | |
| s := NetworkNeighborhoodStrategy{} | |
| errs := s.ValidateUpdate(context.TODO(), newBad, oldGood) |
🤖 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/softwarecomposition/networkneighborhood/strategy_test.go` around
lines 464 - 480, In the ValidateUpdate test, the code currently passes the same
`bad` object as both the old and new parameters to the ValidateUpdate method
call, which prevents the test from properly validating that the method is
checking the correct operand. Instead, create two distinct fixtures: use a valid
NetworkNeighborhood object for the old parameter and the invalid `bad` object
for the new parameter when calling ValidateUpdate, ensuring the test explicitly
validates that the new object is being checked rather than masking potential
bugs in validation logic.
Signed-off-by: entlein <einentlein@gmail.com>
Three matthyx blockers (2026-05-27): (1) tamper_alert.go:28 — imports pkg/signature and pkg/signature/profiles which ship in node-agent#809 (not yet merged). Adds minimum-surface stubs (pkg/signature/stub.go, pkg/signature/profiles/stub.go) so the PR compiles standalone. With IsSigned returning false, the tamper path short-circuits and never invokes Verify — the no-op stub is behavior- safe: signed-profile detection is dormant until kubescape#809 lands and replaces these files with the real implementation. (2) projection_apply.go:265 — NetworkNeighbor.IPAddresses missing. (3) mock.go:202 — same IPAddresses issue. Both (2) and (3) resolved by replacing kubescape/storage with k8sstormcenter/storage's upstream-pr/sbob-network tip 46f37d32 (sibling of kubescape/storage#324). Removed when kubescape#324 merges and a release ships IPAddresses. Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build/protoc.Dockerfile (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialPin the
protocbuilder image to a specific digest for reproducible builds.The current
golang:1.26-trixietag is a floating reference that can silently change the Go compiler version or base OS patch, potentially altering generated code output. Pin theFROMinstruction to the resolved digest below to ensure deterministic builds:Show diff
FROM --platform=$BUILDPLATFORM golang@sha256:76a29248dedcd75870e95cbd90cc8cb356db082404ac7d3a5803f276c3ba79c9 AS builder🤖 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 `@build/protoc.Dockerfile` at line 1, The protoc builder image is using a floating golang:1.26-trixie tag, which can change over time and break reproducible builds. Update the FROM instruction in the builder stage to pin the base image to the provided sha256 digest so the protoc generation environment stays deterministic.
🤖 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 `@api-rules/sample_apiserver_violation_exceptions.list`:
- Line 48: The exception entry for NetworkNeighbor.IPAddresses should be removed
by defining the list semantics directly on the API field instead of adding a new
list-type exception. Update the NetworkNeighbor type to include the appropriate
+listType marker for IPAddresses, regenerate the API artifacts, and then delete
this exception from the sample_apiserver_violation_exceptions list.
---
Nitpick comments:
In `@build/protoc.Dockerfile`:
- Line 1: The protoc builder image is using a floating golang:1.26-trixie tag,
which can change over time and break reproducible builds. Update the FROM
instruction in the builder stage to pin the base image to the provided sha256
digest so the protoc generation environment stays deterministic.
🪄 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
Run ID: 514517b2-2010-4f37-ad70-65995c5f63b6
⛔ Files ignored due to path filters (3)
pkg/generated/applyconfiguration/softwarecomposition/v1beta1/execcalls.gois excluded by!**/generated/**pkg/generated/applyconfiguration/softwarecomposition/v1beta1/networkneighbor.gois excluded by!**/generated/**pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (3)
api-rules/sample_apiserver_violation_exceptions.listbuild/protoc.Dockerfilepkg/apis/softwarecomposition/v1beta1/generated.proto
| API rule violation: list_type_missing,github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1,Match,MatchDetails | ||
| API rule violation: list_type_missing,github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1,Match,RelatedVulnerabilities | ||
| API rule violation: list_type_missing,github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1,NetworkNeighbor,DNSNames | ||
| API rule violation: list_type_missing,github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1,NetworkNeighbor,IPAddresses |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid adding a new list-type exception for IPAddresses.
For a new API field, define the intended list semantics on NetworkNeighbor.IPAddresses instead of expanding the exception list. Use the appropriate +listType marker in the API type and regenerate, then remove this exception.
🤖 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 `@api-rules/sample_apiserver_violation_exceptions.list` at line 48, The
exception entry for NetworkNeighbor.IPAddresses should be removed by defining
the list semantics directly on the API field instead of adding a new list-type
exception. Update the NetworkNeighbor type to include the appropriate +listType
marker for IPAddresses, regenerate the API artifacts, and then delete this
exception from the sample_apiserver_violation_exceptions list.
matthyx
force-pushed
the
upstream-pr/sbob-network
branch
from
1769507 to
5cdffeb
Compare
2 participants
Sorry, we do not accept changes directly against this repository. Please see
CONTRIBUTING.md for information on where and how to contribute instead.
Summary by CodeRabbit
NetworkNeighbor.IPAddresses(list) support for literal IPs, CIDRs, and*, including wildcard-aware matching.⋯and*rules).IPAddress; useIPAddressesinstead.