You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/07/05 18:05:38 UTC
[GitHub] [kafka] divijvaidya opened a new pull request, #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
divijvaidya opened a new pull request, #12381:
URL: https://github.com/apache/kafka/pull/12381
**Scenario**
The scenario is explained in details in https://issues.apache.org/jira/browse/KAFKA-13474 but as a summary:
When a certificate is rotated on a broker via dynamic configuration and the previous certificate expires, the broker to controller connection starts failing with `SSL Handshake failed`.
**Why**
A similar fix was earlier performed in https://github.com/apache/kafka/pull/6721 but when `BrokerToControllerChannelManager` was introduced in v2.7, we didn't enable dynamic reconfiguration for it's channel.
**Summary of testing strategy (including rationale)**
Add a test which fails prior to the fix done in the PR and succeeds afterwards. The bug wasn't caught earlier because there was no test coverage to validate the scenario.
Note: I would suggest that we backport this fix to all versions until 2.7
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon merged PR #12381:
URL: https://github.com/apache/kafka/pull/12381
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1177319338
> Sorry, looks like the `match` case still needs default case:
>
> ```
> [Error] /home/jenkins/workspace/Kafka_kafka-pr_PR-12381/core/src/main/scala/kafka/server/BrokerToControllerChannelManager.scalaː190ː7: match may not be exhaustive.
> ```
My bad Luke! I did a hasty change yesterday without verifying the build. I have fixed it now.
I appreciate your quick responses on this PR. If you get time please take a look at my other open PRs as well: https://github.com/apache/kafka/pulls/divijvaidya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1179497415
@showuon please take a look again when you get a chance. The test failures are unrelated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1179518774
back port back to 3.2 branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12381:
URL: https://github.com/apache/kafka/pull/12381#discussion_r914730050
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -429,6 +430,20 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
verifyProduceConsume(producer, consumer, 10, topic)
}
+ def verifyBrokerToControllerCall(controller: KafkaServer): Unit = {
+ val nonControllerBroker = servers.find(_.config.brokerId != controller.config.brokerId).get
+ val brokerToControllerManager = nonControllerBroker.clientToControllerChannelManager
+ val completionHandler = new TestControllerRequestCompletionHandler()
+ brokerToControllerManager.sendRequest(new MetadataRequest.Builder(new MetadataRequestData()), completionHandler)
+ TestUtils.waitUntilTrue(() => {
+ completionHandler.completed.get() || completionHandler.timedOut.get()
+ }, "Timed out while waiting for broker to controller API call")
+ val response = completionHandler.actualResponse.getOrElse(throw new IllegalStateException("No response recorded even though request is completed"))
Review Comment:
Both your comments make sense. I have make the changes as per your suggestions.
##########
core/src/main/scala/kafka/server/BrokerToControllerChannelManager.scala:
##########
@@ -189,6 +188,10 @@ class BrokerToControllerChannelManagerImpl(
config.saslInterBrokerHandshakeRequestEnable,
logContext
)
+ channelBuilder match {
+ case reconfigurable: Reconfigurable => config.addReconfigurable(reconfigurable)
+ case _ =>
Review Comment:
removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1177230649
Sorry, looks like the `match` case still needs default case:
```
[Error] /home/jenkins/workspace/Kafka_kafka-pr_PR-12381/core/src/main/scala/kafka/server/BrokerToControllerChannelManager.scalaː190ː7: match may not be exhaustive.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1181370970
@divijvaidya , usually we backported to the previous version only since patch release usually has one only. I just backported back to 3.1 branch, too. It took me some time to fix the conflict and make sure test works. If you think it should backport to versions >= 2.7, welcome to submit PRs against each version, I'll help review and merge them. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on PR #12381:
URL: https://github.com/apache/kafka/pull/12381#issuecomment-1180373398
Hey @showuon
How do we make the decision on what version do we want to backport a bug to? This bug exists in versions >= 2.7.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12381:
URL: https://github.com/apache/kafka/pull/12381#discussion_r914394460
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -429,6 +430,20 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
verifyProduceConsume(producer, consumer, 10, topic)
}
+ def verifyBrokerToControllerCall(controller: KafkaServer): Unit = {
+ val nonControllerBroker = servers.find(_.config.brokerId != controller.config.brokerId).get
+ val brokerToControllerManager = nonControllerBroker.clientToControllerChannelManager
+ val completionHandler = new TestControllerRequestCompletionHandler()
+ brokerToControllerManager.sendRequest(new MetadataRequest.Builder(new MetadataRequestData()), completionHandler)
+ TestUtils.waitUntilTrue(() => {
+ completionHandler.completed.get() || completionHandler.timedOut.get()
+ }, "Timed out while waiting for broker to controller API call")
+ val response = completionHandler.actualResponse.getOrElse(throw new IllegalStateException("No response recorded even though request is completed"))
Review Comment:
throwing exception in the test seems not a good practice. Maybe we can change to this:
```
assertTrue(completionHandler.actualResponse.isDefined, "No response recorded even though request is completed")
val response = completionHandler.actualResponse.get
```
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -429,6 +430,20 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
verifyProduceConsume(producer, consumer, 10, topic)
}
+ def verifyBrokerToControllerCall(controller: KafkaServer): Unit = {
+ val nonControllerBroker = servers.find(_.config.brokerId != controller.config.brokerId).get
+ val brokerToControllerManager = nonControllerBroker.clientToControllerChannelManager
+ val completionHandler = new TestControllerRequestCompletionHandler()
+ brokerToControllerManager.sendRequest(new MetadataRequest.Builder(new MetadataRequestData()), completionHandler)
+ TestUtils.waitUntilTrue(() => {
+ completionHandler.completed.get() || completionHandler.timedOut.get()
+ }, "Timed out while waiting for broker to controller API call")
+ val response = completionHandler.actualResponse.getOrElse(throw new IllegalStateException("No response recorded even though request is completed"))
Review Comment:
Also, I think we should fail when timeout, so that we know what happened in the request. Ex:
```
assertTrue(completionHandler.timedOut.isEmpty, "broker to controller request is timeout")
```
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12381: KAFKA-13474: Allow reconfiguration of SSL certs for broker to controller connection
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12381:
URL: https://github.com/apache/kafka/pull/12381#discussion_r914443627
##########
core/src/main/scala/kafka/server/BrokerToControllerChannelManager.scala:
##########
@@ -189,6 +188,10 @@ class BrokerToControllerChannelManagerImpl(
config.saslInterBrokerHandshakeRequestEnable,
logContext
)
+ channelBuilder match {
+ case reconfigurable: Reconfigurable => config.addReconfigurable(reconfigurable)
+ case _ =>
Review Comment:
I think default case is not needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org