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
Add support for SSL, SASL_PLAIN, and SASL_SSL listeners in kafka brokers. #28
Conversation
@stanlemon would you mind code reviewing this when you have a moment? |
kafka-junit-core/pom.xml
Outdated
@@ -99,7 +105,7 @@ | |||
</dependency> | |||
</dependencies> | |||
<configuration> | |||
<argLine>-Xmx2048M</argLine> | |||
<argLine>-Xmx5120M</argLine> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a lot...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hah ya, trying to sort out the OOM errors in travis CI. Odd that it runs without issue locally all the way thru the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smells like a memory leak somewhere.
/** | ||
* Defines which listener has been set to be configured on the brokers. | ||
*/ | ||
private BrokerListener registeredListener = new PlainListener(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be final?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disregard, I see now.
* Test a multi-node cluster instance with various listeners. | ||
* @param listeners The listeners to register. | ||
*/ | ||
// @ParameterizedTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this temporary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, more trying to figure out the source of the OOM on travis :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty straightforward.
This PR adds support to configure the test brokers to use SSL, SASL_PLAIN, or SASL_SSL authentication methods. By default if no explicit authentication scheme is defined, it will fall back to the previous behavior of using PLAIN.
While the underlying KafkaTestServer supports being configured with multiple authentication schemes/listeners, the exposed Test resources only support a single instance. I believe additional thought needs to be given around how to expose connection details when multiple listener types are available. Likely this would result in breaking changes to the API, so it's been pushed off until the need for this is required.
Example for configuring SSL enabled brokers
When configuring to use SSL, the end user will need to supply paths to their own trust and key stores, as well as passwords. The ones included in these tests are dummy stores created just to validate behavior programatically, and have no use outside of those tests.
Example for configuring SASL_PLAIN enabled brokers
When configuring to use SASL, because of how Kafka reads in JAAS configuration files via an environment variable, the end user will need to start the JVM with the argument:
-Djava.security.auth.login.config=path/to/jaas.conf
Example for configuring SASL_SSL enabled brokers
SASL_SSL is basically a combination of SSL + SASL_PLAIN and also requires the JVM argument
-Djava.security.auth.login.config=path/to/jaas.conf