Repository navigation
fix(kafka): take destination tenant from trusted config, not message headers - #140
Merged
Merged
Conversation
…ests Kafka trusts producer-controlled tenant headers, permitting cross-tenant ingestion. Introduce pure routing and startup validation seams, a mode snapshot, and deterministic tests while preserving the old routing and permissive configuration behavior for fails-before verification. Add inactive --consumer-tenant-id / P_KAFKA_CONSUMER_TENANT_ID and rejection instrumentation scaffolding. The follow-up fix will ignore headers in single-tenant mode, validate the configured tenant and require multi-tenancy when it is set, and use only trusted configuration in multi-tenant mode. With multi-tenancy enabled and no configured tenant, headerless records will keep None/default while tenant-bearing records will be dropped and counted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A producer able to write a consumed topic could select another tenant through message headers. In multi-tenant mode, resolve the destination solely from --consumer-tenant-id / P_KAFKA_CONSUMER_TENANT_ID. Reject conflicting, duplicate-conflicting, null or invalid tenant headers before batching, count drops with parseable_kafka_tenant_mismatch_dropped_total, and warn at most once per process. Single-tenant mode ignores all tenant headers and ingests to None/default without counting drops. Fail before consumer creation if a tenant is configured without --multi-tenancy / P_MULTI_TENANCY, or if it fails the server tenant-ID validator. In multi-tenant mode the tenant option is recommended: without it, warn once at startup, preserve headerless None/default behavior, and drop tenant-bearing records. All consumed topics use the configured destination. No storage format changes. Dropped offsets advance only when a later record on the partition is accepted, so a partition carrying only rejected records can replay them after restart and over-count rejections. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Problem
The Kafka connector took the destination tenant from producer-controlled message headers. Any producer able to write to a consumed topic could inject data into another tenant.
Fix
Resolve the destination tenant only from
--consumer-tenant-id/P_KAFKA_CONSUMER_TENANT_ID, using the same configured destination for every consumed topic. Headers can assert that destination but cannot select another tenant.parseable_kafka_tenant_mismatch_dropped_total.x-p-tenantheader are dropped and counted, including headers whose value isDEFAULT_TENANT(the default tenant's literal ID). A startup warning recommends configuring the destination.P_KAFKA_CONSUMER_TENANT_IDunset, not empty.OwnedHeaders, including exact key matching and null values.Rejected records now advance the committed offset even when no later accepted record arrives.
ConsumerRecord::from_messagereturns an offset-only marker for a rejected record: payload and key areNone, the tenant remains the trusted configured destination, and topic, partition, offset, and timestamp are preserved. Markers pass through the ordered partition queue alongside accepted records. All-marker chunks (and tombstone-only chunks) skipprepare_streamand commit their next offset without accessing stream metadata or staging data.Within each partition, ordered buffering allows up to
MAX_IN_FLIGHT_CHUNKS_PER_PARTITION = 8chunks in flight. Chunk processing can complete out of order, but commit offsets are yielded in input order. A fast marker-only tail therefore cannot commit past an earlier unfinished chunk. Existing commit-after-processing-error behavior is preserved; an ordinary error completes that chunk under the existing drop policy.The generic
ConsumerRecord::from_message<M: Message>andKafkaStreams::handle_message<M: Message>seams use the same production conversion and routing logic with borrowed broker messages and owned messages in tests. Tests can verify real header extraction, marker metadata, and delivery to the partition receiver without a Kafka broker.Markers and genuine Kafka tombstones are handled identically: neither emits the former "without a payload" warning, and a chunk containing only tombstones or markers no longer creates a stream or accesses its metadata.
Mismatch warnings are rate-limited to at most one every 60 seconds per process. Each warning describes rejected
x-p-tenantheaders neutrally and includes the triggering record's topic, partition,tenant_configuredboolean, and interval drop count across all partitions. Header values, keys, and payloads are never logged. Warnings repeat only when another drop occurs after the interval; the Prometheus counter remains unlabeled.Upgrade note
Multi-tenant users who relied on message headers to choose a destination must run an ingestor configured with
--consumer-tenant-id/P_KAFKA_CONSUMER_TENANT_IDfor each destination tenant. Each tenant needs its own consumer group or its own topics. When consuming shared topics, assign a distinct--consumer-group-id/P_KAFKA_CONSUMER_GROUP_IDto each tenant's ingestors; do not leave different tenants on the shared defaultparseable-connect-cg. A shared group distributes partitions among tenants' ingestors, which can drop records intended for another tenant and commit past them, causing data loss. Ingestors for the same tenant may share that tenant's group.Leaving the tenant option unset in multi-tenant mode accepts only headerless records at the default destination. Any record with an exact
x-p-tenantheader is dropped, including one namingDEFAULT_TENANT. The reserved default tenant ID cannot be configured explicitly. Single-tenant mode continues to ignore tenant headers.Configure the destination tenant and tenant-specific consumer groups before upgrading. Rejected records are now committed, so a misconfigured rollout permanently skips them for normal consumption, even after configuration is corrected. Recovery requires a manual consumer-group offset reset with
kafka-consumer-groups --reset-offsets. LeaveP_KAFKA_CONSUMER_TENANT_IDunset, not empty, when using the unconfigured behavior; an empty value fails startup parsing.Tests
rejected_message_becomes_marker_with_trusted_tenant_and_broker_metadata: realOwnedMessageconversion strips rejected payload/key, preserves broker metadata and configured tenant, and computes committed next offset 43 from source offset 42.accepted_message_preserves_payload_and_key: acceptedOwnedMessageconversion preserves content and broker metadata.rejected_record_is_delivered_to_partition_stream_receiver: exercises the real routing path and receives the rejected record's marker from its partition receiver. A temporary early-return mutation made this test fail (0 passed, 1 failed, exit 101: no partition receiver); the mutation was restored before validation.all_rejected_partition_commits_next_offset: marker-only chunks produce ordered committed next offsets 12 and 13 using a fake processor/commit seam.chunk_needs_stream_only_for_payload_records: pure predicate checks empty, marker/tombstone-only, and mixed chunks without global PARSEABLE initialization.rejected_tail_waits_for_accepted_records_before_commit: gates accepted processing and proves a rejected tail cannot commit past it.later_chunk_starts_while_earlier_chunk_blocks_commits: later processing starts while the first chunk is blocked; both commits wait and then yield in source order.tenant_drop_warnings_repeat_after_sixty_seconds_with_interval_count: verifies monotonic warning intervals and accumulated drop counts.owned_unrelated_headers_do_not_hide_tenant_mismatch: real header extraction retains mismatches among unrelated headers.owned_case_variant_key_does_not_select_tenant: header keys match exactly.owned_matching_tenant_header_is_accepted: matching headers pass without a drop.owned_null_tenant_header_is_dropped: null values are preserved for rejection.absent_headers_preserve_configured_or_default_destination: absent headers retain trusted routing.empty_consumer_tenant_id_is_rejected_by_cli: empty CLI option fails parsing.empty_consumer_tenant_env_is_rejected_in_child_process/tenant_env_child: subprocess isolation verifies empty environment-value rejection without global environment mutation or test races. The child is ignored in normal runs; its parent explicitly invokes it with--ignored --exact, sets the guard and empty tenant environment value, and checks that one test passed.Restart coverage is unit-level: these tests verify the committed next offset, not live-broker restart/reassignment or commit persistence. The vacuous in-memory restart-filter test was removed. Docker Kafka compose setups exist in the repository, but no automated restart/offset test harness was found; no broker-backed test was added or run. Existing tenant-policy and startup-validation tests remain.
Validation
On trimmed head
176666c3, merged withorigin/mainat65ee74ff:CFLAGS=-std=gnu17 cargo test --lib --features kafka connectors::kafka: 63 passed, 0 failed, 1 ignored, 1435 filtered out; exit 0. The parentempty_consumer_tenant_env_is_rejected_in_child_processpassed, explicitly setting the guard and empty tenant environment value and invoking the ignored child with a1 passedassertion.-- --ignored --exact connectors::kafka::config::tenant_env_tests::tenant_env_child: 1 passed, 0 failed, 0 ignored, 1498 filtered out; exit 0. With no child guard set, this direct run returns before asserting; the parent's passing subprocess run supplies the meaningful environment-value check.cargo fmt --check: passed, exit 0.CFLAGS=-std=gnu17 cargo clippy --features kafka --all-targets: passed, exit 0; 22 diagnostics, 0 new compared with a fresh build oforigin/mainat65ee74ff.Known limitations / scope relative to main
There is no per-chunk processing timeout. A hung chunk stalls that partition's commits: ordered buffering never yields a later commit past an unfinished earlier chunk. For this stall case, no commit passes unprocessed records. On
origin/main,for_each_concurrent(None)lets later chunks finish and commit past a hung earlier chunk.Once all eight buffer positions are occupied behind a hung chunk, the partition stops draining. Its existing 100,000-record queue can fill, and the single consumer thread awaits a send to that full queue, blocking delivery to other partitions and potentially exceeding
max.poll.interval.ms. While that send is blocked, the consumer holds the stream-state lock, so a revoke delivered in that window is skipped (skipping rebalance revoke, as on main when the lock is busy); the stale partition task keeps its in-flight chunks, and if they later finish they can commit after the partition has a new owner. That can move the committed offset backwards (duplicates), not past unprocessed records. For an asynchronously hung chunk, main instead keeps draining into unbounded in-flight work, with potential memory growth. Synchronous blocking or a stalled synchronous commit can impede progress in either implementation.Processing-failure and revoke policies are unchanged from main: ordinary failed chunks still advance the committed offset without retry, and there is no post-revoke generation fence for commits. The separate scaling stack's layer 08 (
stack/08-kafka-commit) supplies a serialrun_partition, retries, partition stop, and a commit gate on the contiguous durable frontier; 08b (stack/08b-kafka-durable-ack) extends durable acknowledgments. Bounded timeout/recovery work belongs to that stack's 08/08b scope. The current refs have a bounded revoke-drain timeout, not a per-chunk processing deadline. These layers are separate from #140 and do not yet include it.Panic handling is unchanged from main. An async panic while polling
process, includingprepare_stream, unwinds the partition task under eitherbuffered(8)orfor_each_concurrent(None); neither converts it into an ordinary processing error. The panicking chunk's own commit callback is not reached. Main may already have committed later chunks before that panic; ordered buffering prevents that while the earlier chunk remains unfinished. In both versions, the consumer retains the closed partition sender, continues fetching, and logs/drops subsequent records in memory without committing them until revoke or restart. A panic inside the spawned Rayon closure follows Rayon's default process-abort policy; no custom panic handler is installed.Records already committed after tenant rejection are not replayed when configuration is corrected. Recovery requires a manual consumer-group offset reset and may duplicate data already ingested.
Per-topic tenant routing is not supported. Configured tenant IDs are validated syntactically but are not checked for existence or ingestion/workspace suspension; that pre-existing issue is tracked as a follow-up.
Restart coverage is unit-level, not broker-backed: tests exercise next-offset calculation and ordered commits through an injected callback, not broker persistence, restart, or reassignment.
🤖 Generated with Claude Code