Skip to content

allowing CIDRs, wildcards and Plural in IP and DNS#324

Open
entlein wants to merge 4 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-network
Open

allowing CIDRs, wildcards and Plural in IP and DNS#324
entlein wants to merge 4 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-network

Conversation

@entlein

@entlein entlein commented May 15, 2026

Copy link
Copy Markdown
Contributor

Sorry, we do not accept changes directly against this repository. Please see
CONTRIBUTING.md for information on where and how to contribute instead.

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@entlein, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 55 minutes and 58 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4b843b18-035b-48d6-9482-e290ef9ee3ec

📥 Commits

Reviewing files that changed from the base of the PR and between 98649ae and 46f37d3.

⛔ Files ignored due to path filters (1)
  • pkg/apis/softwarecomposition/v1beta1/generated.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (18)
  • pkg/apis/softwarecomposition/network_types.go
  • pkg/apis/softwarecomposition/v1beta1/generated.proto
  • pkg/apis/softwarecomposition/v1beta1/network_types.go
  • pkg/apis/softwarecomposition/v1beta1/network_types_protobuf_test.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go
  • pkg/apis/softwarecomposition/v1beta1/zz_generated.deepcopy.go
  • pkg/apis/softwarecomposition/zz_generated.deepcopy.go
  • pkg/registry/file/networkmatch/README.md
  • pkg/registry/file/networkmatch/bench_test.go
  • pkg/registry/file/networkmatch/doc.go
  • pkg/registry/file/networkmatch/match_dns.go
  • pkg/registry/file/networkmatch/match_dns_test.go
  • pkg/registry/file/networkmatch/match_ip.go
  • pkg/registry/file/networkmatch/match_ip_test.go
  • pkg/registry/file/networkmatch/validate.go
  • pkg/registry/file/networkmatch/validate_test.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy.go
  • pkg/registry/softwarecomposition/networkneighborhood/strategy_test.go
✨ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@entlein entlein changed the title Upstream pr/sbob network allowing CIDRs, wildcards and Plural in IP and DNS May 15, 2026
@entlein

entlein commented May 16, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map:

Tier 0 (opens):

Tier 2 (execs):

Tier 3 (network):

Tier 4 (tamper/sign):

Umbrellas:

Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed.

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
🧩 Analysis chain

@entlein: Triggering a fresh review of this PR (#324networkmatch package, Tier 3 network). While the review runs, let me pull metadata on the relevant PRs to cross-check coupling.


🏁 Script executed:

#!/bin/bash
# Fetch details of the current PR
gh pr view 324 --repo kubescape/storage --json title,body,files,baseRefName,headRefName,state

echo "---"
# Fetch related storage PRs
for n in 322 323 325 326; do
  echo "=== storage#$n ==="
  gh pr view $n --repo kubescape/storage --json title,state,files 2>/dev/null | jq '{title,state,files:[.files[].path]}'
  echo ""
done

echo "---"
# Fetch related node-agent PRs
for n in 805 806 807 808 809 810 811 812; do
  echo "=== node-agent#$n ==="
  gh pr view $n --repo kubescape/node-agent --json title,state,files 2>/dev/null | jq '{title,state,files:[.files[].path]}'
  echo ""
done

Length of output: 21242


@entlein: Review triggered. Here is the full atomicity and coupling analysis based on actual file diffs across all PRs.


Atomicity Confirmation

PR Tier Verdict Notes
storage#322 Exec ✅ Atomic Single file pair (compare_exec_args.go + test)
storage#323 Opens ✅ Atomic Scoped to dynamicpathdetector/* + types.go
storage#324 Network Atomic New networkmatch/ package + network_types.go field additions + strategy wiring — no tier bleed
storage#325 Exec/CRD ✅ Atomic CollapseConfig CRD + processors; collapse_config_from_crd.go bridges to #323 types (expected)
storage#326 Umbrella ✅ Correct File union of #322+#323+#324+#325 confirmed
node-agent#805 Exec ✅ Atomic parse/ library + event_reporting only
node-agent#806 Network ✅ Atomic CEL consumer of storage#324 — correct tier
node-agent#807 Exec ⚠️ See below Touches open.go — overlap with #811
node-agent#808 Tamper/CRD ⚠️ See below tamper_alert.go + projection_apply.go overlap with #809 and #812
node-agent#809 Sign/Tamper ⚠️ See below tamper_alert.go overlap with #808
node-agent#810 Umbrella ✅ Correct File union of all node-agent PRs confirmed
node-agent#811 Opens ⚠️ See below open.go overlap with #807
node-agent#812 Exec/Proj ⚠️ See below projection_apply.go + projection_types.go overlap with #808

Cross-PR Coupling Flags

🔴 File-Level Conflicts (merge order matters)

1. node-agent#808 ↔ node-agent#809 — tamper_alert.go + tamper_alert_test.go
Both PRs modify the same two files. These are not independent — one must be rebased onto the other before the umbrella (#810) can merge cleanly.

2. node-agent#808 ↔ node-agent#812 — projection_apply.go + projection_types.go
#812 adds ExecsByPath composite-key to ProjectedContainerProfile; #808 restructures projection_apply.go. These will conflict at the hunk level. Suggested order: merge #812 first, then rebase #808 on top.

3. node-agent#807 ↔ node-agent#811 — open.go
#807 modifies open.go for R0040 args; #811 adds was_path_opened* helpers to the same file. One must be rebased on the other. Suggested order: merge #811 first (it's additive/new helpers), then rebase #807.

🟡 Ordering Dependencies (no file conflicts, but merge sequence required)

4. storage#323 → storage#325
collapse_config_from_crd.go in #325 references types introduced by #323's types.go (WildcardIdentifier, CollapseConfig API). Merge #323 before #325.

5. storage#324 → node-agent#806
node-agent#806's network.go CEL library consumes the networkmatch package from this PR. A go.mod bump pointing to the merged commit of storage#324 is required before node-agent#806 can merge.

6. storage#325 → node-agent#808
The projection overlay in #808 consumes the CollapseConfiguration CRD and client introduced in #325. Cross-repo go.mod bump required.


Recommended Merge Sequence

graph 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
Loading

In short: the three intra-node-agent file conflicts (#808↔#809, #808↔#812, #807↔#811) are the only remaining non-atomic coupling. Everything else is correctly sequenced by your tier map. This PR (#324) itself is clean and ready for review.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@matthyx matthyx moved this to WIP in KS PRs tracking May 26, 2026
// 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"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has now been addressed and the PR compiles standalone

@matthyx matthyx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 entlein force-pushed the upstream-pr/sbob-network branch from 0a8a0b8 to 6473d36 Compare May 27, 2026 16:27
entlein and others added 4 commits May 27, 2026 19:37
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 entlein force-pushed the upstream-pr/sbob-network branch from 6473d36 to 46f37d3 Compare May 27, 2026 17:37
@entlein

entlein commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

the failed test -> can we rerun- it passed on my arm64 3x in a row 🤷🏻‍♀️

entlein pushed a commit to k8sstormcenter/node-agent that referenced this pull request May 27, 2026
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.
@matthyx

matthyx commented May 28, 2026

Copy link
Copy Markdown
Contributor

Rechecked on current head 46f37d3.

The original protobuf/codegen blocker is fixed - go test ./pkg/apis/softwarecomposition/v1beta1 now passes.

Still blocking: pkg/apis/softwarecomposition/v1beta1/zz_generated.conversion.go updates only the internal->v1beta1 conversion. In autoConvert_v1beta1_NetworkNeighbor_To_softwarecomposition_NetworkNeighbor, IPAddresses is still missing, so reads still drop the field after deserialization. Please also add out.IPAddresses = *(*[]string)(unsafe.Pointer(&in.IPAddresses)) in that function before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: WIP

Development

Successfully merging this pull request may close these issues.

2 participants