feat(storageclient): add conditional container-profile fetch contract - #62
Conversation
Dormant contract only: no server sets unchanged=true today, so the sentinel path cannot fire in production. The one non-dormant change is the checksum annotation stamp, which runs on every normal fetch. Proto (additive, both directions wire-compatible via proto3 defaults): - GetContainerProfileStreamRequest.known_checksum = 5 - GetContainerProfileStreamChunkMetadata.unchanged = 5, checksum = 6 Client: - WithProfileKnownChecksum / ProfileOptions.KnownChecksum, default "" - ErrProfileUnchanged sentinel, returned without unmarshaling when the server reports unchanged for a request that carried a checksum - unchanged is checked before exists, since a server implementing only unchanged may leave exists at its proto3 default - unchanged on a request that sent no checksum is a server protocol violation and returns a distinct loud error, never the sentinel - the RPC context is cancelled on return so the sentinel's early return cannot leak a half-read stream - md.Checksum is stamped onto ObjectMeta annotations under backend.kubescape.io/container-profile-checksum, the only channel back through the caller-facing signature Regenerated with `make proto`; the generator also renamed the file-local import alias legacyv1beta1 -> v1beta11, which is cosmetic and changes no exported symbol. 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>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
jnathangreeg
left a comment
There was a problem hiding this comment.
Reviewed the contract together with the plan it implements (armosec/shared-designs-and-docs#201), since a dormant contract can only really be judged against the mechanism it has to carry. The measurement work behind the plan is unusually solid — the cost traced to a ship date, the nginx→ALB metric migration caught rather than mistaken for a fix, the Loki-vs-updated_at trap and the earlier updatePeriod misattribution both recorded. I'm not re-litigating the numbers.
Code findings are inline. Four plan-level points that bear on how this contract gets used:
1. The cheapest lever isn't priced anywhere. The plan pins the driver precisely — profilesCacheRefreshRate (viper default 1m) × up to 3 GetContainerProfile calls per entry, hence the measured 2.93 fetches/profile/minute. I confirmed it in node-agent v0.3.216: viper.SetDefault("profilesCacheRefreshRate", 1*time.Minute) (config.go:200), reconcileEvery := utils.AddJitter(cfg.ProfilesCacheRefreshRate, 10), and separate fetches for the learned CP (reconciler.go:343) and the user-defined CP (:374).
But nothing evaluates changing it — and it needs no code in any repo: the OSS chart's node-agent configmap ranges nodeAgent.config.extra into the same config.json viper reads, documented as an unvalidated passthrough for downstream use. nodeAgent.config.extra.profilesCacheRefreshRate: 5m cuts requests and bytes ~5x, reaching customers by the same chart upgrade that step 4 needs, with no proto change, no five-repo release train and no OSS review cycle on the critical path.
The trade is projection staleness (5m vs 1m), which may well be unacceptable for runtime detection — but that is a reason to state and reject it explicitly, not to omit it. The two are complementary: the knob for immediate relief, this contract for the steady state. Worth noting conditional fetch removes the bytes but keeps all 3.79M requests/day, each still costing a reference+checksum query, a TLS handshake, ALB LCU processing and probably a cross-AZ hop — and the plan itself calls 15.1M wasted queries/day on the other endpoint "a real load issue".
2. The RV question belongs inside step 4, not beside it. The plan files "does the client-side RV comparison ever actually invalidate?" as a separate bug. But if backend-served profiles carry an empty or constant RV, rvsMatchCP always matches and the projection never rebuilds — so step 4a has to move the rebuild decision onto the checksum too, or it ships "changed body arrives, projection still stale". This contract is the first correct validator this path has had; using it only to skip bytes leaves the bug in place.
3. Specify the resync guard before the bytes drop. "Periodic full resync so no agent can pin a stale profile indefinitely" is listed as remaining design work. Make it concrete — unconditional fetch every N reconciles, on reconnect, on config change. The first inline finding below is a live example of how a wrong validator reaches an agent, so this guard is what bounds the blast radius.
4. Cost aside. The plan flags EUN1-DataTransfer-Regional-Bytes ($1,136 / 113 TB) as out of scope, but if storage-be sits behind an ALB with cross-AZ targets, a share of the same 276 GB/day is billed again on that line at $0.01/GB. The fix likely recovers slightly more than the attributed $770, and that larger line deserves its own look for the same traffic.
| if md.Unchanged { | ||
| if profileOpts.KnownChecksum == "" { | ||
| // Protocol violation: the server claims a match against a validator we | ||
| // never sent. Never surface this as ErrProfileUnchanged — a caller | ||
| // treating it as the sentinel would pin its cache on no evidence at all. | ||
| return nil, fmt.Errorf("server reported unchanged for an unconditional request (namespace=%s name=%s)", namespace, name) | ||
| } | ||
| // Returns before the drain loop; the deferred cancel above tears the | ||
| // half-read stream down. | ||
| return nil, ErrProfileUnchanged |
There was a problem hiding this comment.
The empty-validator case is guarded loudly; the contradicting-validator case is trusted silently. The new proto comment says checksum is populated on every successful response, so when a server answers unchanged=true with a md.Checksum that differs from the known_checksum we sent, this code has the contradiction in hand and drops it.
Failure shape: a row mixup or a stale server-side cache replies unchanged=true, checksum="X" to a request carrying known_checksum="Y". The caller gets ErrProfileUnchanged, keeps its stale profile, re-presents "Y" on every subsequent refresh, and never re-fetches — node-agent then enforces an out-of-date profile indefinitely. That's the exact failure the plan's "periodic full resync" guard exists to bound, reachable without the guard ever being exercised, and it's the same hazard class as the violation you already reject.
One comparison closes it:
if md.Checksum != "" && md.Checksum != profileOpts.KnownChecksum {
return nil, fmt.Errorf("server reported unchanged but returned a different checksum (namespace=%s name=%s)", namespace, name)
}| // The ProfileClient-shaped signature has no other channel for the checksum, | ||
| // so stamp it on the object for callers that want to store it as a validator. | ||
| if md.Checksum != "" { | ||
| if profile.Annotations == nil { | ||
| profile.Annotations = map[string]string{} | ||
| } | ||
| profile.Annotations[ContainerProfileChecksumAnnotationKey] = md.Checksum |
There was a problem hiding this comment.
The stamped annotation can round-trip back into storage, and the round-trip lives in this same file. SendContainerProfileStream (line 300) does payload, err := profile.Marshal() on the profile as-is, with nothing stripping this key. So a Get → mutate → Send sequence persists backend.kubescape.io/container-profile-checksum into the stored object, which changes the content the server checksums, which means the validator the client saved never matches again — permanently defeating the mechanism this PR exists to enable. It also perturbs content-hash consumers (storage already carries a kubescape.io/sync-checksum convention over object content).
md.Checksum is also unbounded here, so a buggy server returning a large string makes the caller's next write-back fail API-server validation with metadata.annotations: Too long.
Suggest stripping the key in SendContainerProfileStream (or exporting a strip helper) and bounding the stamped length.
Two invariants worth writing into the doc comment while you're here, so step 4a can't get them wrong: the agent must echo this annotation verbatim and never recompute a hash over the object it holds — that object now contains an annotation the server never hashed, so a locally computed hash misses every time; and whatever the agent caches as its validator must be this value, not a digest of the cached object.
| @@ -397,6 +440,14 @@ func (c *StorageClient) GetContainerProfileStream(ctx context.Context, namespace | |||
| if err := profile.Unmarshal(buf); err != nil { | |||
| return nil, fmt.Errorf("failed to unmarshal ContainerProfile: %w", err) | |||
There was a problem hiding this comment.
exists=true with zero blob chunks now produces a validated empty profile. buf is nil, profile.Unmarshal(nil) succeeds, and the stamp below attaches a real checksum to a blank ContainerProfile (empty Name/Namespace). Before this PR that empty result self-healed on the next fetch; now the caller stores it as a validated cache entry, and every subsequent conditional fetch answers unchanged, so it is never repaired.
Cheapest fix is to treat it as an error — or at minimum skip the stamp — when md.Exists && len(buf) == 0.
| bool unchanged = 5; | ||
|
|
||
| // checksum is the stored profile's current content checksum. It is | ||
| // populated on EVERY successful response, not only on a match, so a | ||
| // client can refresh its stored validator after an ordinary body | ||
| // fetch. Empty when the server does not compute checksums. | ||
| string checksum = 6; |
There was a problem hiding this comment.
unchanged and checksum are independent fields, but the mechanism cannot bootstrap without checksum. The comment allows it to be "Empty when the server does not compute checksums", yet a client only ever learns a checksum from a response — so a server that implements unchanged and never populates checksum leaves every request unconditional and the feature silently dead, with no way for an operator to tell. Worth stating as a requirement: a server that supports unchanged MUST populate checksum on every successful response.
Also: the checksum crosses the wire as a bare string with no algorithm tag. A sha256: prefix costs nothing now and is unfixable later — without it, a future change of hash function is a silent semantic change rather than a detectable one.
Addresses jnathangreeg's review on #62 (three inline findings plus a spec-tightening request on the checksum field's doc comment). 1. unchanged is no longer trusted when the server's returned checksum contradicts the one we sent. Previously only an empty known_checksum was rejected as a protocol violation; a checksum that disagrees is the same hazard (a wrong validator reaching the caller silently) and is now rejected the same way, as a loud non-sentinel error. 2. SendContainerProfileStream no longer round-trips the checksum annotation GetContainerProfileStream stamps on fetched profiles. A Get -> mutate -> Send sequence would otherwise persist that annotation into storage, changing the content the server checksums and permanently invalidating the caller's own cached validator. The new marshalContainerProfileForSend strips the key on a copy without mutating the caller's object. 3. exists=true with zero blob chunks - a pre-existing edge case that unmarshals into a blank profile - is no longer stamped with a checksum. Before checksum stamping existed this self-healed on the next fetch; stamping it would instead validate the blank result permanently. Also bounds the stamped checksum to maxStampedChecksumLength so an oversized value from a misbehaving server can't later fail Kubernetes' annotation-size validation on write-back. Also tightens the checksum field's proto comment: a server that sets unchanged MUST also populate checksum on every successful response, not just on a match, since a client only ever learns a checksum from this field. Left the sha256: wire-prefix suggestion from the same review comment for later - it's a wire-format decision that needs coordinating with cadashboardbe's still-unmerged step 3, out of scope for this fix. 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>
Implements the sha256: wire-prefix suggestion from jnathangreeg's review on #62. The decision lives here, in kubescape/backend, since this repo owns the proto contract and the client library both sides of the wire depend on; postgres-connector and cadashboardbe will be updated to actually emit the prefix in a follow-up once this merges. ChecksumAlgorithmSHA256Prefix ("sha256:") is now the canonical, exported definition of the required format for both known_checksum and checksum. GetContainerProfileStream will not stamp a checksum missing the prefix onto a fetched profile's annotations -- same treatment as an oversized checksum: the optimization degrades to inert rather than the fetch failing. No producer in this pipeline emits a prefixed value yet, so this is a no-op in practice until postgres-connector/cadashboardbe adopt it -- consistent with the rest of this pass, which is a dormant contract until cadashboardbe's step 3 ships. The contradiction check added earlier (unchanged with a mismatched checksum) is left format-agnostic on purpose: it's a raw equality check regardless of what the values look like, so it doesn't need to know about the prefix. 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>
jnathangreeg
left a comment
There was a problem hiding this comment.
Approving at 83c27d6. All four inline findings are closed, and I checked each in the source rather than taking the commit messages for it.
- Contradiction guard —
md.Checksum != "" && md.Checksum != profileOpts.KnownChecksumrejected as a loud non-sentinel error. That was the one that mattered: it's the path by which a wrong validator reaches an agent and pins a stale profile without the plan's resync guard ever being exercised. - Annotation round-trip —
marshalContainerProfileForSendgets the subtle part right: it builds a fresh annotations map instead of sharing the caller's, so stripping the key can't mutate the caller's object, and it short-circuits when the key is absent so the common path allocates nothing. That shared-map bug is exactly what I'd have looked for behind a "strips it on a copy" claim. - Empty-body case — the stamp is gated on
len(buf) > 0alongside the length bound and the prefix check, soexists=truewith zero chunks can no longer be validated into a permanently unrepairable blank profile. - Algorithm tag — went further than I asked.
ChecksumAlgorithmSHA256Prefixis the exported canonical definition, an unprefixed value is never stamped so the optimization degrades to inert rather than failing the fetch, and the proto now states the bootstrap requirement explicitly — a server settingunchangedMUST populatechecksumon every successful response, and a server with no support leaves both unset so "not supported" stays distinguishable from "silently permanent cache miss". Leaving the contradiction check format-agnostic is the right call; it's raw equality either way.
Verified locally at this head: go build ./..., go vet ./pkg/client/v1/... and go test ./pkg/client/v1/... all clean.
The four plan-level points from my review body are untouched here, as they should be — they're decisions for armosec/shared-designs-and-docs#201 rather than feedback on this contract, and none of them blocks merging a dormant contract. Restating them so they don't get lost when this merges and step 3 starts:
- The cadence lever (
nodeAgent.config.extra.profilesCacheRefreshRate) is still unpriced in the plan, and it needs no code in any repo. - Step 4a should move cache invalidation onto the checksum, not just use it to skip bytes — otherwise the
rvsMatchCPquestion leaves "changed body arrives, projection still stale" in place. - The periodic full resync wants specifying before step 4b, not after.
- Cross-AZ (
EUN1-DataTransfer-Regional-Bytes) probably carries a share of the same bytes and is the larger line.
Summary
Adds the proto contract, client option, and sentinel error for conditional
container-profile fetch (step 2 of 5 in armosec/shared-designs-and-docs#201,
status: proposed). A caller that already holds a profile's content checksum can present it, and the server can answer "unchanged" without re-sending the body.This PR is a dormant contract with zero observable behavior change. Nothing
in this org's backend deployment sets
unchanged=trueyet — that's step 3(
cadashboardbe, tracked separately, not part of this PR). The one non-dormantchange is the checksum-annotation stamp on every normal fetch response, which
this PR's tests and description call out explicitly.
Changes
GetContainerProfileStreamRequestgainsknown_checksum(field 5) — proto3default
""means "send the body unconditionally"; a server that doesn'tunderstand the field simply ignores it.
GetContainerProfileStreamChunkMetadatagainsunchanged(field 5) andchecksum(field 6).unchangedis checked beforeexists, since a serverimplementing only
unchangedmay leaveexistsat its default.WithProfileKnownChecksum(checksum string) ProfileOption, mirroring theexisting
WithProfileRegion/WithProfileCloudAccountIdentifier.ErrProfileUnchangedsentinel, returned without unmarshaling when the serverreports unchanged for a request that carried a checksum. A server reporting
unchanged=trueagainst an emptyknown_checksumis treated as aprotocol violation and returns a distinct, loud error instead of the
sentinel — never silently trusted.
ObjectMeta.Annotationsunder an exported, stable key(
ContainerProfileChecksumAnnotationKey=backend.kubescape.io/container-profile-checksum) — this is the onlychannel back through
kubescape/node-agent's unchangedProfileClientinterface (see the companion node-agent PR).
context.WithCancel/defer cancel()), incidental to adding the new earlyreturn but benefits all existing error paths too.
Testing
make protorun twice frompkg/client/v1/proto/; second run byte-identical.pkg/client/v1/storageclient_checksum_test.go: optionround-trip, sentinel-on-unchanged (unmarshal never attempted), the
protocol-violation guard, checksum-annotation stamping, and a
backward-compatibility round-trip proving proto3 defaults hold in both
directions.
go build ./... && go vet ./... && go test ./...green (one pre-existing,unrelated
go vetfinding inpkg/server/v1/systemreports— async.Mutexcopy — untouched by this PR).
Related
Step 2 of 5 in the conditional container-profile fetch plan.
Companion PRs:
kubescape/node-agent(step 4a, cache reconciler),armosec/private-node-agent(step 4b, adapter wiring — currently blocked on this PR and the node-agent PR being tagged, so not yet opened).🤖 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