Hey Mayuresh, Thanks for the KIP. I actually like the suggestions by Ismael and Jun. Here are my comments:
1. I am not sure we need to add the method buildPrincipal(Map<String, ?> principalConfigs). It seems that user can simply do principalBuilder.configure(...).buildPrincipal(...) without using that method. 2. Is there any reason specific reason that we should put the channelPrincipal in KafkaPrincipal class instead of the Session class? If they work equally well to serve the use-case of this KIP, then it seems better to put this field in the Session class to avoid changing interface that needs to be implemented by custom principal. Dong On Mon, Jan 23, 2017 at 5:55 PM, Mayuresh Gharat <gharatmayures...@gmail.com > wrote: > 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 >
