chore: deprecate old profiletypes in favor of containerprofiles - #864
chore: deprecate old profiletypes in favor of containerprofiles#864entlein wants to merge 32 commits into
Conversation
… -> ContainerProfile ConvertUserProfilesToContainerProfile produces the single user-defined ContainerProfile equivalent to a legacy user-authored ApplicationProfile + NetworkNeighborhood pair, reusing the existing projectUserProfiles merge onto an empty base. The differential oracle pins the migration contract at the enforcement level: for representative user-defined shapes (opens/exec argv wildcards, HTTP endpoints, egress/ingress + LabelSelector) the ProjectedContainerProfile from the legacy AP+NN overlay path equals the one from using the converted ContainerProfile as the base with no overlay. Behaviour-preserving by construction. Signed-off-by: entlein <einentlein@gmail.com>
…e authoritative base When the user-defined-profile pod label names a ContainerProfile carrying managed-by: User, the cache now uses it directly as the container's base profile (the migrated "new way"), instead of overlaying a legacy ApplicationProfile + NetworkNeighborhood pair. It falls back to the legacy AP+NN pair when no such CP exists, which still fires the existing deprecation signal. - add-time (tryPopulateEntry) and refresh (reconciler) both prefer the user CP, gated on the managed-by: User annotation so a learned CP at the same name is never mistaken for a user-defined one - UserCPRef/UserCPRV bookkeeping mirrors the legacy UserAPRV/UserNNRV RV tracking so the reconciler re-fetches and rebuilds only when the user CP changes Signed-off-by: entlein <einentlein@gmail.com>
Test_28 now creates one ContainerProfile (managed-by: User) carrying the merged exec/syscall + egress/selector surfaces, instead of a separate user-authored ApplicationProfile + NetworkNeighborhood pair — exercising the migrated read-path end to end. Assertions unchanged. Signed-off-by: entlein <einentlein@gmail.com>
…verlay The new path (converted CP as base, no overlay) is ~35% faster and allocates ~27% less than the legacy AP+NN overlay per projection, since it skips the two-object merge. Signed-off-by: entlein <einentlein@gmail.com>
Test_27 (opens R0002, both the regex and the curl-wildcard profile sites), Test_32 (R0040 argv wildcards), and Test_33 (opens wildcard anchoring — the previously-fixed one) now each create a single user-defined ContainerProfile (managed-by: User) carrying the merged exec/open/syscall (+ egress/selector for 32) surfaces, replacing the legacy ApplicationProfile + NetworkNeighborhood pair. Assertions unchanged. Test_28 was ported in an earlier commit. Signed-off-by: entlein <einentlein@gmail.com>
containsDynamicSegment recognised only the one-segment DynamicIdentifier
('⋯'), so a path-surface opens entry bearing the zero-or-more
WildcardIdentifier ('*') — e.g. '/etc/ssl/*' — was routed to Values as if it
were a literal. was_path_opened tolerates this (Values and Patterns are both
matched via CompareDynamic), but it is wrong for any consumer that treats
Values as exact membership, and it drops '*'-only entries a rule needs when
spec.All is false and no prefix/suffix matcher retains them. Recognise both
wildcard markers. Regression test pins '/etc/ssl/*' -> Patterns.
Pre-existing in upstream (identical containsDynamicSegment).
Signed-off-by: Entlein <eineintlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Test_33's anchoring subtests assert R0002 alerts, but the rule's file-access monitoring is opt-in (monitored prefixes incl. /etc/). Test_33 never applied r0002-files-access-enabled.yaml (Test_27 does via enableR0002ForTest), so R0002 never evaluated the opens and every 'expect alert' case silently passed as a no-alert — invisible because Test_33 had never run in CI. Enable it like Test_27. Verified on a live cluster: /etc/ssl/* correctly alerts on the bare parent /etc/ssl and stays silent on the child /etc/ssl/openssl.cnf. Signed-off-by: entlein <einentlein@gmail.com>
…tations A user-authored profile is authoritative and complete by definition — it should not carry the learning-lifecycle status/completion markers, nor a managed-by annotation. The pod's user-defined-profile label is what declares it user-authored (a signature, added by the signing tooling, is the integrity marker). - read-path/reconciler no longer gate on managed-by: the label-referenced CP is used as authoritative, and the entry State is forced to Completed+Full so the rule engine enforces it despite the absent completion annotation - the converter emits clean CPs (name + namespace + spec only) - fake client models an absent overlay-name fetch (drives the legacy fallback) Signed-off-by: entlein <einentlein@gmail.com>
…mple - Test_28 now loads its user-defined ContainerProfile from a real yaml (resources/containerprofile-user-defined-network.yaml) via a loader helper, so the fixture doubles as the copy-pasteable "how to author a user-defined profile" example. - The example — and all the test CPs — carry only name + spec: the nonsensical learning-lifecycle annotations (status/completion) and the managed-by marker are dropped (node-agent now treats a label-referenced CP as authoritative and forces the enforce-state itself). Signed-off-by: entlein <einentlein@gmail.com>
Migrate the network-wildcards NetworkNeighborhood fixtures into their user-authored ContainerProfile form so users can copy-paste them to author user-defined-profile allow-lists: - per-container spec.egress/ingress (NN's spec.containers[] collapses, one CP document per container; fixture 20 splits into two) - no lifecycle annotations — name only (namespace injected by tooling), matching the clean user-managed CP contract - teaching comments preserved verbatim from the NN fixtures Adds 00-fusioncore-homoglyph-attack.yaml: a pinned single-vendor allow-list and the look-alike (homoglyph) domains its exact-match dnsNames compare rejects (each fires R0005). All 21 documents strict-parse against v1beta1 ContainerProfile and carry zero annotations (verified). Signed-off-by: entlein <einentlein@gmail.com>
The component tests build their user-defined profiles inline as ContainerProfiles (Test_27/32/33) or load a CP yaml (Test_28); the legacy ApplicationProfile / NetworkNeighborhood fixtures they were derived from are no longer referenced by any Go test. Migrate the two CT-relevant ones to their user-authored CP form and delete the orphan: - exec-arg-wildcards-profile.yaml (AP curl-32-overlay) -> containerprofile-exec-arg-wildcards.yaml. CP form mirrors Test_32's inline CP exactly (same execs incl. busybox-symlink + literal-* entries, same syscalls, matchLabels app: curl-32). Demonstrates the exec-arg wildcard grammar for authoring. - known-network-neighborhood.yaml (NN fusioncore-network) -> containerprofile-fusioncore-network.yaml. Clean user-managed CP: name only, no managed-by / status / completion annotations. - user-profile.yaml: deleted. Zero references anywhere; its nginx/server exec surface matches no current test or deployment. Both new documents strict-parse against v1beta1 ContainerProfile and carry zero annotations (verified). CT compiles unchanged (go vet -tags component); no Go test referenced the deleted files. Signed-off-by: entlein <einentlein@gmail.com>
|
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:
📝 WalkthroughWalkthroughThe PR migrates profile handling from legacy ApplicationProfile and NetworkNeighborhood overlays to authored ContainerProfiles. It updates cache refresh, CEL namespaces, storage and watcher APIs, metrics, component tests, and fixtures. ChangesContainerProfile cache and refresh
CEL and integration migration
Platform and test migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/objectcache/containerprofilecache/containerprofilecache.go (1)
380-458: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winNon-terminal consolidated
cppermanently blocks an already-available authoritative user-defined CP.The terminal-status gate on the consolidated
cp(Lines 380-391) runs beforeuserDefinedCPis fetched (Lines 402-444) and before it replacescp(Lines 455-457). If a pod already has a non-terminal (still-learning) consolidated CP at the moment theuser-defined-profilelabel is added — the exact scenario this PR's migration enables for already-running workloads —tryPopulateEntryreturnsfalseforever, even though a ready, authoritativeuserDefinedCPexists. The pending-retry loop re-runs this same function on every tick, so it hits the same gate every time and never gets past it to checkuserDefinedCP.Fetch the overlay first and skip the gate once it resolves an authoritative profile:
🐛 Proposed fix
- // Only cache profiles whose status is terminal (Completed or TooLarge). - // Learning/ready profiles are still being written; caching them would let - // rules fire against incomplete data. TooLarge is terminal: the manager - // stopped collecting but the truncated data is still valid for detection. - // Return false so the synthetic-CP fallback below does not bypass the gate. - if cp != nil && !isTerminalCPStatus(cp.Annotations[helpersv1.StatusMetadataKey]) { - logger.L().Debug("tryPopulateEntry: CP status not terminal; keeping pending", - helpers.String("containerID", containerID), - helpers.String("namespace", ns), - helpers.String("status", cp.Annotations[helpersv1.StatusMetadataKey])) - return false - } - // Fetch user-authored legacy CRDs when the pod carries the - // UserDefinedProfileMetadataKey label. ... + // UserDefinedProfileMetadataKey label. Fetched before the terminal-status + // gate below so a non-terminal (still-learning) consolidated CP never + // blocks an already-available authoritative user-defined CP. var userAP *v1beta1.ApplicationProfile var userNN *v1beta1.NetworkNeighborhood var userDefinedCP *v1beta1.ContainerProfile overlayName, hasOverlay := container.K8s.PodLabels[helpersv1.UserDefinedProfileMetadataKey] if hasOverlay && overlayName != "" { ... (unchanged fetch logic) } + + // Only cache profiles whose status is terminal (Completed or TooLarge), + // unless an authoritative user-defined CP is already available. + if cp != nil && userDefinedCP == nil && !isTerminalCPStatus(cp.Annotations[helpersv1.StatusMetadataKey]) { + logger.L().Debug("tryPopulateEntry: CP status not terminal; keeping pending", + helpers.String("containerID", containerID), + helpers.String("namespace", ns), + helpers.String("status", cp.Annotations[helpersv1.StatusMetadataKey])) + return false + }The same ordering exists in
refreshOneEntry(reconciler.go Lines 301-332 run the terminal-check before the userDefinedCP re-fetch at Lines 406-427); worth the same fix there once addressed here.🤖 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/objectcache/containerprofilecache/containerprofilecache.go` around lines 380 - 458, Reorder the user-defined profile lookup in tryPopulateEntry so userDefinedCP is fetched before the consolidated cp terminal-status gate. Allow processing to continue when an authoritative userDefinedCP is available, while retaining the existing gate for non-terminal cp without one. Apply the same ordering and bypass behavior in refreshOneEntry, preserving legacy fallback handling.tests/component_test.go (1)
2250-2261: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove unneeded metadata labels from the user-defined container profile.
ApiGroupMetadataKey,ApiVersionMetadataKey, andRelated*metadata labels are not consulted when selecting/projecting a user-definedContainerProfile; the cache useskubescape.io/user-defined-profile. Strip these labels on the inline CP so the fixture matches the documented “name + spec” contract.🤖 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 `@tests/component_test.go` around lines 2250 - 2261, Update the inline ContainerProfile fixture in the component test to remove ApiGroupMetadataKey, ApiVersionMetadataKey, and all Related* metadata labels, leaving only the metadata required by the documented name-and-spec contract and the cache’s kubescape.io/user-defined-profile label if applicable.
🧹 Nitpick comments (5)
pkg/objectcache/containerprofilecache/containerprofilecache_test.go (1)
221-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing regression coverage for a failed initial CP fetch.
This test only covers the happy path (successful
userCPfetch). Consider adding a case where the initialGetContainerProfilecall for the overlay name fails (e.g.cpErron that name) and a lateraddContainer/refresh succeeds — this is exactly the scenario whereUserCPRefbookkeeping needs to survive a failed first attempt (see the critical issue raised incontainerprofilecache.goLines 402-546).🤖 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/objectcache/containerprofilecache/containerprofilecache_test.go` around lines 221 - 256, Add regression coverage alongside TestOverlayPath_UserDefinedCP_NewWay for an initial user-defined ContainerProfile fetch failure: configure the fake client to return cpErr for the overlay name, call addContainer, then make the later fetch succeed and trigger addContainer or refresh. Assert the eventual projection succeeds and UserCPRef/UserCPRV are preserved, verifying the bookkeeping survives the failed first attempt.tests/resources/containerprofile-user-defined-network.yaml (1)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant deprecated singular fields (
dns,ipAddress) alongside their plural equivalents.This egress entry sets
dnsanddnsNamesto the same value, and uses singularipAddress— all legacy/deprecated forms per this PR's own08-deprecated-ipaddress.yamlfixture. As an authoring example for "the new way," consider keeping only the canonical plural fields to avoid ambiguity for authors copying this example.📝 Suggested fix
egress: - identifier: fusioncore-egress type: external - dns: fusioncore.ai. dnsNames: - fusioncore.ai. - ipAddress: 162.0.217.171 + ipAddresses: + - 162.0.217.171 ports:🤖 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 `@tests/resources/containerprofile-user-defined-network.yaml` around lines 58 - 68, Update the fusioncore-egress entry in the user-defined network fixture to remove the deprecated singular dns and ipAddress fields, retaining the canonical dnsNames and existing ports configuration.tests/component_test.go (1)
1653-1657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc comment: still describes
ApplicationProfile.The
deployWithProfilehelper now builds av1beta1.ContainerProfile(Lines 1661-1679), but the preceding doc comment still says "creates a user-defined ApplicationProfile." Update it to avoid misleading future readers.📝 Suggested fix
- // deployWithProfile creates a user-defined ApplicationProfile with the + // deployWithProfile creates a user-defined ContainerProfile with the // given Opens list, polls until it is retrievable from storage, then // deploys nginx with the kubescape.io/user-defined-profile label // pointing at it, and waits for the pod to be ready.🤖 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 `@tests/component_test.go` around lines 1653 - 1657, Update the doc comment for the deployWithProfile helper to describe creation of a user-defined v1beta1.ContainerProfile instead of an ApplicationProfile, while preserving the existing description of its deployment and readiness behavior.tests/resources/containerprofile-fusioncore-network.yaml (1)
25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUses deprecated singular
ipAddressin a "new way" authoring example.Elsewhere in this PR (
network-wildcards-cp/08-deprecated-ipaddress.yaml, README) the singularipAddressfield is documented as deprecated in favor of pluralipAddresses. Since this fixture's own comment frames it as the "new way" ContainerProfile authoring example, consider using the canonicalipAddresses: ["162.0.217.171"]form for consistency.📝 Suggested fix
dnsNames: - fusioncore.ai. - ipAddress: 162.0.217.171 + ipAddresses: + - 162.0.217.171 ports:🤖 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 `@tests/resources/containerprofile-fusioncore-network.yaml` around lines 25 - 34, Update the egress entry in the ContainerProfile example to replace the deprecated singular ipAddress field with the canonical ipAddresses field containing 162.0.217.171, while preserving the existing network rule and other fields.tests/resources/network-wildcards-cp/07-mixed-ip-list.yaml (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the fixture description with the actual matching path.
Because
*makes every valid IP match, this fixture cannot exercise list ordering or first-match behavior. Either remove those claims or use a mixed literal/CIDR case without*; keep this fixture focused on accepting mixed forms with the sentinel.🤖 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 `@tests/resources/network-wildcards-cp/07-mixed-ip-list.yaml` around lines 8 - 13, Update the comments in the mixed IP list fixture to remove claims that it tests list ordering, loop ordering, or first-match short-circuiting, since the sentinel `*` matches every valid IP. Keep the description focused on accepting mixed literal/CIDR forms with the unconstrained sentinel.
🤖 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/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 402-444: Record UserCPRef whenever a non-empty user-defined
profile label is present, even when the initial GetContainerProfile call fails,
so refreshOneEntry can retry it later. In rebuildEntryFromSources, clear
UserAPRef and UserNNRef when userDefinedCP becomes authoritative, preserving the
mutually exclusive reference invariant while retaining legacy refs only when no
user CP is available.
In `@tests/resources/network-wildcards-cp/13-dns-trailing-dot-normalisation.yaml`:
- Around line 5-10: Update the YAML fixture’s profile entries to match its
documented cases: replace the api.github.com entry with api.stripe.com if
testing both trailing-dot forms, and ensure the expectations in both referenced
sections consistently describe the actual profile-side hostname.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 380-458: Reorder the user-defined profile lookup in
tryPopulateEntry so userDefinedCP is fetched before the consolidated cp
terminal-status gate. Allow processing to continue when an authoritative
userDefinedCP is available, while retaining the existing gate for non-terminal
cp without one. Apply the same ordering and bypass behavior in refreshOneEntry,
preserving legacy fallback handling.
In `@tests/component_test.go`:
- Around line 2250-2261: Update the inline ContainerProfile fixture in the
component test to remove ApiGroupMetadataKey, ApiVersionMetadataKey, and all
Related* metadata labels, leaving only the metadata required by the documented
name-and-spec contract and the cache’s kubescape.io/user-defined-profile label
if applicable.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache_test.go`:
- Around line 221-256: Add regression coverage alongside
TestOverlayPath_UserDefinedCP_NewWay for an initial user-defined
ContainerProfile fetch failure: configure the fake client to return cpErr for
the overlay name, call addContainer, then make the later fetch succeed and
trigger addContainer or refresh. Assert the eventual projection succeeds and
UserCPRef/UserCPRV are preserved, verifying the bookkeeping survives the failed
first attempt.
In `@tests/component_test.go`:
- Around line 1653-1657: Update the doc comment for the deployWithProfile helper
to describe creation of a user-defined v1beta1.ContainerProfile instead of an
ApplicationProfile, while preserving the existing description of its deployment
and readiness behavior.
In `@tests/resources/containerprofile-fusioncore-network.yaml`:
- Around line 25-34: Update the egress entry in the ContainerProfile example to
replace the deprecated singular ipAddress field with the canonical ipAddresses
field containing 162.0.217.171, while preserving the existing network rule and
other fields.
In `@tests/resources/containerprofile-user-defined-network.yaml`:
- Around line 58-68: Update the fusioncore-egress entry in the user-defined
network fixture to remove the deprecated singular dns and ipAddress fields,
retaining the canonical dnsNames and existing ports configuration.
In `@tests/resources/network-wildcards-cp/07-mixed-ip-list.yaml`:
- Around line 8-13: Update the comments in the mixed IP list fixture to remove
claims that it tests list ordering, loop ordering, or first-match
short-circuiting, since the sentinel `*` matches every valid IP. Keep the
description focused on accepting mixed literal/CIDR forms with the unconstrained
sentinel.
🪄 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 Plus
Run ID: 4b7c84a9-bef1-4de2-8db4-3ffe8bc86801
📒 Files selected for processing (36)
pkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_wildcard_classification_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gotests/component_test.gotests/resources/containerprofile-exec-arg-wildcards.yamltests/resources/containerprofile-fusioncore-network.yamltests/resources/containerprofile-user-defined-network.yamltests/resources/exec-arg-wildcards-profile.yamltests/resources/known-network-neighborhood.yamltests/resources/network-wildcards-cp/00-fusioncore-homoglyph-attack.yamltests/resources/network-wildcards-cp/01-literal-ipv4.yamltests/resources/network-wildcards-cp/02-literal-ipv6.yamltests/resources/network-wildcards-cp/03-cidr-ipv4.yamltests/resources/network-wildcards-cp/04-cidr-ipv6.yamltests/resources/network-wildcards-cp/05-any-ip-sentinel.yamltests/resources/network-wildcards-cp/06-any-as-cidr.yamltests/resources/network-wildcards-cp/07-mixed-ip-list.yamltests/resources/network-wildcards-cp/08-deprecated-ipaddress.yamltests/resources/network-wildcards-cp/09-dns-literal.yamltests/resources/network-wildcards-cp/10-dns-leading-wildcard.yamltests/resources/network-wildcards-cp/11-dns-mid-ellipsis.yamltests/resources/network-wildcards-cp/12-dns-trailing-star.yamltests/resources/network-wildcards-cp/13-dns-trailing-dot-normalisation.yamltests/resources/network-wildcards-cp/14-recursive-star-rejected.yamltests/resources/network-wildcards-cp/15-egress-and-ingress.yamltests/resources/network-wildcards-cp/16-egress-none.yamltests/resources/network-wildcards-cp/17-realistic-stripe-api.yamltests/resources/network-wildcards-cp/18-cluster-dns-via-mid-ellipsis.yamltests/resources/network-wildcards-cp/19-port-protocol-with-cidr.yamltests/resources/network-wildcards-cp/20-multi-container-mixed-wildcards.yamltests/resources/network-wildcards-cp/README.mdtests/resources/network-wildcards/README.mdtests/resources/user-profile.yaml
💤 Files with no reviewable changes (4)
- tests/resources/user-profile.yaml
- tests/resources/exec-arg-wildcards-profile.yaml
- tests/resources/network-wildcards/README.md
- tests/resources/known-network-neighborhood.yaml
… decommission them -step 1 of many -- I understand that this has a long tail of decommissions Signed-off-by: entlein <einentlein@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/objectcache/containerprofilecache/reconciler.go (2)
380-388: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
NotFoundas deletion rather than a transient error.Once
UserCPRVis set, every fetch error preserves the cached authoritative policy. Deleting the referenced CP therefore leaves its old policy enforced indefinitely instead of rebuilding from the learned CP. Return early only for non-NotFoundfailures; letNotFoundsetuserDefinedCPto nil so the RV mismatch triggers a rebuild.Proposed fix
- if userCPErr != nil && e.UserCPRV != "" { + if userCPErr != nil && e.UserCPRV != "" && !apierrors.IsNotFound(userCPErr) { logger.L().Debug("refreshOneEntry: user-defined CP fetch failed; keeping cached entry", helpers.String("containerID", id), helpers.String("name", e.UserCPRef.Name), helpers.Error(userCPErr)) return }🤖 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/objectcache/containerprofilecache/reconciler.go` around lines 380 - 388, Update the user-defined CP error handling in refreshOneEntry so cached policy is preserved only for non-NotFound fetch failures. Detect the NotFound condition before the early return, allow it to continue with userDefinedCP set to nil, and retain the existing return behavior for other errors so the RV mismatch triggers rebuilding.
369-379: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate that the referenced CP is user-managed before adopting it.
The migration contract identifies this source as
managed-by: User, but this path accepts any same-name CP. A learned CP can bypass the terminal-status gate above and then be forced toCompleted/Fullin the rebuilt entry. Reject or ignore profiles withoutManagedByMetadataKey == ManagedByUserValue, with coverage for an invalid reference.🤖 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/objectcache/containerprofilecache/reconciler.go` around lines 369 - 379, Validate the ContainerProfile fetched in the UserCPRef refresh within the reconciler before adopting it. Only accept profiles whose ManagedByMetadataKey equals ManagedByUserValue; ignore or reject invalid references so learned profiles cannot bypass terminal-status handling. Add coverage for a referenced profile with invalid management metadata, while preserving transient fetch-error behavior.
🧹 Nitpick comments (1)
pkg/objectcache/containerprofilecache/containerprofilecache_test.go (1)
223-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the user CP is the effective policy.
These assertions only prove it was fetched and recorded. A regression could retain the learned/synthetic CP while copying
UserCPRef/RV and still pass. Assert a user-CP-specific projected value, or at minimumentry.State.Name == "override".🤖 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/objectcache/containerprofilecache/containerprofilecache_test.go` around lines 223 - 226, Strengthen the assertions in the user-defined CP test to verify the user CP is the effective policy, not merely recorded for refresh. After the existing UserCPRef checks, assert the projected profile contains a user-CP-specific value or, at minimum, that entry.State.Name equals "override"; keep the existing fetch and reference assertions unchanged.
🤖 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 `@tests/resources/nnlint_test.go`:
- Around line 176-211: Update lintEndpoint to apply the existing dnsNameProblem
validation to the singular e.DNS value and validIPEntry validation to the
singular e.IPAddress value, in addition to the plural DNSNames and IPAddresses
loops. Emit the same R-NN-13 and R-NN-15 rules with contextual messages so
malformed deprecated fields are reported.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/reconciler.go`:
- Around line 380-388: Update the user-defined CP error handling in
refreshOneEntry so cached policy is preserved only for non-NotFound fetch
failures. Detect the NotFound condition before the early return, allow it to
continue with userDefinedCP set to nil, and retain the existing return behavior
for other errors so the RV mismatch triggers rebuilding.
- Around line 369-379: Validate the ContainerProfile fetched in the UserCPRef
refresh within the reconciler before adopting it. Only accept profiles whose
ManagedByMetadataKey equals ManagedByUserValue; ignore or reject invalid
references so learned profiles cannot bypass terminal-status handling. Add
coverage for a referenced profile with invalid management metadata, while
preserving transient fetch-error behavior.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/containerprofilecache_test.go`:
- Around line 223-226: Strengthen the assertions in the user-defined CP test to
verify the user CP is the effective policy, not merely recorded for refresh.
After the existing UserCPRef checks, assert the projected profile contains a
user-CP-specific value or, at minimum, that entry.State.Name equals "override";
keep the existing fetch and reference assertions unchanged.
🪄 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 Plus
Run ID: 394d9bb1-b653-454c-8c2e-9dd9ea0f41d9
📒 Files selected for processing (8)
pkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/export_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/t8_overlay_refresh_test.gotests/resources/network-wildcards-cp/13-dns-trailing-dot-normalisation.yamltests/resources/nnlint_test.go
💤 Files with no reviewable changes (3)
- pkg/objectcache/containerprofilecache/t8_overlay_refresh_test.go
- pkg/objectcache/containerprofilecache/export_test.go
- pkg/objectcache/containerprofilecache/reconciler_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/resources/network-wildcards-cp/13-dns-trailing-dot-normalisation.yaml
- pkg/objectcache/containerprofilecache/containerprofilecache.go
| // the container stays pending; UserCPRef — recorded unconditionally below — | ||
| // drives the reconciler to retry the CP on every tick until it materialises. | ||
| var userDefinedCP *v1beta1.ContainerProfile | ||
| overlayName, hasOverlay := container.K8s.PodLabels[helpersv1.UserDefinedProfileMetadataKey] |
There was a problem hiding this comment.
Every container in the pod resolves the same overlayName and adopts the same CP — the binding is a pod label, but a ContainerProfile is per-container. A sidecar inherits the app container's allow-list and vice versa.
The legacy path handled this: projectUserProfiles(..., container.Runtime.ContainerName) selected the matching spec.containers[] entry across containers / initContainers / ephemeralContainers. Nothing replaces it.
We need either a naming convention the read path derives (<label-value>-<containerName>, falling back to the bare name for single-container pods) or a container selector inside the CP spec.
Worth settling before the storage PR locks the shape in — tests/resources/network-wildcards-cp/README.md already documents "one CP document per container", and fixture 20 emits nw-20-multi-container-frontend + nw-20-multi-container-sidecar, but nothing can currently bind either one to a specific container.
| // test is only that iteration stopped early. | ||
| } | ||
|
|
||
| // TestRefreshFastSkipWhenAllRVsMatch — delta #4. When CP RV and both overlay |
There was a problem hiding this comment.
We should migrate these tests to CPs rather than drop them. This hunk and the ones below remove every refresh-path overlay test (−274 here, −114 in t8_overlay_refresh_test.go), and nothing replaces them for the user-CP path — TestOverlayPath_UserDefinedCP_NewWay only covers the add path. That's why the entry.RV bug is green in CI.
The four worth restoring in CP form:
- fast-skip when the CP RV and
UserCPRVboth match (TestRefreshFastSkipWhenAllRVsMatch) - rebuild when only
UserCPRVchanges (TestRefreshRebuildsOnUserAPChange) - end-to-end: edit the authored CP in the fake, one
refreshAllEntries, assert the projection reflects the edit (TestT8_EndToEndRefreshUpdatesProjection) — this is the one that catches the freeze - user-defined CP present with no learned CP (
TestUserDefinedProfileOnly_NoBaseCP)
countingProfileClient.GetContainerProfile (:78) also ignores its name argument, so it can't distinguish base from authored CP — it needs the same name-matching treatment fakeProfileClient just got. And countingMetrics.legacyLoad (:129) is now unused after these deletions; unused/staticcheck will flag it.
| } | ||
| if entry.UserNNRef == nil { | ||
| entry.UserNNRef = &namespacedName{Namespace: ns, Name: overlayName} | ||
| entry.UserCPRef = &namespacedName{Namespace: ns, Name: overlayName} |
There was a problem hiding this comment.
This forces Completed/Full without ever checking that the fetched CP is user-authored, or that it's finished. The isTerminalCPStatus gate at :375 — "Learning/ready profiles are still being written; caching them would let rules fire against incomplete data" — only guards the base CP fetched by the derived slug at :319. The label-referenced CP fetched at :391-407 gets no equivalent check, becomes the profile at :419-421, and has its real annotations overwritten here.
Tested on this branch: a CP carrying status: ready / completion: partial published at the label's name is cached as Status="completed" Completion="complete". Result is false positives — rules alert on everything legitimate that wasn't learned yet.
3f9299b's gate prevented exactly this; f2f730f removed it on the grounds that a signature is the real integrity marker, but "check signature logic" is still an open TODO, so nothing verifies anything today.
Suggest gating on the negative instead — a learned CP wears lifecycle annotations, an authored one doesn't. This belongs right after the fetch (~:407) or inside the if userDefinedCP != nil below, not at this line, where userDefinedCP may be nil:
if userDefinedCP != nil {
if _, learned := userDefinedCP.Annotations[helpersv1.StatusMetadataKey]; learned {
logger.L().Warning("user-defined-profile label resolves to a learned ContainerProfile; ignoring", ...)
userDefinedCP = nil
}
}That makes "authored profiles carry no annotations" self-enforcing without blocking on the signature work.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed locally against main at fd5ed04. The direction is right — one unified user-authored CRD instead of the AP+NN pair is a real simplification, and the commit messages are unusually good at stating the contract each change pins. Two things block, and a couple of design questions are worth settling before this leaves draft.
Blockers
Authored ContainerProfile edits are never picked up. entry.RV ends up holding the authored CP's ResourceVersion, while entry.CPName points at the learned slug — which never exists, because learning is suppressed for user-defined containers. The permanent 404 then reads as a transient error and the entry is frozen for the container's lifetime. I verified this and the two-hunk fix is inline (:419, :466); after it, a refresh test goes red→green and the rest of the package is unchanged. It also fixes the learned+authored case, where the authored RV currently shadows the learned one.
Multi-container pods lose per-container separation. The binding is a pod label but a ContainerProfile is per-container, so every container in the pod adopts the same profile. The old path selected the right spec.containers[] entry by container name; nothing replaces it. Worth noting the PR's own network-wildcards-cp/README.md documents "one CP document per container", which the read path can't currently express — that gap should be closed before the storage PR locks the shape in.
Worth settling
- Nothing validates that the label-referenced CP is user-authored, and its state is force-set to
Completed/Full. A profile withstatus: ready/completion: partialat that name gets enforced as complete — I tested it. 3f9299b's gate prevented exactly this; f2f730f removed it on the grounds that a signature is the real integrity marker, but "check signature logic" is still open on your TODO list, so right now nothing verifies anything. Suggestion inline. - The upgrade path fails silently. A workload still on user-defined AP+NN gets no profile and no warning — just a
Debugline and a container pending forever. AWarning+ metric would make it visible; keeping the AP/NN read behind a deprecation flag for one release would make it safe. Your call which, but "couldn't find any active consumers" is a bet I'd rather not take silently on a security control. - The refresh-path tests were removed where the regressions landed (−274 in
reconciler_test.go, −114 int8_overlay_refresh_test.go), with nothing covering the user-CP path —TestOverlayPath_UserDefinedCP_NewWayonly exercises the add path. That's precisely why the RV bug above is green in CI.countingProfileClient.GetContainerProfilealso ignores itsnameargument, so no reconciler test can currently distinguish base from authored CP. Related:countingMetrics.legacyLoadis now unused and will likely tripunused.
The * classification fix looks right
Checked 903443d against storage: analyze_opens.go:97 uses the same Wildcard || Dynamic test, so this aligns node-agent with the producer, and compareSegmentsIndex only treats a whole segment * as a wildcard — so a path with an embedded literal * still matches literally and no allow-list is broadened. It applies to Execs and Endpoints too, not just Opens; harmless for Endpoints (both Values and Patterns go through CompareDynamic), but the new test only covers Opens — an Execs case would pin it.
The Test_33 R0002 fix (8c82261) is a good catch: a test that had never run in CI and was silently passing every "expect alert" case.
Nits with no anchorable hunk
reconciler.go:224still says "UserAPRV + UserNNRV all match";rebuildEntryFromSources' doc comment (:436-437) still describes the "label-referenced user overlay AP+NN" ladder pass that no longer exists.component_test.go:2343still refers toUserAPRef. Test_28'sconst overlayName = "curl-28-overlay"is now dead, and the comment above it still describes the AP+NN dual lookup — same innginx-user-defined-deployment.yaml:17-18.tests/resources/network-wildcards/README.mdwas deleted, but the NN fixtures it documented are still consumed bypkg/rulemanager/cel/libraries/networkneighborhood/fixtures_test.go:227— those are now undocumented.- No metric or log when an authored CP is adopted; the legacy path emitted overlay metrics, so the new path is a blind spot.
Verified
go build ./pkg/objectcache/..., go test ./pkg/objectcache/... ./tests/resources/, and go vet -tags component ./tests/... all pass on the branch. The two behavioural claims above (frozen entry, forced-complete state) were each confirmed with a purpose-written test run against this branch, then discarded.
| // LEGACY mixed in | ||
| // Need SOMETHING to cache. If we have nothing, stay pending and retry. | ||
| if cp == nil && userManagedAP == nil && userManagedNN == nil && userAP == nil && userNN == nil { | ||
| if cp == nil && userDefinedCP == nil && userManagedAP == nil && userManagedNN == nil { |
There was a problem hiding this comment.
This is where a legacy user goes silently unprotected after upgrade: label set → learning suppressed → CP fetch 404s → no ug- profiles → we return false, the container stays pending forever with no profile, and the only trace is a Debug line.
Please emit a Warning plus a metric when the label is set but nothing resolved, so an upgrade that strands a legacy user is visible rather than silent.
(The ug- path itself isn't affected by this PR — still fetched at :335-368, still merged at :452-457. What's removed is the label-referenced AP/NN pair.)
| return false | ||
| } |
There was a problem hiding this comment.
This gate (from :375) runs against the learned CP and returns before the authored CP is fetched at :391. So if the label is added to a workload that already has a CP mid-learning, the authored profile is never adopted until that CP reaches a terminal status — even though it's meant to replace it outright. refreshOneEntry:326 has the same ordering.
Simplest fix: fetch the authored CP first and skip this gate entirely when one is present — the learned CP is discarded at :419-421 anyway.
| // LEGACY | ||
| userManagedApplied := userManagedAP != nil || userManagedNN != nil |
There was a problem hiding this comment.
Since cp may now be the authored profile (:419-421), this pass lets a legacy ug-<workload> AP/NN widen a profile the comment at :415-418 calls authoritative. reconciler.go:487-497 does the same.
The comments assert both things — :415-418 says the ug- pass "may still union on top", while :489-493 calls the authored CP "authoritative and complete by definition". Worth picking one deliberately, given ug- is part of the same legacy surface being decommissioned. My read: an authored profile shouldn't be silently widened by a legacy object the author may not know exists.
Minor, same hunk: // LEGACY here replaced the comment explaining the ug- naming convention and the Test_12/13 contract. Same at :332 and :409 (that one has trailing whitespace).
| // userCP, when non-nil, is returned by GetContainerProfile for a name | ||
| // matching userCP.Name (the migrated user-defined ContainerProfile). Other | ||
| // names fall through to cp. Lets tests exercise the new-way overlay path. | ||
| userCP *v1beta1.ContainerProfile |
There was a problem hiding this comment.
Inserting userCP (plus its comment) into this aligned field block breaks gofmt — the surrounding ap/nn/cpErr/apErr/nnErr lines need re-padding, and cp needs un-padding.
gofmt -l ./pkg/objectcache/containerprofilecache/ flags this file on the branch but not on main, so it's newly introduced here. gofmt -w on the file fixes it.
Two more in containerprofilecache.go, also new on this branch: a double blank line at :330-331, and trailing whitespace on // LEGACY mixed in at :409. (The struct-alignment warning gofmt reports at :111-113 in that file is pre-existing on main — not yours.)
No CI job runs gofmt, so none of this fails the build; it'll just show up in the next person's editor.
…storage etc Signed-off-by: entlein <einentlein@gmail.com>
…icontainer labels, tests not reviewed yet Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Co-authored-by: Matthias Bertschy <matthias.bertschy@gmail.com> Signed-off-by: Duck <70207455+entlein@users.noreply.github.com>
Component-test validation (live k3s)Ran the migration CT matrix on a live k3s cluster using the component-test chart pinned to freshly built node-agent + storage images, with CI-faithful config (short learning intervals, R0040 defined + enabled, R0002 self-enabled at runtime, alerts collected via Alertmanager).
Review feedback addressed:
|
…ace) The rulemanager CEL libraries already read ContainerProfile data but still carried AP/NN naming. Rename applicationprofile -> containerprofile and networkneighborhood -> containerprofilenetwork (dirs via git mv, packages, types, constructors, LibraryName), and unify the CEL function namespace: ap.* and nn.* -> cp.* (cp.was_executed, cp.is_domain_in_egress, ...). Update cel.go registration, the bundled default-rules.yaml, and the network-wildcards test rules to the cp.* namespace so rules compile against the renamed engine. Rule content mirrors the rulelibrary migrate/sbob rename. Signed-off-by: entlein <einentlein@gmail.com>
The CP-form fixtures lived in a parallel network-wildcards-cp/ dir alongside the now-dead network-wildcards/. 'cp' is an implementation detail, not part of what the fixtures are. Remove the dead dir, move the CP fixtures to network-wildcards/ (update the nnlint/fixtures test references), and drop the explanatory README. Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
rc5k validation — component tests, benchmark, chartComponent tests — fork overlay harness, node-agent + storage both built from Benchmark — run https://github.com/k8sstormcenter/node-agent/actions/runs/30485745861, quality gate passed. Swaps only
21 steady-state samples each. Storage runs chart-default upstream in both arms, so it is not the variable under test. Chart — |
|
I like your chicken |
rc5l released — GeneratedNetworkPolicy fix + backend/synchronizer migrationrc5l supersedes rc5k (rc5k's GeneratedNetworkPolicy was broken — see the correction above and storage Component tests (run https://github.com/k8sstormcenter/node-agent/actions/runs/30518037316): 25 pass / 3 fail — the 3 are the known infra flakes (Test_09 false-positive, Test_20/Test_21 partial-then-learn timing); all migration legs green; GeneratedNetworkPolicy fix — confirmed live on rc5l: a two-container Deployment yields exactly one workload-level backend + synchronizer migrated to ContainerProfile as well (AP/NN removed, storage pinned v0.0.290): backend's gRPC proto regenerated CP-only; synchronizer's filter + integration tests moved to ContainerProfile. Benchmark unchanged from rc5k (node-agent binary identical in rc5l): node-agent memory −20.9% vs upstream v0.3.158, CPU flat. |
Test-coverage gaps to close — ContainerProfile migration (node-agent)The cache half is well-covered: The risk is the rule-engine half — HIGH
MED
LOW
|
Resolve the Test_35 collision: upstream added Test_35_ExecTTYFieldTest at the same location as our Test_35_MultiContainerPerContainerBinding. Keep both; renumber ours to Test_36_MultiContainerPerContainerBinding.
Test_20 / Test_21 — why they time out, and how to rewrite themThese two are not the SQLite write-contention flake (that's Test_09, which shows ~100+
The shared problem: both hinge on a natural learn → restart → re-learn cycle reaching How to rewrite:
Net: the Test_20/21 timeouts are a test-robustness problem (no fail-fast, raced state transitions, fixed sleeps) — distinct from the storage single-writer contention that Test_09 exposes. Rewriting to deterministic authored-profile states + fail-fast waits fixes both and makes them robust to the contention as a side effect. |
rc5o — component tests + benchmarkRebuilt both node-agent + storage from Component tests — https://github.com/k8sstormcenter/node-agent/actions/runs/30550467856 — 26 pass / 2 fail.
Benchmark — https://github.com/k8sstormcenter/node-agent/actions/runs/30553652590 — quality gate passed. Swaps only
21 steady-state samples each. Storage runs chart-default upstream in both arms, so it is not the variable under test. |
rc5p — CT after the Test_16 completion-gate fixhttps://github.com/k8sstormcenter/node-agent/actions/runs/30560284636 — 27 pass / 1 fail.
The single remaining failure is |
|
Correction to the Test_09 characterization above: this rc5p run had 0 literal Effect: the checkoutservice ContainerProfile stayed |
Add cluster-free unit coverage for the migration's rule-side surface: - cp.* HTTP evaluators (http.go, was 0%/no file): endpoint/host match, method and prefix/suffix variants, empty-CP, nil-cache error. - isExecInPodSpec + the wasExecuted/wasExecutedWithArgs podspec-exempt fall-through. - containerprofilenetwork matchers: no-match / wildcard-domain / empty-neighborhood. - Declarations smoke+drift guard: compile every cp.* overload, assert the decl set. - profilehelper GetProjectedContainerProfile/GetPodSpec/GetContainerName. - HasFinalApplicationProfile enforce-vs-learn gate (Completed+Full -> enforce). containerprofile 37->69%, containerprofilenetwork 75->80%, profilehelper 95%, HasFinalApplicationProfile 100%. Test-only; no production changes. Signed-off-by: entlein <einentlein@gmail.com>
… racy learning) Test_20/21 hung to the 20-minute global panic in WaitForContainerProfileCompletion*: Test_20's blacklist wait can never succeed (the old completed profile stays matched and blacklisted after the deployment restart), and Test_21's multi-container profile never reached 'completed'. Both raced the natural learn -> daemonset-restart -> deployment-restart -> re-learn cycle. Rewrite to enforce an AUTHORED user-defined ContainerProfile, updated in place: an action not in the profile alerts; the same action, once added, does not. Determinism comes from a positive reload gate -- the update that adds the subject also removes a canary, so the canary starts alerting the moment node-agent reloads the revision (alert-appears signal), confirming the reload before the negative assertion. Single-container workloads; bounded pollers that dump ContainerProfile status on timeout (never a 20m panic); Eventually-style polling on the real alert condition instead of fixed sleeps. Also fixes a latent bug: the old 'no new alert' check filtered on a process_name label that alerts never carry, so it always matched zero and passed regardless; the rewrite keys on comm (process) and rule_id (network). Verified: both pass twice on a live rig (rc5l), 0 'database is locked'. This now tests profile ENFORCEMENT (authored partial->full), not natural learning. Signed-off-by: entlein <einentlein@gmail.com>
… sleep Test_16 slept a hard-coded 175s after the restart-inducing 'service nginx stop' before exec'ing the ls violation (the WaitForContainerProfileCompletion guard was commented out), then asserted the 'Unexpected process launched' alert. On a quiet system that had ~110s of slack; under full-CT concurrency the storage single-writer SQLite contention (database is locked) defers the merged profile's completion-status write past the fixed window, so ls ran against a non-enforcing profile and no alert fired -> the assertion failed (observed as Test_16 red in a full CT with 12 lock events). Replace the fixed sleep with a bounded poll (5m, fail-fast, dumps ContainerProfiles on timeout) for the MERGED ContainerProfile reaching 'completed' after the restart -- keyed on the merged profile only (name has no -<32 hex> suffix), not WaitForContainerProfileCompletion which requires ALL matching profiles completed and would hang on the transient per-instance profiles the restart spawns. Then exec ls and poll (Eventually) for the alert. Validated 3/3 on a live rig; runtime ~241s -> ~90s since it no longer over-sleeps. Signed-off-by: entlein <einentlein@gmail.com>
|
@CodeRabbit your turn, please review together with kubescape/storage#351 |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== node-agent PR `#864` ==='
gh pr view 864 --repo kubescape/node-agent --json url,title,state,body,commits,files,comments \
--jq '{url,title,state,body,files:[.files[]|{path,additions,deletions}],comments:[.comments[]|{author:.author.login,body,url}]}'
for pr in \
'kubescape/storage 351' \
'kubescape/backend 55' \
'kubescape/synchronizer 168'
do
set -- $pr
repo="$1"
number="$2"
echo "=== ${repo} PR #${number} ==="
gh pr view "$number" --repo "$repo" --json url,title,state,body,baseRefName,headRefName,commits,files \
--jq '{url,title,state,baseRefName,headRefName,body,commits:[.commits[]|{oid,messageHeadline}],files:[.files[]|{path,additions,deletions}]}'
done
echo '=== cited CI run ==='
gh run view 30534641643 --repo k8sstormcenter/node-agent --json url,status,conclusion,event,headSha,displayTitle,jobs \
--jq '{url,status,conclusion,event,headSha,displayTitle,jobs:[.jobs[]|{name,status,conclusion}]}'
echo '=== chart PR links mentioned on node-agent PR `#864` ==='
gh pr view 864 --repo kubescape/node-agent --json body,comments \
--jq -r '[.body, (.comments[]?.body // "")] | join("\n")' \
| grep -Eio 'https://github\.com/[^[:space:])>]+/(helm-charts|kubescape-operator|[^/]+)/pull/[0-9]+' \
| sort -u || trueLength of output: 61652 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== locate chart/operator migration PR candidates ==='
for repo in kubescape/helm-charts kubescape/kubescape-operator kubescape/operator; do
echo "--- $repo ---"
gh pr list --repo "$repo" --state all --limit 100 \
--json number,url,title,state,headRefName,updatedAt \
--jq '.[] | select((.headRefName // "") | test("migrate|sbob"; "i")) | {number,url,title,state,headRefName,updatedAt}' \
|| true
done
echo '=== node-agent module dependencies and component-workflow diff ==='
rg -n -C2 'storage|backend|synchronizer|softwarecomposition' go.mod go.sum || true
git diff --unified=80 'origin/main...HEAD' -- .github/workflows/component-tests.yaml
echo '=== cache and reconciler symbol map ==='
ast-grep outline pkg/objectcache/containerprofilecache/containerprofilecache.go \
--items all --type function,method,struct --match 'tryPopulateEntry|buildEntry|CachedContainerProfile' --view expanded
ast-grep outline pkg/objectcache/containerprofilecache/reconciler.go \
--items all --type function,method --match 'refreshOneEntry|rebuildEntryFromSources' --view expanded
echo '=== binding, validation, RV, and terminal-state code ==='
rg -n -C6 \
'UserDefinedProfileMetadataKey|UserCPRef|UserCPRV|isUserDefined|managed-by|ResourceVersion|Completed|Full|containerName' \
pkg/objectcache/containerprofilecache/containerprofilecache.go \
pkg/objectcache/containerprofilecache/reconciler.go
echo '=== deterministic Test_20/Test_21 and completion-wait implementation ==='
rg -n -C5 \
'Test_20_AlertOnPartialThenLearnProcessTest|Test_21_AlertOnPartialThenLearnNetworkTest|WaitForContainerProfileCompletion|Eventually|Deadline|Timeout' \
tests/component_test.go tests || true
echo '=== API-removal residue in production Go code (excluding tests/mocks) ==='
rg -n --glob '*.go' --glob '!**/*_test.go' --glob '!mocks/**' \
'\b(ApplicationProfile|NetworkNeighborhood|GetApplicationProfile|GetNetworkNeighborhood|ListApplicationProfiles|ListNetworkNeighborhoods)\b' \
pkg cmd || trueLength of output: 149501
I found one reliability gap in the per-container binding retry path:
The historical workflow ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/objectcache/containerprofilecache/reconciler.go (1)
334-350: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA deleted authored ContainerProfile is never dropped from the cache.
Line 340 treats every
userCPErras transient whene.UserCPRV != "", including a genuineNotFound. If the user deletes the authored CP, refresh returns early on every tick and the entry keeps enforcing the deleted profile (forcedCompleted/Full) for the container's lifetime. The add path already distinguishes the two cases viaapierrors.IsNotFound; mirror that here so a confirmed deletion falls through to the rebuild.🐛 Proposed fix
- if userCPErr != nil && e.UserCPRV != "" { + if userCPErr != nil && !apierrors.IsNotFound(userCPErr) && e.UserCPRV != "" { logger.L().Debug("refreshOneEntry: user-defined CP fetch failed; keeping cached entry", helpers.String("containerID", id), helpers.String("name", e.UserCPRef.Name), helpers.Error(userCPErr)) return }🤖 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/objectcache/containerprofilecache/reconciler.go` around lines 334 - 350, Update refreshOneEntry’s user-defined ContainerProfile error handling to distinguish apierrors.IsNotFound(userCPErr) from transient fetch failures. Preserve the cached-entry early return for non-NotFound errors when e.UserCPRV is set, but let confirmed NotFound errors fall through with userDefinedCP cleared so the entry is rebuilt without the deleted profile.
🧹 Nitpick comments (12)
pkg/objectcache/containerprofilecache/reconciler.go (1)
413-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate learned-CP check.
rebuildEntryFromSourcesis only reached fromrefreshOneEntry, which already nils a learneduserDefinedCPat Lines 355-362. Keeping both is harmless but the invariant is easier to maintain in one place (or extracted into a smallisAuthoredCPhelper shared by both paths andtryPopulateEntry).🤖 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/objectcache/containerprofilecache/reconciler.go` around lines 413 - 420, Remove the duplicate learned-profile filtering block from rebuildEntryFromSources, relying on refreshOneEntry’s existing handling before it invokes the rebuild path. Keep the authored-versus-learned check centralized there, or extract it into a shared isAuthoredCP helper only if needed by tryPopulateEntry.pkg/rulemanager/cel/libraries/containerprofile/cache_test.go (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "ApplicationProfile" naming in test function names.
These test functions now exercise
containerProfileLibrary/ContainerProfile/cp.*, but names (TestApplicationProfileCaching,TestApplicationProfileCacheDifferentArguments,TestApplicationProfileCacheExpiration,TestApplicationProfileCachePerformance,TestApplicationProfileCacheClearCache) still reference "ApplicationProfile", unlike sibling files (e.g.exec_test.go,open_test.go) which were renamed consistently.✏️ Suggested renames
-func TestApplicationProfileCaching(t *testing.T) { +func TestContainerProfileCaching(t *testing.T) { ... -func TestApplicationProfileCacheDifferentArguments(t *testing.T) { +func TestContainerProfileCacheDifferentArguments(t *testing.T) { ... -func TestApplicationProfileCacheExpiration(t *testing.T) { +func TestContainerProfileCacheExpiration(t *testing.T) { ... -func TestApplicationProfileCachePerformance(t *testing.T) { +func TestContainerProfileCachePerformance(t *testing.T) { ... -func TestApplicationProfileCacheClearCache(t *testing.T) { +func TestContainerProfileCacheClearCache(t *testing.T) {Also applies to: 156-156, 239-239, 315-315, 391-391
🤖 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/rulemanager/cel/libraries/containerprofile/cache_test.go` at line 16, Rename the five test functions in cache_test.go from the ApplicationProfile-prefixed names to ContainerProfile equivalents: TestContainerProfileCaching, TestContainerProfileCacheDifferentArguments, TestContainerProfileCacheExpiration, TestContainerProfileCachePerformance, and TestContainerProfileCacheClearCache. Keep their test implementations unchanged.pkg/rulemanager/cel/libraries/containerprofile/ap.go (1)
345-409: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the orphaned
CostEstimatorcases.
cp.was_internal_endpoint_accessed,cp.was_external_endpoint_accessed,cp.was_endpoint_accessed_with_direction,cp.was_endpoint_accessed_with_header, andcp.was_endpoint_accessed_with_header_valueare only referenced inEstimateCallCost; they are not declared in the library, so CEL cannot compile calls to them. Drop these dead branches unless these functions are intended to be added toDeclarations().🤖 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/rulemanager/cel/libraries/containerprofile/ap.go` around lines 345 - 409, Remove the five orphaned switch branches from containerProfileCostEstimator.EstimateCallCost: cp.was_internal_endpoint_accessed, cp.was_external_endpoint_accessed, cp.was_endpoint_accessed_with_direction, cp.was_endpoint_accessed_with_header, and cp.was_endpoint_accessed_with_header_value. Leave the declared function cases and default nil behavior unchanged; do not add declarations.tests/component_test.go (4)
935-941: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated completion wait (same as
Test_01).Lines 940-941 call
WaitForContainerProfileCompletion(160)twice; with AP/NN unified the second call is a no-op. Drop one.🤖 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 `@tests/component_test.go` around lines 935 - 941, Remove the duplicated second call to wl.WaitForContainerProfileCompletion(160) in this test, leaving a single completion wait after restarting the node-agent daemonset while preserving the existing error assertion.
2506-2523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo small leftovers in the new authored-profile helper path.
- Line 2535:
overlayNameis now dead — the profile name comes fromresources/containerprofile-user-defined-network.yaml. Remove it.applyUserDefinedContainerProfilereturns the pre-Createlocal, so the returned object has no UID/ResourceVersion. Every caller discards it today; either return the object fromCreateor drop the return value.Also applies to: 2529-2539
🤖 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 `@tests/component_test.go` around lines 2506 - 2523, The authored-profile helper path still contains an unused overlayName variable, and applyUserDefinedContainerProfile returns the pre-create object without server-populated metadata. Remove overlayName, then either return the object produced by ContainerProfiles(ns).Create or change the helper to have no return value and update its callers accordingly.
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
metav1andv1are two aliases of the same package.
k8s.io/apimachinery/pkg/apis/meta/v1is imported twice, and the file then mixesmetav1.GetOptions{}andv1.GetOptions{}for identical calls (e.g. Lines 1051-1054). Dropping thev1alias and normalising onmetav1removes the ambiguity.🤖 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 `@tests/component_test.go` around lines 29 - 30, Remove the duplicate v1 alias import from tests/component_test.go and normalize all references to the Kubernetes meta/v1 package on the existing metav1 alias, including GetOptions usages in the affected test code.
75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant duplicated completion wait with stale AP/NN wording.
With AP and NN merged into one ContainerProfile, the second
WaitForContainerProfileCompletion(80)is a no-op, and the messages still say "application profile" / "network neighborhood". Collapse to one wait and reword.🧹 Proposed cleanup
- err = wl.WaitForContainerProfileCompletion(80) - require.NoError(t, err, "Error waiting for application profile to be completed") - err = wl.WaitForContainerProfileCompletion(80) - require.NoError(t, err, "Error waiting for network neighborhood to be completed") + err = wl.WaitForContainerProfileCompletion(80) + require.NoError(t, err, "Error waiting for container profile to be completed")🤖 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 `@tests/component_test.go` around lines 75 - 85, In the test flow around WaitForContainerProfileCompletion, remove the duplicated second wait and retain a single completion wait. Update its assertion message to reference the merged ContainerProfile rather than application profile or network neighborhood wording, leaving the subsequent sleep and profile retrieval unchanged.tests/resources/network-wildcards/18-cluster-dns-via-mid-ellipsis.yaml (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "this NN" wording.
Reads "declare the equivalent via this NN" — the fixture is a
ContainerProfilenow.🤖 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 `@tests/resources/network-wildcards/18-cluster-dns-via-mid-ellipsis.yaml` at line 31, Update the comment near the ContainerProfile fixture to replace the stale “this NN” wording with “this ContainerProfile,” preserving the comment’s intended meaning.tests/testutils/k8s.go (1)
354-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WaitForContainerProfileCompletionWithBlacklistduplicatesWaitForContainerProfile.The only delta is the name check; the status loop, backoff config, and notify callback are copies. Consider a single private
waitForContainerProfile(maxRetries, expectedStatus, blacklist)with the two exported wrappers delegating to it, so a future change to the gate (e.g. filtering transient per-instance profiles) applies to both.🤖 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 `@tests/testutils/k8s.go` around lines 354 - 377, Refactor WaitForContainerProfileCompletionWithBlacklist and the existing WaitForContainerProfile to delegate to one private waitForContainerProfile helper accepting maxRetries, expectedStatus, and blacklist. Move the shared matching, status validation, retry/backoff configuration, and notification logic into the helper; preserve each wrapper’s current expected status and blacklist behavior.tests/resources/network-wildcards/14-recursive-star-rejected.yaml (1)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
NetworkNeighborhoodreference in the assertion checklist.The fixture is now a
ContainerProfile; the step-3 assertion text should say so.🤖 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 `@tests/resources/network-wildcards/14-recursive-star-rejected.yaml` at line 17, Update the step-3 assertion checklist comment to refer to a ContainerProfile named nw-14-recursive-rejected instead of a NetworkNeighborhood, matching the fixture’s current resource type.tests/resources/mc35-multi-container-userdefined-deployment.yaml (2)
24-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the Debian image by digest.
debian:12-slimis mutable, so registry changes can make this multi-container component test non-reproducible. Use the tested digest, consistent with the digest-pinned fixtures elsewhere in this cohort.🤖 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 `@tests/resources/mc35-multi-container-userdefined-deployment.yaml` around lines 24 - 27, Replace the mutable debian:12-slim image references in the multi-container deployment fixture, including the main container and sidecar, with the tested Debian 12 slim image digest used by the corresponding digest-pinned fixtures.
22-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRun this test workload without root or privilege escalation.
Both containers default to root and omit
allowPrivilegeEscalation: false; neithersleepnor the exec checks require these privileges. Add a non-root UID,allowPrivilegeEscalation: false, and the runtime-default seccomp profile.🤖 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 `@tests/resources/mc35-multi-container-userdefined-deployment.yaml` around lines 22 - 28, Update both containers in the deployment around the app and sidecar definitions to run as a non-root UID, disable privilege escalation, and use the runtime-default seccomp profile. Apply equivalent security context settings to each container without changing their images, commands, or other workload behavior.Source: Linters/SAST tools
🤖 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/objectcache/containerprofilecache/containerprofilecache.go`:
- Around line 330-379: Update the transient-error default branch in
containerprofilecache.go’s resolver to set resolvedOverlayName to
perContainerName, ensuring only confirmed NotFound uses the bare label retry
target. In containerprofilecache_test.go lines 225-249, change the
transient-error expectation to override-nginx and add coverage for per-container
NotFound followed by a transient bare-profile failure, expecting the bare
fallback target.
In `@tests/component_test.go`:
- Around line 1140-1155: Update the phase-2 setup around countR0001("ls") to
prove the R0001 cooldown budget was not exhausted during phase 1 before
asserting no new alert. Add an assertion that the pre-phase-2 ls count remains
below the per-container/per-rule cap of 10, while preserving the existing
phase-2 profile enforcement checks.
- Around line 729-743: The test should stop immediately when either profile
operation fails instead of continuing with invalid data. In the test flow around
WaitForContainerProfileCompletion and GetContainerProfile, replace both t.Errorf
checks with require.NoError assertions so containerProfile is only dereferenced
after successful retrieval.
In `@tests/resources/network-wildcards/20-multi-container-mixed-wildcards.yaml`:
- Around line 18-58: Add the required per-container binding labels to the paired
workload fixture for the frontend and sidecar containers, mapping each container
to its corresponding authored profile using the established profile-label key
format. Do not rely on the `# container:` comments or shared `matchLabels` for
profile resolution, and preserve the existing profile definitions.
In `@tests/testutils/k8s.go`:
- Around line 286-348: Filter transient per-instance profiles from the helpers
in tests/testutils/k8s.go lines 286-348 by preferring merged profiles in
GetContainerProfile and GetContainerProfiles; when selecting among merged
candidates, add a deterministic secondary name tiebreaker after creation time.
In tests/testutils/k8s.go lines 379-397, update the status gate to skip
non-merged profiles so transient failed or ready profiles do not block
completion.
- Around line 571-589: Update getEgressDnsNames and getIngressDnsNames to add
each neighbor’s singular DNS value as well as all entries in DNSNames, ensuring
profiles containing only DNS are included in the returned sets and assertion
checks cannot pass vacuously.
---
Outside diff comments:
In `@pkg/objectcache/containerprofilecache/reconciler.go`:
- Around line 334-350: Update refreshOneEntry’s user-defined ContainerProfile
error handling to distinguish apierrors.IsNotFound(userCPErr) from transient
fetch failures. Preserve the cached-entry early return for non-NotFound errors
when e.UserCPRV is set, but let confirmed NotFound errors fall through with
userDefinedCP cleared so the entry is rebuilt without the deleted profile.
---
Nitpick comments:
In `@pkg/objectcache/containerprofilecache/reconciler.go`:
- Around line 413-420: Remove the duplicate learned-profile filtering block from
rebuildEntryFromSources, relying on refreshOneEntry’s existing handling before
it invokes the rebuild path. Keep the authored-versus-learned check centralized
there, or extract it into a shared isAuthoredCP helper only if needed by
tryPopulateEntry.
In `@pkg/rulemanager/cel/libraries/containerprofile/ap.go`:
- Around line 345-409: Remove the five orphaned switch branches from
containerProfileCostEstimator.EstimateCallCost:
cp.was_internal_endpoint_accessed, cp.was_external_endpoint_accessed,
cp.was_endpoint_accessed_with_direction, cp.was_endpoint_accessed_with_header,
and cp.was_endpoint_accessed_with_header_value. Leave the declared function
cases and default nil behavior unchanged; do not add declarations.
In `@pkg/rulemanager/cel/libraries/containerprofile/cache_test.go`:
- Line 16: Rename the five test functions in cache_test.go from the
ApplicationProfile-prefixed names to ContainerProfile equivalents:
TestContainerProfileCaching, TestContainerProfileCacheDifferentArguments,
TestContainerProfileCacheExpiration, TestContainerProfileCachePerformance, and
TestContainerProfileCacheClearCache. Keep their test implementations unchanged.
In `@tests/component_test.go`:
- Around line 935-941: Remove the duplicated second call to
wl.WaitForContainerProfileCompletion(160) in this test, leaving a single
completion wait after restarting the node-agent daemonset while preserving the
existing error assertion.
- Around line 2506-2523: The authored-profile helper path still contains an
unused overlayName variable, and applyUserDefinedContainerProfile returns the
pre-create object without server-populated metadata. Remove overlayName, then
either return the object produced by ContainerProfiles(ns).Create or change the
helper to have no return value and update its callers accordingly.
- Around line 29-30: Remove the duplicate v1 alias import from
tests/component_test.go and normalize all references to the Kubernetes meta/v1
package on the existing metav1 alias, including GetOptions usages in the
affected test code.
- Around line 75-85: In the test flow around WaitForContainerProfileCompletion,
remove the duplicated second wait and retain a single completion wait. Update
its assertion message to reference the merged ContainerProfile rather than
application profile or network neighborhood wording, leaving the subsequent
sleep and profile retrieval unchanged.
In `@tests/resources/mc35-multi-container-userdefined-deployment.yaml`:
- Around line 24-27: Replace the mutable debian:12-slim image references in the
multi-container deployment fixture, including the main container and sidecar,
with the tested Debian 12 slim image digest used by the corresponding
digest-pinned fixtures.
- Around line 22-28: Update both containers in the deployment around the app and
sidecar definitions to run as a non-root UID, disable privilege escalation, and
use the runtime-default seccomp profile. Apply equivalent security context
settings to each container without changing their images, commands, or other
workload behavior.
In `@tests/resources/network-wildcards/14-recursive-star-rejected.yaml`:
- Line 17: Update the step-3 assertion checklist comment to refer to a
ContainerProfile named nw-14-recursive-rejected instead of a
NetworkNeighborhood, matching the fixture’s current resource type.
In `@tests/resources/network-wildcards/18-cluster-dns-via-mid-ellipsis.yaml`:
- Line 31: Update the comment near the ContainerProfile fixture to replace the
stale “this NN” wording with “this ContainerProfile,” preserving the comment’s
intended meaning.
In `@tests/testutils/k8s.go`:
- Around line 354-377: Refactor WaitForContainerProfileCompletionWithBlacklist
and the existing WaitForContainerProfile to delegate to one private
waitForContainerProfile helper accepting maxRetries, expectedStatus, and
blacklist. Move the shared matching, status validation, retry/backoff
configuration, and notification logic into the helper; preserve each wrapper’s
current expected status and blacklist 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 Plus
Run ID: bd9d1656-54f3-4164-9943-798afe0e2815
📒 Files selected for processing (100)
.github/workflows/component-tests.yamlmocks/readfiles.gomocks/readfiles_test.gomocks/testdata/collection_applicationprofiles.jsonmocks/testdata/collection_networkneighborhood.jsonmocks/testdata/nginx_applicationprofiles.jsonmocks/testdata/nginx_networkneighborhood.jsonpkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/otel/otel_metrics_manager.gopkg/metricsmanager/prometheus/prometheus.gopkg/objectcache/containerprofilecache/containerprofilecache.gopkg/objectcache/containerprofilecache/containerprofilecache_test.gopkg/objectcache/containerprofilecache/integration_helpers_test.gopkg/objectcache/containerprofilecache/metrics.gopkg/objectcache/containerprofilecache/projection.gopkg/objectcache/containerprofilecache/projection_apply.gopkg/objectcache/containerprofilecache/projection_apply_test.gopkg/objectcache/containerprofilecache/projection_golden_test.gopkg/objectcache/containerprofilecache/projection_test.gopkg/objectcache/containerprofilecache/reconciler.gopkg/objectcache/containerprofilecache/reconciler_test.gopkg/objectcache/containerprofilecache/testdata/golden/network_all.jsonpkg/objectcache/containerprofilecache/testdata/golden/rich_filtered.jsonpkg/objectcache/containerprofilecache/testdata/golden/rich_passthrough.jsonpkg/objectcache/v1/mock.gopkg/rulemanager/cel/cel.gopkg/rulemanager/cel/libraries/containerprofile/ap.gopkg/rulemanager/cel/libraries/containerprofile/ap_smoke_test.gopkg/rulemanager/cel/libraries/containerprofile/cache_test.gopkg/rulemanager/cel/libraries/containerprofile/capability.gopkg/rulemanager/cel/libraries/containerprofile/capability_test.gopkg/rulemanager/cel/libraries/containerprofile/exec.gopkg/rulemanager/cel/libraries/containerprofile/exec_podspec_test.gopkg/rulemanager/cel/libraries/containerprofile/exec_test.gopkg/rulemanager/cel/libraries/containerprofile/http.gopkg/rulemanager/cel/libraries/containerprofile/http_test.gopkg/rulemanager/cel/libraries/containerprofile/integration_test.gopkg/rulemanager/cel/libraries/containerprofile/open.gopkg/rulemanager/cel/libraries/containerprofile/open_bench_test.gopkg/rulemanager/cel/libraries/containerprofile/open_test.gopkg/rulemanager/cel/libraries/containerprofile/prestop_cache.gopkg/rulemanager/cel/libraries/containerprofile/prestop_cache_test.gopkg/rulemanager/cel/libraries/containerprofile/syscall.gopkg/rulemanager/cel/libraries/containerprofile/syscall_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/cache_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/fixtures_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/integration_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/network.gopkg/rulemanager/cel/libraries/containerprofilenetwork/network_coverage_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/network_test.gopkg/rulemanager/cel/libraries/containerprofilenetwork/nn.gopkg/rulemanager/cel/libraries/containerprofilenetwork/wildcard_test.gopkg/rulemanager/profilehelper/profilehelper_test.gopkg/rulemanager/rule_manager_hasfinal_test.gopkg/seccompmanager/v1/seccomp_manager_test.gopkg/storage/storage_interface.gopkg/storage/storage_mock.gopkg/storage/v1/applicationprofile.gopkg/storage/v1/networkneighborhood.gopkg/storage/v1/storage.gopkg/watcher/dynamicwatcher/watch.gopkg/watcher/dynamicwatcher/watch_test.gotests/chart/templates/node-agent/default-rules.yamltests/component_bulking_test.gotests/component_test.gotests/resources/containerprofile-fusioncore-network.yamltests/resources/containerprofile-user-defined-network.yamltests/resources/curl-exec-arg-wildcards-deployment.yamltests/resources/mc35-cp-app.yamltests/resources/mc35-cp-sidecar.yamltests/resources/mc35-multi-container-userdefined-deployment.yamltests/resources/network-wildcards/00-fusioncore-homoglyph-attack.yamltests/resources/network-wildcards/01-literal-ipv4.yamltests/resources/network-wildcards/02-literal-ipv6.yamltests/resources/network-wildcards/03-cidr-ipv4.yamltests/resources/network-wildcards/04-cidr-ipv6.yamltests/resources/network-wildcards/05-any-ip-sentinel.yamltests/resources/network-wildcards/06-any-as-cidr.yamltests/resources/network-wildcards/07-mixed-ip-list.yamltests/resources/network-wildcards/08-deprecated-ipaddress.yamltests/resources/network-wildcards/09-dns-literal.yamltests/resources/network-wildcards/10-dns-leading-wildcard.yamltests/resources/network-wildcards/11-dns-mid-ellipsis.yamltests/resources/network-wildcards/12-dns-trailing-star.yamltests/resources/network-wildcards/13-dns-trailing-dot-normalisation.yamltests/resources/network-wildcards/14-recursive-star-rejected.yamltests/resources/network-wildcards/15-egress-and-ingress.yamltests/resources/network-wildcards/16-egress-none.yamltests/resources/network-wildcards/17-realistic-stripe-api.yamltests/resources/network-wildcards/18-cluster-dns-via-mid-ellipsis.yamltests/resources/network-wildcards/19-port-protocol-with-cidr.yamltests/resources/network-wildcards/20-multi-container-mixed-wildcards.yamltests/resources/nginx-user-defined-deployment.yamltests/resources/nnlint_test.gotests/resources/partial-network-deployment.yamltests/resources/partial-process-deployment.yamltests/resources/r0002-files-access-enabled.yamltests/testutils/k8s.go
💤 Files with no reviewable changes (16)
- mocks/testdata/collection_networkneighborhood.json
- mocks/testdata/nginx_applicationprofiles.json
- mocks/testdata/nginx_networkneighborhood.json
- pkg/storage/storage_interface.go
- pkg/storage/v1/applicationprofile.go
- mocks/testdata/collection_applicationprofiles.json
- pkg/storage/v1/networkneighborhood.go
- pkg/objectcache/containerprofilecache/metrics.go
- mocks/readfiles_test.go
- pkg/objectcache/containerprofilecache/projection.go
- pkg/storage/storage_mock.go
- pkg/seccompmanager/v1/seccomp_manager_test.go
- pkg/objectcache/containerprofilecache/projection_test.go
- pkg/objectcache/containerprofilecache/integration_helpers_test.go
- pkg/objectcache/containerprofilecache/projection_apply.go
- pkg/watcher/dynamicwatcher/watch.go
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Referenced PR metadata and changed files ==='
for item in \
'kubescape/node-agent 864' \
'kubescape/storage 351' \
'kubescape/backend 55' \
'kubescape/synchronizer 168'
do
set -- $item
repo="$1"
number="$2"
echo "--- ${repo}#${number} ---"
gh pr view "$number" --repo "$repo" \
--json url,title,state,isDraft,baseRefName,headRefName,commits,files \
--jq '{url,title,state,isDraft,baseRefName,headRefName,commits:[.commits[]|{oid,messageHeadline}],files:[.files[]|{path,additions,deletions}]}'
done
echo '=== Locate and inspect Helm chart PR `#888` ==='
for repo in kubescape/helm-charts kubescape/kubescape-operator; do
echo "--- ${repo}`#888` ---"
gh pr view 888 --repo "$repo" \
--json url,title,state,isDraft,baseRefName,headRefName,commits,files \
--jq '{url,title,state,isDraft,baseRefName,headRefName,commits:[.commits[]|{oid,messageHeadline}],files:[.files[]|{path,additions,deletions}]}' \
|| true
done
echo '=== Cited component-test workflow ==='
gh run view 30560284636 --repo k8sstormcenter/node-agent \
--json url,status,conclusion,event,headSha,displayTitle,jobs \
--jq '{url,status,conclusion,event,headSha,displayTitle,jobs:[.jobs[]|{name,status,conclusion,startedAt,completedAt}]}'
echo '=== Primary PR: cache/reconciler source map ==='
ast-grep outline pkg/objectcache/containerprofilecache/containerprofilecache.go \
--items all --type struct,function,method \
--match 'CachedContainerProfile|tryPopulateEntry|buildEntry' --view expanded
ast-grep outline pkg/objectcache/containerprofilecache/reconciler.go \
--items all --type function,method \
--match 'refreshOneEntry|rebuildEntryFromSources' --view expanded
echo '=== Primary PR: per-container candidate, fallback, retry, and RV handling ==='
rg -n -C8 \
'UserDefinedProfileMetadataKey|UserCPRef|UserCPRV|GetContainerProfile|IsNotFound|NotFound|containerName|ResourceVersion' \
pkg/objectcache/containerprofilecache/containerprofilecache.go \
pkg/objectcache/containerprofilecache/reconciler.go
echo '=== Primary PR: migration pins and production legacy API residue ==='
rg -n -C3 'storage|backend|synchronizer|softwarecomposition' go.mod go.sum || true
rg -n --glob '*.go' --glob '!**/*_test.go' --glob '!mocks/**' \
'\b(ApplicationProfile|NetworkNeighborhood|GetApplicationProfile|GetNetworkNeighborhood|ListApplicationProfiles|ListNetworkNeighborhoods)\b' \
pkg cmd || trueLength of output: 88251 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Node-agent: exact per-container lookup implementation ==='
sed -n '320,425p' pkg/objectcache/containerprofilecache/containerprofilecache.go
sed -n '280,390p' pkg/objectcache/containerprofilecache/reconciler.go
echo '=== Node-agent: focused tests for per-container fallback and transient retries ==='
rg -n -C6 \
'per-container|PerContainer|overlayOnly|Transient|UserCPRef|UserDefinedCP|resolvedOverlayName' \
pkg/objectcache/containerprofilecache/*_test.go
echo '=== Node-agent: component chart and workflow image/version wiring ==='
sed -n '1,180p' .github/workflows/component-tests.yaml
sed -n '1,240p' tests/chart/values.yaml 2>/dev/null || true
rg -n -C4 'storage|node-agent|synchronizer|image:|tag:' tests/chart .github/workflows/component-tests.yaml
echo '=== Storage `#351`: public API / ContainerProfile registration and completed-update guard ==='
gh pr diff 351 --repo kubescape/storage -- \
pkg/apis/softwarecomposition/register.go \
pkg/apis/softwarecomposition/v1beta1/generated.proto \
pkg/registry/file/containerprofile_processor.go \
pkg/registry/file/containerprofile_storage.go \
pkg/registry/file/containerprofile_completed_guard_realmech_test.go \
| sed -n '1,1000p'
echo '=== Backend `#55`: generated storage RPC and client API removals ==='
gh pr diff 55 --repo kubescape/backend -- \
pkg/client/v1/proto/storage_service.proto \
pkg/client/v1/storageclient.go \
pkg/client/v1/storageclient_coverage_test.go \
| sed -n '1,1000p'
echo '=== Synchronizer `#168`: resource filter and configured watched resources ==='
gh pr diff 168 --repo kubescape/synchronizer -- \
adapters/incluster/v1/client.go \
configuration/client/config.json \
config/config_test.go \
adapters/incluster/v1/client_cp_test.go \
| sed -n '1,1000p'
echo '=== Helm charts `#888`: RBAC, synchronizer configuration, and image tags ==='
gh pr diff 888 --repo kubescape/helm-charts -- \
charts/kubescape-operator/Chart.yaml \
charts/kubescape-operator/values.yaml \
charts/kubescape-operator/templates/node-agent/clusterrole.yaml \
charts/kubescape-operator/templates/synchronizer/clusterrole.yaml \
charts/kubescape-operator/templates/synchronizer/configmap.yaml \
| sed -n '1,1200p'Length of output: 90016 🏁 Script executed: #!/bin/bash
set -euo pipefail
fetch_selected_patches() {
local repo="$1"
local number="$2"
shift 2
local wanted
wanted="$(printf '%s\n' "$@")"
gh api --paginate "repos/${repo}/pulls/${number}/files?per_page=100" \
| jq -r --argjson names "$(printf '%s\n' "$wanted" | jq -R . | jq -s .)" '
.[] | select(.filename as $f | $names | index($f)) |
"===== \(.filename) =====\n\(.patch // "[patch unavailable]")\n"
'
}
echo '=== Storage `#351` selected API/storage patches ==='
fetch_selected_patches kubescape/storage 351 \
pkg/apis/softwarecomposition/register.go \
pkg/apis/softwarecomposition/v1beta1/generated.proto \
pkg/registry/file/containerprofile_processor.go \
pkg/registry/file/containerprofile_storage.go \
pkg/registry/file/containerprofile_completed_guard_realmech_test.go
echo '=== Backend `#55` selected RPC/client patches ==='
fetch_selected_patches kubescape/backend 55 \
pkg/client/v1/proto/storage_service.proto \
pkg/client/v1/storageclient.go \
pkg/client/v1/storageclient_coverage_test.go
echo '=== Synchronizer `#168` selected watch/filter patches ==='
fetch_selected_patches kubescape/synchronizer 168 \
adapters/incluster/v1/client.go \
configuration/client/config.json \
config/config_test.go \
adapters/incluster/v1/client_cp_test.go
echo '=== Helm charts `#888` selected deployment wiring patches ==='
fetch_selected_patches kubescape/helm-charts 888 \
charts/kubescape-operator/Chart.yaml \
charts/kubescape-operator/values.yaml \
charts/kubescape-operator/templates/node-agent/clusterrole.yaml \
charts/kubescape-operator/templates/synchronizer/clusterrole.yaml \
charts/kubescape-operator/templates/synchronizer/configmap.yamlLength of output: 89906
The ContainerProfile-only API and deployment wiring are aligned: storage removes the legacy types, backend reserves the retired protobuf fields, synchronizer now lists/watches I found one blocking reliability issue in node-agent:
✅ Action performedReview finished.
|
Signed-off-by: entlein <einentlein@gmail.com>
|
Verdict on the Component Tests: |
Signed-off-by: entlein <einentlein@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/metricsmanager/metrics_manager_mock.go`:
- Around line 70-71: Remove the duplicate MetricsMock.ReportDedupEvent method
declaration, keeping one implementation with the existing signature so the
package compiles.
🪄 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 Plus
Run ID: c254be1d-4408-4eb4-84ec-b48483430322
📒 Files selected for processing (5)
pkg/metricsmanager/metrics_manager_interface.gopkg/metricsmanager/metrics_manager_mock.gopkg/metricsmanager/metrics_manager_noop.gopkg/metricsmanager/otel/otel_metrics_manager.gopkg/metricsmanager/prometheus/prometheus.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/metricsmanager/metrics_manager_interface.go
- pkg/metricsmanager/otel/otel_metrics_manager.go
- pkg/metricsmanager/metrics_manager_noop.go
- pkg/metricsmanager/prometheus/prometheus.go
| func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {} | ||
| func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate ReportDedupEvent declaration.
MetricsMock.ReportDedupEvent is declared twice with the same signature. Go rejects duplicate method declarations, so the package cannot compile. Keep only one declaration.
Proposed fix
func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {}
-func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {} | |
| func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {} | |
| func (m *MetricsMock) ReportDedupEvent(eventType utils.EventType, duplicate bool) {} |
🧰 Tools
🪛 GitHub Actions: Performance Benchmark / 0_benchmark.txt
[error] 71-71: Go build failed: method MetricsMock.ReportDedupEvent is declared more than once; it was already declared at line 70.
🪛 GitHub Actions: Performance Benchmark / benchmark
[error] 70-71: Go build failed: method MetricsMock.ReportDedupEvent is declared twice; the declaration at line 71 duplicates line 70. Failed command: CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o node-agent ./cmd/main.go
🪛 GitHub Actions: pull_request_created / 0_pr-created _ test _ Create cross-platform build.txt
[error] 70-71: go test failed: method MetricsMock.ReportDedupEvent is declared twice. Remove the duplicate declaration.
🪛 GitHub Actions: pull_request_created / pr-created _ test _ Create cross-platform build
[error] 71-71: go test failed: method MetricsMock.ReportDedupEvent is declared more than once; it was already declared at line 70.
🪛 GitHub Check: benchmark
[failure] 71-71:
method MetricsMock.ReportDedupEvent already declared at pkg/metricsmanager/metrics_manager_mock.go:70:23
🪛 GitHub Check: pr-created / test / Create cross-platform build
[failure] 71-71:
method MetricsMock.ReportDedupEvent already declared at pkg/metricsmanager/metrics_manager_mock.go:70:23
🪛 golangci-lint (2.12.2)
[error] 71-71: : # github.com/kubescape/node-agent/pkg/metricsmanager
pkg/metricsmanager/metrics_manager_mock.go:71:23: method MetricsMock.ReportDedupEvent already declared at pkg/metricsmanager/metrics_manager_mock.go:70:23
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/metricsmanager/metrics_manager_mock.go` around lines 70 - 71, Remove the
duplicate MetricsMock.ReportDedupEvent method declaration, keeping one
implementation with the existing signature so the package compiles.
Source: Linters/SAST tools
Signed-off-by: entlein <einentlein@gmail.com>
2 participants
Overview
migrating old CRD structure to container profile, migrating also all tests
Additional Information
See the issue
Also see new documentation under kubescape.io
Should we keep the
migration support of the old wayor just completely switch instantly?Discussion with self -> fully get rid of all AP/NN/ug- the respective CRDs, and clean this all up.
Why? I personally think, it is too confusing to have too many profile types and labels and what-not.
I understand that this set of PRs might not get merged for a long time/ if at all, if there are legacy consumers of the overlays
TODO
Multi-container binding (recorded decision)
A ContainerProfile is per-container, but the user-defined-profile binding is a pod label. Go-forward contract:
<label-value>-<containerName>, where<label-value>is the pod labelkubescape.io/user-defined-profile. The read path binds each container to its own ContainerProfile by that name, falling back to the bare<label-value>only for single-container pods.Example — pod label
kubescape.io/user-defined-profile: mc35on a pod with containersapp+sidecar→ ContainerProfilesmc35-appandmc35-sidecar; a single-container pod → baremc35.Tested: node-agent
Test_35_MultiContainerPerContainerBinding(per-containerR0001isolation, no cross-inheritance) and a storage per-containerGeneratedNetworkPolicytest.Summary by CodeRabbit
New Features
Bug Fixes
Tests