Ashish, For the two benefits that you listed, fail fast can be done just in the implementation w/o getSupportedPrincipalTypes(). For avoiding the same check in all implementations, I think Grant is thinking of adding some ACL RPC request/response btw the client and broker directly, instead of writing the ACL to ZK directly in the future. If we do that, the right place to do any sanity check is probably in each implementation instead of in AclCommand since AclCommand won't necessarily be the only entry point for changing ACLs. So, I am still not quite convinced that we should getSupportedPrincipalTypes() right now. It's easy to add a new method to the interface, but hard to remove/change an existing method. Grant, could you comment on the latter?
Thanks, Jun On Sat, Apr 2, 2016 at 11:43 AM, Ashish Singh <asi...@cloudera.com> wrote: > Hello Jun, > > > On Fri, Apr 1, 2016 at 9:57 PM, Jun Rao <j...@confluent.io> wrote: > > > Ashish, > > > > Thanks for the KIP. > > > > It seems that a specific implementation of Authorizer can reject invalid > > user type in addAcl/removeAcl without needing the new > > getSupportedPrincipalTypes() > > method, right? It's probably useful to provide the supported user types > as > > information through CLI (e.g., when --help is specified). Then, there may > > other information that a specific authorizer may want to provide. So, if > > this is just informational, would it be better to add sth like > > getDescription() in the Authorizer interface and expose that through CLI? > > > Providing information is definitely an important reason, some other reasons > were to fail fast and to avoid same check in all implementations. I agree > having a generic getDescription() will be handy for authorizer > implementations to provide more implementation specific info, including > supported principal types and more. However, do you think other two reasons > I mentioned can convince you for current proposal? > > > > > Jun > > > > On Wed, Mar 30, 2016 at 11:02 AM, Ashish Singh <asi...@cloudera.com> > > wrote: > > > > > Hi Guys, > > > > > > I would like to open the vote for KIP-50. > > > > > > KIP: > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-50+-+Enhance+Authorizer+interface+to+be+aware+of+supported+Principal+Types > > > > > > Discuss thread: here > > > < > > > > > > http://mail-archives.apache.org/mod_mbox/kafka-dev/201603.mbox/%3CCAGQG9cUCLDO0owdziDcL9iStXNF1wURyVNcEZedQJg%3DUuC7j%3DQ%40mail.gmail.com%3E > > > > > > > > > > JIRA: https://issues.apache.org/jira/browse/KAFKA-3186 > > > > > > PR: https://github.com/apache/kafka/pull/861 > > > > > > -- > > > > > > Regards, > > > Ashish > > > > > > > > > -- > > Regards, > Ashish >
