Cool. I will upload a patch for this. Thanks,
Mayuresh On Fri, May 27, 2016 at 12:35 AM, Harsha <ka...@harsha.io> wrote: > Mayuresh & Ismael, > Agree on not breaking interfaces on public API. > +1 on option 2. > Thanks, > Harsha > > On Mon, May 23, 2016, at 10:30 AM, Mayuresh Gharat wrote: > > Hi Harsha and Ismael, > > > > Option 2 sounds like a good idea if we want to make this quick fix I > > think. > > Option 4 might require a KIP as its public interface change. I can > > resubmit > > a patch for option 2 or create a KIP if necessary for option 4. > > > > From the previous conversation here, I think Ismael prefers option 2. > > I don't have a strong opinion here since I understand its not easy to > > make > > public API changes but IMO, would go with option 4. > > > > Harsha what do you think on this? > > > > > > Thanks, > > > > Mayuresh > > > > On Mon, May 23, 2016 at 5:45 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Hi Mayuresh and Harsha, > > > > > > If we were doing this from scratch, I would prefer option 4 too. > However, > > > users have their own custom principal builders now and option 2 with a > > > suitably updated javadoc is the way to go in my opinion. > > > > > > Ismael > > > > > > On Sat, May 21, 2016 at 2:28 AM, Harsha <ka...@harsha.io> wrote: > > > > > > > Mayuresh, > > > > Thanks for the write up. With principal builder, > > > > the idea is to reuse a single principal builder > > > > across all the security protocols where its > > > > applicable and given that principal builder has > > > > access to transportLayer and authenticator it > > > > should be able to figure out what type of > > > > transportLayer it is and it should be able > > > > construct the principal based on that and it > should > > > > handle all the security protocols that we > support. > > > > In your options 1,2 & 4 seems to be doing the > same > > > > thing i.e checking what security protocol that a > > > > given transportLayer is and building a principal > , > > > > correct me if I am wrong here. I like going > with 4 > > > > as others stated on PR . As passing > > > > security_protocol makes it more specific to the > > > > method that its need to be handled . In the > interest > > > > of having less config I think option 4 seems to > be > > > > better even though it breaks the interface. > > > > > > > > Thanks, > > > > Harsha > > > > On Fri, May 20, 2016, at 05:00 PM, Mayuresh Gharat wrote: > > > > > Hi All, > > > > > > > > > > I came across an issue with plugging in a custom PrincipalBuilder > class > > > > > using the config "principal.builder.class" along with a custom > > > Authorizer > > > > > class using the config "authorizer.class.name". > > > > > > > > > > Consider the following scenario : > > > > > > > > > > For PlainText we don't supply any PrincipalBuilder. For SSL we > want to > > > > > supply a PrincipalBuilder using the property > "principal.builder.class". > > > > > > > > > > a) Now consider we have a broker running on these 2 ports and > supply > > > that > > > > > custom principalBuilder class using that config. > > > > > > > > > > b) The interbroker communication is using PlainText. I am using a > > > single > > > > > broker cluster for testing. > > > > > > > > > > c) Now we issue a produce request on the SSL port of the broker. > > > > > > > > > > d) The controller tries to build a channel for plaintext with this > > > broker > > > > > for the new topic instructions. > > > > > > > > > > e) PlainText tries to use the principal builder specified in the > > > > > "principal.builder.class" config which was meant only for SSL port > > > since > > > > > the code path is same > > > "ChannelBuilders.createPrincipalBuilder(configs)". > > > > > > > > > > f) In the custom principal Builder if we are trying to do some cert > > > > > checks > > > > > or down conversion of transportLayer to SSLTransportLayer so that > we > > > can > > > > > use its functionality we get error/exception at runtime. > > > > > > > > > > The basic idea is the PlainText channel should not be using the > > > > > PrincipalBuilder meant for other types of channels. > > > > > > > > > > Now there are few options/workarounds to avoid this : > > > > > > > > > > 1) Do instanceOf check in Authorizer.authorize() on TransportLayer > > > > > instance > > > > > passed in and do the correct handling. This is not intuitive and > > > imposes > > > > > a > > > > > strict coding rule on the programmer. > > > > > > > > > > 2) TransportLayer should expose an API for telling the security > > > protocol > > > > > type. This is not too intuitive either. > > > > > > > > > > 3) Add extra configs for Authorizer and PrincipalBuilder for each > > > channel > > > > > type. This gives us a flexibility for the PrincipalBuilder and > > > Authorizer > > > > > handle requests on different types of ports in a different way. > > > > > > > > > > 4) PrincipalBuilder.buildPrincipal() should take in extra > parameter for > > > > > the > > > > > type of protocol and we should document this in javadoc to use it > to > > > > > handle > > > > > the type of request. This is little better than 1) and 2) but again > > > > > imposes > > > > > a strict coding rule on the programmer. > > > > > > > > > > Just wanted to know what the community thinks about this and get > any > > > > > suggestions/feedback . There's some discussion about this here : > > > > > https://github.com/apache/kafka/pull/1403 > > > > > > > > > > Thanks, > > > > > > > > > > Mayuresh > > > > > > > > > > > > > > > -- > > -Regards, > > Mayuresh R. Gharat > > (862) 250-7125 > -- -Regards, Mayuresh R. Gharat (862) 250-7125
