diff --git a/.config/nextest.toml b/.config/nextest.toml index c18175ac4f..c8428a4e49 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -39,7 +39,7 @@ max-threads = 1 filter = 'package(integration) and test(/connectors::elasticsearch::/)' test-group = "elasticsearch" -# Each test in this binary spawns its own real iggy-server subprocess (isolated data dir, port +# Each test in these binaries spawns its own real iggy-server subprocess (isolated data dir, port # from PortGuard's own flock, so no two ever share a port). #[serial] on them is a dead letter # under nextest - nextest runs every test as its own process, and #[serial]'s mutex only excludes # within one process (nextest does not support #[serial] macro, same gap the workaround above @@ -55,8 +55,14 @@ test-group = "elasticsearch" [test-groups.kafka_bridge] max-threads = 4 +# Every gateway suite that spawns a server belongs here. A new one left out does not fail, it +# just runs unbounded alongside these, which is the failure nobody notices until CI is slow. [[profile.default.overrides]] -filter = 'binary_id(iggy-gateway-kafka::bridge_iggy_integration_tests) or binary_id(iggy-gateway-kafka::list_offsets_real_bridge_tests)' +filter = ''' + binary_id(iggy-gateway-kafka::bridge_iggy_integration_tests) + + binary_id(iggy-gateway-kafka::list_offsets_real_bridge_tests) + + binary_id(iggy-gateway-kafka::produce_real_bridge_tests) +''' test-group = "kafka_bridge" [profile.default] diff --git a/gateways/kafka/README.md b/gateways/kafka/README.md index 71af7d23c8..209911eb13 100644 --- a/gateways/kafka/README.md +++ b/gateways/kafka/README.md @@ -1,14 +1,14 @@ # Kafka gateway (`iggy-gateway-kafka`) -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. +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. With a bridge, Produce writes to Iggy and ListOffsets reads offsets from it. Everything else is a stub. -> **Stub warning:** most APIs still don't persist or read real data. Produce and Fetch 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. ListOffsets is wired -> to the Iggy bridge: with `IGGY_KAFKA_BRIDGE_ENABLED=true` it answers `EARLIEST`/`LATEST` from -> real partition state; with the bridge off (the default) it stays a stub and answers -> `NOT_LEADER_OR_FOLLOWER` (6). See [docs/SCOPE.md](docs/SCOPE.md). +> **Stub warning:** When you set `IGGY_KAFKA_BRIDGE_ENABLED=true`, Produce writes to Iggy and +> ListOffsets answers `EARLIEST`/`LATEST` from real partition state. No other API stores or reads +> real data. Produce and ListOffsets without a bridge, and Fetch with or without one, answer +> retriable `NOT_LEADER_OR_FOLLOWER` (6). Clients then keep their data and do not trust a fake +> success. CreateTopics answers `NOT_CONTROLLER` (41) and creates nothing. Metadata reports every +> topic as unknown, so a real client cannot reach Produce or ListOffsets yet. See +> [docs/SCOPE.md](docs/SCOPE.md). ## Run @@ -78,9 +78,12 @@ See [docs/SCOPE.md](docs/SCOPE.md) for [#3421](https://github.com/apache/iggy/is ### Delivery guarantees Delivery through this gateway is **at-least-once**, and stays at-least-once across a gateway -restart. Transactions are not supported, and will not be. An idempotent Kafka producer is given -a producer id so that it starts, but its retries are not deduplicated: a retry after a network -timeout writes the record twice, and both copies reach the stream with their own offsets. +restart. Transactions are not supported, and will not be. A retry after a timeout can write a +record twice. + +Java producers must set `enable.idempotence=false` until the gateway serves `InitProducerId` +([#3545](https://github.com/apache/iggy/issues/3545)). Produce already stores idempotent batches. +It ignores their producer id, epoch and sequence, so it does not deduplicate a retry. Iggy deduplicates writes on its own partition plane, and that does not close this gap, because it guards the hop from the gateway to Iggy rather than the hop from the producer to the gateway. @@ -145,10 +148,52 @@ Full reasoning, including what was rejected and why, is in `src/bridge/` is the SDK integration layer: connects to Iggy, maps Kafka topics to Iggy streams/topics, provisions them on demand, and looks up high watermarks (one or many partitions of a topic per call) for `ListOffsets`. -**Not wired into the live Produce/Fetch dispatch path yet** - that lands with -[#3535](https://github.com/apache/iggy/issues/3535)/[#3536](https://github.com/apache/iggy/issues/3536). -Exercised today by `bridge`'s own unit tests and `tests/bridge_iggy_integration_tests.rs` (spawns a -real `iggy-server`). +ListOffsets ([#3537](https://github.com/apache/iggy/issues/3537)) and Produce +([#3535](https://github.com/apache/iggy/issues/3535), see below) call it. Fetch does not call it +yet ([#3536](https://github.com/apache/iggy/issues/3536)). +Tested by `bridge`'s own unit tests, `tests/bridge_iggy_integration_tests.rs`, +`tests/list_offsets_real_bridge_tests.rs` and `tests/produce_real_bridge_tests.rs`. The last three +start a real `iggy-server`. + +### Produce ([#3535](https://github.com/apache/iggy/issues/3535)) + +One partition, one Iggy send. Each partition answers for itself. + +| Field | Gateway | +| ----- | ------- | +| Partition | Index as sent. Both count from 0. | +| Base offset | From the send confirmation. `-1` if none. | +| `log_start_offset` | Always `-1`. | +| `acks` | `0`, `1`, `-1` write the same. Other values: 21, before any per-partition check. | +| `acks=0` | Writes, answers nothing. Any failed partition closes the connection. | +| Topics | Creates none. Missing: 3. Bad name: 17. | +| `timeout_ms` | Honored, max 20 s. Past it: 7. | +| Compression | gzip, snappy, lz4. zstd from v7, else 76. | +| Producer id, epoch, sequence | Ignored, so a retry writes twice. | +| Timestamps over about 71 min apart | Clamped into one send. `kafka.ts` keeps the real one. | +| Several batches in one partition | 87, as Kafka. | +| Bytes after the batch | 87. | +| More records than the batch declares | 87, as Kafka. | +| Same partition twice in one request | Written twice. Kafka keeps the last. | +| Repeated header name in a record | 87. | +| Produce v0-2, `acks=0` | Closes the connection. | +| Undecodable request | Closes the connection. | + +| Code | When | Client | +| ---- | ---- | ------ | +| 10 | Record, send or partition too large, even alone | Java splits multi-record batches. Else fails. | +| 87 | Record the gateway cannot map. Reason in `error_message` from v8. | Fails. | +| 35 | Transactional or control batch | Fails. | +| 6 | Request budget ran out. Nothing written. | Retries. | +| 7 | Deadline passed, or connection lost mid-send. May be written. | Retries. Can duplicate. | + +Partition cap: `max_frame_size` decompressed bytes, `max_frame_size / 64` record slots, 3 headers +per slot. Past it: 10. + +Request budget: 8 partition caps, refused partitions included. Past it: 6 for the rest, not +decoded. 4 requests decode at once. + +[docs/BRIDGE_MAPPING.md](docs/BRIDGE_MAPPING.md) describes what a record becomes once it is stored. ### Connection config @@ -159,6 +204,7 @@ real `iggy-server`). | `IGGY_KAFKA_IGGY_PASSWORD` | none - **required** | Iggy password. No default: `iggy-server` only uses the well-known `iggy`/`iggy` root credentials when started with `--with-default-root-credentials` (dev-only); otherwise it generates a random password, so a hardcoded default here could never be right and would invite running as root unnoticed | | `IGGY_KAFKA_IGGY_STREAM` | `kafka` | Default Iggy stream for a Kafka topic with no explicit mapping override | | `IGGY_KAFKA_TOPIC_MAP_PATH` | unset | Path to a topic-mapping TOML file (see below); omit to use only the default rule | +| `IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE` | `64MiB` | Iggy's `message_bus.max_message_size`. Set both together. Larger partitions answer 10 | The initial connect retries a fixed, bounded number of times (`RECONNECTION_RETRIES = 3`, not the Iggy SDK client's own default of unlimited retries, one dial per second, forever), and the whole @@ -170,16 +216,11 @@ reconnects internally, mid-call, on a transport error, through the same undead-l bridge call made well after the initial connect can still hit this if Iggy becomes unreachable later. -This timeout only bounds the *caller's* wait, not the SDK's own work: the SDK writes and reads on -a detached background task specifically so that dropping the awaiting future - what this timeout -does on expiry - cannot abort it mid-flight. A timed-out call can leave that task holding the -shared client's connection lock for up to another 30s (the SDK's own reply deadline), queuing -every other bridge call behind it. `IggyBridge` holds one `IggyClient` with no pooling (see -Concurrency ceiling below) and no semaphore bounding concurrent bridge calls - no longer a -hypothetical now that ListOffsets (`#3537`) calls it from a real handler; more pressing once -CreateTopics (`#3538`), Metadata (`#3534`), Produce (`#3535`) and Fetch (`#3536`) add their own -concurrent callers. See `IggyBridge`'s own doc comment (its rustdoc is private, so this isn't a -followable link outside the crate - read the source at `src/bridge/iggy_bridge.rs`). +`send_records` uses the Produce deadline instead: + +- One send runs in the SDK at a time. +- A send past its deadline keeps that slot until it ends, up to 45 s. Other sends answer 7 meanwhile. +- ListOffsets needs no slot, but it waits behind a stuck send in the SDK. Past `REQUEST_TIMEOUT` it answers 7. ### Topic mapping @@ -234,27 +275,26 @@ cut before that threshold is reached - worth knowing rather than discovering lat ### Concurrency ceiling -One `IggyBridge` (one `IggyClient`) is meant to serve every Kafka connection this gateway handles, -and the Iggy SDK's TCP transport is lockstep - one request in flight per client, its stream mutex -held across write, flush, and read. Every concurrent Kafka connection ends up serialized behind -whichever single Iggy request is in flight; the Kafka side's own connection limit -(`IGGY_KAFKA_MAX_CONNECTIONS`) does nothing to relieve this. No connection pooling exists yet - it -is a known gap to address before `#3535`/`#3536` put this on a hot path, not a design decision to -rely on. +- One `IggyClient` serves every Kafka connection, one Iggy request at a time. +- `IGGY_KAFKA_MAX_CONNECTIONS` does not change that. A client pool is a TODO in + [docs/SCOPE.md](docs/SCOPE.md). +- Order: set `max.in.flight.requests.per.connection=1`, or `retries=0`. Otherwise a retried batch + can land after later ones. ### Error mapping `BridgeError::to_kafka_error_code()` maps Iggy failures to Kafka wire error codes: -- Stream/topic/partition not found → `UNKNOWN_TOPIC_OR_PARTITION` (3) +- Stream, topic or partition not found → `UNKNOWN_TOPIC_OR_PARTITION` (3). This includes the + generic `ResourceNotFound` that a partition request returns when the server cannot resolve it - A rejected *permission* (`Unauthorized`) → `TOPIC_AUTHORIZATION_FAILED` (29) - a real, fixable-by-the-Kafka-operator ACL problem - A rejected *login* (the bridge's own `IGGY_KAFKA_IGGY_USERNAME`/`_PASSWORD` are wrong) → `UNKNOWN_SERVER_ERROR` (-1), deliberately **not** 29 - the Kafka client can't fix a bridge-side credential misconfiguration, and blaming its own ACLs for one is worse than an unexplained fatal error -- Connection-shaped failures → `NOT_LEADER_OR_FOLLOWER` (6, the same retriable code the - foundation's own stubs send, so a client backs off and retries) +- Connection-shaped failures → `NOT_LEADER_OR_FOLLOWER` (6). On a send, `Disconnected`, + `EmptyResponse`, `TcpError` and `StaleClient` → 7 instead: the write may have landed - An Iggy commit whose outcome is genuinely unknown (`TransientNotCommitted`) → `REQUEST_TIMED_OUT` (7) - retriable in real Kafka too, chosen because it's what a real broker sends for the same shape of failure, not to make a client stop retrying diff --git a/gateways/kafka/docs/BRIDGE_MAPPING.md b/gateways/kafka/docs/BRIDGE_MAPPING.md index 8791feffb6..8ae3cac772 100644 --- a/gateways/kafka/docs/BRIDGE_MAPPING.md +++ b/gateways/kafka/docs/BRIDGE_MAPPING.md @@ -39,7 +39,7 @@ Produce, per record: | record value | `payload` | | record key | `kafka.key` user header, `Raw` | | record header `name` | `kafka.h.` user header, `Raw` | -| record timestamp (CreateTime), milliseconds | `origin_timestamp`, microseconds | +| record timestamp (CreateTime), milliseconds | `origin_timestamp`, microseconds, plus `kafka.ts` when that cannot hold it | | record offset | partition offset, assigned by Iggy | | partition index | partition index, both 0-based | | topic | stream and topic per `TopicMapping` | @@ -100,28 +100,25 @@ A record that arrived through Produce survives the round trip exactly, because i value is always a whole number of milliseconds. A message an Iggy client wrote does not. Its sub-millisecond digits are lost on the way out, and Kafka has no field to keep them in. -Kafka sends `-1` for a record with no timestamp. That is stored as `0`, and Fetch already reads -a zero origin timestamp as an instruction to use the server-assigned timestamp instead. A real -broker does the same thing under `LogAppendTime`, so the two agree. +Kafka sends `-1` for a record with no timestamp. That is stored as `0`, and Fetch reads a zero +origin timestamp as the server-assigned one. A real broker does the same under `LogAppendTime`. -A record stamped at exactly `0` ms, the Unix epoch, is a different record that stores the same -`0`. It carries a `kafka.ts` header holding `epoch` to say so, and Fetch reads that header before -it reads the origin timestamp. Without it a producer that stamps a record `0` gets the server's -clock back instead, and never learns that the value changed. +One Iggy send holds origin timestamps at most `u32::MAX` µs apart, about 71.6 min +(`core/binary_protocol/src/batch.rs:55`). Kafka has no such bound. So each produce batch gets one +window, from its earliest real timestamp. A timestamp outside it is clamped in, and the `kafka.ts` +header (`Int64`, ms) keeps the real one: -One Iggy batch holds timestamps that span at most `MAX_TIMESTAMP_DELTA_MICROS`, which is -`u32::MAX` microseconds, about 71.6 minutes (`core/binary_protocol/src/batch.rs:55`). The send -encoder stores each message as a `u32` delta from the batch minimum and refuses a larger one -(`core/binary_protocol/src/requests/messages/send_messages.rs:133`). Kafka puts no such bound on -one produce batch, so two shapes fail: +| Record timestamp | `origin_timestamp` | `kafka.ts` | +| ---------------- | ------------------ | ---------- | +| In the window | timestamp × 1000 | none | +| After the window | window end | timestamp | +| `-1`, no real timestamp in the batch | `0` | none | +| `-1`, next to real timestamps | window start | `-1` | +| `0`, the epoch | window start, or `0` | `0` | -- a batch whose CreateTime values span more than 71.6 minutes, which replay and mirror producers - reach -- a batch that mixes a record with no timestamp, stored as `0`, with normally stamped records - -Neither is fixable in the record mapping, which sees one record and has no batch minimum to work -from. Produce ([#3535](https://github.com/apache/iggy/issues/3535)) owns splitting a produce -batch into sends the server accepts. +Fetch reads `kafka.ts` first. `-1` there means the server-assigned timestamp. An Iggy client sees +the clamped `origin_timestamp`. Only a batch over 71.6 min, or one mixing `-1` with real +timestamps, has one. ## Records Iggy cannot hold natively @@ -157,10 +154,9 @@ A record takes the fallback when any of these hold: - a header value is null, empty, or longer than 255 bytes - the headers together would exceed the 100 KB user-header budget -A repeated header name belonged on that list and is not reachable. Kafka allows one, but -`kafka_protocol` decodes headers into an `IndexMap` and inserts each in turn (`records.rs:919`), -so a repeat overwrites its earlier entry before any gateway code runs. The last value wins and -the record takes the native path. Catching the case needs a decoder this gateway does not have. +A repeated header name is refused with `INVALID_RECORD` (87). Kafka allows one, but +`kafka_protocol` decodes headers into an `IndexMap` and keeps only the last value. The gateway +checks the raw record bytes first (`scan_records`). Such a record is stored with a `kafka.envelope` header whose value is one byte, the byte layout version, currently `1`. The payload holds the key, the value and the headers in the layout below. @@ -238,9 +234,9 @@ bounded here, in different places, because no single place can see them both. The batch header's record count sits in the header, and `RecordBatchDecoder` reserves a `Vec` from it (`kafka-protocol-0.18.0/src/records.rs:517`). A 61-byte batch declaring `i32::MAX` records asks -for 377 GB. Produce reads every batch header before it decodes a record and refuses a count the -blob cannot produce. That ceiling is the blob's own length. If something in the blob is -compressed, the decompression budget is added to it. An uncompressed batch's records are already +for 377 GB. Produce reads the batch header before it decodes a record and refuses a count the +blob cannot produce. That ceiling is the blob's own length. If the batch is compressed, the whole +decompression budget is added to it. An uncompressed batch's records are already in the blob, and granting it the budget as well lets a 70-byte batch declare a million records. The per-record header count sits inside a record body, as a varint, so no batch header reports it @@ -251,8 +247,12 @@ headers aborts the process. Produce therefore walks the record bytes in the deco which runs for every batch including an uncompressed one. It refuses a header count the record's own bytes cannot hold, at two length varints per header. -Neither bound changes the ratio between a minimal wire record and a decoded `Record`. A -legitimate request of small records pays that ratio too. +- One batch per partition, as Kafka from v3. A second one: 87. +- More records than the batch declares: 87. The decoder would drop the extra ones. + +Neither bound changes the ratio between a wire record and a decoded `Record`. Produce therefore +also caps record slots per request at `max_frame_size / 64` (131,072 for 8 MiB). Headers take 1 +slot per 3. Past it: 10, or 6 if earlier partitions used the slots and this one fits alone. Produce decompresses gzip, snappy, lz4 and zstd batches. `gateways/kafka/Cargo.toml` turns those four features on for `kafka-protocol` and the workspace entry stays on `broker` alone, so the @@ -267,7 +267,8 @@ therefore still admit 4096 times that much output. Produce keeps a single decompression budget for the whole request, set to `max_frame_size`, so a compressed request can never yield more than the same client could have sent uncompressed. A -batch that exhausts the budget is rejected with `MESSAGE_TOO_LARGE` (10). Each decompressed +partition over the whole budget: `MESSAGE_TOO_LARGE` (10). One that fits alone after earlier +partitions used the budget: `NOT_LEADER_OR_FOLLOWER` (6). Each decompressed record value has to clear Iggy's own `MAX_PAYLOAD_SIZE` (64 MB, `iggy_message.rs:44`) separately, since one record becomes one message. @@ -281,10 +282,11 @@ write past the budget. Gzip, zstd and lz4 stream through that sink. Snappy is th states its output size up front, and `snap` reads that number without allocating. The declared size is charged before the decoder runs, for Kafka's own block framing and for raw snappy alike. -`records::decode_batches` decompresses, and `records::DecompressionBudget` is the bound. The -budget is a parameter, so setting it to `max_frame_size` for the whole request belongs to the -Produce handler in [#3535](https://github.com/apache/iggy/issues/3535). Until that lands, nothing -calls either one in a server path. +`records::decode_batch` decompresses, and `records::DecompressionBudget` is the bound. The +budget is a parameter, and the Produce handler +([#3535](https://github.com/apache/iggy/issues/3535)) sets it to `max_frame_size` once per +request. The handler converts and sends one entry at a time. It borrows the budget only in sync +calls, because a borrow held across an await makes the connection task `!Send`. ## Offsets diff --git a/gateways/kafka/docs/MANUAL_TESTING.md b/gateways/kafka/docs/MANUAL_TESTING.md index 3b4d4b9626..1524856766 100644 --- a/gateways/kafka/docs/MANUAL_TESTING.md +++ b/gateways/kafka/docs/MANUAL_TESTING.md @@ -134,13 +134,17 @@ The gateway binds `:9093` by default; `kafka-message-gen`'s own default is `:909 broker's usual port). Every command below passes `--host 127.0.0.1:9093` explicitly - omitting it connection-refuses before any assertion runs. +Every case below describes a gateway started **without** a bridge (`IGGY_KAFKA_BRIDGE_ENABLED` +unset). With a bridge, A5 writes one record to `test-topic`: `ec=0` if it exists, `ec=3` if not. +`tests/produce_real_bridge_tests.rs` covers the bridge path. + | ID | Test | Steps | Expected result | Pass criteria | | ---- | ------ | ------- | ----------------- | --------------- | | A1 | Gateway starts | Run `iggy-gateway-kafka` | Binds to `:9093`, no panic | Log shows bind address | | A2 | ApiVersions v1 | `cargo run -p kafka-message-gen -- send --host 127.0.0.1:9093 --api-key 18 --version 1` | Response received | `ec=0`, non-zero byte count | | A3 | ApiVersions v3 (flexible) | Same with `--version 3` | Response received | `ec=0` | | A4 | Metadata v0 | `send --host 127.0.0.1:9093 --api-key 3 --version 0` | Stub broker in response | Topic entries show `ec=3` (UNKNOWN_TOPIC_OR_PARTITION, stub) | -| A5 | Produce v3 | `send --host 127.0.0.1:9093 --api-key 0 --version 3` | Decode + stub retriable error | `ec=6` (NOT_LEADER_OR_FOLLOWER) per partition | +| A5 | Produce v3 | `send --host 127.0.0.1:9093 --api-key 0 --version 3` | Decode + stub retriable error, no bridge | `ec=6` (NOT_LEADER_OR_FOLLOWER) per partition | | A6 | Fetch v4 | `send --host 127.0.0.1:9093 --api-key 1 --version 4` | Decode + stub response | Top-level `ec=0`; per-partition `ec=6` (NOT_LEADER_OR_FOLLOWER) | | A7 | ListOffsets v1 | `send --host 127.0.0.1:9093 --api-key 2 --version 1` | Decode + stub offsets | Per-partition `ec=6` (NOT_LEADER_OR_FOLLOWER) - no top-level error field on this response | | A8 | CreateTopics v2 | `send --host 127.0.0.1:9093 --api-key 19 --version 2` | Decode + stub non-creation ack | `ec=41` (NOT_CONTROLLER) per topic | @@ -196,8 +200,8 @@ that one connection) is still serving other clients. Run every `send` below with `--host 127.0.0.1:9093`. This category validates that the wire encoding round-trips at the legacy/flexible boundary - not the stub error code, which is -per-API and constant across both rows (see Category A: `ec=6` for Produce, `ec=41` for -CreateTopics, and so on). +per-API and constant across both rows (see Category A: `ec=6` for Produce with no bridge, `ec=41` +for CreateTopics, and so on). | ID | API key | Version | Encoding | Validation | | ---- | --------- | --------- | ---------- | ------------ | @@ -239,7 +243,7 @@ Requires `kcat` installed. Gateway does **not** implement SASL or full broker se | ID | Test | Command | Expected (foundation) | | ---- | ------ | --------- | --------------------- | | G1 | Broker metadata | `kcat -b 127.0.0.1:9093 -L` | ApiVersions + Metadata handshake; broker appears in metadata | -| G2 | Produce (likely fails later) | `echo "hello" \| kcat -b 127.0.0.1:9093 -t test -P` | May fail at coordinator/group stage — document actual error | +| G2 | Produce (fails at metadata) | `echo "hello" \| kcat -b 127.0.0.1:9093 -t test -P` | Fails before it sends a Produce request. Metadata reports every topic as unknown, so the client finds no leader. [#3534](https://github.com/apache/iggy/issues/3534) unblocks this, not the Produce handler | | G3 | Consumer (likely fails later) | `kcat -b 127.0.0.1:9093 -t test -C -o beginning` | May fail without consumer groups — document actual error | Record kcat version and exact error strings in your test log. G1 passing is the minimum bar for client compatibility smoke. @@ -248,7 +252,7 @@ Record kcat version and exact error strings in your test log. G1 passing is the | ID | Test | Steps | Expected | | ---- | ------ | ------- | ---------- | -| H1 | Truncated Produce body | Send valid header + incomplete body | **No response at all** - `kafka_protocol` decodes the whole request in one shot, so a failure anywhere leaves `acks` unknowable; answering risks desyncing an `acks=0` fire-and-forget client's correlation stream, so every Produce decode failure stays silent. Connection stays open (send A2 next to confirm); **no panic** | +| H1 | Truncated Produce body | Send valid header + incomplete body | No response. Connection closed. **No panic** | | H2 | Random bytes | `dd if=/dev/urandom bs=64 count=1 \| nc 127.0.0.1 9093` | Connection closed or protocol error; gateway stays up | | H3 | Empty body after header | ApiVersions with valid header, empty body | `ec=0` (ApiVersions accepts empty body) | @@ -260,14 +264,22 @@ Record kcat version and exact error strings in your test log. G1 passing is the | Code | Name | When returned | | ------ | ------ | --------------- | +| -1 | UNKNOWN_SERVER_ERROR | Produce with a bridge: Iggy error with no closer code, or bad bridge login | | 0 | NONE | Fetch top-level error field only (`ec=0` there does not mean per-partition success - see A6) | -| 6 | NOT_LEADER_OR_FOLLOWER | Produce/Fetch/ListOffsets stub, per partition (retriable; payload not persisted) | -| 3 | UNKNOWN_TOPIC_OR_PARTITION | Metadata stub, per topic | -| 35 | UNSUPPORTED_VERSION | **ApiVersions only** (KIP-511 exception). Every other API key's out-of-range version closes the connection instead - see Category B | +| 3 | UNKNOWN_TOPIC_OR_PARTITION | Metadata stub, per topic. Produce with a bridge: missing topic or partition | +| 6 | NOT_LEADER_OR_FOLLOWER | Produce/Fetch/ListOffsets stub (not stored). Produce with a bridge: Iggy unreachable, or the request budget ran out | +| 7 | REQUEST_TIMED_OUT | Produce with a bridge: deadline passed, or connection lost mid-send (may be stored) | +| 10 | MESSAGE_TOO_LARGE | Produce with a bridge: record, send or partition too large, even alone | +| 17 | INVALID_TOPIC_EXCEPTION | Produce with a bridge: bad topic name | +| 21 | INVALID_REQUIRED_ACKS | Produce with a bridge: `acks` is not 0, 1 or -1 | +| 29 | TOPIC_AUTHORIZATION_FAILED | Produce with a bridge: Iggy user lacks permission | +| 35 | UNSUPPORTED_VERSION | **ApiVersions only** (KIP-511 exception), plus Produce with a bridge: transactional or control batch. Every other API key's out-of-range version closes the connection instead - see Category B | | 37 | INVALID_PARTITIONS | CreateTopics: partition count `0` or `< -1` (or any non-positive on v2–v3) | | 38 | INVALID_REPLICATION_FACTOR | CreateTopics: replication factor `0` or `< -1` (or any non-positive on v2–v3) | | 41 | NOT_CONTROLLER | CreateTopics stub (topic not created) | -| 42 | INVALID_REQUEST | Fetch/ListOffsets/CreateTopics/ApiVersions decode failure. **Not** Produce - a Produce decode failure always stays silent (`NoResponse`), never `ec=42` - see H1 | +| 42 | INVALID_REQUEST | Fetch/ListOffsets/CreateTopics/ApiVersions decode failure. Not Produce: it closes the connection (H1) | +| 76 | UNSUPPORTED_COMPRESSION_TYPE | Produce with a bridge: zstd before v7 | +| 87 | INVALID_RECORD | Produce with a bridge: a record batch this gateway cannot map, including a missing, empty or second one, or bytes after it | A malformed request header (before any API-specific body is even reached) has no parsed header to build a version-correct response against, so it closes the connection rather than returning any diff --git a/gateways/kafka/docs/SCOPE.md b/gateways/kafka/docs/SCOPE.md index 4645d04dd0..235342b01e 100644 --- a/gateways/kafka/docs/SCOPE.md +++ b/gateways/kafka/docs/SCOPE.md @@ -2,9 +2,9 @@ ## Issue #3421 — in scope (this iteration) -Foundation layer only: a TCP listener on the Kafka wire port that decodes requests, validates scoped API keys and versions, validates request wire formats, and returns stub responses. **No Iggy backend integration.** +Foundation layer only: a TCP listener on the Kafka wire port that decodes requests, validates scoped API keys and versions, validates request wire formats, and returns stub responses. With a bridge, Produce writes to Iggy and ListOffsets reads offsets from it. -**Stub semantics (important):** Produce discards the payload and answers with retriable `NOT_LEADER_OR_FOLLOWER` (6). CreateTopics validates the request but answers with `NOT_CONTROLLER` (41) so clients do not believe topics were created. Do not treat `ec=0` stub success as durable storage — that arrives in the Iggy bridge phase. +**Stub semantics (important):** without a bridge, every API answers with a stub. Produce discards the payload and answers with retriable `NOT_LEADER_OR_FOLLOWER` (6). CreateTopics validates the request but answers `NOT_CONTROLLER` (41), so clients do not believe topics were created. Do not read `ec=0` from a stub as durable storage. Produce is the one API that stores real data once you configure a bridge ([#3535](https://github.com/apache/iggy/issues/3535)). | Deliverable | Status | Location | | ------------- | -------- | ---------- | @@ -44,7 +44,7 @@ it knows the server supports flexible encoding. | --------- | ------ | ------------- | ------------- | ---------------- | ---------- | | 18 | ApiVersions | 0 | 3 | 0, 1, 2, 3 | Advertise supported ranges; flexible encoding at v3+ | | 3 | Metadata | 0 | 9 | 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 | Decode topic list count; stub broker host from `advertised_host` or the bound `local_addr` IP; flexible encoding at v9+ | -| 0 | Produce | 3 | 9 | 3, 4, 5, 6, 7, 8, 9 | Decode request; stub returns `NOT_LEADER_OR_FOLLOWER` (6) | +| 0 | Produce | 3 | 9 | 3, 4, 5, 6, 7, 8, 9 | With a bridge: one `send_messages` per partition. Without one: stub returns `NOT_LEADER_OR_FOLLOWER` (6) | | 1 | Fetch | 4 | 12 | 4, 5, 6, 7, 8, 9, 10, 11, 12 | Decode request; stub response | | 2 | ListOffsets | 1 | 6 | 1, 2, 3, 4, 5, 6 | Decode request; stub response | | 19 | CreateTopics | 2 | 5 | 2, 3, 4, 5 | Decode request; stub returns `NOT_CONTROLLER` (41); `-1` partitions/RF = broker default on v4+ | @@ -88,8 +88,8 @@ Full reference for future phases: [`kafka_api_keys_reference.md`](kafka_api_keys | Layer | #3421 | Description | | ------- | ------- | ------------- | | **1 — Wire framing** | In scope | `server.rs` — custom, zero-copy frame I/O; `header.rs` delegates version selection to `kafka_protocol::messages::ApiKey` | -| **2 — Request/response codecs** | Partial | Decode/encode via the `kafka_protocol` crate (broker feature only) for 6 hot-path keys; `bounds_guard.rs` pre-validates against unbounded allocation before handing a frame to the crate; stub responses only | -| **3 — Iggy bridge** | Landed, not wired in | `bridge/` module (connection, topic mapping, provisioning, high watermark) landed; Produce/Fetch handler wiring itself is a follow-on ([#3535](https://github.com/apache/iggy/issues/3535)/[#3536](https://github.com/apache/iggy/issues/3536)) | +| **2 — Request/response codecs** | Partial | Decode/encode via the `kafka_protocol` crate (broker feature only) for 6 hot-path keys; `bounds_guard.rs` pre-validates against unbounded allocation before handing a frame to the crate; stub responses except Produce and ListOffsets with a bridge | +| **3 — Iggy bridge** | Produce and ListOffsets wired | `bridge/` module (connection, topic mapping, provisioning, high watermark, `topic_target` + `send_records`). Produce ([#3535](https://github.com/apache/iggy/issues/3535)) and ListOffsets ([#3537](https://github.com/apache/iggy/issues/3537)) call it. Fetch does not call it yet ([#3536](https://github.com/apache/iggy/issues/3536)) | --- @@ -103,10 +103,18 @@ Items from the [hybrid architecture review](https://github.com/apache/iggy/discu below it are still open for the issues that build on top of it. - [x] Add `bridge/` module (`iggy_bridge`) - connection lifecycle, topic mapping, provisioning, - high watermark, error mapping. See [README.md](../README.md#iggy-bridge-3533). Produce → - `send_messages` / Fetch → `poll_messages` handler wiring itself is - [#3535](https://github.com/apache/iggy/issues/3535)/[#3536](https://github.com/apache/iggy/issues/3536), - not part of `bridge/`'s own scope. + high watermark, error mapping. See [README.md](../README.md#iggy-bridge-3533). +- [x] Produce → `send_messages` ([#3535](https://github.com/apache/iggy/issues/3535)) - one call + per partition, base offset from the send confirmation, one error code per partition. See + [README.md](../README.md#produce-3535). +- [ ] Produce: `IggyClient` pool. Pin each partition to one client, so order holds. +- [ ] Produce: write keyed records' header TLVs into one buffer (`records::to_iggy`). Benchmark + first. Keep Iggy's TLV layout and the 100 KB header check. +- [ ] Produce: skip the second batch CRC pass (`records::preflight`) only if profiling shows it + matters. A corrupt header must still answer 87. +- [ ] Produce stub, `acks=0`: close the connection when the request names a partition, as Kafka + does. v0-2 needs the counts read from the raw body. +- [ ] Fetch → `poll_messages` ([#3536](https://github.com/apache/iggy/issues/3536)). - [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] Document partition mapping in [`BRIDGE_MAPPING.md`](BRIDGE_MAPPING.md): diff --git a/gateways/kafka/docs/TEST_SUITE.md b/gateways/kafka/docs/TEST_SUITE.md index e912a421a4..1019321205 100644 --- a/gateways/kafka/docs/TEST_SUITE.md +++ b/gateways/kafka/docs/TEST_SUITE.md @@ -20,10 +20,10 @@ suite goes through `tests/common/fixtures.rs::load_fixture_body_or_skip`, which regeneration hint when a fixture is missing, and panics instead when `KAFKA_FIXTURES_REQUIRED=1` is set (CI sets this) so a broken generation step can't leave a suite green with zero assertions. -### `iggy-server` binary (required for `bridge_iggy_integration_tests`) +### `iggy-server` binary (required for `bridge_iggy_integration_tests`, `list_offsets_real_bridge_tests` and `produce_real_bridge_tests`) -No fixtures needed, but `iggy-server` has to be built *first* - this suite spawns it directly and -does not build it for you: +No fixtures needed, but `iggy-server` has to be built *first* - these suites spawn it directly and +do not build it for you: ```bash cargo build --package server --bin iggy-server @@ -65,9 +65,10 @@ file under `tests/` anymore. | [`listener_robustness_tests.rs`](../tests/listener_robustness_tests.rs) | TCP listener robustness — framing, pipelining, concurrency, connection limits | No | | [`sasl_tests.rs`](../tests/sasl_tests.rs) | SASL/PLAIN over a socket — full handshake, every refusal path, and the disabled default. Drives a stub verifier implementing `SaslAuthenticator`, so no Iggy server is needed | No | | [`bridge_iggy_integration_tests.rs`](../tests/bridge_iggy_integration_tests.rs) | `IggyBridge` against a real, spawned `iggy-server` — provisioning idempotency, high watermark, credential/connection edge cases | No (needs the `iggy-server` binary - see Prerequisites) | +| [`produce_real_bridge_tests.rs`](../tests/produce_real_bridge_tests.rs) | Produce (key 0) through the whole handler against a real, spawned `iggy-server` — records go in as Kafka wire bytes and come back through the Iggy SDK, plus one error code per partition | No (needs the `iggy-server` binary - see Prerequisites) | `tests/common/` holds shared helpers (`codec.rs`, `fixtures.rs`, `scope.rs`, `server.rs`, -`tcp.rs`, `wire.rs`), compiled per test binary via `#[path]`, not a test binary itself. `codec.rs` +`iggy_server.rs`, `tcp.rs`, `wire.rs`), compiled per test binary via `#[path]`, not a test binary itself. `codec.rs` is test-only primitive encode/decode scaffolding for hand-building legacy/adversarial wire shapes `kafka_protocol`'s spec-correct encoder cannot produce - it is not the gateway's production codec. diff --git a/gateways/kafka/src/bridge/config.rs b/gateways/kafka/src/bridge/config.rs index faad801a5a..8e8de7af1f 100644 --- a/gateways/kafka/src/bridge/config.rs +++ b/gateways/kafka/src/bridge/config.rs @@ -17,6 +17,7 @@ use std::path::Path; +use iggy::prelude::IggyByteSize; use secrecy::SecretString; use tracing::warn; @@ -33,6 +34,8 @@ pub(crate) const DEFAULT_IGGY_ADDR: &str = "127.0.0.1:8090"; /// one field is safe. The password is a different story - see why there is no /// `DEFAULT_IGGY_PASSWORD` at [`IggyBridgeConfig::from_env`]. const DEFAULT_IGGY_USERNAME: &str = "iggy"; +/// Iggy's default `message_bus.max_message_size`. +pub const DEFAULT_MAX_MESSAGE_SIZE: u64 = 64 * 1024 * 1024; /// Connection + topic-mapping config for [`IggyBridge`](crate::bridge::iggy_bridge::IggyBridge). /// @@ -45,6 +48,8 @@ pub struct IggyBridgeConfig { pub username: String, pub password: SecretString, pub topic_mapping: TopicMapping, + /// Iggy's `message_bus.max_message_size`, in bytes. Larger partitions answer 10. + pub max_message_size: u64, } impl IggyBridgeConfig { @@ -62,6 +67,7 @@ impl IggyBridgeConfig { "IGGY_KAFKA_IGGY_PASSWORD", "IGGY_KAFKA_IGGY_STREAM", "IGGY_KAFKA_TOPIC_MAP_PATH", + "IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE", ]; /// Builds config from `IGGY_KAFKA_*` env vars, defaulting to the Iggy server's own @@ -142,11 +148,26 @@ impl IggyBridgeConfig { )? }; + let max_message_size = match std::env::var("IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE") { + Err(_) => DEFAULT_MAX_MESSAGE_SIZE, + Ok(value) => value + .parse::() + .ok() + .map(|size| size.as_bytes_u64()) + .filter(|bytes| *bytes > 0) + .ok_or_else(|| { + BridgeError::InvalidConfig(format!( + "IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE '{value}' is not a size, for example 64MiB" + )) + })?, + }; + Ok(Self { address, username, password: SecretString::from(password), topic_mapping, + max_message_size, }) } } @@ -164,6 +185,7 @@ mod tests { password: SecretString::from("iggy"), topic_mapping: TopicMapping::new("kafka".to_string(), std::collections::HashMap::new()) .expect("valid mapping for this test's fixture data"), + max_message_size: DEFAULT_MAX_MESSAGE_SIZE, } } @@ -197,6 +219,7 @@ mod tests { std::env::remove_var("IGGY_KAFKA_IGGY_USERNAME"); std::env::remove_var("IGGY_KAFKA_IGGY_STREAM"); std::env::remove_var("IGGY_KAFKA_TOPIC_MAP_PATH"); + std::env::remove_var("IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE"); } let result = IggyBridgeConfig::from_env(); unsafe { @@ -204,6 +227,7 @@ mod tests { } let config = result.expect("valid config from documented defaults alone"); + assert_eq!(config.max_message_size, DEFAULT_MAX_MESSAGE_SIZE); assert_eq!(config.address, "127.0.0.1:8090"); assert_eq!(config.username, "iggy"); assert_eq!(config.topic_mapping.default_stream(), "kafka"); @@ -215,6 +239,29 @@ mod tests { ); } + #[test] + #[serial] + fn from_env_reads_the_iggy_message_size_cap() { + // Safety: process-wide, not per-variable - see the note on + // from_env_uses_documented_defaults_when_only_password_is_set above. + unsafe { + std::env::set_var("IGGY_KAFKA_IGGY_PASSWORD", "iggy"); + std::env::set_var("IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE", "1MiB"); + } + let one_mebibyte = IggyBridgeConfig::from_env(); + unsafe { + std::env::set_var("IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE", "lots"); + } + let garbage = IggyBridgeConfig::from_env(); + unsafe { + std::env::remove_var("IGGY_KAFKA_IGGY_PASSWORD"); + std::env::remove_var("IGGY_KAFKA_IGGY_MAX_MESSAGE_SIZE"); + } + + assert_eq!(one_mebibyte.unwrap().max_message_size, 1024 * 1024); + assert!(matches!(garbage, Err(BridgeError::InvalidConfig(_)))); + } + #[test] #[serial] fn from_env_rejects_missing_password() { diff --git a/gateways/kafka/src/bridge/error.rs b/gateways/kafka/src/bridge/error.rs index 10f2035e32..83532d0f27 100644 --- a/gateways/kafka/src/bridge/error.rs +++ b/gateways/kafka/src/bridge/error.rs @@ -45,18 +45,13 @@ pub enum BridgeError { /// from an actual server rejection are affected. #[error("Iggy client error: {0}")] Iggy(#[from] IggyError), - /// A bridge call exceeded its wall-clock budget (`with_request_timeout`'s `REQUEST_TIMEOUT`) - /// without a definitive answer from Iggy. - /// - /// Distinct from `Iggy(IggyError::CannotEstablishConnection)`: that variant means the SDK - /// itself gave up on establishing or using the connection, a known failure safe to retry. A - /// timeout here means only that *this caller* stopped waiting - the SDK does its write/read - /// inside a detached task this timeout cannot abort (`with_request_timeout`'s own doc has the - /// mechanism), so the request may already be on the wire, or already applied server-side, by - /// the time this fires. That is the same unknown-outcome shape `IggyError::TransientNotCommitted` - /// is, which is why this maps like that variant, not like a known connection failure. + /// A bridge call ran past its deadline. The SDK cannot cancel its detached write, so the + /// call may still land: 7, like `IggyError::TransientNotCommitted`. #[error("bridge call timed out waiting for a reply from Iggy")] Timeout, + /// The connection broke during a send, and the SDK does not replay it. It may have landed. + #[error("Iggy connection lost during a send: {0}")] + SendLost(IggyError), /// `high_watermark` was asked about a partition index the topic doesn't have. #[error( "partition {partition} out of range for topic '{topic}' ({partitions_count} partitions)" @@ -102,7 +97,7 @@ impl BridgeError { pub const fn to_kafka_error_code(&self) -> i16 { match self { Self::Iggy(err) => iggy_error_to_kafka_code(err), - Self::Timeout => ERROR_REQUEST_TIMED_OUT, + Self::Timeout | Self::SendLost(_) => ERROR_REQUEST_TIMED_OUT, Self::PartitionOutOfRange { .. } => ERROR_UNKNOWN_TOPIC_OR_PARTITION, Self::PartitionCountMismatch { .. } => ERROR_TOPIC_ALREADY_EXISTS, Self::InvalidKafkaTopicName { .. } => ERROR_INVALID_TOPIC_EXCEPTION, @@ -114,6 +109,19 @@ impl BridgeError { Self::InvalidConfig(_) => ERROR_UNKNOWN_SERVER_ERROR, } } + + /// True when Iggy rejected the bridge's own login. The Kafka client only sees -1. + #[must_use] + pub const fn is_bridge_login_rejected(&self) -> bool { + matches!( + self, + Self::Iggy( + IggyError::InvalidCredentials + | IggyError::InvalidUsername + | IggyError::InvalidPassword + ) + ) + } } /// Kept private so this association can change without touching call sites. @@ -127,12 +135,21 @@ impl BridgeError { /// `ClientState::Connected`, the ordinary window between a reconnect's TCP handshake completing /// and its auto-sign-in landing, not for a rejected login. /// +/// `ResourceNotFound` joins them despite naming no resource. `dispatch_partition_request` +/// (`core/server/src/dispatch/partition.rs`) flattens every unresolved stream, topic and +/// partition into it, so a `send_messages` to a missing topic or partition arrives as this and +/// never as the typed variants, which only the bridge's own `get_topic` paths build locally. +/// Every resource this bridge addresses is one of those three. +/// /// `TransientNotAccepted` (Iggy replica-side "retry, on any replica") is retriable the same way. /// `TransientNotCommitted` is not folded into that set: its outcome is genuinely unknown rather /// than known-safe-to-retry, so it maps to `REQUEST_TIMED_OUT` instead of /// `NOT_LEADER_OR_FOLLOWER` - a caller must not treat it as an ordinary retriable failure and risk /// a duplicate write. /// +/// 6 and 7 do not prove whether a send landed. For example, a reconnect that fails after a +/// written send returns `CannotEstablishConnection` (6). Java retries both. +/// /// `Unauthorized` is the only one of the four credential-shaped variants that stays on /// `TOPIC_AUTHORIZATION_FAILED` (29): it means the authenticated user lacks a permission, which is /// a real, fixable-by-the-Kafka-operator ACL problem. `InvalidCredentials`/`InvalidUsername`/ @@ -143,16 +160,16 @@ impl BridgeError { /// startup-only: the SDK re-runs sign-in on every reconnect (`tcp_client.rs`), so a since-rotated /// bridge password surfaces this mid-request, not just at boot. They fall to /// `UNKNOWN_SERVER_ERROR` instead - correctly fatal (retrying won't fix a wrong password), but -/// without asserting a cause the Kafka client cannot act on. A handler wiring this in -/// (`#3535`/`#3536`) should log the real `IggyError` at `error!` level server-side, since the -/// Kafka client will never see more than "-1" for it. +/// without asserting a cause the Kafka client cannot act on. Produce logs them at `error!` +/// (see [`BridgeError::is_bridge_login_rejected`]). const fn iggy_error_to_kafka_code(err: &IggyError) -> i16 { match err { IggyError::StreamIdNotFound(_) | IggyError::StreamNameNotFound(_) | IggyError::TopicIdNotFound(_, _) | IggyError::TopicNameNotFound(_, _) - | IggyError::PartitionNotFound(_, _, _) => ERROR_UNKNOWN_TOPIC_OR_PARTITION, + | IggyError::PartitionNotFound(_, _, _) + | IggyError::ResourceNotFound(_) => ERROR_UNKNOWN_TOPIC_OR_PARTITION, IggyError::Unauthorized => ERROR_TOPIC_AUTHORIZATION_FAILED, IggyError::Disconnected | IggyError::EmptyResponse @@ -194,6 +211,14 @@ mod tests { assert_eq!(err.to_kafka_error_code(), ERROR_UNKNOWN_TOPIC_OR_PARTITION); } + #[test] + fn resource_not_found_maps_to_unknown_topic_or_partition() { + // The only code a `send_messages` to a missing topic or partition ever comes back as: + // the server flattens both to this one rather than to the typed variants above. + let err = BridgeError::Iggy(IggyError::ResourceNotFound(String::new())); + assert_eq!(err.to_kafka_error_code(), ERROR_UNKNOWN_TOPIC_OR_PARTITION); + } + #[test] fn unauthenticated_maps_to_not_leader_or_follower_for_retry() { // Not a credential rejection: fail_if_not_authenticated (binary_auth.rs) returns this for @@ -347,49 +372,23 @@ mod tests { assert_eq!(err.to_kafka_error_code(), ERROR_TOPIC_ALREADY_EXISTS); } - /// Every `ERROR_*` constant this module sends, checked against `kafka-protocol`'s own - /// `ResponseError` table - the crate's canonical copy of Kafka's real wire numbers, not this - /// module's own. Every other test above pins routing (which `IggyError` maps to which - /// constant); without this, a wrong constant value would still pass all of them, since they - /// only ever compare against the same constants the function returns. #[test] - fn every_sent_error_code_matches_kafka_protocols_own_table() { - use kafka_protocol::error::ResponseError; - - for (ours, theirs) in [ - ( - ERROR_UNKNOWN_SERVER_ERROR, - ResponseError::UnknownServerError, - ), - ( - ERROR_UNKNOWN_TOPIC_OR_PARTITION, - ResponseError::UnknownTopicOrPartition, - ), - ( - ERROR_NOT_LEADER_OR_FOLLOWER, - ResponseError::NotLeaderOrFollower, - ), - (ERROR_REQUEST_TIMED_OUT, ResponseError::RequestTimedOut), - ( - ERROR_INVALID_TOPIC_EXCEPTION, - ResponseError::InvalidTopicException, - ), - ( - ERROR_TOPIC_AUTHORIZATION_FAILED, - ResponseError::TopicAuthorizationFailed, - ), - ( - ERROR_TOPIC_ALREADY_EXISTS, - ResponseError::TopicAlreadyExists, - ), - (ERROR_INVALID_PARTITIONS, ResponseError::InvalidPartitions), + 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_eq!( - ours, - theirs.code(), - "{theirs:?} is {} in kafka-protocol, not {ours}", - theirs.code() - ); + 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/bridge/iggy_bridge/mod.rs b/gateways/kafka/src/bridge/iggy_bridge/mod.rs index 5d388ff6f4..c6744e36e4 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/mod.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/mod.rs @@ -16,13 +16,18 @@ // under the License. use std::future::Future; +use std::sync::Arc; use std::time::Duration; -use iggy::prelude::{AutoLogin, Client, Credentials, IggyClient, IggyClientBuilder, IggyError}; +use iggy::prelude::{ + AutoLogin, Client, Credentials, Identifier, IggyClient, IggyClientBuilder, IggyError, +}; +use tokio::sync::Semaphore; use tracing::info; use crate::bridge::config::IggyBridgeConfig; use crate::bridge::error::BridgeError; +use crate::bridge::topic_map::validate_kafka_topic_name; mod fetch; mod offsets; @@ -47,51 +52,17 @@ const RECONNECTION_RETRIES: u32 = 3; /// and to every call made after it succeeds (`get_stream`, `create_stream`, `get_topic`, /// `create_topic`, `shutdown`). /// -/// One constant, not two separately-named ones with the same value: both call sites bound the -/// identical underlying hazard. `TcpClient::establish_bounded` only applies its own -/// `FAILOVER_DIAL_TIMEOUT` (2s) when at least two failover candidates are configured -/// (`tcp_client.rs`) - a bridge always configures exactly one address, so that guard never engages -/// at either site, and the plain `TcpStream::connect` underneath has no deadline of its own. -/// Against a firewall that drops SYN packets instead of refusing them, each dial attempt pays the -/// kernel's own SYN-retry timeout (minutes, not seconds) rather than the `reconnection_interval` -/// between attempts - a closed port (instant RST) never exercises this path, so the failure mode -/// only shows up in production. 15s comfortably covers a slow-but-alive server's handshake (well -/// above p99 login latency) while still failing well short of the pathological multi-minute case. +/// 15s covers a slow but live server's handshake and login. /// -/// Known limitation, not closed by this constant: [`with_request_timeout`] cancels only the -/// *caller's* wait, not the SDK's own work. `TcpClient::send_raw_vsr_attempt` (`tcp_client.rs`) -/// writes, flushes and reads inside a detached `tokio::spawn` specifically so that dropping the -/// awaiting future - exactly what this timeout does on expiry - cannot abort it mid-flight (that -/// function's own "SAFETY: we run code holding the `stream` lock in a task so we can't be -/// cancelled while holding the lock"). A call that times out here can leave that detached task -/// still holding `IggyClient`'s single stream mutex for up to `RESPONSE_READ_TIMEOUT` (30s, -/// `tcp_client.rs`) longer, queuing every other bridge call behind it. Raising this constant does -/// not close the gap either: a mid-call reconnect (`send_raw_with_response`, `tcp_client.rs`) -/// replays on a fresh `RESPONSE_READ_TIMEOUT` budget, and the reconnect dial itself goes through -/// the same undead-lined `TcpStream::connect` this constant exists to bound - genuinely unbounded, -/// so no finite value here can guarantee catching it. That same unboundedness is why this timeout -/// cannot simply be dropped for post-connect calls either: [`IggyBridge`] holds one `IggyClient` -/// with no pooling, so an unbounded reconnect dial with nothing here to stop it would wedge every -/// later call on this bridge, not just the one that triggered it. No longer a hypothetical: since -/// `ListOffsets` (#3537) this bridge is called from a live Kafka handler, with no semaphore -/// bounding concurrent bridge calls. Closing the underlying gap needs either a cooperatively -/// cancellable SDK call or a deadline on the SDK's own reconnect dial, neither of which this -/// bridge can add from the outside; bounding concurrent bridge calls is a separate, addressable -/// fix that becomes more pressing as `CreateTopics` and `Metadata` (#3538/#3534) add more live -/// callers. +/// A reconnect dial runs in the caller's future, so this timeout cancels a hung dial too. +/// +/// Sends use the Produce deadline instead. See `send_records`. const REQUEST_TIMEOUT: Duration = Duration::from_secs(15); -/// Wraps a single Iggy client call in [`REQUEST_TIMEOUT`]. See that constant's doc for why every -/// bridge method needs this, not just [`IggyBridge::connect`] - and for the cancellation gap this -/// wrapper does not close. +/// Wraps a single Iggy client call in [`REQUEST_TIMEOUT`]. /// /// On expiry, maps to [`BridgeError::Timeout`], not `IggyError::CannotEstablishConnection`: the -/// SDK's write/read run in a detached task this timeout cannot abort (see [`REQUEST_TIMEOUT`]'s -/// doc), so by the time this fires the request may already be on the wire, or already applied -/// server-side - a genuinely unknown outcome, not a known connection failure. `BridgeError`'s own -/// mapping already treats an unknown outcome (`IggyError::TransientNotCommitted`) as distinct from -/// a known-safe-to-retry one for exactly this reason; a caller-side timeout is the same shape and -/// must not borrow the known-safe code. +/// SDK's write/read run in a detached task this timeout cannot abort, so the call may still land. async fn with_request_timeout( op: impl Future>, ) -> Result { @@ -103,21 +74,19 @@ async fn with_request_timeout( /// Owns one connected `IggyClient` and resolves Kafka topics against it. /// -/// Produce/Fetch handler wiring is a separate, later change (`#3535`/`#3536`) - this type is the -/// shared plumbing those handlers will call into, exercised standalone here via its own tests and -/// an integration test against a real `iggy-server`. -/// -/// One `IggyClient`, shared across every Kafka connection this gateway serves - and the SDK's TCP -/// transport is lockstep, one request in flight at a time (`tcp_client.rs`: "the connection is -/// lockstep", its stream mutex held across write, flush, and read). Every concurrent Kafka -/// connection ends up serialized behind whichever single Iggy request is in flight; the Kafka -/// side's own connection limit does nothing to relieve this. No pooling exists yet - `close` -/// already takes `self` by value, which anticipates an eventual `Arc`-shared bridge, but nothing -/// wires that up today. Documented here and in the README rather than silently discovered under -/// load once `#3535`/`#3536` land. +/// One lockstep client serves every Kafka connection, so Iggy calls run one at a time. A pool is +/// a TODO in `docs/SCOPE.md`. pub struct IggyBridge { - client: IggyClient, + client: Arc, config: IggyBridgeConfig, + /// One Produce send inside the SDK at a time. See `send_records`. + send_slot: Arc, +} + +/// The Iggy stream and topic one Kafka topic maps to. Resolve once per topic, use many times. +pub struct TopicTarget { + stream_id: Identifier, + topic_id: Identifier, } impl IggyBridge { @@ -158,7 +127,28 @@ impl IggyBridge { with_request_timeout(client.connect()).await?; info!("Iggy bridge connected to {}", config.address); - Ok(Self { client, config }) + Ok(Self { + client: Arc::new(client), + config, + send_slot: Arc::new(Semaphore::new(1)), + }) + } + + /// Checks `kafka_topic` against Kafka's name rules and resolves its Iggy stream and topic. + /// + /// Creates nothing. `Metadata` owns topic creation. + /// + /// # Errors + /// + /// [`BridgeError::InvalidKafkaTopicName`] if the name fails Kafka's own rules. + /// [`BridgeError::Iggy`] if a mapped name is not a valid Iggy identifier. + pub fn topic_target(&self, kafka_topic: &str) -> Result { + validate_kafka_topic_name("kafka_topic", kafka_topic)?; + let (stream_name, topic_name) = self.config.topic_mapping.resolve(kafka_topic); + Ok(TopicTarget { + stream_id: Identifier::named(stream_name).map_err(BridgeError::Iggy)?, + topic_id: Identifier::named(topic_name).map_err(BridgeError::Iggy)?, + }) } /// Tears down the underlying Iggy client, including its background heartbeat task. @@ -174,9 +164,8 @@ impl IggyBridge { /// dials past. /// /// Takes `self` by value: `shutdown` is terminal (no reconnect is coming back from it), so - /// nothing legitimate is left to call on this bridge afterward. This does mean a bridge shared - /// via `Arc` cannot call this directly (`Arc::try_unwrap` first) - not a concern before - /// `#3535`/`#3536` wire an owning caller in. + /// nothing legitimate is left to call on this bridge afterward. A bridge shared via `Arc` + /// needs `Arc::try_unwrap` first. /// /// # Errors /// diff --git a/gateways/kafka/src/bridge/iggy_bridge/offsets.rs b/gateways/kafka/src/bridge/iggy_bridge/offsets.rs index b199f7f656..6a1a306507 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/offsets.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/offsets.rs @@ -17,11 +17,10 @@ //! Offset and high-watermark lookups. -use iggy::prelude::{Identifier, IggyError, TopicClient}; +use iggy::prelude::{IggyError, TopicClient}; use super::{IggyBridge, with_request_timeout}; use crate::bridge::error::BridgeError; -use crate::bridge::topic_map::validate_kafka_topic_name; impl IggyBridge { /// Returns the high watermark (one past the highest *committed* offset - see @@ -94,21 +93,19 @@ impl IggyBridge { kafka_topic: &str, partitions: &[u32], ) -> 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)?; - let details = with_request_timeout(self.client.get_topic(&stream_id, &topic_id)) - .await? - .ok_or_else(|| { - // A missing *stream* also makes get_topic return Ok(None) (responses.rs), so this - // reports TopicNameNotFound even when the stream is what's actually gone - both - // map to the same Kafka wire code either way, so only the log text is affected. - BridgeError::Iggy(IggyError::TopicNameNotFound( - topic_name.to_string(), - stream_name.to_string(), - )) - })?; + let target = self.topic_target(kafka_topic)?; + let details = + with_request_timeout(self.client.get_topic(&target.stream_id, &target.topic_id)) + .await? + .ok_or_else(|| { + // A missing *stream* also makes get_topic return Ok(None) (responses.rs), so this + // reports TopicNameNotFound even when the stream is what's actually gone - both + // map to the same Kafka wire code either way, so only the log text is affected. + BridgeError::Iggy(IggyError::TopicNameNotFound( + target.topic_id.to_string(), + target.stream_id.to_string(), + )) + })?; Ok(partitions .iter() @@ -122,10 +119,8 @@ impl IggyBridge { .binary_search_by_key(&partition, |p| p.id) .map(|index| &details.partitions[index]) .map_err(|_| BridgeError::PartitionOutOfRange { - // The Kafka-side name a caller (a future ListOffsets handler) actually - // asked about, not `topic_name` - a mapping override would otherwise - // quote the wrong (Iggy-side) name back at a Kafka client that never - // heard of it. + // The Kafka-side name the caller asked about. A mapping override + // would otherwise quote an Iggy name the Kafka client never heard of. topic: kafka_topic.to_string(), partition, partitions_count: details.partitions_count, diff --git a/gateways/kafka/src/bridge/iggy_bridge/produce.rs b/gateways/kafka/src/bridge/iggy_bridge/produce.rs index 4537322123..fe261887b1 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/produce.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/produce.rs @@ -17,5 +17,184 @@ //! Produce-side bridge calls. //! -//! Empty until the Produce handler lands. Separate from `fetch.rs` so the two never edit one -//! file. +//! Separate from `fetch.rs` so the two never edit one file. + +use std::future::Future; +use std::sync::Arc; +use std::time::Duration; + +use iggy::prelude::{IggyError, IggyMessage, MessageClient, Partitioning}; +use tokio::sync::{Semaphore, oneshot}; +use tokio::time::{Instant, timeout, timeout_at}; + +use super::{IggyBridge, TopicTarget}; +use crate::bridge::error::BridgeError; + +/// How long a send may hold the slot: the SDK read deadline (30 s) plus a reconnect (15 s). +const SEND_LIMIT: Duration = Duration::from_secs(45); + +/// Frame bytes besides the messages: request header, batch header and ids. +const SEND_FRAME_ROOM: u64 = 4 * 1024; + +impl IggyBridge { + /// Largest partition, in message bytes, that one Iggy send carries. + #[must_use] + pub const fn max_send_bytes(&self) -> u64 { + self.config.max_message_size.saturating_sub(SEND_FRAME_ROOM) + } + + /// Appends `messages` to `partition` of `target` and returns the base offset. + /// + /// `partition` passes through unchanged: both systems count from 0. The server rejects a + /// partition the topic lacks, so no round trip checks first. + /// + /// `Ok(None)`: committed, but the server named no offset (a duplicate request, or a journal + /// entry that is gone). Do not retry it. + /// + /// A stuck send holds the slot up to `SEND_LIMIT`. Other sends answer 7 meanwhile. + /// + /// # Errors + /// + /// [`BridgeError::Timeout`] if `deadline` passes. The send may have landed. + /// [`BridgeError::SendLost`] if the connection broke during the send. It may have landed. + /// [`BridgeError::Iggy`] if the stream, topic or partition is missing, or the send is too + /// large. + pub async fn send_records( + &self, + target: &TopicTarget, + partition: u32, + mut messages: Vec, + deadline: Instant, + ) -> Result, BridgeError> { + let client = Arc::clone(&self.client); + let (stream_id, topic_id) = (target.stream_id.clone(), target.topic_id.clone()); + let send = async move { + let partitioning = Partitioning::partition_id(partition); + client + .send_messages(&stream_id, &topic_id, &partitioning, &mut messages) + .await + }; + let response = send_in_slot(&self.send_slot, deadline, send).await?; + Ok(response + .confirmations + .iter() + .find(|confirmation| confirmation.partition_id == partition) + .map(|confirmation| confirmation.base_offset)) + } +} + +/// Runs `send` in a task that holds `slot` until the send ends, and waits for it until `deadline`. +/// +/// The SDK cannot cancel a send. A send that outlives its caller keeps the slot, so the next one +/// waits here instead of queueing its batch inside the SDK. +async fn send_in_slot( + slot: &Arc, + deadline: Instant, + send: impl Future> + Send + 'static, +) -> Result { + let permit = match timeout_at(deadline, Arc::clone(slot).acquire_owned()).await { + Ok(Ok(permit)) if Instant::now() < deadline => permit, + _ => return Err(BridgeError::Timeout), + }; + let (sender, receiver) = oneshot::channel(); + tokio::spawn(async move { + let result = match timeout(SEND_LIMIT, send).await { + Ok(result) => result.map_err(send_error), + Err(_elapsed) => Err(BridgeError::Timeout), + }; + drop(permit); + // The caller may have stopped waiting. + let _ = sender.send(result); + }); + match timeout_at(deadline, receiver).await { + Ok(Ok(result)) => result, + _ => Err(BridgeError::Timeout), + } +} + +/// The SDK does not replay a send after these, so it may have landed. +const fn send_error(error: IggyError) -> BridgeError { + match error { + IggyError::Disconnected + | IggyError::EmptyResponse + | IggyError::TcpError + | IggyError::StaleClient => BridgeError::SendLost(error), + error => BridgeError::Iggy(error), + } +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicBool, Ordering}; + + use super::*; + + const WAIT: Duration = Duration::from_millis(20); + + /// A send that records that it ran. + async fn flagged(started: Arc) -> Result { + started.store(true, Ordering::SeqCst); + tokio::task::yield_now().await; + Ok(2) + } + + #[tokio::test] + async fn given_a_send_past_its_deadline_when_the_next_one_waits_should_not_start_it() { + let slot = Arc::new(Semaphore::new(1)); + let (release, released) = oneshot::channel::<()>(); + let stuck = async move { + let _ = released.await; + Ok::(1) + }; + assert!(matches!( + send_in_slot(&slot, Instant::now() + WAIT, stuck).await, + Err(BridgeError::Timeout) + )); + + let started = Arc::new(AtomicBool::new(false)); + assert!( + matches!( + send_in_slot(&slot, Instant::now() + WAIT, flagged(Arc::clone(&started))).await, + Err(BridgeError::Timeout) + ), + "the stuck send still holds the slot" + ); + assert!( + !started.load(Ordering::SeqCst), + "a send that never got the slot never reaches the SDK" + ); + + release.send(()).unwrap(); + let after = send_in_slot( + &slot, + Instant::now() + WAIT * 10, + flagged(Arc::clone(&started)), + ) + .await; + assert_eq!(after.unwrap(), 2, "the slot frees when the stuck send ends"); + } + + #[tokio::test] + async fn given_a_passed_deadline_when_sending_should_not_start_the_send() { + let slot = Arc::new(Semaphore::new(1)); + let started = Arc::new(AtomicBool::new(false)); + + assert!(matches!( + send_in_slot(&slot, Instant::now(), flagged(Arc::clone(&started))).await, + Err(BridgeError::Timeout) + )); + tokio::task::yield_now().await; + assert!(!started.load(Ordering::SeqCst)); + } + + #[tokio::test] + async fn given_a_lost_connection_when_sending_should_say_the_send_may_have_landed() { + let slot = Arc::new(Semaphore::new(1)); + let lost = async { Err::(IggyError::Disconnected) }; + + assert!(matches!( + send_in_slot(&slot, Instant::now() + WAIT * 10, lost).await, + Err(BridgeError::SendLost(IggyError::Disconnected)) + )); + } +} diff --git a/gateways/kafka/src/bridge/iggy_bridge/topics.rs b/gateways/kafka/src/bridge/iggy_bridge/topics.rs index 395f6f27d2..f7fcfd5dba 100644 --- a/gateways/kafka/src/bridge/iggy_bridge/topics.rs +++ b/gateways/kafka/src/bridge/iggy_bridge/topics.rs @@ -46,9 +46,8 @@ impl IggyBridge { /// /// `partition_count` is `u32`, so it cannot carry Kafka's own `CreateTopics` sentinel /// (`num_partitions == -1`, KIP-464 "use the broker default" - `protocol/responses.rs` - /// already accepts that sentinel at the wire-validation layer). Resolving *what* the broker - /// 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. + /// already accepts that sentinel at the wire-validation layer). The broker default is for + /// the first caller with a real Kafka request (`Metadata` or `CreateTopics`) to decide. /// /// 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 @@ -56,10 +55,9 @@ impl IggyBridge { /// 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 - /// than once per Produce/Fetch - the cost belongs at the call site's discretion, not hidden - /// (and potentially made wrong) inside this method. + /// scenario, and a naive cache breaks that guarantee to save two round trips. A caller must + /// call it once per topic and remember that it did, not once per request. Produce does not + /// call it: it creates nothing. /// /// # Errors /// diff --git a/gateways/kafka/src/bridge/mod.rs b/gateways/kafka/src/bridge/mod.rs index dde4c5e9db..d837339239 100644 --- a/gateways/kafka/src/bridge/mod.rs +++ b/gateways/kafka/src/bridge/mod.rs @@ -18,15 +18,15 @@ //! Iggy SDK integration layer. //! //! Maps Kafka topics to Iggy streams/topics, exposes create-if-missing provisioning and -//! high-watermark lookups, and translates Iggy errors to Kafka wire error codes. Not yet wired -//! into the live Produce/Fetch dispatch path - that lands with `#3535`/`#3536`. +//! high-watermark lookups, and translates Iggy errors to Kafka wire error codes. `ListOffsets` +//! and Produce call it. Fetch does not yet (`#3536`). pub mod config; pub mod error; pub mod iggy_bridge; pub mod topic_map; -pub use config::IggyBridgeConfig; +pub use config::{DEFAULT_MAX_MESSAGE_SIZE, IggyBridgeConfig}; pub use error::BridgeError; -pub use iggy_bridge::IggyBridge; +pub use iggy_bridge::{IggyBridge, TopicTarget}; pub use topic_map::{TopicMapping, TopicOverride}; diff --git a/gateways/kafka/src/protocol/api.rs b/gateways/kafka/src/protocol/api.rs index 40c2a11886..52c1e249ee 100644 --- a/gateways/kafka/src/protocol/api.rs +++ b/gateways/kafka/src/protocol/api.rs @@ -18,8 +18,9 @@ use std::sync::Arc; use bytes::Bytes; - +use kafka_protocol::error::ResponseError; use kafka_protocol::messages::{SaslAuthenticateRequest, SaslHandshakeRequest}; +use tokio::sync::Semaphore; use crate::bridge::IggyBridge; use crate::error::Result; @@ -46,65 +47,74 @@ pub const DEFAULT_KAFKA_PORT: u16 = 9093; /// Generic catch-all. Not sent by any stub response today; the `bridge` module's error mapping /// uses it for an `IggyError` with no closer Kafka analogue. -pub const ERROR_UNKNOWN_SERVER_ERROR: i16 = -1; +pub const ERROR_UNKNOWN_SERVER_ERROR: i16 = ResponseError::UnknownServerError.code(); pub const ERROR_NONE: i16 = 0; -pub const ERROR_UNKNOWN_TOPIC_OR_PARTITION: i16 = 3; -/// Retriable; Produce stub uses this until the Iggy bridge persists records. -pub const ERROR_NOT_LEADER_OR_FOLLOWER: i16 = 6; -/// `bridge`'s mapping for `IggyError::TransientNotCommitted`: the request's outcome is genuinely -/// unknown (neither confirmed applied nor confirmed rejected). -/// -/// Retriable in real Kafka too (`TimeoutException extends RetriableException`; the Java producer's -/// `Sender.canRetry` treats it the same as `NOT_LEADER_OR_FOLLOWER`) - this is not chosen to make -/// clients stop retrying. It is chosen because it is the code a real broker sends for the same -/// unknown-outcome shape (an ack that timed out with no confirmation either way), and Kafka has no -/// dedicated "outcome unknown, retry could duplicate" code. The duplicate-write risk on retry is -/// real regardless of which retriable code is sent; it closes only once `#3535` has an idempotent -/// produce path, not by picking a different error code here. -pub const ERROR_REQUEST_TIMED_OUT: i16 = 7; +pub const ERROR_UNKNOWN_TOPIC_OR_PARTITION: i16 = ResponseError::UnknownTopicOrPartition.code(); +/// Retriable, nothing written. The Produce stub, and a partition refused because the request +/// budget ran out. +pub const ERROR_NOT_LEADER_OR_FOLLOWER: i16 = ResponseError::NotLeaderOrFollower.code(); +/// Outcome unknown: the write may have landed. Retriable, so a retry can duplicate it. +/// Idempotent produce (#3545) closes that. +pub const ERROR_REQUEST_TIMED_OUT: i16 = ResponseError::RequestTimedOut.code(); +/// Produce: a record, send or partition too large, even alone. Not retried as-is. Java splits a +/// multi-record batch and sends the halves. +pub const ERROR_MESSAGE_TOO_LARGE: i16 = ResponseError::MessageTooLarge.code(); /// `bridge`'s mapping for a Kafka-side topic name that fails Kafka's own naming rules. /// /// Empty, whitespace-padded, over 249 bytes, or outside `[A-Za-z0-9._-]`, checked before any Iggy /// call is made - a real Kafka client library validates topic names client-side and would never /// send one of these, but a raw/non-conformant client could. -pub const ERROR_INVALID_TOPIC_EXCEPTION: i16 = 17; +pub const ERROR_INVALID_TOPIC_EXCEPTION: i16 = ResponseError::InvalidTopicException.code(); +/// Produce: `acks` is not 0, 1 or -1. A conformant client never sends one, since `acks` comes +/// from validated configuration rather than from application input. +pub const ERROR_INVALID_REQUIRED_ACKS: i16 = ResponseError::InvalidRequiredAcks.code(); /// Closest fit for an Iggy permission/credential rejection in `bridge`'s error mapping. /// /// Still not `SASL_AUTHENTICATION_FAILED`, and now for a firmer reason than when this was written: /// a connection reaching the bridge has already completed its SASL exchange, so a credential or /// permission rejection from Iggy at that point is not an authentication failure and saying so /// would send an operator to the wrong hop. Not sent by any stub response today. -pub const ERROR_TOPIC_AUTHORIZATION_FAILED: i16 = 29; +pub const ERROR_TOPIC_AUTHORIZATION_FAILED: i16 = ResponseError::TopicAuthorizationFailed.code(); /// The mechanism a client asked for in `SaslHandshake` is not one this gateway enables. The /// response still carries the enabled mechanism list, which is what the client prints. -pub const ERROR_UNSUPPORTED_SASL_MECHANISM: i16 = 33; +pub const ERROR_UNSUPPORTED_SASL_MECHANISM: i16 = ResponseError::UnsupportedSaslMechanism.code(); /// A request arrived that is legal on the wire but not in this connection's SASL state: a token /// before a handshake, a normal request before authenticating, or a SASL request after. -pub const ERROR_ILLEGAL_SASL_STATE: i16 = 34; -pub const ERROR_UNSUPPORTED_VERSION: i16 = 35; +pub const ERROR_ILLEGAL_SASL_STATE: i16 = ResponseError::IllegalSaslState.code(); +pub const ERROR_UNSUPPORTED_VERSION: i16 = ResponseError::UnsupportedVersion.code(); /// `bridge`'s mapping for `BridgeError::PartitionCountMismatch`: the topic exists, just not with /// the requested partition count. /// /// Not [`ERROR_INVALID_PARTITIONS`] - `kafka-protocol`'s own error table (`error.rs`) defines that /// code's text as "Number of partitions is below 1", which is a different condition (a client /// asking for zero/negative partitions) than "this topic already exists with a different count". -pub const ERROR_TOPIC_ALREADY_EXISTS: i16 = 36; -pub const ERROR_INVALID_PARTITIONS: i16 = 37; -pub const ERROR_INVALID_REPLICATION_FACTOR: i16 = 38; +pub const ERROR_TOPIC_ALREADY_EXISTS: i16 = ResponseError::TopicAlreadyExists.code(); +pub const ERROR_INVALID_PARTITIONS: i16 = ResponseError::InvalidPartitions.code(); +pub const ERROR_INVALID_REPLICATION_FACTOR: i16 = ResponseError::InvalidReplicationFactor.code(); /// `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; +pub const ERROR_NOT_CONTROLLER: i16 = ResponseError::NotController.code(); +pub const ERROR_INVALID_REQUEST: i16 = ResponseError::InvalidRequest.code(); /// `ListOffsets`' code for a timestamp lookup the broker cannot perform. /// /// Real brokers send this for an old-message-format log; this bridge sends it for any timestamp /// 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(); /// A credential was refused. Deliberately undifferentiated: Iggy answers a bad password and an /// unknown user the same way, and distinguishing them here would reintroduce a user-enumeration /// oracle. -pub const ERROR_SASL_AUTHENTICATION_FAILED: i16 = 58; +pub const ERROR_SASL_AUTHENTICATION_FAILED: i16 = ResponseError::SaslAuthenticationFailed.code(); +/// Produce: zstd before v7. +pub const ERROR_UNSUPPORTED_COMPRESSION_TYPE: i16 = + ResponseError::UnsupportedCompressionType.code(); +/// Produce: a record or batch this gateway cannot map. +/// +/// Not `CORRUPT_MESSAGE` (2), whose text fits but which `kafka-protocol`'s table marks +/// retriable, so a client would resend a batch that can never decode. A client older than Kafka +/// 2.4 reads 87 as a generic server error, which is still terminal and still better than a loop. +pub const ERROR_INVALID_RECORD: i16 = ResponseError::InvalidRecord.code(); /// Result of handling one Kafka request body. #[derive(Debug)] @@ -202,8 +212,14 @@ pub struct GatewayState { /// Whether `SaslHandshake` and `SaslAuthenticate` are advertised and routed. Kept on the /// shared state so `ApiVersions` can answer without a widened handler signature. pub sasl_enabled: bool, + /// Produce requests that decode and send at once. Caps their memory. + pub(crate) produce_slots: Semaphore, } +/// Each holds one decoded partition at a time, so about 160 MB at the default 8 MiB frame. Sends +/// run one at a time anyway. +const PRODUCE_SLOTS: usize = 4; + impl GatewayState { #[must_use] pub const fn new( @@ -217,6 +233,7 @@ impl GatewayState { bridge, max_frame_size, sasl_enabled, + produce_slots: Semaphore::const_new(PRODUCE_SLOTS), } } diff --git a/gateways/kafka/src/protocol/handlers/produce.rs b/gateways/kafka/src/protocol/handlers/produce.rs index 944b72503f..9e6fe418ec 100644 --- a/gateways/kafka/src/protocol/handlers/produce.rs +++ b/gateways/kafka/src/protocol/handlers/produce.rs @@ -17,19 +17,33 @@ //! Produce (API key 0). +use std::time::Duration; + use bytes::Bytes; +use iggy::prelude::{IggyError, IggyMessage, Sizeable}; +use kafka_protocol::messages::produce_request::PartitionProduceData; use kafka_protocol::messages::produce_response::{PartitionProduceResponse, TopicProduceResponse}; use kafka_protocol::messages::{ProduceRequest, ProduceResponse}; +use kafka_protocol::protocol::StrBytes; +use tokio::runtime::{Handle, RuntimeFlavor}; +use crate::bridge::{BridgeError, IggyBridge, TopicTarget}; use crate::error::Result; use crate::protocol::api::{ - API_KEY_PRODUCE, ApiVersionRange, ERROR_NOT_LEADER_OR_FOLLOWER, ERROR_UNSUPPORTED_VERSION, - GatewayState, HandleOutcome, supported_max_version, + API_KEY_PRODUCE, ApiVersionRange, ERROR_INVALID_RECORD, ERROR_INVALID_REQUIRED_ACKS, + ERROR_MESSAGE_TOO_LARGE, ERROR_NONE, ERROR_NOT_LEADER_OR_FOLLOWER, ERROR_REQUEST_TIMED_OUT, + ERROR_UNKNOWN_SERVER_ERROR, ERROR_UNKNOWN_TOPIC_OR_PARTITION, + ERROR_UNSUPPORTED_COMPRESSION_TYPE, ERROR_UNSUPPORTED_VERSION, GatewayState, HandleOutcome, + supported_max_version, }; use crate::protocol::bounds_guard::validate_produce_shape; use crate::protocol::handlers::{ decode_guarded, encode_message, respond_or_close, unsupported_version_response, }; +use crate::records::{ + Allowance, DecompressionBudget, RecordCodecError, TimestampWindow, Zstd, decode_batch, + is_compressed, to_iggy, +}; pub const RANGE: ApiVersionRange = ApiVersionRange { api_key: API_KEY_PRODUCE, @@ -37,6 +51,43 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { max_version: 9, }; +/// The three `acks` values the protocol defines. Anything else is a malformed request. +const ACKS_NONE: i16 = 0; +const ACKS_LEADER: i16 = 1; +const ACKS_ALL: i16 = -1; + +/// First Produce version that may carry zstd batches. +const ZSTD_MIN_VERSION: i16 = 7; + +/// Kafka's "no offset here", used for `base_offset` on any failure, for a committed send the +/// server named no offset for, and for `log_start_offset` always. The real log start costs a +/// `get_topic` round trip per request and no producer reads it yet. +const UNKNOWN_OFFSET: i64 = -1; + +/// Ceiling on how long one request may spend writing, whatever the client asked for. +/// +/// One request is many Iggy calls, so without this a request naming many partitions has no +/// bound. Deliberately a clock and not a partition-count cap: a cap refuses a producer writing +/// to a wide topic, and a client set on hogging the shared bridge just sends more requests. +const MAX_REQUEST_DEADLINE: Duration = Duration::from_secs(20); + +/// Frame bytes per record slot. Caps one partition at 131,072 slots, about 40 MB, at the default +/// 8 MiB frame. +/// +/// `GatewayState` caps how many requests hold that at once. +const FRAME_BYTES_PER_RECORD: usize = 64; + +/// Partition allowances one request may decode, refused partitions included. +/// +/// 64 MiB at the default 8 MiB frame, so a 1 MB request can compress 64x before the rest of it +/// answers 6. +const REQUEST_ALLOWANCES: usize = 8; + +/// Blobs this large, or compressed ones, plan off the async worker. +const BLOCKING_PLAN_BYTES: usize = 64 * 1024; + +const RESPONSE_BASE_BYTES: usize = 512; + /// Produce is the only request the wire protocol allows to go unanswered /// (`acks=0`), so it gets its own path that may return [`HandleOutcome::NoResponse`]. /// @@ -45,10 +96,6 @@ pub const RANGE: ApiVersionRange = ApiVersionRange { /// spec-compliant client can legitimately send Produce v0-2 with `acks=0`. Rejecting those /// versions before reading `acks` would send an error response the client never expects, /// desyncing the next correlation id it reads. -#[expect( - clippy::unused_async, - reason = "the shared handler signature, kept until a handler awaits the bridge" -)] pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> HandleOutcome { // Above the encoder max there is no response parseable at the client's version, so close // rather than decode (same policy the other APIs apply). Fail-closed on a missing row @@ -62,48 +109,399 @@ pub async fn handle(state: &GatewayState, api_version: i16, body: Bytes) -> Hand // (still advertised as the min in ApiVersions per KAFKA-18659) can be neither decoded nor // encoded by the crate - there is no parseable response at these versions regardless of // body content. `acks` is always the first i16 on the wire there (`transactional_id` was - // added in v3), so it's peeked by hand: acks=0 must keep the connection open per the wire - // protocol's fire-and-forget rule even though no response can ever be encoded for it. + // added in v3), so it's peeked by hand. if api_version < 3 { let acks = match body.get(0..2) { Some(&[hi, lo]) => Some(i16::from_be_bytes([hi, lo])), _ => None, }; return match acks { - Some(0) | None => HandleOutcome::NoResponse, + // No bridge: nothing is stored at any version, so keep the connection. + Some(ACKS_NONE) | None if state.bridge.is_none() => HandleOutcome::NoResponse, + // With a bridge, v3+ stores records. Close so the client sees these were not. + Some(ACKS_NONE) | None => HandleOutcome::Close, Some(_) => unsupported_version_response(API_KEY_PRODUCE, api_version, |v| { encode_error_response(v, ERROR_UNSUPPORTED_VERSION) }), }; } - match decode_guarded::(api_version, body, |v, b| { + let request = match decode_guarded::(api_version, body, |v, b| { validate_produce_shape(v, b, state.max_frame_size) }) { - // acks=0 is fire-and-forget: the client isn't reading a response, so - // sending one desyncs the next correlation id it expects. - Ok(req) if req.acks == 0 => HandleOutcome::NoResponse, - // `api_version` is always in `[3, supported_max]` here: the `< 3` case returned above, - // and the `> max` case returned at the top of this function - so it is always within - // `SUPPORTED_RANGES`' Produce row and an `is_supported_version` re-check can never fail. - Ok(req) => respond_or_close(encode_response(api_version, &req), "Produce"), + Ok(request) => request, Err(error) => { - // `kafka_protocol` decodes the whole request in one shot; a failure anywhere gives - // no partial-field access, so `acks` is unknowable here (unlike the pre-migration - // field-by-field decoder, which could still know `acks` on a later-field failure). - // Responding risks desyncing an acks=0 fire-and-forget client's correlation stream, - // so every Produce decode failure now stays silent - a behavior change from the - // hand-rolled decoder, which answered with INVALID_REQUEST when `acks` was known and - // nonzero. - // debug!, not warn!: the body is attacker-controlled, not operator-actionable, and - // a client looping malformed bodies on one connection (never disconnected - this - // arm returns NoResponse) has no rate limit here. - tracing::debug!(%error, "failed to decode Produce request (no response)"); + // `acks` is unknown, so no reply is safe. Close, as Kafka does. + tracing::debug!(%error, "failed to decode Produce request, closing"); + return HandleOutcome::Close; + } + }; + + let Some(bridge) = state.bridge.as_deref() else { + return stub_outcome(api_version, &request); + }; + + // `acks=0` is legal, so a request failing this check is one the client waits on. + if !matches!(request.acks, ACKS_NONE | ACKS_LEADER | ACKS_ALL) { + tracing::debug!( + acks = request.acks, + "Produce request names an unknown acks value" + ); + return respond_or_close( + encode_uniform_response(api_version, &request, ERROR_INVALID_REQUIRED_ACKS), + "Produce", + ); + } + + // Before planning, not after: the client waits through decompression too. + let deadline = Deadline::new(request.timeout_ms); + let Ok(Ok(_slot)) = tokio::time::timeout_at(deadline.at, state.produce_slots.acquire()).await + else { + tracing::debug!("Produce deadline passed while waiting for a slot"); + return if request.acks == ACKS_NONE { + HandleOutcome::Close + } else { + respond_or_close( + encode_uniform_response(api_version, &request, ERROR_REQUEST_TIMED_OUT), + "Produce", + ) + }; + }; + let partition = Allowance { + bytes: state.max_frame_size, + records: state.max_frame_size / FRAME_BYTES_PER_RECORD, + }; + let budget = DecompressionBudget::new(partition, partition.times(REQUEST_ALLOWANCES)); + let zstd = if api_version >= ZSTD_MIN_VERSION { + Zstd::Allowed + } else { + Zstd::Refused + }; + let responses = write_request(bridge, &request, budget, zstd, deadline).await; + + // Kafka closes an `acks=0` connection on any error, so the client refreshes metadata. + if request.acks == ACKS_NONE { + let failed = responses + .iter() + .flat_map(|topic| &topic.partition_responses) + .any(|partition| partition.error_code != ERROR_NONE); + return if failed { + HandleOutcome::Close + } else { HandleOutcome::NoResponse + }; + } + respond_or_close(encode_written_response(api_version, responses), "Produce") +} + +/// No bridge: answer retriable so clients keep their data instead of trusting a discarded write. +fn stub_outcome(api_version: i16, request: &ProduceRequest) -> HandleOutcome { + // acks=0 is fire-and-forget: the client isn't reading a response, so + // sending one desyncs the next correlation id it expects. + if request.acks == ACKS_NONE { + return HandleOutcome::NoResponse; + } + respond_or_close(encode_response(api_version, request), "Produce") +} + +/// When a request stops writing. +#[derive(Clone, Copy)] +struct Deadline { + at: tokio::time::Instant, + /// This gateway's ceiling set it, not the client's `timeout_ms`. + ceiling: bool, +} + +impl Deadline { + fn new(timeout_ms: i32) -> Self { + let wait = request_timeout(timeout_ms); + Self { + at: tokio::time::Instant::now() + wait, + ceiling: wait == MAX_REQUEST_DEADLINE, + } + } + + fn passed(self) -> bool { + tokio::time::Instant::now() >= self.at + } +} + +/// Why a partition was not written. `message` reaches the client from Produce v8. +struct Refusal { + code: i16, + message: Option, +} + +impl From for Refusal { + fn from(code: i16) -> Self { + Self { + code, + message: None, + } + } +} + +impl From for Refusal { + fn from(error: RecordCodecError) -> Self { + Self { + code: record_error_code(&error), + message: Some(error.to_string()), + } + } +} + +/// Converts and sends one partition at a time, in request order, one Iggy send each. +/// +/// One budget for the whole request, so a small request cannot make this inflate without end. +/// Only one partition's messages live at a time. The budget is owned here and borrowed only in +/// sync calls: a borrow held across an await makes the connection task `!Send`. +/// +/// Sequential: the one `IggyClient` behind the bridge is lockstep, so concurrent sends would +/// just queue on its stream mutex (see the README's "Concurrency ceiling"). +async fn write_request( + bridge: &IggyBridge, + request: &ProduceRequest, + budget: DecompressionBudget, + zstd: Zstd, + deadline: Deadline, +) -> Vec { + let max_send = bridge.max_send_bytes(); + let mut topics = Vec::with_capacity(request.topic_data.len()); + for topic in &request.topic_data { + let name = topic.name.as_str(); + let target = bridge.topic_target(name); + let mut responses = Vec::with_capacity(topic.partition_data.len()); + for partition in &topic.partition_data { + let outcome = match &target { + // Skip the decode. The answer is 7 either way. + Ok(_) if deadline.passed() => Err(ERROR_REQUEST_TIMED_OUT.into()), + Ok(target) => { + let planned = plan(&budget, zstd, name, partition, max_send); + match planned { + Ok((id, messages)) => send_by(bridge, target, name, id, messages, deadline) + .await + .map_err(Refusal::from), + Err(refusal) => Err(refusal), + } + } + Err(error) => Err(refusal_code(name, error).into()), + }; + responses.push(partition_outcome(partition.index, outcome)); + // Planning is sync CPU work. Let other connections on this worker run. + tokio::task::yield_now().await; + } + topics.push( + TopicProduceResponse::default() + .with_name(topic.name.clone()) + .with_partition_responses(responses), + ); + } + topics +} + +/// [`plan_partition`], off the async worker for a large or compressed partition, so other +/// connections keep running. On a current-thread runtime it runs in place. +fn plan( + budget: &DecompressionBudget, + zstd: Zstd, + kafka_topic: &str, + partition: &PartitionProduceData, + max_send: u64, +) -> std::result::Result<(u32, Vec), Refusal> { + let run = || plan_partition(budget, zstd, kafka_topic, partition, max_send); + // A spent budget refuses before it reads a byte, so there is nothing to move off the worker. + let heavy = !budget.is_spent() + && partition + .records + .as_ref() + .is_some_and(|blob| blob.len() >= BLOCKING_PLAN_BYTES || is_compressed(blob)); + if heavy && Handle::current().runtime_flavor() == RuntimeFlavor::MultiThread { + tokio::task::block_in_place(run) + } else { + run() + } +} + +fn partition_outcome( + index: i32, + outcome: std::result::Result, Refusal>, +) -> PartitionProduceResponse { + match outcome { + // An offset past `i64::MAX` has no response field to fit in, so it reads as unknown + // rather than as a saturated number a client would take for a real position. + Ok(base_offset) => partition_response(index, ERROR_NONE).with_base_offset( + base_offset.map_or(UNKNOWN_OFFSET, |offset| { + i64::try_from(offset).unwrap_or(UNKNOWN_OFFSET) + }), + ), + Err(Refusal { code, message }) => { + partition_response(index, code).with_error_message(message.map(StrBytes::from_string)) + } + } +} + +/// One partition's batch in, its messages out. A batch that cannot be mapped is refused whole, +/// never half-stored. +fn plan_partition( + budget: &DecompressionBudget, + zstd: Zstd, + kafka_topic: &str, + partition: &PartitionProduceData, + max_send: u64, +) -> std::result::Result<(u32, Vec), Refusal> { + // The records are not what is wrong with a negative index, so not `INVALID_RECORD`. + let Ok(partition_id) = u32::try_from(partition.index) else { + return Err(ERROR_UNKNOWN_TOPIC_OR_PARTITION.into()); + }; + // Nothing to append. Answering success would name an offset for a write that never happened. + let Some(blob) = partition.records.as_ref() else { + return Err(ERROR_INVALID_RECORD.into()); + }; + let refuse = |error: RecordCodecError| { + tracing::debug!(%error, kafka_topic, partition_id, "refused a Produce record batch"); + Refusal::from(error) + }; + + // The clone is a refcount, and `decode_batch` drains what it is given. + let records = decode_batch(&mut blob.clone(), budget, zstd).map_err(refuse)?; + // An empty blob decodes to no batch rather than to an error, so this rules on it. + if records.is_empty() { + return Err(ERROR_INVALID_RECORD.into()); + } + + // One window for the whole batch, so it fits one send. + let window = TimestampWindow::of(&records); + let mut messages = Vec::with_capacity(records.len()); + let mut size = 0u64; + for record in &records { + let message = to_iggy(record, window).map_err(refuse)?; + size = size.saturating_add(message.get_size_bytes().as_bytes_u64()); + messages.push(message); + } + // Over this, Iggy drops the frame, and the client retries on 7 until it gives up. + if size > max_send { + tracing::debug!( + size, + kafka_topic, + partition_id, + "Produce partition too large for one Iggy send" + ); + return Err(ERROR_MESSAGE_TOO_LARGE.into()); + } + Ok((partition_id, messages)) +} + +/// The shorter of the client's `timeout_ms` and this gateway's ceiling. +/// +/// Honoring the client's budget narrows, not closes, the window for its retry to duplicate a +/// write still running here: this clock starts when the request is read, after any pipelined one. +/// A non-positive value means no preference, not "fail without trying". +fn request_timeout(timeout_ms: i32) -> Duration { + u64::try_from(timeout_ms) + .ok() + .filter(|ms| *ms > 0) + .map_or(MAX_REQUEST_DEADLINE, Duration::from_millis) + .min(MAX_REQUEST_DEADLINE) +} + +/// Sends one partition's messages, or answers 7 once the deadline has passed. +/// +/// 7 means nothing was written, or the send may still land and a retry duplicate it. +async fn send_by( + bridge: &IggyBridge, + target: &TopicTarget, + kafka_topic: &str, + partition_id: u32, + messages: Vec, + deadline: Deadline, +) -> std::result::Result, i16> { + match bridge + .send_records(target, partition_id, messages, deadline.at) + .await + { + Ok(base_offset) => Ok(base_offset), + Err(BridgeError::Timeout) => { + // The client picks `timeout_ms`, so its expiry is no signal to the operator. + if deadline.ceiling { + tracing::warn!( + kafka_topic, + partition_id, + "Produce ran past the gateway deadline" + ); + } else { + tracing::debug!( + kafka_topic, + partition_id, + "Produce ran past the client deadline" + ); + } + Err(ERROR_REQUEST_TIMED_OUT) + } + Err(error) => Err(refusal_code(kafka_topic, &error)), + } +} + +/// The Kafka code for a bridge refusal, logged at a level that matches who can fix it. +fn refusal_code(kafka_topic: &str, error: &BridgeError) -> i16 { + let code = error.to_kafka_error_code(); + if error.is_bridge_login_rejected() { + tracing::error!(%error, kafka_topic, "Iggy rejected the bridge's credentials"); + } else if matches!(code, ERROR_UNKNOWN_SERVER_ERROR) { + tracing::warn!(%error, kafka_topic, "Iggy refused a Produce batch"); + } else { + // Missing topic, bad name, lost connection: the client sees the code and retries. + tracing::debug!(%error, kafka_topic, "Iggy refused a Produce batch"); + } + code +} + +/// The Kafka code one record or batch failure answers with. +/// +/// Too big is 10, retry is 6, malformed is 87. Not `CORRUPT_MESSAGE` (2) for the malformed half, +/// which `kafka_protocol` marks retriable. Neither 10 nor 87 makes a client resend the same bytes. +/// Java splits a multi-record batch on 10 and sends the halves. +/// +/// No wildcard arm, so a variant added later fails the match instead of taking a default. +const fn record_error_code(error: &RecordCodecError) -> i16 { + match error { + RecordCodecError::Iggy(IggyError::TooBigMessagePayload | IggyError::TooBigUserHeaders) + | RecordCodecError::BudgetExceeded { .. } + | RecordCodecError::RecordBudgetExceeded { .. } + | RecordCodecError::EnvelopeTooLarge { .. } => ERROR_MESSAGE_TOO_LARGE, + // The request budget ran out. Nothing was written. + RecordCodecError::RequestBudgetSpent => ERROR_NOT_LEADER_OR_FOLLOWER, + // Transactional or control. 35 is a fatal code for these producers, so they stop instead + // of retrying forever. + RecordCodecError::TransactionalBatch | RecordCodecError::ControlBatch => { + ERROR_UNSUPPORTED_VERSION } + RecordCodecError::ZstdTooEarly => ERROR_UNSUPPORTED_COMPRESSION_TYPE, + // `IggyMessage::new`'s other refusal is an empty payload, which the codec never builds. + RecordCodecError::Iggy(_) + // Reachable from Produce. + | RecordCodecError::TimestampOutOfRange(_) + | RecordCodecError::Batch(_) + | RecordCodecError::SeveralBatches(_) + | RecordCodecError::BatchTrailingBytes(_) + | RecordCodecError::RecordCountTooLarge { .. } + | RecordCodecError::RecordCountMismatch { .. } + | RecordCodecError::HeaderCountTooLarge { .. } + | RecordCodecError::RecordTruncated + | RecordCodecError::RecordFieldLength(_) + | RecordCodecError::RepeatedHeaderName(_) + // Raised by `from_iggy` only, so Fetch is what actually answers for these. + | RecordCodecError::UserHeadersUnreadable(_) + | RecordCodecError::MappingVersion(_) + | RecordCodecError::ValueMarker(_) + | RecordCodecError::HeaderNameCollision(_) + | RecordCodecError::TimestampHeader(_) + | RecordCodecError::EnvelopeTruncated { .. } + | RecordCodecError::EnvelopeTrailingBytes(_) + | RecordCodecError::EnvelopeVersion(_) + | RecordCodecError::EnvelopeHeaderName => ERROR_INVALID_RECORD, } } -/// Well-formed Produce response with a single placeholder topic/partition. +/// Well-formed response with a single placeholder topic and partition. /// /// # Errors /// @@ -113,16 +511,21 @@ pub fn encode_error_response(version: i16, error_code: i16) -> Result { TopicProduceResponse::default() .with_partition_responses(vec![partition_response(0, error_code)]), ]); - encode_message(&resp, version, 512) + encode_message(&resp, version, RESPONSE_BASE_BYTES) } -/// Stub: discard the payload and return a retriable error so clients keep data locally until -/// the Iggy bridge lands (do not advertise silent success). +/// Stub: discard the payload and answer retriable, rather than advertise a silent success. /// /// # Errors /// /// Returns an error when `kafka_protocol` cannot encode the response at `version`. pub fn encode_response(version: i16, req: &ProduceRequest) -> Result { + encode_uniform_response(version, req, ERROR_NOT_LEADER_OR_FOLLOWER) +} + +/// One code for every partition named, for failures that belong to the request and not to a +/// single partition. +fn encode_uniform_response(version: i16, req: &ProduceRequest, error_code: i16) -> Result { let responses = req .topic_data .iter() @@ -133,18 +536,440 @@ pub fn encode_response(version: i16, req: &ProduceRequest) -> Result { topic .partition_data .iter() - .map(|p| partition_response(p.index, ERROR_NOT_LEADER_OR_FOLLOWER)) + .map(|p| partition_response(p.index, error_code)) .collect(), ) }) .collect(); + encode_written_response(version, responses) +} + +/// # Errors +/// +/// Returns an error when `kafka_protocol` cannot encode the response at `version`. +fn encode_written_response(version: i16, responses: Vec) -> Result { let resp = ProduceResponse::default().with_responses(responses); - encode_message(&resp, version, 512) + encode_message(&resp, version, RESPONSE_BASE_BYTES) } fn partition_response(index: i32, error_code: i16) -> PartitionProduceResponse { PartitionProduceResponse::default() .with_index(index) .with_error_code(error_code) - .with_log_start_offset(0) + .with_base_offset(UNKNOWN_OFFSET) + .with_log_start_offset(UNKNOWN_OFFSET) +} + +#[cfg(test)] +mod tests { + use super::*; + use bytes::BytesMut; + use kafka_protocol::indexmap::IndexMap; + use kafka_protocol::records::{ + Compression, NO_PARTITION_LEADER_EPOCH, NO_PRODUCER_EPOCH, NO_PRODUCER_ID, NO_SEQUENCE, + Record, RecordBatchEncoder, RecordEncodeOptions, TimestampType, + }; + + use crate::records::encode_batch; + + const CREATE_TIME: i64 = 1_700_000_000_123; + const TOPIC: &str = "orders"; + /// Well above anything these batches decompress to. + const TEST_BUDGET: usize = 1024 * 1024; + + fn record(offset: i64, value: &[u8]) -> Record { + Record { + transactional: false, + control: false, + delete_horizon: false, + partition_leader_epoch: NO_PARTITION_LEADER_EPOCH, + producer_id: NO_PRODUCER_ID, + producer_epoch: NO_PRODUCER_EPOCH, + timestamp_type: TimestampType::Creation, + offset, + sequence: NO_SEQUENCE, + timestamp: CREATE_TIME, + key: None, + value: Some(Bytes::copy_from_slice(value)), + headers: IndexMap::new(), + } + } + + fn compressed_batch(records: &[Record], compression: Compression) -> Bytes { + let mut buf = BytesMut::new(); + let options = RecordEncodeOptions { + version: 2, + compression, + }; + RecordBatchEncoder::encode(&mut buf, records.iter(), &options).expect("encode batch"); + buf.freeze() + } + + fn gzip_batch(records: &[Record]) -> Bytes { + compressed_batch(records, Compression::Gzip) + } + + fn entry(index: i32, records: Option) -> PartitionProduceData { + PartitionProduceData::default() + .with_index(index) + .with_records(records) + } + + fn plan_with( + budget: &DecompressionBudget, + zstd: Zstd, + entry: &PartitionProduceData, + ) -> std::result::Result<(u32, Vec), i16> { + plan_partition(budget, zstd, TOPIC, entry, u64::MAX).map_err(|refusal| refusal.code) + } + + fn planned(entry: &PartitionProduceData) -> std::result::Result<(u32, Vec), i16> { + plan_with( + &partition_budget(TEST_BUDGET, usize::MAX), + Zstd::Allowed, + entry, + ) + } + + /// A request allowance of one partition's. + fn partition_budget(bytes: usize, records: usize) -> DecompressionBudget { + let allowance = Allowance { bytes, records }; + DecompressionBudget::new(allowance, allowance) + } + + #[test] + fn given_a_record_batch_when_planned_should_convert_every_record_in_order() { + let batch = encode_batch(&mut [record(0, b"first"), record(1, b"second")]).unwrap(); + let (partition_id, messages) = planned(&entry(2, Some(batch))).expect("batch must convert"); + + assert_eq!( + partition_id, 2, + "the Kafka index passes through unconverted" + ); + assert_eq!(messages.len(), 2, "one record becomes one message"); + assert_eq!(messages[0].payload.as_ref(), b"first"); + assert_eq!(messages[1].payload.as_ref(), b"second"); + } + + #[test] + fn given_a_negative_partition_index_when_planned_should_answer_unknown_topic_or_partition() { + let batch = encode_batch(&mut [record(0, b"v")]).unwrap(); + assert_eq!( + planned(&entry(-1, Some(batch))).unwrap_err(), + ERROR_UNKNOWN_TOPIC_OR_PARTITION, + "the records are not what is wrong with the request" + ); + } + + #[test] + fn given_no_records_field_when_planned_should_answer_invalid_record() { + assert_eq!(planned(&entry(0, None)).unwrap_err(), ERROR_INVALID_RECORD); + } + + #[test] + fn given_an_empty_records_blob_when_planned_should_answer_invalid_record() { + // An empty blob reads as zero batches, not as a malformed one, so nothing below rejects it. + assert_eq!( + planned(&entry(0, Some(Bytes::new()))).unwrap_err(), + ERROR_INVALID_RECORD + ); + } + + #[test] + fn given_a_transactional_batch_when_planned_should_answer_unsupported_version() { + let mut records = [record(0, b"v")]; + records[0].transactional = true; + let batch = encode_batch(&mut records).unwrap(); + + assert_eq!( + planned(&entry(0, Some(batch))).unwrap_err(), + ERROR_UNSUPPORTED_VERSION, + "35 is fatal for a transactional producer" + ); + } + + #[test] + fn given_an_idempotent_batch_when_planned_should_convert_it() { + let mut records = [record(0, b"v")]; + records[0].producer_id = 7; + records[0].producer_epoch = 0; + records[0].sequence = 0; + let batch = compressed_batch(&records, Compression::None); + + let (_, messages) = + planned(&entry(0, Some(batch))).expect("a stock Java producer sends this batch"); + assert_eq!(messages.len(), 1); + assert_eq!(messages[0].payload.as_ref(), b"v"); + } + + #[test] + fn given_zstd_before_v7_when_planned_should_answer_unsupported_compression() { + let batch = compressed_batch(&[record(0, b"v")], Compression::Zstd); + let budget = partition_budget(TEST_BUDGET, usize::MAX); + + assert_eq!( + plan_with(&budget, Zstd::Refused, &entry(0, Some(batch.clone()))).unwrap_err(), + ERROR_UNSUPPORTED_COMPRESSION_TYPE + ); + assert!(plan_with(&budget, Zstd::Allowed, &entry(0, Some(batch))).is_ok()); + } + + #[test] + fn given_a_control_batch_when_planned_should_answer_unsupported_version() { + let mut records = [record(0, b"v")]; + records[0].control = true; + let batch = encode_batch(&mut records).unwrap(); + + assert_eq!( + planned(&entry(0, Some(batch))).unwrap_err(), + ERROR_UNSUPPORTED_VERSION, + "a stored message carries no control flag, so this one cannot be admitted" + ); + } + + #[test] + fn given_a_truncated_batch_when_planned_should_answer_invalid_record() { + let batch = encode_batch(&mut [record(0, b"value")]).unwrap(); + let truncated = batch.slice(..batch.len() - 3); + + assert_eq!( + planned(&entry(0, Some(truncated))).unwrap_err(), + ERROR_INVALID_RECORD, + "not CORRUPT_MESSAGE (2), which is retriable and would loop" + ); + } + + #[test] + fn given_a_batch_that_decompresses_past_the_budget_when_planned_should_answer_too_large() { + let batch = gzip_batch(&[record(0, &[b'a'; 4096])]); + let budget = partition_budget(16, usize::MAX); + + assert_eq!( + plan_with(&budget, Zstd::Allowed, &entry(0, Some(batch))).unwrap_err(), + ERROR_MESSAGE_TOO_LARGE + ); + } + + #[test] + fn given_more_records_than_a_partition_allows_when_planned_should_answer_too_large() { + let batch = encode_batch(&mut [record(0, b"a"), record(1, b"b")]).unwrap(); + let budget = partition_budget(TEST_BUDGET, 1); + + assert_eq!( + plan_with(&budget, Zstd::Allowed, &entry(0, Some(batch))).unwrap_err(), + ERROR_MESSAGE_TOO_LARGE, + "10, so Java splits the batch" + ); + } + + #[test] + fn given_an_earlier_partition_spent_the_budget_when_planned_should_answer_retriable() { + let budget = partition_budget(6144, usize::MAX); + let first = entry(0, Some(gzip_batch(&[record(0, &[b'a'; 4096])]))); + let second = entry(1, Some(gzip_batch(&[record(0, &[b'b'; 4096])]))); + + assert!(plan_with(&budget, Zstd::Allowed, &first).is_ok()); + assert_eq!( + plan_with(&budget, Zstd::Allowed, &second).unwrap_err(), + ERROR_NOT_LEADER_OR_FOLLOWER, + "fits alone, so not 10" + ); + } + + #[test] + fn given_an_entry_over_its_cap_after_another_when_planned_should_answer_too_large() { + // gzip writes 32 KiB at a time, so the second entry passes its cap in several writes. + let partition = Allowance { + bytes: 64 * 1024, + records: usize::MAX, + }; + let budget = DecompressionBudget::new(partition, partition.times(8)); + let first = entry(0, Some(gzip_batch(&[record(0, &vec![b'a'; 60 * 1024])]))); + let second = entry(1, Some(gzip_batch(&[record(0, &vec![b'b'; 200 * 1024])]))); + + assert!(plan_with(&budget, Zstd::Allowed, &first).is_ok()); + assert_eq!( + plan_with(&budget, Zstd::Allowed, &second).unwrap_err(), + ERROR_MESSAGE_TOO_LARGE, + "too large alone, so a retry cannot help" + ); + } + + #[test] + fn given_an_earlier_partition_spent_the_record_budget_when_planned_should_answer_retriable() { + let budget = partition_budget(TEST_BUDGET, 3); + let two = || encode_batch(&mut [record(0, b"a"), record(1, b"b")]).unwrap(); + + assert!(plan_with(&budget, Zstd::Allowed, &entry(0, Some(two()))).is_ok()); + assert_eq!( + plan_with(&budget, Zstd::Allowed, &entry(1, Some(two()))).unwrap_err(), + ERROR_NOT_LEADER_OR_FOLLOWER + ); + } + + #[test] + fn given_a_spent_request_when_a_later_partition_is_planned_should_answer_retry_undecoded() { + let budget = partition_budget(6144, usize::MAX); + let fill = |index| entry(index, Some(gzip_batch(&[record(0, &[b'a'; 4096])]))); + assert!(plan_with(&budget, Zstd::Allowed, &fill(0)).is_ok()); + assert_eq!( + plan_with(&budget, Zstd::Allowed, &fill(1)).unwrap_err(), + ERROR_NOT_LEADER_OR_FOLLOWER + ); + + // Malformed, so a decode would answer 87. + let garbage = entry(2, Some(Bytes::from_static(b"not a record batch"))); + assert_eq!( + plan_with(&budget, Zstd::Allowed, &garbage).unwrap_err(), + ERROR_NOT_LEADER_OR_FOLLOWER, + "nothing decodes once the request budget is gone" + ); + } + + #[test] + fn given_timestamps_past_one_send_when_planned_should_fit_one_send() { + let mut records = [record(0, b"a"), record(1, b"b"), record(2, b"c")]; + records[1].timestamp = CREATE_TIME + 72 * 60 * 1000; + records[2].timestamp = -1; + let (_, messages) = planned(&entry(0, Some(encode_batch(&mut records).unwrap()))).unwrap(); + + let stamps: Vec = messages + .iter() + .map(|message| message.header.origin_timestamp) + .collect(); + let low = stamps.iter().min().unwrap(); + let high = stamps.iter().max().unwrap(); + assert!( + u32::try_from(high - low).is_ok(), + "one Iggy send stores each as a u32 delta: {stamps:?}" + ); + } + + #[test] + fn given_a_client_timeout_when_bounded_should_take_whichever_is_shorter() { + assert_eq!(request_timeout(5_000), Duration::from_secs(5)); + assert_eq!( + request_timeout(30_000), + MAX_REQUEST_DEADLINE, + "a client asking for longer than this gateway allows gets the gateway's ceiling" + ); + assert_eq!(request_timeout(i32::MAX), MAX_REQUEST_DEADLINE); + } + + #[test] + fn given_no_client_timeout_when_bounded_should_still_attempt_the_write() { + // A value naming no budget means no preference, not "fail now". + assert_eq!(request_timeout(0), MAX_REQUEST_DEADLINE); + assert_eq!(request_timeout(-1), MAX_REQUEST_DEADLINE); + assert_eq!(request_timeout(i32::MIN), MAX_REQUEST_DEADLINE); + } + + #[test] + fn given_a_partition_response_when_built_should_name_no_offset_and_no_append_time() { + let response = partition_response(3, ERROR_INVALID_RECORD); + + assert_eq!(response.index, 3); + assert_eq!(response.error_code, ERROR_INVALID_RECORD); + assert_eq!(response.base_offset, UNKNOWN_OFFSET); + assert_eq!(response.log_start_offset, UNKNOWN_OFFSET); + assert_eq!( + response.log_append_time_ms, UNKNOWN_OFFSET, + "CreateTime, which is the only timestamp type the mapping stores" + ); + } + + #[test] + fn given_a_size_failure_when_mapped_should_answer_message_too_large() { + assert_eq!( + record_error_code(&RecordCodecError::BudgetExceeded { size: 2, limit: 1 }), + ERROR_MESSAGE_TOO_LARGE + ); + assert_eq!( + record_error_code(&RecordCodecError::EnvelopeTooLarge { size: 1 }), + ERROR_MESSAGE_TOO_LARGE + ); + assert_eq!( + record_error_code(&RecordCodecError::Iggy(IggyError::TooBigMessagePayload)), + ERROR_MESSAGE_TOO_LARGE + ); + assert_eq!( + record_error_code(&RecordCodecError::Iggy(IggyError::TooBigUserHeaders)), + ERROR_MESSAGE_TOO_LARGE + ); + } + + #[test] + fn given_a_malformed_record_when_mapped_should_answer_a_terminal_code() { + for error in [ + RecordCodecError::Batch("bad".to_string()), + RecordCodecError::RecordTruncated, + RecordCodecError::RecordFieldLength(-1), + RecordCodecError::TimestampOutOfRange(i64::MIN), + RecordCodecError::RecordCountTooLarge { + count: i32::MAX, + limit: 1, + }, + RecordCodecError::RecordCountMismatch { + declared: 1, + walked: 2, + }, + RecordCodecError::SeveralBatches(2), + RecordCodecError::BatchTrailingBytes(1), + RecordCodecError::HeaderCountTooLarge { + count: i32::MAX, + limit: 1, + }, + RecordCodecError::Iggy(IggyError::InvalidMessagePayloadLength), + ] { + assert_eq!( + record_error_code(&error), + ERROR_INVALID_RECORD, + "{error} must not answer a retriable code" + ); + } + } + + #[test] + fn given_a_refused_batch_kind_when_mapped_should_answer_unsupported_version() { + for error in [ + RecordCodecError::TransactionalBatch, + RecordCodecError::ControlBatch, + ] { + assert_eq!(record_error_code(&error), ERROR_UNSUPPORTED_VERSION); + } + } + + #[test] + fn given_a_spent_request_budget_when_mapped_should_answer_retriable() { + assert_eq!( + record_error_code(&RecordCodecError::RequestBudgetSpent), + ERROR_NOT_LEADER_OR_FOLLOWER + ); + } + + #[test] + fn given_a_codec_refusal_when_answered_should_carry_its_reason() { + let refusal = Refusal::from(RecordCodecError::RepeatedHeaderName("a".to_string())); + let response = partition_outcome(0, Err(refusal)); + + assert_eq!(response.error_code, ERROR_INVALID_RECORD); + assert_eq!( + response.error_message.as_deref(), + Some("record repeats header name a"), + "Java shows this text instead of a generic one" + ); + assert_eq!( + partition_outcome(0, Err(Refusal::from(ERROR_REQUEST_TIMED_OUT))).error_message, + None + ); + } + + #[test] + fn given_a_partition_past_the_send_cap_when_planned_should_answer_too_large() { + let batch = encode_batch(&mut [record(0, b"value")]).unwrap(); + let budget = partition_budget(TEST_BUDGET, usize::MAX); + let refusal = + plan_partition(&budget, Zstd::Allowed, TOPIC, &entry(0, Some(batch)), 8).unwrap_err(); + assert_eq!(refusal.code, ERROR_MESSAGE_TOO_LARGE); + } } diff --git a/gateways/kafka/src/records.rs b/gateways/kafka/src/records.rs index 950c370fe8..53640b7552 100644 --- a/gateways/kafka/src/records.rs +++ b/gateways/kafka/src/records.rs @@ -23,6 +23,7 @@ use std::cell::Cell; use std::collections::BTreeMap; use std::io::{self, Write}; +use std::sync::OnceLock; use bytes::{Buf, BufMut, Bytes, BytesMut}; use iggy::prelude::{HeaderKey, HeaderValue, IggyError, IggyMessage, MAX_PAYLOAD_SIZE}; @@ -47,8 +48,8 @@ pub const MAPPING_VERSION: u8 = 1; pub const KEY_HEADER: &str = "kafka.key"; /// Iggy header naming which of null or empty a placeholder payload stands for. pub const VALUE_MARKER_HEADER: &str = "kafka.value"; -/// Iggy header marking a record stamped at the Unix epoch. -pub const TIMESTAMP_MARKER_HEADER: &str = "kafka.ts"; +/// Iggy header holding the Kafka timestamp, `Int64` ms, when `origin_timestamp` cannot. +pub const TIMESTAMP_HEADER: &str = "kafka.ts"; /// Prefix every Kafka record header name is stored under. pub const HEADER_PREFIX: &str = "kafka.h."; /// Iggy header whose one-byte value is the envelope byte layout version. @@ -60,6 +61,8 @@ pub const ENVELOPE_VERSION: u8 = 1; const NO_TIMESTAMP: i64 = -1; /// The one Kafka timestamp an `origin_timestamp` of zero cannot be told apart from. const EPOCH_TIMESTAMP: i64 = 0; +/// Widest `origin_timestamp` span one Iggy send holds (`MAX_TIMESTAMP_DELTA_MICROS`). +const MAX_SEND_SPAN_MICROS: u64 = u32::MAX as u64; /// Stored in place of a null or empty value, discarded on the way back. const PLACEHOLDER: &[u8] = &[0x00]; /// Iggy caps one header name and one header value at this many bytes. @@ -67,7 +70,6 @@ const MAX_FIELD: usize = 255; const MARKER_NULL: &[u8] = b"null"; const MARKER_EMPTY: &[u8] = b"empty"; -const MARKER_EPOCH: &[u8] = b"epoch"; const FLAG_KEY: u8 = 0b01; const FLAG_VALUE: u8 = 0b10; @@ -119,8 +121,8 @@ pub enum RecordCodecError { ValueMarker(Bytes), #[error("two stored header keys both name {0}")] HeaderNameCollision(String), - #[error("timestamp marker {0:?} is not epoch")] - TimestampMarker(Bytes), + #[error("timestamp header {0:?} is not a Kafka timestamp")] + TimestampHeader(Bytes), #[error("envelope for this record is {size} bytes, over Iggy's {MAX_PAYLOAD_SIZE} byte limit")] EnvelopeTooLarge { size: usize }, #[error("envelope is truncated: needed {needed} bytes, {remaining} remain")] @@ -133,23 +135,39 @@ pub enum RecordCodecError { EnvelopeHeaderName, #[error("record batch is malformed: {0}")] Batch(String), + #[error("{0} record batches in one partition, Kafka allows 1")] + SeveralBatches(usize), + #[error("{0} bytes follow the record batch")] + BatchTrailingBytes(usize), #[error("batch declares {count} records, and {limit} bytes can hold fewer")] RecordCountTooLarge { count: i32, limit: usize }, + #[error("batch declares {declared} records and holds {walked}")] + RecordCountMismatch { declared: usize, walked: usize }, #[error("record declares {count} headers, and {limit} bytes can hold fewer")] HeaderCountTooLarge { count: i32, limit: usize }, #[error("record batch ends inside a record")] RecordTruncated, #[error("record field declares {0} bytes")] RecordFieldLength(i32), - #[error("{0} batches are out of scope")] - UnsupportedBatch(&'static str), - #[error("decompressed {produced} bytes with {remaining} left in the request budget")] - BudgetExceeded { produced: usize, remaining: usize }, + #[error("transactional batches are not supported")] + TransactionalBatch, + #[error("control batches are not supported")] + ControlBatch, + #[error("partition decompresses to at least {size} bytes, over its {limit} byte cap")] + BudgetExceeded { size: usize, limit: usize }, + #[error("partition needs {count} record slots, over its {limit} slot cap")] + RecordBudgetExceeded { count: usize, limit: usize }, + #[error("the request budget ran out, retry")] + RequestBudgetSpent, + #[error("zstd batch in a request older than Produce v7")] + ZstdTooEarly, + #[error("record repeats header name {0}")] + RepeatedHeaderName(String), } type Result = std::result::Result; -/// Encodes one Kafka record as one Iggy message. +/// Encodes one Kafka record as one Iggy message, for a send whose timestamps fit `window`. /// /// Takes the native path when Iggy can hold every field, and the envelope otherwise. A caller /// cannot tell which from the return value, which is the point: `from_iggy` reverses both. @@ -158,12 +176,16 @@ type Result = std::result::Result; /// /// Returns an error when the timestamp does not fit, when the envelope would exceed /// `MAX_PAYLOAD_SIZE`, or when Iggy rejects the message for a reason the envelope does not fix. -pub fn to_iggy(record: &Record) -> Result { +pub fn to_iggy(record: &Record, window: TimestampWindow) -> Result { + let stamp = window.stamp(record.timestamp)?; + if let Some(value) = plain_value(record, stamp) { + return plain_message(value.clone(), stamp.origin); + } if needs_envelope(record) { - return envelope_message(record); + return envelope_message(record, stamp); } let (payload, marker) = split_value(record.value.as_ref()); - let mut headers = gateway_headers(record.timestamp); + let mut headers = gateway_headers(stamp.header); if let Some(marker) = marker { headers.insert(header_key(VALUE_MARKER_HEADER), header_value(marker)); } @@ -183,7 +205,46 @@ pub fn to_iggy(record: &Record) -> Result { // The only limit left is the 100 KB budget over all headers together, which no per-field // check can see. Let the constructor rule on it rather than duplicating its arithmetic. - build(payload, headers, record.timestamp)?.map_or_else(|| envelope_message(record), Ok) + build(payload, headers, stamp.origin)?.map_or_else(|| envelope_message(record, stamp), Ok) +} + +/// The `origin_timestamp` range one Iggy send holds: `u32::MAX` µs, about 71.6 min. +/// +/// Starts at the batch's earliest real timestamp. A timestamp outside it is clamped into it, and +/// `kafka.ts` keeps the real one, so a Kafka batch always fits one send. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct TimestampWindow { + start: u64, +} + +impl TimestampWindow { + /// The window for `records`, the batch one send carries. `-1` and the epoch do not move it. + #[must_use] + pub fn of(records: &[Record]) -> Self { + let start = records + .iter() + .filter(|record| record.timestamp > EPOCH_TIMESTAMP) + .filter_map(|record| timestamp_in(record.timestamp).ok()) + .min() + .unwrap_or(0); + Self { start } + } + + fn stamp(self, millis: i64) -> Result { + let native = timestamp_in(millis)?; + let origin = native.clamp(self.start, self.start.saturating_add(MAX_SEND_SPAN_MICROS)); + // A zero origin reads back as "no timestamp", so the epoch needs the header unclamped too. + let header = (origin != native || millis == EPOCH_TIMESTAMP).then_some(millis); + Ok(Stamp { origin, header }) + } +} + +/// Where one record's timestamp is stored. +#[derive(Clone, Copy)] +struct Stamp { + origin: u64, + /// The `kafka.ts` value, when `origin` does not read back as the record's timestamp. + header: Option, } /// Decodes one Iggy message as one Kafka record at `offset`. @@ -220,11 +281,12 @@ pub fn from_iggy(message: &IggyMessage, offset: i64) -> Result { Some(envelope) => decode_envelope(envelope.as_bytes(), &message.payload)?, None => gateway_fields(message, &stored)?, }; - let timestamp = match stored.get(&header_key(TIMESTAMP_MARKER_HEADER)) { + let timestamp = match stored.get(&header_key(TIMESTAMP_HEADER)) { None => timestamp_out(message), - Some(marker) => match marker.as_bytes() { - MARKER_EPOCH => EPOCH_TIMESTAMP, - _ => return Err(RecordCodecError::TimestampMarker(marker.value())), + Some(header) => match header.as_int64() { + Ok(NO_TIMESTAMP) => server_timestamp(message), + Ok(millis) if millis >= EPOCH_TIMESTAMP => millis, + _ => return Err(RecordCodecError::TimestampHeader(header.value())), }, }; Ok(record(key, value, headers, offset, timestamp)) @@ -258,22 +320,22 @@ fn timestamp_in(millis: i64) -> Result { /// Zero means the producer sent no timestamp, so the server-assigned one stands in. /// -/// A record stamped at the epoch stores that same zero, and `from_iggy` reads the `kafka.ts` -/// marker before it calls this, because Iggy has no other way to hold the difference. +/// `from_iggy` reads `kafka.ts` first: an epoch or clamped record stores a value that does not +/// read back as its own. fn timestamp_out(message: &IggyMessage) -> i64 { - let micros = if message.header.origin_timestamp == 0 { - message.header.timestamp - } else { - message.header.origin_timestamp - }; - i64::try_from(micros / 1000).unwrap_or(NO_TIMESTAMP) + match message.header.origin_timestamp { + 0 => server_timestamp(message), + micros => i64::try_from(micros / 1000).unwrap_or(NO_TIMESTAMP), + } +} + +fn server_timestamp(message: &IggyMessage) -> i64 { + i64::try_from(message.header.timestamp / 1000).unwrap_or(NO_TIMESTAMP) } /// Whether any field of `record` is one Iggy refuses to hold natively. /// -/// A repeated header name is on the list in `BRIDGE_MAPPING.md` and is absent here, because -/// `kafka_protocol` decodes headers into an `IndexMap` (`records.rs:919`). A repeat overwrites -/// its earlier entry before this code runs, so the case cannot be observed. +/// A repeated header name never reaches here: `scan_records` refuses it. fn needs_envelope(record: &Record) -> bool { let key_unholdable = record .key @@ -290,6 +352,37 @@ fn needs_envelope(record: &Record) -> bool { }) } +/// The value, when `kafka.v` is the only header the record needs. +fn plain_value(record: &Record, stamp: Stamp) -> Option<&Bytes> { + if record.key.is_some() || !record.headers.is_empty() || stamp.header.is_some() { + return None; + } + record.value.as_ref().filter(|value| !value.is_empty()) +} + +/// Reuses one encoded `kafka.v` header block, so the common record skips a map and an encode. +/// +/// Static bytes: a clone touches no refcount shared across workers. +fn plain_message(value: Bytes, origin: u64) -> Result { + static VERSION_ONLY: OnceLock<&'static [u8]> = OnceLock::new(); + let headers = *VERSION_ONLY.get_or_init(|| { + let block = IggyMessage::builder() + .payload(Bytes::from_static(PLACEHOLDER)) + .user_headers(gateway_headers(None)) + .build() + .ok() + .and_then(|message| message.user_headers) + .unwrap_or_else(|| unreachable!("the kafka.v header always fits")); + Box::leak(block.to_vec().into_boxed_slice()) + }); + let mut message = IggyMessage::builder().payload(value).build()?; + message.header.user_headers_length = + u32::try_from(headers.len()).unwrap_or_else(|_| unreachable!("the kafka.v header is tiny")); + message.user_headers = Some(Bytes::from_static(headers)); + message.header.origin_timestamp = origin; + Ok(message) +} + /// Payload to store, and the marker naming what the original value was when it is not the payload. fn split_value(value: Option<&Bytes>) -> (Bytes, Option<&'static [u8]>) { match value { @@ -299,18 +392,13 @@ fn split_value(value: Option<&Bytes>) -> (Bytes, Option<&'static [u8]>) { } } -/// The headers every gateway-written message carries, whichever path it takes. -/// -/// Iggy reads an `origin_timestamp` of zero as no timestamp at all, so a record that really was -/// stamped at the epoch needs a marker to hold the difference. -fn gateway_headers(timestamp: i64) -> BTreeMap { +/// The headers every gateway-written message carries, whichever path it takes, plus `kafka.ts` +/// when `timestamp` holds one. +fn gateway_headers(timestamp: Option) -> BTreeMap { let mut headers = BTreeMap::new(); headers.insert(header_key(VERSION_HEADER), header_value(&[MAPPING_VERSION])); - if timestamp == EPOCH_TIMESTAMP { - headers.insert( - header_key(TIMESTAMP_MARKER_HEADER), - header_value(MARKER_EPOCH), - ); + if let Some(millis) = timestamp { + headers.insert(header_key(TIMESTAMP_HEADER), HeaderValue::from(millis)); } headers } @@ -319,7 +407,7 @@ fn gateway_headers(timestamp: i64) -> BTreeMap { fn build( payload: Bytes, headers: BTreeMap, - timestamp: i64, + origin: u64, ) -> Result> { let mut message = match IggyMessage::builder() .payload(payload) @@ -330,11 +418,11 @@ fn build( Err(IggyError::TooBigUserHeaders) => return Ok(None), Err(error) => return Err(error.into()), }; - message.header.origin_timestamp = timestamp_in(timestamp)?; + message.header.origin_timestamp = origin; Ok(Some(message)) } -fn envelope_message(record: &Record) -> Result { +fn envelope_message(record: &Record, stamp: Stamp) -> Result { // The envelope moves the key and the headers into the payload, so a record whose value alone // clears `MAX_PAYLOAD_SIZE` can be one the fallback cannot hold. Say so before spending the // allocation, since the native path has already been ruled out and nothing else is left. @@ -343,12 +431,12 @@ fn envelope_message(record: &Record) -> Result { return Err(RecordCodecError::EnvelopeTooLarge { size }); } - let mut headers = gateway_headers(record.timestamp); + let mut headers = gateway_headers(stamp.header); headers.insert( header_key(ENVELOPE_HEADER), header_value(&[ENVELOPE_VERSION]), ); - build(encode_envelope(record, size), headers, record.timestamp)? + build(encode_envelope(record, size), headers, stamp.origin)? .ok_or(IggyError::TooBigUserHeaders) .map_err(Into::into) } @@ -580,49 +668,121 @@ fn header_value(value: &[u8]) -> HeaderValue { HeaderValue::try_from(value).unwrap_or_else(|_| unreachable!("header value is out of range")) } -/// What one Produce request may decompress to, in total. +/// Decompressed bytes and record slots, the two things a decode spends. +/// +/// A record slot costs a `Record` and an `IggyMessage` in memory. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct Allowance { + pub bytes: usize, + pub records: usize, +} + +impl Allowance { + #[must_use] + pub const fn times(self, factor: usize) -> Self { + Self { + bytes: self.bytes.saturating_mul(factor), + records: self.records.saturating_mul(factor), + } + } +} + +/// What one Produce request may decode. /// -/// Charged across every batch in the request, because one frame carries many batches and a cap -/// applied to each on its own admits as many multiples of it as the frame holds entries. +/// - One partition may take `partition`. Past it, it is too large alone (10). +/// - All partitions together may take `request`. It counts work done, refused partitions too, so +/// no refusal lets the same inflate run again for free. Once it runs out, the partition that +/// ran it out and every later one answer 6, undecoded. A retry that comes first in its request +/// gets the full allowance, so a partition too large alone still ends at 10. /// -/// The budget bounds the peak, not just the total. Every decompressor here writes through -/// `BudgetedWriter`, which refuses the write that would pass the budget, so an over-budget frame -/// never reaches its full decompressed size in memory. +/// Writes go through `BudgetedWriter`, so no partition inflates past either one in memory. pub struct DecompressionBudget { - remaining: Cell, + partition: Allowance, + /// What the request has left. + bytes_left: Cell, + records_left: Cell, + /// What the current partition took. + entry_bytes: Cell, + entry_records: Cell, + spent: Cell, /// Why this module refused a batch, when it did. Everything this module reports from inside /// the decoder's decompression hook leaves as an `io::Error` or an `anyhow::Error`, which the - /// decoder stringifies, so the typed reason is parked here and taken by `decode_batches`. + /// decoder stringifies, so the typed reason is parked here and taken by `decode_batch`. reason: Cell>, } impl DecompressionBudget { #[must_use] - pub const fn new(bytes: usize) -> Self { + pub const fn new(partition: Allowance, request: Allowance) -> Self { Self { - remaining: Cell::new(bytes), + partition, + bytes_left: Cell::new(request.bytes), + records_left: Cell::new(request.records), + entry_bytes: Cell::new(0), + entry_records: Cell::new(0), + spent: Cell::new(false), reason: Cell::new(None), } } - fn charge(&self, produced: usize) -> io::Result<()> { - let remaining = self.remaining.get(); - if produced > remaining { - self.refuse(RecordCodecError::BudgetExceeded { - produced, - remaining, + /// Whether the request allowance ran out, so nothing more decodes. + #[must_use] + pub const fn is_spent(&self) -> bool { + self.spent.get() + } + + fn start_entry(&self) -> Result<()> { + if self.is_spent() { + return Err(RecordCodecError::RequestBudgetSpent); + } + self.entry_bytes.set(0); + self.entry_records.set(0); + Ok(()) + } + + /// Takes `len` decoded bytes for the current partition, or refuses. + fn charge(&self, len: usize) -> io::Result<()> { + let entry = self.entry_bytes.get().saturating_add(len); + if entry > self.partition.bytes { + return Err(self.refuse(RecordCodecError::BudgetExceeded { + size: entry, + limit: self.partition.bytes, + })); + } + let Some(left) = self.bytes_left.get().checked_sub(len) else { + return Err(self.refuse(self.spend())); + }; + self.bytes_left.set(left); + self.entry_bytes.set(entry); + Ok(()) + } + + fn charge_records(&self, count: usize) -> Result<()> { + let entry = self.entry_records.get().saturating_add(count); + if entry > self.partition.records { + return Err(RecordCodecError::RecordBudgetExceeded { + count: entry, + limit: self.partition.records, }); - return Err(io::Error::other(format!( - "decompressed {produced} bytes with {remaining} left in the budget" - ))); } - self.remaining.set(remaining - produced); + let Some(left) = self.records_left.get().checked_sub(count) else { + return Err(self.spend()); + }; + self.records_left.set(left); + self.entry_records.set(entry); Ok(()) } - /// Parks the typed reason for the error about to be raised through the hook. - fn refuse(&self, reason: RecordCodecError) { + fn spend(&self) -> RecordCodecError { + self.spent.set(true); + RecordCodecError::RequestBudgetSpent + } + + /// Parks the typed reason and returns the error the decoder stringifies. + fn refuse(&self, reason: RecordCodecError) -> io::Error { + let error = io::Error::other(reason.to_string()); self.reason.set(Some(reason)); + error } /// The typed reason for a decoder error, when this module is what caused it. @@ -651,6 +811,10 @@ impl<'a> BudgetedWriter<'a> { budget, } } + + fn into_output(self) -> Bytes { + self.out.freeze() + } } impl Write for BudgetedWriter<'_> { @@ -665,78 +829,106 @@ impl Write for BudgetedWriter<'_> { } } -/// Decodes every record batch a Produce partition entry carries. -/// -/// A partition's `records` field is one blob that holds one or more batches back to back, so -/// this drains `buf` rather than reading a single batch. +/// Whether a partition may hold zstd. Produce allows it from v7. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Zstd { + Allowed, + Refused, +} + +/// Whether the first batch in `blob` is compressed. Byte 22 is the low byte of its attributes. +#[must_use] +pub fn is_compressed(blob: &[u8]) -> bool { + blob.get(22) + .is_some_and(|attributes| attributes & 0b111 != 0) +} + +/// Decodes the one record batch a Produce partition carries. /// /// # Errors /// -/// Returns an error when a batch is malformed, when it declares more records than the request -/// can hold, when it is a control or transactional batch, or when the request decompresses to -/// more than `budget` allows. -pub fn decode_batches(buf: &mut Bytes, budget: &DecompressionBudget) -> Result> { - preflight(buf, budget)?; - - let mut records = Vec::new(); - while buf.has_remaining() { - let set = RecordBatchDecoder::decode_with_custom_compression( - buf, - Some(|compressed: &mut Bytes, compression| decompress(compressed, compression, budget)), - ) - .map_err(|error| budget.reason(&error.to_string()))?; - records.extend(set.records); +/// Returns an error when the blob holds more than one batch or bytes after it, when the batch is +/// malformed or holds a record count other than the one it declares, when it is a control or +/// transactional batch, when it is zstd and `zstd` refuses it, or when it passes `budget` or the +/// budget is already spent. +pub fn decode_batch( + buf: &mut Bytes, + budget: &DecompressionBudget, + zstd: Zstd, +) -> Result> { + budget.start_entry()?; + preflight(buf, budget, zstd)?; + if !buf.has_remaining() { + return Ok(Vec::new()); + } + + let walked = Cell::new(0); + let set = RecordBatchDecoder::decode_with_custom_compression( + buf, + Some( + |compressed: &mut Bytes, compression| -> anyhow::Result { + let (records, count) = decompress(compressed, compression, budget)?; + walked.set(count); + Ok(records) + }, + ), + ) + .map_err(|error| budget.reason(&error.to_string()))?; + // The decoder stops at the declared count and drops the rest. + if set.records.len() != walked.get() { + return Err(RecordCodecError::RecordCountMismatch { + declared: set.records.len(), + walked: walked.get(), + }); } - Ok(records) + Ok(set.records) } -/// Reads the batch headers before anything decodes a record. -/// -/// `RecordBatchDecoder` reserves from the batch header's record count before it reads the first -/// record (`kafka-protocol-0.18.0/src/records.rs:517`), and that count is checked for sign only. -/// A 61-byte batch can therefore ask for `i32::MAX` records. The counts are charged against what -/// the blob can produce, together rather than one batch at a time, because a blob holds many -/// batches and each reserve lands in the same `Vec`. Reading the headers costs one `Bytes` clone, -/// which is a refcount. +/// Checks the batch header before anything decodes a record. /// -/// What the blob can produce is its own length, and the decompression budget on top only when -/// something in it is compressed. An uncompressed batch's records are already in the blob, so -/// granting it the budget as well would let a 70-byte batch declare a million records. -/// -/// Control and transactional batches are refused in the same pass. `record()` cannot carry either -/// flag, so a control batch admitted here would reach consumers as ordinary data, and consumers -/// filter control records by exactly that flag. -fn preflight(buf: &Bytes, budget: &DecompressionBudget) -> Result<()> { - let mut headers = buf.clone(); - let infos = RecordBatchDecoder::decode_batch_info(&mut headers) +/// - One batch per partition, as Kafka requires from Produce v3, and no bytes after it. +/// - The decoder reserves from `record_count` (`kafka-protocol-0.18.0/src/records.rs:517`), so +/// the count must fit the blob, plus the partition cap when compressed. +/// - Control and transactional batches are refused: a stored message cannot carry their flags. +/// An idempotent batch passes, its producer id, epoch and sequence ignored, because a stock +/// Java producer is idempotent. `IDEMPOTENCE.md` has why a retry is then not deduplicated. +fn preflight(buf: &Bytes, budget: &DecompressionBudget, zstd: Zstd) -> Result<()> { + let mut rest = buf.clone(); + let infos = RecordBatchDecoder::decode_batch_info(&mut rest) .map_err(|error| RecordCodecError::Batch(error.to_string()))?; + let info = match infos.as_slice() { + [] => return Ok(()), + [info] => info, + several => return Err(RecordCodecError::SeveralBatches(several.len())), + }; + // `decode_batch_info` stops at a magic byte other than 2 and leaves the rest unread, and so + // does the decoder. Unchecked, the tail is dropped and the partition answers success. + if rest.has_remaining() { + return Err(RecordCodecError::BatchTrailingBytes(rest.remaining())); + } + if info.transactional { + return Err(RecordCodecError::TransactionalBatch); + } + if info.control { + return Err(RecordCodecError::ControlBatch); + } + if zstd == Zstd::Refused && info.compression == Compression::Zstd { + return Err(RecordCodecError::ZstdTooEarly); + } - let compressed = infos - .iter() - .any(|info| info.compression != Compression::None); - let limit = if compressed { - buf.len().saturating_add(budget.remaining.get()) - } else { + let limit = if info.compression == Compression::None { buf.len() + } else { + buf.len().saturating_add(budget.partition.bytes) }; - - let mut declared = 0usize; - for info in &infos { - if info.transactional { - return Err(RecordCodecError::UnsupportedBatch("transactional")); - } - if info.control { - return Err(RecordCodecError::UnsupportedBatch("control")); - } - declared = declared.saturating_add(usize::try_from(info.record_count).unwrap_or(0)); - if declared.saturating_mul(MIN_RECORD_BYTES) > limit { - return Err(RecordCodecError::RecordCountTooLarge { - count: info.record_count, - limit, - }); - } + let declared = usize::try_from(info.record_count).unwrap_or(0); + if declared.saturating_mul(MIN_RECORD_BYTES) > limit { + return Err(RecordCodecError::RecordCountTooLarge { + count: info.record_count, + limit, + }); } - Ok(()) + budget.charge_records(declared) } /// Encodes records as one uncompressed v2 batch. @@ -800,7 +992,7 @@ fn batch_size(records: &[Record]) -> usize { .sum::() } -/// Decompresses one batch, refusing the write that would pass the request budget. +/// Decompresses one batch, refusing the write that would pass the budget. /// /// `kafka_protocol`'s own decompressors write the whole stream into a growing buffer before they /// hand it over, so the four codecs are driven from here instead. Each one writes through @@ -808,11 +1000,13 @@ fn batch_size(records: &[Record]) -> usize { /// /// The decoder calls this for every batch, uncompressed ones included, which is what makes it the /// one place that sees the record bytes before anything reserves from what they declare. +/// +/// Returns the records and how many there are. fn decompress( compressed: &mut Bytes, compression: Compression, budget: &DecompressionBudget, -) -> anyhow::Result { +) -> anyhow::Result<(Bytes, usize)> { let body = compressed.copy_to_bytes(compressed.remaining()); let mut writer = BudgetedWriter::new(budget); let records = match compression { @@ -824,49 +1018,46 @@ fn decompress( let mut decoder = flate2::write::GzDecoder::new(&mut writer); decoder.write_all(&body)?; decoder.finish()?; - writer.out.freeze() + writer.into_output() } Compression::Zstd => { zstd::stream::copy_decode(body.as_ref(), &mut writer)?; - writer.out.freeze() + writer.into_output() } Compression::Lz4 => { let mut decoder = lz4::Decoder::new(body.as_ref())?; io::copy(&mut decoder, &mut writer)?; decoder.finish().1?; - writer.out.freeze() + writer.into_output() } Compression::Snappy => { inflate_snappy(&body, &mut writer)?; - writer.out.freeze() + writer.into_output() } }; - if let Err(reason) = scan_records(&records) { - let message = reason.to_string(); - budget.refuse(reason); - anyhow::bail!(message); + match scan_records(&records, budget) { + Ok(count) => Ok((records, count)), + Err(reason) => Err(budget.refuse(reason).into()), } - Ok(records) } -/// Walks the records of one batch and refuses a header count the record cannot hold. +/// Headers that cost one record slot. A decoded header takes about a third of a slot's memory. +const HEADERS_PER_SLOT: usize = 3; + +/// Walks the records of one batch before the decoder reserves for them. /// -/// `kafka_protocol` reserves an `IndexMap` from each record's own header count -/// (`kafka-protocol-0.18.0/src/records.rs:896`), which is checked there for sign alone. That -/// count is a varint inside a record body, so no batch header reports it and `preflight` cannot -/// see it. A 72-byte batch declaring `i32::MAX` headers on its one record therefore reaches the -/// reserve, and that allocation is resident rather than virtual, because hashbrown writes its -/// control bytes. Every header costs two length varints at least, so a count past half the -/// record's remaining bytes is refused before the decoder reads the record. +/// - Refuses a header count the record cannot hold. The decoder reserves an `IndexMap` from it +/// (`kafka-protocol-0.18.0/src/records.rs:896`), and that memory is resident. +/// - Charges headers to the record budget, [`HEADERS_PER_SLOT`] per slot. +/// - Returns the record count, which `decode_batch` checks against the batch header. /// -/// The framing below mirrors `Record::decode_new`, field for field, so a record this refuses is -/// one the decoder would refuse too. The one place the two differ is trailing bytes: the decoder -/// stops after the count the batch header gave and ignores anything after it, while this walks to -/// the end of the blob. A v2 batch carries its records and nothing else, so there is nothing to -/// ignore. -fn scan_records(records: &Bytes) -> Result<()> { +/// The framing mirrors `Record::decode_new`, field for field. +fn scan_records(records: &Bytes, budget: &DecompressionBudget) -> Result { let mut blob = records.as_ref(); + let mut walked = 0; + let mut slots = 0; + let mut names = Vec::new(); while !blob.is_empty() { let size = take_varint(&mut blob)?; let size = usize::try_from(size).map_err(|_| RecordCodecError::RecordFieldLength(size))?; @@ -879,18 +1070,40 @@ fn scan_records(records: &Bytes) -> Result<()> { skip_field(&mut record)?; // value let count = take_varint(&mut record)?; - let holds = usize::try_from(count) - .map_err(|_| RecordCodecError::HeaderCountTooLarge { - count, - limit: record.len(), - })? - .saturating_mul(MIN_HEADER_BYTES); - if holds > record.len() { - return Err(RecordCodecError::HeaderCountTooLarge { - count, - limit: record.len(), - }); + let limit = record.len(); + let headers = usize::try_from(count) + .ok() + .filter(|headers| headers.saturating_mul(MIN_HEADER_BYTES) <= limit) + .ok_or(RecordCodecError::HeaderCountTooLarge { count, limit })?; + if headers > 1 { + check_header_names(&mut record, headers, &mut names)?; } + slots += headers / HEADERS_PER_SLOT; + walked += 1; + } + budget.charge_records(slots)?; + Ok(walked) +} + +/// Refuses a record that repeats a header name. `kafka_protocol` keeps only the last value. +/// +/// `names` is scratch space, reused across records. +fn check_header_names<'a>( + record: &mut &'a [u8], + count: usize, + names: &mut Vec<&'a [u8]>, +) -> Result<()> { + names.clear(); + for _ in 0..count { + let len = take_varint(record)?; + let len = usize::try_from(len).map_err(|_| RecordCodecError::RecordFieldLength(len))?; + names.push(take_bytes(record, len)?); + skip_field(record)?; // value + } + names.sort_unstable(); + if let Some(pair) = names.windows(2).find(|pair| pair[0] == pair[1]) { + let name = String::from_utf8_lossy(pair[0]).into_owned(); + return Err(RecordCodecError::RepeatedHeaderName(name)); } Ok(()) } @@ -1030,6 +1243,33 @@ mod tests { message_with(payload, &all) } + /// `to_iggy` for a record sent on its own, so nothing clamps it. + fn to_iggy_alone(record: &Record) -> Result { + to_iggy(record, TimestampWindow::of(std::slice::from_ref(record))) + } + + /// A request allowance of one partition's. + fn partition_budget(bytes: usize, records: usize) -> DecompressionBudget { + let allowance = Allowance { bytes, records }; + DecompressionBudget::new(allowance, allowance) + } + + #[test] + fn given_a_value_only_record_when_encoded_should_match_the_general_path() { + let plain = to_iggy_alone(&record_with(None, Some(b"v"), &[])).unwrap(); + let general = gateway_message(b"v", &[]); + + assert_eq!(plain.user_headers, general.user_headers); + assert_eq!( + plain.header.user_headers_length, + general.header.user_headers_length + ); + assert_eq!(plain.header.origin_timestamp, 1_700_000_000_123_000); + let back = from_iggy(&plain, 0).unwrap(); + assert_eq!(back.value.as_deref(), Some(&b"v"[..])); + assert_eq!(back.timestamp, CREATE_TIME); + } + fn envelope_bytes(count: u32, trailing: &[u8]) -> Bytes { let mut payload = BytesMut::new(); payload.put_u8(0); @@ -1051,7 +1291,7 @@ mod tests { #[test] fn given_a_plain_record_when_round_tripped_should_keep_key_value_and_headers() { let original = record_with(Some(b"k"), Some(b"v"), &[("trace", Some(b"abc"))]); - let message = to_iggy(&original).unwrap(); + let message = to_iggy_alone(&original).unwrap(); assert!(!is_enveloped(&message)); assert_eq!(message.payload.as_ref(), b"v"); @@ -1068,7 +1308,7 @@ mod tests { #[test] fn given_a_null_value_when_round_tripped_should_stay_null() { - let message = to_iggy(&record_with(Some(b"k"), None, &[])).unwrap(); + let message = to_iggy_alone(&record_with(Some(b"k"), None, &[])).unwrap(); assert!( !is_enveloped(&message), "a tombstone stays on the fast path" @@ -1079,7 +1319,7 @@ mod tests { #[test] fn given_an_empty_value_when_round_tripped_should_stay_empty_and_not_null() { - let message = to_iggy(&record_with(Some(b"k"), Some(b""), &[])).unwrap(); + let message = to_iggy_alone(&record_with(Some(b"k"), Some(b""), &[])).unwrap(); assert_eq!(message.payload.as_ref(), PLACEHOLDER); assert_eq!( from_iggy(&message, 0).unwrap().value.as_deref(), @@ -1090,7 +1330,7 @@ mod tests { #[test] fn given_an_empty_key_when_stored_should_take_the_envelope() { let original = record_with(Some(b""), Some(b"v"), &[]); - let message = to_iggy(&original).unwrap(); + let message = to_iggy_alone(&original).unwrap(); assert!(is_enveloped(&message)); let back = from_iggy(&message, 0).unwrap(); assert_eq!(back.key.as_deref(), Some(&[][..]), "empty, not null"); @@ -1100,7 +1340,7 @@ mod tests { #[test] fn given_an_oversized_key_when_stored_should_take_the_envelope() { let key = vec![b'x'; MAX_FIELD + 1]; - let message = to_iggy(&record_with(Some(&key), Some(b"v"), &[])).unwrap(); + let message = to_iggy_alone(&record_with(Some(&key), Some(b"v"), &[])).unwrap(); assert!(is_enveloped(&message)); assert_eq!( from_iggy(&message, 0).unwrap().key.as_deref(), @@ -1110,7 +1350,8 @@ mod tests { #[test] fn given_a_null_header_value_when_stored_should_take_the_envelope() { - let message = to_iggy(&record_with(Some(b"k"), Some(b"v"), &[("flag", None)])).unwrap(); + let message = + to_iggy_alone(&record_with(Some(b"k"), Some(b"v"), &[("flag", None)])).unwrap(); assert!(is_enveloped(&message)); assert_eq!( from_iggy(&message, 0) @@ -1126,7 +1367,7 @@ mod tests { fn given_an_oversized_header_name_when_stored_should_take_the_envelope() { let name = "n".repeat(MAX_FIELD - HEADER_PREFIX.len() + 1); let message = - to_iggy(&record_with(Some(b"k"), Some(b"v"), &[(&name, Some(b"v"))])).unwrap(); + to_iggy_alone(&record_with(Some(b"k"), Some(b"v"), &[(&name, Some(b"v"))])).unwrap(); assert!(is_enveloped(&message)); assert!( from_iggy(&message, 0) @@ -1201,7 +1442,7 @@ mod tests { #[test] fn given_unreadable_user_headers_when_read_should_fail() { - let mut message = to_iggy(&record_with(Some(b"k"), Some(b"v"), &[])).unwrap(); + let mut message = to_iggy_alone(&record_with(Some(b"k"), Some(b"v"), &[])).unwrap(); message.user_headers = Some(Bytes::from_static(b"not a header block")); assert!( @@ -1235,17 +1476,89 @@ mod tests { } #[test] - fn given_an_unknown_timestamp_marker_on_a_gateway_message_when_read_should_fail() { - let message = gateway_message(b"v", &[(TIMESTAMP_MARKER_HEADER, b"later")]); + fn given_a_timestamp_header_that_is_not_int64_on_a_gateway_message_when_read_should_fail() { + let message = gateway_message(b"v", &[(TIMESTAMP_HEADER, b"epoch")]); assert!(matches!( from_iggy(&message, 0), - Err(RecordCodecError::TimestampMarker(_)) + Err(RecordCodecError::TimestampHeader(_)) )); } + #[test] + fn given_a_timestamp_header_below_minus_one_when_read_should_fail() { + let mut headers = BTreeMap::new(); + headers.insert(header_key(VERSION_HEADER), header_value(&[MAPPING_VERSION])); + headers.insert(header_key(TIMESTAMP_HEADER), HeaderValue::from(-2i64)); + let message = IggyMessage::builder() + .payload(Bytes::from_static(b"v")) + .user_headers(headers) + .build() + .unwrap(); + + assert!( + matches!( + from_iggy(&message, 0), + Err(RecordCodecError::TimestampHeader(_)) + ), + "to_iggy refuses these, so no gateway message holds one" + ); + } + + #[test] + fn given_timestamps_past_one_send_when_stored_should_clamp_and_keep_the_real_one() { + let later = CREATE_TIME + 72 * 60 * 1000; + let records = [record_at(CREATE_TIME), record_at(later)]; + let window = TimestampWindow::of(&records); + let first = to_iggy(&records[0], window).unwrap(); + let second = to_iggy(&records[1], window).unwrap(); + + assert_eq!( + second.header.origin_timestamp - first.header.origin_timestamp, + MAX_SEND_SPAN_MICROS, + "clamped to the window end, so one send holds both" + ); + assert_eq!(from_iggy(&first, 0).unwrap().timestamp, CREATE_TIME); + assert_eq!(from_iggy(&second, 1).unwrap().timestamp, later); + } + + #[test] + fn given_no_timestamp_among_real_ones_when_round_tripped_should_read_the_server_timestamp() { + let records = [record_at(CREATE_TIME), record_at(NO_TIMESTAMP)]; + let mut message = to_iggy(&records[1], TimestampWindow::of(&records)).unwrap(); + assert_eq!( + message.header.origin_timestamp, + CREATE_TIME.cast_unsigned() * 1000, + "clamped to the window start" + ); + message.header.timestamp = 5_000_000; + assert_eq!(from_iggy(&message, 0).unwrap().timestamp, 5_000); + } + + #[test] + fn given_an_epoch_timestamp_among_real_ones_when_round_tripped_should_stay_at_the_epoch() { + let records = [record_at(CREATE_TIME), record_at(EPOCH_TIMESTAMP)]; + let message = to_iggy(&records[1], TimestampWindow::of(&records)).unwrap(); + assert_eq!(from_iggy(&message, 0).unwrap().timestamp, EPOCH_TIMESTAMP); + } + + #[test] + fn given_a_batch_when_windowed_should_start_at_its_earliest_real_timestamp() { + let records = [ + record_at(NO_TIMESTAMP), + record_at(EPOCH_TIMESTAMP), + record_at(CREATE_TIME + 5), + record_at(CREATE_TIME), + ]; + assert_eq!( + TimestampWindow::of(&records).start, + CREATE_TIME.cast_unsigned() * 1000 + ); + assert_eq!(TimestampWindow::of(&[record_at(NO_TIMESTAMP)]).start, 0); + } + #[test] fn given_a_create_time_when_round_tripped_should_come_back_unchanged() { - let message = to_iggy(&record_at(CREATE_TIME)).unwrap(); + let message = to_iggy_alone(&record_at(CREATE_TIME)).unwrap(); assert_eq!( message.header.origin_timestamp, CREATE_TIME.cast_unsigned() * 1000 @@ -1272,7 +1585,7 @@ mod tests { #[test] fn given_an_epoch_timestamp_when_round_tripped_should_stay_at_the_epoch() { - let mut message = to_iggy(&record_at(EPOCH_TIMESTAMP)).unwrap(); + let mut message = to_iggy_alone(&record_at(EPOCH_TIMESTAMP)).unwrap(); message.header.timestamp = 5_000_000; assert_eq!(from_iggy(&message, 0).unwrap().timestamp, EPOCH_TIMESTAMP); } @@ -1286,7 +1599,7 @@ mod tests { 0, EPOCH_TIMESTAMP, ); - let mut message = to_iggy(&original).unwrap(); + let mut message = to_iggy_alone(&original).unwrap(); assert!(is_enveloped(&message)); message.header.timestamp = 5_000_000; assert_eq!(from_iggy(&message, 0).unwrap().timestamp, EPOCH_TIMESTAMP); @@ -1294,14 +1607,14 @@ mod tests { #[test] fn given_no_timestamp_when_read_should_use_the_server_timestamp() { - let mut message = to_iggy(&record_at(NO_TIMESTAMP)).unwrap(); + let mut message = to_iggy_alone(&record_at(NO_TIMESTAMP)).unwrap(); message.header.timestamp = 5_000_000; assert_eq!(from_iggy(&message, 0).unwrap().timestamp, 5_000); } #[test] fn given_a_truncated_envelope_when_decoded_should_fail() { - let message = to_iggy(&record_with(Some(b""), Some(b"v"), &[])).unwrap(); + let message = to_iggy_alone(&record_with(Some(b""), Some(b"v"), &[])).unwrap(); assert!(matches!( decode_envelope(&[ENVELOPE_VERSION], &message.payload.slice(0..3)), Err(RecordCodecError::EnvelopeTruncated { .. }) @@ -1361,7 +1674,7 @@ mod tests { CREATE_TIME, ); assert!(matches!( - to_iggy(&oversized), + to_iggy_alone(&oversized), Err(RecordCodecError::EnvelopeTooLarge { size }) if size == MAX_PAYLOAD_SIZE as usize + ENVELOPE_OVERHEAD )); @@ -1409,8 +1722,8 @@ mod tests { fn given_an_uncompressed_batch_when_round_tripped_should_keep_every_record() { let mut records = vec![record_at_offset(0, b"1"), record_at_offset(1, b"2")]; let mut encoded = encode_batch(&mut records).unwrap(); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let budget = partition_budget(1024, usize::MAX); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!(decoded.len(), 2); assert_eq!(decoded[1].value.as_deref(), Some(&b"2"[..])); } @@ -1439,22 +1752,71 @@ mod tests { } #[test] - fn given_two_batches_in_one_blob_when_decoded_should_drain_both() { + fn given_two_batches_in_one_blob_when_decoded_should_reject() { let mut blob = BytesMut::new(); blob.extend_from_slice(&encode_batch(&mut [record_at_offset(0, b"1")]).unwrap()); blob.extend_from_slice(&encode_batch(&mut [record_at_offset(1, b"2")]).unwrap()); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut blob.freeze(), &budget).unwrap(); - assert_eq!(decoded.len(), 2, "a partition blob can hold many batches"); + let budget = partition_budget(1024, usize::MAX); + assert!( + matches!( + decode_batch(&mut blob.freeze(), &budget, Zstd::Allowed), + Err(RecordCodecError::SeveralBatches(2)) + ), + "Kafka allows one batch per partition from Produce v3" + ); + } + + #[test] + fn given_bytes_after_the_batch_when_decoded_should_reject() { + let mut blob = BytesMut::new(); + blob.extend_from_slice(&encode_batch(&mut [record_at_offset(0, b"1")]).unwrap()); + // Byte 16 of the tail is its magic. Not 2, so `decode_batch_info` stops there, no error. + blob.extend_from_slice(&[0; 32]); + let budget = partition_budget(1024, usize::MAX); + + assert!( + matches!( + decode_batch(&mut blob.freeze(), &budget, Zstd::Allowed), + Err(RecordCodecError::BatchTrailingBytes(32)) + ), + "the decoder never reads the tail, so the partition would answer success" + ); + } + + #[test] + fn given_more_records_than_the_batch_declares_when_decoded_should_reject() { + let batch = + encode_batch(&mut [record_at_offset(0, b"1"), record_at_offset(1, b"2")]).unwrap(); + let mut patched = patch_header(&batch, 57, &1i32.to_be_bytes()); + let budget = partition_budget(1024, usize::MAX); + + assert!( + matches!( + decode_batch(&mut patched, &budget, Zstd::Allowed), + Err(RecordCodecError::RecordCountMismatch { + declared: 1, + walked: 2 + }) + ), + "the decoder drops the second record, so storing the rest would lose it" + ); + } + + #[test] + fn given_a_batch_when_checked_for_compression_should_read_its_attributes() { + let records = [record_at_offset(0, b"v")]; + assert!(is_compressed(&encode_with(&records, Compression::Gzip))); + assert!(!is_compressed(&encode_with(&records, Compression::None))); + assert!(!is_compressed(&[])); } #[test] fn given_a_gzip_batch_when_decoded_should_read_it() { let records = vec![record_at_offset(0, b"compressed")]; let mut encoded = encode_with(&records, Compression::Gzip); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let budget = partition_budget(1024, usize::MAX); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!(decoded[0].value.as_deref(), Some(&b"compressed"[..])); } @@ -1462,8 +1824,8 @@ mod tests { fn given_a_snappy_batch_when_decoded_should_read_it() { let records = vec![record_at_offset(0, b"compressed")]; let mut encoded = encode_with(&records, Compression::Snappy); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let budget = partition_budget(1024, usize::MAX); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!(decoded[0].value.as_deref(), Some(&b"compressed"[..])); } @@ -1471,8 +1833,8 @@ mod tests { fn given_an_lz4_batch_when_decoded_should_read_it() { let records = vec![record_at_offset(0, b"compressed")]; let mut encoded = encode_with(&records, Compression::Lz4); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let budget = partition_budget(1024, usize::MAX); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!(decoded[0].value.as_deref(), Some(&b"compressed"[..])); } @@ -1480,8 +1842,8 @@ mod tests { fn given_a_zstd_batch_when_decoded_should_read_it() { let records = vec![record_at_offset(0, b"compressed")]; let mut encoded = encode_with(&records, Compression::Zstd); - let budget = DecompressionBudget::new(1024); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let budget = partition_budget(1024, usize::MAX); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!(decoded[0].value.as_deref(), Some(&b"compressed"[..])); } @@ -1489,50 +1851,116 @@ mod tests { fn given_a_budget_smaller_than_the_batch_when_decoded_should_reject() { let records = vec![record_at_offset(0, &[b'x'; 512])]; let mut encoded = encode_with(&records, Compression::Gzip); - let budget = DecompressionBudget::new(8); + let budget = partition_budget(8, usize::MAX); assert!(matches!( - decode_batches(&mut encoded, &budget), + decode_batch(&mut encoded, &budget, Zstd::Allowed), Err(RecordCodecError::BudgetExceeded { .. }) )); } #[test] - fn given_two_batches_when_the_second_passes_the_budget_should_reject() { - let big = record_at_offset(0, &[b'x'; 256]); - let mut blob = BytesMut::new(); - blob.extend_from_slice(&encode_batch(&mut [big.clone()]).unwrap()); - blob.extend_from_slice(&encode_batch(&mut [big]).unwrap()); + fn given_a_spent_budget_when_a_partition_fits_alone_should_ask_for_a_retry() { + // Enough for one partition, not for both: the budget is per request. + let budget = partition_budget(400, usize::MAX); + let big = || encode_batch(&mut [record_at_offset(0, &[b'x'; 256])]).unwrap(); - // Enough for one batch, not for both: the budget is per request, not per batch. - let budget = DecompressionBudget::new(400); + assert!(decode_batch(&mut big(), &budget, Zstd::Allowed).is_ok()); assert!(matches!( - decode_batches(&mut blob.freeze(), &budget), + decode_batch(&mut big(), &budget, Zstd::Allowed), + Err(RecordCodecError::RequestBudgetSpent) + )); + } + + #[test] + fn given_a_spent_budget_when_a_later_partition_arrives_should_refuse_it_undecoded() { + let budget = partition_budget(400, usize::MAX); + let big = || encode_batch(&mut [record_at_offset(0, &[b'x'; 256])]).unwrap(); + assert!(decode_batch(&mut big(), &budget, Zstd::Allowed).is_ok()); + assert!(decode_batch(&mut big(), &budget, Zstd::Allowed).is_err()); + + // Malformed, so a decode would refuse it as a bad batch instead. + let mut garbage = Bytes::from_static(b"not a record batch"); + assert!( + matches!( + decode_batch(&mut garbage, &budget, Zstd::Allowed), + Err(RecordCodecError::RequestBudgetSpent) + ), + "one spent request inflates nothing more, so it cannot hold a slot inflating" + ); + } + + #[test] + fn given_a_partition_too_large_alone_when_others_follow_should_leave_them_room() { + let partition = Allowance { + bytes: 64 * 1024, + records: usize::MAX, + }; + let budget = DecompressionBudget::new(partition, partition.times(2)); + let mut bomb = encode_with( + &[record_at_offset(0, &vec![0; 1024 * 1024])], + Compression::Gzip, + ); + let normal = || { + encode_with( + &[record_at_offset(0, &vec![b'n'; 32 * 1024])], + Compression::Gzip, + ) + }; + + assert!(matches!( + decode_batch(&mut bomb, &budget, Zstd::Allowed), Err(RecordCodecError::BudgetExceeded { .. }) )); + assert!( + decode_batch(&mut normal(), &budget, Zstd::Allowed).is_ok(), + "the bomb stops at its own cap, so the request has room left" + ); + } + + #[test] + fn given_a_partition_too_large_alone_when_decoded_should_refuse_it() { + // gzip writes 32 KiB at a time, so the refusal comes several writes before the end. + let partition = Allowance { + bytes: 64 * 1024, + records: usize::MAX, + }; + let budget = DecompressionBudget::new(partition, partition.times(8)); + let mut first = encode_with( + &[record_at_offset(0, &vec![b'a'; 60 * 1024])], + Compression::Gzip, + ); + let mut second = encode_with( + &[record_at_offset(0, &vec![b'b'; 200 * 1024])], + Compression::Gzip, + ); + + assert!(decode_batch(&mut first, &budget, Zstd::Allowed).is_ok()); + assert!( + matches!( + decode_batch(&mut second, &budget, Zstd::Allowed), + Err(RecordCodecError::BudgetExceeded { limit, .. }) if limit == 64 * 1024 + ), + "a retry cannot help a partition over its own cap" + ); } #[test] fn given_a_compression_bomb_when_decoded_should_stop_before_it_is_whole() { let bomb = vec![0u8; 4 * 1024 * 1024]; let mut compressed = Bytes::from(encode_gzip(&bomb)); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); let Err(error) = decompress(&mut compressed, Compression::Gzip, &budget) else { panic!("a 4 MB output against a 1 KB budget has to be refused"); }; drop(error); - let Some(RecordCodecError::BudgetExceeded { - produced, - remaining, - }) = budget.reason.take() - else { + let Some(RecordCodecError::BudgetExceeded { size, limit }) = budget.reason.take() else { panic!("the budget is what refused it"); }; - assert_eq!(remaining, 1024); + assert_eq!(limit, 1024); assert!( - produced < bomb.len(), - "the write that passed the budget was refused, not charged afterwards: \ - {produced} of {} bytes", + size < bomb.len(), + "refused at the write that passed the budget: {size} of {} bytes", bomb.len() ); } @@ -1542,11 +1970,11 @@ mod tests { // A raw snappy stream whose leading varint claims u32::MAX bytes of output. `snap` reads // that length without allocating, so the declared size is checkable before the decode. let mut compressed = Bytes::from_static(&[0xff, 0xff, 0xff, 0xff, 0x0f, 0x00]); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); assert!(decompress(&mut compressed, Compression::Snappy, &budget).is_err()); assert!(matches!( budget.reason.take(), - Some(RecordCodecError::BudgetExceeded { produced, .. }) if produced == u32::MAX as usize + Some(RecordCodecError::BudgetExceeded { size, .. }) if size == u32::MAX as usize )); } @@ -1554,11 +1982,11 @@ mod tests { fn given_a_record_count_past_the_frame_when_decoded_should_reject() { let batch = encode_batch(&mut [record_at_offset(0, b"v")]).unwrap(); let mut patched = patch_header(&batch, 57, &i32::MAX.to_be_bytes()); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); assert!( matches!( - decode_batches(&mut patched, &budget), + decode_batch(&mut patched, &budget, Zstd::Allowed), Err(RecordCodecError::RecordCountTooLarge { .. }) ), "the decoder reserves from this count before it reads a record" @@ -1566,27 +1994,41 @@ mod tests { } #[test] - fn given_counts_that_only_pass_one_at_a_time_when_decoded_should_reject() { - // One large batch raises what the blob can hold, and every small batch then declares a - // count that clears the bound on its own. The reserves land in one `Vec` all the same. - let big = encode_batch(&mut [record_at_offset(0, &[b'x'; 2048])]).unwrap(); - let small = patch_header( - &encode_batch(&mut [record_at_offset(0, b"v")]).unwrap(), - 57, - &200i32.to_be_bytes(), - ); + fn given_a_compressed_batch_when_preflighting_should_grant_it_the_partition_cap() { + // 100 tiny records declare more than the blob and the bytes left can hold, and fit the + // partition cap. Earlier partitions spending the request must not make them look + // malformed. One offset for all, so the encoder writes one batch. + let records: Vec<_> = (0..100).map(|_| record_at_offset(0, b"")).collect(); + let mut batch = encode_with(&records, Compression::Gzip); + let budget = partition_budget(4096, usize::MAX); + let mut spend = encode_batch(&mut [record_at_offset(0, &[b'x'; 4000])]).unwrap(); + + assert!(decode_batch(&mut spend, &budget, Zstd::Allowed).is_ok()); + assert!(matches!( + decode_batch(&mut batch, &budget, Zstd::Allowed), + Err(RecordCodecError::RequestBudgetSpent) + )); + } - let mut blob = BytesMut::new(); - blob.extend_from_slice(&big); - for _ in 0..8 { - blob.extend_from_slice(&small); - } + #[test] + fn given_many_headers_when_decoded_should_charge_them_as_records() { + let headers: Vec<_> = (0..6) + .map(|index| (format!("h{index}"), Some(&b"v"[..]))) + .collect(); + let headers: Vec<_> = headers + .iter() + .map(|(name, value)| (name.as_str(), *value)) + .collect(); + let mut batch = encode_batch(&mut [record_with(None, Some(b"v"), &headers)]).unwrap(); - let budget = DecompressionBudget::new(0); + // One slot for the record and two for its six headers. + let tight = partition_budget(1024, 2); assert!(matches!( - decode_batches(&mut blob.freeze(), &budget), - Err(RecordCodecError::RecordCountTooLarge { .. }) + decode_batch(&mut batch.clone(), &tight, Zstd::Allowed), + Err(RecordCodecError::RecordBudgetExceeded { count: 3, limit: 2 }) )); + let enough = partition_budget(1024, 3); + assert!(decode_batch(&mut batch, &enough, Zstd::Allowed).is_ok()); } /// A one-record v2 batch whose record body declares `headers` headers and carries none. @@ -1637,16 +2079,35 @@ mod tests { batch.freeze() } + #[test] + fn given_a_repeated_header_name_when_decoded_should_reject() { + let headers = [("a", Some(&b"x"[..])), ("b", Some(&b"y"[..]))]; + let mut records = [record_with(None, Some(b"v"), &headers)]; + let batch = encode_batch(&mut records).unwrap(); + // Rename header "b" to "a". Same length, so only the CRC needs repair. + let at = batch + .windows(4) + .position(|window| window == [0x02, b'b', 0x02, b'y']) + .unwrap(); + let mut batch = patch_header(&batch, at + 1, b"a"); + let budget = partition_budget(1024, usize::MAX); + + assert!(matches!( + decode_batch(&mut batch, &budget, Zstd::Allowed), + Err(RecordCodecError::RepeatedHeaderName(name)) if name == "a" + )); + } + #[test] fn given_a_header_count_past_the_record_when_decoded_should_reject() { // The batch header says one record, so the record count bound passes. The count that // matters is the one inside the record, and no batch header reports it. let mut batch = batch_declaring_headers(i32::MAX); - let budget = DecompressionBudget::new(8 * 1024 * 1024); + let budget = partition_budget(8 * 1024 * 1024, usize::MAX); assert!( matches!( - decode_batches(&mut batch, &budget), + decode_batch(&mut batch, &budget, Zstd::Allowed), Err(RecordCodecError::HeaderCountTooLarge { count, .. }) if count == i32::MAX ), "this reserve is resident memory, not address space" @@ -1658,8 +2119,13 @@ mod tests { // The same builder with a count the record can hold, so the scan cannot be passing the // test above by rejecting every hand-built batch. let mut batch = batch_declaring_headers(0); - let budget = DecompressionBudget::new(1024); - assert_eq!(decode_batches(&mut batch, &budget).unwrap().len(), 1); + let budget = partition_budget(1024, usize::MAX); + assert_eq!( + decode_batch(&mut batch, &budget, Zstd::Allowed) + .unwrap() + .len(), + 1 + ); } #[test] @@ -1671,9 +2137,9 @@ mod tests { &[("trace", Some(b"abc"))], )]; let mut encoded = encode_batch(&mut with_headers).unwrap(); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); - let decoded = decode_batches(&mut encoded, &budget).unwrap(); + let decoded = decode_batch(&mut encoded, &budget, Zstd::Allowed).unwrap(); assert_eq!( decoded[0].headers.get(&StrBytes::from_static_str("trace")), Some(&Some(Bytes::from_static(b"abc"))) @@ -1686,10 +2152,10 @@ mod tests { // the documented 8 MiB, which an uncompressed batch has no claim on. let batch = encode_batch(&mut [record_at_offset(0, b"v")]).unwrap(); let mut patched = patch_header(&batch, 57, &100_000i32.to_be_bytes()); - let budget = DecompressionBudget::new(8 * 1024 * 1024); + let budget = partition_budget(8 * 1024 * 1024, usize::MAX); assert!(matches!( - decode_batches(&mut patched, &budget), + decode_batch(&mut patched, &budget, Zstd::Allowed), Err(RecordCodecError::RecordCountTooLarge { limit, .. }) if limit == batch.len() )); } @@ -1751,11 +2217,11 @@ mod tests { let mut transactional = record_at_offset(0, b"v"); transactional.transactional = true; let mut encoded = encode_with(&[transactional], Compression::None); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); assert!(matches!( - decode_batches(&mut encoded, &budget), - Err(RecordCodecError::UnsupportedBatch("transactional")) + decode_batch(&mut encoded, &budget, Zstd::Allowed), + Err(RecordCodecError::TransactionalBatch) )); } @@ -1764,12 +2230,12 @@ mod tests { let mut control = record_at_offset(0, b"v"); control.control = true; let mut encoded = encode_with(&[control], Compression::None); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); assert!( matches!( - decode_batches(&mut encoded, &budget), - Err(RecordCodecError::UnsupportedBatch("control")) + decode_batch(&mut encoded, &budget, Zstd::Allowed), + Err(RecordCodecError::ControlBatch) ), "consumers filter control records by a flag no stored message can carry" ); @@ -1779,8 +2245,8 @@ mod tests { fn given_a_truncated_batch_when_decoded_should_reject() { let batch = encode_batch(&mut [record_at_offset(0, b"value")]).unwrap(); let mut truncated = batch.slice(0..batch.len() - 4); - let budget = DecompressionBudget::new(1024); - assert!(decode_batches(&mut truncated, &budget).is_err()); + let budget = partition_budget(1024, usize::MAX); + assert!(decode_batch(&mut truncated, &budget, Zstd::Allowed).is_err()); } #[test] @@ -1791,9 +2257,9 @@ mod tests { bytes[last] ^= 0xff; let mut corrupt = Bytes::from(bytes); - let budget = DecompressionBudget::new(1024); + let budget = partition_budget(1024, usize::MAX); assert!(matches!( - decode_batches(&mut corrupt, &budget), + decode_batch(&mut corrupt, &budget, Zstd::Allowed), Err(RecordCodecError::Batch(_)) )); } @@ -1801,10 +2267,10 @@ mod tests { #[test] fn given_an_earlier_overrun_when_a_later_batch_fails_should_not_reuse_the_budget_reason() { // A failed charge deducts nothing, so the budget still has room for the batch below. - let budget = DecompressionBudget::new(64); + let budget = partition_budget(64, usize::MAX); let mut bomb = encode_with(&[record_at_offset(0, &[b'x'; 512])], Compression::Gzip); assert!(matches!( - decode_batches(&mut bomb, &budget), + decode_batch(&mut bomb, &budget, Zstd::Allowed), Err(RecordCodecError::BudgetExceeded { .. }) )); @@ -1815,7 +2281,7 @@ mod tests { assert!( matches!( - decode_batches(&mut short, &budget), + decode_batch(&mut short, &budget, Zstd::Allowed), Err(RecordCodecError::Batch(_)) ), "a stale overflow reports a malformed batch as MESSAGE_TOO_LARGE" diff --git a/gateways/kafka/tests/api_handler_tests.rs b/gateways/kafka/tests/api_handler_tests.rs index 580cb857ed..b12f9e027a 100644 --- a/gateways/kafka/tests/api_handler_tests.rs +++ b/gateways/kafka/tests/api_handler_tests.rs @@ -203,11 +203,8 @@ async fn apiversions_unsupported_version_uses_v0_encoding_without_throttle() { } #[tokio::test] -async fn produce_malformed_body_with_acks_one_stays_silent() { - // `kafka_protocol` decodes Produce in one shot, so a decode failure never exposes `acks` - // (unlike the pre-migration field-by-field decoder, which could still answer with - // INVALID_REQUEST once it knew acks was nonzero). Every Produce decode failure now stays - // silent rather than risk desyncing an acks=0 fire-and-forget client's correlation stream. +async fn produce_malformed_body_with_acks_one_closes() { + // `acks` is unknown after a failed decode, so no reply is safe. Kafka closes too. let body = Bytes::from_static(&[ 0xff, 0xff, // null transactional_id 0x00, 0x01, // acks = 1 @@ -217,8 +214,8 @@ async fn produce_malformed_body_with_acks_one_stays_silent() { assert!( handle_request(API_KEY_PRODUCE, 3, body, &default_broker()) .await - .is_no_response(), - "malformed Produce body must stay silent regardless of acks" + .is_close(), + "malformed Produce body must close regardless of acks" ); } @@ -806,19 +803,17 @@ async fn create_topics_decodes_request_with_real_topic_and_assignment() { assert_eq!(d.read_i16().unwrap(), ERROR_NOT_CONTROLLER); } -// ── Produce acks=0 (broker must stay silent even on a malformed body) ────── +// ── Produce acks=0 with a malformed body closes ────── #[tokio::test] -async fn produce_acks_zero_malformed_body_stays_silent() { - // topics array declares 1 element but no topic data follows - decode fails after acks=0 - // was on the wire, though `kafka_protocol`'s one-shot decode no longer exposes that acks - // was read. The handler must stay silent regardless. +async fn produce_acks_zero_malformed_body_closes() { + // topics array declares 1 element but no topic data follows. let body = build_produce_v3_body(0, 1); assert!( handle_request(API_KEY_PRODUCE, 3, body, &default_broker()) .await - .is_no_response(), - "handler must not respond when acks=0 even if decode fails after acks" + .is_close(), + "handler must close, not answer, when decode fails" ); } diff --git a/gateways/kafka/tests/bridge_iggy_integration_tests.rs b/gateways/kafka/tests/bridge_iggy_integration_tests.rs index 286f58ad71..3f2afd332b 100644 --- a/gateways/kafka/tests/bridge_iggy_integration_tests.rs +++ b/gateways/kafka/tests/bridge_iggy_integration_tests.rs @@ -30,7 +30,8 @@ use secrecy::SecretString; use serial_test::serial; use iggy_gateway_kafka::bridge::{ - BridgeError, IggyBridge, IggyBridgeConfig, TopicMapping, TopicOverride, + BridgeError, DEFAULT_MAX_MESSAGE_SIZE, IggyBridge, IggyBridgeConfig, TopicMapping, + TopicOverride, }; #[path = "common/iggy_server.rs"] @@ -392,6 +393,7 @@ async fn connect_to_unreachable_iggy_returns_err_not_panic() { password: SecretString::from("iggy"), topic_mapping: TopicMapping::new("kafka".to_string(), HashMap::new()) .expect("valid mapping for this test's fixture data"), + max_message_size: DEFAULT_MAX_MESSAGE_SIZE, }; let result = IggyBridge::connect(config).await; @@ -415,6 +417,7 @@ async fn connect_to_a_black_hole_address_times_out_instead_of_hanging() { password: SecretString::from("iggy"), topic_mapping: TopicMapping::new("kafka".to_string(), HashMap::new()) .expect("valid mapping for this test's fixture data"), + max_message_size: DEFAULT_MAX_MESSAGE_SIZE, }; let start = tokio::time::Instant::now(); diff --git a/gateways/kafka/tests/common/iggy_server.rs b/gateways/kafka/tests/common/iggy_server.rs index 478a7343bf..b8bef7c732 100644 --- a/gateways/kafka/tests/common/iggy_server.rs +++ b/gateways/kafka/tests/common/iggy_server.rs @@ -32,7 +32,7 @@ use std::time::Duration; use iggy::prelude::{AutoLogin, Client, Credentials, IggyClient, IggyClientBuilder}; use secrecy::SecretString; -use iggy_gateway_kafka::bridge::{IggyBridgeConfig, TopicMapping}; +use iggy_gateway_kafka::bridge::{DEFAULT_MAX_MESSAGE_SIZE, IggyBridgeConfig, TopicMapping}; /// Desired slot count - the actual count `port_band()` computes may be smaller if the machine's /// real ephemeral range leaves little room, but this crate never needs more than a handful live @@ -336,6 +336,7 @@ impl TestServer { password: SecretString::from(self.password.clone()), topic_mapping: TopicMapping::new("kafka".to_string(), HashMap::new()) .expect("valid mapping for this test's fixture data"), + max_message_size: DEFAULT_MAX_MESSAGE_SIZE, } } } diff --git a/gateways/kafka/tests/listener_robustness_tests.rs b/gateways/kafka/tests/listener_robustness_tests.rs index 585aba3994..2643a5d0a5 100644 --- a/gateways/kafka/tests/listener_robustness_tests.rs +++ b/gateways/kafka/tests/listener_robustness_tests.rs @@ -518,14 +518,10 @@ async fn e2e_quiet_connection_survives_beyond_read_timeout_idle_cap() { assert_eq!(corr, 502); } -// ── Corrupt body survives on the connection (no disconnect) ──────────────── +// ── Corrupt Produce body closes the connection ──────────────── #[tokio::test] -async fn corrupt_produce_body_e2e_stays_silent_without_disconnect() { - // `kafka_protocol` decodes Produce in one shot, so a decode failure never exposes whether - // `acks` was nonzero (unlike the pre-migration field-by-field decoder, which could still - // answer with INVALID_REQUEST once it knew acks was nonzero). Every Produce decode failure - // now stays silent - see `api::handle_produce_request` - but must not drop the connection. +async fn corrupt_produce_body_e2e_closes_connection() { let (addr, _shutdown) = spawn_test_server().await; let mut stream = TcpStream::connect(addr).await.expect("connect"); @@ -539,20 +535,10 @@ async fn corrupt_produce_body_e2e_stays_silent_without_disconnect() { ], ); stream.write_all(&bad).await.expect("corrupt produce"); - assert!( - read_response_frame_with_timeout(&mut stream, 8 * 1024 * 1024, Duration::from_millis(200)) - .await - .is_none(), - "corrupt Produce must stay silent, not respond with an error" - ); - - let ok = build_request_frame(API_KEY_API_VERSIONS, 1, 392, Some("scope-test"), &[]); - stream.write_all(&ok).await.expect("follow-up"); - let payload = read_response_frame(&mut stream, 8 * 1024 * 1024).await; assert_eq!( - parse_response_payload(API_KEY_API_VERSIONS, 1, payload).0, - 392, - "connection must stay usable after the silent corrupt Produce" + read_byte_with_timeout(&mut stream, Duration::from_secs(2)).await, + ByteRead::Closed, + "corrupt Produce must close the connection" ); } diff --git a/gateways/kafka/tests/produce_real_bridge_tests.rs b/gateways/kafka/tests/produce_real_bridge_tests.rs new file mode 100644 index 0000000000..f22ae0b435 --- /dev/null +++ b/gateways/kafka/tests/produce_real_bridge_tests.rs @@ -0,0 +1,750 @@ +// 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. + +//! Produce (API key 0) driven through the whole handler against a real `iggy-server`. +//! +//! Records go in as Kafka wire bytes and come back out through the Iggy SDK, so a mapping broken +//! the same way in both directions cannot pass. + +use std::sync::Arc; + +use bytes::{Bytes, BytesMut}; +use iggy::prelude::{ + Consumer, HeaderKey, Identifier, IggyMessage, MessageClient, PollingStrategy, TopicClient, +}; +use kafka_protocol::indexmap::IndexMap; +use kafka_protocol::protocol::StrBytes; +use kafka_protocol::records::{ + Compression, NO_PARTITION_LEADER_EPOCH, NO_PRODUCER_EPOCH, NO_PRODUCER_ID, NO_SEQUENCE, Record, + RecordBatchEncoder, RecordEncodeOptions, TimestampType, +}; +use serial_test::serial; + +use iggy_gateway_kafka::bridge::IggyBridge; +use iggy_gateway_kafka::protocol::api::{ + API_KEY_PRODUCE, BrokerAdvertise, ERROR_NONE, ERROR_NOT_LEADER_OR_FOLLOWER, + ERROR_UNKNOWN_TOPIC_OR_PARTITION, GatewayState, handle_request_bounded, +}; +use iggy_gateway_kafka::records::{KEY_HEADER, MAPPING_VERSION, VERSION_HEADER, from_iggy}; + +#[path = "common/codec.rs"] +mod codec; +#[path = "common/iggy_server.rs"] +mod iggy_server; + +use codec::{Decoder, Encoder}; +use iggy_server::{TestServer, raw_client}; + +/// The stream every unmapped Kafka topic resolves to, per `TopicMapping`'s default rule. +const STREAM: &str = "kafka"; +const TOPIC: &str = "orders"; +const CREATE_TIME: i64 = 1_700_000_000_123; +const MAX_FRAME_SIZE: usize = 8 * 1024 * 1024; + +/// Spelled out rather than imported: a test reading its expected code from the same constant the +/// handler answers with cannot catch that constant changing. +const ERROR_REQUEST_TIMED_OUT: i16 = 7; +const ERROR_INVALID_REQUIRED_ACKS: i16 = 21; +const ERROR_UNSUPPORTED_VERSION: i16 = 35; +const ERROR_INVALID_RECORD: i16 = 87; + +/// A live gateway with the topic already provisioned, since Produce creates nothing. +async fn gateway_with_topic(server: &TestServer, partitions: u32) -> GatewayState { + let bridge = IggyBridge::connect(server.test_config()) + .await + .expect("bridge should connect to a ready server"); + bridge + .ensure_stream_and_topic(TOPIC, partitions) + .await + .expect("the topic must exist before producing to it"); + GatewayState::new( + BrokerAdvertise::default(), + Some(Arc::new(bridge)), + MAX_FRAME_SIZE, + false, + ) +} + +fn record(offset: i64, key: Option<&[u8]>, value: &[u8], headers: &[(&str, &[u8])]) -> Record { + Record { + transactional: false, + control: false, + delete_horizon: false, + partition_leader_epoch: NO_PARTITION_LEADER_EPOCH, + producer_id: NO_PRODUCER_ID, + producer_epoch: NO_PRODUCER_EPOCH, + timestamp_type: TimestampType::Creation, + offset, + // Fixed `offset - sequence`, so the encoder writes one batch, as Kafka requires. + sequence: NO_SEQUENCE + i32::try_from(offset).expect("small offset"), + timestamp: CREATE_TIME, + key: key.map(Bytes::copy_from_slice), + value: Some(Bytes::copy_from_slice(value)), + headers: headers + .iter() + .map(|(name, value)| { + ( + StrBytes::from_string((*name).to_string()), + Some(Bytes::copy_from_slice(value)), + ) + }) + .collect::>(), + } +} + +fn batch(records: &[Record], compression: Compression) -> Bytes { + let mut buf = BytesMut::new(); + let options = RecordEncodeOptions { + version: 2, + compression, + }; + RecordBatchEncoder::encode(&mut buf, records.iter(), &options).expect("encode record batch"); + buf.freeze() +} + +/// One partition entry of a request: the index and the records blob it carries. +type Entry = (i32, Bytes); + +/// Builds a request body by hand, at either wire encoding. +/// +/// Not through `kafka_protocol`: the crate is taken with the `broker` feature alone, which +/// encodes responses and decodes requests, neither of the halves a client needs. Hand-built +/// bytes also make these tests exercise the real decoder instead of round-tripping one schema. +fn encode_request(version: i16, acks: i16, topic: &str, entries: &[Entry]) -> Bytes { + encode_timed_request(version, acks, 5_000, topic, entries) +} + +fn encode_timed_request( + version: i16, + acks: i16, + timeout_ms: i32, + topic: &str, + entries: &[Entry], +) -> Bytes { + let flexible = version >= 9; + let mut enc = Encoder::with_capacity(1024); + + if flexible { + enc.write_compact_nullable_string(None); // transactional_id + } else { + enc.write_nullable_string(None).expect("null fits"); + } + enc.write_i16(acks); + enc.write_i32(timeout_ms); + + if flexible { + enc.write_varint(2); // one topic, compact array is N+1 + enc.write_compact_nullable_string(Some(topic)); + enc.write_varint(entries.len() as u64 + 1); + } else { + enc.write_i32(1); + enc.write_nullable_string(Some(topic)).expect("name fits"); + enc.write_i32(i32::try_from(entries.len()).expect("few entries")); + } + + for (index, records) in entries { + enc.write_i32(*index); + if flexible { + enc.write_compact_nullable_bytes(Some(records)); + enc.write_empty_tagged_fields(); + } else { + enc.write_nullable_bytes(Some(records)) + .expect("records fit"); + } + } + if flexible { + enc.write_empty_tagged_fields(); // topic + enc.write_empty_tagged_fields(); // request + } + enc.freeze() +} + +/// Every partition entry of a response, flattened, in the order the response lists them. +fn partition_results(version: i16, body: Bytes) -> Vec<(i32, i16, i64)> { + let flexible = version >= 9; + let mut d = Decoder::new(body); + let topics = read_count(&mut d, flexible); + + let mut results = Vec::new(); + for _ in 0..topics { + read_name(&mut d, flexible); + let partitions = read_count(&mut d, flexible); + for _ in 0..partitions { + let index = d.read_i32().expect("partition index"); + let error_code = d.read_i16().expect("error code"); + let base_offset = d.read_i64().expect("base offset"); + let append_time = d.read_i64().expect("log append time"); + assert_eq!(append_time, -1, "CreateTime topics report no append time"); + if version >= 5 { + d.read_i64().expect("log start offset"); + } + if version >= 8 { + assert_eq!(read_count(&mut d, flexible), 0, "no per-record errors"); + read_name(&mut d, flexible); // error_message + } + if flexible { + d.read_tagged_fields().expect("partition tagged fields"); + } + results.push((index, error_code, base_offset)); + } + if flexible { + d.read_tagged_fields().expect("topic tagged fields"); + } + } + + assert_eq!(d.read_i32().expect("throttle time"), 0); + if flexible { + d.read_tagged_fields().expect("response tagged fields"); + } + assert_eq!(d.remaining(), 0, "the response must decode exactly"); + results +} + +fn read_count(d: &mut Decoder, flexible: bool) -> usize { + if flexible { + let raw = d.read_varint().expect("compact array length"); + usize::try_from(raw.saturating_sub(1)).expect("array length fits") + } else { + usize::try_from(d.read_i32().expect("array length")).expect("array length fits") + } +} + +fn read_name(d: &mut Decoder, flexible: bool) -> Option { + if flexible { + d.read_compact_nullable_string().expect("compact string") + } else { + d.read_nullable_string().expect("string") + } +} + +/// Sends one request through the handler and returns the partition entries it answered with. +async fn produce( + state: &GatewayState, + version: i16, + acks: i16, + topic: &str, + entries: &[Entry], +) -> Vec<(i32, i16, i64)> { + let body = encode_request(version, acks, topic, entries); + let response = handle_request_bounded(state, API_KEY_PRODUCE, version, body) + .await + .expect_response("a Produce request with acks != 0 must be answered"); + partition_results(version, response) +} + +async fn stored(server: &TestServer, partition: u32, count: u32) -> Vec { + raw_client(server) + .await + .poll_messages( + &Identifier::named(STREAM).expect("stream name"), + &Identifier::named(TOPIC).expect("topic name"), + Some(partition), + &Consumer::new(Identifier::named("reader").expect("consumer name")), + &PollingStrategy::offset(0), + count, + false, + ) + .await + .expect("poll the partition back") + .messages +} + +#[tokio::test] +#[serial] +async fn given_a_produce_request_when_handled_should_store_every_record_it_carried() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let records = [ + record(0, Some(b"k1"), b"first", &[("trace", b"abc")]), + record(1, None, b"second", &[]), + ]; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)], + "a two-record batch lands at offset 0 and reports it" + ); + + let messages = stored(&server, 0, 10).await; + assert_eq!( + messages.len(), + 2, + "one Kafka record became one Iggy message" + ); + assert_eq!(messages[0].payload.as_ref(), b"first"); + assert_eq!(messages[1].payload.as_ref(), b"second"); + + // Once on the raw stored shape, since a round trip through the codec proves nothing about + // the shape itself. + let headers = messages[0] + .user_headers_map() + .expect("headers must parse") + .expect("a gateway-written message always carries headers"); + assert_eq!( + headers + .get(&HeaderKey::try_from(VERSION_HEADER).unwrap()) + .map(|value| value.as_bytes().to_vec()), + Some(vec![MAPPING_VERSION]), + "the provenance header marks this message as gateway-written" + ); + assert_eq!( + headers + .get(&HeaderKey::try_from(KEY_HEADER).unwrap()) + .map(|value| value.as_bytes().to_vec()), + Some(b"k1".to_vec()), + "the Kafka key is stored beside the value, not inside it" + ); + + let back = from_iggy(&messages[0], 0).expect("stored message must decode as a record"); + assert_eq!(back.value.as_deref(), Some(&b"first"[..])); + assert_eq!(back.timestamp, CREATE_TIME, "CreateTime survives the store"); + assert_eq!( + back.headers + .get(&StrBytes::from_static_str("trace")) + .and_then(Option::as_deref), + Some(&b"abc"[..]) + ); +} + +#[tokio::test] +#[serial] +async fn given_a_flexible_produce_request_when_handled_should_store_its_records() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let records = [record(0, Some(b"k"), b"flexible", &[])]; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 9, -1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)], + "the compact and tagged-field encoding reaches the same write path" + ); + assert_eq!(stored(&server, 0, 10).await.len(), 1); +} + +// Multi-thread, so the compressed partition plans through `block_in_place`. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn given_a_gzip_batch_when_handled_should_store_its_records() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let records = [record(0, None, b"compressed", &[])]; + let entries = [(0, batch(&records, Compression::Gzip))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)] + ); + assert_eq!( + stored(&server, 0, 10).await[0].payload.as_ref(), + b"compressed" + ); +} + +#[tokio::test] +#[serial] +async fn given_two_batches_in_one_blob_when_handled_should_answer_invalid_record() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + // Kafka allows one batch per partition from v3. + let mut blob = BytesMut::new(); + blob.extend_from_slice(&batch(&[record(0, None, b"a", &[])], Compression::None)); + blob.extend_from_slice(&batch(&[record(0, None, b"b", &[])], Compression::None)); + let entries = [(0, blob.freeze())]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_INVALID_RECORD, -1)] + ); + assert!(stored(&server, 0, 10).await.is_empty()); +} + +#[tokio::test] +#[serial] +async fn given_timestamps_72_minutes_apart_when_handled_should_store_both() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + // One Iggy send spans at most u32::MAX us, about 71.6 min. + let later = CREATE_TIME + 72 * 60 * 1000; + let mut records = [record(0, None, b"a", &[]), record(1, None, b"b", &[])]; + records[1].timestamp = later; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)] + ); + let messages = stored(&server, 0, 10).await; + assert_eq!(messages.len(), 2); + let start = u64::try_from(CREATE_TIME * 1000).expect("positive"); + assert_eq!( + messages[1].header.origin_timestamp, + start + u64::from(u32::MAX), + "clamped into the first record's window, so one send holds both" + ); + let back = from_iggy(&messages[1], 1).expect("stored message must decode as a record"); + assert_eq!(back.timestamp, later, "kafka.ts keeps the real one"); +} + +#[tokio::test] +#[serial] +async fn given_no_timestamp_among_stamped_records_when_handled_should_store_one_send() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let mut records = [record(0, None, b"a", &[]), record(1, None, b"b", &[])]; + records[1].timestamp = -1; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)] + ); + let messages = stored(&server, 0, 10).await; + assert_eq!(messages.len(), 2); + assert_eq!( + messages[1].header.origin_timestamp, messages[0].header.origin_timestamp, + "clamped to the window start, not stored as 0" + ); + let back = from_iggy(&messages[1], 1).expect("stored message must decode as a record"); + let server_millis = i64::try_from(messages[1].header.timestamp / 1000).expect("fits"); + assert_eq!( + back.timestamp, server_millis, + "no timestamp reads as the server's" + ); +} + +#[tokio::test] +#[serial] +async fn given_a_second_request_when_handled_should_report_where_it_landed() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let first = [record(0, None, b"a", &[]), record(1, None, b"b", &[])]; + let second = [record(0, None, b"c", &[])]; + + assert_eq!( + produce( + &state, + 3, + 1, + TOPIC, + &[(0, batch(&first, Compression::None))] + ) + .await, + vec![(0, ERROR_NONE, 0)] + ); + assert_eq!( + produce( + &state, + 3, + 1, + TOPIC, + &[(0, batch(&second, Compression::None))] + ) + .await, + vec![(0, ERROR_NONE, 2)], + "the base offset comes from the send confirmation, not from a count this handler kept" + ); + assert_eq!(stored(&server, 0, 10).await.len(), 3); +} + +#[tokio::test] +#[serial] +async fn given_a_partition_the_topic_lacks_when_handled_should_keep_the_other_answers() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 2).await; + + let blob = batch(&[record(0, None, b"v", &[])], Compression::None); + let entries = [(0, blob.clone()), (9, blob.clone()), (1, blob)]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![ + (0, ERROR_NONE, 0), + (9, ERROR_UNKNOWN_TOPIC_OR_PARTITION, -1), + (1, ERROR_NONE, 0), + ], + "one partition failing costs the others nothing, and a failure names no offset" + ); + assert_eq!(stored(&server, 0, 10).await.len(), 1); + assert_eq!(stored(&server, 1, 10).await.len(), 1); +} + +#[tokio::test] +#[serial] +async fn given_an_unknown_topic_when_handled_should_answer_unknown_topic_or_partition() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let entries = [(0, batch(&[record(0, None, b"v", &[])], Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, "never-created", &entries).await, + vec![(0, ERROR_UNKNOWN_TOPIC_OR_PARTITION, -1)], + "Produce provisions nothing: auto-creation belongs to Metadata" + ); + + let topics = raw_client(&server) + .await + .get_topics(&Identifier::named(STREAM).expect("stream name")) + .await + .expect("list the topics the stream holds"); + assert_eq!( + topics.len(), + 1, + "the refused topic must not have been created on the way" + ); +} + +#[tokio::test] +#[serial] +async fn given_acks_zero_when_handled_should_write_the_records_and_answer_nothing() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let entries = [( + 0, + batch( + &[record(0, None, b"fire-and-forget", &[])], + Compression::None, + ), + )]; + let body = encode_request(3, 0, TOPIC, &entries); + + let outcome = handle_request_bounded(&state, API_KEY_PRODUCE, 3, body).await; + assert!( + outcome.is_no_response(), + "the wire protocol forbids answering an acks=0 request" + ); + + let messages = stored(&server, 0, 10).await; + assert_eq!( + messages.len(), + 1, + "answering nothing is not the same as storing nothing" + ); + assert_eq!(messages[0].payload.as_ref(), b"fire-and-forget"); +} + +#[tokio::test] +#[serial] +async fn given_acks_zero_and_a_failing_partition_when_handled_should_close() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let entries = [(0, batch(&[record(0, None, b"v", &[])], Compression::None))]; + let body = encode_request(3, 0, "never-created", &entries); + + let outcome = handle_request_bounded(&state, API_KEY_PRODUCE, 3, body).await; + assert!( + outcome.is_close(), + "Kafka closes so the client sees the loss" + ); +} + +#[tokio::test] +#[serial] +async fn given_produce_v2_acks_zero_with_a_bridge_when_handled_should_close() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + // v0-2 bodies start with `acks`. No v2 codec exists, so the records cannot be stored. + let body = Bytes::from_static(&[0x00, 0x00]); + let outcome = handle_request_bounded(&state, API_KEY_PRODUCE, 2, body).await; + assert!( + outcome.is_close(), + "closing tells the client the records were not stored" + ); +} + +#[tokio::test] +#[serial] +async fn given_a_passed_deadline_when_handled_should_answer_request_timed_out() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let blob = batch(&[record(0, None, b"v", &[])], Compression::None); + let mut entries: Vec = (0..50).map(|_| (0, blob.clone())).collect(); + // Past the deadline this is not decoded, so it answers 7, not 87. + entries.push((0, Bytes::from_static(b"not a batch"))); + let body = encode_timed_request(3, 1, 1, TOPIC, &entries); + + let response = handle_request_bounded(&state, API_KEY_PRODUCE, 3, body) + .await + .expect_response("acks=1 is answered"); + let codes: Vec = partition_results(3, response) + .into_iter() + .map(|(_, code, _)| code) + .collect(); + assert!( + codes + .iter() + .all(|code| matches!(*code, ERROR_NONE | ERROR_REQUEST_TIMED_OUT)) + ); + assert_eq!( + codes.last(), + Some(&ERROR_REQUEST_TIMED_OUT), + "50 sends outlast 1 ms" + ); +} + +#[tokio::test] +#[serial] +async fn given_many_timed_out_requests_when_a_normal_one_follows_should_write_it() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let blob = batch(&[record(0, None, b"v", &[])], Compression::None); + for _ in 0..100 { + let body = encode_timed_request(3, 1, 1, TOPIC, &[(0, blob.clone())]); + handle_request_bounded(&state, API_KEY_PRODUCE, 3, body).await; + } + assert_eq!( + produce(&state, 3, 1, TOPIC, &[(0, blob)]).await[0].1, + ERROR_NONE, + "a send that outlived its request frees its slot" + ); +} + +#[tokio::test] +#[serial] +async fn given_a_stopped_server_when_handled_should_answer_retriable() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + drop(server); + + let entries = [(0, batch(&[record(0, None, b"v", &[])], Compression::None))]; + let [(_, code, _)] = produce(&state, 3, 1, TOPIC, &entries).await[..] else { + panic!("one partition answered"); + }; + assert!( + matches!(code, ERROR_NOT_LEADER_OR_FOLLOWER | ERROR_REQUEST_TIMED_OUT), + "a lost server is retriable, got {code}" + ); +} + +#[tokio::test] +#[serial] +async fn given_an_unknown_acks_value_when_handled_should_answer_invalid_required_acks() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let blob = batch(&[record(0, None, b"v", &[])], Compression::None); + let entries = [(0, blob.clone()), (1, blob)]; + + assert_eq!( + produce(&state, 3, 2, TOPIC, &entries).await, + vec![ + (0, ERROR_INVALID_REQUIRED_ACKS, -1), + (1, ERROR_INVALID_REQUIRED_ACKS, -1), + ] + ); + assert!( + stored(&server, 0, 10).await.is_empty(), + "a request refused before the write must leave the partition empty" + ); +} + +#[tokio::test] +#[serial] +async fn given_a_transactional_batch_when_handled_should_answer_unsupported_version() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let mut records = [record(0, None, b"v", &[])]; + records[0].transactional = true; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_UNSUPPORTED_VERSION, -1)], + "transactions are out of scope, and 35 stops a transactional producer" + ); + assert!( + stored(&server, 0, 10).await.is_empty(), + "a refused batch must not be half-stored" + ); +} + +#[tokio::test] +#[serial] +async fn given_an_idempotent_batch_sent_twice_when_handled_should_store_both_copies() { + let data_dir = tempfile::tempdir().expect("tempdir"); + let server = TestServer::spawn(data_dir.path()).await; + let state = gateway_with_topic(&server, 1).await; + + let mut records = [record(0, None, b"v", &[])]; + records[0].producer_id = 7; + records[0].producer_epoch = 0; + records[0].sequence = 0; + let entries = [(0, batch(&records, Compression::None))]; + + assert_eq!( + produce(&state, 3, -1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 0)], + "a stock Java producer is idempotent, so its batches must land" + ); + assert_eq!( + produce(&state, 3, -1, TOPIC, &entries).await, + vec![(0, ERROR_NONE, 1)], + "the sequence is ignored, so a resend never answers DUPLICATE_SEQUENCE_NUMBER (46)" + ); + assert_eq!( + stored(&server, 0, 10).await.len(), + 2, + "a retry writes twice until the producer pool in IDEMPOTENCE.md lands" + ); +} + +#[tokio::test] +#[serial] +async fn given_no_bridge_when_handled_should_still_answer_the_retriable_stub() { + // Every other suite drives this path, so a regression in it surfaces far from Produce. + // Pinned here, next to the path that replaced it. + let state = GatewayState::stub(BrokerAdvertise::default(), MAX_FRAME_SIZE); + let entries = [(0, batch(&[record(0, None, b"v", &[])], Compression::None))]; + + assert_eq!( + produce(&state, 3, 1, TOPIC, &entries).await, + vec![(0, ERROR_NOT_LEADER_OR_FOLLOWER, -1)], + "without a bridge nothing is stored, so the answer must stay retriable" + ); +} diff --git a/gateways/kafka/tests/response_negative_tests.rs b/gateways/kafka/tests/response_negative_tests.rs index db05f69d47..14750559a2 100644 --- a/gateways/kafka/tests/response_negative_tests.rs +++ b/gateways/kafka/tests/response_negative_tests.rs @@ -233,11 +233,12 @@ fn produce_error_response_v9_uses_flexible_record_errors_shape() { Some(String::new()) ); assert_eq!(d.read_varint().unwrap(), 2); // one partition - assert_eq!(d.read_i32().unwrap(), 0); + assert_eq!(d.read_i32().unwrap(), 0); // partition index assert_eq!(d.read_i16().unwrap(), ERROR_INVALID_REQUEST); - assert_eq!(d.read_i64().unwrap(), 0); - assert_eq!(d.read_i64().unwrap(), -1); - assert_eq!(d.read_i64().unwrap(), 0); + // An error appended nothing, so there is no offset to name and none is invented. + assert_eq!(d.read_i64().unwrap(), -1); // base_offset + assert_eq!(d.read_i64().unwrap(), -1); // log_append_time_ms, CreateTime + assert_eq!(d.read_i64().unwrap(), -1); // log_start_offset, not looked up assert_eq!(d.read_varint().unwrap(), 1); // empty record_errors array assert_eq!(d.read_compact_nullable_string().unwrap(), None); } diff --git a/gateways/kafka/tests/server_e2e_tests.rs b/gateways/kafka/tests/server_e2e_tests.rs index b09a044793..bca20a8af4 100644 --- a/gateways/kafka/tests/server_e2e_tests.rs +++ b/gateways/kafka/tests/server_e2e_tests.rs @@ -172,11 +172,11 @@ async fn e2e_produce_v3_acks_zero_sends_no_response() { } #[tokio::test] -async fn e2e_produce_v3_acks_zero_malformed_topics_sends_no_response() { +async fn e2e_produce_v3_acks_zero_malformed_topics_closes_connection() { let (addr, _shutdown) = spawn_test_server().await; let mut stream = TcpStream::connect(addr).await.expect("connect"); - // acks=0, claims one topic, no topic bytes - decode fails after acks is read. + // acks=0, claims one topic, no topic bytes. let body = build_produce_v3_body(0, 1); let frame = build_request_frame(API_KEY_PRODUCE, 3, 99, Some("review-test"), &body); stream @@ -184,26 +184,10 @@ async fn e2e_produce_v3_acks_zero_malformed_topics_sends_no_response() { .await .expect("write produce acks=0 malformed"); - let response = - read_response_frame_with_timeout(&mut stream, 8 * 1024 * 1024, Duration::from_millis(500)) - .await; - - assert!( - response.is_none(), - "Produce with acks=0 must stay silent even when the body is malformed; got {} bytes", - response.as_ref().map_or(0, Bytes::len) - ); - - // A silence assertion alone cannot distinguish "stayed open and correctly silent" from "the - // server dropped the connection" - both look like no bytes arrive. Probe with a request that - // must answer, so a regression that closes on decode failure fails here instead of passing. - let follow_up = build_request_frame(API_KEY_API_VERSIONS, 1, 100, Some("review-test"), &[]); - stream.write_all(&follow_up).await.expect("follow-up"); - let payload = read_response_frame(&mut stream, 8 * 1024 * 1024).await; assert_eq!( - parse_response_payload(API_KEY_API_VERSIONS, 1, payload).0, - 100, - "connection must stay usable after the silent malformed acks=0 Produce" + read_byte_with_timeout(&mut stream, Duration::from_secs(2)).await, + ByteRead::Closed, + "a Produce that fails to decode must close the connection" ); } diff --git a/gateways/kafka/tests/version_firewall_tests.rs b/gateways/kafka/tests/version_firewall_tests.rs index fb90877d2e..5f3813133f 100644 --- a/gateways/kafka/tests/version_firewall_tests.rs +++ b/gateways/kafka/tests/version_firewall_tests.rs @@ -360,11 +360,8 @@ async fn unsupported_api_keys_close_connection() { // that generic per-scoped-API loop already exercises the exact same fixtures and version ranges. #[tokio::test] -async fn corrupt_produce_body_with_acks_stays_silent() { - // `kafka_protocol` decodes Produce in one shot, so a decode failure never exposes `acks` - // (unlike the pre-migration field-by-field decoder, which could still answer with - // INVALID_REQUEST once it knew acks was nonzero). Every Produce decode failure now stays - // silent regardless of whether acks was readable before the truncation. +async fn corrupt_produce_body_with_acks_closes() { + // `acks` is unknown after a failed decode, so no reply is safe. Kafka closes too. let body = Bytes::from_static(&[ 0xFF, 0xFF, // null transactional_id 0x00, 0x01, // acks = 1 @@ -374,20 +371,18 @@ async fn corrupt_produce_body_with_acks_stays_silent() { assert!( handle_request(API_KEY_PRODUCE, 3, body, &default_broker()) .await - .is_no_response(), - "malformed Produce body must stay silent regardless of acks" + .is_close(), + "malformed Produce body must close regardless of acks" ); } #[tokio::test] -async fn corrupt_produce_body_before_acks_is_silent() { - // Decode fails before acks is read: the client's response expectation is unknowable, and an - // error response could desync an acks=0 fire-and-forget client, so the server stays silent. +async fn corrupt_produce_body_before_acks_closes() { let body = Bytes::from_static(&[0xFF, 0xFF]); // null transactional_id, then EOF let outcome = handle_request(API_KEY_PRODUCE, 3, body, &default_broker()).await; assert!( - outcome.is_no_response(), - "produce decode failure before acks must be silent" + outcome.is_close(), + "produce decode failure before acks must close" ); }