Skip to content

feat(protocol): add restrictApiVersion method#3170

Closed
trapped wants to merge 15 commits intoIBM:mainfrom
trapped:protocolbody_restrict_api_version
Closed

feat(protocol): add restrictApiVersion method#3170
trapped wants to merge 15 commits intoIBM:mainfrom
trapped:protocolbody_restrict_api_version

Conversation

@trapped
Copy link
Copy Markdown
Contributor

@trapped trapped commented May 20, 2025

No description provided.

@trapped trapped force-pushed the protocolbody_restrict_api_version branch from 0b04572 to 848aa8d Compare May 20, 2025 07:49
@trapped trapped force-pushed the protocolbody_restrict_api_version branch from c20e053 to 6e673e5 Compare May 27, 2025 07:16
dnwe added 3 commits May 27, 2025 19:20
Think this was just missed in the follow-up commit. Pin ProduceRequest
to V7 with the same min(...) pattern as the rest.

Signed-off-by: Dominic Evans <[email protected]>
@dnwe
Copy link
Copy Markdown
Collaborator

dnwe commented May 28, 2025

Nice, with a few extra commits to account for the updates that were missed, we're impressively getting a fully passing suite :)

@trapped
Copy link
Copy Markdown
Contributor Author

trapped commented May 28, 2025

Oops, my bad - missed those. Thanks for the help!

@trapped
Copy link
Copy Markdown
Contributor Author

trapped commented Jun 7, 2025

@dnwe shall I close #3158 and move forward with this one instead? If so I'll add a few tests to cover the changes

@dnwe
Copy link
Copy Markdown
Collaborator

dnwe commented Jun 9, 2025

@trapped yeah, now it is in a passing state maybe squash the commits on this branch down into a single one and then have a quick think about whether this is the best structuring of this or how we could perhaps:

  1. decide if requiredVersion is still necessary once restrictApiVersion exists
  2. consider storing the "max-implemented" version numbers for each protocol api key in a single location and use those in both requiredVersion (if still required) and restrictApiVersion?
  3. consider whether the logic of restrictApiVersion itself could be centralised? Originally we added it as a func on each protocol type because that's the easiest way to allow it to modify the Version field on the struct, but perhaps the func on each protocol could just be a shim to a generic shared func?

@trapped
Copy link
Copy Markdown
Contributor Author

trapped commented Jun 27, 2025

Hey @dnwe, sorry for the delay, couldn't find time for this until now.

After spending some more time learning the lib's structure I ended up addressing your suggestions and making things a bit neater in the process. To keep things clean I'm closing both the old PR and this one - I've pushed the new changes to #3209 🙂

@trapped trapped closed this Jun 27, 2025
dnwe pushed a commit that referenced this pull request Jul 11, 2025
This PR introduces basic Kafka API version negotiation in Sarama by
enabling the client to restrict the API version for each request based
on the version ranges advertised by brokers in the initial `ApiVersions`
request/response - which is now executed synchronously as part of the
broker connection process, rather than as a fire-and-forget deferred
goroutine, and before SASL authentication takes place (but after
SSL/TLS, as indicated by the [Kafka protocol
docs](https://kafka.apache.org/26/protocol.html#api_versions)).

The selected version is the highest supported by both the broker and the
user-defined `Config.Version` upper bound.

To support this dynamic selection, a new `setVersion(int16)` method has
been added to the `protocolBody` interface, exposing the
request/response structs' `Version` field to be set externally.

Also, it defines constants for message keys, and replaces some (read:
those I found) usages of numeric literals to use the constants instead.

I experimented with extracting more logic from `protocolBody`
implementations, too, such as centralizing `requiredVersion()`,
`isValidVersion()`, and the "max protocol version for Kafka version"
selection in most Request constructors [as suggested by @dnwe in my
previous
PR](#3170 (comment));
however, while they can certainly simplify the code, I found these were
significantly larger-scale changes (and kinda out of scope for the
purpose of this PR), and decided to leave them out to keep this PR short
and easy to review.

<details>
<summary>Motivation behind the change: Sarama incompatibility with
Redpanda</summary>

The latest `github.com/IBM/sarama` version fails to connect to Redpanda
brokers (latest version too):

- Redpanda rejects the request with `Unsupported version 10 for metadata
API`
- sarama fails with `kafka: client has run out of available brokers to
talk to: EOF`

<details>
<summary>Sarama logs</summary>
```
2025/05/13 18:26:29 Initializing new client
2025/05/13 18:26:29 ClientID is the default of 'sarama', you should consider setting it to something application-specific.
2025/05/13 18:26:29 ClientID is the default of 'sarama', you should consider setting it to something application-specific.
2025/05/13 18:26:29 client/metadata fetching metadata for all topics from broker localhost:9092
2025/05/13 18:26:29 Connected to broker at localhost:9092 (unregistered)
2025/05/13 18:26:29 client/metadata got error from broker -1 while fetching metadata: EOF
2025/05/13 18:26:29 Closed connection to broker localhost:9092
2025/05/13 18:26:29 client/metadata no available broker to send metadata request to
2025/05/13 18:26:29 client/brokers resurrecting 1 dead seed brokers
2025/05/13 18:26:29 Error while sending ApiVersionsRequest to broker localhost:9092: kafka: broker not connected
2025/05/13 18:26:30 client/metadata retrying after 250ms... (2 attempts remaining)
2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific.
2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092
2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered)
2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF
2025/05/13 18:26:30 Closed connection to broker localhost:9092
2025/05/13 18:26:30 client/metadata no available broker to send metadata request to
2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers
2025/05/13 18:26:30 client/metadata retrying after 250ms... (1 attempts remaining)
2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific.
2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092
2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered)
2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF
2025/05/13 18:26:30 Closed connection to broker localhost:9092
2025/05/13 18:26:30 client/metadata no available broker to send metadata request to
2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers
2025/05/13 18:26:30 client/metadata retrying after 250ms... (0 attempts remaining)
2025/05/13 18:26:30 ClientID is the default of 'sarama', you should consider setting it to something application-specific.
2025/05/13 18:26:30 client/metadata fetching metadata for all topics from broker localhost:9092
2025/05/13 18:26:30 Connected to broker at localhost:9092 (unregistered)
2025/05/13 18:26:30 client/metadata got error from broker -1 while fetching metadata: EOF
2025/05/13 18:26:30 Closed connection to broker localhost:9092
2025/05/13 18:26:30 client/metadata no available broker to send metadata request to
2025/05/13 18:26:30 client/brokers resurrecting 1 dead seed brokers
2025/05/13 18:26:30 Closing Client
```
</details>

The culprit being simply that:
1. Sarama chooses request versions solely according to the provided
`config.Version`, [even though it sends an
ApiVersionsRequest](https://github.com/IBM/sarama/blob/43f8559f9dc49d7bc46f81adec99aed43d423bf8/broker.go#L189-L201)
4. [MetadataRequest uses versions 8-9-10 for Kafka versions >=
v2.4.0.0](https://github.com/IBM/sarama/blob/43f8559f9dc49d7bc46f81adec99aed43d423bf8/metadata_request.go#L26-L29)
5. [Redpanda only supports versions 0 to
8](https://github.com/redpanda-data/redpanda/blob/bee4479bf54ae2d9af54b07b1203179791b0f166/src/v/kafka/server/handlers/metadata.h#L28),
and it correctly advertises it

</details>

---------

Signed-off-by: Giorgio Pellero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants