Skip to content

fix(analyzer): trailing-* anchoring + DefaultCollapseConfigs accessor + FindConfigForPath value-return#25

Merged
entlein merged 2 commits intomainfrom
feat/analyzer-fixes-from-pr316-review
May 1, 2026
Merged

fix(analyzer): trailing-* anchoring + DefaultCollapseConfigs accessor + FindConfigForPath value-return#25
entlein merged 2 commits intomainfrom
feat/analyzer-fixes-from-pr316-review

Conversation

@entlein
Copy link
Copy Markdown

@entlein entlein commented May 1, 2026

Summary

Lands the upstream PR kubescape#316 review feedback on fork main. Two cherry-picks:

  • `793a5ac0` (squashed) — addresses Matthias's R1 (trailing-`*` anchoring) and N1/N2 nits, plus the CodeRabbit cleanup batch.
  • `07dcd1bf` — addresses CodeRabbit's outbound-mutation footgun on `FindConfigForPath`.

What's in here

Behavior fixes

  • CompareDynamic trailing-`*` contract — `/etc/` no longer matches the bare `/etc` directory; `/` does not match `/`; `*` (unanchored) is the explicit way to allowlist root. `splitPath` helper trims trailing-slash artifacts so `/etc/passwd/` is treated as `/etc/passwd`. Empty inputs return false.
  • splitEndpointPortAndPath defensive guard — non-`:`-prefixed inputs no longer return opaque tokens as port numbers.

Defensive-immutability hardening

  • `DefaultCollapseConfigs()` — unexports the package-level slice and exposes an accessor that returns a defensive copy on every call.
  • `FindConfigForPath` — returns `CollapseConfig` by value instead of leaking a pointer into the analyzer's internal `configs`/`defaultCfg` state.

CodeRabbit cleanup

  • Drop the `var _ = fmt.Sprint` import-keeper hack in profile_test.go.
  • Switch three `NewPathAnalyzer(100)` callers to `NewPathAnalyzerWithConfigs(OpenDynamicThreshold, DefaultCollapseConfigs())` for API consistency.
  • Handle `Close()` errors on the four CPU/mem/goroutine profile files.
  • Replace `fmt.Println` in `mergeHeaders` with `logger.L().Debug`.
  • Fix the misleading `/app` comment ("collapses immediately" → "collapses at 50 unique children").
  • Refactor the test-side `etcThreshold := 100` hardcoding to derive from `DefaultCollapseConfigs()` so config drift is caught.
  • Restore `BenchmarkCompareDynamic` across 10 realistic shapes; baseline is 2 allocs/op uniformly — the alloc count Matthias's zero-alloc rewrite needs to drive to 0.

Tests pinning the contracts

  • `TestCompareDynamic_AnchoringAndTrailing` — 22 cases (root, bare-parent, deep-prefix, multi-trailing-`*`)
  • `TestCompareDynamic_EllipsisAndStar` — 15 cases for ⋯/`*` interactions
  • `TestCompareDynamic_MidPathStarZeroOrMore` — 9 cases
  • `TestCompareDynamic_PathSeparatorEdges` — 8 cases
  • `TestSplitEndpointPortAndPath_DefensiveContract` — 12 cases (canonical + bare-path + opaque-token)
  • `TestDefaultCollapseConfigs_DefensiveCopy` — pins the accessor's defensive-copy contract
  • `TestFindConfigForPath_DefensiveCopy` — pins per-prefix and default-fallback mutation isolation
  • `TestCompareDynamic_WildcardRegressions` (pre-existing) — still passes unchanged

Out of scope

  • Performance regression R2 (zero-alloc rewrite of `CompareDynamic` to a two-pointer index scan) — Matthias is taking that one. The benchmark in this PR is the gate.

Test plan

  • `go build ./...` clean
  • `go test -count=1 ./...` — 14/14 packages green, 0 failures
  • All 65+ new dynamicpathdetector subtests pass
  • Pre-existing `TestCompareDynamic_WildcardRegressions`, `TestFindConfigForPath`, `TestProcessSegments_WildcardWiringRegressions` still pass
  • `go vet ./pkg/registry/file/dynamicpathdetector/...` clean
  • `BenchmarkCompareDynamic` baseline captured at 2 allocs/op

entlein and others added 2 commits May 1, 2026 19:23
…xplicit tests and behavior for empty versus wildcard, restores the benchmark test and addresses the nitpick comments - does not include the performance issue re the zero-alloc

Signed-off-by: entlein <einentlein@gmail.com>
Address CodeRabbit's outbound-mutation footgun: the public lookup
returned *CollapseConfig pointing into ua.configs / ua.defaultCfg, so
'cfg := analyzer.FindConfigForPath(p); cfg.Threshold = 1' silently
mutated the analyzer's threshold map for every future call. That
defeated the defensive inbound copy NewPathAnalyzerWithConfigs already
makes.

Switch to value return. The fall-through to defaultCfg is now also a
value copy, so callers cannot poison the analyzer through that path
either. Doc updated — the old 'or nil if none match' wording was wrong;
the function never returned nil, it always falls back to defaultCfg.

TestFindConfigForPath_DefensiveCopy pins the contract: mutating the
returned config (Threshold and Prefix) does not affect a subsequent
call, exercised on both the per-prefix and default-fallback paths.
TestFindConfigForPath drops the dead 'NotNil' assertion (a value can't
be nil); field reads work unchanged on both pointer and value.
@entlein entlein merged commit ac451ce into main May 1, 2026
2 checks passed
@entlein entlein deleted the feat/analyzer-fixes-from-pr316-review branch May 1, 2026 17:25
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

Summary:

  • License scan: failure
  • Credentials scan: skipped
  • Vulnerabilities scan: skipped
  • Unit test: success
  • Go linting: failure

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant