Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi team, Excuse me for duplicate. The last message isn't attached to the thread. I try to fix it by this email. Ping for review / vote for KIP-967 [1]. Voting thread is here [2] Please pay your attention, I beg of you. I'm a little frustrated by the lack of community interest in what seems like a simple and necessary patch. Is the SslEngineFactory refactoring requirement blocking? [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi team, Ping for review / vote for KIP-967 [1]. Voting thread is here [2] Please pay your attention, I beg of you. I'm a little frustrated by the lack of community interest in what seems like a simple and necessary patch. Is the SslEngineFactory refactoring requirement blocking? [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw -- With best regards, Taras Ledkov
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi team, Ping for review / vote for KIP-967 [1]. Voting thread is here [2] [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw -- With best regards, Taras Ledkov On Wed, Dec 6, 2023 at 2:21 PM Taras Ledkov wrote: > > Hi Greg, > > > Taras, are you interested in dynamic SSL reconfiguration in Connect? > > Would you be willing to investigate the details of that for the KIP? > I would prefer to separate this functionality into the next KIP. But > if you / (the community) consider it necessary to combine this in one > KIP/patch, then I’m ready to do it. > > The entry point for dynamic reconfiguration for Connect is not clear for me. > For a broker it is the DynamicConfig functionality. Should we add a > REST endpoint to reconfigure Connect or use another way? > > -- > With best regards, > Taras Ledkov > > On Tue, Dec 5, 2023 at 2:42 AM Greg Harris > wrote: > > > > Hi Chris, > > > > Thank you for your comments above. I disagree with your recommendation > > for a new SslEngineFactory variant/hierarchy. > > > > 1. A superinterface could be more confusing to users. Since this is an > > interface in `clients`, the connect-specific interface would also need > > to be in clients, despite being superfluous for clients users and > > broker developers. > > 2. A user could implement the reduced interface, and then have issues > > loading their implementation in a broker, and need to find > > documentation/javadocs to explain to them why. > > 3. This interface has been in use since 2020, so I don't believe that > > the burden of implementing these methods has been excessive. I assume > > there's at least one "static" implementation out there that would have > > benefitted from the proposed super-interface, but they managed to > > adapt to the standardized interface. > > 4. Implementations that don't want to provide basic implementations > > can throw UnsupportedOperationException from the extra methods as a > > last resort. > > > > On the other hand, how much would it take to support the full suite of > > SslEngineFactory operations in Connect? Could part of this KIP be > > improving Connect to make use of these methods? This would help unify > > the experience for users that implement the interface specifically for > > the dynamic reconfiguration support, and rely on it on the broker > > side. > > > > Taras, are you interested in dynamic SSL reconfiguration in Connect? > > Would you be willing to investigate the details of that for the KIP? > > > > Thanks, > > Greg > > > > On Mon, Dec 4, 2023 at 1:17 PM Chris Egerton > > wrote: > > > > > > Hi Taras, > > > > > > Regarding slimming down the interface: IMO, we should do this right the > > > first time, and that includes not requiring unnecessary methods from > > > users. > > > I think BaseSslEngineFactory is good enough as a superinterface. > > > > > > > > > Regarding the parsing logic: I think the KIP needs to be more explicit. We > > > should add something like this to the proposed changes section: > > > > > > "If any properties are present in the worker config with a prefix of > > > "listeners.https.", then only properties with that prefix will be passed > > > to > > > the SSL engine factory. Otherwise, all top-level worker properties will be > > > passed to the SSL engine factory. Note that this differs slightly from > > > existing logic in that the set of properties (prefixed or otherwise) will > > > not be filtered based on a predefined set of keys; this will enable custom > > > SSL engine factories to define and accept custom properties." > > > > > > I also took a quick look at the prototype (I usually try not to do this > > > since we vote on KIP documents, not PRs). I don't think we should populate > > > default values for SSL-related properties before sending properties to the > > > SSL engine factory, since it may confuse users who have written custom SSL > > > engine factories to see that properties not specified in their Connect > > > worker config are being passed to their factory. Instead, the default SSL > > > engine factory used by Connect can perform this logic, and we can let > > > other > > > custom factories be responsible for their own default values. > > > > > > > > > Cheers, > > > > > > Chris > > > > > > On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov wrote: > > > > > > > Hi team, > > > > > > > > Ping for review / vote for KIP-967 [1]. > > > > Voting thread is here [2] > > > > > > > > [1]. > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > > > > [2]. https://github.com/apache/kafka/pull/14203 > > > > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > > > > > > > -- > > > > With best regards, > > > > Taras Ledkov > > > >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Greg, > Taras, are you interested in dynamic SSL reconfiguration in Connect? > Would you be willing to investigate the details of that for the KIP? I would prefer to separate this functionality into the next KIP. But if you / (the community) consider it necessary to combine this in one KIP/patch, then I’m ready to do it. The entry point for dynamic reconfiguration for Connect is not clear for me. For a broker it is the DynamicConfig functionality. Should we add a REST endpoint to reconfigure Connect or use another way? -- With best regards, Taras Ledkov On Tue, Dec 5, 2023 at 2:42 AM Greg Harris wrote: > > Hi Chris, > > Thank you for your comments above. I disagree with your recommendation > for a new SslEngineFactory variant/hierarchy. > > 1. A superinterface could be more confusing to users. Since this is an > interface in `clients`, the connect-specific interface would also need > to be in clients, despite being superfluous for clients users and > broker developers. > 2. A user could implement the reduced interface, and then have issues > loading their implementation in a broker, and need to find > documentation/javadocs to explain to them why. > 3. This interface has been in use since 2020, so I don't believe that > the burden of implementing these methods has been excessive. I assume > there's at least one "static" implementation out there that would have > benefitted from the proposed super-interface, but they managed to > adapt to the standardized interface. > 4. Implementations that don't want to provide basic implementations > can throw UnsupportedOperationException from the extra methods as a > last resort. > > On the other hand, how much would it take to support the full suite of > SslEngineFactory operations in Connect? Could part of this KIP be > improving Connect to make use of these methods? This would help unify > the experience for users that implement the interface specifically for > the dynamic reconfiguration support, and rely on it on the broker > side. > > Taras, are you interested in dynamic SSL reconfiguration in Connect? > Would you be willing to investigate the details of that for the KIP? > > Thanks, > Greg > > On Mon, Dec 4, 2023 at 1:17 PM Chris Egerton wrote: > > > > Hi Taras, > > > > Regarding slimming down the interface: IMO, we should do this right the > > first time, and that includes not requiring unnecessary methods from users. > > I think BaseSslEngineFactory is good enough as a superinterface. > > > > > > Regarding the parsing logic: I think the KIP needs to be more explicit. We > > should add something like this to the proposed changes section: > > > > "If any properties are present in the worker config with a prefix of > > "listeners.https.", then only properties with that prefix will be passed to > > the SSL engine factory. Otherwise, all top-level worker properties will be > > passed to the SSL engine factory. Note that this differs slightly from > > existing logic in that the set of properties (prefixed or otherwise) will > > not be filtered based on a predefined set of keys; this will enable custom > > SSL engine factories to define and accept custom properties." > > > > I also took a quick look at the prototype (I usually try not to do this > > since we vote on KIP documents, not PRs). I don't think we should populate > > default values for SSL-related properties before sending properties to the > > SSL engine factory, since it may confuse users who have written custom SSL > > engine factories to see that properties not specified in their Connect > > worker config are being passed to their factory. Instead, the default SSL > > engine factory used by Connect can perform this logic, and we can let other > > custom factories be responsible for their own default values. > > > > > > Cheers, > > > > Chris > > > > On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov wrote: > > > > > Hi team, > > > > > > Ping for review / vote for KIP-967 [1]. > > > Voting thread is here [2] > > > > > > [1]. > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > > > [2]. https://github.com/apache/kafka/pull/14203 > > > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > > > > > -- > > > With best regards, > > > Taras Ledkov > > >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Chris, > I don't think we should populate default values for SSL-related properties > before sending properties to the SSL engine factory, > since it may confuse users who have written custom SSL engine factories > to see that properties not specified in their Connect worker config are being > passed to their factory. I slightly disagree with this.. The behavior implemented in PR is consistent with SSL configuration for clients and brokers. This behavior does not prevent the implementation of custom SSL factories for the broker and client. But you are right about reviewing PR and KIP. Let's focus on the KIP. -- With best regards, Taras Ledkov On Tue, Dec 5, 2023 at 12:18 AM Chris Egerton wrote: > > Hi Taras, > > Regarding slimming down the interface: IMO, we should do this right the > first time, and that includes not requiring unnecessary methods from users. > I think BaseSslEngineFactory is good enough as a superinterface. > > > Regarding the parsing logic: I think the KIP needs to be more explicit. We > should add something like this to the proposed changes section: > > "If any properties are present in the worker config with a prefix of > "listeners.https.", then only properties with that prefix will be passed to > the SSL engine factory. Otherwise, all top-level worker properties will be > passed to the SSL engine factory. Note that this differs slightly from > existing logic in that the set of properties (prefixed or otherwise) will > not be filtered based on a predefined set of keys; this will enable custom > SSL engine factories to define and accept custom properties." > > I also took a quick look at the prototype (I usually try not to do this > since we vote on KIP documents, not PRs). I don't think we should populate > default values for SSL-related properties before sending properties to the > SSL engine factory, since it may confuse users who have written custom SSL > engine factories to see that properties not specified in their Connect > worker config are being passed to their factory. Instead, the default SSL > engine factory used by Connect can perform this logic, and we can let other > custom factories be responsible for their own default values. > > > Cheers, > > Chris > > On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov wrote: > > > Hi team, > > > > Ping for review / vote for KIP-967 [1]. > > Voting thread is here [2] > > > > [1]. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > > [2]. https://github.com/apache/kafka/pull/14203 > > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > > > -- > > With best regards, > > Taras Ledkov > >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Chris, Thank you for your comments above. I disagree with your recommendation for a new SslEngineFactory variant/hierarchy. 1. A superinterface could be more confusing to users. Since this is an interface in `clients`, the connect-specific interface would also need to be in clients, despite being superfluous for clients users and broker developers. 2. A user could implement the reduced interface, and then have issues loading their implementation in a broker, and need to find documentation/javadocs to explain to them why. 3. This interface has been in use since 2020, so I don't believe that the burden of implementing these methods has been excessive. I assume there's at least one "static" implementation out there that would have benefitted from the proposed super-interface, but they managed to adapt to the standardized interface. 4. Implementations that don't want to provide basic implementations can throw UnsupportedOperationException from the extra methods as a last resort. On the other hand, how much would it take to support the full suite of SslEngineFactory operations in Connect? Could part of this KIP be improving Connect to make use of these methods? This would help unify the experience for users that implement the interface specifically for the dynamic reconfiguration support, and rely on it on the broker side. Taras, are you interested in dynamic SSL reconfiguration in Connect? Would you be willing to investigate the details of that for the KIP? Thanks, Greg On Mon, Dec 4, 2023 at 1:17 PM Chris Egerton wrote: > > Hi Taras, > > Regarding slimming down the interface: IMO, we should do this right the > first time, and that includes not requiring unnecessary methods from users. > I think BaseSslEngineFactory is good enough as a superinterface. > > > Regarding the parsing logic: I think the KIP needs to be more explicit. We > should add something like this to the proposed changes section: > > "If any properties are present in the worker config with a prefix of > "listeners.https.", then only properties with that prefix will be passed to > the SSL engine factory. Otherwise, all top-level worker properties will be > passed to the SSL engine factory. Note that this differs slightly from > existing logic in that the set of properties (prefixed or otherwise) will > not be filtered based on a predefined set of keys; this will enable custom > SSL engine factories to define and accept custom properties." > > I also took a quick look at the prototype (I usually try not to do this > since we vote on KIP documents, not PRs). I don't think we should populate > default values for SSL-related properties before sending properties to the > SSL engine factory, since it may confuse users who have written custom SSL > engine factories to see that properties not specified in their Connect > worker config are being passed to their factory. Instead, the default SSL > engine factory used by Connect can perform this logic, and we can let other > custom factories be responsible for their own default values. > > > Cheers, > > Chris > > On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov wrote: > > > Hi team, > > > > Ping for review / vote for KIP-967 [1]. > > Voting thread is here [2] > > > > [1]. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > > [2]. https://github.com/apache/kafka/pull/14203 > > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > > > -- > > With best regards, > > Taras Ledkov > >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Taras, Regarding slimming down the interface: IMO, we should do this right the first time, and that includes not requiring unnecessary methods from users. I think BaseSslEngineFactory is good enough as a superinterface. Regarding the parsing logic: I think the KIP needs to be more explicit. We should add something like this to the proposed changes section: "If any properties are present in the worker config with a prefix of "listeners.https.", then only properties with that prefix will be passed to the SSL engine factory. Otherwise, all top-level worker properties will be passed to the SSL engine factory. Note that this differs slightly from existing logic in that the set of properties (prefixed or otherwise) will not be filtered based on a predefined set of keys; this will enable custom SSL engine factories to define and accept custom properties." I also took a quick look at the prototype (I usually try not to do this since we vote on KIP documents, not PRs). I don't think we should populate default values for SSL-related properties before sending properties to the SSL engine factory, since it may confuse users who have written custom SSL engine factories to see that properties not specified in their Connect worker config are being passed to their factory. Instead, the default SSL engine factory used by Connect can perform this logic, and we can let other custom factories be responsible for their own default values. Cheers, Chris On Wed, Nov 29, 2023 at 8:36 AM Taras Ledkov wrote: > Hi team, > > Ping for review / vote for KIP-967 [1]. > Voting thread is here [2] > > [1]. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > [2]. https://github.com/apache/kafka/pull/14203 > [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw > > -- > With best regards, > Taras Ledkov >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi team, Ping for review / vote for KIP-967 [1]. Voting thread is here [2] [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://github.com/apache/kafka/pull/14203 [2]. https://lists.apache.org/thread/wc4v5v3pynl15g1q547m2croqsqgmzpw -- With best regards, Taras Ledkov
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
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 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] - >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
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 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
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
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 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 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 >
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
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 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 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 >
Re:[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
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
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hello Taras, > Do you think that something needs to be corrected in KIP to make it more understandable without PR? Do you have any advice? Ha - no. I just wanted to thank you for sharing the PR which helped me as a newbie. > If I understood the question correctly: 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 in https://github.com/apache/kafka/pull/14203/files. But you are right - this can be discussed in the final PR. > There might be a better place for this public static method that creates the SslEngineFactory. Yes, I think this class should be moved to something like `server-common` module - but would like any of the committers to comment on this. Thanks, Ashwin On Fri, Sep 29, 2023 at 9:22 PM Taras Ledkov wrote: > Hi Ashwin, > > Thanks a lot for your review. > > > Thanks for the KIP and the PR (which helped me understand the change). > Do you think that something needs to be corrected in KIP to make it more > understandable without PR? Do you have any advice? > > > I could not understand one thing though - In > https://github.com/apache/kafka/pull/14203/, > > why did you have to remove the code which sets sslEngineFactoryConfig in > instantiateSslEngineFactory? > If I understood the question correctly: > I've refactored this method a bit. > SslFactory#instantiateSslEngineFactory was a private not-static method. > I've separated the code that really creates new instance of the > SslEngineFactory and place it into a public static method. There might be a > better place for this public static method that creates the > SslEngineFactory. I think we will discuss this at the final PR. Current PR > is just a demo / prototype to play. >
Re:[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi, Kafka team. 1. Ping to review KIP. 2. I dare say that the low activity in the discussion of KIP-967 means that KIP-967 is ready for voting? -- With best regards Taras Ledkov
Re:[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Ashwin, Thanks a lot for your review. > Thanks for the KIP and the PR (which helped me understand the change). Do you think that something needs to be corrected in KIP to make it more understandable without PR? Do you have any advice? > I could not understand one thing though - In > https://github.com/apache/kafka/pull/14203/, > why did you have to remove the code which sets sslEngineFactoryConfig in > instantiateSslEngineFactory? If I understood the question correctly: I've refactored this method a bit. SslFactory#instantiateSslEngineFactory was a private not-static method. I've separated the code that really creates new instance of the SslEngineFactory and place it into a public static method. There might be a better place for this public static method that creates the SslEngineFactory. I think we will discuss this at the final PR. Current PR is just a demo / prototype to play.
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Taras, Thanks for the KIP and the PR (which helped me understand the change). This is a useful feature, change is small and reuses existing functionality in clients/../SslFactory.java - so hopefully, this KIP will get accepted. I could not understand one thing though - In https://github.com/apache/kafka/pull/14203/, why did you have to remove the code which sets sslEngineFactoryConfig in instantiateSslEngineFactory ? Thanks, Ashwin On Tue, Sep 26, 2023 at 6:39 PM Taras Ledkov wrote: > Hi Kafka Team. > > Ping... >
Re:[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Kafka Team. Ping...
Re:[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Kafka Team. Looks like the code freeze of 3.6.0 release done. I hope that community members have more time for review. Please pay your attention for the KIP-967: Support custom SSL configuration for Kafka Connect RestServer [1]. The purpose of this KIP is add ability to use custom SSL factory to configure Kafka Connect RestServer. It looks like the interface 'SslEngineFactory' may be used with simple adapters. The prototype of the patch is available on PR#14203 [2]. It is not a final/clean patch yet. Just for demo & discuss. Thanks in advance for leaving a review! [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://github.com/apache/kafka/pull/14203
Re: [DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hello, Taras. I found this KIP useful. We already has an ability to setup custom SslEngineFactory via ‘ssl.engine.factory.class' So it’s looks logical to extend this feature to connect rest. AFAIK many organization adopts custom SSL storage like HashiCorp Vault or similar so native integration will be useful > 14 авг. 2023 г., в 12:42, Taras Ledkov написал(а): > > Hi Kafka Team. > > I would like to start a discussion for KIP-967: Support custom SSL > configuration for Kafka Connect RestServer [1]. > The purpose of this KIP is add ability to use custom SSL factory to configure > Kafka Connect RestServer. > It looks like the interface 'SslEngineFactory' may be used with simple > adapters. > > The prototype of the patch is available on PR#14203 [2]. > It is not a final/clean patch yet. Just for demo & discuss. > > Thanks in advance for leaving a review! > > [1]. > https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer > [2]. https://github.com/apache/kafka/pull/14203 > > -- > With best regards, > Taras Ledkov
[DISCUSS] KIP-967: Support custom SSL configuration for Kafka Connect RestServer
Hi Kafka Team. I would like to start a discussion for KIP-967: Support custom SSL configuration for Kafka Connect RestServer [1]. The purpose of this KIP is add ability to use custom SSL factory to configure Kafka Connect RestServer. It looks like the interface 'SslEngineFactory' may be used with simple adapters. The prototype of the patch is available on PR#14203 [2]. It is not a final/clean patch yet. Just for demo & discuss. Thanks in advance for leaving a review! [1]. https://cwiki.apache.org/confluence/display/KAFKA/KIP-967%3A+Support+custom+SSL+configuration+for+Kafka+Connect+RestServer [2]. https://github.com/apache/kafka/pull/14203 -- With best regards, Taras Ledkov
