Hi Chris, Regarding item 4: Thanks for clarification. I really missed it. I've updated the 'compatibility' section [1] and prototype [2] accordingly.
Regarding item 5: Perfect naming is the one of hardest things and not my strong point. > The best I've been able to come up with is establishing > two new interfaces: ClientSslEngineFactory and ServerSslEngineFactory I cannot catch an idea with two interfaces because this interfaces make sense only for private code, but user always must implements both especially for Connect config. > Maybe BaseSslEngineFactory? Let me know what you think. All I can think of is: - BaseSslEngineFactory; - StaticSslEngineFactory; - NonReconfigurableSslEngineFactory. But I'm afraid that it would be hard to find sponsors to review this. My proposal: restrict the scope of this KIP to Kafka Connect config and file a new KIP to refactor the `SslEngineFactory`. I see that there are KIPs with smaller changes. I guess simple and clear short steps make success. [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer#KIP967:SupportcustomSSLconfigurationforKafkaConnectRestServer-Compatibility,Deprecation,andMigrationPlan [2]. https://github.com/apache/kafka/pull/14203/files#diff-2c8e6c18e6cc9082d6b8eb38d1ee00e7c8bd4819b5b32e68e7d1fce2f7570c47R195 -- With best regards, Taras Ledkov On Thu, Nov 2, 2023 at 8:25 PM Chris Egerton <chr...@aiven.io.invalid> wrote: > > Hi Taras, > > Thanks for the changes to the KIP! > > Regarding item 4: I think some background may be helpful for people without > context on the Connect code base. The current parsing logic for SSL-related > properties used with the REST API is to use all worker properties prefixed > with "listeners.https." (ignoring admin listeners here for the sake of > discussion, but the logic is essentially the same for them albeit with a > different prefix), or if no properties are found with that prefix, all > worker properties that are defined in the ConfigDef for the worker (which > includes all properties provided by SslConfigs::addClientSupport [1]). This > logic comes from usage of the AbstractConfig::valuesWithPrefixAllOrNothing > method [2] by the SSLUtils class [3], [4]. > > It seems like the restriction on only using properties defined by the > worker is a potential problem here, since an SSL engine factory may want to > use property names that aren't available out-of-the-box with Kafka Connect > (i.e., which aren't defined in the DistributedConfig [5] or > StandaloneConfig [6] class). > > It also sounds like (reading the KIP again) we're proposing that SSL engine > factory classes only be configured with properties prefixed with > "listeners.https.", without the fallback on all non-prefixed worker > properties if none are found. Wouldn't this be a breaking, > backwards-incompatible change for existing Connect clusters that are > configured to use SSL with top-level properties instead of ones prefixed > with "listeners.https."? > > IMO the most reasonable compromise here would be to keep almost the exact > same logic for finding SSL-related properties for the REST API, but without > the restriction on predefined configs for the fallback. So it'd be: if any > properties are present with the "listeners.https." prefix, pass only those > to the SSL engine factory, and otherwise, pass all non-prefixed worker > properties to the factory (without checking to see if they're defined by > the worker already). Do you agree? If so, we should clarify this in the KIP. > > > Regarding item 5: This is a little hairy. I'd prefer to avoid leaving > optional methods in the interface since they'll just add FUD for anyone who > doesn't find the right corner of our docs to read, and they may also imply > that the Connect runtime supports functionality that it doesn't (i.e., > dynamic reconfiguration of SSL). But I do agree that it'd be convenient to > support existing implementations of the SslEngineFactory interface, so a > completely separate interface isn't good here either. I like the idea of > introducing a superinterface for SslEngineFactory, but I've been struggling > to think of a good name for it. The best I've been able to come up with is > establishing two new interfaces: ClientSslEngineFactory and > ServerSslEngineFactory, which contain SSLEngine > createClientSslEngine(String peerHost, int peerPort, String > endpointIdentification) and SSLEngine createServerSslEngine(String > peerHost, int peerPort), respectively. We could require custom Connect SSL > engine factories to implement both ClientSslEngineFactory > and ServerSslEngineFactory. But frankly, the only reason two interfaces > seem clearer than one here is that I can't think of a better name than > SslEngineFactory to capture the two methods we need. Maybe > BaseSslEngineFactory? Let me know what you think. > > > [1] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java#L138-L158 > [2] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java#L317-L340 > [3] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/util/SSLUtils.java#L44 > [4] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/rest/util/SSLUtils.java#L67C70-L67C82 > [5] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/DistributedConfig.java > [6] - > https://github.com/apache/kafka/blob/57662efec9207446ffb32db179e4adf6bff76e18/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/standalone/StandaloneConfig.java > > > Cheers, > > Chris > > On Thu, Nov 2, 2023 at 10:44 AM Taras Ledkov <tled...@apache.org> wrote: > > > Hi Chris, > > > > Thanks a lot for such a close review. > > > > > 1. The "ssl.engine.factory.class" property was originally added for Kafka > > > brokers in KIP-519 [1]. It'd be nice to link to that KIP (possibly in a > > > "Background" section? > > Added "Background" section. > > > > > 2. Can we clarify that the new "listeners.https.ssl.engine.factory.class" > > > property (and the way that the engine factory is configured with all > > > properties prefixed with "listeners.https.") will also affect > > MirrorMaker 2 > > Great observation! I've skipped it. Added to section "Compatibility, > > Deprecation, and Migration Plan" > > > > > 3. We don't need to specify in the KIP that the > > > org.apache.kafka.connect.runtime.rest.util.SSLUtils class will be > > removed, > > Dropped. > > > > > 4. The test plan > > 4.1. Dropped "Default SSL behavior and compatibility" > > 4.2. As far as i see `RestForwardingIntegrationTest` contains the bug > > that is annihilated by the strange behavior of the > > `AbstractConfig#valuesWithPrefixAllOrNothing` at the > > `SSLUtils#createServerSideSslContextFactory` > > (or invalid usage of this method in this place). All ssl properties are > > not prefixed with "listeners.https.". > > I guess that the definitions of SSL properties at the root of the > > `WorkerConfig` is unclean / buggy / may lead to unexpected behavior. > > So, I would like to fix / improve this test. > > > > > 5. There are several methods in the SslEngineFactory interface that don't > > > seem applicable for Kafka Connect. > > Cool idea. Do you have any ideas about design? > > My proposal: > > - Extract base interface from `SslEngineFactory` and simple > > refactoring of the `SslFactory`; > > or > > - Do nothing and document the fact that implementation of these > > methods are not necessary for the SSL Engine Factory used for Connect / > > MM2. > > But I guess, a user that implements its own SslEngineFactory` for a > > the broker just prefers to reuse the code as is. > > > > [1]. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer#KIP967:SupportcustomSSLconfigurationforKafkaConnectRestServer-Background > > > > -- > > With best regards, > > Taras Ledkov > > > > On Mon, Oct 30, 2023 at 8:07 PM Chris Egerton <chr...@aiven.io.invalid> > > wrote: > > > > > > Hi Taras, > > > > > > Thanks for the KIP! I have some feedback but ultimately I like this > > > proposal: > > > > > > 1. The "ssl.engine.factory.class" property was originally added for Kafka > > > brokers in KIP-519 [1]. It'd be nice to link to that KIP (possibly in a > > > "Background" section?) so that reviewers who don't have that context can > > > find it quickly without having to dig through commit histories in the > > code > > > base. > > > > > > 2. Can we clarify that the new "listeners.https.ssl.engine.factory.class" > > > property (and the way that the engine factory is configured with all > > > properties prefixed with "listeners.https.") will also affect > > MirrorMaker 2 > > > clusters with the internal REST server introduced by KIP-710 [2] enabled? > > > > > > 3. We don't need to specify in the KIP that the > > > org.apache.kafka.connect.runtime.rest.util.SSLUtils class will be > > removed, > > > since that class isn't part of public API (i.e., nobody should be using > > > that class directly in external projects). If you're ever in doubt about > > > which classes are part of the public API for the project, you can check > > the > > > Javadocs [3]; if it's part of our public API, it should be included in > > > them. The same applies for changes to the > > > org.apache.kafka.common.security.ssl.SslFactory class. > > > > > > 4. The test plan includes an integration test for "Default SSL behavior > > and > > > compatibility"--is this necessary? Doesn't the > > > existing > > org.apache.kafka.connect.integration.RestForwardingIntegrationTest > > > give us sufficient coverage already? Similarly, the test plan includes an > > > integration test for "RestClient creation" and calls out > > > the RestForwardingIntegrationTest--don't we already create RestClient > > > instances in that test (like here [4])? It seems like this part of the > > KIP > > > may implicitly include tests that are already covered by the existing > > code > > > base, but if that's the case, it'd be nice to see this clarified as the > > > assumption is usually that items in the test plan cover changes that will > > > have to be implemented for the KIP. > > > > > > 5. There are several methods in the SslEngineFactory interface that don't > > > seem applicable for Kafka Connect (or MM2): shouldBeRebuilt(Map<String, > > > Object> nextConfigs), reconfigurableConfigs(), and possibly keystore() > > and > > > truststore(). Does it make sense to require users to implement these? It > > > seems like a new interface may make more sense here. > > > > > > [1] - > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=128650952 > > > [2] - > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-710%3A+Full+support+for+distributed+mode+in+dedicated+MirrorMaker+2.0+clusters > > > [3] - > > https://kafka.apache.org/36/javadoc/index.html?overview-summary.html > > > [4] - > > > > > https://github.com/apache/kafka/blob/9dbee599f13997effd8f7e278fd7256b850c8813/connect/runtime/src/test/java/org/apache/kafka/connect/integration/RestForwardingIntegrationTest.java#L161 > > > > > > Cheers, > > > > > > Chris > > > > > > On Thu, Oct 12, 2023 at 7:40 AM Taras Ledkov <tled...@apache.org> wrote: > > > > > > > Hi Ashwin, > > > > > > > > > I was referring to (and did not understand) the removal of L141 in > > > > > > clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java > > > > This line is moved to "new" private method > > `instantiateSslEngineFactory0 > > > > `. Please take a look at the `SslFactory:L132` at the patch. > > > > Just dummy refactoring. > > > > > > > > > Yes, I think this class [SslEngineFactory] should be moved to > > something > > > > like `server-common` module - but would like any of the committers to > > > > comment on this. > > > > Sorry, not catch an idea. > > > > SslEngineFactory - public interface is placed at the 'clients' > > project. I > > > > don't know a more common place > > > > > >
