Add support for connecting to SASL authenticated Kafka clusters. #109
Conversation
|
Really interested in this! When could we see a docker image. I'm happy to do some testing if this may help. |
|
Hey @OlivierTarnus ! I'd love to get you to play around with it and provide some feedback. Right now the easiest way would be to build and run the source code. You can do that by checking out the source code in this branch and running the If you absolutely need docker, I can likely cut a pre-release build that works on docker in another day or two? Thanks! |
| @@ -281,6 +301,46 @@ public String clusterUpdate( | |||
| cluster.setTrustStorePassword(null); | |||
| } | |||
|
|
|||
| // If sasl is enabled | |||
| if (clusterForm.getSasl()) { | |||
There was a problem hiding this comment.
$.02 If you move the stuff up from below the else, you can probably figure out a way to short circuit here and remove a level of conditional nesting. Generally speaking this method stinks, it's big and hard to follow.
| throw new IllegalArgumentException("ConsumerIdPrefix must be configured!"); | ||
| public KafkaConsumerFactory(final KafkaClientConfigUtil configUtil) { | ||
| if (configUtil == null) { | ||
| throw new RuntimeException("Missing dependency KafkaClientConfigUtil!"); |
There was a problem hiding this comment.
Might consider a more specific exception that extends Runtime here.
| /** | ||
| * Builder class for SaslProperties. | ||
| */ | ||
| public static final class Builder { |
There was a problem hiding this comment.
Any reason not to split this out into it's own class available only to the package?
| @@ -0,0 +1,109 @@ | |||
|
|
|||
| create table if not exists `user` ( | |||
There was a problem hiding this comment.
Is this the flyway stuff? Does this get autogenerated or do you write this?
One note, for consistency sake you might want to match the casing on the other statements.
| assertNotNull("Should have non-null node result", nodes); | ||
| assertFalse("Should have non-empty node", nodes.isEmpty()); | ||
| } | ||
| //assertFalse("Finish writing this test", true); |
Hi, |
|
I've done manual testing and added automated tests over SSL, SASL_PLAIN, and SASL_SSL. I haven't dug into Kerberos yet as I'll have to do some additional research to figure out how to set up a cluster configured with it. @OlivierTarnus If you're able to easily test/verify that, it'd be super helpful. Otherwise I'll try to get into it soon. |
Crim
changed the title
Crim
changed the title
|
Hi @Crim, just wanted to chime in and say that I tested this branch agains an Event Streams Kafka cluster deployed on IBM Cloud which uses SASL_SSL and everything seems to be working. Thanks for implementing this! |
|
If there's no outstanding issues with this, I'll merge this in tomorrow and cut a new release. Thanks for the help from everyone :) |
SASL Plain Support
This PR adds the ability to configure SASL authentication for clusters. It should support both PLAIN and GSSAPI authentication mechanisms.
TODO
for #105 and #108