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 2022/11/09 18:48:34 UTC
[GitHub] [kafka] pprovenzano opened a new pull request, #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
pprovenzano opened a new pull request, #12838:
URL: https://github.com/apache/kafka/pull/12838
Changes needed to get SaslPlainSslEndToEndAuthorizationTest to pass in KRAFT
Removed direct calls in EndToEndAuthorizationTest to zookeeper and instead
use the superuser principal to update the Acls through the brokers. This
removes use of deprecated featuer --authorizer-properties
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12838:
URL: https://github.com/apache/kafka/pull/12838#discussion_r1018304863
##########
core/src/test/scala/integration/kafka/api/SaslPlainSslEndToEndAuthorizationTest.scala:
##########
@@ -147,10 +157,11 @@ class SaslPlainSslEndToEndAuthorizationTest extends SaslEndToEndAuthorizationTes
/**
* Checks that secure paths created by broker and acl paths created by AclCommand
- * have expected ACLs.
+ * have expected ACLs. This only applies for zookeeper
Review Comment:
why does this only work in zk mode?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] pprovenzano commented on pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
pprovenzano commented on PR #12838:
URL: https://github.com/apache/kafka/pull/12838#issuecomment-1309335998
The main focus of these tests is on brokers doing authorization and validating that the client gets (or does not get) access to the topic for various ACLs. So I think that we should not bother to update the CONTROLLER to use something other than PLAINTEXT. That said, for SaslPlainSslEndToEndAuthorizationTest you still need to set a principal so that BROKERS can update the CONTROLLER.
What I have done is set a super user principal which is different from the client and that principal calls the brokers to set ACLs. I believe the BROKER just forwards the request to zookeeper or a CONTROLLER.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] pprovenzano closed pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
pprovenzano closed pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
URL: https://github.com/apache/kafka/pull/12838
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] pprovenzano commented on a diff in pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
pprovenzano commented on code in PR #12838:
URL: https://github.com/apache/kafka/pull/12838#discussion_r1018372760
##########
core/src/test/scala/integration/kafka/api/SaslPlainSslEndToEndAuthorizationTest.scala:
##########
@@ -147,10 +157,11 @@ class SaslPlainSslEndToEndAuthorizationTest extends SaslEndToEndAuthorizationTes
/**
* Checks that secure paths created by broker and acl paths created by AclCommand
- * have expected ACLs.
+ * have expected ACLs. This only applies for zookeeper
Review Comment:
There isn't a zookeeper so while the test would pass, it wouldn't actually test anything.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12838:
URL: https://github.com/apache/kafka/pull/12838#issuecomment-1309232726
Hi @pprovenzano . Thanks for the PR.
Before I can review this I'd like to understand the strategy here. Do you want to leave the `CONTROLLER` endpoint as `PLAINTEXT`? Or will it also be `SASL`? If it is plaintext, then our job will be simpler here, but we are testing less, of course. In general the main focus of the test is on brokers doing authorization, so perhaps the controller being plaintext is OK?
As you noticed, there are some bootstrapping calls to AclCommand. What are we replacing the calls to AclCommand with? Are we going to be modifying the Authorizer objects directly? That is what we have done in other tests. We will need to modify all of the Authorizer objects though (each broker has one)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #12838: [DO NOT MERGE] SaslPlainSslEndToEndAuthorizationTest
Posted by GitBox <gi...@apache.org>.
cmccabe commented on PR #12838:
URL: https://github.com/apache/kafka/pull/12838#issuecomment-1309560732
Rather than putting the `@Timeout(60)` on each test case, I'd suggest putting it on the test class itself, so that it applies to each test.
Actually, can you split out a separate PR for just that change? It would be a good first change for you.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org