Hi all, I resolved the challenge with having Class Type for the ssl.engine.factory.class configuration. Raised pull request to the trunk - https://github.com/apache/kafka/pull/8338. Updated on the JIRA - https://issues.apache.org/jira/browse/KAFKA-8890
Please review & Vote! Thanks Maulin On Mon, Mar 23, 2020 at 6:57 PM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > Hi Rajini > > When I configure the Default value for ssl.engine.factory.class as Type > Class, it is resulting into lot of test cases failures since in many places > - tests and actual classes/scala code it is converting the configuration > maps value to value.toString(). I verified that was the issues by fixing > some of it but still evaluating how many places we need to fix if we really > want to support Class Type key for the configuration. Password and List > Types are also of non-String types but it seems their defaults are Null and > they are optional vs in my case the ssl.engine.factory.class is *not* > optional - it needs a value by default. > > Will keep you posted. I am thinking of reverting the config type to String > and then load it as String and do Class loading in SslFactory. > > Thanks > Maulin > > > On Mon, Mar 23, 2020 at 1:38 AM Maulin Vasavada <maulin.vasav...@gmail.com> > wrote: > >> still working on the pull request. hopefully will be done soon. >> >> On Wed, Mar 11, 2020 at 11:48 AM Maulin Vasavada < >> maulin.vasav...@gmail.com> wrote: >> >>> Thanks Rajini. Sounds good. I'll make changes and update this thread. >>> >>> On Wed, Mar 11, 2020 at 6:41 AM Rajini Sivaram <rajinisiva...@gmail.com> >>> wrote: >>> >>>> Hi Maulin, >>>> >>>> I have reviewed the PR and left some comments, can you turn it into a PR >>>> for https://github.com/apache/kafka? It looks good overall. >>>> >>>> We pass all configs to other plugins, so we can do the same here. That >>>> would be safer than assuming that all custom SSL-related configs start >>>> with >>>> `ssl.`. You can use principal builder as an example for what we do >>>> today. >>>> >>>> Regards, >>>> >>>> Rajini >>>> >>>> On Thu, Mar 5, 2020 at 12:03 AM Maulin Vasavada < >>>> maulin.vasav...@gmail.com> >>>> wrote: >>>> >>>> > Hi Rajini >>>> > >>>> > I made changes suggested by you on >>>> > https://github.com/maulin-vasavada/kafka/pull/4. Please check. >>>> > >>>> > In particular I had challenge in 'SslFactory#configure()' method the >>>> first >>>> > time to know which configs I have to add without having actual >>>> > SslEngineFactory impl. I think it is best to just copy all configs >>>> with >>>> > "ssl." prefix. Can you please review >>>> > >>>> > >>>> https://github.com/maulin-vasavada/kafka/pull/4/files#diff-1e3432211fdbb7b2e2b44b5d8838a40bR90 >>>> > particularly? >>>> > >>>> > Clement, I missed to see your point about Mode in previous post but >>>> even >>>> > after I realized what you are suggesting, my response would be the >>>> same as >>>> > before :) >>>> > >>>> > Thanks >>>> > Maulin >>>> > >>>> > >>>> > On Wed, Feb 5, 2020 at 8:40 PM Maulin Vasavada < >>>> maulin.vasav...@gmail.com> >>>> > wrote: >>>> > >>>> > > Hi Rajini >>>> > > >>>> > > Will accommodate your comments. >>>> > > >>>> > > Celement, while SSLContext factories are common, in this particular >>>> case, >>>> > > we need SSLEngine object because Kafka is using SSLEngine (as >>>> mentioned >>>> > > much before in this email thread, the SSLContext acts as factory for >>>> > > getting SSLEngine, SSLSocketFactory or SSLServerSocketFactory and >>>> Kafka >>>> > > chooses SSLEngine to be used for SSL Connections). The 'Mode' >>>> challenge >>>> > > doesn't go away easily since Kafka is using the "same" classes for >>>> Client >>>> > > side and Server side. Other stacks where you don't see this >>>> challenge is >>>> > > because either libraries are client centric or server centric and >>>> not >>>> > both >>>> > > at the same time. I would suggest you do a deeper dive into the >>>> sample >>>> > Pull >>>> > > request, build the code to get better idea about it. I don't find it >>>> > > strange to have 'Mode' argument in this context of Kafka. Kafka is >>>> not >>>> > > doing anything out of norm here since ultimately it is using JSSE >>>> spec >>>> > for >>>> > > SSL Connection. >>>> > > >>>> > > I will try to respond to code comments in couple of weeks since I >>>> am out >>>> > > for few weeks. Will keep you guys posted. >>>> > > >>>> > > Thanks >>>> > > Maulin >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > >>>> > > On Wed, Feb 5, 2020 at 9:50 PM Pellerin, Clement < >>>> > clement_pelle...@ibi.com> >>>> > > wrote: >>>> > > >>>> > >> Many of these points came up before. >>>> > >> >>>> > >> I had great hope when Maulin suggested the custom factory could >>>> > >> return an SSLContext instead of SSLEngine. SSLContext factories >>>> are >>>> > >> common, >>>> > >> whereas I have never seen an SSLEngine factory being used before. >>>> > >> He must have hit the same problem I had with the Mode. >>>> > >> >>>> > >> If the Mode can be removed, can we find a way to return an >>>> SSLContext >>>> > now? >>>> > >> What is so special about Kafka that it needs to hardcode the Mode >>>> when >>>> > >> everyone >>>> > >> else works with the SSLContext and ignores the other mode they >>>> don't >>>> > use. >>>> > >> >>>> > >> -----Original Message----- >>>> > >> From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] >>>> > >> Sent: Wednesday, February 5, 2020 10:03 AM >>>> > >> To: dev >>>> > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine >>>> configuration >>>> > >> extensible >>>> > >> >>>> > >> One more point: >>>> > >> 5) We should also add a method to SslEngineFactory that returns >>>> > >> `Set<String> >>>> > >> reconfigurableConfigs()` >>>> > >> >>>> > >> On Wed, Feb 5, 2020 at 1:50 PM Rajini Sivaram < >>>> rajinisiva...@gmail.com> >>>> > >> wrote: >>>> > >> >>>> > >> > Hi Maulin, >>>> > >> > >>>> > >> > Thanks for the updates. A few comments below: >>>> > >> > >>>> > >> > 1) SslEngineFactory is currently in the internal package >>>> > >> > org.apache.kafka.common.security.ssl. We should move it to the >>>> public >>>> > >> > package org.apache.kafka.common.security.auth. >>>> > >> > 2) SslEngineFactory should extend Configurable and Closeable. We >>>> > should >>>> > >> > expect the implementation class to have a default constructor and >>>> > >> invoke configure() >>>> > >> > to be consistent with otSslher pluggable classes. >>>> > >> > 3) SslEngineFactory.createSslEngine uses the internal enum >>>> `Mode`. It >>>> > >> > would be better to add two methods instead: >>>> > >> > >>>> > >> > >>>> > >> > - SSLEngine createClientSslEngine(String peerHost, int >>>> peerPort, >>>> > >> String endpointIdentification); >>>> > >> > - SSLEngine createServerSslEngine(String peerHost, int >>>> peerPort); >>>> > >> > >>>> > >> > 4) Don't think we need a method on SslEngineFactory to return >>>> configs. >>>> > >> It seems like an odd thing to do in a pubic Configurable API and is >>>> > >> inconsistent with other APIs. We can track configs in the internal >>>> > >> SslFactory class instead. >>>> > >> >>>> > > >>>> > >>>> >>>
