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