feat(processor): merge ug- AP/NN into consolidated ContainerProfile#319
feat(processor): merge ug- AP/NN into consolidated ContainerProfile#319yugal07 wants to merge 2 commits into
Conversation
Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds server-side consolidation of user-managed ApplicationProfile and NetworkNeighborhood objects into ContainerProfiles during processing. The processor now fetches matching user-managed objects from SQLite storage, merges per-container entries using annotation-based idempotency markers, and persists the consolidated result. ChangesUser-Managed Profile Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
|
@matthyx. Let me know what all things needs to change. 😄 |
I'll try to review it tomorrow, it's a lot of mental load |
No issues. Thanks |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/registry/file/containerprofile_user_managed.go (2)
211-212: 💤 Low value
append(base, user...)aliasesbase's backing array whencap(base) > len(base).The subsequent
out := combined[:0]in-place filter is algorithmically correct (sincelen(out) <= iholds at every step, socombined[i]is always read before being overwritten). However, the shared backing array withbasemakes the invariant non-obvious and fragile for future maintainers. An explicit allocation costs O(n) but makes the intent clear:♻️ Proposed refactor
-combined := append(base, user...) +combined := make([]metav1.LabelSelectorRequirement, 0, len(base)+len(user)) +combined = append(combined, base...) +combined = append(combined, user...)🤖 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_user_managed.go` around lines 211 - 212, appendDedupSortedMatchExpressions currently does combined := append(base, user...) which can alias base's backing array; change to allocate a new slice of size len(base)+len(user), copy base then user into it so combined has its own backing array before the in-place filter (out := combined[:0]) — this ensures the subsequent overwrite loop cannot accidentally mutate the original base slice.
128-131: 💤 Low valueMissing observability when
connis absent from context.The type-assertion miss on lines 121–126 logs a
WarningviauserManagedConnWarnOnce, but theconn-missing path (lines 128–131) returnsok=falsesilently. IfWithConnectionis somehow not called upstream or the context key changes, the ug- merge is silently disabled with no production signal.🔍 Proposed addition
conn, ok := ctx.Value(connKey).(*sqlite.Conn) if !ok { + userManagedConnWarnOnce.Do(func() { + logger.L().Warning("ContainerProfileProcessor.mergeUserManagedProfiles disabled - sqlite connection not found in context") + }) return nil, nil, false }🤖 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_user_managed.go` around lines 128 - 131, The type-assertion failure branch that currently returns (nil, nil, false) when conn := ctx.Value(connKey).(*sqlite.Conn) fails should emit the same observable warning as the earlier path; update the missing-conn path in containerprofile_user_managed.go to call the existing userManagedConnWarnOnce warning (or the same logger used there) before returning so absence of WithConnection/connKey is logged in production; reference connKey, ctx.Value, sqlite.Conn and userManagedConnWarnOnce to locate and patch the branch.
🤖 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/containerprofile_user_managed_test.go`:
- Around line 262-289: The ContainerProfileProcessor in the test harness is
created with an empty HostType which relies on implicit equivalence to
HostTypeKubernetes; update newE2EHarness (and similarly
TestConsolidateUserManagedFanOut) to set HostType: armotypes.HostTypeKubernetes
when constructing the ContainerProfileProcessor so the test harness mirrors
production/NewContainerProfileProcessor behavior and avoids relying on
ParseContainerProfileKey/BuildContainerProfileKey implicit handling of empty
HostType.
---
Nitpick comments:
In `@pkg/registry/file/containerprofile_user_managed.go`:
- Around line 211-212: appendDedupSortedMatchExpressions currently does combined
:= append(base, user...) which can alias base's backing array; change to
allocate a new slice of size len(base)+len(user), copy base then user into it so
combined has its own backing array before the in-place filter (out :=
combined[:0]) — this ensures the subsequent overwrite loop cannot accidentally
mutate the original base slice.
- Around line 128-131: The type-assertion failure branch that currently returns
(nil, nil, false) when conn := ctx.Value(connKey).(*sqlite.Conn) fails should
emit the same observable warning as the earlier path; update the missing-conn
path in containerprofile_user_managed.go to call the existing
userManagedConnWarnOnce warning (or the same logger used there) before returning
so absence of WithConnection/connKey is logged in production; reference connKey,
ctx.Value, sqlite.Conn and userManagedConnWarnOnce to locate and patch the
branch.
🪄 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: ea6679fe-673c-4568-879e-43738d630294
📒 Files selected for processing (3)
pkg/registry/file/containerprofile_processor.gopkg/registry/file/containerprofile_user_managed.gopkg/registry/file/containerprofile_user_managed_test.go
Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
2 participants
Server-side aggregation of user-managed (ug-prefix) ApplicationProfile and
NetworkNeighborhood into the consolidated ContainerProfile during
consolidateKeyTimeSeries. Lets node-agent treat the CP as a single source
of truth and drop its per-tick ug- fetch.
internal softwarecomposition types (no interface or storage method
annotation markers; merge skipped when ug- ResourceVersion is unchanged.
ConsolidateTimeSeries cadence; no AP/NN watcher added (per @matthyx
guidance, memory cost of caching ug- objects rules out a watch path).
SyncChecksum stable across re-merges of unchanged content.
wins on collision (utils.MergeMaps preserves base, so a local
overrideMerge is used here instead).
Refs #315, kubescape/node-agent#788
Known limitations (to discuss in PR review):
removing entries from a ug- CRD does not retract them from already-
merged consolidated CPs. The proper fix is a CRD shape change
(separate user-managed sub-spec, replaced wholesale)
arrives, so ug- edits to Completed workloads do not propagate until
the next tick that has new data. Accepted trade-off for option (c).
worth a note: Changes were initially drafted using help from copilot, and manually reviewed and edited
Summary by CodeRabbit
Release Notes
New Features
Tests