KAFKA-2693: Ducktape tests for SASL/PLAIN and multiple mechanisms#1282
KAFKA-2693: Ducktape tests for SASL/PLAIN and multiple mechanisms#1282rajinisivaram wants to merge 3 commits into
Conversation
rajinisivaram
force-pushed
the
KAFKA-2693
branch
from
7d050b8 to
3b852ec
Compare
| security_config=self.security_config, | ||
| interbroker_security_protocol=self.interbroker_security_protocol, | ||
| sasl_mechanism=self.sasl_mechanism) | ||
| interbroker_security_protocol=self.interbroker_security_protocol) |
There was a problem hiding this comment.
Can we remove the use of self.interbroker_security_protocol here as well, and just get that info out of the security_config. Seems much better to rely on just the SecurityConfig instance.
| def test_replication_with_broker_failure(self, failure_mode, security_protocol, broker_type): | ||
| @matrix(failure_mode=["clean_shutdown", "hard_bounce"], | ||
| broker_type=["leader"], | ||
| security_protocol=["SASL_SSL"], client_sasl_mechanism=["PLAIN"], interbroker_sasl_mechanism=["PLAIN", "GSSAPI"]) |
There was a problem hiding this comment.
Do we need GSSAPI for interbroker_sasl_mechanism? Don't we already test that via the other matrix annotation?
There was a problem hiding this comment.
@ijuma This tests (client=PLAIN, interbroker=PLAIN) and (client=PLAIN, interbroker=GSSAPI). The first tests a broker with just PLAIN and the second tests a broker with multiple mechanisms.
There was a problem hiding this comment.
Because these tests take a while to run, maybe we should just have the multiple mechanisms case. What do you think @ewencp?
There was a problem hiding this comment.
@ijuma The second case (multiple mechanisms) uses GSSAPI for interbroker communication. That leaves no tests with PLAIN for inter-broker (sanity test runs only one broker, so that doesn't test as much as this one).
There was a problem hiding this comment.
@rajinisivaram Do we need to cover all the cases here, though? Replication tests are pretty heavyweight. If we just want sanity checks on different protocol settings, we could also try to get coverage via other, cheaper tests?
I'm fine with the additional tests if we need them to get coverage, I just want to make sure we don't keep increasing the number of variants of expensive tests.
There was a problem hiding this comment.
@ewencp I dont actually know the answer. I chose one sanity test with a single broker (consumer test) and the replication tests because they restart brokers making them useful for testing authentication. I chose two types (PLAIN, multi-mechanism) and two types of tests (clean_bounce, hard_shutdown), that is 4 tests in all. I could reduce to just hard_shutdown (2 tests) or just SASL/PLAIN with hard_shutdown (1 test). Or just do upgrade tests since they do restart as well. Suggestions welcome :-)
There was a problem hiding this comment.
@ewencp Reduced to running just hard_bounce. Two runs, once with SASL/PLAIN and once with multiple mechanisms.
|
This seems fine to me, but I'll wait for @ewencp to take a look as well. |
|
LGTM |
Run a sanity test with SASL/PLAIN and a couple of replication tests with SASL/PLAIN and multiple mechanisms. Author: Rajini Sivaram <rajinisivaram@googlemail.com> Reviewers: Ismael Juma <ismael@juma.me.uk>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#1282 from rajinisivaram/KAFKA-2693
3 participants
Run a sanity test with SASL/PLAIN and a couple of replication tests with SASL/PLAIN and multiple mechanisms.