Skip to content

feat(collapse): rule-aware protection of sensitive open prefixes#335

Open
matthyx wants to merge 3 commits into
mainfrom
feat/rule-aware-collapse-protection
Open

feat(collapse): rule-aware protection of sensitive open prefixes#335
matthyx wants to merge 3 commits into
mainfrom
feat/rule-aware-collapse-protection

Conversation

@matthyx

@matthyx matthyx commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Problem

Profile deflation generalises high-cardinality open paths to keep stored profiles bounded (e.g. /etc with many files → /etc/⋯, or the whole tree → /⋯/⋯). That generalisation silently defeats anomaly rules such as R0010 (unexpected /etc/shadow access): once /etc collapses to a wildcard, ap.was_path_opened("/etc/shadow.evil") matches it and the rule can never fire.

Fix

Pin rule-declared sensitive prefixes (and their ancestors) to literal during collapse, so a never-seen sensitive path stays distinguishable from a generalised one. The analyzer skips collapsing any node on the ancestor chain of — or within the subtree of — a protected prefix.

  • dynamicpathdetector: NewPathAnalyzerWithConfigsAndProtection + OpenProtection (exact/prefix/suffix/contains; suffix/contains are resolved against the observed open paths since they have no fixed location). protectedNode is O(1) for the common node via a precomputed ancestor set + top-dir fast-reject; cost never scales with the number of protected prefixes, and there is zero overhead when no protection is configured.
  • DeflateContainerProfileSpec takes an OpenProtection; OpenProtectionFromMatchers converts armotypes.OpenMatchers. config.Config.ProtectedOpenMatchers feeds the in-cluster processor (populated by the operator from the rule library via armotypes.UnionOpenProtection); the backend feeds it from MongoDB rules.

Tested

  • TestProtectedPrefixKeepsSensitiveDetectable and TestOpenProtectionExactSuffixContains: with /etc far over the collapse threshold, never-seen sensitive paths (/etc/shadow.evil, /etc/sudoers.bak, /etc/sudoers.d/99-evil, /etc/ssh/ssh_host_ed25519_key, /home/alice/.ssh/evil) stay uncovered, while /proc still collapses (bloat control preserved). Existing dynamicpathdetector suite unaffected (zero-value OpenProtection = legacy behaviour).

Performance

Deflation is per profile PreSave (consolidation), not per open event — off the per-event hot path. Benchmarked on a ~1,340-open profile:

per profile
No protection ~0.9 ms
R0010 protection ~1.3 ms (+0.4 ms)

(The precomputed protectedNode keeps this at ~1.4× rather than the ~6.4× a naive per-prefix scan would cost.)

Deps

Related

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Configurable "protected" matchers and a runtime store to pin sensitive path prefixes so they are treated specially during path generalization; optional periodic reloader keeps them in sync.
    • Analysis now respects protected prefixes and caps protected-subtree growth to avoid unbounded memory use.
  • Tests
    • Added regression tests covering protection rules and collapse behavior.
  • Dependencies
    • Updated armoapi-go dependency version.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds an OpenProtection model and reloadable store, exposes config for protected open matchers, updates the dynamic PathAnalyzer to pin protected prefixes (never-collapse/pinned budget behavior), wires the store into container profile deflation and server startup, and adds tests for protection and reloader behavior.

Changes

Protected Path Prefix Detection

Layer / File(s) Summary
Thresholds, types, and config
go.mod, pkg/registry/file/dynamicpathdetector/types.go, pkg/config/config.go
Dependency bump to armoapi-go, add NeverCollapseThreshold and PinnedSubtreeBudget, extend PathAnalyzer with protection maps, and add Config.ProtectedOpenMatchers plus default refresh interval.
Analyzer protection logic
pkg/registry/file/dynamicpathdetector/analyzer.go
Add NewPathAnalyzerWithConfigsAndProtection, NormalizeProtectedPrefixes, protected-node detection, and apply a pinned threshold in processSegments and updateNodeStats to avoid collapsing protected prefixes.
OpenProtection model
pkg/registry/file/dynamicpathdetector/protection.go, pkg/registry/file/dynamicpathdetector/types.go
Introduce OpenProtection struct with Exact/Prefix/Suffix/Contains, Empty() and ProtectedPrefixes(...) for computing pinned prefixes from opens.
ConfigMap-backed store & reloader
pkg/registry/file/openprotection.go
Add OpenProtectionStore (concurrency-safe), ParseOpenProtectionConfigMap, and OpenProtectionReloader with periodic reload and default-interval fallback.
Store and reloader tests
pkg/registry/file/openprotection_test.go
Unit tests for parsing, store Get/Set, and reloader reload behavior (NotFound tolerance, interval fallback).
Processor and server wiring
pkg/registry/file/containerprofile_processor.go, pkg/apiserver/apiserver.go, pkg/cmd/server/start.go, main.go
Seed or inject OpenProtectionStore into ContainerProfileProcessor; add ProtectionStore() and OpenProtectionFromMatchers; pass protection.Get() into DeflateContainerProfileSpec; propagate store through apiserver/start options; initialize and run reloader from main when configured.
Analyzer protection regression tests
pkg/registry/file/dynamicpathdetector/protection_test.go
Tests and helpers (genWith, coveredBy, keysOf) asserting protected prefixes prevent sensitive-path generalization, exercise Exact/Prefix/Suffix/Contains matching, and verify budget-triggered collapse behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • kubescape/storage#323: Changes to dynamicpathdetector/analyzer.go threshold selection and processSegments logic that overlap with this PR's protection-aware threshold pinning.

Poem

🐰 I hop through paths both wide and thin,
I pin the shadows safe within,
A store that watches, steady, bright,
Keeps secret roots from fading sight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(collapse): rule-aware protection of sensitive open prefixes' directly summarizes the main change: adding rule-aware protection to prevent sensitive open paths from being collapsed during profile deflation, which aligns with the primary objective of pinning sensitive prefixes during path generalization.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/rule-aware-collapse-protection

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@go.mod`:
- Line 10: The go.mod currently pins github.com/armosec/armoapi-go to a
pseudo-version (5cfa1e630e5d); replace this temporary pseudo-version with the
appropriate tagged release (e.g., vX.Y.Z) for github.com/armosec/armoapi-go to
ensure reproducible builds, then update the module by running the Go tooling
(e.g., use `go get github.com/armosec/armoapi-go@<tag>` and `go mod tidy`) and
verify go.sum and the build succeed; ensure any references to the pseudo-version
in go.mod are removed and the dependency now references the official tag.
🪄 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: 4af06910-f4ba-4c7b-bcfb-623933563a9e

📥 Commits

Reviewing files that changed from the base of the PR and between 11472e5 and 33957d1.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • go.mod
  • pkg/config/config.go
  • pkg/registry/file/containerprofile_processor.go
  • pkg/registry/file/dynamicpathdetector/analyzer.go
  • pkg/registry/file/dynamicpathdetector/protection.go
  • pkg/registry/file/dynamicpathdetector/protection_test.go
  • pkg/registry/file/dynamicpathdetector/types.go

Comment thread go.mod Outdated
@github-actions

Copy link
Copy Markdown

Summary:

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

Profile deflation generalises high-cardinality open paths to keep stored
profiles bounded (e.g. /etc with many files -> /etc/⋯, or the whole tree ->
/⋯/⋯). That generalisation silently defeats anomaly rules such as R0010
(unexpected /etc/shadow access): once /etc collapses to a wildcard,
ap.was_path_opened("/etc/shadow.evil") matches it and the rule can never fire.

Pin rule-declared sensitive prefixes (and their ancestors) to literal during
collapse so a never-seen sensitive path stays distinguishable from a generalised
one. The analyzer skips collapsing any node on the ancestor chain of — or within
the subtree of — a protected prefix.

- dynamicpathdetector: NewPathAnalyzerWithConfigsAndProtection + OpenProtection
  (exact/prefix/suffix/contains; suffix/contains resolved against the observed
  open paths since they have no fixed location). protectedNode is O(1) for the
  common node via a precomputed ancestor set + top-dir fast-reject; the cost
  never scales with the number of protected prefixes. Zero overhead when no
  protection is configured.
- DeflateContainerProfileSpec takes the OpenProtection; OpenProtectionFromMatchers
  converts armotypes.OpenMatchers. config.Config.ProtectedOpenMatchers feeds the
  in-cluster processor (populated by the operator from the rule library); the
  backend feeds it from MongoDB rules.

Requires armoapi-go v0.0.719 (OpenMatchers / UnionOpenProtection).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
@matthyx matthyx force-pushed the feat/rule-aware-collapse-protection branch from 33957d1 to cb033d5 Compare June 11, 2026 08:25
@github-actions

Copy link
Copy Markdown

Summary:

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

@matthyx matthyx left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the rule-aware collapse protection. The design is sound, it builds clean, and go test ./pkg/registry/file/dynamicpathdetector/... passes. The protectedNode O(1) fast-path and the zero-overhead-when-unconfigured guarantee both hold up.

The armoapi-go re-pin called out in the PR body is already donego.mod is on the tagged v0.0.719 (not the pseudo-version), the tag exists upstream, and armosec/armoapi-go#656 is merged. So that pre-merge item is cleared.

Two things I'd want resolved (or at least explicitly documented as accepted limitations) before merge — left inline. Neither is a compile/correctness bug; both are gaps between what the PR claims to guarantee and what it actually guarantees.

if len(p.Suffix) > 0 || len(p.Contains) > 0 {
for _, op := range openPaths {
if p.matchesUnanchored(op) {
out = append(out, op) // pin this open's ancestor chain

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker (partial coverage): Suffix/Contains protection is learning-dependent and does NOT protect never-seen paths — which is the whole point of the PR.

Exact/Prefix pin statically (their ancestor chain is kept literal even if the sensitive file was never opened during learning). But Suffix/Contains are only pinned for paths that were actually observed in openPaths. If no sibling matching the matcher was opened during learning, and that directory is high-cardinality, the directory collapses to /dir/⋯ and a first-ever access to a matching file is covered by the wildcard — i.e. the R0010-class false negative the PR sets out to fix still happens for these two matcher kinds.

Concrete: matcher Suffix: ["_key"], and during learning /etc/ssh had 80 non-key files opened but no *_key file. /etc/ssh collapses to /etc/ssh/⋯, and a novel /etc/ssh/ssh_host_ed25519_key is then covered → rule can't fire. The test passes only because ssh_host_rsa_key was in the observed opens, which pins /etc/ssh; remove that one observed key and the guarantee disappears.

This asymmetry (static for exact/prefix, best-effort for suffix/contains) isn't surfaced anywhere a rule author would see it. Options: (a) document it loudly as a known limitation of suffix/contains matchers, (b) accept that suffix/contains can only ever be best-effort here and steer rule authors toward exact/prefix for must-detect paths. At minimum the PR description's "keeps a never-seen sensitive path distinguishable" should be scoped to exact/prefix.

}
protectedPrefixes = openProtection.ProtectedPrefixes(openPaths)
}
openAnalyzer := dynamicpathdetector.NewPathAnalyzerWithConfigsAndProtection(OpenDynamicThreshold, nil, protectedPrefixes)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker to confirm (profile-size blowback): pinning a sensitive prefix makes its entire ancestor directory uncollapsible, which can flip the profile to TooLarge.

Protecting e.g. /etc/shadow pins /etc to literal — so every distinct file ever opened under /etc now survives as its own entry instead of folding to /etc/⋯. On a chatty workload (or with several rule-protected top-level dirs), that growth feeds straight into the size accumulation in PreSave, and size > MaxContainerProfileSize sets helpers.TooLarge.

If a TooLarge profile is dropped from / not used by anomaly detection downstream, this is a self-defeating outcome: turning on protection for R0010 could disable detection for the whole container — strictly worse than the wildcard it was meant to fix. The Performance section measures time (+0.4 ms) but not the size delta, which is the dimension deflation exists to bound.

Please confirm what TooLarge does downstream, and add a worst-case size figure (e.g. /etc with N literals) next to the timing table. If TooLarge degrades detection, this needs a guard (cap pinned literals per protected dir, or fall back to collapse when a pinned subtree itself exceeds a budget).

matthyx added a commit that referenced this pull request Jun 12, 2026
@matthyx

matthyx commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Re-reviewed 0841458. Both blockers from my earlier review are addressed — builds clean, dynamicpathdetector tests pass (incl. the new TestProtectedPrefixBudgetCollapse):

  1. Suffix/Contains learning-dependence — now documented as a CRITICAL/KNOWN LIMITATION in protection.go with explicit guidance to use Exact/Prefix where detection must be guaranteed. Good — the gap is real but now surfaced where rule authors will see it.

  2. Profile-size blowbackPinnedSubtreeBudget = 500 caps literals under a pinned node; past the budget it falls back to collapse rather than risk TooLarge (which clears the whole spec). Sound tradeoff: degrade one pathological dir instead of dropping the entire profile.

One non-blocking note: when the budget trips, protection silently flips off for that directory (the test confirms /etc/shadow.evil becomes covered again). Worth a Debug log when a pinned node hits the budget, so an operator can tell why an R0010-class rule stopped firing for a container. Not a merge blocker.

LGTM to merge.

@github-actions

Copy link
Copy Markdown

Summary:

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

Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
@matthyx matthyx force-pushed the feat/rule-aware-collapse-protection branch from 0841458 to 030c0af Compare June 12, 2026 10:02
@github-actions

Copy link
Copy Markdown

Summary:

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

…gMap

The rule-aware collapse protection added in this branch fed the protected open
matchers from static config (config.protectedOpenMatchers), set once at startup.
That cannot track RuntimeRuleAlertBinding changes without a restart.

Move the processor to a shared, concurrency-safe OpenProtectionStore read on
every PreSave (the open-event hot path) and add an OpenProtectionReloader that
periodically reads a ConfigMap (DefaultNamespace / openProtectionConfigMapName)
and refreshes the store. This is the in-cluster reader side of the
"operator writes one object, storage refreshes periodically" wiring: the
operator watches RuntimeRuleAlertBinding, resolves selectors against the rule
library to the union of profileDataRequired.opens, and writes the ConfigMap
(producer side implemented separately).

Behaviour:
- The store is seeded from config (ProtectedOpenMatchers) so environments
  without the operator/ConfigMap keep working unchanged (zero value = legacy
  collapse).
- A missing ConfigMap keeps the current protection rather than wiping it,
  avoiding a transient unprotection window before the operator creates it.
- The reloader only starts when openProtectionConfigMapName is set.

The kube-client GET + ticker loop need a live cluster and are flagged for CI;
the parse (ParseOpenProtectionConfigMap), store Get/Set, and reload-once logic
(present/NotFound/default-interval) are unit-tested with a fake clientset.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/registry/file/containerprofile_processor.go (1)

92-98: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Copy matcher slices when converting into OpenProtection.

This conversion currently aliases the caller’s armotypes.OpenMatchers slices into the shared store. That undermines the store’s concurrency contract: a caller that retains and mutates m.Prefix/m.Exact after Set can change the live protection set without taking the store lock, and Get() readers will observe that mutation concurrently.

Possible fix
 func OpenProtectionFromMatchers(m armotypes.OpenMatchers) dynamicpathdetector.OpenProtection {
 	return dynamicpathdetector.OpenProtection{
-		Exact:    m.Exact,
-		Prefix:   m.Prefix,
-		Suffix:   m.Suffix,
-		Contains: m.Contains,
+		Exact:    append([]string(nil), m.Exact...),
+		Prefix:   append([]string(nil), m.Prefix...),
+		Suffix:   append([]string(nil), m.Suffix...),
+		Contains: append([]string(nil), m.Contains...),
 	}
 }
🤖 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/containerprofile_processor.go` around lines 92 - 98,
OpenProtectionFromMatchers currently aliases the input slice fields (m.Exact,
m.Prefix, m.Suffix, m.Contains) into the returned
dynamicpathdetector.OpenProtection which allows callers to mutate those slices
after Set and break the store's concurrency guarantees; fix it in
OpenProtectionFromMatchers by allocating new slices for each field (preserving
length/capacity as appropriate), copying the elements from
m.Exact/m.Prefix/m.Suffix/m.Contains into those new slices, and returning those
copies (handle nil inputs by leaving nil or returning empty slices consistently)
so the returned OpenProtection owns its own slice memory.
🧹 Nitpick comments (1)
pkg/registry/file/openprotection_test.go (1)

78-115: ⚡ Quick win

Add a malformed-ConfigMap regression case here.

The load-bearing behavior in reloadOnce is “parse failure logs and preserves the previously seeded protection.” This suite covers NotFound, but not the invalid-JSON path, so a future refactor could accidentally start clearing the store on bad operator data without a test catching it.

Example test case
 func TestOpenProtectionReloaderReloadOnce(t *testing.T) {
 	const ns, name = "kubescape", "storage-open-protection"
@@
 	t.Run("missing configmap keeps current protection", func(t *testing.T) {
 		client := fake.NewSimpleClientset() // no configmap
 		store := NewOpenProtectionStore(armotypes.OpenMatchers{Prefix: []string{"/etc/shadow"}})
 		r := NewOpenProtectionReloader(client, ns, name, time.Minute, store)
 		if err := r.reloadOnce(context.Background()); err != nil {
 			t.Fatalf("reloadOnce should tolerate NotFound: %v", err)
 		}
 		if got := store.Get(); len(got.Prefix) != 1 || got.Prefix[0] != "/etc/shadow" {
 			t.Fatalf("expected seeded protection preserved on NotFound, got %+v", got)
 		}
 	})
+
+	t.Run("invalid configmap keeps current protection", func(t *testing.T) {
+		cm := &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Namespace: ns, Name: name},
+			Data:       map[string]string{OpenProtectionConfigMapKey: "{not json"},
+		}
+		client := fake.NewSimpleClientset(cm)
+		store := NewOpenProtectionStore(armotypes.OpenMatchers{Prefix: []string{"/etc/shadow"}})
+		r := NewOpenProtectionReloader(client, ns, name, time.Minute, store)
+		if err := r.reloadOnce(context.Background()); err == nil {
+			t.Fatal("expected parse error")
+		}
+		if got := store.Get(); len(got.Prefix) != 1 || got.Prefix[0] != "/etc/shadow" {
+			t.Fatalf("expected seeded protection preserved on parse error, got %+v", got)
+		}
+	})
🤖 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/openprotection_test.go` around lines 78 - 115, Add a test
that ensures reloadOnce preserves the existing OpenProtectionStore when the
ConfigMap exists but contains malformed JSON: create a fake client with a
ConfigMap named as in NewOpenProtectionReloader(ns,name) whose
Data[OpenProtectionConfigMapKey] is invalid JSON, seed the store via
NewOpenProtectionStore(armotypes.OpenMatchers{Prefix: []string{"/etc/shadow"}}),
call r.reloadOnce(context.Background()), assert no error that would clear the
store and that store.Get() still contains the original prefix; reference
reloadOnce, NewOpenProtectionReloader, NewOpenProtectionStore, and
OpenProtectionConfigMapKey to locate the code under test.
🤖 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/analyzer.go`:
- Around line 474-479: The current logic in updateNodeStats doesn't enforce
PinnedSubtreeBudget for the immediate overflowing insert path because
handleNewSegment increments parent.Count after updateNodeStats runs; modify the
flow so that after a new child is inserted (in handleNewSegment or
processSegment) you immediately re-check the parent's count and call
updateNodeStats (or perform the collapse) when threshold ==
NeverCollapseThreshold && parent.Count > PinnedSubtreeBudget; alternatively
implement the collapse directly in processSegment under the same condition to
guarantee collapse instead of leaving the subtree literal above budget, and add
a regression test that creates exactly PinnedSubtreeBudget+1 unique children to
validate the boundary behavior.

In `@pkg/registry/file/openprotection.go`:
- Around line 107-117: NewOpenProtectionReloader currently accepts nil
dependencies and causes a later panic in reloadOnce; update
NewOpenProtectionReloader to validate that client and store are non-nil up front
(and do the same validation for any other NewOpenProtectionReloader-like
constructors), and fail fast with a clear error (e.g., panic or log.Fatalf) if
either client == nil or store == nil so the misconfiguration is rejected eagerly
instead of causing a runtime panic in reloadOnce or other methods on
OpenProtectionReloader.

---

Outside diff comments:
In `@pkg/registry/file/containerprofile_processor.go`:
- Around line 92-98: OpenProtectionFromMatchers currently aliases the input
slice fields (m.Exact, m.Prefix, m.Suffix, m.Contains) into the returned
dynamicpathdetector.OpenProtection which allows callers to mutate those slices
after Set and break the store's concurrency guarantees; fix it in
OpenProtectionFromMatchers by allocating new slices for each field (preserving
length/capacity as appropriate), copying the elements from
m.Exact/m.Prefix/m.Suffix/m.Contains into those new slices, and returning those
copies (handle nil inputs by leaving nil or returning empty slices consistently)
so the returned OpenProtection owns its own slice memory.

---

Nitpick comments:
In `@pkg/registry/file/openprotection_test.go`:
- Around line 78-115: Add a test that ensures reloadOnce preserves the existing
OpenProtectionStore when the ConfigMap exists but contains malformed JSON:
create a fake client with a ConfigMap named as in
NewOpenProtectionReloader(ns,name) whose Data[OpenProtectionConfigMapKey] is
invalid JSON, seed the store via
NewOpenProtectionStore(armotypes.OpenMatchers{Prefix: []string{"/etc/shadow"}}),
call r.reloadOnce(context.Background()), assert no error that would clear the
store and that store.Get() still contains the original prefix; reference
reloadOnce, NewOpenProtectionReloader, NewOpenProtectionStore, and
OpenProtectionConfigMapKey to locate the code under test.
🪄 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: a23f9d7d-a1f4-46a0-ae2d-4200e1bd8ab3

📥 Commits

Reviewing files that changed from the base of the PR and between cb033d5 and 6b12918.

📒 Files selected for processing (11)
  • main.go
  • pkg/apiserver/apiserver.go
  • pkg/cmd/server/start.go
  • pkg/config/config.go
  • pkg/registry/file/containerprofile_processor.go
  • pkg/registry/file/dynamicpathdetector/analyzer.go
  • pkg/registry/file/dynamicpathdetector/protection.go
  • pkg/registry/file/dynamicpathdetector/protection_test.go
  • pkg/registry/file/dynamicpathdetector/types.go
  • pkg/registry/file/openprotection.go
  • pkg/registry/file/openprotection_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • pkg/config/config.go
  • pkg/registry/file/dynamicpathdetector/types.go
  • pkg/registry/file/dynamicpathdetector/protection.go

Comment on lines 474 to +479
func (ua *PathAnalyzer) updateNodeStats(node *SegmentNode, threshold int) {
if node.Count > threshold && !node.IsNextDynamic() {
effectiveThreshold := threshold
if threshold == NeverCollapseThreshold && node.Count > PinnedSubtreeBudget {
effectiveThreshold = PinnedSubtreeBudget
}
if node.Count > effectiveThreshold && !node.IsNextDynamic() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce the pinned-subtree budget on the overflowing insert.

This fallback only runs when the node is revisited. The path that first takes a protected directory from PinnedSubtreeBudget to PinnedSubtreeBudget+1 increments the parent's Count inside handleNewSegment, after updateNodeStats has already checked that parent on the current walk. If learning stops there, the subtree stays literal above budget, which breaks the stated "collapse instead of TooLarge" tradeoff. Please re-check the parent immediately after inserting a new child, or collapse directly from processSegment when threshold == NeverCollapseThreshold && node.Count > PinnedSubtreeBudget, and add a boundary regression with exactly PinnedSubtreeBudget+1 unique children.

Possible fix sketch
func (ua *PathAnalyzer) processSegment(node *SegmentNode, segment string, threshold int) *SegmentNode {
	...
-	return ua.handleNewSegment(node, segment)
+	child := ua.handleNewSegment(node, segment)
+	if threshold == NeverCollapseThreshold && node.Count > PinnedSubtreeBudget {
+		return ua.createDynamicNode(node)
+	}
+	return child
}
🤖 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/analyzer.go` around lines 474 - 479,
The current logic in updateNodeStats doesn't enforce PinnedSubtreeBudget for the
immediate overflowing insert path because handleNewSegment increments
parent.Count after updateNodeStats runs; modify the flow so that after a new
child is inserted (in handleNewSegment or processSegment) you immediately
re-check the parent's count and call updateNodeStats (or perform the collapse)
when threshold == NeverCollapseThreshold && parent.Count > PinnedSubtreeBudget;
alternatively implement the collapse directly in processSegment under the same
condition to guarantee collapse instead of leaving the subtree literal above
budget, and add a regression test that creates exactly PinnedSubtreeBudget+1
unique children to validate the boundary behavior.

Comment on lines +107 to +117
func NewOpenProtectionReloader(client kubernetes.Interface, namespace, name string, interval time.Duration, store *OpenProtectionStore) *OpenProtectionReloader {
if interval <= 0 {
interval = DefaultOpenProtectionRefreshInterval
}
return &OpenProtectionReloader{
client: client,
namespace: namespace,
name: name,
interval: interval,
store: store,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate client and store up front.

reloadOnce dereferences both unconditionally, so NewOpenProtectionReloader(nil, ..., nil) builds an object that only fails later with a panic. Since this constructor is exported, it should reject invalid dependencies eagerly instead of turning miswiring into a runtime crash.

Possible fix
-func NewOpenProtectionReloader(client kubernetes.Interface, namespace, name string, interval time.Duration, store *OpenProtectionStore) *OpenProtectionReloader {
+func NewOpenProtectionReloader(client kubernetes.Interface, namespace, name string, interval time.Duration, store *OpenProtectionStore) (*OpenProtectionReloader, error) {
+	if client == nil {
+		return nil, fmt.Errorf("open-protection reloader client is nil")
+	}
+	if store == nil {
+		return nil, fmt.Errorf("open-protection reloader store is nil")
+	}
 	if interval <= 0 {
 		interval = DefaultOpenProtectionRefreshInterval
 	}
-	return &OpenProtectionReloader{
+	return &OpenProtectionReloader{
 		client:    client,
 		namespace: namespace,
 		name:      name,
 		interval:  interval,
 		store:     store,
-	}
+	}, nil
 }

Also applies to: 123-139

🤖 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/openprotection.go` around lines 107 - 117,
NewOpenProtectionReloader currently accepts nil dependencies and causes a later
panic in reloadOnce; update NewOpenProtectionReloader to validate that client
and store are non-nil up front (and do the same validation for any other
NewOpenProtectionReloader-like constructors), and fail fast with a clear error
(e.g., panic or log.Fatalf) if either client == nil or store == nil so the
misconfiguration is rejected eagerly instead of causing a runtime panic in
reloadOnce or other methods on OpenProtectionReloader.

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant