Hi Maulin, Thanks for writing that up. I think we are getting close.
As an example interface (which can be adjusted as necessary, but just including something here to refer to), I think we would have an interface along these lines: public interface SslEngineFactory extends Configurable, Closeable { Set<String> reconfigurableConfigs(); boolean shouldBeRebuilt(Map<String, Object> nextConfigs); SSLEngine createSslEngine(Mode mode, String peerHost, int peerPort, String endpointIdentification); SSLContext sslContext(); KeyStore keystore(); KeyStore truststore(); } A) *Creation:* - We typically tend to include configs in constructor for non-pluggable classes, but use Configurable for pluggable classes. So Option 2 works better B) *Returning reconfigurable configs: * - Custom SslEngineFactory implementations will only return what they care about in their implementation of reconfigurableConfigs(). - SslChannelBuilder will delegate to SslFactory as you mentioned. - SslFactory.reconfigurableConfigs() will return SslConfigs.RECONFIGURABLE_CONFIGS plus SslEngineFactory. reconfigurableConfigs(). So one day if we make endpoint validation reconfigurable, it would all just work. We can easily find a different way of continuing to reconfigure SslFactory without config changes if we needed to since it is not a pluggable class. C) *Triggering reconfiguration:* - We continue to use AdminClient for reconfiguration in brokers with validation. That goes through SslEngineFactory.shouldBeRebuilt() and creates a new SslEngineFactory instance if needed. D) *Handling push notification kind of needs* - For brokers, we want SslFactory to manage reconfiguration since configs may change. Also, AdminClient requests in brokers give a clear auditable path for config updates where update failures are returned to the caller. C) enables this. - For client-side, custom SslEngineFactory implementations could reconfigure themselves, we don't really need SslFactory to be involved at all. On Sat, Sep 14, 2019 at 8:17 AM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > Hi Clement/Rajini > > Based on what I get from my own understanding and your latest inputs, I'll > write down what my proposal is here and let us see if we can discuss on > that, > > A) Creation > > 1. SslEngineFactory interface will not extend Configurable. Instead it will > require the implementation to have constructor with config map > 2. Currently, the caller is co-ordinating the call sequences between > configure() and reconfigurableConfigs() in existing code base for > SslChannelBuilder, SslFactory, ChannelBuilders and SocketServer. Hence I am > not sure if we should get stuck arguing having a constructor vs having > configure() to initialize the object. Personally I don't have a preference. > > B) Returning reconfigurable configs > > 1. SslEngineFactory interface will have reconfigurableConfigs() method > which will return Set<String> with values for custom (or with existing) > config keys that it cares about. > Here I agree that all existing SSL configurations may not at all be > applicable to the pluggable engine. I don't see a point in keep referring > to existing SSL Configs. Of course, the default implementation of the > SslEngineFactory will return current SSL Configs since it relies on them to > initialize keys/certs. > > 2. SslFacotry's reconfigurableConfigs() will delegate the call to the > SslEngineFactory's reconfigurableConfigs(). The similar delegation will > need to be done by SslChannelBuilder/SaslChannelBuilder to SslFactory for > the same method (currently that are returning SslConfigs. > RECONFIGURABLE_CONFIGS) > > C) Triggering reconfiguration > > 1. We can use AdminClient to trigger AlterConfig for any configs needed by > the SslEngineFactory's implementation. For the default implementation it > will follow the SSL Configs it uses today. For the custom implementation > everything will funnel via 'custom config' as it is dealt with in below > method, > > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala#L507 > > Btw, I don't see any check like if the reconfiguration needs to be > triggered at the below line for Listeners, > > https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala#L515 > > We do seem to have a check on Line#518 for other reconfigurables. > > D) Handling push notification kind of needs > > As noted by both of you, the reconfiguration will come via AdminClient > probably from the SslEngineFactory's implementation itself when it knows > the keys/certs changed and I need to ask Kafka to re-create myself. > However, that approach ONLY works for Brokers. With keys/cert rotations we > should address Client side challenge also. As you would have noticed in > KIP-486 motivation the deployment challenge on Broker and Client side both > makes it challenging to manage key/cert rotations etc. Hence I feel we > should consider option of method in the SslEngineFactory to take a > reconfigurable and call it when it knows it needs to be reconfigured. > > Overall, I feel we should avoid any coupling with existing handling of SSL > keystore and truststore via the SSL Configs. When we are allowing > customization for the whole SSLEngine then we don't want it to rely on > existing configs to assume/necessarily-reuse any mechanism for loading > keystore and Truststore. > > For MetricReporters I feel it is using custom configs like this except that > MetricReporter interface defines init() and extends Configurable. Please > clarify if I am missing something. > > Thanks > Maulin > > > > > > > > > > > > > > > > On Fri, Sep 13, 2019 at 12:15 PM Maulin Vasavada < > maulin.vasav...@gmail.com> > wrote: > > > Hi Clement/Rajini > > > > I've gone through the code to understand how reconfigruation work > > currently. I sent both of you a note separately also. Let us reconvene > next > > week and proceed. > > > > Thanks > > Maulin > > > > On Thu, Sep 12, 2019 at 12:25 PM Pellerin, Clement < > > clement_pelle...@ibi.com> wrote: > > > >> You are proposing a different design pattern for the SSL reconfigurable > >> extension point than the one already implemented for MetricsReporter. > You > >> need a good reason to justify this decision. It is as if you consider > >> SslFactory the extension point. This is indeed something I proposed > >> multiple times and was always shut down. > >> > >> Going back to your latest proposal, notice you cannot call > >> reconfigurableConfigs() until configure() is called because you need to > >> instantiate SslEngineFactory() first. There is no way to enforce this in > >> the API. > >> > >> You did not react to my observation that ConfigDef does a better job > >> validating/casting configs based on the ConfigKey declarations compared > to > >> ad hoc code you force people to write in their extension point > >> implementations. > >> > >> You suggest not to augment ConfigDef with custom configs, so that takes > >> care of the recursive dependency. > >> I just noticed reconfigurableConfigs() returns Set<String> and that does > >> not force the creation of a ConfigKey for custom configs. > >> > >> >> SslFactory.reconfigurableConfigs() returns all standard > reconfigurable > >> SSL configs as well as MySslEngineFactory.reconfigurableConfigs(). > >> That's no good because that implementation does not use the standard > >> property for the keystore location. For that particular use case, it > would > >> probably be better to reuse the standard keystore location config and > >> change its semantics to a URL. Regardless, my point is the custom > >> implementation should decide all the reconfigurable properties. By the > way, > >> my original use case for KIP-383 was to replace all SSL configs with a > >> single name. > >> > >> It is still not clear in your email if the keystore/truststore exception > >> is handled locally in SslFactory or by the initiator of the whole > >> AlterConfig. That determines whether "AlterConfig without config > changes" > >> always goes through or is usually blocked early by the initiator. > >> > >> > >> -----Original Message----- > >> From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > >> Sent: Thursday, September 12, 2019 2:05 PM > >> To: dev > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> extensible > >> > >> Sorry about the confusion, my notes were conflicting! > >> > >> Let me give an example to clarify. Let's say KIP-519 adds a new > interface > >> SslEngineFactory and I have a custom implementation MySslEngineFactory > >> that > >> gets keystore material from a web service using a custom config ` > >> my.ssl.keystore.url`. MySslEngineFactory.reconfigurableConfigs() returns > >> {` > >> my.ssl.keystore.url`}. We don't want to add this custom config to Kafka. > >> My > >> SslEngineFactory will be managed by the (non-pluggable) Reconfigurable > >> SslFactory that invokes appropriate methods on MySslEngineFactory to > >> create > >> a new SslEngine when required. SslFactory.reconfigurableConfigs() > returns > >> all standard reconfigurable SSL configs as well as > >> MySslEngineFactory.reconfigurableConfigs(). > >> > >> The existing code in the broker is sufficient for both validation and > >> reconfiguration. We can support custom configs as well as > reconfiguration > >> without config change. The only non-SSL change we require is a fix for > >> KAFKA-7588. > >> > >> 1) *Initial validation*: SslFactory.configure() invokes > >> MySslEngineFactory#configure() which validates the custom config ` > >> my.ssl.keystore.url`. Broker will fail to start if the URL is invalid > even > >> though broker knows nothing about this config because the custom > >> implementation fails its `configure()`. > >> 2) *Validation during reconfiguration*: AdminClient sends an AlterConfig > >> request to update `my.ssl.keystore.url` Broker invokes. > >> SslFactory.validateReconfiguration() which will invoke > >> MySslEngineFactory.validateReconfiguration() and that can fail > >> reconfiguration if the provided URL is invalid. And AdminClient sees > this > >> validation failure in its response. This validation is more flexible > than > >> that provided by ConfigDef. During reconfiguration, we are not just > >> validating that the value is of the right type, we may want to verify > that > >> you can connect to the remote web service for example. This validation > >> path > >> for custom configs is already supported. > >> 3) *Reconfiguration without config change*: This is supported > specifically > >> for two configs `ssl.keystore.location` and `ssl.truststore.location`. > >> SslFactory is given the opportunity to reconfigure whenever there is an > >> AlterConfig request from AdminClient since it uses these configs. This > is > >> regardless of whether there is a config change. SslFactory will invoke > >> MySslEngineFactory.shouldBeRebuilt(). This gives MySslEngineFactory the > >> opportunity to provide a new SslEngine whenever there is an AdminClient > >> request to alter configs, regardless of whether any config changed or > not. > >> > >> Hope that helps, > >> > >> Rajini > >> > >> On Thu, Sep 12, 2019 at 5:26 PM Pellerin, Clement < > >> clement_pelle...@ibi.com> > >> wrote: > >> > >> > I'm confused. Can you launch a reconfiguration without a config change > >> or > >> > not? > >> > > >> > If I understand the test case correctly, the design pattern to > >> implement a > >> > reconfigurable extension point in Kafka is to implement the > >> Reconfigurable > >> > interface. This means SslEngineFactory would be created once and > mutate > >> > with reconfigure. There is no ConfigKey created for the custom config > >> and > >> > therefore there is no validation by ConfigDef. > >> > > >> > Optionally, to expose the built-in validation, it might be worthwhile > to > >> > consider making ConfigKey a public API and move an individual config > >> parse > >> > from ConfigDef to ConfigKey. It would be more object oriented anyway. > >> > > >> > One of the use case of custom configs in SslEngineFactory is to remove > >> the > >> > need to specify the keystore and truststore locations. The special > >> handling > >> > to detect changes in keystore/truststore should be pushed to > >> > DefaultSslEngineFactory and all calls to reconfigure should reach the > >> > SslEngineFactory instance. Am I missing something? > >> > > >> > -----Original Message----- > >> > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > >> > Sent: Thursday, September 12, 2019 12:01 PM > >> > To: dev > >> > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > >> > extensible > >> > > >> > Correction on my previous email. Custom implementations can use > >> AlterConfig > >> > request without a config change by including `ssl.keystore.location` > or > >> > `ssl.truststore.location` in their `reconfigurabkeConfigs()`. This > will > >> > trigger the same codepath as we use for keystore updates when the file > >> has > >> > changed. > >> > > >> > On Thu, Sep 12, 2019 at 4:43 PM Rajini Sivaram < > rajinisiva...@gmail.com > >> > > >> > wrote: > >> > > >> > > Hi Clement, > >> > > > >> > > Kafka does special handling for keystore/truststore file changes > when > >> an > >> > > AlterConfig request is processed, but that is not easy to extend to > >> > custom > >> > > configs. I was thinking we could just add a custom config to trigger > >> > > reconfiguration. For example, a config like > >> `my.custom.keystore.version` > >> > that > >> > > is incremented when the custom implementation detects a change in > >> > keystore. > >> > > And the custom implementation would invoke admin client to update > >> > `my.custom.keystore.version`. > >> > > Kafka would do the rest to reconfigure SslFactory. And the custom > >> > > implementation can then create the new builder. > >> > > > >> > > For an example of custom config reconfiguration, this test may be > >> useful: > >> > > > >> > > >> > https://github.com/apache/kafka/blob/d3559f628b2ccb23a9faf531796675376ac06abb/core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala#L792 > >> > > > >> > > > >> > > > >> > > On Thu, Sep 12, 2019 at 3:24 PM Pellerin, Clement < > >> > > clement_pelle...@ibi.com> wrote: > >> > > > >> > >> For the push notification, Rajini prefers if the trigger to > >> reconfigure > >> > >> comes from an admin client. He says the admin client can > reconfigure > >> > even > >> > >> though none of the config values have changed. This allows your > >> custom > >> > impl > >> > >> to discover other conditions that have changed, for example the > >> > contents of > >> > >> the keystore. > >> > >> > >> > >> @Rajini, can you point us to an existing example of a Kafka > extension > >> > >> point that implements reconfigurable custom configs. This way we > >> could > >> > >> study it and do the same thing. Consistency is good. It would be > >> nice if > >> > >> there was a KIP that describes that design pattern. > >> > >> > >> > >> -----Original Message----- > >> > >> From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > >> > >> Sent: Thursday, September 12, 2019 2:24 AM > >> > >> To: dev@kafka.apache.org > >> > >> Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > configuration > >> > >> extensible > >> > >> > >> > >> Thanks Clement and Rajini. Let me digest what both of you said. > >> Clearly > >> > I > >> > >> need to understand more about the configurations - dynamic, custom > >> etc. > >> > >> > >> > >> On the 'push notification' question Clement asked, > >> > >> The way I see is simple - Kafka defines the interface for > >> > >> SslEngineFactory. > >> > >> Implementation of that interface is owned by the Kafka operator who > >> > >> customized the implementation. Let us say, my SslEngineFactoryImpl > >> ONLY > >> > >> customizes loading of keys/certs where it knows when they are > >> updated. > >> > How > >> > >> is Kafka going to know that? You said 'next time it loads' but if > >> there > >> > is > >> > >> NO additional configuration that was needed by my > >> SslEngineFactoryImpl, > >> > >> there won't be any trigger coming from Kafka to reconfigure and > >> > SslFactory > >> > >> will never re-create my SslEngineFactoryImpl code which will help > >> Kafka > >> > >> use > >> > >> new keys/certs in the next calls. Please let me know if this makes > my > >> > >> 'push > >> > >> notification' needs clearer. > >> > >> > >> > >> Thanks > >> > >> Maulin > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> On Wed, Sep 11, 2019 at 2:31 PM Pellerin, Clement < > >> > >> clement_pelle...@ibi.com> > >> > >> wrote: > >> > >> > >> > >> > Indeed, this is a general problem requiring a more general > solution > >> > than > >> > >> > KIP-519. I'm glad there was work done on this already. > >> > >> > > >> > >> > So config.originals() still contains unknown configs but nothing > >> has > >> > >> been > >> > >> > validated and cast to the proper type. > >> > >> > How does validation work for an extension point that receives > >> > >> > config.originals()? Is there a public validator helper to handle > >> this? > >> > >> > Do we need to create ConfigKeys in the ConfigDef for custom > configs > >> > only > >> > >> > known to a custom SslEngineFactory implementation? > >> > >> > Do we need to declare the standard SSL configs in ConfigDef if > >> > >> SslFactory > >> > >> > needs to revalidate them anyway because it receives > >> > config.originals()? > >> > >> > I assume there is such a thing as config.originals() also for a > >> > >> > reconfigure()? > >> > >> > > >> > >> > If we implement KIP-519 and later change from config.values() to > >> > >> > config.originals(), this will affect the contract for the > >> constructor > >> > of > >> > >> > the SslEngineFactory. We might need to add custom configs support > >> to > >> > >> > KIP-519 or delay KIP-519 until the change to config.originals(). > >> > >> > > >> > >> > > >> > >> > -----Original Message----- > >> > >> > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > >> > >> > Sent: Wednesday, September 11, 2019 4:25 PM > >> > >> > To: dev > >> > >> > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > >> configuration > >> > >> > extensible > >> > >> > > >> > >> > Kafka already has the notion of custom configs. And we support > >> > >> > reconfigurable custom configs for some interfaces e.g. > >> > MetricsReporter. > >> > >> We > >> > >> > also recently added custom reconfigurable configs for Authorizer > >> under > >> > >> > KIP-504. > >> > >> > > >> > >> > The issue with custom configs for SSL is described in > >> > >> > https://issues.apache.org/jira/browse/KAFKA-7588. We currently > >> don't > >> > >> pass > >> > >> > in custom configs to ChannelBuilders. We need to fix this, not > just > >> > for > >> > >> SSL > >> > >> > but for other security plugins as well. So it needs to be a > generic > >> > >> > solution, not specific to KIP-519. > >> > >> > > >> > >> > Once KAFKA-7588 is fixed, the existing dynamic reconfiguration > >> > >> mechanism in > >> > >> > brokers would simply work. Dynamic configs works exactly in the > >> same > >> > way > >> > >> > for custom configs as it does for other configs. The list of > >> > >> reconfigurable > >> > >> > configs is returned by the implementation class and the class > gets > >> > >> notified > >> > >> > when any of those configs changes. This includes > >> > >> validateReconfiguration() > >> > >> > as well the actual reconfigure(). > >> > >> > > >> > >> > For SSL alone, we have special handling of dynamic configs to > >> enable > >> > >> > reloading of keystores/truststores when the file changes, even > >> though > >> > >> none > >> > >> > of the config values have changed. Reconfiguration is triggered > by > >> an > >> > >> admin > >> > >> > client request to alter configs. In this case, none of the actual > >> > >> configs > >> > >> > may have changed, but we check if the file has changed. This is > >> > >> currently > >> > >> > done only for the standard keystore/truststore configs. With > >> KIP-519, > >> > I > >> > >> > guess we want the custom SslEngineFactory to be able to decide > >> whether > >> > >> > reconfiguration is required. The simplest way to achieve this > >> would be > >> > >> to > >> > >> > have a custom config that is updated when reconfiguration is > >> required. > >> > >> I am > >> > >> > not sure we need a separate mechanism to trigger reconfiguration > >> that > >> > >> > doesn't rely on admin clients since admin clients provide useful > >> > logging > >> > >> > and auditability. > >> > >> > > >> > >> > Regards, > >> > >> > > >> > >> > Rajini > >> > >> > > >> > >> > On Wed, Sep 11, 2019 at 4:13 PM Pellerin, Clement < > >> > >> > clement_pelle...@ibi.com> > >> > >> > wrote: > >> > >> > > >> > >> > > I'm sorry if I divert the discussion, but without this issue, > it > >> > would > >> > >> > > have been pretty trivial to update KIP-383 to go as far as you > >> did. > >> > I > >> > >> am > >> > >> > > also happy to get a discussion going, the KIP-383 thread was a > >> > >> desolate > >> > >> > > place. > >> > >> > > > >> > >> > > Kafka needs to know about custom configs because it validates > the > >> > >> configs > >> > >> > > before it passes them to (re)configure. Unknown configs are > >> silently > >> > >> > > removed by ConfigDef. We could keep unknown configs as strings > >> > without > >> > >> > > validating them in ConfigDef, but I don't know if the Kafka > >> > community > >> > >> > would > >> > >> > > accept this weaker validation. > >> > >> > > > >> > >> > > It appears we are trying to invent the notion of a meta config. > >> > >> Anyway, I > >> > >> > > think we have shown asking an instance of SslEngineFactory to > >> > >> contribute > >> > >> > to > >> > >> > > ConfigDef is way too late. > >> > >> > > > >> > >> > > For your push notification, would it be simpler to just let > your > >> > >> > > SslEngineFactory notice the change and make it effective the > next > >> > >> time it > >> > >> > > is called. SslFactory would not cache the SslEngine and always > >> ask > >> > >> > > SslEngineFactory for it. You don't even need an inner thread if > >> > >> > > SslEngineFactory checks for a change when it is called. > >> > >> SslEngineFactory > >> > >> > > would no longer be immutable, so maybe it is worth > reconsidering > >> how > >> > >> > > reconfigure works for it. > >> > >> > > > >> > >> > > -----Original Message----- > >> > >> > > From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] > >> > >> > > Sent: Wednesday, September 11, 2019 3:29 AM > >> > >> > > To: dev@kafka.apache.org > >> > >> > > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine > >> > configuration > >> > >> > > extensible > >> > >> > > > >> > >> > > Hi all, > >> > >> > > > >> > >> > > Since the "custom config" seems the main topic of interest let > us > >> > talk > >> > >> > > about it. > >> > >> > > > >> > >> > > 1. I want to confirm that I interpret the definition of 'custom > >> > >> config of > >> > >> > > SslEngineFactory' the same way Clement is suggesting - "a > config > >> > that > >> > >> > does > >> > >> > > not exist in Kafka but is specified by a custom implementation > of > >> > >> > > SslEngineFactory". If there is a disagreement to that we have > to > >> > >> bring > >> > >> > it > >> > >> > > up here sooner. > >> > >> > > > >> > >> > > 2. I've been thinking about it and I question why we are trying > >> to > >> > >> make a > >> > >> > > custom config a first class citizen in standard config? > >> > >> > > The reasoning for that question is- > >> > >> > > Kafka wants to delegate creation of SSLEngine to a class which > is > >> > >> "not" > >> > >> > > part of Kafka's distribution. Since the interface for SSLEngine > >> > >> creator > >> > >> > > will be defined by the public method of createSSLEngine(), why > >> would > >> > >> > Kafka > >> > >> > > care what does the implementation do other than fulfilling the > >> > >> contract > >> > >> > of > >> > >> > > creation of SSLEngine. The implementation can use any special > >> > configs > >> > >> > i.e. > >> > >> > > configs coming from input Map OR configs defined in a new file > >> only > >> > >> known > >> > >> > > to itself. Making the configs part of the interface contract in > >> any > >> > >> way > >> > >> > is > >> > >> > > not necessary. This way we keep it simple and straightforward. > >> > >> > > > >> > >> > > 3. Now, 2nd point raises a question - if we follow that > >> suggestion - > >> > >> how > >> > >> > > can we ever re-create the SSLEngineFactory object and allow new > >> > >> object to > >> > >> > > be created when something changes in the implementation. That > is > >> a > >> > >> valid > >> > >> > > question. If you noticed in the KIP section titled "Other > >> challenge" > >> > >> - we > >> > >> > > do have scenario where the SslEngineFactory implementation ONLY > >> > knows > >> > >> > that > >> > >> > > something changed - example: keystore got updated by a local > >> daemon > >> > >> > process > >> > >> > > only known to the specific implementation. This means we have a > >> need > >> > >> of > >> > >> > > "push notification" from the SslEngineFactory's implementation > to > >> > the > >> > >> > > SslFactory actually. I feel if we build the "push notification" > >> via > >> > >> > adding > >> > >> > > a method in the interface as "public void > >> > >> > > registerReconfigurableListener(Reconfigurable r)" and make > >> > SslFactory > >> > >> > > register itself with the SslEngineFactory's impl class, we can > >> > trigger > >> > >> > the > >> > >> > > reconfiguration of SslEngineFactory implementation based on its > >> own > >> > >> terms > >> > >> > > and conditions without getting into making custom configs > >> > complicated. > >> > >> > > > >> > >> > > I am just thinking out loud here and expressing my opinion not > to > >> > >> avoid > >> > >> > > addressing custom configs BUT what I genuinely believe might > be a > >> > >> better > >> > >> > > approach. > >> > >> > > > >> > >> > > Thanks > >> > >> > > Maulin > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > > >> > >> > > On Tue, Sep 10, 2019 at 9:06 PM Pellerin, Clement < > >> > >> > > clement_pelle...@ibi.com> > >> > >> > > wrote: > >> > >> > > > >> > >> > > > Regarding what I labeled the simplest solution below, > >> SslConfigs > >> > >> could > >> > >> > > > instantiate the custom interface only if the yet to be > >> validated > >> > >> > configs > >> > >> > > > were passed in to the call to get the list of known SSL > >> configs. > >> > >> > > > > >> > >> > > > -----Original Message----- > >> > >> > > > From: Pellerin, Clement > >> > >> > > > Sent: Tuesday, September 10, 2019 11:36 AM > >> > >> > > > To: dev@kafka.apache.org > >> > >> > > > Subject: [EXTERNAL]RE: [DISCUSS] KIP-519: Make SSL > >> context/engine > >> > >> > > > configuration extensible > >> > >> > > > > >> > >> > > > Another solution could be a new standard ssl config that > holds > >> a > >> > >> list > >> > >> > of > >> > >> > > > extra custom configs to accept. > >> > >> > > > Using a custom SslEngineFactory with custom configs would > >> require > >> > >> > setting > >> > >> > > > two configs, one for the class name and another for the list > of > >> > >> custom > >> > >> > > > configs. > >> > >> > > > In SslConfigs, we see that declaring a single config takes 5 > >> > >> values, so > >> > >> > > > I'm not sure how it would work exactly. > >> > >> > > > > >> > >> > > > We could also declare a new interface to return the list of > >> custom > >> > >> ssl > >> > >> > > > configs and the extra standard ssl config I'm proposing holds > >> the > >> > >> name > >> > >> > of > >> > >> > > > the implementation class instead. The reason a different > >> interface > >> > >> is > >> > >> > > > needed is because it would be instantiated by SslConfigs, not > >> > >> > SslFactory. > >> > >> > > > This seems the simplest solution. > >> > >> > > > > >> > >> > > > Anyway, the point of this exercise is to prove an acceptable > >> > >> solution > >> > >> > for > >> > >> > > > custom configs is not affecting the public API in KIP-519. > >> > >> > > > > >> > >> > > > > >> > >> > > > -----Original Message----- > >> > >> > > > From: Pellerin, Clement > >> > >> > > > Sent: Tuesday, September 10, 2019 9:35 AM > >> > >> > > > To: dev@kafka.apache.org > >> > >> > > > Subject: [EXTERNAL]RE: [DISCUSS] KIP-519: Make SSL > >> context/engine > >> > >> > > > configuration extensible > >> > >> > > > > >> > >> > > > Custom config is a term I invented to mean a config that does > >> not > >> > >> exist > >> > >> > > in > >> > >> > > > Kafka but is specified by a custom implementation of > >> > >> SslEngineFactory. > >> > >> > > > The problem with custom configs (as I remember it) is that > the > >> > list > >> > >> of > >> > >> > > > configs is static in SslConfigs and config names are checked > >> > before > >> > >> > > > SslFactory is created. > >> > >> > > > ==> You must solve this problem because that is what killed > >> > KIP-383 > >> > >> and > >> > >> > > > therefore is the sole reason why KIP-519 exists. > >> > >> > > > ==> Your KIP does not have to implement the solution since it > >> can > >> > be > >> > >> > done > >> > >> > > > in a future KIP, but your KIP must be compatible with the > >> solution > >> > >> > found. > >> > >> > > > ==> A method to return the list of configs would help. This > >> cannot > >> > >> be a > >> > >> > > > static method in the interface since it cannot be overridden. > >> > >> > > > ==> You could require a static method in the implementation > >> class > >> > by > >> > >> > > > convention, just like the constructor you require. > >> > >> > > > > >> > >> > > > This email did not originate from inside Information > Builders. > >> > >> > > > > >> > >> > > > >> > >> > > >> > >> > >> > > > >> > > >> > > >