Hi Dong, What I meant was "Right now Kafka just extracts the name out of the Principal that is generated by the PrincipalBuilder. Instead of doing that if it preserves the Principal itself, this issue can be addressed".
May be I should have used the word "preserve" instead of "stores". I have updated the wording in the KIP. Thanks, Mayuresh On Wed, Feb 1, 2017 at 8:30 PM, Dong Lin <lindon...@gmail.com> wrote: > 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 > > > -- -Regards, Mayuresh R. Gharat (862) 250-7125
