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