tier-3 (network): documented bench coverage for matchIPField / matchDNSField / wasAddressPortProtocol#50
Open
entlein wants to merge 3 commits into
Open
tier-3 (network): documented bench coverage for matchIPField / matchDNSField / wasAddressPortProtocol#50entlein wants to merge 3 commits into
entlein wants to merge 3 commits into
Conversation
added 3 commits
May 19, 2026 21:31
…true) Addresses Matthias's upstream PR kubescape#811 blocking review (2026-05-19T10:35Z): Blocking: projectField() still keeps dynamic open paths only in cp.Opens.Patterns when cp.Opens.All == true. Removing the Patterns scan here makes ap.was_path_opened_with_suffix() / _with_prefix() return false for dynamic entries in pass-through mode, so rules that omit profileDataRequired.opens regress. If the old strings.HasPrefix/HasSuffix behavior on pattern text is too permissive, we still need a narrower fallback here instead of ignoring Patterns entirely. Context ------- Pre-PR-kubescape#811, CodeRabbit (PR #43 review on open.go:79) flagged that running strings.HasSuffix("/var/log/pods/*/volumes/...", "foo.log") returns false even though the pattern's concrete realisations might end in "foo.log" — a false negative. We addressed it by SKIPPING the Patterns scan in the All=true branch. Matthias's review reverses that: skipping is too aggressive and silently regresses rules that omit profileDataRequired.opens (which rely on the All=true pass-through behavior). Fix --- Narrower fallback — when Opens.All == true and the Values scan didn't match, scan Patterns with split semantics: patternConcreteSuffix(p) → literal text after the LAST wildcard segment in p; "" if p ends in a wildcard segment. patternConcretePrefix(p) → literal text before the FIRST wildcard segment in p; "" if p starts with a wildcard segment. Suffix/prefix matcher then: for each pattern: tail/head := patternConcrete{Suffix,Prefix}(pattern) if tail/head == "": return true // wildcard tail/head → permissive (concrete // realisations could match ANY suffix/prefix) if strings.HasSuffix/HasPrefix(tail/head, query): return true Boundary slashes are KEPT in the returned concrete suffix/prefix so queries with leading/trailing slashes match correctly (every concrete realisation has the boundary slash too). Tests ----- Updated previous "PatternsNotScanned" tests (which pinned the pre-kubescape#811 CR fix) to reflect Matthias's new contract: TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail pattern "/var/log/⋯/foo.log", query ".log" → true (tail "/foo.log" has suffix ".log") pattern "/var/log/⋯/foo.log", query ".txt" → false TestWasPathOpenedWithSuffix_PatternWildcardTail_Permissive pattern "/var/log/pods/*", any suffix → true TestWasPathOpenedWithPrefix_PatternsScannedWithConcreteHead pattern "/var/⋯/log/foo", query "/var/" → true (head "/var/" has prefix "/var/") pattern "/var/⋯/log/foo", query "/etc/" → false TestWasPathOpenedWithPrefix_PatternWildcardHead_Permissive pattern "*/run", any prefix → true TestPatternConcreteSuffix_AndPrefix pins the splitter helpers against 7 representative shapes (no_wildcards, trailing_star, leading_star, mid_ellipsis, both_mid, lone_star, lone_ellipsis). Benchmarks (new file: open_bench_test.go) ----------------------------------------- Helper itself — zero allocation across all shapes: BenchmarkPatternConcreteSuffix//var/log/⋯/foo.log 11.5 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/log/pods/* 10.0 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/log/foo.log 9.4 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix/* 1.6 ns/op 0 B/op 0 allocs/op BenchmarkPatternConcreteSuffix//var/⋯/log/⋯/foo.log 14.8 ns/op 0 B/op 0 allocs/op Full matcher (50 Values + 10 Patterns) — the Patterns scan adds ~20 ns on top of the Values-only path with no extra allocations (the steady 56 B/op / 3 allocs/op is pre-existing CEL value-wrapping cost, unrelated to this change): BenchmarkWasPathOpenedWithSuffix_AllMode/values_only 372 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithSuffix_AllMode/patterns_concrete 391 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithSuffix_AllMode/patterns_wildcard 393 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/values_only 351 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/patterns_concrete 395 ns/op 56 B/op 3 allocs/op BenchmarkWasPathOpenedWithPrefix_AllMode/patterns_wildcard 428 ns/op 56 B/op 3 allocs/op
Addresses the CodeRabbit PR #43 Major finding on `pkg/rulemanager/cel/libraries/applicationprofile/exec.go:144`, deferred from the NA #46 batch-1 PR body. Tracked there as: exec.go:144 — Exact path match should return false when args don't match, not fall through to pattern matching. Problem ------- Pre-fix `wasExecutedWithArgs` flow on exact-path hit: if pathStr in cp.Execs.Values: if pathStr in cp.ExecsByPath: if CompareExecArgs(...) → return true // else: FALL THROUGH to pattern matching else: return true (back-compat no-constraint) for pattern in cp.Execs.Patterns: ... The fall-through on `CompareExecArgs == false` would let a LOOSER pattern (e.g. `/usr/bin/*` with profileArgs `[*]`) shadow the exact-path entry's strict args constraint. A rule-author who lists `{Path: /usr/bin/curl, Args: [-X, GET]}` expects that ONLY GET requests pass — but if their profile also contains a permissive `/usr/bin/⋯` pattern, the fall-through silently allows DELETE/POST/… via the pattern. Fix --- When the exact path IS in Values AND ExecsByPath has an entry, the constraint result IS the answer for this path. Return it directly — no fall-through: if pathStr in cp.Execs.Values: if pathStr in cp.ExecsByPath: return CompareExecArgs(profileArgs, runtimeArgs) // else: back-compat no-constraint return true // path NOT in Values — try Patterns for pattern in cp.Execs.Patterns: ... Pattern-loop fall-through stays as-is — different patterns CAN encode different `(path, args)` shapes and a non-matching one shouldn't kill the loop. The asymmetry is intentional: exact-path-with-constraint is producer-authored authoritative, pattern-with-constraint is one candidate among many. Bench (`-count=3 -benchtime=200ms`) ----------------------------------- exact_path_args_match ~890 ns/op 2136 B/op 20 allocs/op exact_path_args_mismatch ~860 ns/op 2136 B/op 20 allocs/op pattern_path_args_match ~450 ns/op 360 B/op 13 allocs/op The exact-path mismatch case is no longer paying the pattern-loop cost it used to (and used to silently return true via the pattern shadow). The remaining alloc/op floor is pre-existing CEL value wrapping (matcher unchanged).
…AddressPortProtocol No code change to the matchers — main already has the correct matchIPField + matchDNSField shape (ProjectedField.All is explicitly NOT consulted as a short-circuit; see line 24-29 of network.go, which addresses CodeRabbit PR #43 R-NET-7). This PR captures the per-call cost of the network matchers as a documented baseline against future refactors. Numbers below are the median of 3 runs, count=3, benchtime=200ms, arm64. matchIPField (Values-hit / Patterns-CIDR / `*` sentinel / miss): values_only_hit 6.1 ns/op 0 B/op 0 allocs/op patterns_cidr_hit 263 ns/op 248 B/op 12 allocs/op any_sentinel 78 ns/op 80 B/op 2 allocs/op miss 47 ns/op 8 B/op 1 allocs/op matchDNSField (Values-exact / canon / Patterns-wildcard / miss): values_exact_hit 6.5 ns/op 0 B/op 0 allocs/op values_trailing_dot_canon 6.5 ns/op 0 B/op 0 allocs/op patterns_leading_wildcard_hit 234 ns/op 184 B/op 4 allocs/op miss 21 ns/op 0 B/op 0 allocs/op wasAddressPortProtocolInEgress (full CEL surface): hit_via_values 100 ns/op 120 B/op 8 allocs/op hit_via_cidr 269 ns/op 280 B/op 15 allocs/op miss 236 ns/op 256 B/op 13 allocs/op Known gap (NOT addressed in tier-3, deferred to projection-v2): wasAddressPortProtocolIn* currently ignores port + protocol. The matcher validates their shape (port range, protocol string type) but the actual match only consults the address via matchIPField. See the existing comment at network.go:164-166. Fixing this needs an AddressPortsByAddr projection slice on ProjectedField; that's a coordinated projection-side change and will land separately. CodeRabbit PR #43 noted this as a Major; the production impact is bounded because rules that genuinely care about (address, port, protocol) triples don't currently exist in the default rule set. The bench numbers show the CEL value-wrapping overhead (~50-100 ns, ~100 B, ~8 allocs per call) dominates the matcher cost — that's pre-existing and shared across all CEL libraries.
📝 WalkthroughWalkthroughThe PR improves CEL library matching behavior for application profile exec constraints and open path patterns, and adds comprehensive benchmarks for matcher performance across applicationprofile and networkneighborhood libraries. Core logic changes prevent argv mismatches from falling through to pattern matching and add wildcard pattern evaluation to suffix/prefix matchers. ChangesCEL Library Matching Improvements
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/rulemanager/cel/libraries/applicationprofile/open_test.go (1)
16-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove contradictory legacy test-contract text.
Line 16 through Line 29 still says
cp.Opens.Patternsmust not contribute to suffix/prefix answers, which conflicts with the new contract and assertions below. Please update or remove that stale section to keep the test contract unambiguous.🤖 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/applicationprofile/open_test.go` around lines 16 - 47, The test header in TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail contains stale/contradictory wording (lines describing cp.Opens.Patterns MUST NOT contribute to suffix/prefix answers) that conflicts with the assertions below and the new contract; remove or replace that legacy paragraph with a short, accurate statement that Patterns are scanned in Opens.All==true mode using the narrowed fallback rules (concrete tail/head matches via HasSuffix/HasPrefix and wildcard-edge patterns are permissive), so the test description matches the behavior exercised by TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail and the related TestOpenWithSuffixInProfile/TestOpenWithPrefixInProfile comments.
🤖 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/applicationprofile/exec_bench_test.go`:
- Around line 11-25: The comment says the benchmark header claims four shapes
including "no_match" but the implemented benchmark table omits that case; update
the doc to make them consistent: either remove "no_match" from the header or add
the missing "no_match" benchmark case to the table so the documented matrix
matches the actual measured cases in BenchmarkWasExecutedWithArgs (and any
related benchmark declarations in exec_bench_test.go), and mirror the same
change where the header appears again (lines referenced around the existing
benchmark description).
In `@pkg/rulemanager/cel/libraries/networkneighborhood/network_bench_test.go`:
- Around line 172-174: The ContainerProfileCache accessor on mockNNObjectCache
currently allocates a new &mockNNCPC{pcp: m.pcp} on every call which inflates
benchmark allocations; change the accessor to return a reusable instance instead
(e.g., store a single *mockNNCPC on mockNNObjectCache and return that) so
ContainerProfileCache() reuses the same mockNNCPC instead of creating a fresh
one each call; update mockNNObjectCache to hold the cached mockNNCPC and have
ContainerProfileCache() return that stored pointer (still wrapping m.pcp) to
eliminate per-call synthetic allocations.
---
Outside diff comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/open_test.go`:
- Around line 16-47: The test header in
TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail contains
stale/contradictory wording (lines describing cp.Opens.Patterns MUST NOT
contribute to suffix/prefix answers) that conflicts with the assertions below
and the new contract; remove or replace that legacy paragraph with a short,
accurate statement that Patterns are scanned in Opens.All==true mode using the
narrowed fallback rules (concrete tail/head matches via HasSuffix/HasPrefix and
wildcard-edge patterns are permissive), so the test description matches the
behavior exercised by
TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail and the related
TestOpenWithSuffixInProfile/TestOpenWithPrefixInProfile comments.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 36c1a01e-bf99-4d6e-a4f2-da48f666879c
📒 Files selected for processing (6)
pkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/applicationprofile/exec_bench_test.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.gopkg/rulemanager/cel/libraries/applicationprofile/open_test.gopkg/rulemanager/cel/libraries/networkneighborhood/network_bench_test.go
Comment on lines
+11
to
+25
| // BenchmarkWasExecutedWithArgs covers the R0040 hot path in the four | ||
| // representative shapes: | ||
| // | ||
| // - exact_path_args_match: literal path in Values + matching argv | ||
| // - exact_path_args_mismatch: literal path in Values + mismatching argv | ||
| // (post-tier-2 fix: returns false without | ||
| // falling through to pattern matching) | ||
| // - pattern_path_args_match: path absent from Values, matches a Pattern, | ||
| // ExecsByPath has the matching argv | ||
| // - no_match: path absent from Values AND no Pattern | ||
| // matches (terminal false) | ||
| // | ||
| // Reports allocs/op for the steady-state matcher. The CEL value-wrapping | ||
| // floor is unchanged by this PR — only the exec.go control flow changed | ||
| // (no fall-through on exact-path strict-args mismatch). |
There was a problem hiding this comment.
Keep the documented benchmark matrix consistent with the implemented cases.
The header says this benchmark covers four shapes, including no_match, but the table below intentionally omits that case. Since this PR is documenting bench coverage, that mismatch will mislead readers about what was actually measured.
Doc-only fix
-// BenchmarkWasExecutedWithArgs covers the R0040 hot path in the four
+// BenchmarkWasExecutedWithArgs covers the R0040 hot path in three
// representative shapes:
//
// - exact_path_args_match: literal path in Values + matching argv
// - exact_path_args_mismatch: literal path in Values + mismatching argv
// (post-tier-2 fix: returns false without
// falling through to pattern matching)
// - pattern_path_args_match: path absent from Values, matches a Pattern,
// ExecsByPath has the matching argv
-// - no_match: path absent from Values AND no Pattern
-// matches (terminal false)
//
// Reports allocs/op for the steady-state matcher. The CEL value-wrapping
// floor is unchanged by this PR — only the exec.go control flow changed
// (no fall-through on exact-path strict-args mismatch).
+//
+// The terminal `no_match` shape is intentionally omitted here because it
+// falls through to isExecInPodSpec, which would require extra object-cache
+// wiring and would measure more than the matcher hot path.Also applies to: 62-64
🤖 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/applicationprofile/exec_bench_test.go` around
lines 11 - 25, The comment says the benchmark header claims four shapes
including "no_match" but the implemented benchmark table omits that case; update
the doc to make them consistent: either remove "no_match" from the header or add
the missing "no_match" benchmark case to the table so the documented matrix
matches the actual measured cases in BenchmarkWasExecutedWithArgs (and any
related benchmark declarations in exec_bench_test.go), and mirror the same
change where the header appears again (lines referenced around the existing
benchmark description).
Comment on lines
+172
to
+174
| func (m *mockNNObjectCache) ContainerProfileCache() objectcache.ContainerProfileCache { | ||
| return &mockNNCPC{pcp: m.pcp} | ||
| } |
There was a problem hiding this comment.
Avoid synthetic allocation in the mock cache accessor.
At Line 173, returning a fresh &mockNNCPC{...} per call can inflate benchmark allocs/op with mock overhead rather than matcher cost.
Proposed fix
type mockNNObjectCache struct {
objectcache.ObjectCache
- pcp *objectcache.ProjectedContainerProfile
+ cpc objectcache.ContainerProfileCache
}
func (m *mockNNObjectCache) ContainerProfileCache() objectcache.ContainerProfileCache {
- return &mockNNCPC{pcp: m.pcp}
+ return m.cpc
} pcp := &objectcache.ProjectedContainerProfile{
EgressAddresses: objectcache.ProjectedField{
Values: map[string]struct{}{"10.0.0.1": {}, "10.0.0.2": {}},
Patterns: []string{"10.0.0.0/8"},
},
}
- lib := &nnLibrary{objectCache: &mockNNObjectCache{pcp: pcp}}
+ cpc := &mockNNCPC{pcp: pcp}
+ lib := &nnLibrary{objectCache: &mockNNObjectCache{cpc: cpc}}🤖 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/network_bench_test.go`
around lines 172 - 174, The ContainerProfileCache accessor on mockNNObjectCache
currently allocates a new &mockNNCPC{pcp: m.pcp} on every call which inflates
benchmark allocations; change the accessor to return a reusable instance instead
(e.g., store a single *mockNNCPC on mockNNObjectCache and return that) so
ContainerProfileCache() reuses the same mockNNCPC instead of creating a fresh
one each call; update mockNNObjectCache to hold the cached mockNNCPC and have
ContainerProfileCache() return that stored pointer (still wrapping m.pcp) to
eliminate per-call synthetic allocations.
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
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
1 participant
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.
Tier 3 of the v0.3.26 release chain
Network layer. Branches off node-agent#49 (tier-2 execs). Pairs with storage main / tier-2 (
networkmatchpackage has been on storage main since PR #31; no storage tier-3 branch needed).This PR is bench-only — no production code change. Captures the per-call cost of the network matchers as a documented baseline for future projection-v2 work (which will reach the
was_address_port_protocol_in_*port/protocol gap noted below).Why no code change
The two CR-flagged tier-3 majors from the NA #46 batch-1 deferred-items list either are already addressed on main, or need a coordinated projection-side change that's out of tier-3 scope:
ProjectedField.Alltreated as unconditional match (false negatives for absent IPs/DNS)matchIPField/matchDNSFieldat network.go:24-29 explicitly do NOT consultfield.Allas a short-circuit. Documented in the comment block.was_address_port_protocol_in_*ignores port + protocolmatchIPFieldonly). Documented in code at network.go:164-166. Fix requiresAddressPortsByAddr map[string][]PortProtoonProjectedField— coordinated projection-side change, out of tier-3 scope.Bench results
NA-side (
-count=3 -benchtime=200mson arm64):matchIPFieldvalues_only_hitmatchIPFieldpatterns_cidr_hit (per-call networkmatch.MatchIP compile)matchIPFieldany_sentinelmatchIPFieldmissmatchDNSFieldvalues_exact_hitmatchDNSFieldvalues_trailing_dot_canonmatchDNSFieldpatterns_leading_wildcard_hitmatchDNSFieldmisswasAddressPortProtocolInEgresshit_via_valueswasAddressPortProtocolInEgresshit_via_cidrwasAddressPortProtocolInEgressmissStorage-side
networkmatch(already on storage main since PR #31, included for cross-reference):CompiledIPMatcherLiteralCompiledIPMatcherCIDRCompiledIPMatcherLongMixedListCompiledDNSMatcherLiteralCompiledDNSMatcherLeadingWildcardCompiledDNSMatcherLongMixedListMatchIP(uncached, per-call compile) LiteralMatchIP(uncached) LongMixedListKey observations:
MatchIP/MatchDNScompile cost — this is uncached at the matchIPField layer; production callers use the CEL functionCache memoisation innn.gowhich handles this for the TTL windowwasAddressPortProtocolIn*(~50-100 ns / ~100 B / ~8 allocs) is pre-existing, shared across all CEL libraries, and dominates the matcher costReproduce:
Component test evidence
Run
26172312903(NA Component Tests onrelease/v0.3.26/tier-3-networkwithSTORAGE_TAG=tier2+STORAGE_REF=1159de1f):trigger-integration-testsfailure — same auto-trigger branch-name issue addressed in storage#36.Test plan
go build ./...cleango test ./pkg/rulemanager/cel/libraries/networkneighborhood/... -count=1green