You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/03/10 22:00:28 UTC
[GitHub] [kafka] rondagostino opened a new pull request #10297: MINOR: fix failing ZooKeeper system tests
rondagostino opened a new pull request #10297:
URL: https://github.com/apache/kafka/pull/10297
ZooKeeper-related system tests in `zookeeper_security_upgrade_test.py` and `zookeeper_tls_test.py` broke due to https://github.com/apache/kafka/pull/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.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino edited a comment on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
rondagostino edited a comment on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-800676314
@cmccabe All set. I added the comment and also opened https://issues.apache.org/jira/browse/KAFKA-12488 for the refactor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
cmccabe merged pull request #10297:
URL: https://github.com/apache/kafka/pull/10297
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-800676314
@cmccabe Al set. I added the comment and also opened https://issues.apache.org/jira/browse/KAFKA-12488 for the refactor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-800653669
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 `serves_raft_sasl` and `uses_raft_sasl`). The same kind of teasing apart could be done for Kafka vs. Zookeeper as well. Perhaps we can open a ticket for this and leave it for another time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-796196452
Will need to be cherry-picked to `2.8`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-800668936
Sure, we can refactor this later if it's that easier.
Can you add a comment to `SecurityConfig#enabled_sasl_mechanisms` describing what it's supposed to return, though? If it should return every possible sasl mechanism in use (zk, controller, broker, client) then let's document that
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #10297: MINOR: fix failing ZooKeeper system tests
Posted by GitBox <gi...@apache.org>.
cmccabe commented on pull request #10297:
URL: https://github.com/apache/kafka/pull/10297#issuecomment-800646841
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org