[GitHub] [kafka] cmccabe commented on pull request #10550: MINOR: Add support for ZK Authorizer with KRaft

[GitBox]

cmccabe commented on pull request #10550:
URL: https://github.com/apache/kafka/pull/10550#issuecomment-839281487


   Thanks for working on this, @rondagostino , and sorry about the delays in 
reviewing.
   
   > We forward the Create/Remove operations to the controller, but this patch 
actually short-circuits that if we are using KRaft with the ZooKeeper-based 
`AclAuthorizer` via the changes to `RaftSupport.maybeForward()`. The reason for 
short-circuiting it is because the KRaft controller doesn't have the code to 
create or remove ACLs (`handle{Create,Delete}Acls` in `KafkaApis`). We could 
add it, of course, in which case the changes to the `maybeForward()` method 
would be unnecessary. Perhaps it would be simpler to do that instead of 
delaying it to an additional PR -- is that what you were suggesting?
   
   Yes, I think we should just do this in `ControllerApis.scala` and be done 
with it.  It's kind of annoying to do in a follow-on PR since we'd have to add 
a lot of special-case code which we'd later have to undo, which is not usually 
the right way to go.  We might even be able to move this code into 
`RequestHandlerHelper` or something since it would be the same between 
`BrokerApis` and `ControllerApis` (I think?)


-- 
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:
us...@infra.apache.org

[GitHub] [kafka] cmccabe commented on pull request #10550: MINOR: Add support for ZK Authorizer with KRaft

[GitBox]

cmccabe commented on pull request #10550:
URL: https://github.com/apache/kafka/pull/10550#issuecomment-822694203


   Thanks for tackling this.
   
   Please keep in mind that this is a plugin architecture.  The ZK-based 
authorizer shouldn't get any special treatment, nor should its setup be done 
outside its own code.  The code and logic for accessing ZooKeeper for ACLs 
needs to be contained just in the AclAuthorizer, not everywhere in the code.
   
   For example, KafkaRaftServer does not need to know anything about ZooKeeper 
or AclAuthorizer specifically.  Maybe KafkaRaftServer's constructor needs to 
take an Authorizer object as an argument.  (Although it's not even clear to me 
that that is true, since it seems like all authorization happens in KafkaApis 
and ControllerApis.)  But certainly KafkaRaftServer does not need to be looking 
at `config.zkConnect` or messing around with ZkClient.
   
   Whatever setup the zookeeper authorizer needs to do should be done in its 
`start` function.  So creating the relevant znodes, etc.  We probably need to 
continue doing that setup in KafkaServer.scala as well, since not all users 
will be using KafkaServer + AclAuthorizer (again, plugin architecture, they 
could use using one but not the other.)
   
   A separate issue is that we need to start forwarding the ACL operations to 
the controller.  You did one half of that work here (adding "controller" to the 
message JSONs) but not the other (supporting these calls on the 
controller-side).  If it's easier, we could probably do this in a follow-up 
JIRA.  However, we do need to do it before the bridge release.


-- 
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:
us...@infra.apache.org