From 6bd3ddc81481da5dfecf86c4bef347a15621078c Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Wed, 2 Sep 2026 13:16:41 +0200 Subject: [PATCH] feat(objectcache): carry a content checksum for conditional container-profile fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands the client half of the conditional container-profile fetch contract. It is DORMANT: no in-tree ProfileClient implementer returns the sentinel, so no conditional fetch has been or can be observed here, and there is no behavior or byte-savings change. Green tests prove the contract compiles and that existing behavior is unchanged — nothing more. pkg/storage gains the shared vocabulary, deliberately out-of-band so ProfileClient's signature is byte-identical and its checksum-unaware in-cluster implementer changes by zero lines: a context key (WithKnownChecksum / KnownChecksumFromContext) for the request, and ErrProfileUnchanged plus ContainerProfileChecksumAnnotationKey for the response. The annotation key is a string contract shared with an out-of-tree implementer and fails silently, not loudly, if the two sides ever disagree. CachedContainerProfile gains Checksum, populated at BOTH construction sites. buildEntry matters as much as rebuildEntryFromSources: a profile that never changes is built once and thereafter always fast-skips, so populating only the rebuild path would leave the validator empty forever and make the optimization silently inert for exactly the steady-state population it targets. On the adoption path the value is corrected post-call from the pre-repoint learned CP, mirroring the existing entry.RV = learnedRV fix, so the validator always describes the object CPName points at. refreshOneEntry offers the validator only under a five-conjunct guard — no authored CP ref, no recorded authored RV, unchanged spec hash, a non-empty stored checksum, and a cached state that has already reached Completed+Full. The last conjunct is load-bearing: the lifecycle annotations sit outside the content checksum, so a profile finishing its learning period presents an unchanged checksum, and without it the entry would answer "unchanged" on that tick and on every later one — freezing entry.State permanently. That state is not internal bookkeeping; rulemanager gates HasFinalApplicationProfile on Completed+Full and stamps FailOnProfile from it, so a frozen state would keep alerting a finished profile as partial forever. Unlike the RV staleness below, that staleness would never self-correct. The validator is attached per call, never to the shared context, so the authored-CP fetch can never receive the learned CP's checksum. The projection spec is snapshotted once above the fetch and reused, which moves detection of a spec swap landing mid-fetch to the next tick; that is self-healing. One accepted, tested divergence remains: a checksum match proves content identity, not ResourceVersion identity, so on the sentinel path e.RV may lag a metadata-only write until the next unconditional fetch. That one is bounded and self-correcting, which is why it is accepted where the state freeze is not. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HnqMRD3r2kGYUBTxMHM5vi Signed-off-by: Matthias Bertschy --- ...iner-profile-conditional-fetch-contract.md | 114 +++ .../containerprofilecache.go | 19 + .../containerprofilecache/reconciler.go | 86 ++- .../reconciler_checksum_test.go | 721 ++++++++++++++++++ pkg/storage/checksum.go | 63 ++ 5 files changed, 997 insertions(+), 6 deletions(-) create mode 100644 docs/features/container-profile-conditional-fetch-contract.md create mode 100644 pkg/objectcache/containerprofilecache/reconciler_checksum_test.go create mode 100644 pkg/storage/checksum.go diff --git a/docs/features/container-profile-conditional-fetch-contract.md b/docs/features/container-profile-conditional-fetch-contract.md new file mode 100644 index 0000000000..7f14dc232f --- /dev/null +++ b/docs/features/container-profile-conditional-fetch-contract.md @@ -0,0 +1,114 @@ +# Container Profile Conditional-Fetch Contract + +`storage.ProfileClient` fetches a container's learned `ContainerProfile` by name: + +```go +GetContainerProfile(ctx context.Context, namespace, name string) (*v1beta1.ContainerProfile, error) +``` + +A remote implementation of this interface can often answer "the body you already have is still current" far more cheaply than it can re-stream an identical profile. That answer needs two things the signature above has no room for: a **validator** the caller sends, and a way to say **"unchanged"** instead of returning a body. + +This document describes the vocabulary node-agent exports so a remote implementer can do that, and the guard the reconciler applies before it ever asks. It covers only node-agent's half — the transport that carries the signal off-node is the implementer's concern. + +## Why the signal travels out-of-band + +`ProfileClient` has a second implementer with no concept of a remote checksum: the in-cluster CRD/aggregated-API client at `pkg/storage/v1/storage.go` (`var _ storage.ProfileClient = (*Storage)(nil)`). Adding a parameter or a return value for a capability only one implementer has would touch every implementer, every mock, and the six-odd test files that declare conformance. + +So the request travels on the `context.Context` and the response as a sentinel `error` plus an `ObjectMeta` annotation. The interface signature is unchanged, and the in-cluster implementer changes by **zero lines** — it never attaches a checksum, never returns the sentinel, and behaves exactly as before. + +A narrower optional interface discovered by type assertion was considered and rejected: a type assertion fails *silently* when an implementer drifts out of conformance (the fast path just quietly stops engaging), and it does not survive the wrapper layers this call already passes through. A context value and a wrapped error propagate through wrappers for free. + +## The exported vocabulary + +All four symbols live in `pkg/storage/checksum.go`, next to the interface they extend. That package imports no client library of any kind, which is what keeps `pkg/objectcache` free of transport dependencies. + +| Symbol | Type | Value / signature | +|---|---|---| +| `storage.ContainerProfileChecksumAnnotationKey` | `const string` | `"backend.kubescape.io/container-profile-checksum"` | +| `storage.ErrProfileUnchanged` | `var error` | `errors.New("container profile unchanged")` | +| `storage.WithKnownChecksum` | `func` | `(ctx context.Context, checksum string) context.Context` | +| `storage.KnownChecksumFromContext` | `func` | `(ctx context.Context) string` | + +The context key is an unexported `knownChecksumKey struct{}`, so no other package can collide with it or forge a value. + +### Request: `WithKnownChecksum` + +The reconciler attaches the checksum of the profile it already holds to the context of a single `GetContainerProfile` call. An implementer reads it with `KnownChecksumFromContext`; `""` (the value for a bare context) means **send the body unconditionally**. An implementer that ignores the value entirely is always correct — it just returns the body, as today. + +The checksum is attached **per call, never to a shared parent context**. It is a claim about one specific object, and `refreshOneEntry` fetches two different objects from contexts derived from the same parent. + +### Response: `ErrProfileUnchanged` + +An implementer that confirmed the caller's checksum still matches returns `(nil, ErrProfileUnchanged)` — no profile, because none was transferred. The reconciler matches it with `errors.Is`, so it may be wrapped. + +Returning this sentinel for a request that carried **no** checksum is a protocol violation: it claims a match against a validator the caller never supplied. Implementers must reject that case with a distinct, loud error rather than the sentinel, so a client cache can never be frozen on the basis of nothing. + +### Response: the checksum annotation + +On a **normal** fetch, an implementer stamps the profile's current checksum onto `ObjectMeta.Annotations` under `ContainerProfileChecksumAnnotationKey`. This is the only channel back through the unchanged signature, and it is what lets an entry acquire the validator it will offer on a later tick. Without it the whole mechanism is permanently inert. + +The key is namespaced under `backend.kubescape.io` specifically so it cannot collide with the learning-lifecycle annotations in `k8s-interface/instanceidhandler/v1/helpers` that this cache reads for status and completion (`StatusMetadataKey`, `CompletionMetadataKey`). + +### Cross-repo key agreement + +The annotation key is a **string contract between repositories**, and it fails silently rather than loudly if the two sides disagree — nothing errors, the cache simply never observes a checksum and every fetch stays unconditional. + +The remote implementer today is `armosec/private-node-agent`'s backend adapter (`pkg/backend/storage.go`), which wraps `kubescape/backend`'s `StorageClient`. That client stamps its own constant, `backendv1.ContainerProfileChecksumAnnotationKey`, whose value is the identical string `"backend.kubescape.io/container-profile-checksum"`. The adapter is responsible for translating the backend's vocabulary onto node-agent's — re-keying the annotation if the two ever diverge, and mapping the backend's own unchanged-sentinel onto `storage.ErrProfileUnchanged` so `errors.Is` matches here. + +Treat the value as frozen. Changing it on one side only is not a compile error. + +## Where the validator is stored + +`CachedContainerProfile.Checksum` (`pkg/objectcache/containerprofilecache/containerprofilecache.go`) holds the content checksum of the **learned** CP at last load, read via `checksumOfCP` (`reconciler.go`, mirroring `rvOfCP`). It is best-effort: empty whenever the source supplied no annotation, which is always the case for the in-cluster implementer. + +Two properties are easy to get wrong and are both covered by tests in `reconciler_checksum_test.go`: + +- **Both construction sites populate it.** `rebuildEntryFromSources` is the obvious one, but `buildEntry` (reached from `tryPopulateEntry` and the pending-promotion retry) matters more. A profile that never changes is built once by `buildEntry` and thereafter always returns at `refreshOneEntry`'s fast-skip, never reaching `rebuildEntryFromSources`. Populating only the rebuild path would leave `Checksum` empty forever for exactly the steady-state profiles this exists for — with every test still green. +- **It tracks the learned CP, never an adopted authored one.** On the adoption path `tryPopulateEntry` repoints `cp` at the authored profile *before* calling `buildEntry`, so the value is corrected after the call from a checksum captured beforehand — the same shape as the existing `entry.RV = learnedRV` correction, and for the same reason: the validator is offered back on a `GET` of the learned slug, so it must describe that object. + +## The five-conjunct guard + +`refreshOneEntry` does more than refresh the learned CP: it also re-fetches the user-authored CP, propagates projection-spec changes, and refreshes the entry's cached lifecycle state. A conditional fetch is only legitimate when the body is genuinely not needed for any of that. So the checksum is offered only when all five hold: + +```go +e.UserCPRef == nil && e.UserCPRV == "" && e.SpecHash == currentSpecHash && e.Checksum != "" && + e.State != nil && e.State.Status == helpersv1.Completed && e.State.Completion == helpersv1.Full +``` + +| Conjunct | Why | +|---|---| +| `e.UserCPRef == nil` | An authored CP is re-fetched and re-adopted this tick, so the body is needed regardless. | +| `e.UserCPRV == ""` | `UserCPRef == nil` alone does not establish the fast-skip's `rvsMatchCP(userDefinedCP, e.UserCPRV)`, which with no authored CP present reduces to `rvsMatchCP(nil, e.UserCPRV)` — true only for `""`. Without this, an entry in the `authoredJustDropped` shape (an authored RV on record but no authored CP any more) could skip that handling. | +| `e.SpecHash == currentSpecHash` | The projection spec moved, so the entry must be re-projected from a real body even if the content is identical. | +| `e.Checksum != ""` | Nothing to validate against. Sending `""` would mean "unconditional" anyway, and a server answering "unchanged" to it would be the protocol violation described above. | +| state is `Completed` + `Full` | The lifecycle annotations sit **outside** the content checksum, so a profile finishing its learning period presents an unchanged checksum. Without this conjunct the entry would answer "unchanged" on that tick *and every later one* — the checksum stays valid indefinitely — so the rebuild that refreshes `e.State` would never run and the cached state would freeze permanently. | + +Each conjunct has its own negative test asserting that no checksum is sent when it fails. + +### Why the state conjunct is not optional + +`entry.State` is not internal bookkeeping. `pkg/rulemanager/rule_manager.go`'s `HasFinalApplicationProfile` gates on `state.Status == helpersv1.Completed && state.Completion == helpersv1.Full`, and `pkg/rulemanager/ruleadapters/creator.go` stamps `FailOnProfile` and the reported profile status onto every alert built from it. A frozen state would make a finished profile keep alerting as partial forever. + +Note the asymmetry with `e.RV` below: RV staleness self-corrects at the next unconditional fetch, whereas a frozen state has no next unconditional fetch — the condition that caused it also perpetuates it. That is why one is accepted and the other is guarded against. + +The predicate is `Completed` + `Full` rather than `isTerminalCPStatus` (which also admits `TooLarge`) deliberately: it is the exact state rulemanager treats as final, and the only one from which no further lifecycle transition is expected. A `TooLarge` profile simply keeps fetching bodies — a lost optimization, not a correctness risk. + +`currentSpecHash` is snapshotted **once, above the fetch**, and reused for both the guard and the later fast-skip, so the two decisions cannot disagree within a tick. The trade-off: a projection-spec swap landing *during* the fetch is noticed on the next tick rather than this one. That is self-healing — the next tick's `e.SpecHash != currentSpecHash` comparison forces the rebuild regardless. + +## Handling the sentinel + +`refreshOneEntry` matches `errors.Is(cpErr, storage.ErrProfileUnchanged)` **before** the `apierrors.IsNotFound` check and returns with the cache entry left completely untouched. + +The explicit branch is load-bearing, not cosmetic. Without it, a sentinel that also carries a not-found shape would fall into the not-found path, which sets `cp = nil`, finds no authored CP either, and **evicts the entry**. A sentinel that does not carry a not-found shape would instead land in the generic transient-error path — which happens to keep the entry, but logs it as a fetch failure, making a successful optimization indistinguishable from a broken connection. + +### Accepted freshness divergence + +A checksum match proves the **content** is byte-identical. It does not prove `ResourceVersion` equality: RV can bump on a metadata-only write, and annotations are outside the content checksum. On a sentinel response the client never sees the new object, so **`e.RV` keeps its previous value**. + +This one is deliberate and accepted. Every consumer downstream of this cache reads the projected *content*; `e.RV` serves only as a change detector for the next tick, where a stale-but-lower RV is conservative — it can cause an extra rebuild, never a missed one. Including annotations in the checksum would defeat the optimization entirely, since the learning pipeline rewrites them continuously. The behavior is pinned by a test that asserts the staleness *positively*, so it reads as intended rather than as a defect waiting to be found. + +The cached `State` is a different matter and is **not** allowed to go stale: the guard's state conjunct keeps an entry on the unconditional path until its lifecycle has finished, precisely because that staleness would be permanent rather than bounded. See "Why the state conjunct is not optional" above. + +## Current status in this repo + +No in-tree `ProfileClient` implementer returns `ErrProfileUnchanged`, so within node-agent alone this is a contract with no observable behavior change: the guard evaluates, `e.Checksum` stays empty for the in-cluster client, and every fetch is unconditional exactly as before. The vocabulary exists so an out-of-tree implementer can opt in without any change to the interface or to the in-cluster client. diff --git a/pkg/objectcache/containerprofilecache/containerprofilecache.go b/pkg/objectcache/containerprofilecache/containerprofilecache.go index 2a5394d18e..6a5ef1d5f0 100644 --- a/pkg/objectcache/containerprofilecache/containerprofilecache.go +++ b/pkg/objectcache/containerprofilecache/containerprofilecache.go @@ -84,6 +84,15 @@ type CachedContainerProfile struct { RV string // ContainerProfile resourceVersion at last load UserCPRV string // user-defined ContainerProfile (label-referenced) RV at last load, "" if not used + // Checksum is the content checksum of the learned CP at last load, read + // from storage.ContainerProfileChecksumAnnotationKey. Best-effort: empty + // when the source supplies none, which is always the case for the + // in-cluster CRD-backed client. The reconciler offers it back to the source + // as a conditional-fetch validator; an empty value simply means every fetch + // is unconditional. Like RV, it tracks the LEARNED CP (the object CPName + // points at), never an adopted authored one. + Checksum string + // terminatedSeenAt is set by the reconciler the first time it observes the // container Terminated; eviction happens on a later tick once the removal // grace has elapsed. Accessed only from the reconciler goroutine. @@ -456,8 +465,13 @@ func (c *ContainerProfileCacheImpl) tryPopulateEntry( // transient error, freezing the entry so authored-CP edits are never picked // up (review finding on node-agent#864). learnedRV := "" + learnedChecksum := "" if cp != nil { learnedRV = cp.ResourceVersion + // Captured here for the same reason as learnedRV: entry.Checksum is a + // validator for the object entry.CPName points at (the learned slug), + // so it must be read before cp is repointed at the authored profile. + learnedChecksum = checksumOfCP(cp) } // A user-defined ContainerProfile is authoritative for this container: it is @@ -496,6 +510,10 @@ func (c *ContainerProfileCacheImpl) tryPopulateEntry( // (the learned slug), so leaving the authored RV here makes the permanent 404 // on that slug look transient and freezes the entry. Track the learned RV. entry.RV = learnedRV + // Same correction for the conditional-fetch validator: buildEntry derived it + // from the adopted (possibly authored) cp, but it is offered back on a GET of + // the learned slug, so it must describe the learned CP or nothing. + entry.Checksum = learnedChecksum // WorkloadName is the synthesize-name source refreshOneEntry uses when it // rebuilds an entry whose consolidated CP is not yet in storage. entry.WorkloadName = workloadName @@ -555,6 +573,7 @@ func (c *ContainerProfileCacheImpl) buildEntry( WorkloadID: sharedData.Wlid + "/" + sharedData.InstanceID.GetTemplateHash(), CPName: cp.Name, RV: cp.ResourceVersion, + Checksum: checksumOfCP(cp), } if pod != nil { entry.PodUID = string(pod.UID) diff --git a/pkg/objectcache/containerprofilecache/reconciler.go b/pkg/objectcache/containerprofilecache/reconciler.go index bd7517c728..3be11be991 100644 --- a/pkg/objectcache/containerprofilecache/reconciler.go +++ b/pkg/objectcache/containerprofilecache/reconciler.go @@ -18,6 +18,7 @@ package containerprofilecache import ( "context" + "errors" "time" "github.com/kubescape/go-logger" @@ -25,6 +26,7 @@ import ( helpersv1 "github.com/kubescape/k8s-interface/instanceidhandler/v1/helpers" "github.com/kubescape/node-agent/pkg/objectcache" "github.com/kubescape/node-agent/pkg/objectcache/callstackcache" + "github.com/kubescape/node-agent/pkg/storage" "github.com/kubescape/node-agent/pkg/utils" "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" corev1 "k8s.io/api/core/v1" @@ -331,6 +333,56 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri ns := e.Namespace + // Snapshot the projection spec ONCE, before the fetch, and reuse it for both + // the conditional-fetch guard below and the fast-skip further down. The + // guard needs it pre-fetch, and reading it twice could straddle a concurrent + // SetProjectionSpec and let the two decisions disagree within one tick. + // Consequence: a spec swap landing DURING the fetch is now noticed on the + // next tick rather than this one. That is self-healing — the next tick's + // e.SpecHash != currentSpecHash comparison forces the rebuild regardless. + currentSpecHash := "" + if spec := c.snapshotSpec(); spec != nil { + currentSpecHash = spec.Hash + } + + // Offer the stored checksum as a conditional-fetch validator only when an + // "unchanged" answer would have led to the fast-skip below anyway — i.e. + // when the body is genuinely not needed for anything else this tick: + // - UserCPRef == nil: no authored CP to re-fetch and re-adopt. + // - UserCPRV == "": no authored RV on record either. Without this, + // an entry in the authoredJustDropped shape (a recorded authored RV but + // no authored CP any more) could skip that handling. It mirrors + // rvsMatchCP(nil, e.UserCPRV) in the fast-skip, which is true only for "". + // - SpecHash == currentSpecHash: the projection would be identical. + // - Checksum != "": we actually hold a validator to offer. + // - State is already terminal: see below. + // + // The state conjunct is not redundant with the others. e.State is derived + // from the StatusMetadataKey/CompletionMetadataKey ANNOTATIONS, which sit + // outside the content checksum — so a lifecycle flip (partial -> full) + // leaves the checksum matching. Without this conjunct such an entry would + // answer "unchanged" on that tick AND on every later one (its checksum + // stays valid indefinitely), so the rebuild that refreshes e.State would + // never fire and the cached state would freeze permanently. That is not + // the bounded one-fetch staleness accepted for e.RV: it never self-corrects. + // It matters because e.State is not internal bookkeeping — rulemanager gates + // HasFinalApplicationProfile on Completed+Full and stamps FailOnProfile on + // every alert from it, so a frozen state keeps alerting a completed profile + // as partial forever. + // + // Requiring Completed+Full (rather than isTerminalCPStatus, which also + // admits TooLarge) is deliberate: it is the exact predicate rulemanager + // treats as final, and it is the only state from which no further lifecycle + // transition is expected. A TooLarge profile simply keeps fetching bodies. + // + // Attached per call, never to the shared ctx: the authored-CP fetch below + // derives from the same ctx and must never carry the learned CP's checksum. + cpCtx := ctx + if e.UserCPRef == nil && e.UserCPRV == "" && e.SpecHash == currentSpecHash && e.Checksum != "" && + e.State != nil && e.State.Status == helpersv1.Completed && e.State.Completion == helpersv1.Full { + cpCtx = storage.WithKnownChecksum(ctx, e.Checksum) + } + // Re-fetch all sources. CP fetch errors (including 404) are treated as // "not available right now" — mirroring tryPopulateEntry's behavior. We // leave cp=nil and rely on the RV-match fast-skip below to preserve the @@ -339,11 +391,24 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri // while the storage-side consolidated CP remains unpublished. var cp *v1beta1.ContainerProfile var cpErr error - _ = c.refreshRPC(ctx, func(rctx context.Context) error { + _ = c.refreshRPC(cpCtx, func(rctx context.Context) error { cp, cpErr = c.storageClient.GetContainerProfile(rctx, ns, e.CPName) return cpErr }) if cpErr != nil { + // Checked before IsNotFound: an implementation is free to return a + // sentinel that also carries a not-found shape, and "unchanged" is the + // more specific claim. The source verified our checksum still matches + // and sent no body, so the entry we hold is known-good — leave it + // entirely alone. Deliberately does NOT refresh e.RV or e.Checksum: + // there is no fresh object to read them from, so e.RV may lag a + // metadata-only write until the next unconditional fetch. + if errors.Is(cpErr, storage.ErrProfileUnchanged) { + logger.L().Debug("refreshOneEntry: CP unchanged (checksum match); keeping cached entry without rebuild", + helpers.String("containerID", id), + helpers.String("cpName", e.CPName)) + return + } if !apierrors.IsNotFound(cpErr) { logger.L().Debug("refreshOneEntry: CP fetch failed transiently; keeping cached entry", helpers.String("containerID", id), @@ -413,11 +478,9 @@ func (c *ContainerProfileCacheImpl) refreshOneEntry(ctx context.Context, id stri // this avoids spurious rebuilds when an optional source is still missing, // as long as it was also missing at the last build. Also skip when the // projection spec hash matches: if neither the data nor the spec changed, - // the projected output would be identical. - currentSpecHash := "" - if spec := c.snapshotSpec(); spec != nil { - currentSpecHash = spec.Hash - } + // the projected output would be identical. currentSpecHash is the value + // hoisted above the fetch — deliberately not re-read here, so the guard and + // this comparison cannot disagree within a tick. if rvsMatchCP(cp, e.RV) && rvsMatchCP(userDefinedCP, e.UserCPRV) && e.SpecHash == currentSpecHash { @@ -510,6 +573,7 @@ func (c *ContainerProfileCacheImpl) rebuildEntryFromSources( WorkloadName: prev.WorkloadName, RV: rvOfCP(cp), UserCPRV: rvOfCP(userDefinedCP), + Checksum: checksumOfCP(cp), terminatedSeenAt: prev.terminatedSeenAt, } if userDefinedCP != nil { @@ -541,6 +605,16 @@ func rvOfCP(o *v1beta1.ContainerProfile) string { return o.ResourceVersion } +// checksumOfCP returns the content checksum a ProfileClient stamped on the +// object, or "" when the object is absent or the source supplied none (the +// in-cluster CRD-backed client never does). Mirrors rvOfCP. +func checksumOfCP(o *v1beta1.ContainerProfile) string { + if o == nil { + return "" + } + return o.Annotations[storage.ContainerProfileChecksumAnnotationKey] +} + // observeMemoryMetrics records per-field entry counts, retention ratios, and // total byte sizes for the raw vs projected profile. Called only when // DetailedMetricsEnabled is true. diff --git a/pkg/objectcache/containerprofilecache/reconciler_checksum_test.go b/pkg/objectcache/containerprofilecache/reconciler_checksum_test.go new file mode 100644 index 0000000000..b79c1dc10b --- /dev/null +++ b/pkg/objectcache/containerprofilecache/reconciler_checksum_test.go @@ -0,0 +1,721 @@ +package containerprofilecache + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + + helpersv1 "github.com/kubescape/k8s-interface/instanceidhandler/v1/helpers" + "github.com/kubescape/node-agent/pkg/objectcache" + "github.com/kubescape/node-agent/pkg/storage" + "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +// recordedFetch is one observed GetContainerProfile call: the object name asked +// for, and the conditional-fetch validator the caller attached to that call's +// context (empty when none was attached). +type recordedFetch struct { + name string + checksum string +} + +// checksumRecordingClient is a storage.ProfileClient that records the known +// checksum carried by EACH call's context. Recording per call (rather than per +// client) is what makes the negative assertions possible: the authored-CP fetch +// and the learned-CP fetch derive from the same parent context, so a design that +// attached the checksum to that shared context instead of per call would show up +// here as the authored fetch seeing a non-empty value. +type checksumRecordingClient struct { + mu sync.Mutex + + // learned is served for any name not present in authored. learnedErr, when + // set, is returned instead (still after recording the call). + learned *v1beta1.ContainerProfile + learnedErr error + + // authored maps an object name to the authored CP served for it. Names in + // this map are never served the learned CP. + authored map[string]*v1beta1.ContainerProfile + + fetches []recordedFetch +} + +var _ storage.ProfileClient = (*checksumRecordingClient)(nil) + +func (c *checksumRecordingClient) GetContainerProfile(ctx context.Context, _, name string) (*v1beta1.ContainerProfile, error) { + c.mu.Lock() + c.fetches = append(c.fetches, recordedFetch{name: name, checksum: storage.KnownChecksumFromContext(ctx)}) + authored, isAuthored := c.authored[name] + c.mu.Unlock() + + if isAuthored { + return authored, nil + } + if c.learnedErr != nil { + return nil, c.learnedErr + } + if c.learned != nil { + return c.learned, nil + } + return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "containerprofiles"}, name) +} + +// checksumsFor returns the validators observed on every call for `name`, in +// call order. A name never fetched yields an empty slice. +func (c *checksumRecordingClient) checksumsFor(name string) []string { + c.mu.Lock() + defer c.mu.Unlock() + var out []string + for _, f := range c.fetches { + if f.name == name { + out = append(out, f.checksum) + } + } + return out +} + +func (c *checksumRecordingClient) fetchCount(name string) int { + return len(c.checksumsFor(name)) +} + +// assertNoChecksumOnAuthoredFetches is the standing per-call-site invariant: no +// fetch of a name registered as an authored CP may ever carry a validator. The +// learned CP's checksum describes the learned object only; offering it on an +// authored fetch would let a source answer "unchanged" for the wrong object. +func (c *checksumRecordingClient) assertNoChecksumOnAuthoredFetches(t *testing.T) { + t.Helper() + c.mu.Lock() + defer c.mu.Unlock() + for _, f := range c.fetches { + if _, isAuthored := c.authored[f.name]; isAuthored { + assert.Empty(t, f.checksum, "authored-CP fetch of %q must never carry a known checksum", f.name) + } + } +} + +// learnedCPWithChecksum builds a terminal-status learned ContainerProfile +// carrying `checksum` under the cross-repo checksum annotation key. A checksum +// of "" produces a profile with no checksum annotation at all (the shape the +// in-cluster CRD-backed client always returns). +func learnedCPWithChecksum(name, rv, checksum string) *v1beta1.ContainerProfile { + annotations := map[string]string{ + helpersv1.StatusMetadataKey: helpersv1.Completed, + helpersv1.CompletionMetadataKey: helpersv1.Full, + } + if checksum != "" { + annotations[storage.ContainerProfileChecksumAnnotationKey] = checksum + } + return &v1beta1.ContainerProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: "default", ResourceVersion: rv, + Annotations: annotations, + }, + Spec: v1beta1.ContainerProfileSpec{Execs: []v1beta1.ExecCalls{{Path: "/bin/learned"}}}, + } +} + +// authoredCPWithChecksum is authoredCP plus a checksum annotation, used to prove +// the adoption path never adopts the AUTHORED object's checksum as the entry's +// validator. +func authoredCPWithChecksum(name, execPath, rv, checksum string) *v1beta1.ContainerProfile { + cp := authoredCP(name, execPath, rv) + cp.Annotations[storage.ContainerProfileChecksumAnnotationKey] = checksum + return cp +} + +// seedChecksumEntry installs a cache entry directly, bypassing addContainer, so +// each conjunct of the conditional-fetch guard can be varied independently. +func seedChecksumEntry(c *ContainerProfileCacheImpl, id string, cp *v1beta1.ContainerProfile, checksum, specHash string) *CachedContainerProfile { + e := &CachedContainerProfile{ + Projected: Apply(nil, cp, nil), + // Mirrors what both real construction sites store, so a test can tell + // whether the cached State was re-derived from a fresh body. + State: &objectcache.ProfileState{ + Name: cp.Name, + Status: cp.Annotations[helpersv1.StatusMetadataKey], + Completion: cp.Annotations[helpersv1.CompletionMetadataKey], + }, + ContainerName: "nginx", + PodName: "nginx-abc", + Namespace: "default", + PodUID: "uid-1", + CPName: cp.Name, + RV: cp.ResourceVersion, + Checksum: checksum, + SpecHash: specHash, + } + c.entries.Set(id, e) + return e +} + +// --------------------------------------------------------------------------- +// D4 — anti-inertness: the population path must store a validator +// --------------------------------------------------------------------------- + +// TestPopulatePathStoresChecksum_AntiInertness is the test that proves the +// optimization is not dead code. +// +// A profile that never changes is built ONCE by tryPopulateEntry/buildEntry and +// thereafter always returns at refreshOneEntry's fast-skip, so it never reaches +// rebuildEntryFromSources. If only rebuildEntryFromSources populated Checksum, +// such an entry would hold "" forever, the guard's `e.Checksum != ""` conjunct +// would never hold, and no conditional fetch would EVER be requested for exactly +// the steady-state population this work targets — while every test stayed green. +// +// This test therefore asserts both halves: (a) the entry carries a validator +// immediately after population, and (b) the very next reconcile tick actually +// offers it. It fails if buildEntry's Checksum population is reverted. +func TestPopulatePathStoresChecksum_AntiInertness(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-learned") + client := &checksumRecordingClient{learned: learned} + c, k8s := newTestCache(t, client) + + id := "cid-antiinert" + primeSharedData(t, k8s, id, "wlid://cluster-a/namespace-default/deployment-nginx") + require.NoError(t, c.addContainer(eventContainer(id), context.Background())) + + entry, ok := c.entries.Load(id) + require.True(t, ok, "container must be promoted out of pending") + require.Equal(t, 0, c.pending.Len()) + + // (a) populated without any rebuild ever having run. + assert.Equal(t, "sum-learned", entry.Checksum, + "an entry built only via tryPopulateEntry/buildEntry must carry the learned CP's checksum") + + // (b) eligible on the next tick: unchanged profile, unchanged spec. + client.mu.Lock() + client.fetches = nil + client.mu.Unlock() + + c.refreshAllEntries(context.Background()) + + sent := client.checksumsFor(entry.CPName) + require.Len(t, sent, 1, "exactly one learned-CP fetch on the tick") + assert.Equal(t, "sum-learned", sent[0], + "the tick after population must offer the stored validator; an empty value here means the optimization is inert") + + // Nothing changed, so the entry must have been fast-skipped, not rebuilt. + after, ok := c.entries.Load(id) + require.True(t, ok) + assert.Same(t, entry, after, "an unchanged profile must fast-skip, not rebuild") +} + +// TestPopulatePathStoresNoChecksumWhenSourceSuppliesNone pins the best-effort +// contract: a source that stamps no annotation (every in-cluster CRD client) +// leaves the validator empty, and the guard then declines to request a +// conditional fetch rather than sending "". +func TestPopulatePathStoresNoChecksumWhenSourceSuppliesNone(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "") + client := &checksumRecordingClient{learned: learned} + c, k8s := newTestCache(t, client) + + id := "cid-nosum" + primeSharedData(t, k8s, id, "wlid://cluster-a/namespace-default/deployment-nginx") + require.NoError(t, c.addContainer(eventContainer(id), context.Background())) + + entry, ok := c.entries.Load(id) + require.True(t, ok) + assert.Empty(t, entry.Checksum, "no annotation on the object means no stored validator") + + client.mu.Lock() + client.fetches = nil + client.mu.Unlock() + c.refreshAllEntries(context.Background()) + + for _, sum := range client.checksumsFor(entry.CPName) { + assert.Empty(t, sum, "no validator held means none offered") + } +} + +// TestAdoptionPathStoresLearnedChecksumNotAuthored covers the correction that +// mirrors the existing entry.RV = learnedRV fix. On the adoption path +// tryPopulateEntry repoints cp at the authored profile BEFORE calling +// buildEntry, so buildEntry's literal sees the authored object. entry.Checksum +// is offered back on a GET of the LEARNED slug, so adopting the authored +// object's checksum would plant a validator describing the wrong object. +func TestAdoptionPathStoresLearnedChecksumNotAuthored(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-learned") + authored := authoredCPWithChecksum("authored-cp-nginx", "/bin/authored", "a1", "sum-authored") + client := &checksumRecordingClient{ + learned: learned, + authored: map[string]*v1beta1.ContainerProfile{"authored-cp-nginx": authored}, + } + c, k8s := newTestCache(t, client) + + id := "cid-adopt" + primeSharedData(t, k8s, id, "wlid://cluster-a/namespace-default/deployment-nginx") + ev := eventContainer(id) + ev.K8s.PodLabels = map[string]string{helpersv1.UserDefinedProfileMetadataKey: "authored-cp"} + require.NoError(t, c.addContainer(ev, context.Background())) + + entry, ok := c.entries.Load(id) + require.True(t, ok) + require.NotNil(t, entry.UserCPRef, "the authored CP must have been adopted for this test to mean anything") + assert.Equal(t, "a1", entry.UserCPRV) + assert.Equal(t, "sum-learned", entry.Checksum, + "the validator must describe the LEARNED CP (the object CPName points at), never the adopted authored one") + assert.Equal(t, "1", entry.RV, "the existing learned-RV correction still holds") +} + +// TestAdoptionPathWithNoLearnedCPStoresEmptyChecksum is the same correction in +// its other shape: learning is suppressed for user-defined containers, so there +// is often no learned CP at all. The validator must then be empty rather than +// falling back to the authored object's. +func TestAdoptionPathWithNoLearnedCPStoresEmptyChecksum(t *testing.T) { + authored := authoredCPWithChecksum("authored-cp-nginx", "/bin/authored", "a1", "sum-authored") + client := &checksumRecordingClient{ + learnedErr: apierrors.NewNotFound(schema.GroupResource{Resource: "containerprofiles"}, "learned"), + authored: map[string]*v1beta1.ContainerProfile{"authored-cp-nginx": authored}, + } + c, k8s := newTestCache(t, client) + + id := "cid-adopt-nolearned" + primeSharedData(t, k8s, id, "wlid://cluster-a/namespace-default/deployment-nginx") + ev := eventContainer(id) + ev.K8s.PodLabels = map[string]string{helpersv1.UserDefinedProfileMetadataKey: "authored-cp"} + require.NoError(t, c.addContainer(ev, context.Background())) + + entry, ok := c.entries.Load(id) + require.True(t, ok) + require.NotNil(t, entry.UserCPRef) + assert.Empty(t, entry.Checksum, "no learned CP means no learned validator") + assert.Empty(t, entry.RV, "matches the existing learned-RV invariant") +} + +// --------------------------------------------------------------------------- +// The guard — each conjunct must independently suppress the offer +// --------------------------------------------------------------------------- + +// TestGuardOffersChecksumWhenAllConjunctsHold is the positive control the four +// negative tests below are read against. +func TestGuardOffersChecksumWhenAllConjunctsHold(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + seedChecksumEntry(c, "cid", learned, "sum-1", "") + + c.refreshAllEntries(context.Background()) + + assert.Equal(t, []string{"sum-1"}, client.checksumsFor("learned-cp")) +} + +// TestGuardDeclinesWhenAuthoredCPPresent — UserCPRef != nil. The body is needed +// regardless (the authored CP is re-fetched and re-adopted this tick), so no +// conditional fetch is requested even though a validator is held. The authored +// CP must still be refreshed. +func TestGuardDeclinesWhenAuthoredCPPresent(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + authored := authoredCP("authored-cp", "/bin/authored", "a1") + client := &checksumRecordingClient{ + learned: learned, + authored: map[string]*v1beta1.ContainerProfile{"authored-cp": authored}, + } + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + e := seedChecksumEntry(c, "cid", learned, "sum-1", "") + e.UserCPRef = &namespacedName{Namespace: "default", Name: "authored-cp"} + + c.refreshAllEntries(context.Background()) + + for _, sum := range client.checksumsFor("learned-cp") { + assert.Empty(t, sum, "an entry with an authored CP needs the body anyway; it must not ask for a conditional fetch") + } + assert.Equal(t, 1, client.fetchCount("authored-cp"), "the authored CP must still be refreshed on this tick") + client.assertNoChecksumOnAuthoredFetches(t) +} + +// TestGuardDeclinesWhenAuthoredRVRecordedButNoAuthoredCP — UserCPRV != "" with +// UserCPRef == nil, the authoredJustDropped shape. UserCPRef == nil alone does +// NOT establish the fast-skip's rvsMatchCP(userDefinedCP, e.UserCPRV) conjunct: +// with no authored CP present that call reduces to rvsMatchCP(nil, e.UserCPRV), +// which is true only for "". Without this conjunct such an entry could take the +// unchanged path and skip its authoredJustDropped handling. +func TestGuardDeclinesWhenAuthoredRVRecordedButNoAuthoredCP(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + e := seedChecksumEntry(c, "cid", learned, "sum-1", "") + e.UserCPRV = "a1" // recorded from a previous tick; UserCPRef is nil now + + c.refreshAllEntries(context.Background()) + + for _, sum := range client.checksumsFor("learned-cp") { + assert.Empty(t, sum, "the authoredJustDropped shape must not request a conditional fetch") + } +} + +// TestGuardDeclinesWhenSpecHashChanged — the projection spec moved, so the entry +// must be re-projected from a real body regardless of whether the content +// changed. No validator is offered, and the rebuild still happens. +func TestGuardDeclinesWhenSpecHashChanged(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + c.SetProjectionSpec(execsAllSpec("spec-v2")) + seedChecksumEntry(c, "cid", learned, "sum-1", "spec-v1") + + c.refreshAllEntries(context.Background()) + + for _, sum := range client.checksumsFor("learned-cp") { + assert.Empty(t, sum, "a changed projection spec needs the body; no conditional fetch") + } + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Equal(t, "spec-v2", after.SpecHash, "the rebuild must still happen and adopt the new spec") +} + +// TestGuardDeclinesWhenNoStoredChecksum — nothing to validate against. An empty +// value must not be sent: to a source, "" means "send unconditionally", and +// answering "unchanged" to it would be a protocol violation. +func TestGuardDeclinesWhenNoStoredChecksum(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + seedChecksumEntry(c, "cid", learned, "", "") + + c.refreshAllEntries(context.Background()) + + assert.Equal(t, []string{""}, client.checksumsFor("learned-cp")) +} + +// TestChecksumIsAttachedPerCallSiteNotToSharedContext proves R2's defense. +// +// Both GetContainerProfile call sites in refreshOneEntry derive from the same +// parent context; attaching the validator to that context instead of to the +// learned call would send the learned CP's checksum on the authored CP's fetch, +// and a source could legitimately answer "unchanged" for the wrong object. +// +// The guard makes the literal "same entry, both call sites, one +// carrying a checksum" shape unreachable — an entry with an authored CP never +// offers a validator at all. The reachable equivalent is asserted instead: two +// entries refreshed in ONE tick, one offering a validator and one performing an +// authored fetch. If the validator lived on shared state rather than a per-call +// context, it would leak onto the authored fetch here. +func TestChecksumIsAttachedPerCallSiteNotToSharedContext(t *testing.T) { + learnedA := learnedCPWithChecksum("learned-a", "1", "sum-a") + learnedB := learnedCPWithChecksum("learned-b", "1", "sum-b") + authored := authoredCP("authored-b", "/bin/authored", "a1") + client := &checksumRecordingClient{ + learned: learnedA, // served for any non-authored name, including learned-b + authored: map[string]*v1beta1.ContainerProfile{"authored-b": authored}, + } + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + + seedChecksumEntry(c, "cid-a", learnedA, "sum-a", "") // guard passes + eB := seedChecksumEntry(c, "cid-b", learnedB, "sum-b", "") + eB.UserCPRef = &namespacedName{Namespace: "default", Name: "authored-b"} // guard fails + + c.refreshAllEntries(context.Background()) + + assert.Equal(t, []string{"sum-a"}, client.checksumsFor("learned-a"), + "the guard-passing entry's learned fetch carries its own validator") + for _, sum := range client.checksumsFor("learned-b") { + assert.Empty(t, sum, "the authored-CP entry's learned fetch must carry nothing") + } + require.Equal(t, 1, client.fetchCount("authored-b")) + client.assertNoChecksumOnAuthoredFetches(t) +} + +// conditionalChecksumClient behaves like a real conditional source: when the +// caller offers a validator equal to the profile's CURRENT content checksum it +// answers ErrProfileUnchanged and sends no body; otherwise it returns the +// profile. This is what makes the state-freeze regression observable — a +// recording-only client would hand back a body regardless and hide it. +type conditionalChecksumClient struct { + mu sync.Mutex + cp *v1beta1.ContainerProfile + offered []string + unchange int +} + +var _ storage.ProfileClient = (*conditionalChecksumClient)(nil) + +func (c *conditionalChecksumClient) GetContainerProfile(ctx context.Context, _, _ string) (*v1beta1.ContainerProfile, error) { + c.mu.Lock() + defer c.mu.Unlock() + offered := storage.KnownChecksumFromContext(ctx) + c.offered = append(c.offered, offered) + if offered != "" && offered == c.cp.Annotations[storage.ContainerProfileChecksumAnnotationKey] { + c.unchange++ + return nil, storage.ErrProfileUnchanged + } + return c.cp, nil +} + +func (c *conditionalChecksumClient) lastOffered() string { + c.mu.Lock() + defer c.mu.Unlock() + if len(c.offered) == 0 { + return "" + } + return c.offered[len(c.offered)-1] +} + +// TestGuardDeclinesWhileStateNotYetTerminal is the regression test for the +// cached-state freeze. +// +// e.State comes from the status/completion ANNOTATIONS, which sit outside the +// content checksum. So a profile finishing its learning period — partial -> +// full, byte-identical content — presents a checksum that still matches. If the +// guard ignored the cached state, that tick would be answered "unchanged", the +// rebuild that refreshes e.State would never run, and because the checksum stays +// valid the SAME thing would happen on every later tick: the state freezes at +// partial permanently, and rulemanager keeps reporting a completed profile as +// incomplete. +// +// The walkthrough below covers all three phases: declined while learning, +// state correctly picked up when it terminalizes, and the shortcut engaging +// afterwards so the steady-state win is genuinely preserved. +func TestGuardDeclinesWhileStateNotYetTerminal(t *testing.T) { + // A profile still learning: non-terminal state, but a validator already + // stored from a previous fetch. + learning := &v1beta1.ContainerProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: "learned-cp", Namespace: "default", ResourceVersion: "1", + Annotations: map[string]string{ + helpersv1.StatusMetadataKey: helpersv1.Completed, + helpersv1.CompletionMetadataKey: helpersv1.Partial, + storage.ContainerProfileChecksumAnnotationKey: "sum-1", + }, + }, + Spec: v1beta1.ContainerProfileSpec{Execs: []v1beta1.ExecCalls{{Path: "/bin/learned"}}}, + } + client := &conditionalChecksumClient{cp: learning} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + seedChecksumEntry(c, "cid", learning, "sum-1", "") + + // Phase 1 — still partial: no validator offered, so a body is fetched. + c.refreshAllEntries(context.Background()) + assert.Empty(t, client.lastOffered(), "a non-terminal cached state must not take the conditional shortcut") + assert.Zero(t, client.unchange) + + // Phase 2 — the learning period completes. This is a METADATA-ONLY write: + // the RV moves and the completion flips, but the content checksum is + // unchanged, which is exactly what makes the freeze possible. + learning.ResourceVersion = "2" + learning.Annotations[helpersv1.CompletionMetadataKey] = helpersv1.Full + + c.refreshAllEntries(context.Background()) + assert.Empty(t, client.lastOffered(), "the cached state is still partial at guard time; the body is still needed") + + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Equal(t, helpersv1.Full, after.State.Completion, + "the completion flip MUST reach the cache; freezing here is what rulemanager would report as a permanently-partial profile") + assert.Equal(t, helpersv1.Completed, after.State.Status) + + // Phase 3 — now genuinely terminal, so the shortcut engages and the source + // gets to answer "unchanged". The optimization is preserved, not disabled. + c.refreshAllEntries(context.Background()) + assert.Equal(t, "sum-1", client.lastOffered(), "a Completed+Full entry must still use the conditional shortcut") + assert.Equal(t, 1, client.unchange, "the source answered unchanged exactly once, on the terminal tick") + + final, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Same(t, after, final, "the unchanged answer keeps the entry as-is") +} + +// TestGuardDeclinesForTooLargeState pins the deliberate narrowness of the state +// conjunct: TooLarge is terminal for the learned-status gate, but it is not the +// Completed+Full predicate rulemanager treats as final, so such an entry keeps +// fetching bodies rather than risking a frozen state. +func TestGuardDeclinesForTooLargeState(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + learned.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge + learned.Annotations[helpersv1.CompletionMetadataKey] = helpersv1.Partial + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + seedChecksumEntry(c, "cid", learned, "sum-1", "") + + c.refreshAllEntries(context.Background()) + + for _, sum := range client.checksumsFor("learned-cp") { + assert.Empty(t, sum, "a TooLarge/partial entry must not take the conditional shortcut") + } +} + +// --------------------------------------------------------------------------- +// Sentinel handling +// --------------------------------------------------------------------------- + +// TestSentinelKeepsEntryPointerIdentical — on ErrProfileUnchanged the entry the +// caller already holds is known-good, so refreshOneEntry must leave it entirely +// alone. Identity, not equality, is the assertion that discriminates: +// rebuildEntryFromSources always constructs a FRESH literal, so an equal-but-new +// pointer would mean a rebuild happened. +func TestSentinelKeepsEntryPointerIdentical(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learnedErr: storage.ErrProfileUnchanged} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + before := seedChecksumEntry(c, "cid", learned, "sum-1", "") + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok, "the entry must not be evicted") + assert.Same(t, before, after, "the sentinel must not rebuild the entry") + assert.Equal(t, "sum-1", after.Checksum, "the stored validator is left untouched") + assert.Equal(t, []string{"sum-1"}, client.checksumsFor("learned-cp"), + "the sentinel is only legitimate in reply to a request that carried a validator") +} + +// TestSentinelLeavesResourceVersionIntentionallyStale pins the one accepted +// divergence of the conditional path from the body-fetching path. +// +// A checksum match proves the CONTENT is byte-identical; it says nothing about +// ResourceVersion. RV can bump on a metadata-only write (e.g. a status-annotation +// flip), and annotations are outside the content checksum — so on a sentinel +// response the client never sees the new RV and e.RV stays at its old value. +// +// This is deliberate, not a bug: every consumer downstream of this cache reads +// the projected CONTENT, and e.RV only serves as a change detector for the next +// tick, where a stale-but-lower RV is conservative (it can cause an extra +// rebuild, never a missed one). Asserted POSITIVELY so the behavior is pinned as +// intended rather than rediscovered later as a defect. +func TestSentinelLeavesResourceVersionIntentionallyStale(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learnedErr: storage.ErrProfileUnchanged} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + before := seedChecksumEntry(c, "cid", learned, "sum-1", "") + + // Simulate a metadata-only write on the server: the object's RV moved but + // its content checksum did not, so the source still answers "unchanged". + learned.ResourceVersion = "2" + learned.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Same(t, before, after) + assert.Equal(t, "1", after.RV, + "e.RV is INTENDED to lag a metadata-only write until the next unconditional fetch") + assert.Equal(t, helpersv1.Completed, after.State.Status, + "the learned-status gate is likewise not re-evaluated on the sentinel path") +} + +// TestSentinelIsNotSwallowedByTheNotFoundPath pins that the sentinel is handled +// EXPLICITLY rather than left to the pre-existing error handling (D3). +// +// Scope, stated precisely, because it is narrower than "the branch is first": +// verified by deliberately breaking the code, this test fails when the sentinel +// branch is absent (the error's not-found shape then sets cp = nil, no authored +// CP is found either, and the entry is EVICTED), and it passes with the branch +// present. It does NOT distinguish the branch sitting before the IsNotFound +// check from it sitting just after — both return early with the entry intact. +// Ordering is kept as written for clarity, not because this test enforces it. +// +// Note also that a bare storage.ErrProfileUnchanged would NOT be caught by this +// test's mechanism at all: it falls into the generic transient-error path, which +// also keeps the entry, so the two are indistinguishable by behavior alone (they +// differ only in the log line emitted). That is exactly why the fixture below is +// built to satisfy the not-found predicate too — it is the one shape where the +// missing branch has a visible consequence. +func TestSentinelIsNotSwallowedByTheNotFoundPath(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + both := fmt.Errorf("%w: %w", + storage.ErrProfileUnchanged, + apierrors.NewNotFound(schema.GroupResource{Resource: "containerprofiles"}, "learned-cp")) + require.True(t, errors.Is(both, storage.ErrProfileUnchanged), "fixture must satisfy the sentinel predicate") + require.True(t, apierrors.IsNotFound(both), "fixture must also satisfy the not-found predicate") + + client := &checksumRecordingClient{learnedErr: both} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + before := seedChecksumEntry(c, "cid", learned, "sum-1", "") + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok, "not-found won the ordering: the entry was evicted instead of kept") + assert.Same(t, before, after) +} + +// TestNonSentinelErrorIsNotMistakenForUnchanged — an unrelated transport failure +// must fall through to the existing transient-error handling, which also keeps +// the entry. Guards against a too-broad match (e.g. a string comparison). +func TestNonSentinelErrorIsNotMistakenForUnchanged(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learnedErr: errors.New("container profile unchanged-ish: connection reset")} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + before := seedChecksumEntry(c, "cid", learned, "sum-1", "") + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Same(t, before, after, "a transient error also keeps the entry, by the pre-existing path") +} + +// --------------------------------------------------------------------------- +// Rebuild path +// --------------------------------------------------------------------------- + +// TestRebuildRefreshesStoredChecksum — a genuine content change must roll the +// stored validator forward, or the next tick would offer a checksum describing +// the previous body. +func TestRebuildRefreshesStoredChecksum(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-1") + client := &checksumRecordingClient{learned: learned} + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + seedChecksumEntry(c, "cid", learned, "sum-1", "") + + // Content changed on the server: new RV, new checksum, new body. + learned.ResourceVersion = "2" + learned.Annotations[storage.ContainerProfileChecksumAnnotationKey] = "sum-2" + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.Equal(t, "sum-2", after.Checksum, "a rebuild must adopt the fresh validator") + assert.Equal(t, "2", after.RV) +} + +// TestRebuildStoresLearnedChecksumNotAuthored — the rebuild path's counterpart +// to the adoption-path correction: Checksum tracks the learned CP even when an +// authored CP replaces it as the projection base. +func TestRebuildStoresLearnedChecksumNotAuthored(t *testing.T) { + learned := learnedCPWithChecksum("learned-cp", "1", "sum-learned") + authored := authoredCPWithChecksum("authored-cp", "/bin/authored", "a1", "sum-authored") + client := &checksumRecordingClient{ + learned: learned, + authored: map[string]*v1beta1.ContainerProfile{"authored-cp": authored}, + } + c := newReconcilerCache(t, client, newControllableK8sCache(), newCountingMetrics()) + e := seedChecksumEntry(c, "cid", learned, "", "") + e.UserCPRef = &namespacedName{Namespace: "default", Name: "authored-cp"} + + c.refreshAllEntries(context.Background()) + + after, ok := c.entries.Load("cid") + require.True(t, ok) + assert.NotSame(t, e, after, "an authored CP appearing must rebuild the entry") + assert.Equal(t, "sum-learned", after.Checksum, + "the validator tracks the learned CP even when an authored CP is the projection base") +} + +// --------------------------------------------------------------------------- +// Context vocabulary +// --------------------------------------------------------------------------- + +func TestKnownChecksumContextRoundTrip(t *testing.T) { + assert.Empty(t, storage.KnownChecksumFromContext(context.Background()), + "a bare context must report no validator, never panic") + + ctx := storage.WithKnownChecksum(context.Background(), "sum-1") + assert.Equal(t, "sum-1", storage.KnownChecksumFromContext(ctx)) + + // The parent is not mutated: attaching per call is what keeps the authored + // fetch clean. + assert.Empty(t, storage.KnownChecksumFromContext(context.Background())) +} diff --git a/pkg/storage/checksum.go b/pkg/storage/checksum.go new file mode 100644 index 0000000000..c34a7e03d4 --- /dev/null +++ b/pkg/storage/checksum.go @@ -0,0 +1,63 @@ +package storage + +import ( + "context" + "errors" +) + +// ContainerProfileChecksumAnnotationKey is the ObjectMeta annotation under which +// a fetched ContainerProfile carries the content checksum of its body. +// +// CROSS-REPO CONTRACT — this exact string is part of an interface between +// repositories. A ProfileClient implementation that talks to a remote storage +// backend (today: armosec/private-node-agent's pkg/backend adapter) is +// responsible for re-keying whatever its own transport calls the checksum onto +// THIS key before returning the profile. The container-profile cache reads the +// validator from here and nowhere else. +// +// Changing this value fails silently rather than loudly: the cache simply never +// observes a checksum, every entry keeps an empty validator, and every fetch +// degrades to an unconditional one. Nothing breaks; the optimization just stops +// existing. Treat it as frozen. +// +// The key is deliberately namespaced under backend.kubescape.io so it cannot +// collide with the learning-lifecycle annotations in +// k8s-interface/instanceidhandler/v1/helpers, which the cache reads for status +// and completion. +const ContainerProfileChecksumAnnotationKey = "backend.kubescape.io/container-profile-checksum" + +// ErrProfileUnchanged is returned by a ProfileClient implementation when the +// caller supplied a known checksum via WithKnownChecksum and the source +// confirmed the profile's content is byte-identical, so no body was +// transferred. There is no profile to return: the caller must keep the one it +// already holds. +// +// Implementations that cannot answer conditionally (for example the in-cluster +// CRD-backed client) never return this and need no knowledge of it. +var ErrProfileUnchanged = errors.New("container profile unchanged") + +// knownChecksumKey is the unexported context key type for the known checksum, +// so no other package can collide with or forge the value. +type knownChecksumKey struct{} + +// WithKnownChecksum returns a context carrying the content checksum of the +// ContainerProfile the caller already holds, as a hint that the body may be +// omitted if it still matches. +// +// It travels on the context rather than as a parameter because ProfileClient's +// signature must stay stable for its checksum-unaware implementers. A client +// that does not support conditional fetches ignores it and returns the body as +// usual, so attaching it is always safe. +// +// Attach it per call, never to a context shared by fetches of different +// objects: a checksum is a claim about one specific profile. +func WithKnownChecksum(ctx context.Context, checksum string) context.Context { + return context.WithValue(ctx, knownChecksumKey{}, checksum) +} + +// KnownChecksumFromContext returns the checksum attached by WithKnownChecksum, +// or "" when none was attached. "" means "send the body unconditionally". +func KnownChecksumFromContext(ctx context.Context) string { + checksum, _ := ctx.Value(knownChecksumKey{}).(string) + return checksum +}