Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sarama and kafkajs drivers #1690

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Fix sarama and kafkajs drivers #1690

merged 3 commits into from
Jul 16, 2024

Conversation

rukai
Copy link
Member

@rukai rukai commented Jul 15, 2024

The exact difference in behaviour is that all the existing drivers will always send an ApiVersions request as the first request when opening a new connection.
However the sarama and kafkajs driver will sometimes skip the ApiVersions request when opening a connection. This is valid use of the protocol and its a bug in shotover that we are not handling this case properly.

Skipping the ApiVersions request resulted in shotover attempting to open the first outgoing connection of an incoming connection using a delegation token, this failed since we havent yet gathered the username required for creating a delegation token.

The fix here is to only use token auth when OriginalScramState::AuthSuccess.
If we are in any other state we should perform no handshake.

Along with the fix, this PR also refactors AuthorizeScramOverMtls to make it impossible to retrieve the username until a successful auth has completed.
While this is impossible to reach due to the above fix, I believe the security in depth is valuable here.
Additionally this refactor enables us to prefetch tokens which should be a reasonable perf gain on production systems with many nodes.

The first commit introduces a failing test case cluster_sasl_scram_over_mtls_nodejs which is fixed by the 2nd commit.
The kafkajs tests are added as a simple smoke test for now.
However, in the future we should be able to extend this into:

  • A generic nodejs+rust interop crate with interop performed by sending js strings over IPC sockets to be eval'd on the javascript side and the return value JSON'd and sent back over the socket.
    • The current use of eval to create the kafka config is a small demonstration of this.
    • stdout should not be used for communication because kafkajs logs things there
  • A full implementation of the kafka connection abstraction for nodejs.

Copy link

codspeed-hq bot commented Jul 15, 2024

CodSpeed Performance Report

Merging #1690 will create unknown performance changes

Comparing rukai:kafka_js (a88befd) with main (ce4a153)

Summary

🆕 39 new benchmarks

Benchmarks breakdown

Benchmark main rukai:kafka_js Change
🆕 cassandra_protect_protected N/A 74.5 µs N/A
🆕 cassandra_protect_unprotected N/A 41.9 µs N/A
🆕 cassandra_request_throttling_unparsed N/A 14.7 µs N/A
🆕 cassandra_rewrite_peers_passthrough N/A 81.4 µs N/A
🆕 loopback N/A 6.5 µs N/A
🆕 nullsink N/A 14.1 µs N/A
🆕 query_counter_fresh N/A 15.5 µs N/A
🆕 query_counter_pre_used N/A 12 µs N/A
🆕 redis_cluster_ports_rewrite N/A 19 µs N/A
🆕 redis_filter N/A 22.3 µs N/A
🆕 decode_system.local_query_v4_lz4_compression N/A 55.1 µs N/A
🆕 decode_system.local_query_v4_no_compression N/A 55.1 µs N/A
🆕 decode_system.local_query_v5_lz4_compression N/A 57 µs N/A
🆕 decode_system.local_query_v5_no_compression N/A 57 µs N/A
🆕 decode_system.local_result_v4_lz4_compression N/A 44.7 µs N/A
🆕 decode_system.local_result_v4_no_compression N/A 14.8 µs N/A
🆕 decode_system.local_result_v5_lz4_compression N/A 44.6 µs N/A
🆕 decode_system.local_result_v5_no_compression N/A 14.8 µs N/A
🆕 encode_system.local_query_v4_lz4_compression N/A 14.2 µs N/A
🆕 encode_system.local_query_v4_no_compression N/A 13.8 µs N/A
... ... ... ... ...

ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.

@rukai rukai force-pushed the kafka_js branch 2 times, most recently from 56dccd8 to 6f8b257 Compare July 15, 2024 06:20
@rukai rukai changed the title Add kafkajs integration test Fix sarama and kafkajs drivers Jul 15, 2024
@rukai rukai marked this pull request as ready for review July 15, 2024 07:19
@rukai rukai merged commit 2d37bfe into shotover:main Jul 16, 2024
41 checks passed
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.

3 participants