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/28 19:48:26 UTC
[GitHub] [kafka] mumrah opened a new pull request, #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
mumrah opened a new pull request, #12455:
URL: https://github.com/apache/kafka/pull/12455
This patch fixes an issue in KRaft where sensitive dynamic broker configs were failing to get updated on the brokers. In the ZK code path, we expect the sensitive config values to be encrypted in-place, and so the update logic was decrypting these values. In KRaft, we do not encrypt the values in ConfigRecords regardless of the type.
This PR defines a new passthrough password encoder which is used in KRaft mode only.
Most of the test cases in DynamicBrokerReconfigurationTest have been converted to also run in KRaft mode.
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932719870
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -537,13 +564,23 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
stopAndVerifyProduceConsume(producerThread, consumerThread)
}
- @Test
- def testConsecutiveConfigChange(): Unit = {
+ @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
+ @ValueSource(strings = Array("zk", "kraft"))
+ def testConsecutiveConfigChange(quorum: String): Unit = {
val topic2 = "testtopic2"
val topicProps = new Properties
topicProps.put(KafkaConfig.MinInSyncReplicasProp, "2")
- TestUtils.createTopic(zkClient, topic2, 1, replicationFactor = numServers, servers, topicProps)
- var log = servers.head.logManager.getLog(new TopicPartition(topic2, 0)).getOrElse(throw new IllegalStateException("Log not found"))
+ TestUtils.createTopicWithAdmin(adminClients.head, topic2, servers, numPartitions = 1, replicationFactor = numServers, topicConfig = topicProps)
+
+ def getLogOrThrow(tp: TopicPartition): UnifiedLog = {
+ var (logOpt, found) = TestUtils.computeUntilTrue {
+ servers.head.logManager.getLog(tp)
Review Comment:
this should be `brokers` not `servers`, right?
--
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] mumrah commented on pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12455:
URL: https://github.com/apache/kafka/pull/12455#issuecomment-1204298550
Manually cherry-picked to 3.3 as a687d4d3f687
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932718283
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -335,7 +361,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
@Test
def testKeyStoreAlter(): Unit = {
val topic2 = "testtopic2"
- TestUtils.createTopic(zkClient, topic2, numPartitions, replicationFactor = numServers, servers)
+ TestUtils.createTopicWithAdmin(adminClients.head, topic2, servers, numPartitions, replicationFactor = numServers)
Review Comment:
this should be `brokers`, not `servers`, right?
Also, can you add a comment referencing the JIRA for converting this test (I think you said you made one for this)
Also for any other unconverted ones I guess
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932717184
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -138,17 +151,23 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
props ++= securityProps(sslProperties1, KEYSTORE_PROPS, listenerPrefix(SecureExternal))
val kafkaConfig = KafkaConfig.fromProps(props)
- configureDynamicKeystoreInZooKeeper(kafkaConfig, sslProperties1)
+ if (isKRaftTest()) {
Review Comment:
since this block is empty, probably better to do `if (!isKraftTest()) { ... }`, right?
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932722682
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -111,15 +113,26 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
(0 until numServers).foreach { brokerId =>
- val props = TestUtils.createBrokerConfig(brokerId, zkConnect)
+ val props = if (isKRaftTest()) {
+ val properties = TestUtils.createBrokerConfig(brokerId, "")
Review Comment:
Hmm, you're supposed to pass `null` for `zkConnect` in order to get the KRaft setup, not empty string. Admittedly, this could be documented better.
This would probably avoid having to mess around with a lot of the stuff that you're doing below, like voter ids, controller listener names, etc. etc. `TestUtils#createBrokerConfig` should do that for you.
The only thing you probably really need to do explicitly here in zk vs. kraft mode is initialize (or not) ZkEnableSecureAclsProp. The rest should be done automatically.
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932724376
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -20,7 +20,6 @@ package kafka.admin
import java.nio.charset.StandardCharsets
import java.util.concurrent.TimeUnit
import java.util.{Collections, Properties}
-
Review Comment:
probably nice to avoid messing with the whitespace here
--
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] mumrah commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r933264814
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -20,7 +20,6 @@ package kafka.admin
import java.nio.charset.StandardCharsets
import java.util.concurrent.TimeUnit
import java.util.{Collections, Properties}
-
Review Comment:
yea, we need to figure out how to stop IntelliJ from doing this 🤔
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r934916979
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -1493,6 +1493,7 @@ class KafkaConfig private(doLog: Boolean, val props: java.util.Map[_, _], dynami
// Cache the current config to avoid acquiring read lock to access from dynamicConfig
@volatile private var currentConfig = this
+ val processRoles: Set[ProcessRole] = parseProcessRoles()
Review Comment:
thanks for the explanation. sounds good
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932720750
##########
core/src/main/scala/kafka/utils/PasswordEncoder.scala:
##########
@@ -38,6 +38,33 @@ object PasswordEncoder {
val IterationsProp = "iterations"
val EncyrptedPasswordProp = "encryptedPassword"
val PasswordLengthProp = "passwordLength"
+
+ def encrypting(secret: Password,
+ keyFactoryAlgorithm: Option[String],
+ cipherAlgorithm: String,
+ keyLength: Int,
+ iterations: Int): EncryptingPasswordEncoder = {
+ new EncryptingPasswordEncoder(secret, keyFactoryAlgorithm, cipherAlgorithm, keyLength, iterations)
+ }
+
+ def passthrough(): PassthroughPasswordEncoder = {
+ new PassthroughPasswordEncoder()
+ }
+}
+
+trait PasswordEncoder {
+ def encode(password: Password): String
+ def decode(encodedPassword: String): Password
+
+ private[utils] def base64Decode(encoded: String): Array[Byte] = Base64.getDecoder.decode(encoded)
+}
+
+/**
+ * A password encoder that does not modify the given password. This is used in KRaft mode only.
Review Comment:
hmm, not sure about "passthrough". how do you like `NoOpPasswordEncoder` as a name
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932724051
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -1493,6 +1493,7 @@ class KafkaConfig private(doLog: Boolean, val props: java.util.Map[_, _], dynami
// Cache the current config to avoid acquiring read lock to access from dynamicConfig
@volatile private var currentConfig = this
+ val processRoles: Set[ProcessRole] = parseProcessRoles()
Review Comment:
hmm, what's the reason to move this?
--
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] mumrah merged pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah merged PR #12455:
URL: https://github.com/apache/kafka/pull/12455
--
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] dengziming commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
dengziming commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r934084148
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -20,7 +20,6 @@ package kafka.admin
import java.nio.charset.StandardCharsets
import java.util.concurrent.TimeUnit
import java.util.{Collections, Properties}
-
Review Comment:
IntelliJ has a default scalafmt config, we also have a config at checkstyle/.scalafmt.conf but it's only applied to strams-scala module.
--
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] mumrah commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r933262928
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -1493,6 +1493,7 @@ class KafkaConfig private(doLog: Boolean, val props: java.util.Map[_, _], dynami
// Cache the current config to avoid acquiring read lock to access from dynamicConfig
@volatile private var currentConfig = this
+ val processRoles: Set[ProcessRole] = parseProcessRoles()
Review Comment:
On the line below, we are creating the DynamicBrokerConfig which gets a partially initialized KafkaConfig. In this PR, we're now reading the `processRoles` to determine which encoder to create. Since the KafkaConfig isn't fully initialized, this was null when DynamicBrokerConfig was constructed.
Moving `parseProcessRoles` up here seemed simpler than refactoring a bunch of this config code.
--
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] mimaison commented on pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #12455:
URL: https://github.com/apache/kafka/pull/12455#issuecomment-1208297825
The backport of this commit to 3.2 broke the build, I get the following failures:
```
DynamicBrokerReconfigurationTest. testConfigDescribeUsingAdminClient(String).quorum=kraft
DynamicBrokerReconfigurationTest. testConsecutiveConfigChange(String).quorum=kraft
DynamicBrokerReconfigurationTest. testKeyStoreAlter(String).quorum=kraft
DynamicBrokerReconfigurationTest. testLogCleanerConfig(String).quorum=kraft
DynamicBrokerReconfigurationTest. testTrustStoreAlter(String).quorum=kraft
DynamicBrokerReconfigurationTest. testUpdatesUsingConfigProvider(String).quorum=kraft
```
Cause by:
```
org.apache.kafka.common.errors.InvalidRequestException: Invalid value org.apache.kafka.common.config.ConfigException: Dynamic reconfiguration of listeners is not yet supported when using a Raft-based metadata quorum for configuration Invalid dynamic configuration
```
I opened https://issues.apache.org/jira/browse/KAFKA-14149
--
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] mumrah commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r933262928
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -1493,6 +1493,7 @@ class KafkaConfig private(doLog: Boolean, val props: java.util.Map[_, _], dynami
// Cache the current config to avoid acquiring read lock to access from dynamicConfig
@volatile private var currentConfig = this
+ val processRoles: Set[ProcessRole] = parseProcessRoles()
Review Comment:
On the line below, we are creating the DynamicBrokerConfig which gets a partially initialized KafkaConfig. Moving `parseProcessRoles` up here was simpler than refactoring a bunch of this config code :)
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932718283
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -335,7 +361,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
@Test
def testKeyStoreAlter(): Unit = {
val topic2 = "testtopic2"
- TestUtils.createTopic(zkClient, topic2, numPartitions, replicationFactor = numServers, servers)
+ TestUtils.createTopicWithAdmin(adminClients.head, topic2, servers, numPartitions, replicationFactor = numServers)
Review Comment:
Can you add a comment referencing the JIRA for converting this test (I think you said you made one for this)
Also for any other unconverted ones I guess
--
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] cmccabe commented on a diff in pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #12455:
URL: https://github.com/apache/kafka/pull/12455#discussion_r932718618
##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -431,7 +457,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
}
def verifyBrokerToControllerCall(controller: KafkaServer): Unit = {
- val nonControllerBroker = servers.find(_.config.brokerId != controller.config.brokerId).get
+ val nonControllerBroker = servers.find(_.config.brokerId != controller.config.brokerId).get.asInstanceOf[KafkaServer]
Review Comment:
is this typecast 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
[GitHub] [kafka] mumrah commented on pull request #12455: KAFKA-14111 Fix sensitive dynamic broker configs in KRaft
Posted by GitBox <gi...@apache.org>.
mumrah commented on PR #12455:
URL: https://github.com/apache/kafka/pull/12455#issuecomment-1204262912
After latest commit, only 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