allowing wildcards in exec args#322
Conversation
Signed-off-by: entlein <einentlein@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds ChangesExec argument matching with wildcard support
🎯 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 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[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 (2)
pkg/registry/file/dynamicpathdetector/compare_exec_args.go (1)
45-55: 💤 Low valueCollapse
seen+memointo a single map using the comma-ok idiom.Two parallel maps keyed by the same state double allocations and obscure intent. The standard Go pattern is to check presence on the result map directly. Behavior is identical.
♻️ Proposed simplification
type state struct{ pi, ri int } - memo := make(map[state]bool, (len(profileArgs)+1)*(len(runtimeArgs)+1)) - seen := make(map[state]bool, (len(profileArgs)+1)*(len(runtimeArgs)+1)) + memo := make(map[state]bool, (len(profileArgs)+1)*(len(runtimeArgs)+1)) var match func(pi, ri int) bool match = func(pi, ri int) bool { s := state{pi: pi, ri: ri} - if seen[s] { - return memo[s] + if v, ok := memo[s]; ok { + return v } - seen[s] = true🤖 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/file/dynamicpathdetector/compare_exec_args.go` around lines 45 - 55, The code uses two maps (seen and memo) keyed by state in compare logic; collapse them into a single map (e.g., memo map[state]bool) and use the comma-ok idiom inside match to check if an entry exists: construct s := state{pi: pi, ri: ri}, if val, ok := memo[s]; ok { return val } then continue computing and store the result in memo[s] before returning. Update references to seen and memo in the match closure and initialization (remove seen, keep memo sized with (len(profileArgs)+1)*(len(runtimeArgs)+1)).pkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go (1)
300-313: 💤 Low valueWall-clock budget is shared-runner flaky; consider a deterministic cap.
A 100ms wall-clock budget on adversarial input is a reasonable smoke test, but shared CI runners under load can blow past it without an actual algorithmic regression — producing flakes that erode trust in the suite. Options that retain the guarantee without time-sensitivity:
- Increase budget substantially (e.g., 1–2s) — still catches an exponential blow-up (which would be many seconds to minutes), with much wider headroom for noisy runners.
- Or, scale the wildcard count up (e.g., 30–40
*s) so the unmemoised form would take effectively forever, then use a generous budget. The memoised form stays microseconds either way.🤖 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/file/dynamicpathdetector/tests/compare_exec_args_test.go` around lines 300 - 313, Replace the flaky 100ms wall-clock assertion in the test that times CompareExecArgs by increasing the deterministic cap: change the elapsed check to use a much larger budget (e.g., 2*time.Second) instead of 100*time.Millisecond (update the local variables start/elapsed check around CompareExecArgs), or alternatively make the adversarial input stronger by increasing the wildcard count (the pattern used to build the profile) so the unmemoised algorithm would take orders of magnitude longer while keeping the memoised form fast; update the test assertion and any comment to reflect the new budget or stronger input.
🤖 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/file/dynamicpathdetector/compare_exec_args.go`:
- Line 66: The package references an undefined constant WildcardIdentifier (used
in compare_exec_args.go and tests) which causes compilation failure; add a
string constant named WildcardIdentifier (value "*" to represent "zero or more
consecutive arguments") next to the existing DynamicIdentifier constant in the
package's types definitions so all references to WildcardIdentifier resolve
correctly and behavior matches the documented wildcard semantics.
---
Nitpick comments:
In `@pkg/registry/file/dynamicpathdetector/compare_exec_args.go`:
- Around line 45-55: The code uses two maps (seen and memo) keyed by state in
compare logic; collapse them into a single map (e.g., memo map[state]bool) and
use the comma-ok idiom inside match to check if an entry exists: construct s :=
state{pi: pi, ri: ri}, if val, ok := memo[s]; ok { return val } then continue
computing and store the result in memo[s] before returning. Update references to
seen and memo in the match closure and initialization (remove seen, keep memo
sized with (len(profileArgs)+1)*(len(runtimeArgs)+1)).
In `@pkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go`:
- Around line 300-313: Replace the flaky 100ms wall-clock assertion in the test
that times CompareExecArgs by increasing the deterministic cap: change the
elapsed check to use a much larger budget (e.g., 2*time.Second) instead of
100*time.Millisecond (update the local variables start/elapsed check around
CompareExecArgs), or alternatively make the adversarial input stronger by
increasing the wildcard count (the pattern used to build the profile) so the
unmemoised algorithm would take orders of magnitude longer while keeping the
memoised form fast; update the test assertion and any comment to reflect the new
budget or stronger input.
🪄 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: e50038fd-c79c-449f-b3e3-2d53a45b7e78
📒 Files selected for processing (2)
pkg/registry/file/dynamicpathdetector/compare_exec_args.gopkg/registry/file/dynamicpathdetector/tests/compare_exec_args_test.go
|
|
||
| head := profileArgs[pi] | ||
|
|
||
| if head == WildcardIdentifier { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate definitions of WildcardIdentifier and DynamicIdentifier in the package.
rg -nP --type=go -C2 '\b(Wildcard|Dynamic)Identifier\b\s*=' pkg/registry/file/dynamicpathdetector
echo "---"
rg -nP --type=go -C2 '\bconst\s*\(' pkg/registry/file/dynamicpathdetector | head -50
echo "---"
# Also list all Go files in the package to spot a likely constants file.
fd -e go . pkg/registry/file/dynamicpathdetector --max-depth 1Repository: kubescape/storage
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Check types.go for any constant or identifier definitions
echo "=== Checking types.go ==="
cat -n pkg/registry/file/dynamicpathdetector/types.go
echo ""
echo "=== Checking compare_exec_args.go for imports and usage ==="
head -80 pkg/registry/file/dynamicpathdetector/compare_exec_args.go
echo ""
echo "=== Checking for DynamicIdentifier and WildcardIdentifier (broader search) ==="
rg -n "WildcardIdentifier|DynamicIdentifier" pkg/registry/file/dynamicpathdetector --no-headingRepository: kubescape/storage
Length of output: 6507
🏁 Script executed:
#!/bin/bash
# Check if WildcardIdentifier is defined anywhere in the entire package
echo "=== Searching for WildcardIdentifier definition ==="
rg -n "const.*WildcardIdentifier|WildcardIdentifier.*=" pkg/registry/file/dynamicpathdetector --type=go
echo ""
echo "=== Reading full compare_exec_args.go to check for missing definitions ==="
wc -l pkg/registry/file/dynamicpathdetector/compare_exec_args.go
echo ""
echo "=== Checking if WildcardIdentifier might be an import from elsewhere ==="
rg -n "import|WildcardIdentifier" pkg/registry/file/dynamicpathdetector/compare_exec_args.go | head -20
echo ""
echo "=== Checking all Go files for any WildcardIdentifier const/var definition ==="
rg -n "WildcardIdentifier" pkg/registry/file/dynamicpathdetector --type=go -B2 | grep -E "const|var|=|:" | head -30Repository: kubescape/storage
Length of output: 1715
🏁 Script executed:
#!/bin/bash
# Read the complete compare_exec_args.go file
cat -n pkg/registry/file/dynamicpathdetector/compare_exec_args.goRepository: kubescape/storage
Length of output: 4160
Define missing WildcardIdentifier constant in the dynamicpathdetector package.
DynamicIdentifier is properly defined in types.go, but WildcardIdentifier is referenced at lines 66 and 85 in compare_exec_args.go and at line 291 in the test file without being defined anywhere in the package. This will cause compilation failure. Based on the documented behavior (matching "zero or more consecutive arguments") and usage, WildcardIdentifier should be defined as a string constant (likely "*") in types.go alongside DynamicIdentifier.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 66-66: : # github.com/kubescape/storage/pkg/registry/file/dynamicpathdetector
pkg/registry/file/dynamicpathdetector/compare_exec_args.go:66:14: undefined: WildcardIdentifier
(typecheck)
🤖 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/file/dynamicpathdetector/compare_exec_args.go` at line 66, The
package references an undefined constant WildcardIdentifier (used in
compare_exec_args.go and tests) which causes compilation failure; add a string
constant named WildcardIdentifier (value "*" to represent "zero or more
consecutive arguments") next to the existing DynamicIdentifier constant in the
package's types definitions so all references to WildcardIdentifier resolve
correctly and behavior matches the documented wildcard semantics.
There was a problem hiding this comment.
good catch, its in the other PR #323 , can you review it, as if that one was merged first and add the merge order to the description of this PR, please
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch PR `#323` details
gh pr view 323 --repo kubescape/storage --json title,body,state,files,additions,deletions,headRefName,baseRefName
echo "---FILES---"
gh pr diff 323 --repo kubescape/storage
Length of output: 143880
@entlein, here's the review of PR #323 as if merged first (i.e. WildcardIdentifier is already in scope for PR #322).
Review of PR #323 (as if merged first)
pkg/registry/file/dynamicpathdetector/types.go
✅ WildcardIdentifier = "*" added alongside DynamicIdentifier — resolves the compilation failure in PR #322.
✅ CollapseConfig, DefaultCollapseConfigs() defensive-copy accessor, and DefaultCollapseConfig fallback are well-structured.
defaultCollapseConfigs has /app at threshold 50, but testCollapseConfigs in the test file sets /app at threshold 1 (configThreshold("/app")). If production code ever reads DefaultCollapseConfigs() and expects /app to wildcard immediately (as the test comment says "threshold 1 = immediate wildcard"), this divergence will surface as a silent mismatch. Worth a comment in defaultCollapseConfigs explicitly noting the difference.
pkg/registry/file/dynamicpathdetector/analyzer.go
CompareDynamic rewrite
✅ Anchoring contract (trailing * = one-or-more, unanchored * = matches root) is clear and well-tested.
✅ Removing the optimistic dynamic[1] peek correctly fixes consecutive-wildcard matching.
compareSegments: mid-path * recurses i = 0..len(regular) at each wildcard level. For a pattern like /*/*/b against a long path /a/a/a/a/a/a/a/b, the two nested stars produce O(n²) recursion. There's no memoisation here (unlike CompareExecArgs in PR #322 which does memo). For file paths this is probably fine in practice, but worth a comment or TODO.
processSegments / collapseAdjacentDynamic
✅ bufPool reuse is sound — string(buf) copies before the pool return.
✅ collapseAdjacentDynamic byte-level scan is correct and allocation-free.
removeEndpoint in analyze_endpoints.go uses append(s[:i], s[i+1:]...) which mutates the original backing array. If any caller holds a reference to the pre-remove slice it will see stale data. Looks safe here since MergeDuplicateEndpoints owns the slice, but worth a comment.
pkg/registry/file/dynamicpathdetector/analyze_endpoints.go
✅ Two-pass design (trie build then process) avoids the original contamination bug.
✅ getEndpointKey now includes Internal — matches HTTPEndpoint.Equal semantics.
MergeDuplicateEndpoints order dependency: if two *:0 wildcard entries with the same (path, direction, Internal) appear, the second one will attempt the sweep on seen but will find the first wildcard already occupying the slot under its own wildcardKey, and both survive. This is a corner-case but worth a note since it's not tested.
pkg/registry/file/dynamicpathdetector/analyze_opens.go — consolidateOpens
✅ isPattern / patternOrder sorted longest-first is a clean design.
Flags mutation: opens[pi].Flags = mapset.Sorted(...) modifies the slice element in the caller's view. Since consolidateOpens takes []types.OpenCalls by value but the slice header is copied not the underlying structs, the caller's original elements get their Flags modified. This is intentional but is a subtle side-effect worth documenting.
Tests
✅ Extensive — TestConsolidateOpens_*, TestCompareDynamic_*, TestProcessSegments_WildcardWiringRegressions etc. all pin known-tricky edges.
✅ TestDefaultCollapseConfigs_DefensiveCopy and TestFindConfigForPath_DefensiveCopy are exactly the right contracts to pin.
✅ DoS-resistance test pattern from PR #322 is mirrored in the benchmark suite here.
Now updating PR #322's description:
🏁 Script executed:
#!/bin/bash
# Get current PR `#322` body
gh pr view 322 --repo kubescape/storage --json body -q .body
Length of output: 895
🏁 Script executed:
#!/bin/bash
gh pr edit 322 --repo kubescape/storage --body "exec-path: symlink-faithful argv[0] resolution
## ⚠️ Merge order
**This PR depends on `#323` and must be merged AFTER it.**
PR `#323` defines \`WildcardIdentifier\` (and the broader \`CollapseConfig\` / \`NewPathAnalyzerWithConfigs\` surface) in \`types.go\` and \`analyzer.go\`. Without it, \`compare_exec_args.go\` (this PR) fails to compile because \`WildcardIdentifier\` is undefined.
**Correct merge order:**
1. `#323` — path-wildcards: anchored trailing-\`*\` + per-endpoint port + R0040 args
2. `#322` (this PR) — allowing wildcards in exec args
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
## Release Notes
* **New Features**
* Implemented wildcard-aware command-line argument pattern matching with support for flexible argument constraints and performance-optimized comparison logic.
* **Tests**
* Added extensive test coverage for argument pattern matching scenarios including literal matches, wildcard patterns, dynamic constraints, realistic command patterns, and performance resilience validation.
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->"
Length of output: 642
🏁 Script executed:
#!/bin/bash
cat > /tmp/pr322_body.md << 'BODY'
exec-path: symlink-faithful argv[0] resolution
## ⚠️ Merge order
**This PR depends on `#323` and must be merged AFTER it.**
PR `#323` defines `WildcardIdentifier` (and the broader `CollapseConfig` / `NewPathAnalyzerWithConfigs` surface) in `types.go` and `analyzer.go`. Without it, `compare_exec_args.go` (this PR) fails to compile because `WildcardIdentifier` is undefined.
**Correct merge order:**
1. `#323` — path-wildcards: anchored trailing-`*` + per-endpoint port + R0040 args
2. `#322` (this PR) — allowing wildcards in exec args
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
## Release Notes
* **New Features**
* Implemented wildcard-aware command-line argument pattern matching with support for flexible argument constraints and performance-optimized comparison logic.
* **Tests**
* Added extensive test coverage for argument pattern matching scenarios including literal matches, wildcard patterns, dynamic constraints, realistic command patterns, and performance resilience validation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
BODY
gh pr edit 322 --repo kubescape/storage --body-file /tmp/pr322_body.md
echo "Exit code: $?"
Length of output: 266
@entlein, unfortunately gh pr edit is blocked by the workflow restriction settings on this repository, so I'm unable to update the PR #322 description automatically. Please paste the following into the PR #322 description manually:
exec-path: symlink-faithful argv[0] resolution
## ⚠️ Merge order
**This PR depends on `#323` and must be merged AFTER it.**
PR `#323` defines `WildcardIdentifier` (and the broader `CollapseConfig` / `NewPathAnalyzerWithConfigs` surface) in `types.go` and `analyzer.go`. Without it, `compare_exec_args.go` (this PR) fails to compile because `WildcardIdentifier` is undefined.
**Correct merge order:**
1. `#323` — path-wildcards: anchored trailing-`*` + per-endpoint port + R0040 args
2. `#322` (this PR) — allowing wildcards in exec argsReview of PR #323 (treating it as merged first)
Overall PR #323 is solid and well-tested. A few findings:
🔴 types.go — threshold divergence between production and tests
defaultCollapseConfigs sets /app at threshold 50, but testCollapseConfigs (used across the test suite) sets /app at 1. Tests like TestAnalyzeOpensThreshold1ImmediateWildcard use configThreshold("/app") which resolves to 1 from testCollapseConfigs. If any future code reads DefaultCollapseConfigs() expecting /app to wildcard immediately, it will be surprised. A comment on the /app entry in defaultCollapseConfigs clarifying that the test suite intentionally uses a stricter value would prevent future confusion.
🟡 analyzer.go — compareSegments has no memoisation
The mid-path * recurses at every offset i = 0..len(regular). For patterns with multiple * tokens (e.g. /*/*/b), this is O(n²) in path depth. CompareExecArgs in PR #322 solves this exact problem with a seen/memo map. For file paths in practice the depth is bounded and this is unlikely to cause issues, but it's worth a comment alongside the loop noting the quadratic bound — especially since PR #322 already documents this pattern.
🟡 analyze_endpoints.go — removeEndpoint mutates shared backing array
return append(s[:i], s[i+1:]...)This shifts elements in place in the existing backing array. It is safe here because MergeDuplicateEndpoints is the sole owner of the newEndpoints slice, but a comment noting that callers must not retain a reference to the pre-call slice would make the ownership contract explicit.
🟡 analyze_opens.go — consolidateOpens silently mutates callers' Flags
opens[pi].Flags = mapset.Sorted(...) modifies the struct in the slice that was passed in. Since consolidateOpens receives []types.OpenCalls by value (slice header copy), the underlying elements are shared with the caller. This is intentional for efficiency but should be documented — a caller reading opens[pi].Flags after the call will see the merged result rather than the original.
✅ Everything else looks good
- Two-pass endpoint analysis correctly isolates the trie build from per-port processing.
getEndpointKeynow includesInternal— matchesHTTPEndpoint.Equalexactly.collapseAdjacentDynamicbyte-level scan is zero-allocation and correct.bufPool+processSegmentsrewrite is sound (string(buf)copies before pool return).- Test coverage (
TestProcessSegments_WildcardWiringRegressions,TestCompareDynamic_AnchoringAndTrailing,TestConsolidateOpens_*,TestFindConfigForPath_DefensiveCopy) is comprehensive and pins the right contracts.
🐇 ～(‾▿‾～)
Signed-off-by: entlein <einentlein@gmail.com>
|
Re-review requested after applying atomic-split advisory and rabbit-feedback fixes. Please assess ✅ Actions performedFull review triggered. |
2 participants
exec-path: symlink-faithful argv[0] resolution
This PR depends on
#323and must be merged AFTER it.PR
#323definesWildcardIdentifier(and the broaderCollapseConfig/NewPathAnalyzerWithConfigssurface) intypes.goandanalyzer.go. Without it,compare_exec_args.go(this PR) fails to compile becauseWildcardIdentifieris undefined.Correct merge order:
#323— path-wildcards: anchored trailing-*+ per-endpoint port + R0040 args#322(this PR) — allowing wildcards in exec argsSummary by CodeRabbit
Release Notes
New Features
Tests