allowing CIDRs, wildcards and Plural in IP and DNS#806
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdds wildcard-aware IP (CIDR, ChangesNetwork Wildcard Matching Feature
Sequence Diagram(s)sequenceDiagram
participant CELRule as CEL Rule (network.go)
participant matchIPField
participant matchDNSField
participant networkmatch as networkmatch package
participant ProjectedCache as ProjectedContainerProfile
CELRule->>ProjectedCache: wasAddressInEgress(address)
ProjectedCache-->>CELRule: cp.EgressAddresses (Values + Patterns)
CELRule->>matchIPField: matchIPField(&cp.EgressAddresses, address)
matchIPField->>matchIPField: exact match in Values
matchIPField->>matchIPField: net.ParseIP canonicalize
matchIPField->>networkmatch: MatchIP(Values+Patterns, address)
networkmatch-->>matchIPField: bool
matchIPField-->>CELRule: bool
CELRule->>ProjectedCache: isDomainInEgress(domain)
ProjectedCache-->>CELRule: cp.EgressDomains (Values + Patterns)
CELRule->>matchDNSField: matchDNSField(&cp.EgressDomains, domain)
matchDNSField->>matchDNSField: normalize trailing dot
matchDNSField->>networkmatch: MatchDNS(Values+Patterns, domain)
networkmatch-->>matchDNSField: bool
matchDNSField-->>CELRule: bool
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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 |
|
@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. |
|
On your atomicity question for the full SBOB set: the tier structure you've described looks well-scoped. A few cross-PR coupling observations specific to this PR's position: Tier 3 coupling checks (PR
Tier 4 coupling risk
General atomicity verdict No cross-tier coupling jumps are apparent from the tier descriptions alone. The re-review will surface any remaining intra-PR issues in ✅ Actions performedReview triggered.
|
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Requesting changes for one blocking issue: this PR imports github.com/kubescape/storage/pkg/registry/file/networkmatch, but the repo still pins github.com/kubescape/storage v0.0.258, which does not contain that package. As a result, go test ./pkg/rulemanager/cel/libraries/networkneighborhood/... fails before the new tests can run.
I left the inline comment on the import; once the storage dependency is wired to a version/replace that includes networkmatch, I’m happy to re-review.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked the updated branch: the original missing-package blocker is fixed, but the new github.com/kubescape/storage replace still leaves this PR unbuildable.
On a fresh checkout, go test ./pkg/rulemanager/cel/libraries/networkneighborhood/... now stops with go: updates to go.mod needed; to update it: go mod tidy, so go.mod/go.sum are not in a buildable state as committed.
If I let Go resolve modules (GOFLAGS=-mod=mod go build ./pkg/rulemanager/cel/libraries/networkneighborhood/...), the dependency graph shifts and the build then fails in github.com/containerd/containerd/oci with cannot use limit (variable of type int64) as *int64 value in assignment.
So the branch still needs a storage version / dependency set that builds cleanly in node-agent as committed, without requiring local module rewrites.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
entlein
force-pushed
the
upstream-pr/sbob-network
branch
from
7529d82 to
16a5dd3
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: 6
🧹 Nitpick comments (1)
pkg/objectcache/containerprofilecache/projection.go (1)
316-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake merged
IPAddressesordering deterministic.Rebuilding from a map makes the resulting slice order non-deterministic. Sorting here avoids unstable output and reconciliation churn.
Proposed patch
@@ if len(user.IPAddresses) > 0 { ipSet := make(map[string]struct{}) for _, ip := range merged.IPAddresses { ipSet[ip] = struct{}{} } for _, ip := range user.IPAddresses { ipSet[ip] = struct{}{} } merged.IPAddresses = make([]string, 0, len(ipSet)) for ip := range ipSet { merged.IPAddresses = append(merged.IPAddresses, ip) } + sort.Strings(merged.IPAddresses) }@@ import ( + "sort" ... )🤖 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/objectcache/containerprofilecache/projection.go` around lines 316 - 327, The merged.IPAddresses slice is rebuilt from the ipSet map by iterating and appending values, which produces non-deterministic ordering since Go maps have random iteration order. After the loop that rebuilds merged.IPAddresses from the ipSet map, sort the resulting slice using the sort package to ensure consistent, deterministic ordering. This will prevent unstable output and reconciliation churn.
🤖 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/rulemanager/cel/libraries/networkneighborhood/wildcard_test.go`:
- Around line 266-301: The test case "in-range miss" on line 286 in
TestWasAddressPortProtocolInEgress_PortWrapRejected expects true for port 444
even though the neighbor is defined with a literal IP address "10.1.2.3" and
only has port 443 configured. The address-only fallback behavior (which ignores
port matching) should only apply to CIDR-only neighbors, not to literal-IP
neighbors. Change the expected value for the "in-range miss" test case from true
to false, and apply the same correction to similar test cases in the other
location indicated by the "Also applies to" comment.
In `@tests/component_test.go`:
- Around line 1690-1701: The waitAlerts helper function currently returns as
soon as GetAlerts becomes callable, without verifying that the expected alert
condition is actually present, which causes it to miss asynchronously-delivered
alerts or incorrectly pass when no alerts have yet arrived. Modify the
waitAlerts function to accept a predicate parameter that validates the expected
alert condition, then update the require.Eventually polling logic to return true
only when both GetAlerts succeeds AND the predicate is satisfied by the returned
alerts, ensuring the function waits for the actual expected alert state rather
than just API availability.
- Line 2062: The function `Test_28_UserDefinedNetworkNeighborhood` is declared
multiple times in the same package, which causes a Go compilation error since
duplicate top-level function names are not allowed. The documentation headers
immediately preceding the second and third occurrences of this function name
indicate they should be named `Test_29_SignedApplicationProfile` instead. Rename
the function declarations at the second and third occurrences from
`Test_28_UserDefinedNetworkNeighborhood` to `Test_29_SignedApplicationProfile`
to match their preceding documentation headers and resolve the naming conflict.
In `@tests/resources/known-network-neighborhood.yaml`:
- Around line 25-49: The NetworkNeighborhood fixture in
known-network-neighborhood.yaml contains label mismatches and is unused by any
test cases. Either remove the entire fixture file if it is not needed, or update
the matchLabels field to use app: curl-28 instead of app: curl-fusioncore-28-1,
update the workload-name label to match the actual deployment name, and ensure
the kubescape.io/user-defined-network label matches what test cases are actually
referencing. Additionally, verify that at least one test case or deployment
fixture explicitly references this network via the
kubescape.io/user-defined-network label to confirm it is being used.
In `@tests/resources/network-wildcards/README.md`:
- Around line 13-18: The README in the network-wildcards directory claims that
"the runtime matcher also defends by silently dropping it on read" but this
defensive behavior is not actually implemented in the matcher code. Remove this
unsupported claim from the NOTE section about 14-recursive-star-rejected.yaml,
specifically the phrase about the runtime matcher silently dropping the `**`
token. Keep only the documented fact that rejection happens at the apiserver
level via admission validation, and remove the fallback claim about the broken
admission layer not accidentally letting it through since that runtime defense
does not exist.
In `@tests/resources/nginx-user-defined-deployment.yaml`:
- Around line 22-25: The curl container in the
nginx-user-defined-deployment.yaml manifest lacks a securityContext definition,
which leaves privilege escalation and root execution settings dependent on the
image defaults. Add a securityContext block to the curl container specification
that includes security settings such as runAsNonRoot set to true and
allowPrivilegeEscalation set to false to ensure the test fixture has explicit
security constraints independent of the external image configuration.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/projection.go`:
- Around line 316-327: The merged.IPAddresses slice is rebuilt from the ipSet
map by iterating and appending values, which produces non-deterministic ordering
since Go maps have random iteration order. After the loop that rebuilds
merged.IPAddresses from the ipSet map, sort the resulting slice using the sort
package to ensure consistent, deterministic ordering. This will prevent unstable
output and reconciliation churn.
🪄 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: f862fecc-7e8b-4c18-856e-efe2af303aeb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
go.modpkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/v1/mock.gopkg/rulemanager/cel/libraries/networkneighborhood/fixtures_test.gopkg/rulemanager/cel/libraries/networkneighborhood/network.gopkg/rulemanager/cel/libraries/networkneighborhood/wildcard_test.gotests/chart/templates/node-agent/default-rules.yamltests/component_test.gotests/resources/known-network-neighborhood.yamltests/resources/network-wildcards/01-literal-ipv4.yamltests/resources/network-wildcards/02-literal-ipv6.yamltests/resources/network-wildcards/03-cidr-ipv4.yamltests/resources/network-wildcards/04-cidr-ipv6.yamltests/resources/network-wildcards/05-any-ip-sentinel.yamltests/resources/network-wildcards/06-any-as-cidr.yamltests/resources/network-wildcards/07-mixed-ip-list.yamltests/resources/network-wildcards/08-deprecated-ipaddress.yamltests/resources/network-wildcards/09-dns-literal.yamltests/resources/network-wildcards/10-dns-leading-wildcard.yamltests/resources/network-wildcards/11-dns-mid-ellipsis.yamltests/resources/network-wildcards/12-dns-trailing-star.yamltests/resources/network-wildcards/13-dns-trailing-dot-normalisation.yamltests/resources/network-wildcards/14-recursive-star-rejected.yamltests/resources/network-wildcards/15-egress-and-ingress.yamltests/resources/network-wildcards/16-egress-none.yamltests/resources/network-wildcards/17-realistic-stripe-api.yamltests/resources/network-wildcards/18-cluster-dns-via-mid-ellipsis.yamltests/resources/network-wildcards/19-port-protocol-with-cidr.yamltests/resources/network-wildcards/20-multi-container-mixed-wildcards.yamltests/resources/network-wildcards/README.mdtests/resources/nginx-user-defined-deployment.yaml
| func TestWasAddressPortProtocolInEgress_PortWrapRejected(t *testing.T) { | ||
| lib := buildLibWithContainer(t, []v1beta1.NetworkNeighbor{ | ||
| { | ||
| IPAddress: "10.1.2.3", | ||
| Ports: []v1beta1.NetworkPort{ | ||
| {Name: "TCP-443", Protocol: "TCP", Port: ptr.To(int32(443))}, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| // See TestWasAddressPortProtocolInEgress_WithCIDR for the | ||
| // port/protocol regression note. The port-range guard ([0, 65535]) | ||
| // still applies — what's gone is port-specific matching: any in-range | ||
| // port matches if the address matches. | ||
| cases := []struct { | ||
| name string | ||
| port int64 | ||
| want bool | ||
| }{ | ||
| {"in-range hit", 443, true}, | ||
| {"in-range miss", 444, true}, // was: false (port mismatch). Now matches: address-only after projection-v1. | ||
| {"wrap-to-443 rejected", 4294967739, false}, // (1<<32)+443 — range guard fires | ||
| {"negative rejected", -1, false}, // range guard fires | ||
| {"too-large rejected", 65536, false}, // range guard fires | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| res := lib.wasAddressPortProtocolInEgress( | ||
| types.String("cid"), types.String("10.1.2.3"), | ||
| types.Int(tc.port), types.String("TCP"), | ||
| ) | ||
| res = cache.ConvertProfileNotAvailableErrToBool(res, false) | ||
| assert.Equal(t, types.Bool(tc.want), res) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Scope the address-only fallback to CIDR-only neighbors.
Line 286 currently expects a literal-IP neighbor (IPAddress: "10.1.2.3") to match even on a wrong port (444). That codifies loss of port/protocol enforcement outside the stated CIDR-only v1 fallback and weakens network policy guarantees.
Suggested expectation once runtime behavior is corrected
- {"in-range miss", 444, true}, // was: false (port mismatch). Now matches: address-only after projection-v1.
+ {"in-range miss", 444, false}, // literal IP should retain port/protocol enforcementAlso applies to: 343-386
🤖 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/rulemanager/cel/libraries/networkneighborhood/wildcard_test.go` around
lines 266 - 301, The test case "in-range miss" on line 286 in
TestWasAddressPortProtocolInEgress_PortWrapRejected expects true for port 444
even though the neighbor is defined with a literal IP address "10.1.2.3" and
only has port 443 configured. The address-only fallback behavior (which ignores
port matching) should only apply to CIDR-only neighbors, not to literal-IP
neighbors. Change the expected value for the "in-range miss" test case from true
to false, and apply the same correction to similar test cases in the other
location indicated by the "Also applies to" comment.
| waitAlerts := func(t *testing.T, ns string) []testutils.Alert { | ||
| t.Helper() | ||
| var alerts []testutils.Alert | ||
| var err error | ||
| require.Eventually(t, func() bool { | ||
| alerts, err = testutils.GetAlerts(ns) | ||
| return err == nil | ||
| }, 60*time.Second, 5*time.Second, "must be able to fetch alerts") | ||
| // Extra settle time for remaining alerts. | ||
| time.Sleep(10 * time.Second) | ||
| alerts, _ = testutils.GetAlerts(ns) | ||
| return alerts |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Poll for the expected alert condition, not just alert-fetch success.
waitAlerts returns as soon as GetAlerts is callable, then samples once after 10 seconds. Since R0005/R0011 delivery is asynchronous, positive checks can miss late alerts and no-alert checks can pass before an alert arrives. Make the helper accept a predicate, or add separate waitForRule / assertNoRuleWithin helpers.
Example direction
- waitAlerts := func(t *testing.T, ns string) []testutils.Alert {
+ waitForAlerts := func(t *testing.T, ns string, ok func([]testutils.Alert) bool) []testutils.Alert {
t.Helper()
var alerts []testutils.Alert
var err error
require.Eventually(t, func() bool {
alerts, err = testutils.GetAlerts(ns)
- return err == nil
- }, 60*time.Second, 5*time.Second, "must be able to fetch alerts")
- // Extra settle time for remaining alerts.
- time.Sleep(10 * time.Second)
- alerts, _ = testutils.GetAlerts(ns)
+ return err == nil && ok(alerts)
+ }, 60*time.Second, 5*time.Second, "expected alerts condition was not met")
return alerts
}📝 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.
| waitAlerts := func(t *testing.T, ns string) []testutils.Alert { | |
| t.Helper() | |
| var alerts []testutils.Alert | |
| var err error | |
| require.Eventually(t, func() bool { | |
| alerts, err = testutils.GetAlerts(ns) | |
| return err == nil | |
| }, 60*time.Second, 5*time.Second, "must be able to fetch alerts") | |
| // Extra settle time for remaining alerts. | |
| time.Sleep(10 * time.Second) | |
| alerts, _ = testutils.GetAlerts(ns) | |
| return alerts | |
| waitForAlerts := func(t *testing.T, ns string, ok func([]testutils.Alert) bool) []testutils.Alert { | |
| t.Helper() | |
| var alerts []testutils.Alert | |
| var err error | |
| require.Eventually(t, func() bool { | |
| alerts, err = testutils.GetAlerts(ns) | |
| return err == nil && ok(alerts) | |
| }, 60*time.Second, 5*time.Second, "expected alerts condition was not met") | |
| return alerts | |
| } |
🤖 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 1690 - 1701, The waitAlerts helper
function currently returns as soon as GetAlerts becomes callable, without
verifying that the expected alert condition is actually present, which causes it
to miss asynchronously-delivered alerts or incorrectly pass when no alerts have
yet arrived. Modify the waitAlerts function to accept a predicate parameter that
validates the expected alert condition, then update the require.Eventually
polling logic to return true only when both GetAlerts succeeds AND the predicate
is satisfied by the returned alerts, ensuring the function waits for the actual
expected alert state rather than just API availability.
| // pushes it to storage, verifies the signature survives the round-trip, | ||
| // deploys a pod referencing the signed profile, and asserts that executing | ||
| // a binary NOT in the profile fires R0001 (Unexpected process launched). | ||
| func Test_28_UserDefinedNetworkNeighborhood(t *testing.T) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate top-level test declarations and whether Test_29 has an implementation.
rg -n '^func Test_28_UserDefinedNetworkNeighborhood\(' tests/component_test.go
rg -n '^func Test_29_SignedApplicationProfile\(' tests/component_test.go || trueRepository: kubescape/node-agent
Length of output: 353
🏁 Script executed:
sed -n '2050,2070p' tests/component_test.go
sed -n '2540,2560p' tests/component_test.goRepository: kubescape/node-agent
Length of output: 2036
Remove the duplicate Test_28_UserDefinedNetworkNeighborhood declarations (lines 2062 and 2552).
Go does not allow multiple top-level functions with the same name in one package. The file currently declares Test_28_UserDefinedNetworkNeighborhood three times (lines 1572, 2062, 2552), which will cause a compilation error. Additionally, lines 2054–2061 and 2544–2551 contain documentation headers for Test_29_SignedApplicationProfile, but the function declarations at lines 2062 and 2552 are incorrectly named Test_28. Keep the first Test_28 implementation (line 1572) and either rename the second occurrence at line 2062 to match its documentation header as Test_29_SignedApplicationProfile, or remove it entirely if it is a duplicate.
🤖 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 2062, The function
`Test_28_UserDefinedNetworkNeighborhood` is declared multiple times in the same
package, which causes a Go compilation error since duplicate top-level function
names are not allowed. The documentation headers immediately preceding the
second and third occurrences of this function name indicate they should be named
`Test_29_SignedApplicationProfile` instead. Rename the function declarations at
the second and third occurrences from `Test_28_UserDefinedNetworkNeighborhood`
to `Test_29_SignedApplicationProfile` to match their preceding documentation
headers and resolve the naming conflict.
| labels: | ||
| kubescape.io/workload-api-group: apps | ||
| kubescape.io/workload-api-version: v1 | ||
| kubescape.io/workload-kind: Deployment | ||
| kubescape.io/workload-name: curl-fusioncore-deployment | ||
| kubescape.io/workload-namespace: "{{NAMESPACE}}" | ||
| spec: | ||
| matchLabels: | ||
| app: curl-fusioncore-28-1 | ||
| containers: | ||
| - name: curl | ||
| ingress: [] | ||
| egress: | ||
| - dns: fusioncore.ai. | ||
| dnsNames: | ||
| - fusioncore.ai. | ||
| identifier: a5e64ff1db824089b1706ac872303e55075f92cf6a652b5272f06c3a2b9e8d10 | ||
| ipAddress: 162.0.217.171 | ||
| namespaceSelector: null | ||
| podSelector: null | ||
| ports: | ||
| - name: TCP-80 | ||
| port: 80 | ||
| protocol: TCP | ||
| type: external |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --no-heading -S 'curl-fusioncore-deployment|curl-fusioncore-28-1|curl-28-overlay|fusioncore-network|user-defined-network|user-defined-profile' tests pkgRepository: kubescape/node-agent
Length of output: 1144
🏁 Script executed:
cat tests/resources/nginx-user-defined-deployment.yamlRepository: kubescape/node-agent
Length of output: 932
🏁 Script executed:
sed -n '1580,1600p' tests/component_test.goRepository: kubescape/node-agent
Length of output: 939
🏁 Script executed:
sed -n '2070,2090p' tests/component_test.goRepository: kubescape/node-agent
Length of output: 939
🏁 Script executed:
head -n 25 tests/resources/known-network-neighborhood.yamlRepository: kubescape/node-agent
Length of output: 915
🏁 Script executed:
rg -n 'fusioncore-network|curl-fusioncore-28-1' tests/component_test.goRepository: kubescape/node-agent
Length of output: 158
🏁 Script executed:
rg -r 'fusioncore-network|curl-fusioncore-28-1' tests/Repository: kubescape/node-agent
Length of output: 158
This NetworkNeighborhood fixture is not used by any test case and has unresolved label mismatches.
The known-network-neighborhood.yaml fixture expects pods with app: curl-fusioncore-28-1 and workload name curl-fusioncore-deployment, but the actual deployment in this patch uses app: curl-28 and is referenced via kubescape.io/user-defined-profile: curl-28-overlay. Additionally, no test case or deployment references this fixture via the kubescape.io/user-defined-network: fusioncore-network label it provides. Either remove this unused fixture or update it to match the curl-28 deployment's labels.
🤖 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/known-network-neighborhood.yaml` around lines 25 - 49, The
NetworkNeighborhood fixture in known-network-neighborhood.yaml contains label
mismatches and is unused by any test cases. Either remove the entire fixture
file if it is not needed, or update the matchLabels field to use app: curl-28
instead of app: curl-fusioncore-28-1, update the workload-name label to match
the actual deployment name, and ensure the kubescape.io/user-defined-network
label matches what test cases are actually referencing. Additionally, verify
that at least one test case or deployment fixture explicitly references this
network via the kubescape.io/user-defined-network label to confirm it is being
used.
| **Note on `14-recursive-star-rejected.yaml`:** this fixture is intentionally | ||
| **rejected at admission** — it carries `dnsNames: ["**"]` to demonstrate | ||
| that the recursive-wildcard token is invalid v0.0.2 syntax. Don't `kubectl | ||
| apply` it; the apiserver will return a 400. The runtime matcher also | ||
| defends by silently dropping it on read, so a broken admission layer | ||
| won't accidentally let it through. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --no-heading -S 'recursive.*wildcard|drop.*read|rejected at admission' pkg/rulemanager/cel/libraries/networkneighborhood tests/resources/network-wildcards
rg -n --no-heading -F '**' pkg/rulemanager/cel/libraries/networkneighborhood tests/resources/network-wildcardsRepository: kubescape/node-agent
Length of output: 2875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, map the networkneighborhood package structure
echo "=== Package structure ==="
fd -t f '\.go$' pkg/rulemanager/cel/libraries/networkneighborhood | head -20
# Search for the matcher implementation that handles dnsNames
echo "=== Search for matcher implementation ==="
rg -n 'type.*[Mm]atcher|func.*Match|dnsNames' pkg/rulemanager/cel/libraries/networkneighborhood -A 3 | head -50
# Look for handling of ** specifically in the matcher logic
echo "=== Search for ** filtering/handling logic ==="
rg -n -B 5 -A 5 'len.*==.*2|validate|sanitize|filter' pkg/rulemanager/cel/libraries/networkneighborhood/*.go | head -80Repository: kubescape/node-agent
Length of output: 3430
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the main implementation files
echo "=== network.go structure ==="
ast-grep outline pkg/rulemanager/cel/libraries/networkneighborhood/network.go
echo -e "\n=== nn.go structure ==="
ast-grep outline pkg/rulemanager/cel/libraries/networkneighborhood/nn.goRepository: kubescape/node-agent
Length of output: 2135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== matchDNSField implementation ==="
sed -n '62,85p' pkg/rulemanager/cel/libraries/networkneighborhood/network.go
echo -e "\n=== Full network.go for context ==="
wc -l pkg/rulemanager/cel/libraries/networkneighborhood/network.go
echo -e "\n=== Search for any validation or filtering logic ==="
rg -n 'Test.*Validate|func.*Validate|strings\.Contains.*\*\*|len.*==.*2' pkg/rulemanager/cel/libraries/networkneighborhood/ -A 3Repository: kubescape/node-agent
Length of output: 1691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for TestValidate_NetworkProfileEntries ==="
rg -n 'TestValidate.*NetworkProfileEntries' pkg/rulemanager/cel/libraries/networkneighborhood/ -A 30
echo -e "\n=== Search in storage/validation packages ==="
fd -t f 'validat' pkg/ --exec rg -l 'NetworkNeighbor|NetworkProfile' {} \;
echo -e "\n=== Direct search for validation of ** in dnsNames ==="
rg -n 'dnsNames.*\*\*|recursive.*wildcard.*valid|validate.*dnsNames' . -A 3 -B 3Repository: kubescape/node-agent
Length of output: 3579
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for storage REST strategy or validation ==="
rg -n 'storage.*REST|REST.*strategy|apiserver.*validat' . -t go -t yaml
echo -e "\n=== Look for where networking objects are loaded into cache ==="
rg -n 'objectcache|ObjectCache.*NetworkNeighbor|\.Values\[' pkg/rulemanager/cel/libraries/networkneighborhood/ -B 2 -A 2
echo -e "\n=== Check if there's any filtering when data is read ==="
rg -n 'for.*range.*Values|for.*range.*dnsNames|silently.*drop|filter' pkg/rulemanager/cel/libraries/networkneighborhood/ -A 3Repository: kubescape/node-agent
Length of output: 25676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for ProjectedField definition and population ==="
rg -n 'type ProjectedField|func.*ProjectedField' pkg/ -A 5
echo -e "\n=== Search for where dnsNames are loaded/populated ==="
rg -n 'dnsNames|DNSNames' pkg/ -B 3 -A 3 | head -100
echo -e "\n=== Look for objectcache internals ==="
fd -t f 'objectcache.*\.go' pkg/objectcache/ | head -10Repository: kubescape/node-agent
Length of output: 12010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read projection_apply.go to understand field projection ==="
cat -n pkg/objectcache/containerprofilecache/projection_apply.go | head -150
echo -e "\n=== Search for any filtering of ** in the file ==="
rg -n 'strings\.Contains|Filter|filterEntries|\*\*' pkg/objectcache/containerprofilecache/projection_apply.go -B 2 -A 2Repository: kubescape/node-agent
Length of output: 6294
Remove the unsupported claim about runtime enforcement.
The README states the runtime matcher "silently drops ** on read," but this fallback is not implemented in the matcher code. The matcher functions pass all entries through without filtering ** tokens, and field projection similarly retains all values. Only admission-time rejection is confirmed by code comments and fixtures. Narrow the documentation to state only that rejection happens at the apiserver level via admission validation.
🤖 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/network-wildcards/README.md` around lines 13 - 18, The README
in the network-wildcards directory claims that "the runtime matcher also defends
by silently dropping it on read" but this defensive behavior is not actually
implemented in the matcher code. Remove this unsupported claim from the NOTE
section about 14-recursive-star-rejected.yaml, specifically the phrase about the
runtime matcher silently dropping the `**` token. Keep only the documented fact
that rejection happens at the apiserver level via admission validation, and
remove the fallback claim about the broken admission layer not accidentally
letting it through since that runtime defense does not exist.
| containers: | ||
| - name: curl | ||
| image: docker.io/curlimages/curl@sha256:08e466006f0860e54fc299378de998935333e0e130a15f6f98482e9f8dab3058 | ||
| command: ["sleep", "infinity"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Lock down the test pod security context.
The manifest leaves privilege escalation and root execution to the image defaults. Add a minimal securityContext here so the fixture doesn't depend on external image settings.
Suggested hardening
containers:
- name: curl
image: docker.io/curlimages/curl@sha256:08e466006f0860e54fc299378de998935333e0e130a15f6f98482e9f8dab3058
+ securityContext:
+ allowPrivilegeEscalation: false
+ runAsNonRoot: true
command: ["sleep", "infinity"]📝 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.
| containers: | |
| - name: curl | |
| image: docker.io/curlimages/curl@sha256:08e466006f0860e54fc299378de998935333e0e130a15f6f98482e9f8dab3058 | |
| command: ["sleep", "infinity"] | |
| containers: | |
| - name: curl | |
| image: docker.io/curlimages/curl@sha256:08e466006f0860e54fc299378de998935333e0e130a15f6f98482e9f8dab3058 | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| runAsNonRoot: true | |
| command: ["sleep", "infinity"] |
🧰 Tools
🪛 Checkov (3.3.1)
[medium] 1-25: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-25: Minimize the admission of root containers
(CKV_K8S_23)
🤖 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/nginx-user-defined-deployment.yaml` around lines 22 - 25, The
curl container in the nginx-user-defined-deployment.yaml manifest lacks a
securityContext definition, which leaves privilege escalation and root execution
settings dependent on the image defaults. Add a securityContext block to the
curl container specification that includes security settings such as
runAsNonRoot set to true and allowPrivilegeEscalation set to false to ensure the
test fixture has explicit security constraints independent of the external image
configuration.
Source: Linters/SAST tools
|
Component tests: 21/21 passing ✅ Validated on a coherent node-agent + storage image pair built from this branch and its matching storage network changes. Full matrix green, including the new network end-to-end test:
|
Signed-off-by: entlein <einentlein@gmail.com>
Resolves matthyx review on network.go:12 (2026-05-27). Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
how do we lose these "strings" package once a day? Signed-off-by: entlein <einentlein@gmail.com>
we also dont need a triplication of Test28 Signed-off-by: entlein <einentlein@gmail.com>
entlein
force-pushed
the
upstream-pr/sbob-network
branch
from
278fb58 to
08def6d
Compare
2 participants
Overview
network-wildcards: DNS leading/trailing/mid + CIDR + IPAddresses + "*" sentinel
Additional Information
CEL
networkneighborhoodhelpers now queryprojected fields with wildcard/CIDR/DNS-wildcard support and the v0.0.2
spec semantics (port/protocol checks degrade to address-only in v1
when only a CIDR is declared).
*(RFC 4592 single-label wildcard)*(subdomain-prefix wildcard)⋯(interior dynamic segment)*any-IP sentinelIPAddresseslist (deprecated single-IP field) preserved on thedecode path
How to Test
20 declarative fixture YAMLs under
tests/resources/network-wildcards/pin the wildcard contract end-to-end:
*any-IP and*as CIDRIPAddressesfield path⋯, trailing-**rejection⋯Has sister PR in storage
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores