feat(objectcache): carry a content checksum for conditional container-profile fetches - #941
Open
matthyx wants to merge 1 commit into
Open
feat(objectcache): carry a content checksum for conditional container-profile fetches#941matthyx wants to merge 1 commit into
matthyx wants to merge 1 commit into
Conversation
…-profile fetches 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HnqMRD3r2kGYUBTxMHM5vi Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
added
the
ai-assisted
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a checksum contract for conditional container-profile fetches. The cache stores the learned profile checksum, refresh logic sends it under defined conditions, and unchanged responses preserve the cached entry. Tests cover the contract and refresh behavior. Documentation describes the protocol. ChangesContainer profile conditional fetch
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The cache can now skip rebuilding when profile content is unchanged, but a status-only transition could leave lifecycle metadata stale and affect downstream rule or alert decisions once a checksum-aware client is deployed. The PR is mergeable with explicit owner awareness and follow-up before activating the downstream integration. Sequence Diagram(s)sequenceDiagram
participant CacheReconciler
participant ProfileClient
participant ContainerProfileCache
CacheReconciler->>ProfileClient: Fetch learned profile with known checksum
ProfileClient-->>CacheReconciler: ErrProfileUnchanged when checksum matches
CacheReconciler->>ContainerProfileCache: Keep existing entry without rebuild
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 4 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Performance Benchmark ResultsNode-Agent Resource Usage
Dedup EffectivenessNo data available. |
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
1 participant
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.
Overview
Lands the client half of "conditional container-profile fetch" (step 4a of 5 in
armosec/shared-designs-and-docs#201,
status: proposed). The container-profile cache reconciler can now present thecontent checksum of the profile it already holds, and treat a server's
"unchanged" reply as "keep the cached entry, skip the rebuild" — instead of
today's behavior, which fetches the full body every tick and only decides
after the object is on the wire whether a rebuild is needed.
This PR is DORMANT. No in-tree
ProfileClientimplementer returns the newsentinel, so no conditional fetch has been or can be observed here. Green
tests prove the contract compiles and existing behavior is byte-for-byte
unchanged — nothing more. The companion
kubescape/backendPR (kubescape/backend#62)adds the server-side contract; a private downstream adapter (not part of this
org, not this PR) is what will eventually make node-agent actually send the
checksum in production.
Design constraint
storage.ProfileClient.GetContainerProfile(ctx, namespace, name)has a secondimplementer in this repo (
pkg/storage/v1, the in-cluster CRD/aggregated-APIbackend), which has no concept of a remote checksum. This PR does not
change that interface's signature —
pkg/storage/v1/,storage_mock.go, andthe interface definition are byte-for-byte untouched (
git diff --statisempty). The checksum crosses the boundary out-of-band instead: a
context.Contextkey on the request side, a sentinel
errorplus anObjectMetaannotation onthe response side.
Changes
pkg/storage/checksum.go(new):WithKnownChecksum/KnownChecksumFromContextover an unexported context key;
ErrProfileUnchanged; the exportedContainerProfileChecksumAnnotationKey(backend.kubescape.io/container-profile-checksum,matching feat(storageclient): add conditional container-profile fetch contract backend#62's key exactly — pinned by a dedicated
cross-repo equality test in the downstream adapter).
CachedContainerProfilegains aChecksumfield, populated at bothplaces a cache entry is constructed (
rebuildEntryFromSourcesandbuildEntry/tryPopulateEntry— missing the second site would have madethe optimization silently inert for every profile that never changes, i.e.
exactly the population it targets).
refreshOneEntry's guard for attaching a checksum to the learned-CP fetchrequires: no authored-CP override in play, the projection spec hash
unchanged, a known checksum available, and the entry's learned status is
already terminal (
Completed+Full) — that last conjunct exists because alifecycle-only change (e.g. completion status flipping) doesn't change the
content checksum, and this cache's
Statefeeds real alerting logicdownstream, so a conditional fetch is only requested once nothing about the
entry can still move.
receives a checksum and is completely unaffected.
docs/features/container-profile-conditional-fetch-contract.md(new):documents this repo's half of the cross-repo contract for future
maintainers and upstream reviewers.
Testing
pkg/objectcache/containerprofilecache/reconciler_checksum_test.go,covering: both construction sites (including that the adopted entry stores
the learned CP's checksum, not the authored CP's), every guard conjunct
individually, per-call-site isolation (the authored-CP fetch never sees a
checksum even in the same tick), the sentinel keeping the cache entry
pointer-identical with no rebuild, and the terminal-state guard walking all
three phases (declined while partial → completion flip reaches the cache →
shortcut engages once terminal).
go test ./pkg/objectcache/containerprofilecache/... -racegreen.go test ./pkg/objectcache/... -run Goldengreen — no projection-goldenchurn from the new annotation.
edits, no new skips.
go build ./...— confirms the in-clusterStorageimplementer stillsatisfies
ProfileClientwith zero changes on this PR's part.Related
Step 4a of 5 in the conditional container-profile fetch plan.
Companion PR: kubescape/backend#62 (step 2, proto contract).
🤖 Generated with Claude Code
https://claude.ai/code/session_01HnqMRD3r2kGYUBTxMHM5vi
AI-skills: oh-my-claudecode:plan,oh-my-claudecode:team | cmds: /oh-my-claudecode:deep-interview
Summary by CodeRabbit
New Features
Documentation