MINOR: fix failing ZooKeeper system tests#10297
Conversation
|
Will need to be cherry-picked to |
|
I'm struggling a bit to understand whether enabled_sasl_mechanisms is supposed to be for the broker or for ZK? It seems like you're treating it like it's for both, but then in a few other places, like admin_client_as_broker_jaas.conf, it seems to be just broker. Or did I miss something here? |
|
Yeah, there never has been a clear delineation between "what SASL mechanism are enabled for Kafka" vs. "what SASL mechanisms are enabled for ZooKeeper". I had to tease this apart for Kafka brokers vs. Kafka Raft controllers (see |
|
Sure, we can refactor this later if it's that easier. Can you add a comment to |
|
@cmccabe All set. I added the comment and also opened https://issues.apache.org/jira/browse/KAFKA-12488 for the refactor. |
ZooKeeper-related system tests in zookeeper_security_upgrade_test.py and zookeeper_tls_test.py broke due to #10199. That patch changed the logic of SecurityConfig.enabled_sasl_mechanisms() to only add the inter-broker SASL mechanism when the inter-broker protocol was SASL_{PLAINTEXT,SSL}. The inter-broker protocol is left to default to PLAINTEXT for the SecurityConfig instance associated with Zookeeper since that value doesn't apply to ZooKeeper, so the default inter-broker SASL mechanism of GSSAPI was not being added into the set returned by enabled_sasl_mechanisms(). This is actually correct -- GSSAPI shouldn't be added since inter-broker communication is a Kafka concept and doesn't apply to ZooKeeper. GSSAPI should be added when ZooKeeper uses it, though -- which is the case in these tests. So the prior patch referred to above uncovered a bug: we were relying on the default inter-broker SASL mechanism to signal that Kerberos was being used by ZooKeeper even though the inter-broker protocol has nothing to do with that determination in such cases. This patch explicitly includes GSSAPI in the list of enabled SASL mechanisms when SASL is enabled for use by ZooKeeper. Reviewers: Colin P. McCabe <cmccabe@apache.org>
Conflicts: * build.gradle: keep `dependencySubstitution` Confluent addition in `resolutionStrategy` and take upstream changes. Commits: * apache-github/trunk: KAFKA-12503: inform threads to resize their cache instead of doing so for them (apache#10356) KAFKA-10697: Remove ProduceResponse.responses (apache#10332) MINOR: Exclude KIP-500.md from rat check (apache#10354) MINOR: Move `configurations.all` to be a child of `allprojects` (apache#10349) MINOR: Remove use of `NoSuchElementException` in `KafkaMetadataLog` (apache#10344) MINOR: Start the broker-to-controller channel for request forwarding (apache#10340) KAFKA-12382: add a README for KIP-500 (apache#10227) MINOR: Fix BaseHashTable sizing (apache#10334) KAFKA-10357: Add setup method to internal topics (apache#10317) MINOR: remove redundant null check when testing specified type (apache#10314) KAFKA-12293: Remove JCenter from buildscript and delete buildscript.gradle KAFKA-12491: Make rocksdb an `api` dependency for `streams` (apache#10341) KAFKA-12454: Add ERROR logging on kafka-log-dirs when given brokerIds do not exist in current kafka cluster (apache#10304) KAFKA-12459; Use property testing library for raft event simulation tests (apache#10323) MINOR: fix failing ZooKeeper system tests (apache#10297) MINOR: fix client_compatibility_features_test.py (apache#10292)
2 participants
ZooKeeper-related system tests in
zookeeper_security_upgrade_test.pyandzookeeper_tls_test.pybroke due to #10199. That patch changed the logic ofSecurityConfig.enabled_sasl_mechanisms()to only add the inter-broker SASL mechanism when the inter-broker protocol wasSASL_{PLAINTEXT,SSL}. The inter-broker protocol is left to default toPLAINTEXTfor theSecurityConfiginstance associated with Zookeeper since that value doesn't apply to ZooKeeper, so the default inter-broker SASL mechanism ofGSSAPIwas not being added into the set returned byenabled_sasl_mechanisms(). This is actually correct --GSSAPIshouldn't be added since inter-broker communication is a Kafka concept and doesn't apply to ZooKeeper.GSSAPIshould be added when ZooKeeper uses it, though -- which is the case in these tests. So the prior patch referred to above uncovered a bug: we were relying on the default inter-broker SASL mechanism to signal that Kerberos was being used by ZooKeeper even though the inter-broker protocol has nothing to do with that determination in such cases. This patch explicitly includesGSSAPIin the list of enabled SASL mechanisms when SASL is enabled for use by ZooKeeper.Committer Checklist (excluded from commit message)