Hi All, If there are no more concerns, I will like to start vote for this KIP.
Thanks, Mayuresh On Wed, Feb 1, 2017 at 8:38 PM, Mayuresh Gharat <gharatmayures...@gmail.com> wrote: > 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 > -- -Regards, Mayuresh R. Gharat (862) 250-7125
