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. > >> > > > > >> > > > >> > > >> > > >
