Hi Rajini, Thanks a lot for the review. Please see the comments inline :
It feels like the goal is to expose custom Principal as an opaque object between PrincipalBuilder and Authorizer so that Kafka doesn't really need to know anything about additional stuff added for customization. But kafka-acls.sh is expecting a key-value map from which Principal is constructed. This is a breaking change to the PrincipalBuilder interface - and I am not sure what it achieves. -----> kafka-acls is a commandline tool where in currently we just specify the "names" of the principal that are allowed or denied. The Principal generated by PrincipalBuilder is still opaque and Kafka as such does not need to know the details. The key-value map that is been passed in, will be used specifically by the user PrincipalBuilder to create the Principal. The main motivation of the KIP is that, the Principal built by the PrincipalBuilder can have other fields apart from the "name", which are ignored currently. Allowing a key-value pair to be passed in will enable the PrincipalBuilder to create such type of Principal. 1. A custom Principal is (a) created during authentication using custom PrincipalBuilder (b) checked during authorization using Principal.equals() and (c) stored in Zookeeper using Principal.toString(). Is that correct? -----> The authorization will be done as per the user supplied Authorizer. As not everyone might be using zookeeper for storing ACLs, its storage is again Authorizer implementation dependent. 2. Is the reason for the new parameters in kafka-acls.sh and the breaking change in PrincipalBuilder interface to enable users to specify a Principal using properties rather than create the String in 1c) themselves? -----> Please see the explanation above. 3. Since the purpose of the new PrincipalBuilder method buildPrincipal(Map<String, ?> principalConfigs) is to create a new Principal from command line parameters, perhaps Properties or Map<String, String> would be more appropriate? -----> Yes we can, but I actually prefer to keep it similar to configure(Map<String, ?> configs) API. Hi Ismael, Thanks a lot for the review. Please see the comments inline. 1. PrincipalBuilder implements Configurable and gets a map of properties via the `configure` method. Do we really need a new `buildPrincipal` method given that? ------> The configure() API will actually be used to configure the PrincipalBuilder in the same way as the Authorizer. The buildPrincipal() API will be used by the PrincipalBuilder to build individual principals. Each of these principals can be of different custom types like GroupPrincipals, ServicePrincipals and so on, based on the Map<String, ?> principalConfigs provided to the buildPrincipal() API. 2. Jun suggested in the JIRA that it may make sense to pass the `channelPrincipal` as a field in `Session` instead of `KafkaPrincipal`. It would be good to understand why this was rejected. -----> Now I understand what Jun meant by "Perhaps, we could extend the Session object with channelPrincipal instead.". Actually thinking more on this, there is a PrincipalType in KafkaPrincipal, that was inserted for a specific purpose when it was created for the first time, I think. I thought that we should preserve it, if its useful for future. Thanks, Mayuresh On Mon, Jan 23, 2017 at 8:56 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Mayuresh, > > Thanks for updating the KIP. A couple of questions: > > 1. PrincipalBuilder implements Configurable and gets a map of properties > via the `configure` method. Do we really need a new `buildPrincipal` method > given that? > > 2. Jun suggested in the JIRA that it may make sense to pass the > `channelPrincipal` as a field in `Session` instead of `KafkaPrincipal`. It > would be good to understand why this was rejected. > > Ismael > > On Thu, Jan 12, 2017 at 7:07 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi Mayuresh, > > > > Thanks for the KIP. A quick comment before I do a more detailed analysis, > > the KIP says: > > > > `This KIP is a pure addition to existing functionality and does not > > include any backward incompatible changes.` > > > > However, the KIP is proposing the addition of a method to the > > PrincipalBuilder pluggable interface, which is not a compatible change. > > Existing implementations would no longer compile, for example. It would > be > > good to make this clear in the KIP. > > > > Ismael > > > > On Thu, Jan 12, 2017 at 5:44 PM, Mayuresh Gharat < > > gharatmayures...@gmail.com> wrote: > > > >> Hi all. > >> > >> We created KIP-111 to propose that Kafka should preserve the Principal > >> generated by the PrincipalBuilder while processing the request received > on > >> socket channel, on the broker. > >> > >> Please find the KIP wiki in the link > >> https://cwiki.apache.org/confluence/pages/viewpage. > action?pageId=67638388 > >> . > >> We would love to hear your comments and suggestions. > >> > >> > >> Thanks, > >> > >> Mayuresh > >> > > > > > -- -Regards, Mayuresh R. Gharat (862) 250-7125
