The last paragraph of the motivation section is a bit confusing. I guess you want to say "This issue can be addressed if the Session class stores the Principal object extracted from a request".
I like the approach of changing Session class to be case class *Session(principal: KafkaPrincipal, clientAddress: InetAddress)* under the assumption that the Session class doesn't really need principalType of the KafkaPrincipal. I am wondering if anyone in the open source mailing list knows why we need to have principalType in KafkaPrincipal. For the record, I actually prefer that we use the existing configure() to provide properties to PrincipalBuilder instead of adding the method *buildPrincipal(Map<String, ?> principalConfigs)* in the PrincipalBuilder interface. But this is not a blocking issue for me. On Wed, Feb 1, 2017 at 2:54 PM, Mayuresh Gharat <gharatmayures...@gmail.com> wrote: > Hi All, > > I have updated the KIP as per our discussion here. > It would be great if you can take another look and let me know if there are > any concerns. > > Thanks, > > Mayuresh > > On Sat, Jan 28, 2017 at 6:10 PM, Mayuresh Gharat < > gharatmayures...@gmail.com > > wrote: > > > I had offline discussions with Joel, Dong and Radai. > > > > I agree that we can replace the KafkaPrincipal in Session with the > > ChannelPrincipal. > > KafkaPrincipal can be provided as an out of box implementation. > > > > The only gotcha will be users will have to implement there own > Authorizer, > > if they decide to use there own PrincipalBuilder in kafka-acls.sh. > > > > I will update the KIP accordingly. > > > > Thanks, > > > > Mayuresh > > > > On Thu, Jan 26, 2017 at 6:01 PM, Mayuresh Gharat < > > gharatmayures...@gmail.com> wrote: > > > >> Hi Dong, > >> > >> Thanks for the review. Please see the replies inline. > >> > >> > >> 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. > >> ---------> I am not sure if I understand the question. > >> buildPrincipal(Map<String, ?> principalConfigs) will be used to build > >> individual Principals from the passed in configs. Each Principal can be > >> different type and the PrincipalBuilder is responsible for handling > those > >> configs correctly and build those Principals. > >> > >> 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. > >> ---------> Doing this might be backwards incompatible as we need to > >> preserve the existing behavior of kafka-acls.sh. Also as we have field > of > >> PrincipalType which can be used in future if Kafka decides to support > >> different Principal types (currently it just says "User"), we might > loose > >> that functionality. > >> > >> Thanks, > >> > >> Mayuresh > >> > >> > >> On Tue, Jan 24, 2017 at 3:35 PM, Dong Lin <lindon...@gmail.com> wrote: > >> > >>> 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 > >>> > > >>> > >> > >> > >> > >> -- > >> -Regards, > >> Mayuresh R. Gharat > >> (862) 250-7125 > >> > > > > > > > > -- > > -Regards, > > Mayuresh R. Gharat > > (862) 250-7125 > > > > > > -- > -Regards, > Mayuresh R. Gharat > (862) 250-7125 >
