From b410d91dbf946dfc632fb6d8c6f9aafdb7e84f1f Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Mon, 21 Sep 2026 22:22:34 -0400 Subject: [PATCH 01/10] Initial version of CreateTopics and MetaData implementation Wires the Kafka gateway's CreateTopics (#3538) and Metadata (#3534) handlers to the real Iggy bridge. --- gateways/kafka/src/bridge/error.rs | 50 +- gateways/kafka/src/bridge/iggy_bridge/mod.rs | 2 + .../kafka/src/bridge/iggy_bridge/topics.rs | 184 ++++++- gateways/kafka/src/bridge/mod.rs | 2 +- gateways/kafka/src/bridge/topic_map.rs | 6 + gateways/kafka/src/protocol/api.rs | 6 + .../src/protocol/handlers/create_topics.rs | 463 +++++++++++++++++- .../kafka/src/protocol/handlers/metadata.rs | 381 +++++++++++++- .../tests/bridge_iggy_integration_tests.rs | 267 ++++++++++ .../tests/create_topics_real_bridge_tests.rs | 430 ++++++++++++++++ .../kafka/tests/metadata_real_bridge_tests.rs | 363 ++++++++++++++ 11 files changed, 2105 insertions(+), 49 deletions(-) create mode 100644 gateways/kafka/tests/create_topics_real_bridge_tests.rs create mode 100644 gateways/kafka/tests/metadata_real_bridge_tests.rs diff --git a/gateways/kafka/src/bridge/error.rs b/gateways/kafka/src/bridge/error.rs index 10f2035e32..34ec7452eb 100644 --- a/gateways/kafka/src/bridge/error.rs +++ b/gateways/kafka/src/bridge/error.rs @@ -19,9 +19,9 @@ use iggy::prelude::IggyError; use thiserror::Error; use crate::protocol::api::{ - ERROR_INVALID_PARTITIONS, ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NOT_LEADER_OR_FOLLOWER, - ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, ERROR_TOPIC_AUTHORIZATION_FAILED, - ERROR_UNKNOWN_SERVER_ERROR, ERROR_UNKNOWN_TOPIC_OR_PARTITION, + ERROR_INVALID_PARTITIONS, ERROR_INVALID_REQUEST, ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NONE, + ERROR_NOT_LEADER_OR_FOLLOWER, ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, + ERROR_TOPIC_AUTHORIZATION_FAILED, ERROR_UNKNOWN_SERVER_ERROR, ERROR_UNKNOWN_TOPIC_OR_PARTITION, }; /// Errors from the `IggyBridge`: connection lifecycle, config, and Iggy SDK calls. @@ -85,6 +85,12 @@ pub enum BridgeError { /// raw or non-conformant client, not an expected path. #[error("invalid Kafka topic name '{kafka_topic}': {reason}")] InvalidKafkaTopicName { kafka_topic: String, reason: String }, + /// `ensure_stream_and_topic` was asked for `partition_count == 0`. Enforced here, not only + /// at the `CreateTopics` wire-validation layer that's this bridge's only caller today - the + /// method is `pub`, so a future caller (a test, or a later Produce auto-create path) + /// bypassing that layer must not be able to provision a topic nothing can produce to. + #[error("partition count must be at least 1, got 0 for topic '{kafka_topic}'")] + InvalidPartitionCount { kafka_topic: String }, } impl BridgeError { @@ -106,6 +112,9 @@ impl BridgeError { Self::PartitionOutOfRange { .. } => ERROR_UNKNOWN_TOPIC_OR_PARTITION, Self::PartitionCountMismatch { .. } => ERROR_TOPIC_ALREADY_EXISTS, Self::InvalidKafkaTopicName { .. } => ERROR_INVALID_TOPIC_EXCEPTION, + // Same code the wire-validation layer already uses for this exact condition - this + // is the same failure reached through a different door, not a new kind of error. + Self::InvalidPartitionCount { .. } => ERROR_INVALID_PARTITIONS, // Not a wire-response case in practice: an invalid bridge config is caught at // `IggyBridge::connect` before any handler exists to answer a Kafka request, so this // is reachable only if a future caller starts constructing configs at request time. @@ -163,7 +172,17 @@ const fn iggy_error_to_kafka_code(err: &IggyError) -> i16 { | IggyError::TcpError | IggyError::TransientNotAccepted => ERROR_NOT_LEADER_OR_FOLLOWER, IggyError::TransientNotCommitted => ERROR_REQUEST_TIMED_OUT, - IggyError::TooManyPartitions => ERROR_INVALID_PARTITIONS, + // Not `ERROR_INVALID_PARTITIONS` (37): that code's own text, per `kafka-protocol`'s + // table, is "Number of partitions is below 1" - the opposite condition from "too many" + // (Iggy's server-side cap, above 1000). Reusing 37 for both directions would return a + // client-visible error message that contradicts the actual request it sent. + IggyError::TooManyPartitions => ERROR_INVALID_REQUEST, + // The operation *did* commit - the SDK's own reconnect path replayed a write whose first + // attempt already applied, and the server's client-table dedup caught the replay. Falling + // into the catch-all below would report a permanent server fault for a request that + // actually succeeded; a Java client treats `UNKNOWN_SERVER_ERROR` as non-retriable and + // would surface a spurious failure for a `CreateTopics` that in fact created the topic. + IggyError::RequestAlreadyApplied => ERROR_NONE, _ => ERROR_UNKNOWN_SERVER_ERROR, } } @@ -211,8 +230,28 @@ mod tests { } #[test] - fn too_many_partitions_maps_to_invalid_partitions() { + fn too_many_partitions_maps_to_invalid_request_not_invalid_partitions() { + // INVALID_PARTITIONS (37) means "count is below 1" (kafka-protocol's own error table) - + // the opposite condition from "too many": reusing it here would send a client-visible + // message that contradicts the request it just sent. let err = BridgeError::Iggy(IggyError::TooManyPartitions); + assert_eq!(err.to_kafka_error_code(), ERROR_INVALID_REQUEST); + } + + #[test] + fn request_already_applied_maps_to_no_error_not_unknown_server_error() { + // The operation committed on its first attempt; the client-table dedup on a replay is + // not a fault. Falling into the catch-all (-1) would tell a Java client the CreateTopics + // it just replayed permanently failed, when the topic it asked for now exists. + let err = BridgeError::Iggy(IggyError::RequestAlreadyApplied); + assert_eq!(err.to_kafka_error_code(), ERROR_NONE); + } + + #[test] + fn invalid_partition_count_maps_to_invalid_partitions() { + let err = BridgeError::InvalidPartitionCount { + kafka_topic: "orders".to_string(), + }; assert_eq!(err.to_kafka_error_code(), ERROR_INVALID_PARTITIONS); } @@ -383,6 +422,7 @@ mod tests { ResponseError::TopicAlreadyExists, ), (ERROR_INVALID_PARTITIONS, ResponseError::InvalidPartitions), + (ERROR_INVALID_REQUEST, ResponseError::InvalidRequest), ] { assert_eq!( ours, diff --git a/gateways/kafka/src/bridge/iggy_bridge/mod.rs b/gateways/kafka/src/bridge/iggy_bridge/mod.rs index e11079141f..f656352ae8 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/mod.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/mod.rs @@ -29,6 +29,8 @@ mod offsets; mod produce; mod topics; +pub use topics::{KafkaTopicMetadata, TopicCreationOutcome}; + /// Passes attempted, after the first, before [`IggyBridge::connect`] gives up and returns `Err`. /// /// Not the SDK's own default (`TcpClientReconnectionConfig::default()` is `max_retries: None` - diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index 395f6f27d2..75d25b5536 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -17,13 +17,44 @@ //! Stream and topic provisioning. -use iggy::prelude::{Identifier, IggyError, StreamClient, TopicClient, TopicCreateOptions}; +use std::collections::HashSet; + +use iggy::prelude::{ + Identifier, IggyError, StreamClient, TopicClient, TopicCreateOptions, TopicDetails, +}; use tracing::{debug, info}; use super::{IggyBridge, with_request_timeout}; use crate::bridge::error::BridgeError; use crate::bridge::topic_map::validate_kafka_topic_name; +/// Outcome of [`IggyBridge::create_kafka_topic`]. +/// +/// Distinguishes "this call is the one that created it" from "it already existed", so +/// `CreateTopics` can answer `TOPIC_ALREADY_EXISTS` correctly even when two requests for the same +/// new topic race each other. Unlike [`IggyBridge::ensure_stream_and_topic`]'s idempotent-success +/// contract, `CreateTopics` itself is not an upsert: real Kafka guarantees exactly one caller sees +/// a create succeed, every other concurrent caller for the same new name sees +/// `TOPIC_ALREADY_EXISTS`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TopicCreationOutcome { + Created, + AlreadyExists, +} + +/// One Kafka-visible topic, as reported by [`IggyBridge::list_kafka_topics`]. +/// +/// Deliberately narrower than the SDK's own `TopicDetails` - `Metadata`'s "all topics" listing +/// needs only the Kafka-side name and a partition count, not every Iggy-internal field. +/// [`IggyBridge::get_kafka_topic`] (a single named lookup) returns the full `TopicDetails` +/// instead; this type exists only because `list_kafka_topics` must carry a *resolved* Kafka-side +/// name for each entry, which `TopicDetails` alone (just the raw Iggy-side name) cannot. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct KafkaTopicMetadata { + pub kafka_topic: String, + pub partitions_count: u32, +} + impl IggyBridge { /// Ensures the Iggy stream and topic backing `kafka_topic` exist, creating either or both if /// missing. Resolves `kafka_topic` through the configured [`TopicMapping`](crate::bridge::topic_map::TopicMapping). @@ -67,13 +98,19 @@ impl IggyBridge { /// topic-naming rules. Returns [`BridgeError::Timeout`] if a call takes longer than /// `REQUEST_TIMEOUT`. Returns [`BridgeError::Iggy`] for connectivity/auth failures. Returns /// [`BridgeError::PartitionCountMismatch`] if the topic already exists with a different - /// partition count than `partition_count`. + /// partition count than `partition_count`. Returns [`BridgeError::InvalidPartitionCount`] if + /// `partition_count` is 0. pub async fn ensure_stream_and_topic( &self, kafka_topic: &str, partition_count: u32, ) -> Result<(), BridgeError> { validate_kafka_topic_name("kafka_topic", kafka_topic)?; + if partition_count == 0 { + return Err(BridgeError::InvalidPartitionCount { + kafka_topic: kafka_topic.to_string(), + }); + } let (stream_name, topic_name) = self.config.topic_mapping.resolve(kafka_topic); let stream_id = self.ensure_stream(stream_name).await?; self.ensure_topic(&stream_id, topic_name, kafka_topic, partition_count) @@ -123,9 +160,10 @@ impl IggyBridge { /// `Identifier::named`, not `Identifier::try_from` - the same numeric-name ambiguity /// [`Self::ensure_stream`]'s doc comment describes for stream names applies to topic names. /// - /// `partition_count == 0` is accepted here as defense in depth, not the primary guard: the - /// server allows it by design (`rewrite.rs`), and the `CreateTopics` stub already rejects it - /// at the wire level (`protocol/responses.rs`) before any bridge call would be reachable. + /// `partition_count == 0` is unreachable here: [`IggyBridge::ensure_stream_and_topic`] rejects + /// it with [`BridgeError::InvalidPartitionCount`] before calling this method. The server + /// itself allows 0 by design (`rewrite.rs`), so this is defense in depth against a future + /// caller of this crate-private method skipping that check, not the primary guard. async fn ensure_topic( &self, stream_id: &Identifier, @@ -217,4 +255,140 @@ impl IggyBridge { Err(err) => Err(err), } } + + /// Looks up `kafka_topic`, resolved through the configured + /// [`TopicMapping`](crate::bridge::topic_map::TopicMapping), without creating it. + /// + /// Returns `Ok(None)` when either the mapped stream or the mapped topic doesn't exist - + /// callers (`CreateTopics`' existence check, `Metadata`'s lookup) treat both the same way: + /// nothing answers to this Kafka-side name yet. + /// + /// # Errors + /// + /// Returns [`BridgeError::InvalidKafkaTopicName`] if `kafka_topic` fails Kafka's own + /// topic-naming rules. Returns [`BridgeError::Timeout`] if a call takes longer than + /// `REQUEST_TIMEOUT`. Returns [`BridgeError::Iggy`] for connectivity/auth failures. + pub async fn get_kafka_topic( + &self, + kafka_topic: &str, + ) -> Result, BridgeError> { + validate_kafka_topic_name("kafka_topic", kafka_topic)?; + let (stream_name, topic_name) = self.config.topic_mapping.resolve(kafka_topic); + let stream_id = Identifier::named(stream_name).map_err(BridgeError::Iggy)?; + let topic_id = Identifier::named(topic_name).map_err(BridgeError::Iggy)?; + // No separate get_stream probe: get_topic already answers Ok(None) when the stream + // itself is missing (see high_watermarks' own doc on this same fact), so a probe first + // would just pay a second round trip to learn something this one call already tells us. + with_request_timeout(self.client.get_topic(&stream_id, &topic_id)).await + } + + /// Creates the Iggy stream/topic backing `kafka_topic`, or reports that it already exists. + /// + /// Atomic from this call's perspective, unlike a separate existence check + /// ([`Self::get_kafka_topic`]) followed by [`Self::ensure_stream_and_topic`]: that sequence + /// has a TOCTOU window between the two calls, and `ensure_stream_and_topic`'s own idempotent + /// contract would then absorb a second concurrent caller's create into a silent `Ok`, so both + /// callers see success for a `CreateTopics` request Kafka promises exactly one `NONE` for. + /// Here, the create attempt itself is the existence check: no separate read precedes it, and + /// [`TopicCreationOutcome::AlreadyExists`] comes from the server's own rejection of the write, + /// not from an earlier read that could already be stale by the time this call's write lands. + /// + /// # Errors + /// + /// Same as [`Self::ensure_stream_and_topic`], except an already-existing topic is reported as + /// [`TopicCreationOutcome::AlreadyExists`] rather than [`BridgeError::PartitionCountMismatch`]. + /// `CreateTopics` is not an upsert, so a pre-existing topic is never itself an error here, + /// regardless of whether its partition count matches `partition_count`. + pub async fn create_kafka_topic( + &self, + kafka_topic: &str, + partition_count: u32, + ) -> Result { + validate_kafka_topic_name("kafka_topic", kafka_topic)?; + if partition_count == 0 { + return Err(BridgeError::InvalidPartitionCount { + kafka_topic: kafka_topic.to_string(), + }); + } + let (stream_name, topic_name) = self.config.topic_mapping.resolve(kafka_topic); + let stream_id = self.ensure_stream(stream_name).await?; + + let options = TopicCreateOptions { + partitions_count: Some(partition_count), + ..TopicCreateOptions::default() + }; + match with_request_timeout(self.client.create_topic(&stream_id, topic_name, &options)).await + { + Ok(created) => { + info!("created Iggy topic '{topic_name}' with {partition_count} partitions"); + // Same postcondition check ensure_topic's own create branch makes: partitions_count + // is a hard argument (Some(partition_count), never None), so a mismatch here means + // a future server-side clamp/cap, not a client input problem - fails loudly instead + // of silently reporting Created under a broken contract. + if created.partitions_count != partition_count { + return Err(BridgeError::PartitionCountMismatch { + topic: kafka_topic.to_string(), + existing: created.partitions_count, + requested: partition_count, + }); + } + Ok(TopicCreationOutcome::Created) + } + Err(BridgeError::Iggy(IggyError::TopicNameAlreadyExists(_, _))) => { + Ok(TopicCreationOutcome::AlreadyExists) + } + Err(err) => Err(err), + } + } + + /// Every Kafka-visible topic: the target of every configured + /// [`TopicMapping`](crate::bridge::topic_map::TopicMapping) override that actually exists in + /// Iggy, plus every topic in the default stream that isn't itself one of those override + /// targets - checked so an overridden topic is never listed twice, once under its Kafka-side + /// name and once under its raw Iggy name. + /// + /// An Iggy stream this bridge has no mapping rule pointing at (neither the default stream nor + /// any override's target) holds data no Kafka client ever named - deliberately excluded, the + /// same way a real Kafka broker never reports storage it doesn't own. + /// + /// # Errors + /// + /// Returns [`BridgeError::Timeout`] if a call takes longer than `REQUEST_TIMEOUT`. Returns + /// [`BridgeError::Iggy`] for connectivity/auth failures. + pub async fn list_kafka_topics(&self) -> Result, BridgeError> { + let default_stream = self.config.topic_mapping.default_stream(); + let mut default_stream_override_targets: HashSet<&str> = HashSet::new(); + let mut results = Vec::new(); + + for (kafka_topic, over) in self.config.topic_mapping.overrides() { + if over.stream == default_stream { + default_stream_override_targets.insert(over.topic.as_str()); + } + if let Some(details) = self.get_kafka_topic(kafka_topic).await? { + results.push(KafkaTopicMetadata { + kafka_topic: kafka_topic.to_string(), + partitions_count: details.partitions_count, + }); + } + } + + let default_stream_id = Identifier::named(default_stream).map_err(BridgeError::Iggy)?; + if with_request_timeout(self.client.get_stream(&default_stream_id)) + .await? + .is_some() + { + let topics = with_request_timeout(self.client.get_topics(&default_stream_id)).await?; + for topic in topics { + if default_stream_override_targets.contains(topic.name.as_str()) { + continue; + } + results.push(KafkaTopicMetadata { + kafka_topic: topic.name, + partitions_count: topic.partitions_count, + }); + } + } + + Ok(results) + } } diff --git a/gateways/kafka/src/bridge/mod.rs b/gateways/kafka/src/bridge/mod.rs index dde4c5e9db..b0eda2fd98 100644 --- a/gateways/kafka/src/bridge/mod.rs +++ b/gateways/kafka/src/bridge/mod.rs @@ -28,5 +28,5 @@ pub mod topic_map; pub use config::IggyBridgeConfig; pub use error::BridgeError; -pub use iggy_bridge::IggyBridge; +pub use iggy_bridge::{IggyBridge, KafkaTopicMetadata, TopicCreationOutcome}; pub use topic_map::{TopicMapping, TopicOverride}; diff --git a/gateways/kafka/src/bridge/topic_map.rs b/gateways/kafka/src/bridge/topic_map.rs index 9802ad5647..66e2ed8084 100644 --- a/gateways/kafka/src/bridge/topic_map.rs +++ b/gateways/kafka/src/bridge/topic_map.rs @@ -250,6 +250,12 @@ impl TopicMapping { &self.default_stream } + /// Every configured `(kafka_topic, override)` pair, for callers that need to enumerate the + /// mapping rather than resolve one name through it (`Metadata`'s "list all topics" case). + pub fn overrides(&self) -> impl Iterator { + self.topics.iter().map(|(k, v)| (k.as_str(), v)) + } + /// Resolves a Kafka topic name to `(iggy_stream, iggy_topic)`. /// /// Not injective: two distinct Kafka topics can resolve to the same Iggy stream/topic pair, diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index adf815eeda..89a36cbb86 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -75,6 +75,12 @@ pub const ERROR_INVALID_REPLICATION_FACTOR: i16 = 38; /// `CreateTopics` stub: do not claim topics were created (no controller / no Iggy bridge). pub const ERROR_NOT_CONTROLLER: i16 = 41; pub const ERROR_INVALID_REQUEST: i16 = 42; +/// `CreateTopics`: a requested topic carried one or more per-topic Kafka configs. +/// +/// None of `retention.ms`, `cleanup.policy`, etc. maps onto an Iggy topic option this bridge +/// applies, so every non-empty `configs` list is rejected outright rather than silently dropping +/// a subset an operator might believe took effect. +pub const ERROR_INVALID_CONFIG: i16 = 40; /// Result of handling one Kafka request body. #[derive(Debug)] diff --git a/gateways/kafka/src/protocol/handlers/create_topics.rs b/gateways/kafka/src/protocol/handlers/create_topics.rs index 19d763476d..c29f8d91e4 100644 --- a/gateways/kafka/src/protocol/handlers/create_topics.rs +++ b/gateways/kafka/src/protocol/handlers/create_topics.rs @@ -17,19 +17,27 @@ //! `CreateTopics` (API key 19). +use std::collections::HashSet; +use std::time::Duration; + use bytes::Bytes; use kafka_protocol::messages::create_topics_request::CreatableTopic; use kafka_protocol::messages::create_topics_response::CreatableTopicResult; -use kafka_protocol::messages::{CreateTopicsRequest, CreateTopicsResponse}; +use kafka_protocol::messages::{CreateTopicsRequest, CreateTopicsResponse, TopicName}; +use kafka_protocol::protocol::StrBytes; +use crate::bridge::{IggyBridge, TopicCreationOutcome}; use crate::error::Result; use crate::protocol::api::{ - API_KEY_CREATE_TOPICS, ApiVersionRange, ERROR_INVALID_PARTITIONS, - ERROR_INVALID_REPLICATION_FACTOR, ERROR_NONE, ERROR_NOT_CONTROLLER, GatewayState, - HandleOutcome, + API_KEY_CREATE_TOPICS, ApiVersionRange, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, + ERROR_INVALID_REPLICATION_FACTOR, ERROR_INVALID_REQUEST, ERROR_NONE, ERROR_NOT_CONTROLLER, + ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, HandleOutcome, }; use crate::protocol::bounds_guard::validate_create_topics_shape; -use crate::protocol::handlers::{decode_guarded, encode_message, handle_versioned_request}; +use crate::protocol::handlers::{ + decode_guarded, encode_message, handle_versioned_request, is_supported_version, + respond_or_close, unsupported_version_response, +}; pub const RANGE: ApiVersionRange = ApiVersionRange { api_key: API_KEY_CREATE_TOPICS, @@ -37,24 +45,296 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { max_version: 5, }; -#[expect( - clippy::unused_async, - reason = "the shared handler signature, kept until a handler awaits the bridge" -)] +/// KIP-464 `num_partitions = -1` with no manual assignment: the count this bridge creates. +/// +/// Matches real Kafka's own out-of-box `num.partitions=1` broker default. Not read from any +/// bridge config - there is no such config surface today. +const DEFAULT_PARTITION_COUNT: u32 = 1; + +/// Cap on distinct topic names one `CreateTopics` request may address through the bridge. +/// +/// `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode `DoS` ceiling, not a usability +/// recommendation: each non-duplicate requested name here costs up to ~4 Iggy round trips +/// (`ensure_stream` + `create_topic`, plus a possible race-retry read on either) against the +/// single lockstep `IggyClient` every Kafka connection on this gateway shares +/// (`bridge/iggy_bridge/mod.rs`'s "Concurrency ceiling"). 100 keeps a worst-case batch's +/// aggregate bridge cost small relative to that shared resource while remaining generous for any +/// real admin batch. Duplicate names never count against this cap - they're rejected by +/// [`find_duplicate_names`] before ever reaching the bridge. +const MAX_BRIDGE_BACKED_TOPICS: usize = 100; + +/// Bounds imposed on the request's own `timeout_ms` before it becomes the aggregate bridge-work +/// deadline. That value is client-supplied and otherwise unchecked: `0` or negative would abort +/// every topic on arrival, and an oversized one would tie up the shared `IggyClient` past any +/// reasonable request. +const MIN_REQUEST_TIMEOUT: Duration = Duration::from_millis(1_000); +const MAX_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); + +/// Clamps the wire's own `timeout_ms` (KIP-4's field for exactly this) into +/// `[MIN_REQUEST_TIMEOUT, MAX_REQUEST_TIMEOUT]` - unlike `ListOffsets`/`Metadata`, `CreateTopics` +/// carries a real client-supplied deadline to honor, not just a fixed internal ceiling. +fn clamp_request_timeout(timeout_ms: i32) -> Duration { + let requested = Duration::from_millis(u64::try_from(timeout_ms).unwrap_or(0)); + requested.clamp(MIN_REQUEST_TIMEOUT, MAX_REQUEST_TIMEOUT) +} + pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> HandleOutcome { - handle_versioned_request( - API_KEY_CREATE_TOPICS, - api_version, - body, - |v, b| { - decode_guarded::(v, b, |v, b| { - validate_create_topics_shape(v, b, state.max_frame_size) + let Some(bridge) = &state.bridge else { + return handle_versioned_request( + API_KEY_CREATE_TOPICS, + api_version, + body, + |v, b| { + decode_guarded::(v, b, |v, b| { + validate_create_topics_shape(v, b, state.max_frame_size) + }) + }, + encode_response, + encode_error_response, + "CreateTopics", + ); + }; + + if !is_supported_version(API_KEY_CREATE_TOPICS, api_version) { + return unsupported_version_response(API_KEY_CREATE_TOPICS, api_version, |version| { + encode_error_response(version, ERROR_INVALID_REQUEST) + }); + } + + let req = match decode_guarded::(api_version, body, |v, b| { + validate_create_topics_shape(v, b, state.max_frame_size) + }) { + Ok(req) => req, + Err(error) => { + // debug!, not warn!: attacker-controlled, not operator-actionable. + tracing::debug!(%error, "Failed to decode CreateTopics request"); + return respond_or_close( + encode_error_response(api_version, ERROR_INVALID_REQUEST), + "CreateTopics", + ); + } + }; + + let duplicate_names = find_duplicate_names(&req.topics); + + let distinct_bridge_backed: HashSet<&TopicName> = req + .topics + .iter() + .map(|topic| &topic.name) + .filter(|name| !duplicate_names.contains(*name)) + .collect(); + if distinct_bridge_backed.len() > MAX_BRIDGE_BACKED_TOPICS { + tracing::warn!( + distinct_topics = distinct_bridge_backed.len(), + max = MAX_BRIDGE_BACKED_TOPICS, + "CreateTopics request addresses too many distinct topics; rejecting" + ); + let results = req + .topics + .iter() + .map(|topic| { + CreatableTopicResult::default() + .with_name(topic.name.clone()) + .with_error_code(ERROR_INVALID_REQUEST) }) - }, - encode_response, - encode_error_response, - "CreateTopics", + .collect(); + let resp = CreateTopicsResponse::default().with_topics(results); + return respond_or_close(encode_message(&resp, api_version, 256), "CreateTopics"); + } + + let deadline = clamp_request_timeout(req.timeout_ms); + let results = match tokio::time::timeout( + deadline, + create_all_topics( + bridge, + api_version, + &req.topics, + &duplicate_names, + req.validate_only, + ), ) + .await + { + Ok(results) => results, + Err(_elapsed) => { + tracing::warn!( + distinct_topics = distinct_bridge_backed.len(), + deadline_ms = deadline.as_millis(), + "CreateTopics request's aggregate bridge work exceeded its deadline; \ + answering retriable instead of blocking further" + ); + req.topics + .iter() + .map(|topic| { + CreatableTopicResult::default() + .with_name(topic.name.clone()) + .with_error_code(ERROR_REQUEST_TIMED_OUT) + }) + .collect() + } + }; + let resp = CreateTopicsResponse::default().with_topics(results); + respond_or_close(encode_message(&resp, api_version, 256), "CreateTopics") +} + +/// Creates (or reports on) every requested topic, skipping the bridge entirely for a duplicate +/// name - real Kafka refuses the whole name, not a first-wins/last-wins split: creating one +/// occurrence and reporting `TOPIC_ALREADY_EXISTS` for the other would let a client observe a +/// create it never got a `NONE` for (`AdminClient` keys its futures by name, so a second +/// per-topic result for the same name is silently discarded client-side regardless of which one +/// this bridge picked). +async fn create_all_topics( + bridge: &IggyBridge, + api_version: i16, + topics: &[CreatableTopic], + duplicate_names: &HashSet, + validate_only: bool, +) -> Vec { + let mut results = Vec::with_capacity(topics.len()); + for topic in topics { + let result = if duplicate_names.contains(&topic.name) { + CreatableTopicResult::default() + .with_name(topic.name.clone()) + .with_error_code(ERROR_INVALID_REQUEST) + } else { + create_one_topic(bridge, api_version, topic, validate_only).await + }; + results.push(result); + } + results +} + +/// Every topic name that appears more than once in `topics` - real Kafka +/// (`ControllerApis.createTopics`) refuses every occurrence of a duplicate name with +/// `INVALID_REQUEST` (42) and creates nothing for it, rather than creating the first occurrence +/// and reporting the rest as already existing. +fn find_duplicate_names(topics: &[CreatableTopic]) -> HashSet { + let mut seen = HashSet::with_capacity(topics.len()); + let mut duplicates = HashSet::new(); + for topic in topics { + if !seen.insert(topic.name.clone()) { + duplicates.insert(topic.name.clone()); + } + } + duplicates +} + +/// Validates and, when the topic is not rejected outright, provisions one requested topic. +/// +/// `configs` is rejected before the shape check - a config-bearing request is rejected the same +/// way regardless of how its partitions/replication are shaped. +/// +/// `validate_only` and the real create path diverge deliberately below that point, not just in +/// whether they call the bridge: `validate_only` never mutates anything, so a plain existence +/// read ([`IggyBridge::get_kafka_topic`]) is fine - there's no race to protect against when +/// nothing gets created either way. The real path instead calls +/// [`IggyBridge::create_kafka_topic`], which folds the existence check and the create into one +/// atomic call - a separate read-then-write here would let two concurrent `CreateTopics` for the +/// same new name both observe `Ok(None)` and both receive `NONE`, when Kafka guarantees exactly +/// one caller does. +async fn create_one_topic( + bridge: &IggyBridge, + version: i16, + topic: &CreatableTopic, + validate_only: bool, +) -> CreatableTopicResult { + let result = CreatableTopicResult::default().with_name(topic.name.clone()); + + if !topic.configs.is_empty() { + return result + .with_error_code(ERROR_INVALID_CONFIG) + .with_error_message(Some(StrBytes::from( + "per-topic configs are not supported by this bridge".to_string(), + ))); + } + + let partition_count = match validate_create_topic_shape(version, topic) { + Ok(count) => count, + Err(code) => return result.with_error_code(code), + }; + + let kafka_topic = topic.name.as_str(); + let success = || { + result + .clone() + .with_error_code(ERROR_NONE) + .with_num_partitions(i32::try_from(partition_count).unwrap_or(i32::MAX)) + .with_replication_factor(1) + }; + + if validate_only { + return match bridge.get_kafka_topic(kafka_topic).await { + Ok(Some(_existing)) => result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS), + Ok(None) => success(), + Err(err) => result + .with_error_code(err.to_kafka_error_code()) + .with_error_message(Some(StrBytes::from(err.to_string()))), + }; + } + + match bridge + .create_kafka_topic(kafka_topic, partition_count) + .await + { + Ok(TopicCreationOutcome::Created) => success(), + Ok(TopicCreationOutcome::AlreadyExists) => { + result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS) + } + Err(err) => result + .with_error_code(err.to_kafka_error_code()) + .with_error_message(Some(StrBytes::from(err.to_string()))), + } +} + +/// Validates one requested topic's KIP-464 shape and resolves its partition count. +/// +/// A manual partition `assignments` list and an explicit `num_partitions`/`replication_factor` +/// are mutually exclusive inputs, not two independently-checked values that happen to agree: real +/// Kafka's own `ReplicationControlManager` rejects a manual assignment unless both are exactly +/// `-1`, regardless of whether an explicit `num_partitions` matches `assignments.len()`. A count +/// that only *disagrees* with the assignment length is not a distinct, more lenient case - the +/// combination itself is what's invalid, so both are `INVALID_REQUEST` (42), never `NONE`. +/// +/// With no assignments, `num_partitions = -1` / `replication_factor = -1` mean "use the broker +/// default" from v4+ (pre-v4 requires an explicit positive value for both, since v2/v3 have no +/// broker-default sentinel absent a manual assignment). +fn validate_create_topic_shape( + version: i16, + topic: &CreatableTopic, +) -> core::result::Result { + if !topic.assignments.is_empty() { + if topic.num_partitions != -1 || topic.replication_factor != -1 { + return Err(ERROR_INVALID_REQUEST); + } + return Ok(u32::try_from(topic.assignments.len()).unwrap_or(DEFAULT_PARTITION_COUNT)); + } + + let broker_default_ok = version >= 4; + + let partitions_ok = if broker_default_ok { + topic.num_partitions == -1 || topic.num_partitions > 0 + } else { + topic.num_partitions > 0 + }; + if !partitions_ok { + return Err(ERROR_INVALID_PARTITIONS); + } + + let replication_ok = if broker_default_ok { + topic.replication_factor == -1 || topic.replication_factor > 0 + } else { + topic.replication_factor > 0 + }; + if !replication_ok { + return Err(ERROR_INVALID_REPLICATION_FACTOR); + } + + let partition_count = if topic.num_partitions > 0 { + u32::try_from(topic.num_partitions).unwrap_or(DEFAULT_PARTITION_COUNT) + } else { + DEFAULT_PARTITION_COUNT + }; + Ok(partition_count) } /// Well-formed `CreateTopics` response with a single placeholder topic. @@ -78,7 +358,7 @@ pub fn encode_response(version: i16, req: &CreateTopicsRequest) -> Result encode_inner(version, &req.topics, ERROR_NONE) } -/// Resolve per-topic `CreateTopics` error. +/// Resolve per-topic `CreateTopics` error for the stub (no-bridge) path. /// /// KIP-464: `num_partitions = -1` / `replication_factor = -1` mean broker default when either /// (a) the version is v4+, or (b) the topic carries a manual partition assignment (valid on @@ -128,3 +408,144 @@ fn encode_inner(version: i16, topics: &[CreatableTopic], forced_error: i16) -> R let resp = CreateTopicsResponse::default().with_topics(results); encode_message(&resp, version, 256) } + +#[cfg(test)] +mod tests { + use kafka_protocol::messages::create_topics_request::CreatableReplicaAssignment; + + use super::*; + + fn topic_name(name: &str) -> TopicName { + TopicName(StrBytes::from_string(name.to_string())) + } + + fn creatable_topic(num_partitions: i32, replication_factor: i16) -> CreatableTopic { + CreatableTopic::default() + .with_name(topic_name("orders")) + .with_num_partitions(num_partitions) + .with_replication_factor(replication_factor) + } + + fn assignment(partition_index: i32) -> CreatableReplicaAssignment { + CreatableReplicaAssignment::default() + .with_partition_index(partition_index) + .with_broker_ids(vec![0.into()]) + } + + #[test] + fn explicit_partitions_and_replication_resolve_as_given() { + let topic = creatable_topic(3, 1); + assert_eq!(validate_create_topic_shape(2, &topic), Ok(3)); + } + + #[test] + fn v4_plus_accepts_broker_default_sentinel_with_no_assignments() { + let topic = creatable_topic(-1, -1); + assert_eq!( + validate_create_topic_shape(4, &topic), + Ok(DEFAULT_PARTITION_COUNT) + ); + } + + #[test] + fn pre_v4_rejects_broker_default_sentinel_with_no_assignments() { + let topic = creatable_topic(-1, -1); + assert_eq!( + validate_create_topic_shape(2, &topic), + Err(ERROR_INVALID_PARTITIONS) + ); + } + + #[test] + fn pre_v4_accepts_broker_default_sentinel_with_a_manual_assignment() { + let topic = creatable_topic(-1, -1).with_assignments(vec![assignment(0), assignment(1)]); + assert_eq!(validate_create_topic_shape(2, &topic), Ok(2)); + } + + #[test] + fn zero_partitions_is_rejected_regardless_of_version() { + let topic = creatable_topic(0, 1); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_PARTITIONS) + ); + } + + #[test] + fn zero_replication_factor_is_rejected_regardless_of_version() { + let topic = creatable_topic(1, 0); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REPLICATION_FACTOR) + ); + } + + #[test] + fn explicit_num_partitions_with_assignments_is_rejected_even_when_it_agrees_with_their_length() + { + // Real Kafka rejects an explicit num_partitions alongside a manual assignment outright - + // a manual assignment requires num_partitions == -1, full stop. Agreeing with + // assignments.len() does not make the combination valid; there is no wire rule saying + // which one would win if it did. + let topic = creatable_topic(2, 1).with_assignments(vec![assignment(0), assignment(1)]); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REQUEST) + ); + } + + #[test] + fn explicit_num_partitions_disagreeing_with_assignments_length_is_also_rejected() { + let topic = creatable_topic(3, 1).with_assignments(vec![assignment(0), assignment(1)]); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REQUEST) + ); + } + + #[test] + fn explicit_replication_factor_with_assignments_is_rejected_even_with_num_partitions_at_minus_one() + { + let topic = creatable_topic(-1, 1).with_assignments(vec![assignment(0), assignment(1)]); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REQUEST) + ); + } + + #[test] + fn find_duplicate_names_finds_a_name_repeated_across_two_requested_topics() { + let topics = vec![creatable_topic(1, 1), creatable_topic(1, 1)]; + let duplicates = find_duplicate_names(&topics); + assert_eq!(duplicates, HashSet::from([topic_name("orders")])); + } + + #[test] + fn find_duplicate_names_is_empty_when_every_name_is_unique() { + let topics = vec![ + creatable_topic(1, 1), + CreatableTopic::default() + .with_name(topic_name("payments")) + .with_num_partitions(1) + .with_replication_factor(1), + ]; + assert!(find_duplicate_names(&topics).is_empty()); + } + + #[test] + fn clamp_request_timeout_rejects_a_zero_or_negative_value_up_to_the_floor() { + assert_eq!(clamp_request_timeout(0), MIN_REQUEST_TIMEOUT); + assert_eq!(clamp_request_timeout(-1), MIN_REQUEST_TIMEOUT); + assert_eq!(clamp_request_timeout(i32::MIN), MIN_REQUEST_TIMEOUT); + } + + #[test] + fn clamp_request_timeout_caps_an_oversized_value_at_the_ceiling() { + assert_eq!(clamp_request_timeout(i32::MAX), MAX_REQUEST_TIMEOUT); + } + + #[test] + fn clamp_request_timeout_passes_through_a_reasonable_value_unchanged() { + assert_eq!(clamp_request_timeout(5_000), Duration::from_secs(5)); + } +} diff --git a/gateways/kafka/src/protocol/handlers/metadata.rs b/gateways/kafka/src/protocol/handlers/metadata.rs index 7a4cc29a98..934f3eee25 100644 --- a/gateways/kafka/src/protocol/handlers/metadata.rs +++ b/gateways/kafka/src/protocol/handlers/metadata.rs @@ -17,16 +17,22 @@ //! Metadata (API key 3). +use std::collections::{HashMap, HashSet}; +use std::time::Duration; + use bytes::Bytes; -use kafka_protocol::messages::metadata_response::{MetadataResponseBroker, MetadataResponseTopic}; +use kafka_protocol::messages::metadata_response::{ + MetadataResponseBroker, MetadataResponsePartition, MetadataResponseTopic, +}; use kafka_protocol::messages::{BrokerId, MetadataRequest, MetadataResponse, TopicName}; use kafka_protocol::protocol::StrBytes; +use crate::bridge::IggyBridge; use crate::error::{KafkaProtocolError, Result}; use crate::protocol::api::{ - API_KEY_METADATA, ApiVersionRange, BrokerAdvertise, ERROR_NONE, - ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, HandleOutcome, is_supported_version, - supported_max_version, + API_KEY_METADATA, ApiVersionRange, BrokerAdvertise, ERROR_INVALID_REQUEST, ERROR_NONE, + ERROR_REQUEST_TIMED_OUT, ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, HandleOutcome, + is_supported_version, supported_max_version, }; use crate::protocol::bounds_guard::validate_metadata_shape; use crate::protocol::handlers::{decode_guarded, encode_message, respond_or_close}; @@ -37,10 +43,24 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { max_version: 9, }; -#[expect( - clippy::unused_async, - reason = "the shared handler signature, kept until a handler awaits the bridge" -)] +/// Cap on distinct topic names one named-lookup `Metadata` request may address through the +/// bridge. Does not apply to a null topics array ("all topics") - that path is server-driven +/// (bounded by [`response_would_exceed_frame_size`] instead), not client-count-driven. +/// +/// `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode `DoS` ceiling, not a usability +/// recommendation: each distinct name costs one `get_kafka_topic` round trip against the single +/// lockstep `IggyClient` every Kafka connection on this gateway shares +/// (`bridge/iggy_bridge/mod.rs`'s "Concurrency ceiling"). +const MAX_BRIDGE_BACKED_TOPICS: usize = 100; + +/// Wall-clock ceiling for a named-lookup request's aggregate bridge work. +/// +/// `Metadata` carries no `timeout_ms` field in any version (unlike `CreateTopics`), so this is a +/// fixed ceiling, not a client-honored one - sized well above one `get_kafka_topic` call's own +/// `REQUEST_TIMEOUT` (15s, bridge-internal) so a single slow-but-alive call is not the common +/// trigger, while still bounding the sum across up to [`MAX_BRIDGE_BACKED_TOPICS`] calls. +const REQUEST_DEADLINE: Duration = Duration::from_secs(20); + pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> HandleOutcome { if !is_supported_version(API_KEY_METADATA, api_version) { // Clamping the response to the supported max leaves a body the client parses at its own @@ -53,23 +73,203 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand ); return HandleOutcome::Close; } - match decode_topics(api_version, body, state.max_frame_size) { - Ok(topics) => respond_or_close( - encode_response(api_version, &topics, &state.broker, ERROR_NONE), - "Metadata", - ), + + let Some(bridge) = &state.bridge else { + return match decode_topics(api_version, body, state.max_frame_size) { + Ok(topics) => respond_or_close( + encode_response(api_version, &topics, &state.broker, ERROR_NONE), + "Metadata", + ), + Err(error) => { + // Metadata has no top-level error field; a malformed body cannot carry + // INVALID_REQUEST in a version-correct way for every client. Close. + // debug!, not warn!: attacker-controlled, not operator-actionable. + tracing::debug!( + %error, + api_version, + "Failed to decode Metadata request; closing connection" + ); + HandleOutcome::Close + } + }; + }; + + let requested = match decode_requested_topics(api_version, body, state.max_frame_size) { + Ok(requested) => requested, Err(error) => { - // Metadata has no top-level error field; a malformed body cannot carry - // INVALID_REQUEST in a version-correct way for every client. Close. - // debug!, not warn!: attacker-controlled, not operator-actionable. tracing::debug!( %error, api_version, "Failed to decode Metadata request; closing connection" ); - HandleOutcome::Close + return HandleOutcome::Close; + } + }; + + let results = match requested { + None => match bridge.list_kafka_topics().await { + Ok(topics) => topics.into_iter().map(found_result).collect(), + Err(error) => { + // Same "no top-level error field" constraint as a decode failure: there is no + // way to answer "the bridge itself is unreachable" for an all-topics request + // that doesn't also falsely claim zero topics exist. + tracing::warn!(%error, "Failed to list Kafka topics from the Iggy bridge; closing connection"); + return HandleOutcome::Close; + } + }, + Some(names) => { + let distinct_names: HashSet<&str> = names.iter().map(StrBytes::as_str).collect(); + if distinct_names.len() > MAX_BRIDGE_BACKED_TOPICS { + tracing::warn!( + distinct_topics = distinct_names.len(), + max = MAX_BRIDGE_BACKED_TOPICS, + "Metadata request addresses too many distinct topics; rejecting" + ); + names + .iter() + .map(|name| error_result(name.clone(), ERROR_INVALID_REQUEST)) + .collect() + } else { + match tokio::time::timeout(REQUEST_DEADLINE, resolve_named_topics(bridge, &names)) + .await + { + Ok(results) => results, + Err(_elapsed) => { + tracing::warn!( + distinct_topics = distinct_names.len(), + deadline_secs = REQUEST_DEADLINE.as_secs(), + "Metadata request's aggregate bridge work exceeded its deadline; \ + answering retriable instead of blocking further" + ); + names + .iter() + .map(|name| error_result(name.clone(), ERROR_REQUEST_TIMED_OUT)) + .collect() + } + } + } } + }; + + // `bounds_guard` cannot see this: it charges the projected response by *requested* element + // (one topic name), but one real topic can carry up to Iggy's own per-topic partition cap + // (1000) - a handful of names, or one `list_kafka_topics()` call, can still expand into a + // response `bounds_guard` never had the information to price in before this bridge round + // trip returned. Checked here, before `encode_real_response` builds one + // `MetadataResponsePartition` per partition, not after - the expensive part is building that + // `Vec`, not encoding the bytes that follow it. + let total_partitions: usize = results + .iter() + .filter(|result| result.error_code == ERROR_NONE) + .map(|result| result.partitions_count as usize) + .sum(); + if response_would_exceed_frame_size(total_partitions, state.max_frame_size) { + tracing::warn!( + total_partitions, + max_frame_size = state.max_frame_size, + "Metadata response would exceed max_frame_size; closing connection" + ); + return HandleOutcome::Close; } + + respond_or_close( + encode_real_response(api_version, &results, &state.broker), + "Metadata", + ) +} + +/// Conservative per-partition byte cost of one encoded `MetadataResponsePartition` at v9 (the +/// densest wire shape this handler emits): measured ~26 bytes (`error_code` + `partition_index` + +/// `leader_id` + `leader_epoch` + a 1-entry `replica_nodes` + a 1-entry `isr_nodes` + an empty +/// `offline_replicas` + tagged fields). 64 matches the margin `bounds_guard`'s own +/// `RESPONSE_BYTES_PER_ELEMENT` uses for the same kind of estimate, rather than shaving this to +/// the measured minimum. +const RESPONSE_BYTES_PER_PARTITION: usize = 64; + +const fn response_would_exceed_frame_size(total_partitions: usize, max_frame_size: usize) -> bool { + total_partitions.saturating_mul(RESPONSE_BYTES_PER_PARTITION) > max_frame_size +} + +/// One resolved topic result for the real (bridge-backed) path - `partitions_count` is +/// meaningless when `error_code != ERROR_NONE`. +struct TopicResult { + name: StrBytes, + error_code: i16, + partitions_count: u32, +} + +fn found_result(metadata: crate::bridge::KafkaTopicMetadata) -> TopicResult { + TopicResult { + name: StrBytes::from_string(metadata.kafka_topic), + error_code: ERROR_NONE, + partitions_count: metadata.partitions_count, + } +} + +async fn lookup_one_topic(bridge: &IggyBridge, name: StrBytes) -> TopicResult { + match bridge.get_kafka_topic(&name).await { + // `get_kafka_topic` (unlike `list_kafka_topics`) returns the SDK's own `TopicDetails`, + // which carries the topic's raw Iggy-side name, not the Kafka-side one under an override + // - so this echoes the caller's own `name`, not a field off the result. + Ok(Some(details)) => TopicResult { + name, + error_code: ERROR_NONE, + partitions_count: details.partitions_count, + }, + Ok(None) => TopicResult { + name, + error_code: ERROR_UNKNOWN_TOPIC_OR_PARTITION, + partitions_count: 0, + }, + Err(err) => TopicResult { + name, + error_code: err.to_kafka_error_code(), + partitions_count: 0, + }, + } +} + +const fn error_result(name: StrBytes, error_code: i16) -> TopicResult { + TopicResult { + name, + error_code, + partitions_count: 0, + } +} + +/// Resolves every requested name, deduping first so a name repeated in the request (or asked +/// about more than once, which the wire technically allows) costs one `get_kafka_topic` round +/// trip, not one per occurrence. +async fn resolve_named_topics(bridge: &IggyBridge, names: &[StrBytes]) -> Vec { + let mut seen = HashSet::with_capacity(names.len()); + let mut distinct = Vec::new(); + for name in names { + if seen.insert(name.as_str()) { + distinct.push(name.clone()); + } + } + + let mut results_by_name: HashMap<&str, TopicResult> = HashMap::with_capacity(distinct.len()); + for name in &distinct { + let result = lookup_one_topic(bridge, name.clone()).await; + results_by_name.insert(name.as_str(), result); + } + + names + .iter() + .map(|name| { + // Always present: `distinct` (and so results_by_name) was built from exactly these + // same requested names, just above. + let cached = results_by_name + .get(name.as_str()) + .expect("every requested name was resolved above"); + TopicResult { + name: name.clone(), + error_code: cached.error_code, + partitions_count: cached.partitions_count, + } + }) + .collect() } /// # Errors @@ -111,6 +311,51 @@ pub fn encode_response( encode_message(&resp, response_version, 256) } +/// Real (bridge-backed) response: unlike [`encode_response`], each topic carries its own +/// resolved error code and, on success, one [`MetadataResponsePartition`] per partition with +/// this gateway's single broker (node id 1) as leader/replica/ISR - there is only ever one +/// broker behind this gateway, so that triple is never actually in question. +fn encode_real_response( + response_version: i16, + results: &[TopicResult], + broker: &BrokerAdvertise, +) -> Result { + let response_topics = results + .iter() + .map(|result| { + let partitions = if result.error_code == ERROR_NONE { + (0..result.partitions_count) + .map(|index| { + MetadataResponsePartition::default() + .with_partition_index(i32::try_from(index).unwrap_or(i32::MAX)) + .with_leader_id(BrokerId(1)) + .with_replica_nodes(vec![BrokerId(1)]) + .with_isr_nodes(vec![BrokerId(1)]) + }) + .collect() + } else { + Vec::new() + }; + MetadataResponseTopic::default() + .with_error_code(result.error_code) + .with_name(Some(TopicName(result.name.clone()))) + .with_partitions(partitions) + }) + .collect(); + + let broker_entry = MetadataResponseBroker::default() + .with_node_id(BrokerId(1)) + .with_host(StrBytes::from_string(broker.host.clone())) + .with_port(broker.port); + + let resp = MetadataResponse::default() + .with_brokers(vec![broker_entry]) + .with_controller_id(BrokerId(1)) + .with_topics(response_topics); + + encode_message(&resp, response_version, 256) +} + /// Decodes a Metadata request body so the response can echo topic names. /// /// A null topics array (`-1` legacy / `varint=0` compact) means "all topics" and decodes to an @@ -132,6 +377,43 @@ fn decode_topics(api_version: i16, body: Bytes, max_frame_size: usize) -> Result .collect() } +/// Like [`decode_topics`], but for the real (bridge-backed) path, which must tell apart what +/// `decode_topics`' `unwrap_or_default()` deliberately collapses: a null topics array (`None`, +/// "all topics") from an explicit, merely empty one (`Some(vec![])`, "these zero topics") - the +/// stub has no topic catalog to answer either request differently, but the real path does. +/// +/// At `api_version == 0` an explicit empty array is folded into `None` too: Kafka's own rule +/// (`MetadataRequest.isAllTopics()`) is `topics == null || (topics.isEmpty() && version == 0)` - +/// there is no v0 wire shape for "cluster info only, zero topics" (that distinct shape, KIP-4's +/// `describeCluster()`, starts at v1), so an empty array at v0 can only mean "all topics." +fn decode_requested_topics( + api_version: i16, + body: Bytes, + max_frame_size: usize, +) -> Result>> { + let req = decode_guarded::(api_version, body, |v, b| { + validate_metadata_shape(v, b, max_frame_size) + })?; + let requested = req + .topics + .map(|topics| { + topics + .into_iter() + .map(|topic| { + topic + .name + .map(|name| name.0) + .ok_or(KafkaProtocolError::NullTopicName) + }) + .collect::>>() + }) + .transpose()?; + Ok(match requested { + Some(topics) if topics.is_empty() && api_version == 0 => None, + other => other, + }) +} + #[cfg(test)] mod tests { use super::*; @@ -168,4 +450,69 @@ mod tests { let body = Bytes::from_static(&[0x00]); assert!(decode_topics(9, body, TEST_MAX_FRAME_SIZE).is_err()); } + + #[test] + fn decode_requested_topics_legacy_null_array_is_none_not_some_empty() { + // The exact distinction decode_topics' unwrap_or_default() collapses: -1 must decode to + // None ("all topics"), not Some(vec![]) ("these zero topics"). + let body = Bytes::from_static(&[0xff, 0xff, 0xff, 0xff]); // -1 + let requested = decode_requested_topics(0, body, TEST_MAX_FRAME_SIZE).unwrap(); + assert_eq!(requested, None); + } + + #[test] + fn decode_requested_topics_v1_explicit_empty_array_is_some_empty_not_none() { + // v1+, unlike v0 (see the next test): an explicit empty array really does mean "these + // zero topics" (KIP-4's describeCluster() shape). + let body = Bytes::from_static(&[0x00, 0x00, 0x00, 0x00]); // 0 topics, not -1 + let requested = decode_requested_topics(1, body, TEST_MAX_FRAME_SIZE).unwrap(); + assert_eq!(requested, Some(Vec::new())); + } + + #[test] + fn decode_requested_topics_v0_explicit_empty_array_means_all_topics_too() { + // Kafka's own isAllTopics(): topics == null || (topics.isEmpty() && version == 0). No v0 + // client can express "cluster info only, zero topics" - that shape starts at v1. + let body = Bytes::from_static(&[0x00, 0x00, 0x00, 0x00]); // 0 topics, not -1 + let requested = decode_requested_topics(0, body, TEST_MAX_FRAME_SIZE).unwrap(); + assert_eq!(requested, None); + } + + #[test] + fn decode_requested_topics_legacy_named_topic_is_some_with_that_name() { + let body = Bytes::from_static(&[ + 0x00, 0x00, 0x00, 0x01, // one topic + 0x00, 0x06, b'o', b'r', b'd', b'e', b'r', b's', // "orders" + ]); + let requested = decode_requested_topics(0, body, TEST_MAX_FRAME_SIZE).unwrap(); + assert_eq!(requested, Some(vec![StrBytes::from_static_str("orders")])); + } + + #[test] + fn response_would_exceed_frame_size_rejects_a_projection_over_the_limit() { + // bounds_guard cannot see this cost: one requested topic name can expand into up to + // Iggy's own per-topic partition cap (1000) worth of MetadataResponsePartition entries, + // information only known after the bridge round trip this check runs after. + let max_frame_size = 1024; + let total_partitions = (max_frame_size / RESPONSE_BYTES_PER_PARTITION) + 1; + assert!(response_would_exceed_frame_size( + total_partitions, + max_frame_size + )); + } + + #[test] + fn response_would_exceed_frame_size_accepts_a_projection_at_or_under_the_limit() { + let max_frame_size = 1024; + let total_partitions = max_frame_size / RESPONSE_BYTES_PER_PARTITION; + assert!(!response_would_exceed_frame_size( + total_partitions, + max_frame_size + )); + } + + #[test] + fn response_would_exceed_frame_size_does_not_overflow_on_a_pathological_partition_count() { + assert!(response_would_exceed_frame_size(usize::MAX, 1024)); + } } diff --git a/gateways/kafka/tests/bridge_iggy_integration_tests.rs b/gateways/kafka/tests/bridge_iggy_integration_tests.rs index 286f58ad71..45ee41ac3e 100644 --- a/gateways/kafka/tests/bridge_iggy_integration_tests.rs +++ b/gateways/kafka/tests/bridge_iggy_integration_tests.rs @@ -637,3 +637,270 @@ async fn high_watermark_rejects_a_padded_kafka_topic_name() { .expect_err("a padded Kafka topic name must be rejected before any Iggy lookup"); assert!(matches!(err, BridgeError::InvalidKafkaTopicName { .. })); } + +#[tokio::test] +#[serial] +async fn get_kafka_topic_returns_none_when_neither_stream_nor_topic_exists() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + let found = bridge + .get_kafka_topic("orders") + .await + .expect("lookup against a nonexistent stream must not error"); + assert!(found.is_none()); +} + +#[tokio::test] +#[serial] +async fn get_kafka_topic_returns_none_when_the_stream_exists_but_the_topic_does_not() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + // Creates the mapped stream ("kafka", the default) without the "orders" topic in it, so the + // stream-exists / topic-missing branch is reachable independently of the neither-exists one. + bridge + .ensure_stream_and_topic("different-topic", 1) + .await + .expect("seed a different topic under the same default stream"); + + let found = bridge + .get_kafka_topic("orders") + .await + .expect("lookup against an existing stream with no matching topic must not error"); + assert!(found.is_none()); +} + +#[tokio::test] +#[serial] +async fn get_kafka_topic_finds_a_topic_created_through_ensure_stream_and_topic() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + bridge + .ensure_stream_and_topic("orders", 3) + .await + .expect("seed the topic this lookup should find"); + + let found = bridge + .get_kafka_topic("orders") + .await + .expect("lookup call") + .expect("topic was just created, must be found"); + assert_eq!(found.name, "orders"); + assert_eq!(found.partitions_count, 3); +} + +#[tokio::test] +#[serial] +async fn get_kafka_topic_reports_the_kafka_side_name_through_a_real_topic_mapping_override() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let mut config = server.test_config(); + let mut topics = HashMap::new(); + topics.insert( + "orders".to_string(), + TopicOverride { + stream: "commerce".to_string(), + topic: "orders-v2".to_string(), + }, + ); + config.topic_mapping = TopicMapping::new("kafka".to_string(), topics) + .expect("valid mapping for this test's fixture data"); + let bridge = IggyBridge::connect(config) + .await + .expect("bridge should connect to a ready server"); + + bridge + .ensure_stream_and_topic("orders", 2) + .await + .expect("seed the mapped stream/topic"); + + let found = bridge + .get_kafka_topic("orders") + .await + .expect("lookup call") + .expect("resolves through the override to the real Iggy topic"); + // The Iggy-side name under the override ("orders-v2"), not the Kafka-side name asked about - + // TopicDetails carries only what the server itself knows the topic as. + assert_eq!(found.name, "orders-v2"); +} + +#[tokio::test] +#[serial] +async fn ensure_stream_and_topic_rejects_zero_partitions() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + let err = bridge + .ensure_stream_and_topic("orders", 0) + .await + .expect_err("zero partitions must not provision an unproducible topic"); + + assert_eq!( + err.to_kafka_error_code(), + iggy_gateway_kafka::protocol::api::ERROR_INVALID_PARTITIONS + ); + match err { + BridgeError::InvalidPartitionCount { kafka_topic } => { + assert_eq!(kafka_topic, "orders"); + } + other => panic!("expected InvalidPartitionCount, got {other:?}"), + } + + // No stream must have been created either - rejected before ensure_stream runs. + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!( + streams.is_empty(), + "zero-partition request must not leave a dangling stream behind" + ); +} + +#[tokio::test] +#[serial] +async fn list_kafka_topics_is_empty_when_nothing_exists() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + let topics = bridge + .list_kafka_topics() + .await + .expect("listing against a nonexistent default stream must not error"); + assert!(topics.is_empty()); +} + +#[tokio::test] +#[serial] +async fn list_kafka_topics_lists_every_default_stream_topic() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + + bridge + .ensure_stream_and_topic("orders", 3) + .await + .expect("seed orders"); + bridge + .ensure_stream_and_topic("payments", 1) + .await + .expect("seed payments"); + + let mut topics = bridge.list_kafka_topics().await.expect("list call"); + topics.sort_by(|a, b| a.kafka_topic.cmp(&b.kafka_topic)); + assert_eq!(topics.len(), 2); + assert_eq!(topics[0].kafka_topic, "orders"); + assert_eq!(topics[0].partitions_count, 3); + assert_eq!(topics[1].kafka_topic, "payments"); + assert_eq!(topics[1].partitions_count, 1); +} + +/// Regression test: an override's target Iggy topic, when it lives in the default stream, must +/// be listed exactly once - under its Kafka-side (override) name - not a second time under its +/// raw Iggy name when the default stream's topics are enumerated. +#[tokio::test] +#[serial] +async fn list_kafka_topics_does_not_duplicate_an_override_target_living_in_the_default_stream() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let mut config = server.test_config(); + let default_stream = config.topic_mapping.default_stream().to_string(); + let mut overrides = HashMap::new(); + overrides.insert( + "orders".to_string(), + TopicOverride { + stream: default_stream.clone(), + topic: "orders_internal".to_string(), + }, + ); + // Required by `TopicMapping::new`'s own anti-aliasing check: an override targeting the + // default stream must not leave its own target name `over.topic` free for an unmapped Kafka + // topic of that literal name to alias by accident. Never fires in this test - it exists only + // to satisfy that check, since no Kafka topic literally named "orders_internal" is ever used. + overrides.insert( + "orders_internal".to_string(), + TopicOverride { + stream: "elsewhere".to_string(), + topic: "orders_internal".to_string(), + }, + ); + config.topic_mapping = TopicMapping::new(default_stream, overrides) + .expect("valid mapping for this test's fixture data"); + let bridge = IggyBridge::connect(config) + .await + .expect("bridge should connect to a ready server"); + + bridge + .ensure_stream_and_topic("orders", 2) + .await + .expect("seed the mapped topic"); + + let topics = bridge.list_kafka_topics().await.expect("list call"); + assert_eq!( + topics.len(), + 1, + "must list the override's target exactly once, not once per (override name, raw Iggy \ + name): {topics:?}" + ); + assert_eq!(topics[0].kafka_topic, "orders"); + assert_eq!(topics[0].partitions_count, 2); +} + +/// An override targeting a non-default stream is listed under its Kafka-side name, alongside +/// whatever the default stream itself holds - the two enumeration sources don't interfere. +#[tokio::test] +#[serial] +async fn list_kafka_topics_lists_an_override_target_in_a_non_default_stream_alongside_default_stream_topics() + { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let mut config = server.test_config(); + let mut overrides = HashMap::new(); + overrides.insert( + "orders".to_string(), + TopicOverride { + stream: "billing".to_string(), + topic: "orders_v2".to_string(), + }, + ); + config.topic_mapping = + TopicMapping::new(config.topic_mapping.default_stream().to_string(), overrides) + .expect("valid mapping for this test's fixture data"); + let bridge = IggyBridge::connect(config) + .await + .expect("bridge should connect to a ready server"); + + bridge + .ensure_stream_and_topic("orders", 2) + .await + .expect("seed the overridden topic in the billing stream"); + bridge + .ensure_stream_and_topic("payments", 1) + .await + .expect("seed a plain default-stream topic"); + + let mut topics = bridge.list_kafka_topics().await.expect("list call"); + topics.sort_by(|a, b| a.kafka_topic.cmp(&b.kafka_topic)); + assert_eq!(topics.len(), 2); + assert_eq!(topics[0].kafka_topic, "orders"); + assert_eq!(topics[0].partitions_count, 2); + assert_eq!(topics[1].kafka_topic, "payments"); + assert_eq!(topics[1].partitions_count, 1); +} diff --git a/gateways/kafka/tests/create_topics_real_bridge_tests.rs b/gateways/kafka/tests/create_topics_real_bridge_tests.rs new file mode 100644 index 0000000000..daf722aadc --- /dev/null +++ b/gateways/kafka/tests/create_topics_real_bridge_tests.rs @@ -0,0 +1,430 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Wire-level `CreateTopics` tests against a real `iggy-server` process, through +//! [`create_topics::handle`] with a connected [`GatewayState`] - not the stub (no-bridge) path +//! `api_handler_tests.rs` covers, and not `IggyBridge` called directly as +//! `bridge_iggy_integration_tests.rs` does. Exercises the handler's own orchestration: the +//! `configs` check, the existence check running before `validate_only` returns, and real error +//! codes reaching the wire. +//! +//! Requests are hand-built at the v5 flexible wire shape (not `kafka_protocol`'s own +//! `Encodable`/`Decodable`): this crate builds with `default-features = false, features = +//! ["broker"]`, which gives request `Decodable` and response `Encodable` (what a broker needs) +//! but not the reverse - the same reason every other wire-level test in this suite hand-builds +//! request bytes and hand-decodes response bytes via `common/codec.rs`. + +use std::sync::Arc; + +use bytes::Bytes; +use iggy::prelude::{Identifier, StreamClient, TopicClient}; +use serial_test::serial; + +use iggy_gateway_kafka::bridge::IggyBridge; +use iggy_gateway_kafka::protocol::api::{ + BrokerAdvertise, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, ERROR_INVALID_REQUEST, + ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NONE, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, +}; +use iggy_gateway_kafka::protocol::handlers::create_topics; + +#[path = "common/codec.rs"] +mod codec; +#[path = "common/iggy_server.rs"] +mod iggy_server; + +use codec::{Decoder, Encoder}; +use iggy_server::{TestServer, raw_client}; + +const REQUEST_VERSION: i16 = 5; +const TEST_MAX_FRAME_SIZE: usize = 8 * 1024 * 1024; + +/// One requested topic's shape, for [`build_request`]. `assignments` holds partition indices; +/// each gets a single placeholder broker id. +struct TopicSpec<'a> { + name: &'a str, + num_partitions: i32, + replication_factor: i16, + assignments: &'a [i32], + has_config: bool, +} + +impl<'a> TopicSpec<'a> { + const fn new(name: &'a str, num_partitions: i32) -> Self { + Self { + name, + num_partitions, + replication_factor: 1, + assignments: &[], + has_config: false, + } + } +} + +/// Builds a v5 flexible `CreateTopics` request body for one or more topics. +fn build_request(topics: &[TopicSpec], validate_only: bool) -> Bytes { + let mut enc = Encoder::with_capacity(256); + enc.write_varint((topics.len() + 1) as u64); + for topic in topics { + enc.write_compact_nullable_string(Some(topic.name)); + enc.write_i32(topic.num_partitions); + enc.write_i16(topic.replication_factor); + + enc.write_varint((topic.assignments.len() + 1) as u64); + for &partition_index in topic.assignments { + enc.write_i32(partition_index); + enc.write_varint(2); // one broker + enc.write_i32(0); // broker_id + enc.write_empty_tagged_fields(); + } + + if topic.has_config { + enc.write_varint(2); // one config + enc.write_compact_nullable_string(Some("cleanup.policy")); + enc.write_compact_nullable_string(Some("delete")); + enc.write_empty_tagged_fields(); + } else { + enc.write_varint(1); // empty configs + } + + enc.write_empty_tagged_fields(); // topic tagged fields + } + enc.write_i32(5_000); // timeout_ms + enc.write_bool(validate_only); + enc.write_empty_tagged_fields(); + enc.freeze() +} + +/// Decodes a v5 flexible `CreateTopics` response's first topic result into `(error_code, +/// num_partitions)`. Stops there - fine for tests that only ever send one topic. +fn decode_first_result(body: Bytes) -> (i16, i32) { + let mut d = Decoder::new(body); + let _throttle_time_ms = d.read_i32().expect("throttle_time_ms"); + let _topics_plus_one = d.read_varint().expect("topics array count"); + let _name = d.read_compact_nullable_string().expect("topic name"); + let error_code = d.read_i16().expect("error_code"); + let _error_message = d.read_compact_nullable_string().expect("error_message"); + // `topic_config_error_code` is not a positional field at v5-7 despite the struct's field + // list implying it is - the crate's own `Encodable` impl encodes it as a tagged field + // (present only when non-zero), so `num_partitions` follows `error_message` directly. + let num_partitions = d.read_i32().expect("num_partitions"); + (error_code, num_partitions) +} + +/// Decodes every topic result in a v5 flexible `CreateTopics` response into `(name, error_code)`, +/// in wire order. +fn decode_all_results(body: Bytes) -> Vec<(Option, i16)> { + let mut d = Decoder::new(body); + let _throttle_time_ms = d.read_i32().expect("throttle_time_ms"); + let topics_plus_one = d.read_varint().expect("topics array count"); + let mut results = Vec::new(); + for _ in 1..topics_plus_one { + let name = d.read_compact_nullable_string().expect("topic name"); + let error_code = d.read_i16().expect("error_code"); + let _error_message = d.read_compact_nullable_string().expect("error_message"); + let _num_partitions = d.read_i32().expect("num_partitions"); + let _replication_factor = d.read_i16().expect("replication_factor"); + let _configs = d.read_varint().expect("configs array count"); + let _tagged = d.read_varint().expect("topic tagged fields"); + results.push((name, error_code)); + } + results +} + +async fn send(state: &GatewayState, topics: &[TopicSpec<'_>], validate_only: bool) -> (i16, i32) { + let body = build_request(topics, validate_only); + let outcome = create_topics::handle(state, REQUEST_VERSION, body).await; + let resp_body = outcome.expect_response("CreateTopics request always answers"); + decode_first_result(resp_body) +} + +async fn send_all( + state: &GatewayState, + topics: &[TopicSpec<'_>], + validate_only: bool, +) -> Vec<(Option, i16)> { + let body = build_request(topics, validate_only); + let outcome = create_topics::handle(state, REQUEST_VERSION, body).await; + let resp_body = outcome.expect_response("CreateTopics request always answers"); + decode_all_results(resp_body) +} + +async fn connected_state(server: &TestServer) -> GatewayState { + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + GatewayState::new( + BrokerAdvertise::default(), + Some(Arc::new(bridge)), + TEST_MAX_FRAME_SIZE, + ) +} + +#[tokio::test] +#[serial] +async fn create_topics_creates_a_real_iggy_topic() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (error_code, num_partitions) = send(&state, &[TopicSpec::new("orders", 3)], false).await; + assert_eq!(error_code, ERROR_NONE); + assert_eq!(num_partitions, 3); + + let raw = raw_client(&server).await; + let topics = raw + .get_topics(&Identifier::named("kafka").expect("valid stream name")) + .await + .expect("get_topics call"); + assert_eq!(topics.len(), 1, "the handler must have actually created it"); + assert_eq!(topics[0].name, "orders"); + assert_eq!(topics[0].partitions_count, 3); +} + +/// Regression test: before the existence check landed, re-`CreateTopics`-ing the same topic went +/// straight to `ensure_stream_and_topic`, whose idempotent-success contract made a same-spec +/// recreate answer `ERROR_NONE` - wrong for `CreateTopics` itself, whose own contract is that a +/// second create of the same topic is `TOPIC_ALREADY_EXISTS`. +#[tokio::test] +#[serial] +async fn create_topics_recreating_the_same_topic_returns_topic_already_exists() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (first_code, _) = send(&state, &[TopicSpec::new("orders", 3)], false).await; + assert_eq!(first_code, ERROR_NONE); + + let (second_code, _) = send(&state, &[TopicSpec::new("orders", 3)], false).await; + assert_eq!(second_code, ERROR_TOPIC_ALREADY_EXISTS); +} + +/// The existence check must run before `validate_only` returns - `validate_only` against a topic +/// that already exists must still answer `TOPIC_ALREADY_EXISTS`, not a false `ERROR_NONE`. +#[tokio::test] +#[serial] +async fn create_topics_validate_only_against_an_existing_topic_returns_topic_already_exists() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (created_code, _) = send(&state, &[TopicSpec::new("orders", 3)], false).await; + assert_eq!(created_code, ERROR_NONE); + + let (validated_code, _) = send(&state, &[TopicSpec::new("orders", 3)], true).await; + assert_eq!(validated_code, ERROR_TOPIC_ALREADY_EXISTS); +} + +/// `validate_only` against a topic that does not yet exist must succeed without creating it. +#[tokio::test] +#[serial] +async fn create_topics_validate_only_against_a_new_topic_succeeds_without_creating_it() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (error_code, num_partitions) = send(&state, &[TopicSpec::new("orders", 3)], true).await; + assert_eq!(error_code, ERROR_NONE); + assert_eq!(num_partitions, 3); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!( + streams.is_empty(), + "validate_only must not create the stream or topic" + ); +} + +/// Regression test: `validate_only` must still run real name validation, not just report success +/// for anything shaped correctly on the wire. `get_kafka_topic` (the existence check +/// `validate_only` takes) validates the Kafka topic name first, but that path is easy to lose if +/// this handler ever stops routing through it. +#[tokio::test] +#[serial] +async fn create_topics_validate_only_rejects_an_illegal_topic_name() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (error_code, _) = send(&state, &[TopicSpec::new(" orders ", 3)], true).await; + assert_eq!(error_code, ERROR_INVALID_TOPIC_EXCEPTION); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty()); +} + +#[tokio::test] +#[serial] +async fn create_topics_zero_partitions_returns_invalid_partitions_and_creates_nothing() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (error_code, _) = send(&state, &[TopicSpec::new("orders", 0)], false).await; + assert_eq!(error_code, ERROR_INVALID_PARTITIONS); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty()); +} + +#[tokio::test] +#[serial] +async fn create_topics_with_a_per_topic_config_returns_invalid_config_and_creates_nothing() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topic = TopicSpec { + has_config: true, + ..TopicSpec::new("orders", 3) + }; + let (error_code, _) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_INVALID_CONFIG); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty()); +} + +/// A manual replica assignment resolves the created partition count from its own length, not +/// from `num_partitions` (`-1` here, per KIP-464). +#[tokio::test] +#[serial] +async fn create_topics_resolves_partition_count_from_a_manual_assignment() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topic = TopicSpec { + replication_factor: -1, + assignments: &[0, 1], + ..TopicSpec::new("orders", -1) + }; + let (error_code, num_partitions) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_NONE); + assert_eq!(num_partitions, 2); + + let raw = raw_client(&server).await; + let topics = raw + .get_topics(&Identifier::named("kafka").expect("valid stream name")) + .await + .expect("get_topics call"); + assert_eq!(topics[0].partitions_count, 2); +} + +/// Regression test: real Kafka rejects an explicit `num_partitions` alongside a manual +/// assignment outright - a manual assignment requires `num_partitions == -1`. Agreeing with +/// `assignments.len()` does not make the combination valid, and this must not silently succeed. +#[tokio::test] +#[serial] +async fn create_topics_rejects_an_explicit_partition_count_alongside_a_manual_assignment() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topic = TopicSpec { + replication_factor: -1, + assignments: &[0, 1], + ..TopicSpec::new("orders", 2) // agrees with assignments.len(), still invalid + }; + let (error_code, _) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_INVALID_REQUEST); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty()); +} + +/// Regression test: real Kafka refuses every occurrence of a duplicate topic name in one request +/// with `INVALID_REQUEST` (42) and creates nothing - not a first-wins create plus a +/// `TOPIC_ALREADY_EXISTS` for the rest, which would let a client observe a create it never got a +/// `NONE` result for (`AdminClient` keys its futures by name and drops one of the two results). +#[tokio::test] +#[serial] +async fn create_topics_rejects_every_occurrence_of_a_duplicate_topic_name() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topics = [TopicSpec::new("orders", 3), TopicSpec::new("orders", 3)]; + let results = send_all(&state, &topics, false).await; + assert_eq!(results.len(), 2); + for (name, error_code) in &results { + assert_eq!(name.as_deref(), Some("orders")); + assert_eq!(*error_code, ERROR_INVALID_REQUEST); + } + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty(), "neither occurrence must be created"); +} + +/// A duplicate name in the request must not block an unrelated, uniquely-named topic in the +/// same batch from being created normally. +#[tokio::test] +#[serial] +async fn create_topics_still_creates_unrelated_topics_alongside_a_duplicate() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topics = [ + TopicSpec::new("orders", 3), + TopicSpec::new("orders", 3), + TopicSpec::new("payments", 1), + ]; + let results = send_all(&state, &topics, false).await; + assert_eq!(results.len(), 3); + assert_eq!(results[0].1, ERROR_INVALID_REQUEST); + assert_eq!(results[1].1, ERROR_INVALID_REQUEST); + assert_eq!(results[2], (Some("payments".to_string()), ERROR_NONE)); + + let raw = raw_client(&server).await; + let topics = raw + .get_topics(&Identifier::named("kafka").expect("valid stream name")) + .await + .expect("get_topics call"); + assert_eq!(topics.len(), 1); + assert_eq!(topics[0].name, "payments"); +} + +/// Regression test: a request naming more than the bridge-backed topic cap must be rejected +/// wholesale (every entry, `INVALID_REQUEST`, nothing created) rather than partially served. +#[tokio::test] +#[serial] +async fn create_topics_rejects_more_than_the_topic_cap() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let names: Vec = (0..101).map(|i| format!("topic-{i}")).collect(); + let topics: Vec = names.iter().map(|name| TopicSpec::new(name, 1)).collect(); + + let results = send_all(&state, &topics, false).await; + assert_eq!(results.len(), 101); + for (_, error_code) in &results { + assert_eq!(*error_code, ERROR_INVALID_REQUEST); + } + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!( + streams.is_empty(), + "an over-cap request must create nothing" + ); +} diff --git a/gateways/kafka/tests/metadata_real_bridge_tests.rs b/gateways/kafka/tests/metadata_real_bridge_tests.rs new file mode 100644 index 0000000000..a4a4f4da60 --- /dev/null +++ b/gateways/kafka/tests/metadata_real_bridge_tests.rs @@ -0,0 +1,363 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Wire-level `Metadata` tests against a real `iggy-server` process, through +//! [`metadata::handle`] with a connected [`GatewayState`]. See +//! `create_topics_real_bridge_tests.rs` for why requests/responses are hand-built here rather +//! than through `kafka_protocol`'s own `Encodable`/`Decodable` (this crate builds +//! `broker`-feature-only: request `Decodable` and response `Encodable`, not the reverse). + +use std::collections::HashMap; +use std::sync::Arc; + +use bytes::Bytes; +use serial_test::serial; + +use iggy_gateway_kafka::bridge::{IggyBridge, TopicMapping, TopicOverride}; +use iggy_gateway_kafka::protocol::api::{ + BrokerAdvertise, ERROR_INVALID_REQUEST, ERROR_NONE, ERROR_UNKNOWN_TOPIC_OR_PARTITION, + GatewayState, +}; +use iggy_gateway_kafka::protocol::handlers::metadata; + +#[path = "common/codec.rs"] +mod codec; +#[path = "common/iggy_server.rs"] +mod iggy_server; + +use codec::{Decoder, Encoder}; +use iggy_server::TestServer; + +const REQUEST_VERSION: i16 = 9; +const TEST_MAX_FRAME_SIZE: usize = 8 * 1024 * 1024; + +/// Builds a v9 flexible `Metadata` request. `topics: None` means "all topics" (the null-array +/// sentinel); `Some(names)` requests exactly those topics. +fn build_request(topics: Option<&[&str]>) -> Bytes { + let mut enc = Encoder::with_capacity(128); + match topics { + None => enc.write_varint(0), // null compact array: all topics + Some(names) => { + enc.write_varint((names.len() + 1) as u64); + for name in names { + enc.write_compact_nullable_string(Some(name)); + enc.write_empty_tagged_fields(); // per-topic tagged fields + } + } + } + enc.write_bool(false); // allow_auto_topic_creation + enc.write_bool(false); // include_cluster_authorized_operations + enc.write_bool(false); // include_topic_authorized_operations + enc.write_empty_tagged_fields(); // top-level tagged fields + enc.freeze() +} + +/// One decoded `MetadataResponseTopic` entry: `(name, error_code, partitions_count)`. +/// One decoded `MetadataResponsePartition`: `(leader_id, replica_nodes, isr_nodes)`. Asserted on +/// directly, not just counted - a regression emitting `leader_id = -1` or empty `replica_nodes` +/// would otherwise pass a suite that only checks partition *count*, while making the topic look +/// leaderless to a real client (that client picks its produce/fetch broker from exactly these +/// fields). +type PartitionEntry = (i32, Vec, Vec); +type TopicEntry = (Option, i16, Vec); + +/// Decodes a v9 flexible `Metadata` response into every topic entry, in wire order. +/// +/// Assumes every partition entry has zero offline replicas - true for every response +/// `metadata::handle`'s real path can produce (nothing here ever reports an offline broker). +fn decode_topics(body: Bytes) -> Vec { + let mut d = Decoder::new(body); + let _throttle_time_ms = d.read_i32().expect("throttle_time_ms"); + + let brokers_plus_one = d.read_varint().expect("brokers array count"); + for _ in 1..brokers_plus_one { + let _node_id = d.read_i32().expect("node_id"); + let _host = d.read_compact_nullable_string().expect("host"); + let _port = d.read_i32().expect("port"); + let _rack = d.read_compact_nullable_string().expect("rack"); + let _tagged = d.read_varint().expect("broker tagged fields"); + } + + let _cluster_id = d.read_compact_nullable_string().expect("cluster_id"); + let _controller_id = d.read_i32().expect("controller_id"); + + let topics_plus_one = d.read_varint().expect("topics array count"); + let mut topics = Vec::new(); + for _ in 1..topics_plus_one { + let error_code = d.read_i16().expect("topic error_code"); + let name = d.read_compact_nullable_string().expect("topic name"); + let _is_internal = d.read_bool().expect("is_internal"); + + let partitions_plus_one = d.read_varint().expect("partitions array count"); + let mut partitions = Vec::new(); + for _ in 1..partitions_plus_one { + let _error_code = d.read_i16().expect("partition error_code"); + let _partition_index = d.read_i32().expect("partition_index"); + let leader_id = d.read_i32().expect("leader_id"); + let _leader_epoch = d.read_i32().expect("leader_epoch"); + let replica_plus_one = d.read_varint().expect("replica_nodes count"); + let mut replica_nodes = Vec::new(); + for _ in 1..replica_plus_one { + replica_nodes.push(d.read_i32().expect("replica node id")); + } + let isr_plus_one = d.read_varint().expect("isr_nodes count"); + let mut isr_nodes = Vec::new(); + for _ in 1..isr_plus_one { + isr_nodes.push(d.read_i32().expect("isr node id")); + } + let offline_plus_one = d.read_varint().expect("offline_replicas count"); + for _ in 1..offline_plus_one { + let _offline = d.read_i32().expect("offline replica node id"); + } + let _tagged = d.read_varint().expect("partition tagged fields"); + partitions.push((leader_id, replica_nodes, isr_nodes)); + } + + let _topic_authorized_operations = d.read_i32().expect("topic_authorized_operations"); + let _tagged = d.read_varint().expect("topic tagged fields"); + + topics.push((name, error_code, partitions)); + } + + topics +} + +/// `count` partitions, each with this gateway's single broker (node id 1) as leader, sole +/// replica, and sole ISR member - the only shape `metadata::handle`'s real path ever produces. +fn expected_partitions(count: u32) -> Vec { + (0..count).map(|_| (1, vec![1], vec![1])).collect() +} + +async fn send(state: &GatewayState, topics: Option<&[&str]>) -> Vec { + let body = build_request(topics); + let outcome = metadata::handle(state, REQUEST_VERSION, body).await; + let resp_body = outcome.expect_response("Metadata request always answers"); + decode_topics(resp_body) +} + +async fn connected_state(server: &TestServer) -> (GatewayState, IggyBridge) { + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + let seed = IggyBridge::connect(server.test_config()) + .await + .expect("seed bridge should connect to a ready server"); + let state = GatewayState::new( + BrokerAdvertise::default(), + Some(Arc::new(bridge)), + TEST_MAX_FRAME_SIZE, + ); + (state, seed) +} + +#[tokio::test] +#[serial] +async fn a_named_lookup_of_an_existing_topic_succeeds() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, seed) = connected_state(&server).await; + seed.ensure_stream_and_topic("orders", 3) + .await + .expect("seed the topic"); + + let topics = send(&state, Some(&["orders"])).await; + assert_eq!(topics.len(), 1); + assert_eq!( + topics[0], + ( + Some("orders".to_string()), + ERROR_NONE, + expected_partitions(3) + ) + ); +} + +#[tokio::test] +#[serial] +async fn a_named_lookup_of_a_nonexistent_topic_reports_unknown_topic_or_partition() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, _seed) = connected_state(&server).await; + + let topics = send(&state, Some(&["orders"])).await; + assert_eq!(topics.len(), 1); + assert_eq!( + topics[0], + ( + Some("orders".to_string()), + ERROR_UNKNOWN_TOPIC_OR_PARTITION, + Vec::new() + ) + ); +} + +#[tokio::test] +#[serial] +async fn a_null_topics_array_lists_every_real_topic() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, seed) = connected_state(&server).await; + seed.ensure_stream_and_topic("orders", 3) + .await + .expect("seed orders"); + seed.ensure_stream_and_topic("payments", 1) + .await + .expect("seed payments"); + + let mut topics = send(&state, None).await; + topics.sort_by(|a, b| a.0.cmp(&b.0)); + assert_eq!( + topics, + vec![ + ( + Some("orders".to_string()), + ERROR_NONE, + expected_partitions(3) + ), + ( + Some("payments".to_string()), + ERROR_NONE, + expected_partitions(1) + ), + ] + ); +} + +#[tokio::test] +#[serial] +async fn a_null_topics_array_against_an_empty_bridge_lists_nothing() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, _seed) = connected_state(&server).await; + + let topics = send(&state, None).await; + assert!(topics.is_empty()); +} + +#[tokio::test] +#[serial] +async fn a_named_lookup_reports_the_kafka_side_name_through_a_topic_mapping_override() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let mut config = server.test_config(); + let mut overrides = HashMap::new(); + overrides.insert( + "orders".to_string(), + TopicOverride { + stream: "billing".to_string(), + topic: "orders_v2".to_string(), + }, + ); + let default_stream = config.topic_mapping.default_stream().to_string(); + config.topic_mapping = + TopicMapping::new(default_stream, overrides).expect("valid mapping for this test"); + let seed = IggyBridge::connect(config.clone()) + .await + .expect("seed bridge should connect to a ready server"); + seed.ensure_stream_and_topic("orders", 2) + .await + .expect("seed the mapped topic"); + + let bridge = IggyBridge::connect(config) + .await + .expect("bridge should connect to a ready server"); + let state = GatewayState::new( + BrokerAdvertise::default(), + Some(Arc::new(bridge)), + TEST_MAX_FRAME_SIZE, + ); + + let topics = send(&state, Some(&["orders"])).await; + assert_eq!(topics.len(), 1); + // The Kafka-side name asked about ("orders"), not the Iggy-side name under the override + // ("orders_v2") - a Kafka client never heard of "orders_v2" and would be confused by it. + assert_eq!( + topics[0], + ( + Some("orders".to_string()), + ERROR_NONE, + expected_partitions(2) + ) + ); +} + +/// Regression test: `bounds_guard`'s pre-decode projection cannot see a real topic's partition +/// count, only known after the bridge round trip - a real, oversized-relative-to-`max_frame_size` +/// response must still close the connection rather than build and send it. +#[tokio::test] +#[serial] +async fn a_response_projected_over_max_frame_size_closes_instead_of_answering() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, seed) = connected_state(&server).await; + seed.ensure_stream_and_topic("orders", 50) + .await + .expect("seed a topic with enough partitions to trip a tiny cap"); + + // 50 partitions * 64 bytes/partition (this crate's own conservative per-partition estimate) + // = 3200 bytes, comfortably over a 512-byte max_frame_size. + let tiny_state = GatewayState::new(state.broker, state.bridge, 512); + let body = build_request(Some(&["orders"])); + let outcome = metadata::handle(&tiny_state, REQUEST_VERSION, body).await; + assert!(outcome.is_close(), "expected Close, got {outcome:?}"); +} + +/// Regression test: a topic named more than once in one request must still resolve every +/// occurrence correctly, not just avoid a crash - proves the dedup-by-name cache is actually +/// populated and read back correctly, not merely harmless. +#[tokio::test] +#[serial] +async fn a_topic_named_twice_in_one_request_resolves_both_occurrences_correctly() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, seed) = connected_state(&server).await; + seed.ensure_stream_and_topic("orders", 2) + .await + .expect("seed the topic"); + + let topics = send(&state, Some(&["orders", "orders"])).await; + assert_eq!(topics.len(), 2); + for topic in &topics { + assert_eq!( + topic, + &( + Some("orders".to_string()), + ERROR_NONE, + expected_partitions(2) + ) + ); + } +} + +/// Regression test: a named-lookup request addressing more than the bridge-backed topic cap must +/// be rejected wholesale (every name, `INVALID_REQUEST`) rather than partially served. +#[tokio::test] +#[serial] +async fn a_named_lookup_of_more_than_the_topic_cap_is_rejected() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, _seed) = connected_state(&server).await; + + let names: Vec = (0..101).map(|i| format!("topic-{i}")).collect(); + let name_refs: Vec<&str> = names.iter().map(String::as_str).collect(); + + let topics = send(&state, Some(&name_refs)).await; + assert_eq!(topics.len(), 101); + for (_, error_code, _) in &topics { + assert_eq!(*error_code, ERROR_INVALID_REQUEST); + } +} From dafcba827e0289dd127955d1fde0fd4a2ef88aee Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Mon, 21 Sep 2026 22:23:12 -0400 Subject: [PATCH 02/10] updating documentation --- gateways/kafka/README.md | 10 ++++- gateways/kafka/docs/SCOPE.md | 74 +++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/gateways/kafka/README.md b/gateways/kafka/README.md index 13ad30652c..fd19519a7a 100644 --- a/gateways/kafka/README.md +++ b/gateways/kafka/README.md @@ -2,7 +2,15 @@ Foundation layer for [apache/iggy#3421](https://github.com/apache/iggy/issues/3421): a TCP listener on the Kafka wire port that decodes requests, validates scoped API keys and versions, and returns stub responses. -> **Stub warning:** no API persists or reads real data yet. Produce, Fetch, and ListOffsets return retriable `NOT_LEADER_OR_FOLLOWER` (6) so clients keep data locally / retry elsewhere instead of trusting a fake success. CreateTopics does **not** create topics; valid requests return `NOT_CONTROLLER` (41). Metadata still reports requested topics as unknown. Persistence lands with the Iggy bridge (see [docs/SCOPE.md](docs/SCOPE.md)). +> **Stub warning:** Produce, Fetch, and ListOffsets still don't persist or read real data - they +> return retriable `NOT_LEADER_OR_FOLLOWER` (6) so clients keep data locally / retry elsewhere +> instead of trusting a fake success. CreateTopics and Metadata are wired to the Iggy bridge: with +> `IGGY_KAFKA_BRIDGE_ENABLED=true`, CreateTopics creates a real Iggy stream/topic and Metadata +> reports real topics and partition counts (a topic not requested by name and not found is +> silently absent from a null-topics "list all" response, and `UNKNOWN_TOPIC_OR_PARTITION` when +> named explicitly); with the bridge off (the default), both stay stubs - CreateTopics answers +> `NOT_CONTROLLER` (41), Metadata reports every requested topic unknown. See +> [docs/SCOPE.md](docs/SCOPE.md). ## Run diff --git a/gateways/kafka/docs/SCOPE.md b/gateways/kafka/docs/SCOPE.md index 30289a934e..90ca0f67d8 100644 --- a/gateways/kafka/docs/SCOPE.md +++ b/gateways/kafka/docs/SCOPE.md @@ -109,11 +109,83 @@ below it are still open for the issues that build on top of it. not part of `bridge/`'s own scope. - [x] Idempotent `ensure_stream_and_topic()` (create-if-not-exists) - `src/bridge/iggy_bridge.rs`, exercised end-to-end in `tests/bridge_iggy_integration_tests.rs`. +- [x] Real CreateTopics ([#3538](https://github.com/apache/iggy/issues/3538)): with + `IGGY_KAFKA_BRIDGE_ENABLED=true`, creates the Iggy stream/topic through + `IggyBridge::create_kafka_topic` - an atomic create-or-report-exists call, not a separate + existence read followed by an idempotent create (that sequence has a TOCTOU window: two + concurrent requests for the same new name could both observe "doesn't exist yet" and both + receive `NONE`, when Kafka guarantees exactly one caller does). + `src/protocol/handlers/create_topics.rs`, `tests/create_topics_real_bridge_tests.rs`. With + the bridge off, the stub from #3421 answers `NOT_CONTROLLER` (41) as before. + - Every occurrence of a duplicate topic name within one request is rejected with + `INVALID_REQUEST` (42) and nothing is created for it, matching real Kafka + (`ControllerApis.createTopics`) rather than creating the first occurrence and reporting the + rest as already existing. + - A manual partition `assignments` list combined with an explicit `num_partitions`/ + `replication_factor` is rejected with `INVALID_REQUEST` (42) even when the count agrees with + `assignments.len()` - real Kafka's own `ReplicationControlManager` treats the two as mutually + exclusive inputs, not independently-checked values that happen to agree. Retired + `ERROR_INVALID_REPLICA_ASSIGNMENT` (39), which this bridge never actually needed: the earlier + "count disagrees with assignment length" check it backed doesn't correspond to a condition + real Kafka's own validation order can even reach. + - `IggyError::RequestAlreadyApplied` (the SDK's reconnect path replayed a write that already + committed) maps to `NONE`, not the `UNKNOWN_SERVER_ERROR` catch-all - the operation did + succeed, and a Java client treats `UNKNOWN_SERVER_ERROR` as non-retriable. + - `IggyBridge::get_kafka_topic` no longer probes `get_stream` separately before `get_topic` - + `get_topic` already answers `Ok(None)` when the stream itself is missing, so the probe was a + second round trip to learn something the one call already told it. + - Bridge fan-out is bounded independently of `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096, + still a pre-decode ceiling, not a usability one): a duplicate name never reaches the bridge at + all (rejected up front, see above), a request naming more than 100 distinct non-duplicate + topics is rejected outright (`INVALID_REQUEST`, no bridge call for any of them), and the + request's own wire `timeout_ms` (clamped to `[1s, 30s]`) now bounds the whole handler's + aggregate bridge work, not just decoded and discarded - a deadline that fires answers every + topic `REQUEST_TIMED_OUT` rather than continuing to hold the shared lockstep `IggyClient`. - [x] Document partition mapping in [`BRIDGE_MAPPING.md`](BRIDGE_MAPPING.md): - Iggy partitions are **0-based** (same as Kafka) — direct `partition_id` mapping, no offset conversion - Kafka consumer groups do **not** map onto Iggy consumer groups. Assignment stays client-side, and Iggy's group registry is used as an offset key only ([`OFFSET_STORAGE.md`](OFFSET_STORAGE.md)) - `Partitioning::partition_id(index)` on every Produce. A Kafka producer resolves the partition before it builds the request, so `Partitioning::balanced()` has no trigger there. The `-1` default-partition-count case belongs to CreateTopics -- [ ] Real Metadata topology (brokers, partitions, leaders) backed by Iggy state +- [x] Real Metadata topic/partition data ([#3534](https://github.com/apache/iggy/issues/3534)): + with `IGGY_KAFKA_BRIDGE_ENABLED=true`, a named lookup answers from + `IggyBridge::get_kafka_topic` (`UNKNOWN_TOPIC_OR_PARTITION` if not found) and a null + topics array ("all topics") answers from `IggyBridge::list_kafka_topics` - the target of + every configured `TopicMapping` override plus every other topic in the default stream, so + an Iggy stream this bridge has no mapping rule pointing at is never listed (nothing a Kafka + client ever named). Every reported partition names this gateway's single broker (node id 1) + as leader/replica/ISR, since there is only ever one. `src/protocol/handlers/metadata.rs`, + `src/bridge/iggy_bridge/topics.rs` (`get_kafka_topic`/`list_kafka_topics`), + `tests/metadata_real_bridge_tests.rs`. With the bridge off, the stub from #3421 reports + every requested topic unknown, as before. + - Fixed alongside: the stub's `decode_topics` collapsed a null topics array ("all topics") and + an explicit empty one (`Some(vec![])`, "these zero topics") to the same `Vec::new()` - the + real path's `decode_requested_topics` keeps `Option>` so the two aren't conflated. + Also: at `api_version == 0` an explicit empty array is folded into "all topics" too, matching + Kafka's own `isAllTopics()` rule (`topics == null || (topics.isEmpty() && version == 0)`) - + there is no v0 wire shape for "cluster info only, zero topics" (that distinct shape, KIP-4's + `describeCluster()`, starts at v1). + - `get_kafka_topic` shares its implementation with CreateTopics' own existence check above - + both need "does this Kafka-side name resolve to a real Iggy topic," so this bridge exposes one + method returning the SDK's own `TopicDetails`, not two narrower, independently-maintained + lookups. + - The response-size cap above only covers the "all topics" and per-name-expansion cases. A + named lookup is separately bounded on the request side: names repeated in one request are + deduped to one `get_kafka_topic` round trip before any bridge call (not one per occurrence), + a lookup naming more than 100 distinct topics is rejected outright + (`INVALID_REQUEST`, no bridge call for any of them - `bounds_guard`'s `MAX_REQUEST_ELEMENTS` + (4,096) is a pre-decode ceiling, not a usability one), and the whole lookup's aggregate bridge + work runs under a fixed 20s wall-clock deadline (`Metadata` carries no `timeout_ms` field in + any version, unlike `CreateTopics`, so this cannot be client-honored) - a deadline that fires + answers every name `REQUEST_TIMED_OUT` rather than continuing to hold the shared lockstep + `IggyClient`. The "all topics" path is server-driven, not client-count-driven, so it has no + equivalent cap - only the response-size projection applies there. +- [ ] Multi-broker topology (this gateway is, and will stay, a single logical broker - node id 1 + always leads every partition it reports; nothing here models an Iggy cluster as multiple + Kafka-visible brokers) +- [ ] A raw/non-conformant client's extra trailing byte on some Metadata request shapes seen + against real `kcat`/`librdkafka` traffic in earlier testing on a since-restructured branch - + not reproduced or root-caused against the current `kafka_protocol`-based decode path in this + session, so not carried forward as a fix here rather than guessed at. Needs fresh + reproduction against a real `kcat` before it's re-closed. ### `kafka-protocol` crate adoption — superseded, done differently From cd398c773ae0f84a414ca39b2d0a57090b23f98d Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Tue, 22 Sep 2026 20:21:49 -0400 Subject: [PATCH 03/10] Fixed CreateTopics/Metadata review finding --- gateways/kafka/docs/SCOPE.md | 46 +++-- gateways/kafka/src/bridge/error.rs | 26 +-- gateways/kafka/src/protocol/api.rs | 7 + .../src/protocol/handlers/create_topics.rs | 70 ++++++- .../kafka/src/protocol/handlers/metadata.rs | 174 +++++++++++------- .../tests/create_topics_real_bridge_tests.rs | 84 ++++++++- .../kafka/tests/metadata_real_bridge_tests.rs | 66 +++++-- 7 files changed, 357 insertions(+), 116 deletions(-) diff --git a/gateways/kafka/docs/SCOPE.md b/gateways/kafka/docs/SCOPE.md index 90ca0f67d8..08827ac878 100644 --- a/gateways/kafka/docs/SCOPE.md +++ b/gateways/kafka/docs/SCOPE.md @@ -124,13 +124,22 @@ below it are still open for the issues that build on top of it. - A manual partition `assignments` list combined with an explicit `num_partitions`/ `replication_factor` is rejected with `INVALID_REQUEST` (42) even when the count agrees with `assignments.len()` - real Kafka's own `ReplicationControlManager` treats the two as mutually - exclusive inputs, not independently-checked values that happen to agree. Retired - `ERROR_INVALID_REPLICA_ASSIGNMENT` (39), which this bridge never actually needed: the earlier - "count disagrees with assignment length" check it backed doesn't correspond to a condition - real Kafka's own validation order can even reach. + exclusive inputs, not independently-checked values that happen to agree. + - A manual assignment's own partition indices are checked too, not just its length: + `ERROR_INVALID_REPLICA_ASSIGNMENT` (39) for a duplicate index or one that isn't exactly + `0..assignments.len()` - `{5: [...], 7: [...]}` has the right length for a 2-partition topic + but names neither partition `0` nor `1`, matching real Kafka's own + `ReplicationControlManager.createTopic` key-set validation. - `IggyError::RequestAlreadyApplied` (the SDK's reconnect path replayed a write that already committed) maps to `NONE`, not the `UNKNOWN_SERVER_ERROR` catch-all - the operation did - succeed, and a Java client treats `UNKNOWN_SERVER_ERROR` as non-retriable. + succeed, and a Java client treats `UNKNOWN_SERVER_ERROR` as non-retriable. Special-cased + locally in `create_topics.rs`, not in the shared `BridgeError -> Kafka error code` mapping + every handler's error path goes through: "the write already applied" is a write-only fact, + and a read (Metadata, ListOffsets) reaching this variant has no write to have applied. + - `error_message` on a rejected topic never re-embeds the topic name `CreatableTopicResult.name` + already carries - `BridgeError::InvalidKafkaTopicName`'s `Display` does, so using it directly + would roughly double the response cost per invalid name, for free (name validation runs before + any bridge I/O). - `IggyBridge::get_kafka_topic` no longer probes `get_stream` separately before `get_topic` - `get_topic` already answers `Ok(None)` when the stream itself is missing, so the probe was a second round trip to learn something the one call already told it. @@ -169,15 +178,24 @@ below it are still open for the issues that build on top of it. lookups. - The response-size cap above only covers the "all topics" and per-name-expansion cases. A named lookup is separately bounded on the request side: names repeated in one request are - deduped to one `get_kafka_topic` round trip before any bridge call (not one per occurrence), - a lookup naming more than 100 distinct topics is rejected outright - (`INVALID_REQUEST`, no bridge call for any of them - `bounds_guard`'s `MAX_REQUEST_ELEMENTS` - (4,096) is a pre-decode ceiling, not a usability one), and the whole lookup's aggregate bridge - work runs under a fixed 20s wall-clock deadline (`Metadata` carries no `timeout_ms` field in - any version, unlike `CreateTopics`, so this cannot be client-honored) - a deadline that fires - answers every name `REQUEST_TIMED_OUT` rather than continuing to hold the shared lockstep - `IggyClient`. The "all topics" path is server-driven, not client-count-driven, so it has no - equivalent cap - only the response-size projection applies there. + deduped up front to one response entry, not just one `get_kafka_topic` round trip - real + Kafka answers a topic named twice in one request with one response entry, and re-expanding to + match the request would let a handful of repeats of one large topic name amplify a response + sized off the repeat count instead of the distinct count. A lookup naming more than 100 + distinct topics is rejected outright (`INVALID_REQUEST`, no bridge call for any of them - + `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode ceiling, not a usability one), + and the whole lookup's aggregate bridge work runs under a fixed 20s wall-clock deadline + (`Metadata` carries no `timeout_ms` field in any version, unlike `CreateTopics`, so this cannot + be client-honored) - a deadline that fires answers every name `REQUEST_TIMED_OUT` rather than + continuing to hold the shared lockstep `IggyClient`. + - The "all topics" path is server-driven, not client-count-driven, so it has no distinct-topic + cap - only the response-size projection applies there, and it **truncates** rather than + closes on overflow: `list_kafka_topics()`'s result is trimmed to as many whole topics (in + listing order) as `max_frame_size` allows. Closing instead, as the named-lookup path still + does, would make every all-topics Metadata call - the bootstrap/refresh shape both librdkafka + and the Java client use - fail identically and permanently once the cluster's total partition + count crosses the trip point, since that size is the catalog's own, not anything the + requesting client chose or can shrink. - [ ] Multi-broker topology (this gateway is, and will stay, a single logical broker - node id 1 always leads every partition it reports; nothing here models an Iggy cluster as multiple Kafka-visible brokers) diff --git a/gateways/kafka/src/bridge/error.rs b/gateways/kafka/src/bridge/error.rs index 34ec7452eb..ac514c0d12 100644 --- a/gateways/kafka/src/bridge/error.rs +++ b/gateways/kafka/src/bridge/error.rs @@ -19,7 +19,7 @@ use iggy::prelude::IggyError; use thiserror::Error; use crate::protocol::api::{ - ERROR_INVALID_PARTITIONS, ERROR_INVALID_REQUEST, ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NONE, + ERROR_INVALID_PARTITIONS, ERROR_INVALID_REQUEST, ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NOT_LEADER_OR_FOLLOWER, ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, ERROR_TOPIC_AUTHORIZATION_FAILED, ERROR_UNKNOWN_SERVER_ERROR, ERROR_UNKNOWN_TOPIC_OR_PARTITION, }; @@ -177,12 +177,14 @@ const fn iggy_error_to_kafka_code(err: &IggyError) -> i16 { // (Iggy's server-side cap, above 1000). Reusing 37 for both directions would return a // client-visible error message that contradicts the actual request it sent. IggyError::TooManyPartitions => ERROR_INVALID_REQUEST, - // The operation *did* commit - the SDK's own reconnect path replayed a write whose first - // attempt already applied, and the server's client-table dedup caught the replay. Falling - // into the catch-all below would report a permanent server fault for a request that - // actually succeeded; a Java client treats `UNKNOWN_SERVER_ERROR` as non-retriable and - // would surface a spurious failure for a `CreateTopics` that in fact created the topic. - IggyError::RequestAlreadyApplied => ERROR_NONE, + // Deliberately NOT special-cased here to ERROR_NONE: this function is shared by every + // handler's error path, but "the operation did commit, so report success" only holds for + // a caller that issued a *write* - the SDK's own reconnect path replayed a write whose + // first attempt already applied, and the server's client-table dedup caught the replay. + // A read that somehow reaches this variant has no write to have "already applied"; a + // write-side caller (CreateTopics) special-cases it locally, close to the write it + // concerns, instead of baking a write-only assumption into a mapping every read also + // goes through. _ => ERROR_UNKNOWN_SERVER_ERROR, } } @@ -239,12 +241,12 @@ mod tests { } #[test] - fn request_already_applied_maps_to_no_error_not_unknown_server_error() { - // The operation committed on its first attempt; the client-table dedup on a replay is - // not a fault. Falling into the catch-all (-1) would tell a Java client the CreateTopics - // it just replayed permanently failed, when the topic it asked for now exists. + fn request_already_applied_falls_to_the_generic_mapping_here() { + // This shared mapping has no write to know "already applied" refers to - CreateTopics + // (the only caller for whom that's a success, not a fault) special-cases it locally + // instead (`create_topics.rs`), close to the write it concerns. let err = BridgeError::Iggy(IggyError::RequestAlreadyApplied); - assert_eq!(err.to_kafka_error_code(), ERROR_NONE); + assert_eq!(err.to_kafka_error_code(), ERROR_UNKNOWN_SERVER_ERROR); } #[test] diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index 89a36cbb86..81584c4bdb 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -72,6 +72,13 @@ pub const ERROR_UNSUPPORTED_VERSION: i16 = 35; pub const ERROR_TOPIC_ALREADY_EXISTS: i16 = 36; pub const ERROR_INVALID_PARTITIONS: i16 = 37; pub const ERROR_INVALID_REPLICATION_FACTOR: i16 = 38; +/// `CreateTopics`: a manual partition `assignments` list whose partition indices are not exactly +/// `0..assignments.len()` in some order, or repeat an index. +/// +/// Matches real Kafka's `ReplicationControlManager.createTopic`, which validates the assignment +/// map's keys the same way regardless of what a client's replica list under each key says (this +/// bridge doesn't model replicas at all, so only the key set is checked). +pub const ERROR_INVALID_REPLICA_ASSIGNMENT: i16 = 39; /// `CreateTopics` stub: do not claim topics were created (no controller / no Iggy bridge). pub const ERROR_NOT_CONTROLLER: i16 = 41; pub const ERROR_INVALID_REQUEST: i16 = 42; diff --git a/gateways/kafka/src/protocol/handlers/create_topics.rs b/gateways/kafka/src/protocol/handlers/create_topics.rs index c29f8d91e4..d4a43512f2 100644 --- a/gateways/kafka/src/protocol/handlers/create_topics.rs +++ b/gateways/kafka/src/protocol/handlers/create_topics.rs @@ -21,17 +21,19 @@ use std::collections::HashSet; use std::time::Duration; use bytes::Bytes; -use kafka_protocol::messages::create_topics_request::CreatableTopic; +use iggy::prelude::IggyError; +use kafka_protocol::messages::create_topics_request::{CreatableReplicaAssignment, CreatableTopic}; use kafka_protocol::messages::create_topics_response::CreatableTopicResult; use kafka_protocol::messages::{CreateTopicsRequest, CreateTopicsResponse, TopicName}; use kafka_protocol::protocol::StrBytes; -use crate::bridge::{IggyBridge, TopicCreationOutcome}; +use crate::bridge::{BridgeError, IggyBridge, TopicCreationOutcome}; use crate::error::Result; use crate::protocol::api::{ API_KEY_CREATE_TOPICS, ApiVersionRange, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, - ERROR_INVALID_REPLICATION_FACTOR, ERROR_INVALID_REQUEST, ERROR_NONE, ERROR_NOT_CONTROLLER, - ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, HandleOutcome, + ERROR_INVALID_REPLICA_ASSIGNMENT, ERROR_INVALID_REPLICATION_FACTOR, ERROR_INVALID_REQUEST, + ERROR_NONE, ERROR_NOT_CONTROLLER, ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, + GatewayState, HandleOutcome, }; use crate::protocol::bounds_guard::validate_create_topics_shape; use crate::protocol::handlers::{ @@ -268,7 +270,7 @@ async fn create_one_topic( Ok(None) => success(), Err(err) => result .with_error_code(err.to_kafka_error_code()) - .with_error_message(Some(StrBytes::from(err.to_string()))), + .with_error_message(Some(StrBytes::from(error_message_for(&err)))), }; } @@ -276,13 +278,35 @@ async fn create_one_topic( .create_kafka_topic(kafka_topic, partition_count) .await { - Ok(TopicCreationOutcome::Created) => success(), + // The second arm: the write committed on its first attempt, the SDK's own reconnect path + // replayed it, and the server's client-table dedup caught the replay - not a fault. + // `to_kafka_error_code`'s shared mapping deliberately doesn't special-case this - it's a + // write-only fact, checked here, at the one write this bridge makes, rather than assumed + // true for the reads that share that mapping too. + Ok(TopicCreationOutcome::Created) + | Err(BridgeError::Iggy(IggyError::RequestAlreadyApplied)) => success(), Ok(TopicCreationOutcome::AlreadyExists) => { result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS) } Err(err) => result .with_error_code(err.to_kafka_error_code()) - .with_error_message(Some(StrBytes::from(err.to_string()))), + .with_error_message(Some(StrBytes::from(error_message_for(&err)))), + } +} + +/// Error text for `CreatableTopicResult.error_message`, without re-embedding the topic name: +/// `result.name` (`CreatableTopicResult::with_name`) already carries it, so `err.to_string()`'s +/// own embedded copy for these two variants would double the per-topic response cost for a name +/// the client already sent and already has back. Validation runs before any bridge I/O, so this +/// is a purely local, zero-round-trip amplification if left in - 100 topics named with a maximal +/// legal length build a response roughly twice the size the name alone would justify. +fn error_message_for(err: &BridgeError) -> String { + match err { + BridgeError::InvalidKafkaTopicName { reason, .. } => reason.clone(), + BridgeError::InvalidPartitionCount { .. } => { + "partition count must be at least 1".to_string() + } + other => other.to_string(), } } @@ -295,6 +319,13 @@ async fn create_one_topic( /// that only *disagrees* with the assignment length is not a distinct, more lenient case - the /// combination itself is what's invalid, so both are `INVALID_REQUEST` (42), never `NONE`. /// +/// An assignment's own partition indices are checked too, not just its length: real Kafka +/// requires the key set to be exactly `0..assignments.len()`, each index appearing once, and +/// rejects anything else - a duplicate or non-consecutive index (`{5: [...], 7: [...]}`) - with +/// `INVALID_REPLICA_ASSIGNMENT` (39), the one condition that code exists for. This bridge doesn't +/// model per-partition replica placement, so only the index set is checked, not each entry's +/// replica list. +/// /// With no assignments, `num_partitions = -1` / `replication_factor = -1` mean "use the broker /// default" from v4+ (pre-v4 requires an explicit positive value for both, since v2/v3 have no /// broker-default sentinel absent a manual assignment). @@ -306,6 +337,9 @@ fn validate_create_topic_shape( if topic.num_partitions != -1 || topic.replication_factor != -1 { return Err(ERROR_INVALID_REQUEST); } + if !assignment_indices_are_consecutive_from_zero(&topic.assignments) { + return Err(ERROR_INVALID_REPLICA_ASSIGNMENT); + } return Ok(u32::try_from(topic.assignments.len()).unwrap_or(DEFAULT_PARTITION_COUNT)); } @@ -337,6 +371,26 @@ fn validate_create_topic_shape( Ok(partition_count) } +/// Real Kafka requires a manual `assignments` list's partition indices to be exactly +/// `0..assignments.len()`, each appearing once - not merely that many entries as there are +/// partitions. `{5: [...], 7: [...]}` has the right length for a 2-partition topic but names +/// neither partition `0` nor `1`. +fn assignment_indices_are_consecutive_from_zero( + assignments: &[CreatableReplicaAssignment], +) -> bool { + let Some(last_index) = assignments.len().checked_sub(1) else { + return false; // empty: the caller never reaches here with an empty list, but no gap math to underflow on. + }; + let mut indices: Vec = assignments.iter().map(|a| a.partition_index).collect(); + indices.sort_unstable(); + indices.dedup(); + // Sorted, deduped, and matching the original count rules out both a duplicate and a gap: `n` + // distinct integers spanning exactly `[0, n-1]` must be all of `0..n`, nothing else fits. + indices.len() == assignments.len() + && indices.first().copied() == Some(0) + && indices.last().copied() == i32::try_from(last_index).ok() +} + /// Well-formed `CreateTopics` response with a single placeholder topic. /// /// # Errors @@ -411,8 +465,6 @@ fn encode_inner(version: i16, topics: &[CreatableTopic], forced_error: i16) -> R #[cfg(test)] mod tests { - use kafka_protocol::messages::create_topics_request::CreatableReplicaAssignment; - use super::*; fn topic_name(name: &str) -> TopicName { diff --git a/gateways/kafka/src/protocol/handlers/metadata.rs b/gateways/kafka/src/protocol/handlers/metadata.rs index 934f3eee25..a5d315f530 100644 --- a/gateways/kafka/src/protocol/handlers/metadata.rs +++ b/gateways/kafka/src/protocol/handlers/metadata.rs @@ -17,7 +17,7 @@ //! Metadata (API key 3). -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use std::time::Duration; use bytes::Bytes; @@ -108,7 +108,10 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand let results = match requested { None => match bridge.list_kafka_topics().await { - Ok(topics) => topics.into_iter().map(found_result).collect(), + Ok(topics) => { + let results: Vec = topics.into_iter().map(found_result).collect(); + truncate_all_topics_to_frame_budget(results, state.max_frame_size) + } Err(error) => { // Same "no top-level error field" constraint as a decode failure: there is no // way to answer "the bridge itself is unreachable" for an all-topics request @@ -117,47 +120,18 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand return HandleOutcome::Close; } }, - Some(names) => { - let distinct_names: HashSet<&str> = names.iter().map(StrBytes::as_str).collect(); - if distinct_names.len() > MAX_BRIDGE_BACKED_TOPICS { - tracing::warn!( - distinct_topics = distinct_names.len(), - max = MAX_BRIDGE_BACKED_TOPICS, - "Metadata request addresses too many distinct topics; rejecting" - ); - names - .iter() - .map(|name| error_result(name.clone(), ERROR_INVALID_REQUEST)) - .collect() - } else { - match tokio::time::timeout(REQUEST_DEADLINE, resolve_named_topics(bridge, &names)) - .await - { - Ok(results) => results, - Err(_elapsed) => { - tracing::warn!( - distinct_topics = distinct_names.len(), - deadline_secs = REQUEST_DEADLINE.as_secs(), - "Metadata request's aggregate bridge work exceeded its deadline; \ - answering retriable instead of blocking further" - ); - names - .iter() - .map(|name| error_result(name.clone(), ERROR_REQUEST_TIMED_OUT)) - .collect() - } - } - } - } + Some(names) => resolve_requested_named_topics(bridge, &names).await, }; // `bounds_guard` cannot see this: it charges the projected response by *requested* element // (one topic name), but one real topic can carry up to Iggy's own per-topic partition cap - // (1000) - a handful of names, or one `list_kafka_topics()` call, can still expand into a - // response `bounds_guard` never had the information to price in before this bridge round - // trip returned. Checked here, before `encode_real_response` builds one - // `MetadataResponsePartition` per partition, not after - the expensive part is building that - // `Vec`, not encoding the bytes that follow it. + // (1000) - a handful of names can still expand into a response `bounds_guard` never had the + // information to price in before this bridge round trip returned. The all-topics arm is + // pre-truncated to this same budget above (`truncate_all_topics_to_frame_budget`) since its + // size is server-side, not client-controllable - closing over it would take down every + // client's bootstrap Metadata call, permanently, the moment the catalog grows past the + // trip point. This check is what still enforces the budget for the named-lookup arm, where + // the cap (100 distinct topics) bounds the request but not what each one costs to answer. let total_partitions: usize = results .iter() .filter(|result| result.error_code == ERROR_NONE) @@ -178,6 +152,45 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand ) } +/// Resolves a named-lookup Metadata request's topics: dedupes first so every path below (cap, +/// deadline, success) builds its response from the distinct set rather than one entry per +/// request occurrence - real Kafka answers a topic named more than once with one response entry, +/// not one per repeat, and re-expanding to match the request let a handful of repeats of one +/// large topic name amplify a response sized off the repeat count instead of the distinct count. +async fn resolve_requested_named_topics( + bridge: &IggyBridge, + names: &[StrBytes], +) -> Vec { + let distinct = dedup_topic_names(names); + if distinct.len() > MAX_BRIDGE_BACKED_TOPICS { + tracing::warn!( + distinct_topics = distinct.len(), + max = MAX_BRIDGE_BACKED_TOPICS, + "Metadata request addresses too many distinct topics; rejecting" + ); + return distinct + .iter() + .map(|name| error_result(name.clone(), ERROR_INVALID_REQUEST)) + .collect(); + } + + match tokio::time::timeout(REQUEST_DEADLINE, resolve_named_topics(bridge, &distinct)).await { + Ok(results) => results, + Err(_elapsed) => { + tracing::warn!( + distinct_topics = distinct.len(), + deadline_secs = REQUEST_DEADLINE.as_secs(), + "Metadata request's aggregate bridge work exceeded its deadline; answering \ + retriable instead of blocking further" + ); + distinct + .iter() + .map(|name| error_result(name.clone(), ERROR_REQUEST_TIMED_OUT)) + .collect() + } + } +} + /// Conservative per-partition byte cost of one encoded `MetadataResponsePartition` at v9 (the /// densest wire shape this handler emits): measured ~26 bytes (`error_code` + `partition_index` + /// `leader_id` + `leader_epoch` + a 1-entry `replica_nodes` + a 1-entry `isr_nodes` + an empty @@ -190,6 +203,48 @@ const fn response_would_exceed_frame_size(total_partitions: usize, max_frame_siz total_partitions.saturating_mul(RESPONSE_BYTES_PER_PARTITION) > max_frame_size } +/// Trims an all-topics [`IggyBridge::list_kafka_topics`] result to fit `max_frame_size`, keeping +/// as many whole topics (in listing order) as the budget allows. +/// +/// Unlike the named-lookup arm, the all-topics response's size is a server-side property (the +/// cluster's total partition count) that the requesting client never chose and cannot shrink - +/// closing the connection over it, as the shared frame-size guard below does for the +/// client-controllable named-lookup case, would make every all-topics Metadata call fail +/// identically and permanently once the catalog crosses the trip point. That call is the +/// bootstrap and refresh shape both librdkafka and the Java client use, so a hard close reads as +/// "broker down" and reconnect-loops rather than surfacing a usable, if partial, result. This +/// wire protocol has no pagination cursor to ask for the rest with, so a truncated list - honest +/// about being incomplete via the dropped entries, not via an error code - is what's available. +fn truncate_all_topics_to_frame_budget( + mut results: Vec, + max_frame_size: usize, +) -> Vec { + let mut cumulative_partitions = 0usize; + let mut keep = results.len(); + for (index, result) in results.iter().enumerate() { + if result.error_code != ERROR_NONE { + continue; + } + let next = cumulative_partitions + result.partitions_count as usize; + if response_would_exceed_frame_size(next, max_frame_size) { + keep = index; + break; + } + cumulative_partitions = next; + } + if keep < results.len() { + tracing::warn!( + total_topics = results.len(), + kept_topics = keep, + max_frame_size, + "All-topics Metadata response would exceed max_frame_size; truncating rather than \ + closing the connection" + ); + results.truncate(keep); + } + results +} + /// One resolved topic result for the real (bridge-backed) path - `partitions_count` is /// meaningless when `error_code != ERROR_NONE`. struct TopicResult { @@ -237,39 +292,30 @@ const fn error_result(name: StrBytes, error_code: i16) -> TopicResult { } } -/// Resolves every requested name, deduping first so a name repeated in the request (or asked -/// about more than once, which the wire technically allows) costs one `get_kafka_topic` round -/// trip, not one per occurrence. -async fn resolve_named_topics(bridge: &IggyBridge, names: &[StrBytes]) -> Vec { +/// Drops repeats, keeping first-seen order so a capped or timed-out response still answers a +/// deterministic prefix of the request rather than an arbitrary hash-order subset. +fn dedup_topic_names(names: &[StrBytes]) -> Vec { let mut seen = HashSet::with_capacity(names.len()); - let mut distinct = Vec::new(); + let mut distinct = Vec::with_capacity(names.len()); for name in names { if seen.insert(name.as_str()) { distinct.push(name.clone()); } } + distinct +} - let mut results_by_name: HashMap<&str, TopicResult> = HashMap::with_capacity(distinct.len()); - for name in &distinct { - let result = lookup_one_topic(bridge, name.clone()).await; - results_by_name.insert(name.as_str(), result); +/// Resolves each of `names`, one `get_kafka_topic` round trip per entry. +/// +/// `names` must already be the distinct set ([`dedup_topic_names`]) - this makes no attempt to +/// re-derive or re-expand it, so a caller passing a list with repeats gets one round trip and one +/// result per repeat, silently paying for the amplification this split was written to avoid. +async fn resolve_named_topics(bridge: &IggyBridge, names: &[StrBytes]) -> Vec { + let mut results = Vec::with_capacity(names.len()); + for name in names { + results.push(lookup_one_topic(bridge, name.clone()).await); } - - names - .iter() - .map(|name| { - // Always present: `distinct` (and so results_by_name) was built from exactly these - // same requested names, just above. - let cached = results_by_name - .get(name.as_str()) - .expect("every requested name was resolved above"); - TopicResult { - name: name.clone(), - error_code: cached.error_code, - partitions_count: cached.partitions_count, - } - }) - .collect() + results } /// # Errors diff --git a/gateways/kafka/tests/create_topics_real_bridge_tests.rs b/gateways/kafka/tests/create_topics_real_bridge_tests.rs index daf722aadc..a0679670e7 100644 --- a/gateways/kafka/tests/create_topics_real_bridge_tests.rs +++ b/gateways/kafka/tests/create_topics_real_bridge_tests.rs @@ -36,8 +36,9 @@ use serial_test::serial; use iggy_gateway_kafka::bridge::IggyBridge; use iggy_gateway_kafka::protocol::api::{ - BrokerAdvertise, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, ERROR_INVALID_REQUEST, - ERROR_INVALID_TOPIC_EXCEPTION, ERROR_NONE, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, + BrokerAdvertise, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, + ERROR_INVALID_REPLICA_ASSIGNMENT, ERROR_INVALID_REQUEST, ERROR_INVALID_TOPIC_EXCEPTION, + ERROR_NONE, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, }; use iggy_gateway_kafka::protocol::handlers::create_topics; @@ -350,6 +351,85 @@ async fn create_topics_rejects_an_explicit_partition_count_alongside_a_manual_as assert!(streams.is_empty()); } +/// Regression test: real Kafka's `ReplicationControlManager` requires a manual assignment's +/// partition indices to be exactly `0..assignments.len()`, each appearing once - a topic whose +/// assignment keys are `{5, 7}` has the right *length* for a 2-partition topic but names neither +/// partition `0` nor `1`. Checking only `assignments.len()` (the pre-fix behavior) would silently +/// create a 2-partition topic whose real partitions are `0`/`1`, disagreeing with what the client +/// asked for. +#[tokio::test] +#[serial] +async fn create_topics_rejects_a_manual_assignment_with_non_consecutive_indices() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topic = TopicSpec { + replication_factor: -1, + assignments: &[5, 7], + ..TopicSpec::new("orders", -1) + }; + let (error_code, _) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_INVALID_REPLICA_ASSIGNMENT); + + let raw = raw_client(&server).await; + let streams = raw.get_streams().await.expect("get_streams call"); + assert!(streams.is_empty()); +} + +/// Regression test: a manual assignment repeating one partition index (`{0, 0}`) is the other +/// half of the same real-Kafka rule - same index set size as a valid 2-partition assignment, but +/// not the distinct `0..2` it requires. +#[tokio::test] +#[serial] +async fn create_topics_rejects_a_manual_assignment_with_a_duplicate_index() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let topic = TopicSpec { + replication_factor: -1, + assignments: &[0, 0], + ..TopicSpec::new("orders", -1) + }; + let (error_code, _) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_INVALID_REPLICA_ASSIGNMENT); +} + +/// Regression test: `error_message` must not re-embed the topic name `CreatableTopicResult.name` +/// already carries. `BridgeError::InvalidKafkaTopicName`'s `Display` embeds the full (invalid) +/// name in its text; validation runs before any bridge I/O, so an unfixed double-echo here costs +/// nothing to trigger and roughly doubles the response per invalid name for free. +#[tokio::test] +#[serial] +async fn create_topics_error_message_does_not_repeat_the_topic_name() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let bad_name = "has a space"; + let topic = TopicSpec::new(bad_name, 1); + let body = build_request(&[topic], false); + let outcome = create_topics::handle(&state, REQUEST_VERSION, body).await; + let resp_body = outcome.expect_response("CreateTopics request always answers"); + + let mut d = Decoder::new(resp_body); + let _throttle_time_ms = d.read_i32().expect("throttle_time_ms"); + let _topics_plus_one = d.read_varint().expect("topics array count"); + let _name = d.read_compact_nullable_string().expect("topic name"); + let error_code = d.read_i16().expect("error_code"); + let error_message = d + .read_compact_nullable_string() + .expect("error_message") + .unwrap_or_default(); + + assert_eq!(error_code, ERROR_INVALID_TOPIC_EXCEPTION); + assert!( + !error_message.contains(bad_name), + "error_message repeats the topic name `.name` already carries: {error_message:?}" + ); +} + /// Regression test: real Kafka refuses every occurrence of a duplicate topic name in one request /// with `INVALID_REQUEST` (42) and creates nothing - not a first-wins create plus a /// `TOPIC_ALREADY_EXISTS` for the rest, which would let a client observe a create it never got a diff --git a/gateways/kafka/tests/metadata_real_bridge_tests.rs b/gateways/kafka/tests/metadata_real_bridge_tests.rs index a4a4f4da60..b5e97abbd7 100644 --- a/gateways/kafka/tests/metadata_real_bridge_tests.rs +++ b/gateways/kafka/tests/metadata_real_bridge_tests.rs @@ -316,12 +316,50 @@ async fn a_response_projected_over_max_frame_size_closes_instead_of_answering() assert!(outcome.is_close(), "expected Close, got {outcome:?}"); } -/// Regression test: a topic named more than once in one request must still resolve every -/// occurrence correctly, not just avoid a crash - proves the dedup-by-name cache is actually -/// populated and read back correctly, not merely harmless. +/// Regression test: unlike the named-lookup path above, an all-topics response over budget must +/// be truncated, not closed - its size is the cluster's own catalog, not anything the requesting +/// client chose or can shrink, so closing would make every all-topics call (the bootstrap/refresh +/// shape both librdkafka and the Java client use) fail identically and permanently. #[tokio::test] #[serial] -async fn a_topic_named_twice_in_one_request_resolves_both_occurrences_correctly() { +async fn an_all_topics_response_over_max_frame_size_truncates_instead_of_closing() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let (state, seed) = connected_state(&server).await; + seed.ensure_stream_and_topic("small", 1) + .await + .expect("seed a topic that alone fits any reasonable budget"); + seed.ensure_stream_and_topic("big", 50) + .await + .expect("seed a topic that alone exceeds the tiny budget below"); + + // 50 partitions * 64 bytes/partition (this crate's own conservative per-partition estimate) + // = 3200 bytes, comfortably over a 512-byte max_frame_size - so the catalog as a whole cannot + // fit, but neither topic's own partition count is malformed or attacker-shaped. + let tiny_state = GatewayState::new(state.broker, state.bridge, 512); + let topics = send(&tiny_state, None).await; + assert!( + topics.len() < 2, + "expected truncation to drop at least one topic, got {topics:?}" + ); + let total_partitions: usize = topics + .iter() + .map(|(_, _, partitions)| partitions.len()) + .sum(); + assert!( + total_partitions * 64 <= 512, + "kept topics must themselves fit the budget, got {total_partitions} partitions" + ); +} + +/// Regression test: a topic named more than once in one request must resolve to exactly one +/// response entry, not one per repeat - real Kafka answers a `Metadata` request naming the same +/// topic twice with one entry, and this bridge re-expanding to match the request is what let a +/// handful of repeats of one large topic name amplify a response sized off the repeat count +/// instead of the distinct count. +#[tokio::test] +#[serial] +async fn a_topic_named_twice_in_one_request_resolves_to_one_entry() { let data_dir = tempfile::tempdir().expect("tempdir"); let server = TestServer::spawn(data_dir.path()).await; let (state, seed) = connected_state(&server).await; @@ -330,17 +368,15 @@ async fn a_topic_named_twice_in_one_request_resolves_both_occurrences_correctly( .expect("seed the topic"); let topics = send(&state, Some(&["orders", "orders"])).await; - assert_eq!(topics.len(), 2); - for topic in &topics { - assert_eq!( - topic, - &( - Some("orders".to_string()), - ERROR_NONE, - expected_partitions(2) - ) - ); - } + assert_eq!(topics.len(), 1); + assert_eq!( + topics[0], + ( + Some("orders".to_string()), + ERROR_NONE, + expected_partitions(2) + ) + ); } /// Regression test: a named-lookup request addressing more than the bridge-backed topic cap must From 5546dc8ea4384078aeeb74e6521920ef2803ff06 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Wed, 23 Sep 2026 20:00:35 -0400 Subject: [PATCH 04/10] Fixed on review comments --- .../kafka/src/bridge/iggy_bridge/topics.rs | 17 ++- .../src/protocol/handlers/create_topics.rs | 69 +++++++++- .../kafka/src/protocol/handlers/metadata.rs | 126 +++++++++++++----- .../tests/create_topics_real_bridge_tests.rs | 2 +- 4 files changed, 170 insertions(+), 44 deletions(-) diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index 75d25b5536..575728a3bd 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -344,8 +344,15 @@ impl IggyBridge { /// Every Kafka-visible topic: the target of every configured /// [`TopicMapping`](crate::bridge::topic_map::TopicMapping) override that actually exists in /// Iggy, plus every topic in the default stream that isn't itself one of those override - /// targets - checked so an overridden topic is never listed twice, once under its Kafka-side - /// name and once under its raw Iggy name. + /// targets and isn't itself named the same as an override key - checked so an overridden + /// topic is never listed twice, once under its Kafka-side name and once under its raw Iggy + /// name, and so a raw default-stream topic never masquerades under a Kafka-side name an + /// override has already claimed for different data. The second check matters for a chained + /// override (`foo -> (kafka, bar)`, `bar -> (other, x)`): without it, a raw Iggy topic + /// literally named `foo` sitting in the default stream would be reported a second time under + /// the same `foo` name the override loop already emitted (backed by `kafka/bar`'s data), and + /// that second `foo` would be unreachable by name anyway, since `get_kafka_topic("foo")` + /// always resolves through the override to `kafka/bar`, never to the raw `kafka/foo`. /// /// An Iggy stream this bridge has no mapping rule pointing at (neither the default stream nor /// any override's target) holds data no Kafka client ever named - deliberately excluded, the @@ -358,9 +365,11 @@ impl IggyBridge { pub async fn list_kafka_topics(&self) -> Result, BridgeError> { let default_stream = self.config.topic_mapping.default_stream(); let mut default_stream_override_targets: HashSet<&str> = HashSet::new(); + let mut override_keys: HashSet<&str> = HashSet::new(); let mut results = Vec::new(); for (kafka_topic, over) in self.config.topic_mapping.overrides() { + override_keys.insert(kafka_topic); if over.stream == default_stream { default_stream_override_targets.insert(over.topic.as_str()); } @@ -379,7 +388,9 @@ impl IggyBridge { { let topics = with_request_timeout(self.client.get_topics(&default_stream_id)).await?; for topic in topics { - if default_stream_override_targets.contains(topic.name.as_str()) { + if default_stream_override_targets.contains(topic.name.as_str()) + || override_keys.contains(topic.name.as_str()) + { continue; } results.push(KafkaTopicMetadata { diff --git a/gateways/kafka/src/protocol/handlers/create_topics.rs b/gateways/kafka/src/protocol/handlers/create_topics.rs index d4a43512f2..6dd7783c25 100644 --- a/gateways/kafka/src/protocol/handlers/create_topics.rs +++ b/gateways/kafka/src/protocol/handlers/create_topics.rs @@ -24,7 +24,7 @@ use bytes::Bytes; use iggy::prelude::IggyError; use kafka_protocol::messages::create_topics_request::{CreatableReplicaAssignment, CreatableTopic}; use kafka_protocol::messages::create_topics_response::CreatableTopicResult; -use kafka_protocol::messages::{CreateTopicsRequest, CreateTopicsResponse, TopicName}; +use kafka_protocol::messages::{BrokerId, CreateTopicsRequest, CreateTopicsResponse, TopicName}; use kafka_protocol::protocol::StrBytes; use crate::bridge::{BridgeError, IggyBridge, TopicCreationOutcome}; @@ -322,9 +322,11 @@ fn error_message_for(err: &BridgeError) -> String { /// An assignment's own partition indices are checked too, not just its length: real Kafka /// requires the key set to be exactly `0..assignments.len()`, each index appearing once, and /// rejects anything else - a duplicate or non-consecutive index (`{5: [...], 7: [...]}`) - with -/// `INVALID_REPLICA_ASSIGNMENT` (39), the one condition that code exists for. This bridge doesn't -/// model per-partition replica placement, so only the index set is checked, not each entry's -/// replica list. +/// `INVALID_REPLICA_ASSIGNMENT` (39). Each entry's own replica list is checked too, not just the +/// index: this gateway advertises exactly one broker (node id 1, `metadata.rs`'s `BrokerId(1)`), +/// so the only legal replica list is `[1]` - never empty, never repeating it, never naming a +/// broker Metadata never advertised. Real Kafka's `ReplicationControlManager` validates the whole +/// binding this way, not just the index. /// /// With no assignments, `num_partitions = -1` / `replication_factor = -1` mean "use the broker /// default" from v4+ (pre-v4 requires an explicit positive value for both, since v2/v3 have no @@ -337,7 +339,9 @@ fn validate_create_topic_shape( if topic.num_partitions != -1 || topic.replication_factor != -1 { return Err(ERROR_INVALID_REQUEST); } - if !assignment_indices_are_consecutive_from_zero(&topic.assignments) { + if !assignment_indices_are_consecutive_from_zero(&topic.assignments) + || !assignment_replicas_are_valid(&topic.assignments) + { return Err(ERROR_INVALID_REPLICA_ASSIGNMENT); } return Ok(u32::try_from(topic.assignments.len()).unwrap_or(DEFAULT_PARTITION_COUNT)); @@ -391,6 +395,18 @@ fn assignment_indices_are_consecutive_from_zero( && indices.last().copied() == i32::try_from(last_index).ok() } +/// The only legal replica list for any partition this gateway assigns: this gateway advertises +/// exactly one broker (node id 1, `metadata.rs`'s `BrokerId(1)`), so `[1]` is the sole valid +/// shape - length 1 rules out both empty and a duplicated id, and the value itself rules out a +/// broker Metadata never advertised. +const SINGLE_BROKER_ID: i32 = 1; + +fn assignment_replicas_are_valid(assignments: &[CreatableReplicaAssignment]) -> bool { + assignments + .iter() + .all(|assignment| assignment.broker_ids == [BrokerId(SINGLE_BROKER_ID)]) +} + /// Well-formed `CreateTopics` response with a single placeholder topic. /// /// # Errors @@ -481,7 +497,16 @@ mod tests { fn assignment(partition_index: i32) -> CreatableReplicaAssignment { CreatableReplicaAssignment::default() .with_partition_index(partition_index) - .with_broker_ids(vec![0.into()]) + .with_broker_ids(vec![SINGLE_BROKER_ID.into()]) + } + + fn assignment_with_replicas( + partition_index: i32, + broker_ids: Vec, + ) -> CreatableReplicaAssignment { + CreatableReplicaAssignment::default() + .with_partition_index(partition_index) + .with_broker_ids(broker_ids.into_iter().map(BrokerId).collect()) } #[test] @@ -514,6 +539,38 @@ mod tests { assert_eq!(validate_create_topic_shape(2, &topic), Ok(2)); } + #[test] + fn manual_assignment_with_an_empty_replica_list_is_rejected() { + let topic = + creatable_topic(-1, -1).with_assignments(vec![assignment_with_replicas(0, vec![])]); + assert_eq!( + validate_create_topic_shape(2, &topic), + Err(ERROR_INVALID_REPLICA_ASSIGNMENT) + ); + } + + #[test] + fn manual_assignment_with_a_duplicated_replica_is_rejected() { + let topic = + creatable_topic(-1, -1).with_assignments(vec![assignment_with_replicas(0, vec![1, 1])]); + assert_eq!( + validate_create_topic_shape(2, &topic), + Err(ERROR_INVALID_REPLICA_ASSIGNMENT) + ); + } + + #[test] + fn manual_assignment_naming_an_unregistered_broker_is_rejected() { + // This gateway advertises exactly one broker (node id 1) - naming any other id claims a + // replica placement on a broker Metadata never advertised. + let topic = + creatable_topic(-1, -1).with_assignments(vec![assignment_with_replicas(0, vec![7])]); + assert_eq!( + validate_create_topic_shape(2, &topic), + Err(ERROR_INVALID_REPLICA_ASSIGNMENT) + ); + } + #[test] fn zero_partitions_is_rejected_regardless_of_version() { let topic = creatable_topic(0, 1); diff --git a/gateways/kafka/src/protocol/handlers/metadata.rs b/gateways/kafka/src/protocol/handlers/metadata.rs index a5d315f530..b3451646a2 100644 --- a/gateways/kafka/src/protocol/handlers/metadata.rs +++ b/gateways/kafka/src/protocol/handlers/metadata.rs @@ -132,14 +132,10 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand // client's bootstrap Metadata call, permanently, the moment the catalog grows past the // trip point. This check is what still enforces the budget for the named-lookup arm, where // the cap (100 distinct topics) bounds the request but not what each one costs to answer. - let total_partitions: usize = results - .iter() - .filter(|result| result.error_code == ERROR_NONE) - .map(|result| result.partitions_count as usize) - .sum(); - if response_would_exceed_frame_size(total_partitions, state.max_frame_size) { + let total_bytes: usize = results.iter().map(estimated_response_bytes).sum(); + if response_would_exceed_frame_size(total_bytes, state.max_frame_size) { tracing::warn!( - total_partitions, + total_bytes, max_frame_size = state.max_frame_size, "Metadata response would exceed max_frame_size; closing connection" ); @@ -199,8 +195,37 @@ async fn resolve_requested_named_topics( /// the measured minimum. const RESPONSE_BYTES_PER_PARTITION: usize = 64; -const fn response_would_exceed_frame_size(total_partitions: usize, max_frame_size: usize) -> bool { - total_partitions.saturating_mul(RESPONSE_BYTES_PER_PARTITION) > max_frame_size +/// Fixed per-`MetadataResponseTopic` cost, independent of name length or partition count: +/// `error_code`(2) + `is_internal`(1) + the `partitions` array's own compact-length varint +/// (charged at its 5-byte worst case) + `topic_authorized_operations`(4) + empty tagged fields(1). +/// Every result pays this once - `encode_real_response` emits a full wrapper even for an error +/// result, just with an empty `partitions` array, so this cost isn't conditional on `ERROR_NONE`. +const RESPONSE_BYTES_TOPIC_OVERHEAD: usize = 13; + +/// Estimated encoded byte cost of one [`TopicResult`]: the fixed per-topic overhead, the name's +/// own bytes (compact string: length plus a short varint prefix, charged at a flat +2), and - +/// only for a successful result, since `encode_real_response` sends an empty array otherwise - +/// [`RESPONSE_BYTES_PER_PARTITION`] per partition. +/// +/// `bounds_guard` cannot see any of this: it charges the request by name count alone, with no way +/// to know a name's length, a topic's per-entry overhead, or its partition count before this +/// bridge round trip returns real data. Charging partitions alone (this function's earlier form) +/// undercounted every result: a name costs bytes whether or not the lookup succeeded, and a +/// zero-partition or errored result - free under a partitions-only charge - still costs a full +/// wrapper to encode. 500 topics with 255-byte names and one partition each encode to ~146 KB +/// against the ~32 KB a partitions-only charge would have priced in. +fn estimated_response_bytes(result: &TopicResult) -> usize { + let name_bytes = result.name.as_str().len() + 2; + let partition_bytes = if result.error_code == ERROR_NONE { + (result.partitions_count as usize).saturating_mul(RESPONSE_BYTES_PER_PARTITION) + } else { + 0 + }; + RESPONSE_BYTES_TOPIC_OVERHEAD + name_bytes + partition_bytes +} + +const fn response_would_exceed_frame_size(total_bytes: usize, max_frame_size: usize) -> bool { + total_bytes > max_frame_size } /// Trims an all-topics [`IggyBridge::list_kafka_topics`] result to fit `max_frame_size`, keeping @@ -219,18 +244,15 @@ fn truncate_all_topics_to_frame_budget( mut results: Vec, max_frame_size: usize, ) -> Vec { - let mut cumulative_partitions = 0usize; + let mut cumulative_bytes = 0usize; let mut keep = results.len(); for (index, result) in results.iter().enumerate() { - if result.error_code != ERROR_NONE { - continue; - } - let next = cumulative_partitions + result.partitions_count as usize; + let next = cumulative_bytes + estimated_response_bytes(result); if response_would_exceed_frame_size(next, max_frame_size) { keep = index; break; } - cumulative_partitions = next; + cumulative_bytes = next; } if keep < results.len() { tracing::warn!( @@ -535,30 +557,66 @@ mod tests { } #[test] - fn response_would_exceed_frame_size_rejects_a_projection_over_the_limit() { - // bounds_guard cannot see this cost: one requested topic name can expand into up to - // Iggy's own per-topic partition cap (1000) worth of MetadataResponsePartition entries, - // information only known after the bridge round trip this check runs after. - let max_frame_size = 1024; - let total_partitions = (max_frame_size / RESPONSE_BYTES_PER_PARTITION) + 1; - assert!(response_would_exceed_frame_size( - total_partitions, - max_frame_size - )); + fn response_would_exceed_frame_size_rejects_bytes_over_the_limit() { + assert!(response_would_exceed_frame_size(1025, 1024)); + } + + #[test] + fn response_would_exceed_frame_size_accepts_bytes_at_the_limit() { + assert!(!response_would_exceed_frame_size(1024, 1024)); + } + + fn topic_result(name: &'static str, error_code: i16, partitions_count: u32) -> TopicResult { + TopicResult { + name: StrBytes::from_static_str(name), + error_code, + partitions_count, + } + } + + #[test] + fn estimated_response_bytes_charges_name_and_overhead_even_at_zero_partitions() { + // Regression: a partitions-only charge (the earlier form of this function) priced a + // zero-partition result at 0, when `encode_real_response` still emits a full + // `MetadataResponseTopic` wrapper with the name in it. + let result = topic_result("orders", ERROR_NONE, 0); + assert_eq!( + estimated_response_bytes(&result), + RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 + ); } #[test] - fn response_would_exceed_frame_size_accepts_a_projection_at_or_under_the_limit() { - let max_frame_size = 1024; - let total_partitions = max_frame_size / RESPONSE_BYTES_PER_PARTITION; - assert!(!response_would_exceed_frame_size( - total_partitions, - max_frame_size - )); + fn estimated_response_bytes_charges_overhead_and_name_on_an_error_result_too() { + // Regression: the earlier per-topic sum filtered to `error_code == ERROR_NONE` before + // charging anything, so an all-erroring batch (e.g. every requested name unknown) was + // priced at 0 bytes total despite `encode_real_response` emitting one wrapper per result + // regardless of its error code. + let result = topic_result("orders", ERROR_UNKNOWN_TOPIC_OR_PARTITION, 0); + assert_eq!( + estimated_response_bytes(&result), + RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 + ); + } + + #[test] + fn estimated_response_bytes_ignores_partitions_count_on_an_error_result() { + // `encode_real_response` sends an empty `partitions` array whenever `error_code != + // ERROR_NONE`, so a stale non-zero `partitions_count` on an error result (shouldn't occur, + // but nothing enforces it structurally) must not inflate the charge. + let ok = topic_result("orders", ERROR_NONE, 10); + let err = topic_result("orders", ERROR_UNKNOWN_TOPIC_OR_PARTITION, 10); + assert_eq!( + estimated_response_bytes(&err), + RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 + ); + assert!(estimated_response_bytes(&ok) > estimated_response_bytes(&err)); } #[test] - fn response_would_exceed_frame_size_does_not_overflow_on_a_pathological_partition_count() { - assert!(response_would_exceed_frame_size(usize::MAX, 1024)); + fn estimated_response_bytes_charges_longer_names_more() { + let short = topic_result("a", ERROR_NONE, 0); + let long = topic_result("a-much-longer-topic-name", ERROR_NONE, 0); + assert!(estimated_response_bytes(&long) > estimated_response_bytes(&short)); } } diff --git a/gateways/kafka/tests/create_topics_real_bridge_tests.rs b/gateways/kafka/tests/create_topics_real_bridge_tests.rs index a0679670e7..ad94710575 100644 --- a/gateways/kafka/tests/create_topics_real_bridge_tests.rs +++ b/gateways/kafka/tests/create_topics_real_bridge_tests.rs @@ -88,7 +88,7 @@ fn build_request(topics: &[TopicSpec], validate_only: bool) -> Bytes { for &partition_index in topic.assignments { enc.write_i32(partition_index); enc.write_varint(2); // one broker - enc.write_i32(0); // broker_id + enc.write_i32(1); // broker_id - the one broker this gateway advertises (node id 1) enc.write_empty_tagged_fields(); } From 9d5eda87d95d89cc9147790524138923f2afd92a Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Thu, 24 Sep 2026 07:07:28 -0400 Subject: [PATCH 05/10] Fixed review comments --- gateways/kafka/README.md | 16 +- gateways/kafka/docs/SCOPE.md | 19 +- .../kafka/src/bridge/iggy_bridge/topics.rs | 159 ++++++++--- gateways/kafka/src/protocol/api.rs | 5 + .../src/protocol/handlers/create_topics.rs | 269 +++++++++++++----- .../kafka/src/protocol/handlers/metadata.rs | 257 ++++++++++------- .../tests/create_topics_real_bridge_tests.rs | 29 +- .../kafka/tests/metadata_real_bridge_tests.rs | 19 +- 8 files changed, 555 insertions(+), 218 deletions(-) diff --git a/gateways/kafka/README.md b/gateways/kafka/README.md index aa39186f7a..b95fcc9441 100644 --- a/gateways/kafka/README.md +++ b/gateways/kafka/README.md @@ -2,14 +2,18 @@ Foundation layer for [apache/iggy#3421](https://github.com/apache/iggy/issues/3421): a TCP listener on the Kafka wire port that decodes requests, validates scoped API keys and versions, and returns stub responses. -> **Stub warning:** Produce, Fetch, and ListOffsets still don't persist or read real data - they -> return retriable `NOT_LEADER_OR_FOLLOWER` (6) so clients keep data locally / retry elsewhere -> instead of trusting a fake success. CreateTopics and Metadata are wired to the Iggy bridge: with -> `IGGY_KAFKA_BRIDGE_ENABLED=true`, CreateTopics creates a real Iggy stream/topic and Metadata +> **Stub warning:** Produce and Fetch still don't persist or read real data - they return +> retriable `NOT_LEADER_OR_FOLLOWER` (6) so clients keep data locally / retry elsewhere instead of +> trusting a fake success. CreateTopics, Metadata, and ListOffsets are wired to the Iggy bridge: +> with `IGGY_KAFKA_BRIDGE_ENABLED=true`, CreateTopics creates a real Iggy stream/topic, Metadata > reports real topics and partition counts (a topic not requested by name and not found is > silently absent from a null-topics "list all" response, and `UNKNOWN_TOPIC_OR_PARTITION` when -> named explicitly); with the bridge off (the default), both stay stubs - CreateTopics answers -> `NOT_CONTROLLER` (41), Metadata reports every requested topic unknown. See +> named explicitly), and ListOffsets answers `EARLIEST`/`LATEST` from real partition state; with +> the bridge off (the default), all three stay stubs - CreateTopics answers `NOT_CONTROLLER` (41), +> Metadata reports every requested topic unknown, and ListOffsets answers `NOT_LEADER_OR_FOLLOWER` +> (6). **CreateTopics has no authentication gate yet**: with the bridge on, any client that can +> reach this port can create topics (up to 1000 partitions each) as the bridge's own Iggy user, +> until SASL ([#3549](https://github.com/apache/iggy/issues/3549)) lands. See > [docs/SCOPE.md](docs/SCOPE.md). ## Run diff --git a/gateways/kafka/docs/SCOPE.md b/gateways/kafka/docs/SCOPE.md index 9f9ebfaaa7..b90cbae6b6 100644 --- a/gateways/kafka/docs/SCOPE.md +++ b/gateways/kafka/docs/SCOPE.md @@ -178,13 +178,18 @@ below it are still open for the issues that build on top of it. lookups. - The response-size cap above only covers the "all topics" and per-name-expansion cases. A named lookup is separately bounded on the request side: names repeated in one request are - deduped up front to one response entry, not just one `get_kafka_topic` round trip - real - Kafka answers a topic named twice in one request with one response entry, and re-expanding to - match the request would let a handful of repeats of one large topic name amplify a response - sized off the repeat count instead of the distinct count. A lookup naming more than 100 - distinct topics is rejected outright (`INVALID_REQUEST`, no bridge call for any of them - - `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode ceiling, not a usability one), - and the whole lookup's aggregate bridge work runs under a fixed 20s wall-clock deadline + deduped up front to one response entry, not just one bridge round trip - real Kafka answers a + topic named twice in one request with one response entry, and re-expanding to match the + request would let a handful of repeats of one large topic name amplify a response sized off + the repeat count instead of the distinct count. No cap on distinct names: `bounds_guard`'s + `MAX_REQUEST_ELEMENTS` (4,096) is still the pre-decode ceiling, but `IggyBridge::get_kafka_topics` + batches by the *stream* each name resolves to rather than paying one round trip per name, so + the real bridge cost is bounded by distinct streams involved (config-time-bounded), not by + how many names the client asks about. A per-name cap here previously permanently broke a + long-lived Java producer once its `ProducerMetadata`'s cumulative tracked-topic set - resent + in full on every refresh - crossed the cap: every later request answered every topic + `INVALID_REQUEST`, and the producer had no way to shrink its own tracked set to recover. The + whole lookup's aggregate bridge work still runs under a fixed 20s wall-clock deadline (`Metadata` carries no `timeout_ms` field in any version, unlike `CreateTopics`, so this cannot be client-honored) - a deadline that fires answers every name `REQUEST_TIMED_OUT` rather than continuing to hold the shared lockstep `IggyClient`. diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index 575728a3bd..945ebcc0fb 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -17,11 +17,12 @@ //! Stream and topic provisioning. -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use iggy::prelude::{ Identifier, IggyError, StreamClient, TopicClient, TopicCreateOptions, TopicDetails, }; +use kafka_protocol::protocol::StrBytes; use tracing::{debug, info}; use super::{IggyBridge, with_request_timeout}; @@ -81,11 +82,12 @@ impl IggyBridge { /// default should be for this bridge is a decision for whichever of `#3535`/`#3536` first /// calls this with a real Kafka request in hand, not one to invent here ahead of that need. /// - /// No caching: every call pays a `get_stream` and a `get_topic` (two round trips once both - /// already exist), even for a topic this same bridge already confirmed a moment ago. A cache - /// keyed on `kafka_topic` would remove that cost, but would also have to answer "how does a - /// cache entry ever get invalidated" - the topic being deleted and recreated with a different - /// partition count out from under a stale cache entry is exactly + /// No caching: every call pays a stream-create attempt ([`Self::ensure_stream`]) and a + /// `get_topic` (two round trips once both already exist), even for a topic this same bridge + /// already confirmed a moment ago. A cache keyed on `kafka_topic` would remove that cost, but + /// would also have to answer "how does a cache entry ever get invalidated" - the topic being + /// deleted and recreated with a different partition count out from under a stale cache entry + /// is exactly /// `ensure_topic_targets_the_streams_live_incarnation_after_a_delete_and_recreate`'s own /// scenario, and a naive cache breaks that guarantee to save two round trips. Whatever wires /// this into `#3535`/`#3536` should call it once per topic and remember that it did, rather @@ -132,23 +134,24 @@ impl IggyBridge { /// `stm/stream.rs`: freed keys are reused by the next created stream), so a numeric id /// captured here could point at a *different* stream by the time `ensure_topic` uses it, if /// this stream is deleted and recreated in between. The name has no such window. + /// + /// Attempts the create directly rather than probing existence first: `get_stream` answers + /// with `StreamDetails`, which embeds every topic header in the stream (`iggy_common`'s + /// `StreamDetails.topics: Vec`), not just the stream's own metadata - a probe paid on + /// every call, including the steady-state case where the stream (almost always) already + /// exists, could cost one full per-stream topic listing per topic in a `CreateTopics` batch + /// (up to 100 today). `StreamNameAlreadyExists` on the create attempt is exactly as + /// informative as a prior `get_stream` would have been - the stream exists either way - so + /// this pays one round trip in every case instead of up to two in the common one. async fn ensure_stream(&self, stream_name: &str) -> Result { let identifier = Identifier::named(stream_name).map_err(BridgeError::Iggy)?; - if with_request_timeout(self.client.get_stream(&identifier)) - .await? - .is_some() - { - debug!("Iggy stream '{stream_name}' already exists"); - return Ok(identifier); - } - match with_request_timeout(self.client.create_stream(stream_name)).await { Ok(_created) => { info!("created Iggy stream '{stream_name}'"); Ok(identifier) } Err(BridgeError::Iggy(IggyError::StreamNameAlreadyExists(_))) => { - // Lost a create race - the name now exists regardless of who won it. + debug!("Iggy stream '{stream_name}' already exists"); Ok(identifier) } Err(err) => Err(err), @@ -282,6 +285,70 @@ impl IggyBridge { with_request_timeout(self.client.get_topic(&stream_id, &topic_id)).await } + /// Resolves many Kafka-visible names at once, one entry per input in the same order. + /// + /// Batches by the Iggy stream each name resolves to, rather than paying one round trip per + /// name: most requested names share the default stream, and override targets are a handful + /// at most, so the real round-trip cost is the number of *distinct streams* involved, not the + /// number of names. A caller with many requested names but few distinct target streams pays + /// one `get_topics` call per stream - this is what makes it safe to resolve an unbounded + /// number of names in one call, unlike looping [`Self::get_kafka_topic`] per name, which pays + /// one round trip per name regardless of how many share a stream and needs its own + /// caller-side cap to keep that cost bounded. + /// + /// # Errors + /// + /// Returns [`BridgeError::Timeout`] if a call takes longer than `REQUEST_TIMEOUT`. Returns + /// [`BridgeError::Iggy`] for connectivity/auth failures. + pub async fn get_kafka_topics( + &self, + kafka_topics: &[StrBytes], + ) -> Result)>, BridgeError> { + let resolved: Vec<(StrBytes, String, String)> = kafka_topics + .iter() + .map(|kafka_topic| { + let (stream_name, topic_name) = + self.config.topic_mapping.resolve(kafka_topic.as_str()); + ( + kafka_topic.clone(), + stream_name.to_string(), + topic_name.to_string(), + ) + }) + .collect(); + + let distinct_streams: HashSet = resolved + .iter() + .map(|(_, stream_name, _)| stream_name.clone()) + .collect(); + + let mut topics_by_stream: HashMap> = + HashMap::with_capacity(distinct_streams.len()); + for stream_name in distinct_streams { + let stream_id = Identifier::named(&stream_name).map_err(BridgeError::Iggy)?; + let topics = with_request_timeout(self.client.get_topics(&stream_id)).await?; + let by_name = topics + .into_iter() + .map(|topic| (topic.name, topic.partitions_count)) + .collect(); + topics_by_stream.insert(stream_name, by_name); + } + + Ok(resolved + .into_iter() + .map(|(kafka_topic, stream_name, topic_name)| { + let metadata = topics_by_stream + .get(&stream_name) + .and_then(|topics| topics.get(&topic_name)) + .map(|&partitions_count| KafkaTopicMetadata { + kafka_topic: kafka_topic.as_str().to_string(), + partitions_count, + }); + (kafka_topic, metadata) + }) + .collect()) + } + /// Creates the Iggy stream/topic backing `kafka_topic`, or reports that it already exists. /// /// Atomic from this call's perspective, unlike a separate existence check @@ -373,31 +440,57 @@ impl IggyBridge { if over.stream == default_stream { default_stream_override_targets.insert(over.topic.as_str()); } - if let Some(details) = self.get_kafka_topic(kafka_topic).await? { - results.push(KafkaTopicMetadata { + match self.get_kafka_topic(kafka_topic).await { + Ok(Some(details)) => results.push(KafkaTopicMetadata { kafka_topic: kafka_topic.to_string(), partitions_count: details.partitions_count, - }); + }), + Ok(None) => {} + // One override naming a topic this caller (the bridge user) can't read must not + // abort every other topic's listing - real Kafka's own DescribeTopics skips a + // topic the caller lacks ACLs for rather than failing the whole response. Any + // other error kind (timeout, connectivity) still propagates: those aren't + // per-topic facts, they mean nothing in this response can be trusted. + Err(BridgeError::Iggy(IggyError::Unauthorized)) => { + debug!( + "skipping override '{kafka_topic}' -> '{}/{}' in Metadata: caller is \ + unauthorized to read it", + over.stream, over.topic + ); + } + Err(err) => return Err(err), } } let default_stream_id = Identifier::named(default_stream).map_err(BridgeError::Iggy)?; - if with_request_timeout(self.client.get_stream(&default_stream_id)) - .await? - .is_some() - { - let topics = with_request_timeout(self.client.get_topics(&default_stream_id)).await?; - for topic in topics { - if default_stream_override_targets.contains(topic.name.as_str()) - || override_keys.contains(topic.name.as_str()) - { - continue; - } - results.push(KafkaTopicMetadata { - kafka_topic: topic.name, - partitions_count: topic.partitions_count, - }); + // No separate get_stream probe: get_topics already answers an empty list when the stream + // itself is missing (server-side "legacy parity: a missing stream lists as empty, not + // StreamNotFound"), so a probe first would pay a second round trip to learn something + // this one call already tells us - and it costs an extra ACL check this caller might not + // even have, when a user with only read_topics on the default stream (no read_streams) + // should still see its topics listed. + let topics = with_request_timeout(self.client.get_topics(&default_stream_id)).await?; + for topic in topics { + if default_stream_override_targets.contains(topic.name.as_str()) + || override_keys.contains(topic.name.as_str()) + { + continue; + } + if let Err(reason) = validate_kafka_topic_name("kafka_topic", &topic.name) { + // A raw Iggy topic name that isn't itself a legal Kafka topic name (e.g. contains + // a space) would list under a name no named Metadata/CreateTopics lookup can ever + // resolve back - `validate_kafka_topic_name` is the same gate every named path + // already enforces, so this keeps "listed" and "reachable by name" the same set. + debug!( + "skipping raw Iggy topic '{}' in Metadata: {reason}", + topic.name + ); + continue; } + results.push(KafkaTopicMetadata { + kafka_topic: topic.name, + partitions_count: topic.partitions_count, + }); } Ok(results) diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index ca30230dc0..77a45b9a8e 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -95,6 +95,11 @@ pub const ERROR_INVALID_CONFIG: i16 = 40; /// Non-retriable, so a Java client resolves immediately instead of retrying /// [`ERROR_UNKNOWN_SERVER_ERROR`] until its own `default.api.timeout.ms`. pub const ERROR_UNSUPPORTED_FOR_MESSAGE_FORMAT: i16 = 43; +/// `CreateTopics`: request addressed more distinct topics than this bridge admits in one call. +/// +/// A server-imposed limit, not a malformed request - `INVALID_REQUEST` would blame the client for +/// a request Kafka itself would accept. +pub const ERROR_POLICY_VIOLATION: i16 = 44; /// Result of handling one Kafka request body. #[derive(Debug)] diff --git a/gateways/kafka/src/protocol/handlers/create_topics.rs b/gateways/kafka/src/protocol/handlers/create_topics.rs index 6dd7783c25..a2cd9c8d61 100644 --- a/gateways/kafka/src/protocol/handlers/create_topics.rs +++ b/gateways/kafka/src/protocol/handlers/create_topics.rs @@ -27,13 +27,15 @@ use kafka_protocol::messages::create_topics_response::CreatableTopicResult; use kafka_protocol::messages::{BrokerId, CreateTopicsRequest, CreateTopicsResponse, TopicName}; use kafka_protocol::protocol::StrBytes; +use tokio::time::Instant; + use crate::bridge::{BridgeError, IggyBridge, TopicCreationOutcome}; use crate::error::Result; use crate::protocol::api::{ API_KEY_CREATE_TOPICS, ApiVersionRange, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, ERROR_INVALID_REPLICA_ASSIGNMENT, ERROR_INVALID_REPLICATION_FACTOR, ERROR_INVALID_REQUEST, - ERROR_NONE, ERROR_NOT_CONTROLLER, ERROR_REQUEST_TIMED_OUT, ERROR_TOPIC_ALREADY_EXISTS, - GatewayState, HandleOutcome, + ERROR_NONE, ERROR_NOT_CONTROLLER, ERROR_POLICY_VIOLATION, ERROR_REQUEST_TIMED_OUT, + ERROR_TOPIC_ALREADY_EXISTS, GatewayState, HandleOutcome, }; use crate::protocol::bounds_guard::validate_create_topics_shape; use crate::protocol::handlers::{ @@ -53,6 +55,14 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { /// bridge config - there is no such config surface today. const DEFAULT_PARTITION_COUNT: u32 = 1; +/// Iggy's own server-side per-topic partition cap (`rewrite.rs`'s `IggyError::TooManyPartitions`). +/// +/// Not exported by `iggy::prelude`, so mirrored here as a named constant rather than a bare +/// literal repeated at every call site. Enforcing it locally means `validate_only` answers the +/// same rejection the real path would eventually get from the bridge, instead of reporting +/// `NONE` for a partition count the real path can never actually create. +const MAX_PARTITIONS_COUNT: u32 = 1000; + /// Cap on distinct topic names one `CreateTopics` request may address through the bridge. /// /// `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode `DoS` ceiling, not a usability @@ -131,50 +141,35 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand max = MAX_BRIDGE_BACKED_TOPICS, "CreateTopics request addresses too many distinct topics; rejecting" ); + // A server-imposed limit, not a malformed request - INVALID_REQUEST would blame the + // client for a request Kafka itself would accept. + let message = StrBytes::from(format!( + "this gateway addresses at most {MAX_BRIDGE_BACKED_TOPICS} distinct topics per CreateTopics request" + )); let results = req .topics .iter() .map(|topic| { CreatableTopicResult::default() .with_name(topic.name.clone()) - .with_error_code(ERROR_INVALID_REQUEST) + .with_error_code(ERROR_POLICY_VIOLATION) + .with_error_message(Some(message.clone())) }) .collect(); let resp = CreateTopicsResponse::default().with_topics(results); return respond_or_close(encode_message(&resp, api_version, 256), "CreateTopics"); } - let deadline = clamp_request_timeout(req.timeout_ms); - let results = match tokio::time::timeout( + let deadline = Instant::now() + clamp_request_timeout(req.timeout_ms); + let results = create_all_topics( + bridge, + api_version, + &req.topics, + &duplicate_names, + req.validate_only, deadline, - create_all_topics( - bridge, - api_version, - &req.topics, - &duplicate_names, - req.validate_only, - ), ) - .await - { - Ok(results) => results, - Err(_elapsed) => { - tracing::warn!( - distinct_topics = distinct_bridge_backed.len(), - deadline_ms = deadline.as_millis(), - "CreateTopics request's aggregate bridge work exceeded its deadline; \ - answering retriable instead of blocking further" - ); - req.topics - .iter() - .map(|topic| { - CreatableTopicResult::default() - .with_name(topic.name.clone()) - .with_error_code(ERROR_REQUEST_TIMED_OUT) - }) - .collect() - } - }; + .await; let resp = CreateTopicsResponse::default().with_topics(results); respond_or_close(encode_message(&resp, api_version, 256), "CreateTopics") } @@ -185,12 +180,20 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand /// create it never got a `NONE` for (`AdminClient` keys its futures by name, so a second /// per-topic result for the same name is silently discarded client-side regardless of which one /// this bridge picked). +/// +/// `deadline` bounds each topic's own bridge work individually (`timeout_at`), not the whole +/// loop: a single `timeout` around the entire call would discard every already-resolved result +/// the moment one topic's call ran long, answering `REQUEST_TIMED_OUT` even for topics that had +/// already committed. Once `deadline` passes, every remaining topic's own `timeout_at` elapses +/// immediately rather than making a fresh bridge call, so a stuck topic near the front of a large +/// batch does not turn into one slow round trip per topic behind it. async fn create_all_topics( bridge: &IggyBridge, api_version: i16, topics: &[CreatableTopic], duplicate_names: &HashSet, validate_only: bool, + deadline: Instant, ) -> Vec { let mut results = Vec::with_capacity(topics.len()); for topic in topics { @@ -198,8 +201,27 @@ async fn create_all_topics( CreatableTopicResult::default() .with_name(topic.name.clone()) .with_error_code(ERROR_INVALID_REQUEST) + .with_error_message(None) } else { - create_one_topic(bridge, api_version, topic, validate_only).await + match tokio::time::timeout_at( + deadline, + create_one_topic(bridge, api_version, topic, validate_only), + ) + .await + { + Ok(result) => result, + Err(_elapsed) => { + tracing::warn!( + kafka_topic = topic.name.as_str(), + "CreateTopics: this topic's bridge work exceeded the request deadline; \ + answering retriable instead of blocking further" + ); + CreatableTopicResult::default() + .with_name(topic.name.clone()) + .with_error_code(ERROR_REQUEST_TIMED_OUT) + .with_error_message(None) + } + } }; results.push(result); } @@ -223,10 +245,16 @@ fn find_duplicate_names(topics: &[CreatableTopic]) -> HashSet { /// Validates and, when the topic is not rejected outright, provisions one requested topic. /// -/// `configs` is rejected before the shape check - a config-bearing request is rejected the same -/// way regardless of how its partitions/replication are shaped. +/// Existence beats a local (config/shape) rejection, not the other way around: real Kafka's +/// controller checks its in-memory topic set before it ever looks at the request's configs or +/// shape, so an existing topic answers `TOPIC_ALREADY_EXISTS` regardless of whether the new +/// request would itself be valid. Kafka Connect's idempotent bootstrap depends on this - it +/// resends `cleanup.policy=compact` against topics it doesn't know already exist, and expects +/// `ALREADY_EXISTS` back, not a config rejection. The existence probe only runs when a local +/// check would otherwise reject, so the common (valid-request) path pays no extra round trip for +/// it. /// -/// `validate_only` and the real create path diverge deliberately below that point, not just in +/// `validate_only` and the real create path diverge deliberately past that point, not just in /// whether they call the bridge: `validate_only` never mutates anything, so a plain existence /// read ([`IggyBridge::get_kafka_topic`]) is fine - there's no race to protect against when /// nothing gets created either way. The real path instead calls @@ -240,22 +268,25 @@ async fn create_one_topic( topic: &CreatableTopic, validate_only: bool, ) -> CreatableTopicResult { - let result = CreatableTopicResult::default().with_name(topic.name.clone()); - - if !topic.configs.is_empty() { - return result - .with_error_code(ERROR_INVALID_CONFIG) - .with_error_message(Some(StrBytes::from( - "per-topic configs are not supported by this bridge".to_string(), - ))); - } + let result = CreatableTopicResult::default() + .with_name(topic.name.clone()) + .with_error_message(None); + let kafka_topic = topic.name.as_str(); - let partition_count = match validate_create_topic_shape(version, topic) { + let partition_count = match local_shape_error(version, topic) { Ok(count) => count, - Err(code) => return result.with_error_code(code), + Err((error_code, error_message)) => { + return match bridge.get_kafka_topic(kafka_topic).await { + Ok(Some(_existing)) => result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS), + // A failed existence probe doesn't get to mask a real, independently-valid + // rejection - the request has a local defect either way. + Ok(None) | Err(_) => result + .with_error_code(error_code) + .with_error_message(error_message), + }; + } }; - let kafka_topic = topic.name.as_str(); let success = || { result .clone() @@ -268,9 +299,7 @@ async fn create_one_topic( return match bridge.get_kafka_topic(kafka_topic).await { Ok(Some(_existing)) => result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS), Ok(None) => success(), - Err(err) => result - .with_error_code(err.to_kafka_error_code()) - .with_error_message(Some(StrBytes::from(error_message_for(&err)))), + Err(err) => bridge_error_result(result, kafka_topic, &err), }; } @@ -288,25 +317,75 @@ async fn create_one_topic( Ok(TopicCreationOutcome::AlreadyExists) => { result.with_error_code(ERROR_TOPIC_ALREADY_EXISTS) } - Err(err) => result - .with_error_code(err.to_kafka_error_code()) - .with_error_message(Some(StrBytes::from(error_message_for(&err)))), + Err(err) => bridge_error_result(result, kafka_topic, &err), } } -/// Error text for `CreatableTopicResult.error_message`, without re-embedding the topic name: -/// `result.name` (`CreatableTopicResult::with_name`) already carries it, so `err.to_string()`'s -/// own embedded copy for these two variants would double the per-topic response cost for a name -/// the client already sent and already has back. Validation runs before any bridge I/O, so this -/// is a purely local, zero-round-trip amplification if left in - 100 topics named with a maximal -/// legal length build a response roughly twice the size the name alone would justify. -fn error_message_for(err: &BridgeError) -> String { +/// `configs`-then-shape local validation, bundled so [`create_one_topic`] can probe existence +/// once, only on the rejection path. Returns `(error_code, error_message)` rather than a whole +/// [`CreatableTopicResult`] to keep this `Result`'s `Err` arm small (`clippy::result_large_err`); +/// the caller already holds the shared `name`/base fields to rebuild the full result from. +fn local_shape_error( + version: i16, + topic: &CreatableTopic, +) -> core::result::Result)> { + if !topic.configs.is_empty() { + return Err(( + ERROR_INVALID_CONFIG, + Some(StrBytes::from( + "per-topic configs are not supported by this bridge".to_string(), + )), + )); + } + validate_create_topic_shape(version, topic).map_err(|code| (code, None)) +} + +/// Maps a bridge failure to a Kafka result, logging the real cause server-side. +/// +/// The Kafka client never sees more than the fixed text below: `err.to_string()`'s own embedded +/// detail can be wrong for an error reconstructed from a bare wire status code +/// (`BridgeError::Iggy`'s own doc explains why `IggyError::from_code` fills data fields with +/// defaults on that path), so sending it to the client risks sending a wrong claim rather than no +/// claim. The two client-caused variants are the exception - their text is fixed and always +/// correct, so it's safe to forward and logged at `debug!` (attacker/misuse-controlled, not +/// operator-actionable); everything else points at the bridge or Iggy itself and is logged at +/// `error!` (`bridge/error.rs:155`'s own guidance: handlers log the real Iggy error). +fn bridge_error_result( + result: CreatableTopicResult, + kafka_topic: &str, + err: &BridgeError, +) -> CreatableTopicResult { + let error_code = err.to_kafka_error_code(); match err { - BridgeError::InvalidKafkaTopicName { reason, .. } => reason.clone(), + BridgeError::InvalidKafkaTopicName { reason, .. } => { + tracing::debug!( + kafka_topic, + reason, + "CreateTopics rejected an invalid topic name" + ); + result + .with_error_code(error_code) + .with_error_message(Some(StrBytes::from(reason.clone()))) + } BridgeError::InvalidPartitionCount { .. } => { - "partition count must be at least 1".to_string() + tracing::debug!( + kafka_topic, + "CreateTopics rejected an invalid partition count" + ); + result + .with_error_code(error_code) + .with_error_message(Some(StrBytes::from( + "partition count must be at least 1".to_string(), + ))) + } + other => { + tracing::error!(kafka_topic, %other, "CreateTopics failed against the Iggy bridge"); + result + .with_error_code(error_code) + .with_error_message(Some(StrBytes::from( + "internal error provisioning this topic".to_string(), + ))) } - other => other.to_string(), } } @@ -349,6 +428,17 @@ fn validate_create_topic_shape( let broker_default_ok = version >= 4; + // Checked ahead of (and separately from) `partitions_ok` below: that check's own + // `INVALID_PARTITIONS` (37) means "count is below 1" on the wire (`bridge/error.rs`'s own + // comment on `IggyError::TooManyPartitions`), the opposite condition from "too many" - reusing + // it here would send a client-visible message that contradicts the request it just sent. This + // is the same code the real (non-`validate_only`) path eventually gets back from the bridge + // once a count this large actually reaches `create_kafka_topic`, so `validate_only` now + // answers what the real create would. + if u32::try_from(topic.num_partitions).is_ok_and(|count| count > MAX_PARTITIONS_COUNT) { + return Err(ERROR_INVALID_REQUEST); + } + let partitions_ok = if broker_default_ok { topic.num_partitions == -1 || topic.num_partitions > 0 } else { @@ -358,10 +448,16 @@ fn validate_create_topic_shape( return Err(ERROR_INVALID_PARTITIONS); } + // Exactly 1, not merely positive: this gateway advertises exactly one broker (node id 1, + // `metadata.rs`'s `BrokerId(1)`), the same ceiling the manual-assignment branch above already + // enforces per partition (`assignment_replicas_are_valid`). A real single-broker Kafka + // controller rejects `replication_factor > 1` the same way (`ReplicationControlManager`) - + // accepting it here and silently reporting back `1` (`success()`, below) would tell the + // client its request succeeded as sent when it didn't. let replication_ok = if broker_default_ok { - topic.replication_factor == -1 || topic.replication_factor > 0 + topic.replication_factor == -1 || topic.replication_factor == 1 } else { - topic.replication_factor > 0 + topic.replication_factor == 1 }; if !replication_ok { return Err(ERROR_INVALID_REPLICATION_FACTOR); @@ -589,6 +685,47 @@ mod tests { ); } + #[test] + fn replication_factor_above_one_is_rejected_even_though_positive() { + // This gateway advertises exactly one broker - the manual-assignment branch already + // rejects a multi-replica assignment (`manual_assignment_with_a_duplicated_replica_is_rejected`); + // the equivalent numeric-field request must be rejected the same way, not merely + // accepted-and-silently-downgraded. + let topic = creatable_topic(1, 3); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REPLICATION_FACTOR) + ); + } + + #[test] + fn replication_factor_of_exactly_one_is_accepted() { + let topic = creatable_topic(1, 1); + assert_eq!(validate_create_topic_shape(5, &topic), Ok(1)); + } + + #[test] + fn partition_count_at_the_cap_is_accepted() { + let topic = creatable_topic(i32::try_from(MAX_PARTITIONS_COUNT).unwrap(), 1); + assert_eq!( + validate_create_topic_shape(5, &topic), + Ok(MAX_PARTITIONS_COUNT) + ); + } + + #[test] + fn partition_count_above_the_cap_is_rejected_with_the_real_creates_own_code() { + // Not INVALID_PARTITIONS (37): that code's wire text is "below 1", the opposite + // condition. This is the same code the real (non-validate_only) path eventually gets + // back from the bridge once IggyError::TooManyPartitions reaches it + // (`bridge/error.rs::too_many_partitions_maps_to_invalid_request_not_invalid_partitions`). + let topic = creatable_topic(i32::try_from(MAX_PARTITIONS_COUNT).unwrap() + 1, 1); + assert_eq!( + validate_create_topic_shape(5, &topic), + Err(ERROR_INVALID_REQUEST) + ); + } + #[test] fn explicit_num_partitions_with_assignments_is_rejected_even_when_it_agrees_with_their_length() { diff --git a/gateways/kafka/src/protocol/handlers/metadata.rs b/gateways/kafka/src/protocol/handlers/metadata.rs index b3451646a2..8e1cd5dda7 100644 --- a/gateways/kafka/src/protocol/handlers/metadata.rs +++ b/gateways/kafka/src/protocol/handlers/metadata.rs @@ -25,14 +25,14 @@ use kafka_protocol::messages::metadata_response::{ MetadataResponseBroker, MetadataResponsePartition, MetadataResponseTopic, }; use kafka_protocol::messages::{BrokerId, MetadataRequest, MetadataResponse, TopicName}; -use kafka_protocol::protocol::StrBytes; +use kafka_protocol::protocol::{Encodable, StrBytes}; use crate::bridge::IggyBridge; use crate::error::{KafkaProtocolError, Result}; use crate::protocol::api::{ - API_KEY_METADATA, ApiVersionRange, BrokerAdvertise, ERROR_INVALID_REQUEST, ERROR_NONE, - ERROR_REQUEST_TIMED_OUT, ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, HandleOutcome, - is_supported_version, supported_max_version, + API_KEY_METADATA, ApiVersionRange, BrokerAdvertise, ERROR_NONE, ERROR_REQUEST_TIMED_OUT, + ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, HandleOutcome, is_supported_version, + supported_max_version, }; use crate::protocol::bounds_guard::validate_metadata_shape; use crate::protocol::handlers::{decode_guarded, encode_message, respond_or_close}; @@ -43,24 +43,21 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { max_version: 9, }; -/// Cap on distinct topic names one named-lookup `Metadata` request may address through the -/// bridge. Does not apply to a null topics array ("all topics") - that path is server-driven -/// (bounded by [`response_would_exceed_frame_size`] instead), not client-count-driven. -/// -/// `bounds_guard`'s `MAX_REQUEST_ELEMENTS` (4,096) is a pre-decode `DoS` ceiling, not a usability -/// recommendation: each distinct name costs one `get_kafka_topic` round trip against the single -/// lockstep `IggyClient` every Kafka connection on this gateway shares -/// (`bridge/iggy_bridge/mod.rs`'s "Concurrency ceiling"). -const MAX_BRIDGE_BACKED_TOPICS: usize = 100; - /// Wall-clock ceiling for a named-lookup request's aggregate bridge work. /// /// `Metadata` carries no `timeout_ms` field in any version (unlike `CreateTopics`), so this is a -/// fixed ceiling, not a client-honored one - sized well above one `get_kafka_topic` call's own -/// `REQUEST_TIMEOUT` (15s, bridge-internal) so a single slow-but-alive call is not the common -/// trigger, while still bounding the sum across up to [`MAX_BRIDGE_BACKED_TOPICS`] calls. +/// fixed ceiling, not a client-honored one - sized well above one `get_topics` call's own +/// `REQUEST_TIMEOUT` (15s, bridge-internal) so a single slow-but-alive stream lookup is not the +/// common trigger, while still bounding the sum across every distinct stream +/// [`IggyBridge::get_kafka_topics`] ends up calling for this request. const REQUEST_DEADLINE: Duration = Duration::from_secs(20); +/// Above this many total partitions in an all-topics response, `encode_real_response` runs on a +/// blocking-pool thread instead of inline (see the call site). Conservative relative to the one +/// measured data point available (20-54ms at 131,000 partitions) rather than a profiled +/// threshold - tune with a real benchmark if it proves too high or too low in practice. +const BLOCKING_ENCODE_PARTITION_THRESHOLD: u32 = 20_000; + pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> HandleOutcome { if !is_supported_version(API_KEY_METADATA, api_version) { // Clamping the response to the supported max leaves a body the client parses at its own @@ -106,19 +103,36 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand } }; + let per_partition_bytes = template_partition_bytes(api_version); + let results = match requested { - None => match bridge.list_kafka_topics().await { - Ok(topics) => { + None => match tokio::time::timeout(REQUEST_DEADLINE, bridge.list_kafka_topics()).await { + Ok(Ok(topics)) => { let results: Vec = topics.into_iter().map(found_result).collect(); - truncate_all_topics_to_frame_budget(results, state.max_frame_size) + truncate_all_topics_to_frame_budget( + results, + state.max_frame_size, + per_partition_bytes, + ) } - Err(error) => { + Ok(Err(error)) => { // Same "no top-level error field" constraint as a decode failure: there is no // way to answer "the bridge itself is unreachable" for an all-topics request // that doesn't also falsely claim zero topics exist. tracing::warn!(%error, "Failed to list Kafka topics from the Iggy bridge; closing connection"); return HandleOutcome::Close; } + Err(_elapsed) => { + // No per-name cap to answer against here (this is the server-driven all-topics + // path), so there is no honest per-topic RETRIABLE list to send back either - + // same close as an unreachable bridge, not a different case. + tracing::warn!( + deadline_secs = REQUEST_DEADLINE.as_secs(), + "Listing Kafka topics from the Iggy bridge exceeded its deadline; closing \ + connection" + ); + return HandleOutcome::Close; + } }, Some(names) => resolve_requested_named_topics(bridge, &names).await, }; @@ -132,7 +146,10 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand // client's bootstrap Metadata call, permanently, the moment the catalog grows past the // trip point. This check is what still enforces the budget for the named-lookup arm, where // the cap (100 distinct topics) bounds the request but not what each one costs to answer. - let total_bytes: usize = results.iter().map(estimated_response_bytes).sum(); + let total_bytes: usize = results + .iter() + .map(|result| estimated_response_bytes(result, per_partition_bytes)) + .sum(); if response_would_exceed_frame_size(total_bytes, state.max_frame_size) { tracing::warn!( total_bytes, @@ -142,36 +159,72 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand return HandleOutcome::Close; } - respond_or_close( - encode_real_response(api_version, &results, &state.broker), - "Metadata", - ) + let total_partitions: u64 = results + .iter() + .filter(|result| result.error_code == ERROR_NONE) + .map(|result| u64::from(result.partitions_count)) + .sum(); + + let encoded = if total_partitions > u64::from(BLOCKING_ENCODE_PARTITION_THRESHOLD) { + // Building `MetadataResponsePartition` once per partition and serializing the whole + // response is measured at 20-54ms at 131,000 partitions - long enough, on a + // shared-nothing shard's single executor, to stall every other connection's request on + // this shard for the duration. `spawn_blocking` moves that cost off the shard's own task + // so ordinary requests keep their latency regardless of how large one all-topics response + // gets. + let broker = state.broker.clone(); + tokio::task::spawn_blocking(move || encode_real_response(api_version, &results, &broker)) + .await + .unwrap_or_else(|join_error| { + tracing::error!(%join_error, "Metadata all-topics encode task panicked"); + Err(KafkaProtocolError::Malformed(join_error.to_string())) + }) + } else { + encode_real_response(api_version, &results, &state.broker) + }; + respond_or_close(encoded, "Metadata") } -/// Resolves a named-lookup Metadata request's topics: dedupes first so every path below (cap, -/// deadline, success) builds its response from the distinct set rather than one entry per -/// request occurrence - real Kafka answers a topic named more than once with one response entry, -/// not one per repeat, and re-expanding to match the request let a handful of repeats of one -/// large topic name amplify a response sized off the repeat count instead of the distinct count. +/// Resolves a named-lookup Metadata request's topics: dedupes first so the response is built from +/// the distinct set rather than one entry per request occurrence - real Kafka answers a topic +/// named more than once with one response entry, not one per repeat, and re-expanding to match +/// the request would let a handful of repeats of one large topic name amplify a response sized +/// off the repeat count instead of the distinct count. +/// +/// No cap on how many distinct names one request may carry (`bounds_guard`'s +/// `MAX_REQUEST_ELEMENTS`, 4,096, is still the decode-time ceiling): the bridge round trip cost +/// this used to bound was one call per name, but [`IggyBridge::get_kafka_topics`] batches by the +/// distinct *streams* those names resolve to instead, which is a config-time-bounded quantity a +/// large name count doesn't grow. A Java producer's `ProducerMetadata` resends every topic it has +/// ever produced to on each refresh, not just the ones in the current batch - once that set +/// crossed a per-request name cap, every subsequent Metadata call answered every topic +/// `INVALID_REQUEST` and the producer could never recover, since it has no way to shrink its own +/// tracked set. A single timeout still bounds the one batched call. async fn resolve_requested_named_topics( bridge: &IggyBridge, names: &[StrBytes], ) -> Vec { let distinct = dedup_topic_names(names); - if distinct.len() > MAX_BRIDGE_BACKED_TOPICS { - tracing::warn!( - distinct_topics = distinct.len(), - max = MAX_BRIDGE_BACKED_TOPICS, - "Metadata request addresses too many distinct topics; rejecting" - ); - return distinct - .iter() - .map(|name| error_result(name.clone(), ERROR_INVALID_REQUEST)) - .collect(); - } - - match tokio::time::timeout(REQUEST_DEADLINE, resolve_named_topics(bridge, &distinct)).await { - Ok(results) => results, + match tokio::time::timeout(REQUEST_DEADLINE, bridge.get_kafka_topics(&distinct)).await { + Ok(Ok(resolved)) => resolved + .into_iter() + .map(|(name, metadata)| match metadata { + Some(metadata) => TopicResult { + name, + error_code: ERROR_NONE, + partitions_count: metadata.partitions_count, + }, + None => error_result(name, ERROR_UNKNOWN_TOPIC_OR_PARTITION), + }) + .collect(), + Ok(Err(error)) => { + tracing::error!(%error, "Metadata: resolving requested topics failed against the Iggy bridge"); + let error_code = error.to_kafka_error_code(); + distinct + .into_iter() + .map(|name| error_result(name, error_code)) + .collect() + } Err(_elapsed) => { tracing::warn!( distinct_topics = distinct.len(), @@ -180,20 +233,38 @@ async fn resolve_requested_named_topics( retriable instead of blocking further" ); distinct - .iter() - .map(|name| error_result(name.clone(), ERROR_REQUEST_TIMED_OUT)) + .into_iter() + .map(|name| error_result(name, ERROR_REQUEST_TIMED_OUT)) .collect() } } } -/// Conservative per-partition byte cost of one encoded `MetadataResponsePartition` at v9 (the -/// densest wire shape this handler emits): measured ~26 bytes (`error_code` + `partition_index` + -/// `leader_id` + `leader_epoch` + a 1-entry `replica_nodes` + a 1-entry `isr_nodes` + an empty -/// `offline_replicas` + tagged fields). 64 matches the margin `bounds_guard`'s own -/// `RESPONSE_BYTES_PER_ELEMENT` uses for the same kind of estimate, rather than shaving this to -/// the measured minimum. -const RESPONSE_BYTES_PER_PARTITION: usize = 64; +/// Fallback per-partition byte cost, used only if [`template_partition_bytes`]'s own +/// `compute_size` call somehow fails for the fixed, always-well-formed shape it builds. Chosen +/// well above every version's real cost (measured 26-34 bytes) so a fallback can only truncate +/// more eagerly than the real encoder would, never less. +const RESPONSE_BYTES_PER_PARTITION_FALLBACK: usize = 64; + +/// Exact per-partition byte cost of one encoded `MetadataResponsePartition` at `version`, shaped +/// identically to what [`encode_real_response`] actually emits for a successful topic: one +/// `replica_nodes` entry, one `isr_nodes` entry, an empty `offline_replicas`. +/// +/// A flat constant here previously overcharged every version below v9 (the densest wire shape, +/// which a flat 64-byte charge was measured against) - v0-v8's smaller encodings (fewer or +/// narrower fixed fields, no tagged-field byte) cost as little as 26 bytes, so the flat charge +/// started truncating an all-topics response at 41-53% of `max_frame_size` instead of near its +/// actual limit. `compute_size` asks the encoder itself, so this tracks every version exactly +/// instead of re-deriving and re-verifying a magic number per version by hand. +fn template_partition_bytes(version: i16) -> usize { + MetadataResponsePartition::default() + .with_partition_index(0) + .with_leader_id(BrokerId(1)) + .with_replica_nodes(vec![BrokerId(1)]) + .with_isr_nodes(vec![BrokerId(1)]) + .compute_size(version) + .unwrap_or(RESPONSE_BYTES_PER_PARTITION_FALLBACK) +} /// Fixed per-`MetadataResponseTopic` cost, independent of name length or partition count: /// `error_code`(2) + `is_internal`(1) + the `partitions` array's own compact-length varint @@ -214,10 +285,10 @@ const RESPONSE_BYTES_TOPIC_OVERHEAD: usize = 13; /// zero-partition or errored result - free under a partitions-only charge - still costs a full /// wrapper to encode. 500 topics with 255-byte names and one partition each encode to ~146 KB /// against the ~32 KB a partitions-only charge would have priced in. -fn estimated_response_bytes(result: &TopicResult) -> usize { +fn estimated_response_bytes(result: &TopicResult, per_partition_bytes: usize) -> usize { let name_bytes = result.name.as_str().len() + 2; let partition_bytes = if result.error_code == ERROR_NONE { - (result.partitions_count as usize).saturating_mul(RESPONSE_BYTES_PER_PARTITION) + (result.partitions_count as usize).saturating_mul(per_partition_bytes) } else { 0 }; @@ -243,11 +314,12 @@ const fn response_would_exceed_frame_size(total_bytes: usize, max_frame_size: us fn truncate_all_topics_to_frame_budget( mut results: Vec, max_frame_size: usize, + per_partition_bytes: usize, ) -> Vec { let mut cumulative_bytes = 0usize; let mut keep = results.len(); for (index, result) in results.iter().enumerate() { - let next = cumulative_bytes + estimated_response_bytes(result); + let next = cumulative_bytes + estimated_response_bytes(result, per_partition_bytes); if response_would_exceed_frame_size(next, max_frame_size) { keep = index; break; @@ -283,29 +355,6 @@ fn found_result(metadata: crate::bridge::KafkaTopicMetadata) -> TopicResult { } } -async fn lookup_one_topic(bridge: &IggyBridge, name: StrBytes) -> TopicResult { - match bridge.get_kafka_topic(&name).await { - // `get_kafka_topic` (unlike `list_kafka_topics`) returns the SDK's own `TopicDetails`, - // which carries the topic's raw Iggy-side name, not the Kafka-side one under an override - // - so this echoes the caller's own `name`, not a field off the result. - Ok(Some(details)) => TopicResult { - name, - error_code: ERROR_NONE, - partitions_count: details.partitions_count, - }, - Ok(None) => TopicResult { - name, - error_code: ERROR_UNKNOWN_TOPIC_OR_PARTITION, - partitions_count: 0, - }, - Err(err) => TopicResult { - name, - error_code: err.to_kafka_error_code(), - partitions_count: 0, - }, - } -} - const fn error_result(name: StrBytes, error_code: i16) -> TopicResult { TopicResult { name, @@ -327,19 +376,6 @@ fn dedup_topic_names(names: &[StrBytes]) -> Vec { distinct } -/// Resolves each of `names`, one `get_kafka_topic` round trip per entry. -/// -/// `names` must already be the distinct set ([`dedup_topic_names`]) - this makes no attempt to -/// re-derive or re-expand it, so a caller passing a list with repeats gets one round trip and one -/// result per repeat, silently paying for the amplification this split was written to avoid. -async fn resolve_named_topics(bridge: &IggyBridge, names: &[StrBytes]) -> Vec { - let mut results = Vec::with_capacity(names.len()); - for name in names { - results.push(lookup_one_topic(bridge, name.clone()).await); - } - results -} - /// # Errors /// /// Returns an error when `kafka_protocol` cannot encode the response at `response_version`. @@ -574,6 +610,8 @@ mod tests { } } + const TEST_PER_PARTITION_BYTES: usize = RESPONSE_BYTES_PER_PARTITION_FALLBACK; + #[test] fn estimated_response_bytes_charges_name_and_overhead_even_at_zero_partitions() { // Regression: a partitions-only charge (the earlier form of this function) priced a @@ -581,7 +619,7 @@ mod tests { // `MetadataResponseTopic` wrapper with the name in it. let result = topic_result("orders", ERROR_NONE, 0); assert_eq!( - estimated_response_bytes(&result), + estimated_response_bytes(&result, TEST_PER_PARTITION_BYTES), RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 ); } @@ -594,7 +632,7 @@ mod tests { // regardless of its error code. let result = topic_result("orders", ERROR_UNKNOWN_TOPIC_OR_PARTITION, 0); assert_eq!( - estimated_response_bytes(&result), + estimated_response_bytes(&result, TEST_PER_PARTITION_BYTES), RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 ); } @@ -607,16 +645,39 @@ mod tests { let ok = topic_result("orders", ERROR_NONE, 10); let err = topic_result("orders", ERROR_UNKNOWN_TOPIC_OR_PARTITION, 10); assert_eq!( - estimated_response_bytes(&err), + estimated_response_bytes(&err, TEST_PER_PARTITION_BYTES), RESPONSE_BYTES_TOPIC_OVERHEAD + "orders".len() + 2 ); - assert!(estimated_response_bytes(&ok) > estimated_response_bytes(&err)); + assert!( + estimated_response_bytes(&ok, TEST_PER_PARTITION_BYTES) + > estimated_response_bytes(&err, TEST_PER_PARTITION_BYTES) + ); } #[test] fn estimated_response_bytes_charges_longer_names_more() { let short = topic_result("a", ERROR_NONE, 0); let long = topic_result("a-much-longer-topic-name", ERROR_NONE, 0); - assert!(estimated_response_bytes(&long) > estimated_response_bytes(&short)); + assert!( + estimated_response_bytes(&long, TEST_PER_PARTITION_BYTES) + > estimated_response_bytes(&short, TEST_PER_PARTITION_BYTES) + ); + } + + #[test] + fn template_partition_bytes_is_smaller_at_v0_than_the_conservative_fallback() { + // v0 has no leader_epoch, no offline_replicas tagged-field byte, and a legacy (non-compact) + // array encoding - real cost is well under the flat 64-byte charge this replaced. + assert!(template_partition_bytes(0) < RESPONSE_BYTES_PER_PARTITION_FALLBACK); + } + + #[test] + fn template_partition_bytes_is_never_larger_than_the_fallback() { + for version in RANGE.min_version..=RANGE.max_version { + assert!( + template_partition_bytes(version) <= RESPONSE_BYTES_PER_PARTITION_FALLBACK, + "v{version} exceeded the conservative fallback" + ); + } } } diff --git a/gateways/kafka/tests/create_topics_real_bridge_tests.rs b/gateways/kafka/tests/create_topics_real_bridge_tests.rs index ad94710575..8fb7e038fb 100644 --- a/gateways/kafka/tests/create_topics_real_bridge_tests.rs +++ b/gateways/kafka/tests/create_topics_real_bridge_tests.rs @@ -38,7 +38,7 @@ use iggy_gateway_kafka::bridge::IggyBridge; use iggy_gateway_kafka::protocol::api::{ BrokerAdvertise, ERROR_INVALID_CONFIG, ERROR_INVALID_PARTITIONS, ERROR_INVALID_REPLICA_ASSIGNMENT, ERROR_INVALID_REQUEST, ERROR_INVALID_TOPIC_EXCEPTION, - ERROR_NONE, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, + ERROR_NONE, ERROR_POLICY_VIOLATION, ERROR_TOPIC_ALREADY_EXISTS, GatewayState, }; use iggy_gateway_kafka::protocol::handlers::create_topics; @@ -302,6 +302,30 @@ async fn create_topics_with_a_per_topic_config_returns_invalid_config_and_create assert!(streams.is_empty()); } +/// Kafka Connect's idempotent bootstrap sends `createTopics` with `cleanup.policy=compact` +/// against topics it doesn't know already exist, expecting `TOPIC_ALREADY_EXISTS` for ones that +/// are - real Kafka's controller checks existence before it looks at configs. This bridge doesn't +/// support per-topic configs at all, so if it checked configs first, Connect's already-existing +/// topics would always answer `INVALID_CONFIG` instead, which Connect does not treat as +/// "already there, fine" the way it treats `TOPIC_ALREADY_EXISTS`. +#[tokio::test] +#[serial] +async fn create_topics_with_a_config_against_an_existing_topic_answers_already_exists() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = connected_state(&server).await; + + let (first_error, _) = send(&state, &[TopicSpec::new("orders", 3)], false).await; + assert_eq!(first_error, ERROR_NONE); + + let topic = TopicSpec { + has_config: true, + ..TopicSpec::new("orders", 3) + }; + let (error_code, _) = send(&state, &[topic], false).await; + assert_eq!(error_code, ERROR_TOPIC_ALREADY_EXISTS); +} + /// A manual replica assignment resolves the created partition count from its own length, not /// from `num_partitions` (`-1` here, per KIP-464). #[tokio::test] @@ -498,7 +522,8 @@ async fn create_topics_rejects_more_than_the_topic_cap() { let results = send_all(&state, &topics, false).await; assert_eq!(results.len(), 101); for (_, error_code) in &results { - assert_eq!(*error_code, ERROR_INVALID_REQUEST); + // A server-imposed limit, not a malformed request. + assert_eq!(*error_code, ERROR_POLICY_VIOLATION); } let raw = raw_client(&server).await; diff --git a/gateways/kafka/tests/metadata_real_bridge_tests.rs b/gateways/kafka/tests/metadata_real_bridge_tests.rs index b5e97abbd7..2bd3e261e6 100644 --- a/gateways/kafka/tests/metadata_real_bridge_tests.rs +++ b/gateways/kafka/tests/metadata_real_bridge_tests.rs @@ -29,8 +29,7 @@ use serial_test::serial; use iggy_gateway_kafka::bridge::{IggyBridge, TopicMapping, TopicOverride}; use iggy_gateway_kafka::protocol::api::{ - BrokerAdvertise, ERROR_INVALID_REQUEST, ERROR_NONE, ERROR_UNKNOWN_TOPIC_OR_PARTITION, - GatewayState, + BrokerAdvertise, ERROR_NONE, ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, }; use iggy_gateway_kafka::protocol::handlers::metadata; @@ -379,11 +378,17 @@ async fn a_topic_named_twice_in_one_request_resolves_to_one_entry() { ); } -/// Regression test: a named-lookup request addressing more than the bridge-backed topic cap must -/// be rejected wholesale (every name, `INVALID_REQUEST`) rather than partially served. +/// Regression test: a named-lookup request addressing more than the old per-name bridge-backed +/// topic cap (100) must resolve every name individually, not be rejected wholesale. A hard cap +/// here permanently broke a long-lived Java producer once its `ProducerMetadata`'s cumulative +/// tracked-topic set (resent in full on every refresh) crossed it - every later request answered +/// every topic `INVALID_REQUEST`, with no way for the producer to shrink its own tracked set and +/// recover. `IggyBridge::get_kafka_topics` batches by resolved stream instead of by name, so a +/// name count this large costs the same one-`get_topics`-call-per-stream (here: one, the default +/// stream) it would for any smaller batch. #[tokio::test] #[serial] -async fn a_named_lookup_of_more_than_the_topic_cap_is_rejected() { +async fn a_named_lookup_of_more_than_the_old_topic_cap_resolves_every_name() { let data_dir = tempfile::tempdir().expect("tempdir"); let server = TestServer::spawn(data_dir.path()).await; let (state, _seed) = connected_state(&server).await; @@ -393,7 +398,9 @@ async fn a_named_lookup_of_more_than_the_topic_cap_is_rejected() { let topics = send(&state, Some(&name_refs)).await; assert_eq!(topics.len(), 101); + // None of these topics exist - each is resolved individually and unknown, not blanket + // rejected as a batch. for (_, error_code, _) in &topics { - assert_eq!(*error_code, ERROR_INVALID_REQUEST); + assert_eq!(*error_code, ERROR_UNKNOWN_TOPIC_OR_PARTITION); } } From 78c57e04dfd3c87e51115d6dac84b02077d3f7f1 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Thu, 24 Sep 2026 08:32:29 -0400 Subject: [PATCH 06/10] fixing the doc test --- gateways/kafka/src/bridge/iggy_bridge/topics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index 945ebcc0fb..ae6e45261d 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -82,7 +82,7 @@ impl IggyBridge { /// default should be for this bridge is a decision for whichever of `#3535`/`#3536` first /// calls this with a real Kafka request in hand, not one to invent here ahead of that need. /// - /// No caching: every call pays a stream-create attempt ([`Self::ensure_stream`]) and a + /// No caching: every call pays a stream-create attempt ([Self::ensure_stream]) and a /// `get_topic` (two round trips once both already exist), even for a topic this same bridge /// already confirmed a moment ago. A cache keyed on `kafka_topic` would remove that cost, but /// would also have to answer "how does a cache entry ever get invalidated" - the topic being From 0ce3343dfe1437fb8c7fa9eaae7202c93d80740c Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Thu, 24 Sep 2026 08:54:45 -0400 Subject: [PATCH 07/10] fixing doc test and clippy errors --- gateways/kafka/src/bridge/iggy_bridge/topics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index ae6e45261d..083ed30204 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -82,7 +82,7 @@ impl IggyBridge { /// default should be for this bridge is a decision for whichever of `#3535`/`#3536` first /// calls this with a real Kafka request in hand, not one to invent here ahead of that need. /// - /// No caching: every call pays a stream-create attempt ([Self::ensure_stream]) and a + /// No caching: every call pays a stream-create attempt (`Self::ensure_stream`) and a /// `get_topic` (two round trips once both already exist), even for a topic this same bridge /// already confirmed a moment ago. A cache keyed on `kafka_topic` would remove that cost, but /// would also have to answer "how does a cache entry ever get invalidated" - the topic being From 63a6b571033d42394d471b5b527ec8d620b1ec0c Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Fri, 25 Sep 2026 22:05:18 -0400 Subject: [PATCH 08/10] Fixed clippy errors from the merge issues --- gateways/kafka/tests/create_topics_real_bridge_tests.rs | 1 + gateways/kafka/tests/metadata_real_bridge_tests.rs | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/gateways/kafka/tests/create_topics_real_bridge_tests.rs b/gateways/kafka/tests/create_topics_real_bridge_tests.rs index 8fb7e038fb..9341c6fb0e 100644 --- a/gateways/kafka/tests/create_topics_real_bridge_tests.rs +++ b/gateways/kafka/tests/create_topics_real_bridge_tests.rs @@ -171,6 +171,7 @@ async fn connected_state(server: &TestServer) -> GatewayState { BrokerAdvertise::default(), Some(Arc::new(bridge)), TEST_MAX_FRAME_SIZE, + false, ) } diff --git a/gateways/kafka/tests/metadata_real_bridge_tests.rs b/gateways/kafka/tests/metadata_real_bridge_tests.rs index 2bd3e261e6..ef81a6c09e 100644 --- a/gateways/kafka/tests/metadata_real_bridge_tests.rs +++ b/gateways/kafka/tests/metadata_real_bridge_tests.rs @@ -159,6 +159,7 @@ async fn connected_state(server: &TestServer) -> (GatewayState, IggyBridge) { BrokerAdvertise::default(), Some(Arc::new(bridge)), TEST_MAX_FRAME_SIZE, + false, ); (state, seed) } @@ -278,6 +279,7 @@ async fn a_named_lookup_reports_the_kafka_side_name_through_a_topic_mapping_over BrokerAdvertise::default(), Some(Arc::new(bridge)), TEST_MAX_FRAME_SIZE, + false, ); let topics = send(&state, Some(&["orders"])).await; @@ -309,7 +311,7 @@ async fn a_response_projected_over_max_frame_size_closes_instead_of_answering() // 50 partitions * 64 bytes/partition (this crate's own conservative per-partition estimate) // = 3200 bytes, comfortably over a 512-byte max_frame_size. - let tiny_state = GatewayState::new(state.broker, state.bridge, 512); + let tiny_state = GatewayState::new(state.broker, state.bridge, 512, false); let body = build_request(Some(&["orders"])); let outcome = metadata::handle(&tiny_state, REQUEST_VERSION, body).await; assert!(outcome.is_close(), "expected Close, got {outcome:?}"); @@ -335,7 +337,7 @@ async fn an_all_topics_response_over_max_frame_size_truncates_instead_of_closing // 50 partitions * 64 bytes/partition (this crate's own conservative per-partition estimate) // = 3200 bytes, comfortably over a 512-byte max_frame_size - so the catalog as a whole cannot // fit, but neither topic's own partition count is malformed or attacker-shaped. - let tiny_state = GatewayState::new(state.broker, state.bridge, 512); + let tiny_state = GatewayState::new(state.broker, state.bridge, 512, false); let topics = send(&tiny_state, None).await; assert!( topics.len() < 2, From 912cc34d893cb411335069bbaac5e475b6f70ae1 Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sun, 27 Sep 2026 07:55:05 -0400 Subject: [PATCH 09/10] Resolving merge conflicts --- gateways/kafka/src/bridge/error.rs | 21 +++++++++++++++++++++ gateways/kafka/src/protocol/api.rs | 3 ++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/gateways/kafka/src/bridge/error.rs b/gateways/kafka/src/bridge/error.rs index ac514c0d12..3edfe47c3b 100644 --- a/gateways/kafka/src/bridge/error.rs +++ b/gateways/kafka/src/bridge/error.rs @@ -434,4 +434,25 @@ mod tests { ); } } + + #[test] + fn send_lost_maps_to_request_timed_out() { + // The SDK does not replay a send after a lost connection, so it may have landed. + let err = BridgeError::SendLost(IggyError::Disconnected); + assert_eq!(err.to_kafka_error_code(), ERROR_REQUEST_TIMED_OUT); + } + + #[test] + fn rejected_bridge_login_is_told_apart_from_a_missing_permission() { + for rejected in [ + IggyError::InvalidCredentials, + IggyError::InvalidUsername, + IggyError::InvalidPassword, + ] { + assert!(BridgeError::Iggy(rejected).is_bridge_login_rejected()); + } + assert!(!BridgeError::Iggy(IggyError::Unauthorized).is_bridge_login_rejected()); + assert!(!BridgeError::Timeout.is_bridge_login_rejected()); + } + } diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index 48e4ef09dc..c2ac87516c 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -113,7 +113,8 @@ pub const ERROR_INVALID_CONFIG: i16 = 40; /// other than the two KIP-79 sentinels, since Iggy has no per-message timestamp index at all. /// Non-retriable, so a Java client resolves immediately instead of retrying /// [`ERROR_UNKNOWN_SERVER_ERROR`] until its own `default.api.timeout.ms`. -pub const ERROR_UNSUPPORTED_FOR_MESSAGE_FORMAT: i16 = 43; +pub const ERROR_UNSUPPORTED_FOR_MESSAGE_FORMAT: i16 = + ResponseError::UnsupportedForMessageFormat.code(); /// `CreateTopics`: request addressed more distinct topics than this bridge admits in one call. /// /// A server-imposed limit, not a malformed request - `INVALID_REQUEST` would blame the client for From 0a0f871a83f59b6ecad287d639295ad9d16a344c Mon Sep 17 00:00:00 2001 From: ryerraguntla Date: Sun, 27 Sep 2026 08:26:58 -0400 Subject: [PATCH 10/10] Fixing merge issues --- gateways/kafka/src/bridge/error.rs | 7 ++----- gateways/kafka/src/bridge/mod.rs | 3 +-- gateways/kafka/src/protocol/api.rs | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/gateways/kafka/src/bridge/error.rs b/gateways/kafka/src/bridge/error.rs index 82233369ec..d9560c1f9a 100644 --- a/gateways/kafka/src/bridge/error.rs +++ b/gateways/kafka/src/bridge/error.rs @@ -446,13 +446,11 @@ mod tests { (ERROR_INVALID_PARTITIONS, ResponseError::InvalidPartitions), (ERROR_INVALID_REQUEST, ResponseError::InvalidRequest), ] { - assert!(BridgeError::Iggy(rejected).is_bridge_login_rejected()); + assert_eq!(ours, theirs.code()); } - assert!(!BridgeError::Iggy(IggyError::Unauthorized).is_bridge_login_rejected()); - assert!(!BridgeError::Timeout.is_bridge_login_rejected()); } - #[test] + #[test] fn send_lost_maps_to_request_timed_out() { // The SDK does not replay a send after a lost connection, so it may have landed. let err = BridgeError::SendLost(IggyError::Disconnected); @@ -471,5 +469,4 @@ mod tests { assert!(!BridgeError::Iggy(IggyError::Unauthorized).is_bridge_login_rejected()); assert!(!BridgeError::Timeout.is_bridge_login_rejected()); } - } diff --git a/gateways/kafka/src/bridge/mod.rs b/gateways/kafka/src/bridge/mod.rs index a64e068709..fc31934fbd 100644 --- a/gateways/kafka/src/bridge/mod.rs +++ b/gateways/kafka/src/bridge/mod.rs @@ -28,6 +28,5 @@ pub mod topic_map; pub use config::{DEFAULT_MAX_MESSAGE_SIZE, IggyBridgeConfig}; pub use error::BridgeError; -pub use iggy_bridge::{IggyBridge, KafkaTopicMetadata, TopicCreationOutcome}; -pub use iggy_bridge::{IggyBridge, TopicTarget}; +pub use iggy_bridge::{IggyBridge, KafkaTopicMetadata, TopicCreationOutcome, TopicTarget}; pub use topic_map::{TopicMapping, TopicOverride}; diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index bd960d8171..b92274f929 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -113,7 +113,7 @@ pub const ERROR_INVALID_CONFIG: i16 = 40; /// other than the two KIP-79 sentinels, since Iggy has no per-message timestamp index at all. /// Non-retriable, so a Java client resolves immediately instead of retrying /// [`ERROR_UNKNOWN_SERVER_ERROR`] until its own `default.api.timeout.ms`. -pub const ERROR_UNSUPPORTED_FOR_MESSAGE_FORMAT: i16 = +pub const ERROR_UNSUPPORTED_FOR_MESSAGE_FORMAT: i16 = ResponseError::UnsupportedForMessageFormat.code(); /// `CreateTopics`: request addressed more distinct topics than this bridge admits in one call. ///