Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Upgrade to kafka-clients 3.4.0 #57

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

robobario
Copy link
Contributor

@robobario robobario commented May 18, 2023

This is an enabler for integration with the kroxylicious proxy framework because it depends on kafka-clients too, causing problems at runtime if we try to call the EncryptionModule built against an older kafka-clients.

Note: topic-encryption with the vertx-proxy will still require the proxied Kafka cluster or client to be less that version 3.1.0 because it does not yet work with topic ids in the FetchResponse (see #17 ).

This is an enabler for integration with the kroxylicious proxy
framework because it depends on kafka-clients too, causing problems
at runtime if we try to call the EncryptionModule built against an
older kafka-clients.

Note: topic-encryption with the vertx-proxy will still requires the
proxied Kafka cluster or client to be less that version 3.1.0
because it does not yet work with topic ids in the FetchResponse.

Signed-off-by: Robert Young <[email protected]>
@tombentley
Copy link
Member

Note: topic-encryption with the vertx-proxy will still require the proxied Kafka cluster or client to be less that version 3.1.0 because it does not yet work with topic ids in the FetchResponse (see #17 ).

The solution to that is to intercept the ApiVersionsResponse, rewriting the version of the fetch api to use something pre-topic id.

@tombentley
Copy link
Member

@chris-giblin could you take a look?

@chris-giblin
Copy link
Contributor

@robobario Were you able to test these changes with older clients to verify topic encryption with names still works?

@robobario
Copy link
Contributor Author

@robobario Were you able to test these changes with older clients to verify topic encryption with names still works?

I tested it with the vertx-proxy in front of a 3.4.0 broker, and observed that decryption did not work for a 3.4.0 consumer and did work with a 3.0.2 consumer. Happy to check other combinations (old broker versions) on Monday.

@chris-giblin
Copy link
Contributor

chris-giblin commented May 19, 2023

Thanks, testing with 3.0.2 suffices for me to verify the behavior for older versions is unchanged. It is nice that the kafka level has been brought up to a recent level with this PR. Thanks!

This change however does necessitate implementing the version interception referred to by Tom, above. This is a good time for me to address #17.

OK with me to merge this.

@tombentley
Copy link
Member

@chris-giblin can you approve in code review so we can merge (you're a https://github.com/strimzi/governance/blob/main/COMPONENT-OWNERS and I'm a maintainer, and we need two such approvals to merge). Thanks!

@chris-giblin chris-giblin self-requested a review May 19, 2023 11:34
Copy link
Contributor

@chris-giblin chris-giblin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@k-wall
Copy link

k-wall commented Oct 3, 2023

@tombentley @robobario is there anything stopping this being merged? I know the client is out of date already, but it still makes sense to take this.

@tombentley tombentley merged commit e863aff into strimzi:main Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants