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

Feature: Add SASL authentication support #7

Merged
merged 13 commits into from
Aug 26, 2021
Merged

Conversation

victorlcm
Copy link
Contributor

Hi 👋 ,

I've started using this extension as the built in kakfa output is being deprecated. I had a requirement to connect in a topic using SASL authentication, but It was not possible to do that using the current implementation of the kafka output. So, I've added the support in a fork and thought it would be a great thing to add in the extension, as other people might need this 😄

Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Thanks for this PR 🎉 and sorry for the late response.

I haven't tested this functionally as I don't really use kafka 😬 , but maybe @mostafa will like to help on that front.

I would really want you to rebase on the latest master(sorry we have been planning the changes that landed for months) and to have some of the configuration fixed.

Other than that it seems like a solid PR, good job 👍

pkg/kafka/collector.go Outdated Show resolved Hide resolved
pkg/kafka/config.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/kafka/collector.go Show resolved Hide resolved
@victorlcm
Copy link
Contributor Author

Hey @mstoykov, I've implemented the requested changes 😄
Thank you for the review!

@victorlcm victorlcm requested a review from mstoykov July 8, 2021 10:59
go.mod Outdated Show resolved Hide resolved
pkg/kafka/config.go Outdated Show resolved Hide resolved
@mostafa
Copy link
Member

mostafa commented Jul 12, 2021

Hi @victorlcm,

thanks for your contribution. I have chosen kafka-go for my project, xk6-kafka, but the basics are the same. So, I suggest you have look at mostafa/xk6-kafka#3 and specifically the auth.go file. I'll definitely review it after you apply @mstoykov's suggestions.

@victorlcm
Copy link
Contributor Author

Hey @mstoykov , I've finished updating the code reviews and I've added some new extra tests. Do you think we're good to merge this one? Thanks!

pkg/kafka/collector.go Outdated Show resolved Hide resolved
@mstoykov mstoykov requested a review from mostafa August 18, 2021 15:31
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

I have no more comments on the code, but haven't tested it and am still somewhat skeptical of the need for version as it didn't need it so far ... or did it 🤔

@mostafa
Copy link
Member

mostafa commented Aug 19, 2021

Well done @victorlcm!

@mstoykov I'll test it.

@mostafa
Copy link
Member

mostafa commented Aug 19, 2021

I tried to run a test and this is the result:

[appuser@broker /]$ ./k6 run -o xk6-kafka=brokers=localhost:9092.topic=k6,user=kafka,password=kafka,auth_mechanism=scram-sha-512,format=json script.js

          /\      |‾‾| /‾‾/   /‾‾/
     /\  /  \     |  |/  /   /  /
    /  \/    \    |     (   /   ‾‾\
   /          \   |  |\  \ |  (‾)  |
  / __________ \  |__| \__\ \_____/ .io

ERRO[0000] could not create the 'xk6-kafka' output: 3 error(s) decoding:

* 'auth_mechanism' expected a map, got 'string'
* 'password' expected a map, got 'string'
* 'user' expected a map, got 'string'

@victorlcm
Copy link
Contributor Author

Hey @mostafa,

Just fixed it, should be working now 😄

@mostafa
Copy link
Member

mostafa commented Aug 20, 2021

I am using kafka-docker-playground's sasl env and this doesn't work:

./k6 run -o xk6-kafka=brokers=broker:9092,topic=k6,user=client,password=client-secret,auth_mechanism=scram-sha-512,format=json script.js

          /\      |‾‾| /‾‾/   /‾‾/
     /\  /  \     |  |/  /   /  /
    /  \/    \    |     (   /   ‾‾\
   /          \   |  |\  \ |  (‾)  |
  / __________ \  |__| \__\ \_____/ .io

ERRO[0001] could not create the 'xk6-kafka' output: kafka: client has run out of available brokers to talk to (Is your cluster reachable?)

Init      [--------------------------------------]
default   [--------------------------------------]

Kafka logs:

2021-08-20T08:30:17.671978365Z [2021-08-20 08:30:17,671] INFO [SocketServer listenerType=ZK_BROKER, nodeId=1] Failed authentication with /172.21.0.1 (errorMessage=Unexpected Kafka request of type METADATA during SASL handshake.) (org.apache.kafka.common.network.Selector)

@victorlcm
Copy link
Contributor Author

Hey @mostafa,

I've investigated the issue and apparently it's related to the security protocol. The env you're using is PLAINTEXT and not SSL. So, I've changed the default configs to use PLAINTEXT and added a SSL option. The Kafkas broker I was using had SSL enabled, that's why I was not getting this error. Thank you for pointing it out!

@mostafa
Copy link
Member

mostafa commented Aug 20, 2021

This worked and it successfully published messages to Kafka. Yay! 🎉

I'll review the code on Monday.

./k6 run -d 10s -o xk6-kafka=brokers=broker:9092,topic=k6,user=client,password=client-secret,auth_mechanism=scram-sha-256,format=json script.js

          /\      |‾‾| /‾‾/   /‾‾/
     /\  /  \     |  |/  /   /  /
    /  \/    \    |     (   /   ‾‾\
   /          \   |  |\  \ |  (‾)  |
  / __________ \  |__| \__\ \_____/ .io

  execution: local
     script: script.js
     output: kafka: TODO <<<<<< What is this?

  scenarios: (100.00%) 1 scenario, 1 max VUs, 40s max duration (incl. graceful stop):
           * default: 1 looping VUs for 10s (gracefulStop: 30s)


running (10.5s), 0/1 VUs, 9 complete and 0 interrupted iterations
default ✓ [======================================] 1 VUs  10s

     data_received..................: 108 kB 10 kB/s
     data_sent......................: 1.8 kB 171 B/s
     http_req_blocked...............: avg=67.31ms  min=371ns    med=842ns    max=605.84ms p(90)=121.17ms p(95)=363.5ms
     http_req_connecting............: avg=10.65ms  min=0s       med=0s       max=95.9ms   p(90)=19.18ms  p(95)=57.54ms
     http_req_duration..............: avg=99.96ms  min=98.23ms  med=100.16ms max=102.85ms p(90)=101.3ms  p(95)=102.07ms
       { expected_response:true }...: avg=99.96ms  min=98.23ms  med=100.16ms max=102.85ms p(90)=101.3ms  p(95)=102.07ms
     http_req_failed................: 0.00%  ✓ 0        ✗ 9
     http_req_receiving.............: avg=859.72µs min=298.59µs med=426.95µs max=2.73ms   p(90)=1.79ms   p(95)=2.26ms
     http_req_sending...............: avg=233.93µs min=159.89µs med=198.95µs max=493.25µs p(90)=293.31µs p(95)=393.28µs
     http_req_tls_handshaking.......: avg=27.27ms  min=0s       med=0s       max=245.49ms p(90)=49.09ms  p(95)=147.29ms
     http_req_waiting...............: avg=98.87ms  min=97.21ms  med=98.53ms  max=101.1ms  p(90)=100.13ms p(95)=100.62ms
     http_reqs......................: 9      0.855762/s
     iteration_duration.............: avg=1.16s    min=1.09s    med=1.1s     max=1.7s     p(90)=1.22s    p(95)=1.46s
     iterations.....................: 9      0.855762/s
     vus............................: 1      min=1      max=1
     vus_max........................: 1      min=1      max=1

Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

Well done! Left just one comment.

pkg/kafka/collector.go Outdated Show resolved Hide resolved
@victorlcm victorlcm requested a review from mostafa August 24, 2021 16:24
@victorlcm
Copy link
Contributor Author

hey @mstoykov, can we merge this one? 😄

Copy link

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Hi @victorlcm,
thanks for your contribution. Added few comments.

pkg/kafka/collector.go Outdated Show resolved Hide resolved
pkg/kafka/scram_client.go Outdated Show resolved Hide resolved
pkg/kafka/config.go Outdated Show resolved Hide resolved
@victorlcm
Copy link
Contributor Author

Hey @codebien, I've applied your comments 😄

Copy link

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

@mstoykov mstoykov merged commit 5dcf974 into grafana:master Aug 26, 2021
@mstoykov
Copy link
Collaborator

Thanks for the work on this PR @victorlcm 🙇

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.

None yet

4 participants