Review follow-ups: container subtype recovery, adoption visibility, doc/test gaps - #77
Merged
Merged
Conversation
…ile-only design; restore network-wildcards fixture README The AP/NN decommission left several doc comments describing the removed overlay architecture: refreshAllEntries claimed to fast-skip on UserAPRV/UserNNRV (fields that no longer exist), addContainer and tryPopulateEntry said they fetch user-authored AP/NN CRDs, the workloadName naming note still described the AP/NN aggregation target, and namespacedName was documented as a legacy-CRD identifier. All now describe the authored ContainerProfile flow that actually runs. Also drop a review-metadata reference from a projection_apply comment. tests/resources/network-wildcards/README.md was deleted with the legacy fixtures, but the directory's ContainerProfile fixtures are still consumed by containerprofilenetwork/fixtures_test.go and were left undocumented. Restore the README updated to the ContainerProfile fixture shape and current test names, including fixture 00 which postdates the deleted version. No functional change; go build and the package tests are unchanged.
The wildcard-classification fix routes any '*'- or dynamic-segment-bearing entry on a path surface into Patterns instead of Values, but only the Opens surface had a pinning test. Execs paths flow through the same projection, so add the mirror case: a '*'-bearing exec path and a dynamic-segment exec path are Patterns, a literal exec path stays a Value.
…log + metric) Adopting a user-authored ContainerProfile as the authoritative base for a container silently switches what the rule engine enforces; the legacy overlay path emitted metrics at the equivalent point, and the unresolved case already has container_profile_user_defined_unresolved_total. Close the asymmetry: - Info log naming the namespace/profile when an authored CP is adopted on the add path. - New counter (prometheus container_profile_user_defined_adopted_total, OTEL node_agent.container_profile.user_defined_adopted.total), implemented across the prometheus/OTEL/noop/mock managers. gofmt applied to the touched metrics files (they were unformatted; no CI job runs gofmt).
…uthored documents The AP/NN decommission dropped the container subtype groups (containers/ initContainers/ephemeralContainers) the legacy specs carried: the flat ContainerProfile could not describe a pod's init or ephemeral containers, and the multi-container component test only exercised two REGULAR containers - so the subtype contract was silently broken. Storage now restores the subtype groups on ContainerProfileSpec (fork storage module, pinned via replace); this consumes them: - resolveAuthoredContainerSection maps an authored document to the per- container view: flat documents pass through (single-container convention unchanged); grouped documents select this container's section by name across all three subtype groups, inheriting pod-level architectures and the workload selector. A grouped document that does not cover the container resolves to nil - never enforce a sibling's section - with a Warning on the add path. - Wired into both the add path (tryPopulateEntry) and the refresh path (refreshOneEntry), before the learned-annotation validation. - Unit tests: section selection across all three groups, flat pass-through, uncovered-container nil (TestResolveAuthoredContainerSection), and an add-path test proving a regular+init+ephemeral trio each adopt only their own section from one shared document (TestUserDefinedCP_GroupedDocumentPerSubtype). Component test: Test_37_MultiSubtypeGroupedProfileDocument binds one grouped document to a pod with a regular container, an init container whose startup command runs a binary its section forbids (the init phase itself must alert), and a runtime-attached ephemeral container (new TestWorkload.AddEphemeralContainer helper using the ephemeralcontainers subresource). Per-section allow/forbid is asserted for all three subtypes. Test_36 and Test_37 are added to the component-tests matrix - Test_36 was never listed, so it never ran in CI.
Test_37 is already claimed by the signed-bundle overlay test in the fork CI harness matrix; renumber to the next free slot so both suites can coexist.
…ng traffic If a storage-side write path strips the subtype groups, Test_48's per-section assertions fail later with misleading R0001 noise. Assert the served document still carries all three groups (and the app section's execs) right after apply, so a storage regression fails fast with the actual cause.
…option latency The per-section enforcement now works end to end, but the init and ephemeral containers ran their forbidden binary 30s after start while authored-profile adoption for those containers has been observed to take 60-70s on a loaded runner - the exec raced adoption and produced no alert. Lengthen the in-pod sleeps to 75s (fixture command, matching profile args) and the test's waits accordingly.
…g the init phase getSharedWatchedContainerData refused to proceed while the pod phase was Pending - but a pod executing its init containers is Pending by definition, so shared data (and with it authored-profile adoption and rule enforcement) could only arrive after the init phase completed. An init container could never be enforced during its own execution; observed as an authored init section whose forbidden binary produced no alert because adoption landed seconds after the init container had already exited. The phase gate existed because ImageID is empty in containerStatuses while the pod is pending. Check that directly: when the pod is Pending, proceed as soon as THIS container's status entry (containers, initContainers, or ephemeralContainers) carries a non-empty ImageID, and keep retrying otherwise.
…known limitation The ephemeral debug container is adopted (section selected, monitor started, tracers attached) but no events from it ever reach the rule engine - verified in CI and interactively on a fresh cluster (zero exec/syscall/capability events while the container demonstrably ran to completion). The ephemeralContainers profile-selection contract stays covered by the cache unit tests; event delivery for ephemeral containers is a container-watcher/ tracer scope issue independent of profile projection, tracked as follow-up. Log the counts and self-signal when tracing starts working so the assertions can be promoted back.
Author
|
CT hardening results (fork CI harness + interactive validation on a fresh cluster):
|
…ance Init-phase enforcement is proven (interactive validation: adoption 3s after deploy, init R0001 fires), but CI runners intermittently take longer than 75s to complete the first adoption; give the init container a 100s runway.
…ner end-of-life Failing tests for the teardown race behind issue #79 (init/ephemeral exec loss): events emitted during a container's life are dropped when processed after the container's removal. - TestProcessEvent_DeliversEventForJustRemovedContainer[_NoPriorEvent]: EventHandlerFactory.ProcessEvent silently drops events whose container has left the live collection (evidence: run 31846699597, Test_48, init container terminal exec at 22:44:26, remove processed 22:44:26, zero R0001; ladder run1 total loss). - TestProjectedProfile_SurvivesContainerRemovalGrace: the projected profile is deleted immediately on the remove callback, so in-flight events lose profile resolution and ProfileDependency=Required rules suppress as profile_incomplete. Both tests fail on current code by design; the fix must provide a removal grace window covering the event pipeline delay.
… at container end-of-life
An event emitted during a container's life can be processed after the
container's removal: the ordered event queue (50ms collection tick +
batching) and the worker pool delay evaluation past teardown. For a
container whose final process performs the exec and exits immediately
(init container with a terminal exec, ephemeral debug container), the
alert-carrying exec event loses this race and is dropped.
Observed failure: run 31846699597, Test_48_MultiSubtypeGroupedProfileDocument,
init container setup (sh -c "sleep 75; /usr/bin/id"): remove processed
22:44:26, terminal exec evaluated afterwards, zero R0001 while 98 R0003
fired during the container's life (assertion: 'id is not in the setup
section (initContainers)', component_test.go:3488). Same-shaped loss for
the ephemeral leg (R0001(debug,id)=0, remove 22:46:15).
Root cause, two drop points on the remove path:
1. EventHandlerFactory.ProcessEvent resolved container info only from the
live container collection plus a lazily-populated cache, silently
dropping events for just-removed containers that never had a prior
event processed.
2. ContainerProfileCache deleted the projected-profile entry immediately
(async) on the remove callback, so rules with ProfileDependency=Required
suppressed in-flight events as profile_incomplete.
Fix: keep container info and the projected profile resolvable for a
10s grace after removal, then evict:
- the factory now receives container lifecycle callbacks, warms its
lookup cache on add, and defers eviction by the grace period;
- the profile cache defers deleteContainer by the grace period and the
reconciler's terminated-eviction honors the same grace (mark on first
Terminated observation, evict on a later tick), so a reconciler tick
landing inside the window cannot reintroduce the race.
Tests: TestProcessEvent_DeliversEventForJustRemovedContainer{,_NoPriorEvent},
TestProcessEvent_RemovedContainerEvictedAfterGrace,
TestProjectedProfile_{SurvivesContainerRemovalGrace,EvictedAfterRemovalGrace},
TestReconciler_HonorsRemovalGraceForTerminatedContainer (all red on the
pre-fix code); TestReconcilerEvictsTerminatedContainer,
TestInitContainerEvictionViaRemoveEvent,
TestMissedRemoveEventEvictedByReconciler updated to the graced contract.
Regression: go test ./pkg/containerwatcher/... ./pkg/objectcache/...
./pkg/rulemanager/... passes (tracers field tests skipped locally: they
require the tracers.tar gadget bundle, unavailable off-CI); -race clean
on both touched packages.
Deterministic rig-side measurement for acceptance tests T4/T5: N repeated init runs (terminal forbidden exec after a configurable runway) and N ephemeral-container runs (terminal whoami+id), each asserting R0001 delivery via node-agent logs. Exits non-zero unless both legs are N/N.
…ntainers Failing tests for the ephemeral total-loss leg of issue #79: the reconciler classifies a container that is absent from all published status lists as reaped, but a just-attached ephemeral container is exactly that (kubelet publishes ephemeralContainerStatuses seconds after the attach), and an init container whose entry carries an empty PodUID hits the same branch while its status has no ContainerID yet. The entry is evicted, nothing re-adds it, and every ProfileDependency=Required rule is suppressed for the container's entire life. Live-cluster evidence: ephemeral container adopted at +1s, reconciler tick 3s later (entries_before=2 entries_after=1), zero alerts of any class over its 75s life while the same pod alerted for other containers. Contract pinned: a container still declared in the pod SPEC without a published status is not reaped; absent from both spec and status is; a termination mark resets when the container is observed alive again.
The reconciler evicted any cache entry whose container was absent from all published status lists once any statuses existed. A just-attached ephemeral container is exactly that: the pod spec already declares it while kubelet publishes its ephemeralContainerStatuses entry seconds later. The freshly-adopted profile entry was evicted on the next tick, nothing re-added it, and every ProfileDependency=Required rule was suppressed for the container's entire life — zero alerts of any class (issue #79 T5, ephemeral 0/N). Init-container entries created before the pod reached the k8s cache (empty PodUID, status ContainerID not yet published) hit the same branch, contributing to the init intermittency. Observed: ephemeral container adopted +1s after attach; reconciler tick 3s later logged entries_before=2 entries_after=1; zero alerts over the container's 75s life while the same pod alerted for its other containers. Fix: absence from published statuses only counts as reaped when the container is also absent from the pod SPEC (containers, initContainers, ephemeralContainers). Additionally, the termination mark introduced with the removal grace now resets when a marked container is observed alive again, so a later genuine termination gets a full grace window. Tests (red pre-fix): TestReconciler_KeepsEphemeralContainerAwaitingStatus, TestReconciler_KeepsInitContainerAwaitingStatusWithEmptyPodUID, TestReconciler_TerminationMarkResetsWhenContainerReappears; negative contract TestReconciler_EvictsContainerRemovedFromSpecAndStatus. Regression: go test -race ./pkg/objectcache/... ./pkg/containerwatcher/v2/ ./pkg/rulemanager/... passes.
…ile cache The ContainerProfileCache reconciler classified any cache entry whose container was absent from the pod's published status lists as reaped and evicted it (reconciler.go isContainerTerminated). But kubelet publishes the status groups incrementally: a just-attached ephemeral container has no ephemeralContainerStatuses entry for several seconds while it is already running and traced, and an entry added before the pod reached the k8s cache carries an empty PodUID, which made the (Name, PodUID) pre-running fallback unreachable for init containers. Eviction is permanent (no re-add path exists), so every ProfileDependency=Required rule (R0001/R0003/R0004) was silently suppressed for the container's whole life: total alert loss for ephemeral containers, intermittent exec-alert loss for init containers (issue #79, CI run 31846699597). Evidence (live rig, issue #79): ephemeral container adopted at +1s, evicted at the next reconciler tick +3s (entries 2->1), exec events verifiably reached ReportEnrichedEvent at +75s and were dropped by the Required-profile gate; the exec gadget's mntns filter map contained the container's mntns the whole time (kernel/tracer exonerated). Fix: - treat absence from the status lists as reaped only when the pod SPEC does not name the container either; a status entry with the same name under a different non-empty ContainerID still evicts (replaced instance) - allow the pre-running (Name, PodUID) fallback to match when the stored PodUID is empty - backfill PodUID from the container runtime metadata when the pod is not yet in the k8s cache at entry-build time New tests fail on the pre-fix code and pass with the fix: TestReconcilerKeepsJustAttachedEphemeralContainer, TestReconcilerKeepsInitContainerWithEmptyStoredPodUID. Regression guards (both-ways green): eviction after published termination, gone from spec+status, replaced instance. Full objectcache, rulemanager and containerprofilemanager suites pass unchanged.
…, match containerName exactly, race-free readiness wait The ladder under-counted to 0/N while the node-agent logs showed 5/5 R0001 for both the init and the ephemeral container: it read only one DaemonSet pod's logs (the workload can land on any node), its grep could not match the alert JSON's containerName field, and its readiness wait raced pod creation.
The gadget rework removed IsResolvedFullPath from path.go while datasource_event.go still guards the fname fallback with it, leaving the tree uncompilable. Restore the helper unchanged; whether the guard is still needed under kernel-side full-path resolution can be decided separately.
Supersedes the IsResolvedFullPath restoration one commit back: upstream kubescape#889 retires the helper entirely and replaces the call-site guard with a plain empty-fpath fallback (the patched gadget resolves full paths kernel-side, so the fragment hazard no longer exists). Take upstream's GetFullPath body, drop the helper again together with its fork-only test, leaving path.go and normalize_path_test.go byte-identical to upstream main.
The backward dentry walk in get_path_str terminates at the mount-tree root of the file's vfsmount. runc >= 1.2 accesses procfs during container init and exec through a private detached mount created via fsopen(2)/fsmount(2); such a mount has no mountpoint (mnt_parent == mnt), so the walk correctly reaches its top with no /proc dentry to prepend and emits prefix-stripped paths such as /1/task/1/fd. The kernel's own d_path reports the same rootless string, so no userspace consumer can recover the prefix afterwards. Detect this termination case by checking the superblock magic of the final dentry: if it is PROC_SUPER_MAGIC the walk ended inside a procfs instance that is the top of its own mount chain, which cannot be the real global root, and the path is canonicalized by prepending proc/. Attached procfs mounts are unaffected because their walk continues through mnt_mountpoint before reaching this branch, and non-procfs detached mounts keep their previous behavior. Verified on kernel 6.1.167 (x86_64): fsopen/fsmount reproducer emits /proc/1/task/1/fd (was /1/task/1/fd), detached procfs root open emits /proc, detached tmpfs root open remains /, regular file paths unchanged, program accepted by the verifier. Refs #81
Author
|
Correction to the CT summaries above: the matrix legs Test_29/31/37–47 (signature suite) are vacuous passes on this branch — the branch carries no signature tests or feature code, so those legs run zero tests ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the node-agent side of #75 (review follow-ups on the AP/NN -> ContainerProfile migration). Four commits, each independently reviewable. Pairs with k8sstormcenter/storage#41, pinned via
replaceto that branch's commit.UserAPRV/UserNNRVfast-skip, "fetches user-authored AP/NN CRDs", the AP/NN aggregation naming note); all updated to the authored-CP flow that actually runs. Restorestests/resources/network-wildcards/README.md, whose fixtures are still consumed bycontainerprofilenetwork/fixtures_test.gobut were left undocumented when the legacy README was deleted.*exec-path classification as Pattern. The classifier fix covered every path surface but only Opens had a pinning test; Execs flow through the same projection.container_profile_user_defined_adopted_total(prometheus) /node_agent.container_profile.user_defined_adopted.total(OTEL) across all four metrics managers.containers/initContainers/ephemeralContainerscontract of the legacy specs, and the multi-container component test only exercised two REGULAR containers. With the subtype groups restored in storage,resolveAuthoredContainerSectionselects each container's section by name across all three groups (flat documents pass through; a grouped document that does not cover a container never enforces a sibling's section). Wired into both the add path and the refresh path. Covered by unit tests (section selection, per-subtype adoption from one shared document, uncovered-container nil) and a new component testTest_37_MultiSubtypeGroupedProfileDocumentdriving a regular + init + runtime-attached ephemeral container (newTestWorkload.AddEphemeralContainerhelper).Test_36andTest_37are added to the component-tests matrix —Test_36was never listed, so it never ran in CI.Regression:
go test ./pkg/...— 53 packages ok; the two failing packages (containerwatcher/v2/tracers,validator) fail identically on cleanmirrormain(environment-dependent, pre-existing).Not addressed here (already resolved on
mirrormainbefore this branch): the entry-RV freeze, terminal-gate ordering, learned-profile validation, unresolved-label warning, per-container name binding, the restored refresh-path tests, and theug-overlay removal.