>> Pluggable instances have a default constructor and implement Configurable. >> Why wouldn't we just do that for SslEngineFactory? Because the constructor that you are replacing has more than just a map. You need to pass those extra arguments somewhere. I once proposed to fake extra arguments as synthetic configs and I was rightfully shut down. I agree though that the map should be passed in configure()
>> We can rename SslEngineFactory.reconfigurableConfigs() if it helps You don't want to rename this method because it should contain only the reconfigurable configs, not all configs. >> Perhaps we need to focus on the aspects of your use case that are not >> handled with the proposed approach. My use case is documented in KIP-383. In more detail, I want to pass an object in a config and the custom implementation can call this object to get the SslEngine from elsewhere in my application. There should be no keystore/truststore validation what so ever and there should not be any SSL Configs except the implementation class name and that object. The only known objection why SslFactory cannot be the extension point is because it contains a lot of validation code that would likely be by-passed by custom implementations. I don't agree with this objection because the keystore validation must be moved to DefaultSslEngineFactory and the rest can be moved to the SslChannelBuilder. This way SslFactory would fit the existing extension pattern in Kafka. An extension point that extends Reconfigurable is a lot cleaner. If you wonder why I changed my mind so strongly, it's because I realized there is less and less validation left in SslFactory and I expect SslChannelBuilder to be the only caller so it gives an anchor where to attach the remaining validation code. Can we discuss the possibility of moving the validation code to SslChannelBuilder? I did not look at the code and I don't know the challenges we would face. -----Original Message----- From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] Sent: Monday, September 16, 2019 8:59 AM To: dev Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible I am not sure what the confusion regarding Configurable interface is. Pluggable instances have a default constructor and implement Configurable. Why wouldn't we just do that for SslEngineFactory? We can rename SslEngineFactory.reconfigurableConfigs() if it helps, but basically it is the factory that decides what configs it is interested in. It doesn't really matter that we don't know what the reconfigurable configs are until the SslFactory is configured. `sslContext()` method was restored to support some connectors that were using it, but we don't need to add that to the interface since the plan is to remove that anyway. We do need `keystore()` and `truststore()` methods for validation. It is always the responsibility of plugins to validate the configs passed to them. Custom plugins must validate configs and fail configure() if configs are invalid. We would expect the same for SslEngineFactory. Perhaps we need to focus on the aspects of your use case that are not handled with the proposed approach. On Mon, Sep 16, 2019 at 1:26 PM Pellerin, Clement <clement_pelle...@ibi.com> wrote: > There are pending issues we need to address. > > We want to be able to call config.getConfiguredInstance(key, Class) to > instantiate the extension point. This requires a default constructor. The > former constructor arguments must now be passed in a separate init() > method. This has the advantage of moving the constructor signature from the > comment prose to the compiled language. I took inspiration from > MetricsReporter for the init() method. > > I question the object oriented design that requires the > reconfigurableConfigs() method but declares the interface to be > non-reconfigurable with just the Configurable interface. > > My use case removes all built-in SSL configs (except the interface class > name of course). SslFactory should not hardcode any SSL configs in the > reconfigurableConfigs. It should delegate to the interface instance for all > reconfigurableConfigs. In particular, it cannot assume there are keystores > and truststores to validate. These checks should be moved to > DefaultSslEngineFactory. We can then consider moving the SslEngine > validation from SslFactory to SslChannelBuilder. What would be left in > SslFactory that forces us to keep it instead of making it the > Reconfigurable extension point itself? > > I believe we don't need the sslContext() method since it is only used by a > junit. > If we support my use case, there is a good chance we don't need the > keystore() and truststore() method. > > I am still not comfortable with the fact that reconfigurableConfigs() are > not known until the SslEngineFactory implementation is created and that > happens only after configure() is called. Notice this goes away if > SslFactory is the extension point, which would explain why this might not > have been an issue with the other extension points exposing reconfigurable > custom configs. > > We must document if the configs sent to the extension point implementation > have been validated or not. I am pretty sure they are not since > config.getConfiguredInstance() passes the originals() and there is no > configurableConfigs() method (there is only the subset in > reconfigurableConfigs()). Non-validated configs might be of the wrong type, > be out of range, or missing since the default value is not applied. This is > a burden to the extension point developer and Kafka should provide > utilities for this. > > Can you confirm where the special case for the reconfiguration of > keystore/truststore is implemented? I am still trying to determine if it is > possible to trigger a reconfiguration when none of the known configs have > changed. > > > -----Original Message----- > From: Rajini Sivaram [mailto:rajinisiva...@gmail.com] > Sent: Monday, September 16, 2019 5:29 AM > To: dev > Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration > extensible > > 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. > > >> > >> > > > > > >> > >> > > > > >> > >> > > > >> > >> > > >> > > > > >> > > > >> > > > > > >
