fix(binding-kafka): honor per-message authorization on the cache fetch path - #2431
Open
jfallows wants to merge 8 commits into
Open
fix(binding-kafka): honor per-message authorization on the cache fetch path#2431jfallows wants to merge 8 commits into
jfallows wants to merge 8 commits into
Conversation
…ch path KafkaCacheServerFetchFactory decoded a fetched Kafka record's value once, while populating the local cache from the broker, with authorization hardcoded to NO_AUTHORIZATION -- there is no requesting consumer yet at that point. The decoded result was stored in the partition's convertedFile and served to every future reader verbatim, including the per-consumer fetch stream (KafkaCacheClientFetchFactory), which constructed no model pipeline of its own at all and never threaded its own live authorization into anything. ModelHandler gains a third pipeline-vending method, supplyCacheable, alongside supplyDecoder/supplyEncoder -- deliberately abstract, not defaulted, so every implementor must explicitly decide its behavior rather than silently inheriting a delegate that could leak a consumer-specific value into the shared cache. Every existing implementor (model-core, model-json, model-protobuf, model-avro, engine's TestModel, and the anonymous test doubles in binding-kafka's own unit tests) now implements it explicitly; all but TestModel simply delegate to supplyDecoder, so today's behavior is unchanged byte-for-byte everywhere except the new test coverage. KafkaCacheServerFetchFactory's populate-time pipeline now uses supplyCacheable instead of supplyDecoder. KafkaCacheClientFetchFactory gains a genuine per-stream decode: it resolves the topic's value model at stream construction (mirroring KafkaCacheClientProduceStream's existing per-stream pattern), and when that model's decoder isn't a byte-for-byte identity, decodes each cached entry's value against the requesting stream's own live authorization before forwarding it -- exactly once per message, so a consumer whose authorization changes mid-stream sees the change reflected starting with the next fetched message. A model with nothing consumer-specific configured never triggers the extra decode at all (identity() stays true), so there is no behavior change or added cost for the common case. Verified test-first: CacheFetchIT#shouldReceiveMessageValueAuthorizationDistinct fails cleanly (both consumers see the same undisclosed value) before the fix and passes after. Extended the engine's TestModel with discloseAuthorized/discloseRedacted config to simulate consumer-specific disclosure without depending on any production model module. Full runtime/binding-kafka suite (583 tests), specs/binding-kafka.spec peer-to-peer suite (667 tests), and runtime/engine + config/engine.conf + all model module unit tests (509 tests) all pass with no regressions; checkstyle and license checks clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzxN4tJLhiHQ8ikNeuN8WS
ModelHandler.supplyCacheable already vends a separate pipeline from supplyDecoder, but every *ModelExtHandler's own extension-stage fold (model-avro, model-protobuf, model-json, model-core) still built that pipeline by unconditionally invoking each installed extension's decode() stage — so an extension had no way to behave differently between a pipeline populating shared storage ahead of any specific reader's request and one resolving the view for the reader making a request right now. Add a cacheable() method to each *ModelExtHandler interface, defaulting to decode() for full backward compatibility, and thread a cacheable flag through each handler's pipeline construction so supplyCacheable folds installed extensions via cacheable() while supplyDecoder continues to fold via decode().
…pii-status-nfzbgr
…Decoder PR #2431 (edb34fd, 011d0bf) added a two-way supplyCacheable()/ supplyDecoder() split to give the Kafka local cache per-consumer authorization decode without re-fetching from Kafka. Both methods used the same frontend parser and output format, differing only in which ext-handler fold ran -- but any model whose decode does real, format-converting work (view: json, disclosed/encrypted fields, ...) produces output that differs from its wire-format input, so caching that output and later re-decoding it with the wire-format frontend is never correct. This surfaced in CI as a permanent hang on the http.kafka.avro.json example: the avro model's `view: json` config made the populate-time decode cache JSON text, and the new per-consumer fetch-time decode then fed that JSON into a parser expecting avro wire bytes. Replace the two-method split with a single supplyDecoder(envelope, transform, ModelCache) parameterized by a three-way cache context: NONE (today's plain decode, no cache), WRITE (decode ahead of any reader, producing the value a cache persists), and READ (decode a value already in whatever form WRITE produced, for the reader requesting it now). WRITE and NONE share the same wire-format frontend; READ's frontend matches whatever WRITE actually emitted -- for avro/protobuf's `view: json`, that's AvroJson.stream/ ProtobufJson.parser (the same json-view frontend already used on the encode-shaped pipeline), reused rather than duplicated. Mirror the same three-way split into each model's own *ModelExtHandler SPI as a single decode(T, <Model>Cache) method -- a local enum per model (AvroCache/ProtobufCache/JsonCache/CoreCache), not the shared engine ModelCache, so each model's extension surface stays independent -- replacing the previous decode(T)/cacheable(T) pair. Also close a related gap found while building this: protobuf's own message-index framing (separate from catalog framing) has no magic-byte-style validity guard, so a cached view:json value with no index framing at all would misparse its leading JSON bytes as an index varint on READ. Resolve the message via the same static catalog.record path the encode side already uses whenever cache == READ. Persist the schema id WRITE resolves as catalog framing ahead of its cached output (reusing the same CatalogHandler.encode(...) machinery the encode-direction pipeline already uses), so READ recovers it directly from the cached bytes via the existing resolveSchemaId/prefix logic, with no changes needed on the read side. This closes the one case that otherwise stays broken even with the frontend fix: a strategy: encoded schema id is per-message authoritative (schema evolution means two cached messages on the same topic can carry different ids), and a view-converting WRITE output previously discarded it entirely with no static fallback available -- unlike strategy: topic/subject-pinned configs, whose schema was never message-embedded to begin with. Add a CacheFetchIT scenario driving a real avro + view: json config through a live engine populate-then-fetch round trip -- the exact shape of the original regression -- since TestModel's identity decode cannot exercise this failure mode. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CzxN4tJLhiHQ8ikNeuN8WS
…pii-status-nfzbgr
…pii-status-nfzbgr
…pii-status-nfzbgr
…pii-status-nfzbgr
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
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.
Description
KafkaCacheServerFetchFactorydecoded a fetched Kafka record's value once, while populating the local cache from the broker, withauthorizationhardcoded toNO_AUTHORIZATION— there is no requesting consumer yet at that point. The decoded result was stored in the partition'sconvertedFileand served to every future reader verbatim, including the per-consumer fetch stream (KafkaCacheClientFetchFactory), which constructed no model pipeline of its own at all and never threaded its own liveauthorizationinto anything — meaning the per-messageauthorizationparameterModelPipeline.transform(...)already carries (#2369) never reached a real, per-consumer value on this path.Correction (superseding the original design)
The first version of this fix (now superseded by later commits on this branch) added a separate, abstract
ModelHandler.supplyCacheable(...)method alongsidesupplyDecoder/supplyEncoder. CI caught a real regression in that design: thehttp.kafka.avro.jsonexample hung for its full 30-minute timeout. Root cause —supplyCacheableandsupplyDecoderused the same frontend parser and output format, differing only in which ext-handler fold ran. That's fine for a byte-for-byte identity decode, but any model whose decode does real, format-converting work (e.g.view: json) produces output that differs from its wire-format input. Caching that converted output and then re-decoding it with a wire-format frontend can never work: the avro model'sview: jsonconfig made the populate-time decode cache JSON text, and the per-consumer fetch-time decode then fed that JSON into a parser expecting avro wire bytes.The corrected design replaces the two-method split with a single
supplyDecoder(envelope, transform, ModelCache), parameterized by a three-way cache context:NONE— today's plain decode, no cache involved.WRITE— decode ahead of any specific reader, producing the value a cache persists (the oldsupplyCacheable's role).READ— decode a value already in whatever formWRITEproduced, for the reader requesting it now. Its frontend matches whateverWRITEactually emitted — for avro/protobuf'sview: json, that'sAvroJson.stream/ProtobufJson.parser, the same json-view frontend already used on the encode-shaped pipeline, reused rather than duplicated.Each model's own
*ModelExtHandlerSPI mirrors this as a singledecode(T, Cache)method — a local enum per model (AvroCache/ProtobufCache/JsonCache/CoreCache), not the shared engineModelCache, so each model's extension surface stays independent — replacing the previousdecode(T)/cacheable(T)pair.Building this also surfaced and closed a related gap: protobuf's own message-index framing (separate from catalog framing) has no magic-byte-style validity guard, so a cached
view:jsonvalue with no index framing at all would misparse its leading JSON bytes as an index varint onREAD. Fixed by resolving the message via the same staticcatalog.recordpath the encode side already uses whenevercache == READ.Finally,
WRITEnow persists the schema id it resolves as catalog framing ahead of its cached output (reusing the sameCatalogHandler.encode(...)machinery the encode-direction pipeline already uses), soREADrecovers it directly from the cached bytes via the existingresolveSchemaId/prefixlogic, with no changes needed on the read side. This closes the one case that otherwise stays broken even with the frontend fix: astrategy: encodedschema id is per-message authoritative (schema evolution means two cached messages on the same topic can carry different ids), and a view-convertingWRITEoutput previously discarded it entirely with no static fallback available — unlikestrategy: topic/subject-pinned configs, whose schema was never message-embedded to begin with.Verification
AvroModelDecoderPipelineTest/ProtobufModelDecoderPipelineTestgainedWRITE→READround-trip tests for both theview: jsonand no-view cases, plus a case provingREADrecovers the schema id fromWRITE-injected framing even when the model's own static catalog reference is deliberately wrong — the exactstrategy: encodedshape. These are the tests that would have caught the original regression;TestModel's identity decode structurally cannot exercise this failure mode. Exercising the framing case required extending the engine'sTestCatalogHandlertest double (resolve(DirectBufferEx,...)/decodePadding(...)) to honor its ownprefixoption symmetrically with its existingencode(...), per this repo's convention of extendingtype: testimplementations additively rather than reaching for a production catalog implementation in test scope.CacheFetchITgained a new scenario (shouldReceiveMessageValueAvroViewJson) driving a realavro+view: jsonconfig through a live engine populate-then-fetch round trip — the exact shape of the original regression — verified passing against the live engine, not just unit-level.runtime/binding-kafkasuite (401 tests, all k3po ITs included),specs/binding-kafka.specsuite (peer-to-peer + config),runtime/model-avro/model-protobuf/model-json/model-coresuites, andruntime/binding-http/binding-sse/binding-mqtt/binding-mcpsuites all pass with no regressions; checkstyle and license checks clean.develop(commitd63e78ab9) is green across all 42 checks, includingtesting (http.kafka.avro.json)(2m27s, vs. the original 30-minute hang) andtesting (mcp.proxy).Fixes #2423
Generated by Claude Code