KAFKA-14111 Fix sensitive dynamic broker configs in KRaft by mumrah · Pull Request #12455 · apache/kafka

mumrah · 2022-07-28T19:48:24Z

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.

And enable dynamic broker reconfiguration test in KRaft mode

cmccabe · 2022-07-28T22:08:05Z

core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala

      val kafkaConfig = KafkaConfig.fromProps(props)
-      configureDynamicKeystoreInZooKeeper(kafkaConfig, sslProperties1)
+      if (isKRaftTest()) {



since this block is empty, probably better to do if (!isKraftTest()) { ... }, right?

core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala

cmccabe · 2022-07-28T22:10:54Z

core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala


    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]


is this typecast needed?

core/src/main/scala/kafka/utils/PasswordEncoder.scala

cmccabe · 2022-07-28T22:19:19Z

core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala


-      val props = TestUtils.createBrokerConfig(brokerId, zkConnect)
+      val props = if (isKRaftTest()) {
+        val properties = TestUtils.createBrokerConfig(brokerId, "")


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.

cmccabe · 2022-07-28T22:22:11Z

core/src/main/scala/kafka/server/KafkaConfig.scala


  // Cache the current config to avoid acquiring read lock to access from dynamicConfig
  @volatile private var currentConfig = this
+  val processRoles: Set[ProcessRole] = parseProcessRoles()


hmm, what's the reason to move this?

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.

thanks for the explanation. sounds good

cmccabe · 2022-07-28T22:22:53Z

core/src/main/scala/kafka/admin/ConfigCommand.scala

 import java.nio.charset.StandardCharsets
 import java.util.concurrent.TimeUnit
 import java.util.{Collections, Properties}
-


probably nice to avoid messing with the whitespace here

yea, we need to figure out how to stop IntelliJ from doing this 🤔

IntelliJ has a default scalafmt config, we also have a config at checkstyle/.scalafmt.conf but it's only applied to strams-scala module.

cmccabe

LGTM

mumrah · 2022-08-03T17:27:53Z

After latest commit, only test failures are unrelated.

Enable some of the dynamic broker reconfiguration tests in KRaft mode

mumrah · 2022-08-03T18:00:23Z

Manually cherry-picked to 3.3 as a687d4d

Enable some of the dynamic broker reconfiguration tests in KRaft mode

…(5 August 2022) Version related conflicts: * Jenkinsfile * gradle.properties * streams/quickstart/java/pom.xml * streams/quickstart/java/src/main/resources/archetype-resources/pom.xml * streams/quickstart/pom.xml * tests/kafkatest/__init__.py * tests/kafkatest/version.py * commit 'add7cd85baa61cd0e1430': (66 commits) KAFKA-14136 Generate ConfigRecord for brokers even if the value is unchanged (apache#12483) HOTFIX / KAFKA-14130: Reduce RackAwarenesssTest to unit Test (apache#12476) MINOR: Remove ARM/PowerPC builds from Jenkinsfile (apache#12380) KAFKA-14111 Fix sensitive dynamic broker configs in KRaft (apache#12455) KAFKA-13877: Fix flakiness in RackAwarenessIntegrationTest (apache#12468) KAFKA-14129: KRaft must check manual assignments for createTopics are contiguous (apache#12467) KAFKA-13546: Do not fail connector validation if default topic creation group is explicitly specified (apache#11615) KAFKA-14122: Fix flaky test DynamicBrokerReconfigurationTest#testKeyStoreAlter (apache#12452) MINOR; Use right enum value for broker registration change (apache#12236) MINOR; Synchronize access to snapshots' TreeMap (apache#12464) MINOR; Bump trunk to 3.4.0-SNAPSHOT (apache#12463) MINOR: Stop logging 404s at ERROR level in Connect KAFKA-14095: Improve handling of sync offset failures in MirrorMaker (apache#12432) Minor: enable index for emit final sliding window (apache#12461) MINOR: convert some more junit tests to support KRaft (apache#12456) KAFKA-14108: Ensure both JUnit 4 and JUnit 5 tests run (apache#12441) MINOR: Remove code of removed metric (apache#12453) MINOR: Update comment on verifyTaskGenerationAndOwnership method in DistributedHerder KAFKA-14012: Add warning to closeQuietly documentation about method references of null objects (apache#12321) MINOR: Fix static mock usage in ThreadMetricsTest (apache#12454) ...

mimaison · 2022-08-08T15:47:06Z

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

Fix sensitive dynamic broker configs in KRaft

c9b6acb

And enable dynamic broker reconfiguration test in KRaft mode

cmccabe reviewed Jul 28, 2022

View reviewed changes

core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala Show resolved Hide resolved

cmccabe reviewed Jul 28, 2022

View reviewed changes

core/src/main/scala/kafka/utils/PasswordEncoder.scala Show resolved Hide resolved

cmccabe reviewed Jul 28, 2022

View reviewed changes

mumrah added 2 commits July 29, 2022 10:13

PR feedback

2558bd0

whitespace fix

dc1fd0a

cmccabe approved these changes Aug 1, 2022

View reviewed changes

Fix testAddRemoveSaslListeners test

5dc024b

mumrah merged commit bfd9e6e into apache:trunk Aug 3, 2022

mumrah added a commit that referenced this pull request Aug 3, 2022

KAFKA-14111 Fix sensitive dynamic broker configs in KRaft (#12455)

a687d4d

Enable some of the dynamic broker reconfiguration tests in KRaft mode

mumrah added a commit that referenced this pull request Aug 4, 2022

KAFKA-14111 Fix sensitive dynamic broker configs in KRaft (#12455)

4e049c7

Enable some of the dynamic broker reconfiguration tests in KRaft mode

Conversation

mumrah commented Jul 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumrah Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe left a comment

Choose a reason for hiding this comment

Uh oh!

mumrah commented Aug 3, 2022

Uh oh!

mumrah commented Aug 3, 2022

Uh oh!

mimaison commented Aug 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mumrah Jul 29, 2022 •

edited

Loading