Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA projection subsystem was added: rules compile into RuleProjectionSpec, installed into the container-profile cache; container profiles are projected into compact ProjectedContainerProfile via Compile/Apply/Trie; CEL libraries now use projected profiles; metrics were extended; many tests and go.mod dependency declarations updated. Changes
Sequence Diagram(s)sequenceDiagram
participant RM as RuleManager
participant QC as CompileSpec
participant CPC as ContainerProfileCache
participant OC as ObjectCache
participant CEL as CEL / Libraries
participant MM as MetricsManager
RM->>QC: Compile rules -> RuleProjectionSpec (hash)
QC-->>RM: RuleProjectionSpec
RM->>CPC: SetProjectionSpec(spec)
CPC-->>RM: ack (idempotent)
Note over CPC: On nudge or tick -> refreshAllEntries (coalesced)
CPC->>OC: For each cached entry, build call-stack tree
CPC->>CPC: Apply(spec, cp, tree) -> ProjectedContainerProfile
CPC->>MM: ObserveProjectionApplyDuration / SetProjectionStaleEntries
CEL->>OC: GetProjectedContainerProfile(containerID) -> ProjectedContainerProfile
CEL->>MM: IncHelperCall / IncProjectionUndeclaredLiteral / other metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 7
🧹 Nitpick comments (7)
pkg/objectcache/v1/mock.go (1)
123-123: CopyPolicyByRuleIdinstead of aliasing source map.Line 123 currently shares the original map reference. A shallow copy keeps mock outputs isolated and prevents accidental cross-test mutation coupling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/v1/mock.go` at line 123, The assignment PolicyByRuleId: cp.Spec.PolicyByRuleId currently aliases the original map; change it to set PolicyByRuleId to a shallow copy of cp.Spec.PolicyByRuleId by allocating a new map of the same type and copying each key/value from cp.Spec.PolicyByRuleId into it so the mock output is isolated from the source (locate the construction that sets PolicyByRuleId in pkg/objectcache/v1/mock.go where cp and cp.Spec.PolicyByRuleId are used).pkg/objectcache/containerprofilecache/projection_trie.go (1)
5-7: Docstring mismatch: implementation is rune-level, not byte-level.The trie stores
map[rune]*trie, so the comment should say rune-level prefix trie.Suggested wording fix
-// trie implements a simple byte-level prefix trie for O(n) prefix matching +// trie implements a simple rune-level prefix trie for O(n) prefix matchingAlso applies to: 9-9
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/projection_trie.go` around lines 5 - 7, Update the package comment and any related docstrings to say "rune-level prefix trie" instead of "byte-level" (the implementation uses map[rune]*trie), e.g., change the top-of-file comment describing trie and the similar comment around line 9; reference the trie type and its use by FieldSpec for prefix/suffix matching to ensure the wording accurately reflects rune-level matching.pkg/config/config.go (1)
32-38: Fix the misplaced struct doc comment.The comment above
ProfileProjectionConfigcurrently describesEventDedupConfig, which makes the docs misleading.Suggested doc-comment cleanup
-// EventDedupConfig controls eBPF event deduplication before CEL rule evaluation. -// ProfileProjectionConfig controls rule-aware profile projection behaviour. +// ProfileProjectionConfig controls rule-aware profile projection behaviour. type ProfileProjectionConfig struct { // StrictValidation rejects rules with profileDependency>0 but no // profileDataRequired, instead of merely logging. Default false (soft mode). StrictValidation bool `mapstructure:"strictValidation"` } +// EventDedupConfig controls eBPF event deduplication before CEL rule evaluation. type EventDedupConfig struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` around lines 32 - 38, The top-of-file doc comment is attached to the wrong struct: the line "// EventDedupConfig controls eBPF event deduplication before CEL rule evaluation." sits above ProfileProjectionConfig. Move or split the comments so EventDedupConfig has the eBPF/event-deduplication comment and ProfileProjectionConfig has its own doc describing rule-aware profile projection and the StrictValidation field; update the comments adjacent to the struct declarations (EventDedupConfig and ProfileProjectionConfig) so each description matches the correct type name.pkg/objectcache/containerprofilecache/shared_pointer_race_test.go (1)
81-100: Verify seeded entry uses installed projection spec.The projection spec is installed at lines 82-87, but
cpc.Apply(nil, cpV1, nil)at line 91 passesnilfor the spec parameter. This means the seeded entry'sProjectedfield won't reflect the installed spec's requirements (e.g.,All: truefor execs/opens/capabilities).If the intent is to test concurrent access with properly projected data, consider:
♻️ Use installed spec when seeding
+ spec := objectcache.RuleProjectionSpec{ + Execs: objectcache.FieldSpec{InUse: true, All: true}, + Opens: objectcache.FieldSpec{InUse: true, All: true}, + Capabilities: objectcache.FieldSpec{InUse: true, All: true}, + Hash: "race-test", + } + cache.SetProjectionSpec(spec) seedV1 := func() { cache.SeedEntryForTest(id, &cpc.CachedContainerProfile{ - Projected: cpc.Apply(nil, cpV1, nil), + Projected: cpc.Apply(&spec, cpV1, nil),However, if the test's goal is purely to detect data races (not projection correctness), the current approach is acceptable since
Applystill produces a validProjectedContainerProfilestruct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/shared_pointer_race_test.go` around lines 81 - 100, The seeded entry uses cpc.Apply(nil, cpV1, nil) which ignores the installed projection spec set by SetProjectionSpec; update seedV1 to pass the same RuleProjectionSpec used above (or store that spec in a variable) into cpc.Apply so the CachedContainerProfile.Projected reflects the installed spec (reference: SetProjectionSpec, objectcache.RuleProjectionSpec, seedV1, cpc.Apply, cache.SeedEntryForTest, cpV1).pkg/objectcache/containerprofilecache/projection_compile.go (1)
151-154: Simplify hash formatting—intermediate buffer is unnecessary.The buffer write and read-back can be replaced with direct formatting.
♻️ Proposed simplification
- var buf [8]byte - binary.LittleEndian.PutUint64(buf[:], h.Sum64()) - return fmt.Sprintf("%016x", binary.LittleEndian.Uint64(buf[:])) + return fmt.Sprintf("%016x", h.Sum64())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/projection_compile.go` around lines 151 - 154, The code in projection_compile.go uses an unnecessary intermediate 8-byte buffer to format a uint64 hash; replace the buffer write/read with a direct call to fmt.Sprintf using the hash value returned by h.Sum64() (i.e., change the tail of the function that currently builds buf and calls binary.LittleEndian.Uint64 to simply return fmt.Sprintf("%016x", h.Sum64())). Ensure no other logic changes and keep the same zero-padded 16-hex-digit format.pkg/rulemanager/types/v1/profiledata.go (1)
94-103: Consider addingMarshalYAMLfor symmetry if YAML output is needed.
UnmarshalYAMLis implemented butMarshalYAMLis not. IfFieldRequirementis ever serialized to YAML (e.g., for debugging or config generation), the default struct marshaling won't emit"all"ornullcorrectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/types/v1/profiledata.go` around lines 94 - 103, FieldRequirement implements custom JSON marshaling (MarshalJSON) and has UnmarshalYAML, but lacks a MarshalYAML counterpart so YAML encoding won't emit "all" or null consistently; add a MarshalYAML method on FieldRequirement mirroring MarshalJSON's logic: when !Declared return nil (or YAML null), when All return the string "all", otherwise return the Patterns slice so YAML emits the pattern list; implement it as a method named MarshalYAML on the same type to ensure YAML encoding matches MarshalJSON/UnmarshalYAML behavior.pkg/objectcache/containerprofilecache/projection_apply.go (1)
153-160: Consider documenting performance assumption forPatternsslice size.
slices.Containsis O(n) onPatterns. This is acceptable given dynamic paths are typically rare, but a brief comment noting this assumption would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/projection_apply.go` around lines 153 - 160, The isRetained function uses slices.Contains on pf.Patterns which is O(n); add a brief comment (near the isRetained function or the ProjectedField type) stating the performance assumption that pf.Patterns is expected to be small/rare so linear search is acceptable, and that Values uses map lookups for fast checks; reference ProjectedField, isRetained, pf.Patterns, pf.Values, and the slices.Contains call so maintainers know why no optimization (e.g., converting Patterns to a set) was chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/metricsmanager/prometheus/prometheus.go`:
- Around line 494-499: The five stub methods on PrometheusMetric
(IncMissingProfileDataRequired, IncWasHostAccessedWithoutEndpointsAll,
ReportProjectionSpecInstalled, IncProjectionEntryRebuilt, ReportProjectionApply)
must be backed by real Prometheus collectors: add appropriate metric fields to
the PrometheusMetric struct (e.g., counters for missing profile dependencies and
endpoint-access patterns, a counter or gauge for projection spec installs, a
counter for projection entry rebuilds, and a histogram or counter for projection
apply timing/outcome), initialize and register them in the PrometheusMetric
constructor/initializer (the same place other metrics are created), and then
implement each method to increment/observe the corresponding collector (include
sensible labels if used elsewhere). Ensure metric names and help strings follow
existing naming conventions in this file and reuse label sets already present
for similar metrics so the new metrics appear on /metrics and correspond to the
documented semantics.
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 117-125: Remove the dead fields convergenceCount and
lastRefreshedSpecHash from the container profile cache struct: locate the struct
that declares currentSpecMu, currentSpec, specGeneration, convergenceCount,
lastRefreshedSpecHash and nudge, then delete the convergenceCount and
lastRefreshedSpecHash fields (or replace them with a TODO comment if you intend
to implement reconciler metrics later). Also search for any references to
convergenceCount or lastRefreshedSpecHash (e.g., in methods like refreshOneEntry
or refreshAllEntries) and remove or update those references to keep the code
compilable.
In `@pkg/objectcache/v1/mock.go`:
- Around line 171-176: The mock currently overwrites
pcp.Endpoints.EndpointMethodsByPath[e.Endpoint] for each cp.Spec.Endpoints
entry, dropping methods when multiple entries share the same endpoint; update
the loop that handles cp.Spec.Endpoints so it unions methods instead of
replacing them: for each e use the existing slice at
pcp.Endpoints.EndpointMethodsByPath[e.Endpoint] (if present) to build a set, add
all e.Methods into that set, then write back the combined unique-methods slice
into pcp.Endpoints.EndpointMethodsByPath[e.Endpoint]; reference symbols:
cp.Spec.Endpoints, e.Endpoint, e.Methods, and
pcp.Endpoints.EndpointMethodsByPath.
In `@pkg/rulemanager/cel/libraries/applicationprofile/capability.go`:
- Around line 29-33: The current check in capability.go short-circuits on
cp.Capabilities.All and returns true for any queried capability, causing false
positives; update the logic to only return true when the queried capability name
(capabilityNameStr) is actually present in cp.Capabilities.Values (the observed
set). In other words, remove or disable the unconditional "if
cp.Capabilities.All { return types.Bool(true) }" branch and make the function
rely on the map membership check (cp.Capabilities.Values[capabilityNameStr]) to
determine usage; keep using capabilityNameStr and cp.Capabilities.Values as the
identifying symbols to locate and fix the code.
In `@pkg/rulemanager/cel/libraries/applicationprofile/http.go`:
- Around line 238-248: The info-level logger in was_host_accessed
(logger.L().Info call when cp.Endpoints.All==false) is too noisy; change it to
debug-level or implement a one-time-per-container warning using a sync.Map or
sync.Once keyed by containerIDStr so the message is emitted only once per
container; update the logging call site in was_host_accessed (and related
handling of cp.Endpoints.Values / Patterns) to use logger.L().Debug(...) or
guard the Info call with the one-time check so repeated calls do not flood logs.
In `@pkg/rulemanager/cel/libraries/applicationprofile/syscall.go`:
- Around line 29-33: The code incorrectly treats cp.Syscalls.All as proof that
any syscall exists; instead membership must be determined solely from
cp.Syscalls.Values. Remove or disable the early return that checks
cp.Syscalls.All in the syscall membership function (the block using
cp.Syscalls.All and returning types.Bool(true)) and ensure the function only
returns true when syscallNameStr is present in cp.Syscalls.Values (the map
lookup logic).
In `@pkg/rulemanager/rulepolicy.go`:
- Around line 22-27: Validate should guard against a nil projected profile
before accessing pcp.PolicyByRuleId to avoid a panic: in the
RulePolicyValidator.Validate method, add an early nil check (if pcp == nil) and
return false, nil (consistent with the existing missing-key behavior) before any
map dereference; update logic around the existing PolicyByRuleId lookups (the
uses of pcp.PolicyByRuleId and the ruleId variable) so they only run after the
nil check.
---
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 32-38: The top-of-file doc comment is attached to the wrong
struct: the line "// EventDedupConfig controls eBPF event deduplication before
CEL rule evaluation." sits above ProfileProjectionConfig. Move or split the
comments so EventDedupConfig has the eBPF/event-deduplication comment and
ProfileProjectionConfig has its own doc describing rule-aware profile projection
and the StrictValidation field; update the comments adjacent to the struct
declarations (EventDedupConfig and ProfileProjectionConfig) so each description
matches the correct type name.
In `@pkg/objectcache/containerprofilecache/projection_apply.go`:
- Around line 153-160: The isRetained function uses slices.Contains on
pf.Patterns which is O(n); add a brief comment (near the isRetained function or
the ProjectedField type) stating the performance assumption that pf.Patterns is
expected to be small/rare so linear search is acceptable, and that Values uses
map lookups for fast checks; reference ProjectedField, isRetained, pf.Patterns,
pf.Values, and the slices.Contains call so maintainers know why no optimization
(e.g., converting Patterns to a set) was chosen.
In `@pkg/objectcache/containerprofilecache/projection_compile.go`:
- Around line 151-154: The code in projection_compile.go uses an unnecessary
intermediate 8-byte buffer to format a uint64 hash; replace the buffer
write/read with a direct call to fmt.Sprintf using the hash value returned by
h.Sum64() (i.e., change the tail of the function that currently builds buf and
calls binary.LittleEndian.Uint64 to simply return fmt.Sprintf("%016x",
h.Sum64())). Ensure no other logic changes and keep the same zero-padded
16-hex-digit format.
In `@pkg/objectcache/containerprofilecache/projection_trie.go`:
- Around line 5-7: Update the package comment and any related docstrings to say
"rune-level prefix trie" instead of "byte-level" (the implementation uses
map[rune]*trie), e.g., change the top-of-file comment describing trie and the
similar comment around line 9; reference the trie type and its use by FieldSpec
for prefix/suffix matching to ensure the wording accurately reflects rune-level
matching.
In `@pkg/objectcache/containerprofilecache/shared_pointer_race_test.go`:
- Around line 81-100: The seeded entry uses cpc.Apply(nil, cpV1, nil) which
ignores the installed projection spec set by SetProjectionSpec; update seedV1 to
pass the same RuleProjectionSpec used above (or store that spec in a variable)
into cpc.Apply so the CachedContainerProfile.Projected reflects the installed
spec (reference: SetProjectionSpec, objectcache.RuleProjectionSpec, seedV1,
cpc.Apply, cache.SeedEntryForTest, cpV1).
In `@pkg/objectcache/v1/mock.go`:
- Line 123: The assignment PolicyByRuleId: cp.Spec.PolicyByRuleId currently
aliases the original map; change it to set PolicyByRuleId to a shallow copy of
cp.Spec.PolicyByRuleId by allocating a new map of the same type and copying each
key/value from cp.Spec.PolicyByRuleId into it so the mock output is isolated
from the source (locate the construction that sets PolicyByRuleId in
pkg/objectcache/v1/mock.go where cp and cp.Spec.PolicyByRuleId are used).
In `@pkg/rulemanager/types/v1/profiledata.go`:
- Around line 94-103: FieldRequirement implements custom JSON marshaling
(MarshalJSON) and has UnmarshalYAML, but lacks a MarshalYAML counterpart so YAML
encoding won't emit "all" or null consistently; add a MarshalYAML method on
FieldRequirement mirroring MarshalJSON's logic: when !Declared return nil (or
YAML null), when All return the string "all", otherwise return the Patterns
slice so YAML emits the pattern list; implement it as a method named MarshalYAML
on the same type to ensure YAML encoding matches MarshalJSON/UnmarshalYAML
behavior.
🪄 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: 39bae3da-f034-497f-9cc2-3f8e8a791e39
📒 Files selected for processing (38)
go.modpkg/config/config.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/init_eviction_test.gopkg/objectcache/containerprofilecache/lock_stress_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/projection_compile.gopkg/objectcache/containerprofilecache/projection_compile_test.gopkg/objectcache/containerprofilecache/projection_trie.gopkg/objectcache/containerprofilecache/projection_trie_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/shared_pointer_race_test.gopkg/objectcache/containerprofilecache/t8_overlay_refresh_test.gopkg/objectcache/containerprofilecache_interface.gopkg/objectcache/projection_types.gopkg/objectcache/v1/mock.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/applicationprofile/capability.gopkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/applicationprofile/http.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/syscall.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/cel/libraries/networkneighborhood/network.gopkg/rulemanager/cel/libraries/networkneighborhood/nn.gopkg/rulemanager/profilehelper/profilehelper.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulepolicy.gopkg/rulemanager/types/v1/profiledata.gopkg/rulemanager/types/v1/profiledata_test.gopkg/rulemanager/types/v1/types.go
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
6992805 to
11947f5
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
11947f5 to
0d24ec7
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
0d24ec7 to
604c97a
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
118-124:⚠️ Potential issue | 🟡 MinorRemove the dead convergence-tracking fields.
convergenceCountandlastRefreshedSpecHashare still unused here, sogolangci-lintwill keep this file red.♻️ Proposed fix
// Projection spec — installed by SetProjectionSpec when rulemanager loads rules. currentSpecMu sync.RWMutex currentSpec *objectcache.RuleProjectionSpec specGeneration atomic.Int64 // bumped on each distinct spec hash change - convergenceCount atomic.Int64 // bumped by refreshOneEntry per converged entry - lastRefreshedSpecHash atomic.Value // string; set after refreshAllEntries completes nudge chan struct{} // buffered cap 1; signals reconciler on spec change🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/containerprofilecache.go` around lines 118 - 124, Remove the unused convergence-tracking fields to satisfy golangci-lint: delete the struct fields convergenceCount (atomic.Int64) and lastRefreshedSpecHash (atomic.Value) from the container profile cache struct and remove any references to them (e.g., in methods like refreshOneEntry or refreshAllEntries if present). Ensure no remaining code depends on these symbols and run tests/linter to confirm the file is no longer flagged.
🧹 Nitpick comments (3)
pkg/metricsmanager/metrics_manager_interface.go (1)
32-33: AlignReportProjectionSpecInstalledcontract with actual usage.
numSurfacesis part of the API but is currently dropped by implementations. Either emit it (e.g., gauge/histogram) or remove the parameter to keep the interface tight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/metricsmanager/metrics_manager_interface.go` around lines 32 - 33, The interface method ReportProjectionSpecInstalled currently accepts numSurfaces but implementations ignore it; remove the unused parameter from the contract by changing ReportProjectionSpecInstalled(numSurfaces int) to ReportProjectionSpecInstalled() and then update all implementations and call sites to the no-arg variant (search for implementations of ReportProjectionSpecInstalled and callers to update their signatures and invocations). If you prefer to keep the data, alternatively implement emitting numSurfaces as a gauge/histogram in the implementations (e.g., in the metrics manager's concrete types) instead of dropping it—pick one approach and apply it consistently across all implementations and callers.pkg/config/config.go (1)
32-38: Split the exported type comments.Line 32 still starts with
EventDedupConfig, so that doc comment now attaches toProfileProjectionConfigand leavesEventDedupConfigundocumented in godoc/lint output. Give each exported type its own name-prefixed comment directly above the declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` around lines 32 - 38, The comment block currently above ProfileProjectionConfig contains text for two exported types, causing EventDedupConfig to be undocumented; split the comment so each exported type has its own name-prefixed doc comment: add a comment starting with "EventDedupConfig ..." immediately above the EventDedupConfig type declaration and keep a separate comment starting with "ProfileProjectionConfig ..." immediately above the ProfileProjectionConfig type declaration (referencing the existing text for each), ensuring both comments begin with the exact type name so they attach correctly in godoc/lint.pkg/objectcache/containerprofilecache/projection_apply.go (1)
275-287: Sort the merged method list before storing it.
EndpointMethodsByPathis treated like a set here, but the final slice preserves first-seen order. Sorting it would make equivalent profiles project identically even when raw endpoint order changes, which aligns with the idempotency work already done forPatterns.♻️ Proposed fix
for _, m := range e.Methods { if _, ok := methodSet[m]; !ok { methodSet[m] = struct{}{} existing = append(existing, m) } } + slices.Sort(existing) pf.EndpointMethodsByPath[e.Endpoint] = existing } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/projection_apply.go` around lines 275 - 287, The merged methods slice stored into pf.EndpointMethodsByPath[e.Endpoint] should be deterministically ordered: after building/merging existing and e.Methods into the existing slice (using methodSet to de-duplicate), sort the resulting slice (e.g., via sort.Strings) before assigning it back so EndpointMethodsByPath behaves idempotently; update the block that manipulates existing/methodSet (referenced symbols: pf.EndpointMethodsByPath, existing, methodSet, e.Endpoint, e.Methods) to perform the sort immediately prior to the assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/objectcache/containerprofilecache/projection_apply.go`:
- Around line 209-214: The extractIngressDomains function only appends
n.DNSNames but omits the singular n.DNS value; update extractIngressDomains to
also check n.DNS and append it when non-empty (mirroring extractEgressDomains)
so both DNS and DNSNames from each cp.Spec.Ingress NetworkNeighbor are
collected.
In `@pkg/objectcache/containerprofilecache/reconciler.go`:
- Around line 51-59: The nudge handling currently drops a spec-change signal
when refreshInProgress is true; update the logic so a missed nudge is recorded
and triggers another refresh after the current run finishes: introduce an
atomic/boolean flag (e.g., refreshPending) or drain c.nudge and set that flag
when CompareAndSwap(false, true) fails inside the case handling for c.nudge,
then in the goroutine that defers refreshInProgress.Store(false) call
refreshAllEntries in a loop (or re-invoke it) while refreshPending is set,
clearing refreshPending before each additional pass; reference c.nudge,
c.refreshInProgress, c.refreshPending (new), and refreshAllEntries in
reconciler.go.
In `@pkg/objectcache/containerprofilecache/shared_pointer_race_test.go`:
- Around line 81-99: The seeded test entry restores an empty projection because
seedV1 calls cpc.Apply(nil, cpV1, nil); update seedV1 to apply the same
projection you installed with cache.SetProjectionSpec by passing that spec into
cpc.Apply (e.g., cpc.Apply(&yourRuleProjectionSpec, cpV1, nil) or inline the
same objectcache.RuleProjectionSpec literal), so the seeded
CachedContainerProfile uses the populated Projected fields and the test
exercises traversal of the real projected structures; reference
cache.SetProjectionSpec and seedV1/cpc.Apply(cpV1) when making the change.
In `@pkg/objectcache/v1/mock.go`:
- Around line 114-243: The mock currently ignores installed RuleProjectionSpec
(SetProjectionSpec is a no-op) and GetProjectedContainerProfile always returns
everything with All=true; change RuleObjectCacheMock to store the last
SetProjectionSpec (accepting objectcache.RuleProjectionSpec) on the mock struct
and have GetProjectedContainerProfile consult that spec when building the
ProjectedContainerProfile: apply filters from the RuleProjectionSpec to omit or
mark surfaces not retained, populate Patterns for dynamic path matches, update
PrefixHits/SuffixHits counters as entries are accepted, and only build per-path
composites (Execs.ExecArgsByPath, Opens.OpenFlagsByPath,
Endpoints.EndpointMethodsByPath, Egress/Ingress AddressPortsByAddr) for retained
paths; ensure SetProjectionSpec updates the stored spec and
GetProjectedContainerProfile uses it instead of unconditionally setting
All=true.
In `@pkg/rulebindingmanager/cache/cache.go`:
- Around line 196-199: The notification loop is sending on c.notifiers while
holding c.mutex, which can block; change Notify logic in cache.go to first
acquire c.mutex, copy the notifier channels from c.notifiers into a local slice
(or map of channels), then release c.mutex and iterate over the copied slice to
send rulebindingmanager.RuleBindingNotify{} to each notifier; this ensures the
sends do not occur under c.mutex and prevents blocking other cache operations.
In `@pkg/rulemanager/cel/libraries/cache/function_cache.go`:
- Around line 109-114: WithCache currently only applies the first extraKeyFn
(extraKeyFn[0]) which ignores additional callbacks and can cause cache-key
collisions; update FunctionCache.WithCache so it iterates over the entire
extraKeyFn slice and appends each extraKeyFn[i](values) to the generated key
(e.g., join results with a separator like "|") to ensure all provided key
augmenters contribute to the final cache key when invoking the returned
CelFunction.
In `@pkg/rulemanager/rule_manager.go`:
- Around line 394-397: The current check treats any cached profile with
Status==helpersv1.Completed as final; update the condition in
HasFinalApplicationProfile (the block that calls
rm.objectCache.ContainerProfileCache().GetContainerProfileState(utils.TrimRuntimePrefix(c.ContainerID)))
to also require Completion==helpersv1.Full and no error before returning true.
Concretely, change the if to check state != nil && state.Error == nil &&
state.Status == helpersv1.Completed && state.Completion == helpersv1.Full so
only fully completed profiles are considered final.
In `@pkg/rulemanager/types/v1/profiledata.go`:
- Around line 32-58: Reset the receiver structs before decoding to avoid
carrying over stale state: in ProfileDataRequired's custom unmarshallers
(UnmarshalJSON and UnmarshalYAML) set *p = ProfileDataRequired{} before calling
json.Unmarshal / value.Decode, and likewise in FieldRequirement's UnmarshalJSON
and UnmarshalYAML set *f = FieldRequirement{} (or the zero value of the
receiver) before decoding; keep the existing unknown-field checks and then
decode into the zeroed receiver so omitted fields from previous parses are not
retained.
- Around line 105-175: Reset the FieldRequirement receiver state at the start of
unmarshals to preserve the invariant that exactly one of All or Patterns is
meaningful: in FieldRequirement.UnmarshalJSON and
FieldRequirement.UnmarshalYAML, after setting f.Declared = true, explicitly
clear f.All = false and f.Patterns = nil (or empty slice) before attempting to
unmarshal so stale data from a reused receiver cannot persist across calls.
---
Duplicate comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 118-124: Remove the unused convergence-tracking fields to satisfy
golangci-lint: delete the struct fields convergenceCount (atomic.Int64) and
lastRefreshedSpecHash (atomic.Value) from the container profile cache struct and
remove any references to them (e.g., in methods like refreshOneEntry or
refreshAllEntries if present). Ensure no remaining code depends on these symbols
and run tests/linter to confirm the file is no longer flagged.
---
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 32-38: The comment block currently above ProfileProjectionConfig
contains text for two exported types, causing EventDedupConfig to be
undocumented; split the comment so each exported type has its own name-prefixed
doc comment: add a comment starting with "EventDedupConfig ..." immediately
above the EventDedupConfig type declaration and keep a separate comment starting
with "ProfileProjectionConfig ..." immediately above the ProfileProjectionConfig
type declaration (referencing the existing text for each), ensuring both
comments begin with the exact type name so they attach correctly in godoc/lint.
In `@pkg/metricsmanager/metrics_manager_interface.go`:
- Around line 32-33: The interface method ReportProjectionSpecInstalled
currently accepts numSurfaces but implementations ignore it; remove the unused
parameter from the contract by changing
ReportProjectionSpecInstalled(numSurfaces int) to
ReportProjectionSpecInstalled() and then update all implementations and call
sites to the no-arg variant (search for implementations of
ReportProjectionSpecInstalled and callers to update their signatures and
invocations). If you prefer to keep the data, alternatively implement emitting
numSurfaces as a gauge/histogram in the implementations (e.g., in the metrics
manager's concrete types) instead of dropping it—pick one approach and apply it
consistently across all implementations and callers.
In `@pkg/objectcache/containerprofilecache/projection_apply.go`:
- Around line 275-287: The merged methods slice stored into
pf.EndpointMethodsByPath[e.Endpoint] should be deterministically ordered: after
building/merging existing and e.Methods into the existing slice (using methodSet
to de-duplicate), sort the resulting slice (e.g., via sort.Strings) before
assigning it back so EndpointMethodsByPath behaves idempotently; update the
block that manipulates existing/methodSet (referenced symbols:
pf.EndpointMethodsByPath, existing, methodSet, e.Endpoint, e.Methods) to perform
the sort immediately prior to the assignment.
🪄 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: 8836ed85-0281-46a1-a2bc-4cf63b04c45e
📒 Files selected for processing (39)
go.modpkg/config/config.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/init_eviction_test.gopkg/objectcache/containerprofilecache/lock_stress_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/projection_compile.gopkg/objectcache/containerprofilecache/projection_compile_test.gopkg/objectcache/containerprofilecache/projection_trie.gopkg/objectcache/containerprofilecache/projection_trie_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/shared_pointer_race_test.gopkg/objectcache/containerprofilecache/t8_overlay_refresh_test.gopkg/objectcache/containerprofilecache_interface.gopkg/objectcache/projection_types.gopkg/objectcache/v1/mock.gopkg/rulebindingmanager/cache/cache.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/applicationprofile/capability.gopkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/applicationprofile/http.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/syscall.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/cel/libraries/networkneighborhood/network.gopkg/rulemanager/cel/libraries/networkneighborhood/nn.gopkg/rulemanager/profilehelper/profilehelper.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulepolicy.gopkg/rulemanager/types/v1/profiledata.gopkg/rulemanager/types/v1/profiledata_test.gopkg/rulemanager/types/v1/types.go
✅ Files skipped from review due to trivial changes (5)
- go.mod
- pkg/metricsmanager/metrics_manager_noop.go
- pkg/objectcache/projection_types.go
- pkg/rulemanager/cel/libraries/networkneighborhood/nn.go
- pkg/rulemanager/cel/libraries/applicationprofile/ap.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/rulemanager/types/v1/types.go
- pkg/rulemanager/cel/libraries/applicationprofile/syscall.go
- pkg/objectcache/containerprofilecache/projection_trie_test.go
- pkg/metricsmanager/metrics_manager_mock.go
- pkg/rulemanager/rulepolicy.go
- pkg/rulemanager/cel/libraries/applicationprofile/exec.go
- pkg/objectcache/containerprofilecache/t8_overlay_refresh_test.go
- pkg/rulemanager/cel/libraries/applicationprofile/open.go
- pkg/rulemanager/cel/libraries/networkneighborhood/network.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
604c97a to
390aa05
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/exec.go (1)
73-99:⚠️ Potential issue | 🟠 MajorDon't silently drop arg matching in
wasExecutedWithArgs.This now returns
truefor any projected path hit even when the profiled argv differs. Rules using the args-aware helper become much broader and can stop alerting on unexpected invocations. Please preserve the old args check on the raw profile path, or fail closed until exec args are projected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/libraries/applicationprofile/exec.go` around lines 73 - 99, The current wasExecutedWithArgs logic drops arg matching and returns true on any path hit; restore args-aware checks by using the parsed args from celparse.ParseList[string](args) to verify the stored entry for the raw path (cp.Execs.Values[pathStr]) actually contains a matching argv (use the same ExecArgsByPath semantics the code previously used), and only return true when both path and args match; if args cannot be projected/parsed or the profile lacks arg information, fail closed by returning cache.NewProfileNotAvailableErr("%v", err) (or a similar non-cached error) instead of treating a path-only hit as a match; keep the existing dynamic-segment loop (cp.Execs.Patterns + dynamicpathdetector.CompareDynamic) but apply the same args-check semantics for pattern matches too.
♻️ Duplicate comments (4)
pkg/rulebindingmanager/cache/cache.go (1)
196-199:⚠️ Potential issue | 🟠 MajorBlocking channel sends under
c.mutexlock.The notification loop sends on
c.notifierschannels while holding the write lock (acquired at line 189). If any notifier channel's buffer fills up, this blocks the entire cache and locks out other operations.This same pattern exists in
AddHandler,ModifyHandler, andDeleteHandler. Consider copying the notifiers slice before releasing the lock, then sending outside the critical section.Proposed fix
func (c *RBCache) RefreshRuleBindingsRules() { c.mutex.Lock() - defer c.mutex.Unlock() for _, rbName := range c.rbNameToRB.Keys() { rb := c.rbNameToRB.Get(rbName) c.rbNameToRules.Set(rbName, c.createRules(rb.Spec.Rules)) } + notifiers := append([]*chan rulebindingmanager.RuleBindingNotify(nil), c.notifiers...) + c.mutex.Unlock() + logger.L().Info("RBCache - refreshed rule bindings rules", helpers.Int("ruleBindings", len(c.rbNameToRB.Keys()))) // Notify listeners so the projection spec is recompiled with the updated rules. - for n := range c.notifiers { - *c.notifiers[n] <- rulebindingmanager.RuleBindingNotify{} + for _, ch := range notifiers { + *ch <- rulebindingmanager.RuleBindingNotify{} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulebindingmanager/cache/cache.go` around lines 196 - 199, The loop in cache.notify (and similar loops in AddHandler, ModifyHandler, DeleteHandler) sends on c.notifiers while holding c.mutex, which can block the cache if a notifier's channel is full; to fix, under the mutex copy the current slice/map of notifier channels (c.notifiers) into a local slice, then release the lock and iterate over the local copy to send rulebindingmanager.RuleBindingNotify{}, ensuring sends occur outside the critical section (optionally perform a non-blocking send or recover from closed channels as appropriate).pkg/rulemanager/rule_manager.go (1)
431-434:⚠️ Potential issue | 🟠 MajorRequire
Completion=Fullbefore reporting a final profile.
Status == Completedis also used for partial profiles, so this can report readiness too early and enable final-profile-dependent flows before profiling is actually complete.Suggested fix
- if state != nil && state.Error == nil { - // in theory, only completed profiles are stored in cache, but we check anyway - return state.Status == helpersv1.Completed + if state != nil && state.Error == nil { + return state.Status == helpersv1.Completed && + state.Completion == helpersv1.Full }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/rule_manager.go` around lines 431 - 434, The current check in rm.objectCache.ContainerProfileCache().GetContainerProfileState(...) returns final when state != nil && state.Error == nil && state.Status == helpersv1.Completed, which can be set for partial profiles; update the condition to also require state.Completion == helpersv1.Full (or the equivalent Completion Full enum/constant) so the function only reports a final profile when state.Error is nil, state.Status == helpersv1.Completed and state.Completion == helpersv1.Full.pkg/objectcache/containerprofilecache/shared_pointer_race_test.go (1)
81-92:⚠️ Potential issue | 🟡 MinorSeed the race fixture with the same projection spec you install in the cache.
cache.SetProjectionSpec(...)makes the real refresh path populate projected fields, butseedV1()still usescpc.Apply(nil, cpV1, nil), which restores an empty projection. That weakens the race window because readers spend much of it traversing nil maps instead of the populated projected structures this test is supposed to stress.Suggested fix
- cache.SetProjectionSpec(objectcache.RuleProjectionSpec{ + spec := objectcache.RuleProjectionSpec{ Execs: objectcache.FieldSpec{InUse: true, All: true}, Opens: objectcache.FieldSpec{InUse: true, All: true}, Capabilities: objectcache.FieldSpec{InUse: true, All: true}, Hash: "race-test", - }) + } + cache.SetProjectionSpec(spec) @@ cache.SeedEntryForTest(id, &cpc.CachedContainerProfile{ - Projected: cpc.Apply(nil, cpV1, nil), + Projected: cpc.Apply(&spec, cpV1, nil), State: &objectcache.ProfileState{Name: "cp-race"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/shared_pointer_race_test.go` around lines 81 - 92, The seeded fixture uses cpc.Apply(nil, cpV1, nil) which leaves Projected empty while you called cache.SetProjectionSpec(...); update seedV1 to apply the same projection spec used by the cache so the seeded CachedContainerProfile.Projected matches the real refresh path — locate seedV1 and replace the nil projection argument to cpc.Apply with the projection spec (the same struct passed to cache.SetProjectionSpec) so readers exercise the populated projected maps during the race test.pkg/objectcache/v1/mock.go (1)
114-215:⚠️ Potential issue | 🟠 MajorHonor the installed projection spec in the mock.
SetProjectionSpecis a no-op andGetProjectedContainerProfilereturns every populated surface withAll=true, so tests using this mock can pass even when the real cache would project those values away. Please store the lastRuleProjectionSpecand build projected profiles with the same projection logic as production.Suggested direction
type RuleObjectCacheMock struct { ... + projectionSpec objectcache.RuleProjectionSpec } func (r *RuleObjectCacheMock) GetProjectedContainerProfile(containerID string) *objectcache.ProjectedContainerProfile { cp := r.GetContainerProfile(containerID) if cp == nil { return nil } - // Build a minimal ProjectedContainerProfile from the raw CP so that CEL - // helpers and tests work without importing containerprofilecache. - // All surfaces use All=true so every raw entry is visible (test mock semantics). - pcp := &objectcache.ProjectedContainerProfile{ - PolicyByRuleId: cp.Spec.PolicyByRuleId, - } - ... - return pcp + return containerprofilecache.Apply(&r.projectionSpec, cp, nil) } -func (r *RuleObjectCacheMock) SetProjectionSpec(_ objectcache.RuleProjectionSpec) {} +func (r *RuleObjectCacheMock) SetProjectionSpec(spec objectcache.RuleProjectionSpec) { + r.projectionSpec = spec +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/v1/mock.go` around lines 114 - 215, The mock ignores the installed projection and always returns All=true; modify RuleObjectCacheMock so SetProjectionSpec(spec objectcache.RuleProjectionSpec) stores the spec on the receiver (e.g., r.projectionSpec) and change GetProjectedContainerProfile(containerID string) to consult that stored r.projectionSpec when constructing the objectcache.ProjectedContainerProfile: only populate each surface (Capabilities, Syscalls, Execs, Opens, Endpoints, EgressAddresses/EgressDomains, IngressAddresses/IngressDomains, PolicyByRuleId) according to the projection flags/fields in the stored RuleProjectionSpec instead of unconditionally setting All=true and returning every entry, preserving the existing construction logic but gating which maps/All flags are created based on the projection spec.
🧹 Nitpick comments (2)
pkg/rulemanager/cel/libraries/networkneighborhood/integration_test.go (1)
213-223: Clarify two test names to match what is actually being tested.At Line 214 and Line 220, the address exists; only the port is non-existent. Renaming will make failures easier to interpret.
Suggested rename diff
- name: "Check non-existent egress address with port and protocol", + name: "Check existing egress address with non-existent port and protocol", ... - name: "Check non-existent ingress address with port and protocol", + name: "Check existing ingress address with non-existent port and protocol",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/libraries/networkneighborhood/integration_test.go` around lines 213 - 223, The test case names are misleading: the address exists but the port/protocol do not. Update the name fields for the two test cases so they reflect "existing address with non-existent port and protocol" (rather than "non-existent address..."). Specifically, rename the test whose name is "Check non-existent egress address with port and protocol" (which uses nn.was_address_port_protocol_in_egress(containerID, "192.168.1.100", 9999, "TCP")) to something like "Check existing egress address with non-existent port and protocol", and rename "Check non-existent ingress address with port and protocol" (which uses nn.was_address_port_protocol_in_ingress(containerID, "172.16.0.10", 9999, "TCP")) to "Check existing ingress address with non-existent port and protocol".pkg/rulemanager/cel/libraries/applicationprofile/http.go (1)
65-86: Method matching removed for v1 — consider documenting the behavioral change.The method parameter is validated but ignored; only path membership is checked. This is a deliberate v1 scope reduction, but callers may expect method-aware behavior. Consider adding a comment in the function doc explaining this limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/libraries/applicationprofile/http.go` around lines 65 - 86, Add a short doc comment to the function containing the method.Value() validation (above the function declaration) stating that in v1 the method parameter is validated but ignored and only path membership is checked (EndpointMethodsByPath is out of scope), so callers should not expect method-aware matching; reference the existing code paths/profile lookup (profilehelper.GetProjectedContainerProfile), the endpoint collections (cp.Endpoints.Values and cp.Endpoints.Patterns), and the dynamic matching used (dynamicpathdetector.CompareDynamic) to make the scope limitation explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/metricsmanager/prometheus/prometheus.go`:
- Around line 397-399: Destroy() currently unregisters only three metrics; add
prometheus.Unregister calls for every metric you register when the
PrometheusMetric is created. Specifically, in Destroy() add unregisters for all
detailed projection metrics (the other cpProjection* variables defined alongside
cpProjectionMissingDeclCounter/cpProjectionUndeclaredLiteralCounter/cpProjectionStaleEntriesGauge)
and for all memory-savings metrics (the cpMemorySavings* variables defined near
the projection metrics), mirroring the set of prometheus.Register calls in the
constructor/initializer so every metric that was registered is unregistered.
In `@pkg/objectcache/containerprofilecache/projection_compile.go`:
- Around line 58-80: mergeField currently keeps previously-merged
Exact/Prefixes/Suffixes/Contains selectors when dst.All becomes true, which
causes dead selectors to affect hashSpec; fix by ensuring that when setting
dst.All = true you immediately drop any narrower selectors (clear dst.Exact map
and zero dst.Prefixes/dst.Suffixes/dst.Contains) and return, and also guard the
pattern-append loop so it no-ops if dst.All is already true (i.e., check dst.All
before appending new patterns). Update mergeField to clear existing selector
fields upon setting dst.All and to skip adding selectors when dst.All is true so
hashSpec won't include dead selectors.
In `@pkg/rulemanager/cel/libraries/applicationprofile/http.go`:
- Around line 251-262: The fallback strings.Contains(ep, hostStr) is too broad
and causes false positives; replace it with a stricter check: if url.Parse(ep)
fails, try extracting a host via net.SplitHostPort(ep) and compare that host to
hostStr, and if SplitHostPort also fails only accept exact equality (ep ==
hostStr) or host-with-port prefix (strings.HasPrefix(ep, hostStr+":" )) before
returning types.Bool(true). Update the logic around the allEndpoints loop and
variables allEndpoints and hostStr accordingly so only real host matches pass.
In `@pkg/rulemanager/rule_manager.go`:
- Around line 138-150: The loop currently unconditionally continues (dropping
the rule) when r.ProfileDependency > 0 && r.ProfileDataRequired == nil; change
it so the rule is only filtered out when strict validation is enabled: log and
call rm.metrics.IncMissingProfileDataRequired(r.ID) in all cases, but only
perform "continue" (i.e., do not append to filtered) when a strict flag is true
(use an existing flag like rm.strictMode or add a boolean field/parameter such
as strictValidation on RuleManager); otherwise append the rule to filtered so
soft-launch behavior is non-breaking.
---
Outside diff comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/exec.go`:
- Around line 73-99: The current wasExecutedWithArgs logic drops arg matching
and returns true on any path hit; restore args-aware checks by using the parsed
args from celparse.ParseList[string](args) to verify the stored entry for the
raw path (cp.Execs.Values[pathStr]) actually contains a matching argv (use the
same ExecArgsByPath semantics the code previously used), and only return true
when both path and args match; if args cannot be projected/parsed or the profile
lacks arg information, fail closed by returning
cache.NewProfileNotAvailableErr("%v", err) (or a similar non-cached error)
instead of treating a path-only hit as a match; keep the existing
dynamic-segment loop (cp.Execs.Patterns + dynamicpathdetector.CompareDynamic)
but apply the same args-check semantics for pattern matches too.
---
Duplicate comments:
In `@pkg/objectcache/containerprofilecache/shared_pointer_race_test.go`:
- Around line 81-92: The seeded fixture uses cpc.Apply(nil, cpV1, nil) which
leaves Projected empty while you called cache.SetProjectionSpec(...); update
seedV1 to apply the same projection spec used by the cache so the seeded
CachedContainerProfile.Projected matches the real refresh path — locate seedV1
and replace the nil projection argument to cpc.Apply with the projection spec
(the same struct passed to cache.SetProjectionSpec) so readers exercise the
populated projected maps during the race test.
In `@pkg/objectcache/v1/mock.go`:
- Around line 114-215: The mock ignores the installed projection and always
returns All=true; modify RuleObjectCacheMock so SetProjectionSpec(spec
objectcache.RuleProjectionSpec) stores the spec on the receiver (e.g.,
r.projectionSpec) and change GetProjectedContainerProfile(containerID string) to
consult that stored r.projectionSpec when constructing the
objectcache.ProjectedContainerProfile: only populate each surface (Capabilities,
Syscalls, Execs, Opens, Endpoints, EgressAddresses/EgressDomains,
IngressAddresses/IngressDomains, PolicyByRuleId) according to the projection
flags/fields in the stored RuleProjectionSpec instead of unconditionally setting
All=true and returning every entry, preserving the existing construction logic
but gating which maps/All flags are created based on the projection spec.
In `@pkg/rulebindingmanager/cache/cache.go`:
- Around line 196-199: The loop in cache.notify (and similar loops in
AddHandler, ModifyHandler, DeleteHandler) sends on c.notifiers while holding
c.mutex, which can block the cache if a notifier's channel is full; to fix,
under the mutex copy the current slice/map of notifier channels (c.notifiers)
into a local slice, then release the lock and iterate over the local copy to
send rulebindingmanager.RuleBindingNotify{}, ensuring sends occur outside the
critical section (optionally perform a non-blocking send or recover from closed
channels as appropriate).
In `@pkg/rulemanager/rule_manager.go`:
- Around line 431-434: The current check in
rm.objectCache.ContainerProfileCache().GetContainerProfileState(...) returns
final when state != nil && state.Error == nil && state.Status ==
helpersv1.Completed, which can be set for partial profiles; update the condition
to also require state.Completion == helpersv1.Full (or the equivalent Completion
Full enum/constant) so the function only reports a final profile when
state.Error is nil, state.Status == helpersv1.Completed and state.Completion ==
helpersv1.Full.
---
Nitpick comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/http.go`:
- Around line 65-86: Add a short doc comment to the function containing the
method.Value() validation (above the function declaration) stating that in v1
the method parameter is validated but ignored and only path membership is
checked (EndpointMethodsByPath is out of scope), so callers should not expect
method-aware matching; reference the existing code paths/profile lookup
(profilehelper.GetProjectedContainerProfile), the endpoint collections
(cp.Endpoints.Values and cp.Endpoints.Patterns), and the dynamic matching used
(dynamicpathdetector.CompareDynamic) to make the scope limitation explicit.
In `@pkg/rulemanager/cel/libraries/networkneighborhood/integration_test.go`:
- Around line 213-223: The test case names are misleading: the address exists
but the port/protocol do not. Update the name fields for the two test cases so
they reflect "existing address with non-existent port and protocol" (rather than
"non-existent address..."). Specifically, rename the test whose name is "Check
non-existent egress address with port and protocol" (which uses
nn.was_address_port_protocol_in_egress(containerID, "192.168.1.100", 9999,
"TCP")) to something like "Check existing egress address with non-existent port
and protocol", and rename "Check non-existent ingress address with port and
protocol" (which uses nn.was_address_port_protocol_in_ingress(containerID,
"172.16.0.10", 9999, "TCP")) to "Check existing ingress address with
non-existent port and protocol".
🪄 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: dfd82e45-3b61-4fd0-9ba4-7af434ab06e0
📒 Files selected for processing (45)
cmd/main.gogo.modpkg/config/config.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/init_eviction_test.gopkg/objectcache/containerprofilecache/lock_stress_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/projection_compile.gopkg/objectcache/containerprofilecache/projection_compile_test.gopkg/objectcache/containerprofilecache/projection_trie.gopkg/objectcache/containerprofilecache/projection_trie_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/shared_pointer_race_test.gopkg/objectcache/containerprofilecache/t8_overlay_refresh_test.gopkg/objectcache/containerprofilecache_interface.gopkg/objectcache/projection_types.gopkg/objectcache/v1/mock.gopkg/rulebindingmanager/cache/cache.gopkg/rulemanager/cel/cel.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/applicationprofile/capability.gopkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/applicationprofile/exec_test.gopkg/rulemanager/cel/libraries/applicationprofile/http.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/open_test.gopkg/rulemanager/cel/libraries/applicationprofile/syscall.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/cel/libraries/networkneighborhood/integration_test.gopkg/rulemanager/cel/libraries/networkneighborhood/network.gopkg/rulemanager/cel/libraries/networkneighborhood/network_test.gopkg/rulemanager/cel/libraries/networkneighborhood/nn.gopkg/rulemanager/profilehelper/profilehelper.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulepolicy.gopkg/rulemanager/types/v1/profiledata.gopkg/rulemanager/types/v1/profiledata_test.gopkg/rulemanager/types/v1/types.go
✅ Files skipped from review due to trivial changes (3)
- go.mod
- pkg/objectcache/containerprofilecache/projection_trie.go
- pkg/objectcache/containerprofilecache/projection_compile_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/rulemanager/cel/libraries/applicationprofile/syscall.go
- pkg/rulemanager/types/v1/types.go
- pkg/rulemanager/profilehelper/profilehelper.go
- pkg/objectcache/containerprofilecache/lock_stress_test.go
- pkg/objectcache/containerprofilecache/init_eviction_test.go
- pkg/rulemanager/types/v1/profiledata_test.go
- pkg/rulemanager/cel/libraries/networkneighborhood/network.go
- pkg/metricsmanager/metrics_manager_interface.go
- pkg/metricsmanager/metrics_manager_noop.go
- pkg/rulemanager/cel/libraries/networkneighborhood/nn.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
390aa05 to
a2c38bd
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
a2c38bd to
cabe39a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/objectcache/containerprofilecache/reconciler.go (1)
93-97:⚠️ Potential issue | 🟠 MajorTick-triggered refreshes still drop mid-flight spec nudges.
The trailing-edge
refreshPendingloop only exists in the nudge path. If a periodic tick startsrefreshAllEntries()andSetProjectionSpec()fires while that refresh is running, the nudge setsrefreshPending=true, but this goroutine exits after a single pass and clearsrefreshInProgress, so reprojection can still be delayed until the next tick. Reuse the samefor c.refreshPending.Swap(false)loop here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/containerprofilecache/reconciler.go` around lines 93 - 97, The tick-triggered goroutine currently runs a single call to c.refreshAllEntries() and then clears c.refreshInProgress, which allows a concurrent SetProjectionSpec nudge to set c.refreshPending=true but get dropped until the next tick; change the goroutine started when c.refreshInProgress.CompareAndSwap(false, true) to loop like the nudge path using for c.refreshPending.Swap(false) { c.refreshAllEntries(ctx) } (or equivalent) and only defer c.refreshInProgress.Store(false) after the loop so any pending refreshes set by SetProjectionSpec are handled before clearing refreshInProgress; reference c.refreshInProgress, c.refreshPending, and c.refreshAllEntries to locate the code to modify.
🧹 Nitpick comments (1)
pkg/rulemanager/cel/libraries/applicationprofile/exec.go (1)
91-99: Duplicated exec matching logic could be extracted.Lines 91-99 duplicate the exact same matching logic as lines 41-49 in
wasExecuted. Consider extracting this to a helper function.♻️ Suggested helper extraction
+func (l *apLibrary) matchExecPath(cp *objectcache.ProjectedContainerProfile, pathStr string) bool { + if _, ok := cp.Execs.Values[pathStr]; ok { + return true + } + for _, execPath := range cp.Execs.Patterns { + if dynamicpathdetector.CompareDynamic(execPath, pathStr) { + return true + } + } + return false +}Then both
wasExecutedandwasExecutedWithArgscan calll.matchExecPath(cp, pathStr).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/libraries/applicationprofile/exec.go` around lines 91 - 99, Extract the duplicated exec matching logic into a helper function (e.g., matchExecPath(cp, pathStr) bool) that checks cp.Execs.Values for pathStr and iterates cp.Execs.Patterns using dynamicpathdetector.CompareDynamic, returning true on a match and false otherwise; then replace the duplicated blocks in wasExecuted and wasExecutedWithArgs to call matchExecPath and convert its bool result to types.Bool where needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/exec.go`:
- Around line 73-77: Update the user-facing documentation (README and docs/) to
explicitly state that in v1 the library degrades argument matching to path-only:
wasExecutedWithArgs(containerID, "/bin/sh", ["a","b"]) currently behaves the
same as wasExecuted for path matching and does not validate the argument list;
reference the behavior implemented in
pkg/rulemanager/cel/libraries/applicationprofile/exec.go (the args
projection/degradation and celparse.ParseList usage) and the tests in
exec_test.go that verify this behavior so rule authors understand the limitation
and expected semantics.
In `@pkg/rulemanager/cel/libraries/cache/function_cache.go`:
- Around line 82-103: HashForContainerProfile currently returns only
pcp.SpecHash which doesn't change when profile content (CP/AP/NN) is updated;
update the cache key generated in HashForContainerProfile (inside the returned
func in function HashForContainerProfile) to append a per-entry content
fingerprint/generation in addition to pcp.SpecHash so cache invalidation also
occurs on content updates. Locate the call chain
oc.ContainerProfileCache().GetProjectedContainerProfile(containerIDStr) and,
when pcp != nil, build the suffix as SpecHash + delimiter + content fingerprint
(e.g., pcp.Generation or pcp.ContentFingerprint — choose the existing field that
represents profile content/version; if none exists compute a stable content
hash/fingerprint from the profile's mutable fields) and return that combined
string; ensure nil/empty fingerprints degrade safely to just SpecHash.
In `@pkg/rulemanager/rule_manager.go`:
- Around line 115-126: The notifier loop currently calls
r.recompileProjectionSpec() for every message and can let bursts fill
specNotify; change the handler that reads from specNotify (the goroutine started
after ruleBindingCache.AddNotifier) so that when it receives one notification
(case <-specNotify) it first drains any currently queued notifications from the
specNotify channel in a non-blocking loop (e.g., a select with case
<-specNotify: ... default: break) before calling r.recompileProjectionSpec(),
ensuring only one compile per burst while still honoring ctx.Done().
In `@pkg/rulemanager/types/v1/profiledata.go`:
- Around line 122-133: The JSON/YAML decoding for the pattern list currently
ignores unknown keys inside each PatternObject, so update the parsing in the
FieldRequirement unmarshalling path to reject unknown fields: decode each
element using a decoder with DisallowUnknownFields (or decode into a
map[string]json.RawMessage and compare keys) and return a clear error if any
extra key is present; keep the subsequent per-item validate() checks on
PatternObject unchanged. Also apply the same unknown-key rejection to the other
parsing site that handles PatternObject lists (the second block referenced
alongside PatternObject.validate). Ensure error messages include the pattern
index and the unknown key name for easier debugging.
---
Duplicate comments:
In `@pkg/objectcache/containerprofilecache/reconciler.go`:
- Around line 93-97: The tick-triggered goroutine currently runs a single call
to c.refreshAllEntries() and then clears c.refreshInProgress, which allows a
concurrent SetProjectionSpec nudge to set c.refreshPending=true but get dropped
until the next tick; change the goroutine started when
c.refreshInProgress.CompareAndSwap(false, true) to loop like the nudge path
using for c.refreshPending.Swap(false) { c.refreshAllEntries(ctx) } (or
equivalent) and only defer c.refreshInProgress.Store(false) after the loop so
any pending refreshes set by SetProjectionSpec are handled before clearing
refreshInProgress; reference c.refreshInProgress, c.refreshPending, and
c.refreshAllEntries to locate the code to modify.
---
Nitpick comments:
In `@pkg/rulemanager/cel/libraries/applicationprofile/exec.go`:
- Around line 91-99: Extract the duplicated exec matching logic into a helper
function (e.g., matchExecPath(cp, pathStr) bool) that checks cp.Execs.Values for
pathStr and iterates cp.Execs.Patterns using dynamicpathdetector.CompareDynamic,
returning true on a match and false otherwise; then replace the duplicated
blocks in wasExecuted and wasExecutedWithArgs to call matchExecPath and convert
its bool result to types.Bool where needed.
🪄 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: e4ec9e70-83fb-4726-bc94-99aedb62bada
📒 Files selected for processing (45)
cmd/main.gogo.modpkg/config/config.gopkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/init_eviction_test.gopkg/objectcache/containerprofilecache/lock_stress_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/projection_compile.gopkg/objectcache/containerprofilecache/projection_compile_test.gopkg/objectcache/containerprofilecache/projection_trie.gopkg/objectcache/containerprofilecache/projection_trie_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/shared_pointer_race_test.gopkg/objectcache/containerprofilecache/t8_overlay_refresh_test.gopkg/objectcache/containerprofilecache_interface.gopkg/objectcache/projection_types.gopkg/objectcache/v1/mock.gopkg/rulebindingmanager/cache/cache.gopkg/rulemanager/cel/cel.gopkg/rulemanager/cel/libraries/applicationprofile/ap.gopkg/rulemanager/cel/libraries/applicationprofile/capability.gopkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/applicationprofile/exec_test.gopkg/rulemanager/cel/libraries/applicationprofile/http.gopkg/rulemanager/cel/libraries/applicationprofile/open.gopkg/rulemanager/cel/libraries/applicationprofile/open_test.gopkg/rulemanager/cel/libraries/applicationprofile/syscall.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/cel/libraries/networkneighborhood/integration_test.gopkg/rulemanager/cel/libraries/networkneighborhood/network.gopkg/rulemanager/cel/libraries/networkneighborhood/network_test.gopkg/rulemanager/cel/libraries/networkneighborhood/nn.gopkg/rulemanager/profilehelper/profilehelper.gopkg/rulemanager/rule_manager.gopkg/rulemanager/rulepolicy.gopkg/rulemanager/types/v1/profiledata.gopkg/rulemanager/types/v1/profiledata_test.gopkg/rulemanager/types/v1/types.go
✅ Files skipped from review due to trivial changes (8)
- pkg/rulemanager/types/v1/types.go
- go.mod
- pkg/objectcache/containerprofilecache/lock_stress_test.go
- pkg/objectcache/containerprofilecache/projection_trie_test.go
- pkg/objectcache/containerprofilecache/init_eviction_test.go
- pkg/rulemanager/cel/libraries/networkneighborhood/network.go
- pkg/objectcache/containerprofilecache/projection_trie.go
- pkg/rulemanager/cel/libraries/applicationprofile/ap.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/main.go
- pkg/rulemanager/cel/libraries/applicationprofile/capability.go
- pkg/config/config.go
- pkg/rulemanager/cel/libraries/applicationprofile/open_test.go
- pkg/rulemanager/cel/cel.go
- pkg/rulemanager/cel/libraries/applicationprofile/exec_test.go
- pkg/rulemanager/cel/libraries/networkneighborhood/network_test.go
- pkg/rulemanager/rulepolicy.go
- pkg/objectcache/containerprofilecache_interface.go
- pkg/rulemanager/cel/libraries/networkneighborhood/integration_test.go
- pkg/metricsmanager/metrics_manager_noop.go
- pkg/objectcache/v1/mock.go
- pkg/metricsmanager/metrics_manager_interface.go
- pkg/metricsmanager/metrics_manager_mock.go
- pkg/rulemanager/cel/libraries/applicationprofile/http.go
- pkg/rulemanager/cel/libraries/networkneighborhood/nn.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
- profiledata.go: reset receiver in UnmarshalJSON/YAML for ProfileDataRequired and FieldRequirement; add PatternObject unknown-field rejection - function_cache.go: include SyncChecksum in cache key to invalidate on profile content changes (not only spec changes); iterate all extraKeyFn callbacks - rule_manager.go: gate strict-validation rejection behind StrictValidation flag; coalesce specNotify bursts before recompile - exec.go: document wasExecutedWithArgs v1 limitation for rule authors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
feat/rule-aware-profile-projection
branch
from
a19607e to
d3c04b9
Compare
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Add a CEL Helper Limitations table to the Detection Rules section noting that wasExecutedWithArgs currently performs path-only matching (equivalent to wasExecuted) and does not validate the argument list in v1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
…equired
When InUse=false (no rule declared a requirement for a field), projectField
was returning an empty ProjectedField{}, causing CEL helpers to see no profile
data and fire false-positive alerts for every exec/open/capability/etc.
Fix: treat InUse=false as All=true (pass-through), so existing rules that omit
profileDataRequired continue working with the full raw profile.
Update TestApply_NilSpec, TestApply_DynamicNotRetainedWhenNotInUse (renamed),
and TestSpecChange_TriggersReprojection to reflect the new pass-through semantics.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/rulemanager/cel/libraries/cache/function_cache.go (1)
115-117:⚠️ Potential issue | 🟡 MinorGuard
extraKeyFncallbacks against nil to prevent panic.
WithCachecalls every variadic callback directly; a nil callback will panic at runtime.💡 Proposed fix
- for _, fn := range extraKeyFn { - key += "|" + fn(values) + for _, keyFn := range extraKeyFn { + if keyFn == nil { + continue + } + key += "|" + keyFn(values) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/cel/libraries/cache/function_cache.go` around lines 115 - 117, The loop that concatenates extraKeyFn callbacks can panic if a nil callback is passed (the variadic callbacks from WithCache); update the loop in function_cache.go to skip nil functions before invoking them (e.g., inside the for _, fn := range extraKeyFn block check fn != nil and continue if nil) so key construction safely ignores nil callbacks while preserving existing behavior.pkg/rulemanager/rule_manager.go (1)
203-203:⚠️ Potential issue | 🟡 MinorAdd nil check before calling
SetProjectionSpec.
ContainerProfileCache()can returnnil(as seen in the nil check withinprofilehelper.GetProjectedContainerProfile). A direct method call without a guard will panic.🛡️ Suggested fix
- rm.objectCache.ContainerProfileCache().SetProjectionSpec(spec) + if cpc := rm.objectCache.ContainerProfileCache(); cpc != nil { + cpc.SetProjectionSpec(spec) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rulemanager/rule_manager.go` at line 203, The call rm.objectCache.ContainerProfileCache().SetProjectionSpec(spec) can panic if ContainerProfileCache() returns nil; before invoking SetProjectionSpec, retrieve the cache into a local variable (e.g., cpCache := rm.objectCache.ContainerProfileCache()) and add a nil guard (if cpCache != nil { cpCache.SetProjectionSpec(spec) }) to mirror the nil-check pattern used in profilehelper.GetProjectedContainerProfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/objectcache/containerprofilecache/projection_apply.go`:
- Around line 17-19: Update the doc comments in projection_apply.go (around the
top comment block and the similar comment at lines 97-99) to reflect current
pass-through semantics: state that a nil spec results in a zero-spec that allows
pass-through rather than dropping all data, and clarify that surfaces with
InUse=false still pass data through while dynamic patterns are still captured on
path surfaces; reference the Apply function/logic and the callStackTree mention
so the comment accurately describes the current behavior.
In `@pkg/objectcache/containerprofilecache/reconciler_test.go`:
- Around line 1234-1237: The test currently only asserts capability values and
can pass without reprojection; update the test around
c.GetProjectedContainerProfile(id) to capture the initial profile into the
variable before (already present) and then after triggering the spec update
capture the new profile into after and assert that before.SpecHash !=
after.SpecHash to prove reprojection occurred; apply the same SpecHash
transition assertion to the other occurrence around lines 1248-1252 so both
cases verify a real reprojection rather than just capability presence.
---
Duplicate comments:
In `@pkg/rulemanager/cel/libraries/cache/function_cache.go`:
- Around line 115-117: The loop that concatenates extraKeyFn callbacks can panic
if a nil callback is passed (the variadic callbacks from WithCache); update the
loop in function_cache.go to skip nil functions before invoking them (e.g.,
inside the for _, fn := range extraKeyFn block check fn != nil and continue if
nil) so key construction safely ignores nil callbacks while preserving existing
behavior.
In `@pkg/rulemanager/rule_manager.go`:
- Line 203: The call
rm.objectCache.ContainerProfileCache().SetProjectionSpec(spec) can panic if
ContainerProfileCache() returns nil; before invoking SetProjectionSpec, retrieve
the cache into a local variable (e.g., cpCache :=
rm.objectCache.ContainerProfileCache()) and add a nil guard (if cpCache != nil {
cpCache.SetProjectionSpec(spec) }) to mirror the nil-check pattern used in
profilehelper.GetProjectedContainerProfile.
🪄 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: d079be5c-2dee-4f0c-80bb-243eab4e2122
📒 Files selected for processing (8)
README.mdpkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/rulemanager/cel/libraries/applicationprofile/exec.gopkg/rulemanager/cel/libraries/cache/function_cache.gopkg/rulemanager/rule_manager.gopkg/rulemanager/types/v1/profiledata.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/rulemanager/cel/libraries/applicationprofile/exec.go
- pkg/rulemanager/types/v1/profiledata.go
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
- projection_apply.go: update Apply doc-comment and dynamic-patterns comment to reflect pass-through semantics (InUse=false retains all data) - reconciler_test.go: add SpecHash assertions to TestSpecChange to prove reprojection actually occurred rather than testing pass-through twice Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
…ments Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
…s for rule-aware projection Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
… rule-aware projection Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
… tests; update malicious job to include command and args Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
…nd for service account token access Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
…elf/environ Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. Event Counters
|
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup Effectiveness (AFTER only)
Event Counters
|
slashben
approved these changes
May 3, 2026
Comment on lines
+74
to
+95
| cpProjectionMissingDeclCounter *prometheus.CounterVec | ||
| cpProjectionUndeclaredLiteralCounter *prometheus.CounterVec | ||
| cpProjectionStaleEntriesGauge prometheus.Gauge | ||
| cpProjectionUndeclaredRulesGauge prometheus.Gauge | ||
|
|
||
| // Profile projection metrics — detailed (gated by caller checking detailedMetricsEnabled) | ||
| cpProjectionSpecCompileCounter prometheus.Counter | ||
| cpProjectionSpecHashChangeCounter prometheus.Counter | ||
| cpProjectionSpecPatternsGauge *prometheus.GaugeVec | ||
| cpProjectionSpecAllFieldsGauge *prometheus.GaugeVec | ||
| cpProjectionApplyDurationHistogram prometheus.Histogram | ||
| cpProjectionReconcileTriggeredCounter *prometheus.CounterVec | ||
| cpHelperCallCounter *prometheus.CounterVec | ||
| cpProjectionUndeclaredRulesListGauge *prometheus.GaugeVec | ||
|
|
||
| // Memory-savings metrics — detailed | ||
| cpProfileRawSizeHistogram prometheus.Histogram | ||
| cpProfileProjectedSizeHistogram prometheus.Histogram | ||
| cpProfileEntriesRawHistogram *prometheus.HistogramVec | ||
| cpProfileEntriesRetainedHistogram *prometheus.HistogramVec | ||
| cpProfileRetentionRatioHistogram *prometheus.HistogramVec | ||
|
|
Contributor
There was a problem hiding this comment.
@matthyx aren't this "too many"? These metrics are not for free (memory perspective and we should take a second look if all of these worth the trade off
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
2 participants
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.
Summary by CodeRabbit
New Features
Behavior Changes
Chores
Tests