tier-1 (path+opens): narrower-fallback Patterns scan for suffix/prefix queries (Matthias #811)#48
Open
entlein wants to merge 1 commit into
Open
tier-1 (path+opens): narrower-fallback Patterns scan for suffix/prefix queries (Matthias #811)#48entlein wants to merge 1 commit into
entlein wants to merge 1 commit into
Conversation
…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
📝 WalkthroughWalkthroughUpdates suffix and prefix path opening checks to evaluate wildcard patterns in ChangesPattern-aware suffix/prefix matching
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed 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 |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 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/open_test.go`:
- Around line 30-46: The test comments in open_test.go contain contradictory
contracts: the newer block (starting at the shown diff) states "Patterns ARE
scanned when Opens.All == true" while an earlier legacy section still asserts
"MUST NOT contribute"; remove or update the stale legacy block so the file
documents only the new contract. Locate the old paragraph that claims Patterns
"MUST NOT contribute" and either delete it or rewrite it to match the new policy
(e.g., explain the narrower fallback behavior and wildcard permissiveness),
ensuring the test comments consistently describe the behavior checked by the
tests.
🪄 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: d830f2b7-7ffb-4ee7-8489-0a21925a4bca
📒 Files selected for processing (3)
pkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/open_bench_test.gopkg/rulemanager/cel/libraries/applicationprofile/open_test.go
Comment on lines
+30
to
+46
| // These tests pin Matthias's upstream PR #811 review contract: | ||
| // | ||
| // Patterns ARE scanned when Opens.All == true, but with a NARROWER | ||
| // fallback than text-level strings.HasSuffix/HasPrefix: | ||
| // | ||
| // * Pattern with a concrete tail/head (text after/before the last/first | ||
| // wildcard segment): match via HasSuffix/HasPrefix on the concrete | ||
| // piece — every realisation has that text textually. | ||
| // * Pattern ending/starting with a wildcard segment: be PERMISSIVE | ||
| // (return true). The concrete realisations could match ANY | ||
| // suffix/prefix; refusing would silently regress rules that omit | ||
| // profileDataRequired.opens (Matthias's "we still need a narrower | ||
| // fallback here instead of ignoring Patterns entirely"). | ||
| // | ||
| // Pre-PR-#811 (CR's HasSuffix-on-Patterns concern) the matcher SKIPPED | ||
| // Patterns entirely. That made wildcard-only profiles silently fail | ||
| // suffix/prefix queries — the regression Matthias's review reverses. |
There was a problem hiding this comment.
Resolve contradictory test contract comments.
Line 30 onward states the new “patterns are scanned” contract, but the preceding legacy block (for example Line 16) still asserts “MUST NOT contribute.” Please remove/update the stale section so the test documents one contract only.
Suggested cleanup
-// TestWasPathOpenedWithSuffix_PatternsNotScanned pins the contract from
-// the CodeRabbit PR `#43` review on open.go:79 (Major). Wildcard-shaped
-// entries in cp.Opens.Patterns MUST NOT contribute to suffix/prefix
-// answers — their literal text answers the wrong question. A retained
-// pattern "/var/log/pods/*/volumes/...." doesn't END with "foo.log"
-// even though the concrete open it stands in for might. Only concrete
-// paths in cp.Opens.Values are valid sources of suffix/prefix truth in
-// pass-through (Opens.All=true) mode.
-//
-// In projection-active mode (Opens.All=false), the rule manager
-// precomputes Opens.SuffixHits / PrefixHits from the spec, which is
-// the correct mechanism — those are exercised in
-// TestOpenWithSuffixInProfile / TestOpenWithPrefixInProfile.
+// TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail pins the
+// post-PR-#811 contract for Opens.All=true pattern scanning.🤖 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
30 - 46, The test comments in open_test.go contain contradictory contracts: the
newer block (starting at the shown diff) states "Patterns ARE scanned when
Opens.All == true" while an earlier legacy section still asserts "MUST NOT
contribute"; remove or update the stale legacy block so the file documents only
the new contract. Locate the old paragraph that claims Patterns "MUST NOT
contribute" and either delete it or rewrite it to match the new policy (e.g.,
explain the narrower fallback behavior and wildcard permissiveness), ensuring
the test comments consistently describe the behavior checked by the tests.
5 tasks
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 1 of the v0.3.26 release chain
Path+opens layer. Pairs with storage#35 (
release/v0.0.240/tier-1-path-opens) — both must deploy together; storage tier-1 SHA511a9c11is pinned in this branch's CT build.Addresses Matthias's upstream PR kubescape#811 BLOCKING review (2026-05-19T10:35Z).
Matthias's concern
On
pkg/rulemanager/cel/libraries/applicationprofile/open.go:125:Context
Pre-PR-kubescape#811, CodeRabbit (PR #43 review on
open.go:79) flagged that runningstrings.HasSuffix("/var/log/pods/*/volumes/...", "foo.log")returns false even though the pattern's concrete realisations might end infoo.log— a false negative. We addressed it by SKIPPING thePatternsscan in theAll=truebranch. Matthias's review reverses that: skipping is too aggressive and silently regresses rules that omitprofileDataRequired.opens(which rely on theAll=truepass-through behavior).Fix — narrower fallback
New helpers:
patternConcreteSuffix(p string)→ literal text after the LAST wildcard segment inp; returns""ifpends in a wildcard segment.patternConcretePrefix(p string)→ literal text before the FIRST wildcard segment inp; returns""ifpstarts with a wildcard segment.Suffix/prefix matcher then:
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).
Local bench evidence (
-count=3 -benchtime=200ms)Helper itself — zero-alloc on every shape:
/var/log/⋯/foo.log/var/log/pods/*/var/log/foo.log(no wildcards)*/var/⋯/log/⋯/foo.logFull matcher (50 Values + 10 Patterns):
Patterns scan adds ~20 ns vs Values-only with no extra allocs. The steady 56 B / 3 allocs is pre-existing CEL value-wrapping cost, unchanged by this PR.
Reproduce:
Tests
Updated previous "PatternsNotScanned" tests (which pinned the pre-kubescape#811 CR fix) to reflect Matthias's new contract. New / updated assertions:
TestWasPathOpenedWithSuffix_PatternsScannedWithConcreteTail— pattern/var/log/⋯/foo.log, query.log→ true (tail/foo.loghas suffix.log); query.txt→ falseTestWasPathOpenedWithSuffix_PatternWildcardTail_Permissive— pattern/var/log/pods/*, any suffix → trueTestWasPathOpenedWithPrefix_PatternsScannedWithConcreteHead— pattern/var/⋯/log/foo, query/var/→ true; query/etc/→ falseTestWasPathOpenedWithPrefix_PatternWildcardHead_Permissive— pattern*/run, any prefix → trueTestPatternConcreteSuffix_AndPrefix— pins the splitter helpers on 7 shapes (no_wildcards, trailing_star, leading_star, mid_ellipsis, both_mid, lone_star, lone_ellipsis)Component test evidence
Run
26121369338(NA Component Tests workflow onrelease/v0.3.26/tier-1-path-openswithSTORAGE_TAG=tier1+STORAGE_REF=511a9c11):was_path_opened*matchers25969722538), not introduced by this PRTest plan
go build ./...cleango test ./pkg/rulemanager/... -count=1green26121369338), Matthias-relevant legs green