KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig #16824

brandboat · 2024-08-07T12:15:25Z

related to https://issues.apache.org/jira/browse/KAFKA-17256

Currently in ClusterConfig setListenerName and setSecurityProtocol not work under KRaft mode, while we could set it in zookeeper mode. This PR aims to align the behavior.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…l from ClusterConfig

chia7712 · 2024-08-10T17:30:21Z

@brandboat Could you please add tests?

chia7712 · 2024-08-20T20:10:46Z

@brandboat could you please rebase to trigger CI again?

chia7712

@brandboat thanks for this patch

chia7712 · 2024-08-22T01:48:52Z

core/src/test/java/kafka/test/ClusterTestExtensionsTest.java

@@ -251,8 +251,7 @@ public void testClusterAliveBrokers(ClusterInstance clusterInstance) throws Exce
        Assertions.assertTrue(clusterInstance.brokers().containsKey(0));
    }

-
-    @ClusterTest(types = {Type.ZK, Type.CO_KRAFT, Type.KRAFT}, brokers = 4)
+    @ClusterTest(types = {Type.ZK, Type.CO_KRAFT, Type.KRAFT}, brokers = 4, listener = "TEST_LISTENER")


why we need this change?

In the beginning I just want to show an example about how to set listener in annotation. But that's not needed, I will remove this in the next commit. Thanks.

chia7712 · 2024-08-22T01:55:03Z

core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java


        RaftClusterInstance(ClusterConfig clusterConfig, boolean isCombined) {
            this.clusterConfig = clusterConfig;
            this.isCombined = isCombined;
+            this.listenerName = clusterConfig.listenerName().map(ListenerName::normalised)
+                    .orElse(TestKitNodes.BROKER_LISTENER_NAME);


Could we align the behavior to zk?

clusterConfig.listenerName().map(ListenerName::normalised) .orElseGet(() -> ListenerName.forSecurityProtocol(securityProtocol()));

chia7712 · 2024-08-22T01:56:30Z

core/src/test/java/kafka/testkit/TestKitNodes.java

@@ -53,6 +57,8 @@ public static class Builder {
        private Map<Integer, Map<String, String>> perServerProperties = Collections.emptyMap();
        private BootstrapMetadata bootstrapMetadata = BootstrapMetadata.
            fromVersion(MetadataVersion.latestTesting(), "testkit");
+        private ListenerName listenerName = BROKER_LISTENER_NAME;
+        private SecurityProtocol securityProtocol = BROKER_SECURITY_PROTOCOL;


we don't need this default value as in IT the default value is defined by ClusterTest. We can add null check for securityProtocol instead

Not only ClusterTest use TestKitNode, there are many places like BrokerMetadataPublisherTest, KRaftClusterTest, ZkMigrationIntegrationTest directly TestKitNodes. That means we still need this default value.

(Update): I'll try to remove those default values, but that might need to change some test (perhaps add listener config or something else). Anyway, worth a try.

chia7712 · 2024-10-07T09:48:11Z

@brandboat do you have free time to rebase it?

brandboat · 2024-10-08T01:04:05Z

@brandboat do you have free time to rebase it?

Done, thanks for looking into this PR again.

chia7712

@brandboat thanks for this patch!

chia7712 · 2024-10-08T11:42:55Z

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

@@ -1155,6 +1199,8 @@ class KRaftClusterTest {
  def testAuthorizerFailureFoundInControllerStartup(): Unit = {
    val cluster = new KafkaClusterTestKit.Builder(
      new TestKitNodes.Builder().
+        setSecurityProtocol(SecurityProtocol.PLAINTEXT).


Could we add default value to securityProtocol and listener to avoid those duplicate code?

But this is what I addressed in #16824 (comment)... Or did you mean something else ?

Okay, that's a bit awkward. These tests don't use the annotation, so they can't reuse the default values of ClusterTest...

A possible workaround is to add default values for securityProtocol and listener. Additionally, we should add comments to remind developers that the default values need to stay consistent.

@brandboat WDYT?

OK, let's add back those default values, will add comments to the setters also. Thanks !

chia7712 · 2024-10-08T11:44:27Z

test-common/src/main/java/org/apache/kafka/common/test/TestKitNodes.java

@@ -96,6 +100,16 @@ public Builder setPerServerProperties(Map<Integer, Map<String, String>> perServe
            return this;
        }

+        public Builder setListenerName(ListenerName listenerName) {


Although we don't support setting a controller at the moment, it would still be better to rename it to setBrokerListenerName.

chia7712 · 2024-10-08T11:44:31Z

test-common/src/main/java/org/apache/kafka/common/test/TestKitNodes.java

+            return this;
+        }
+
+        public Builder setSecurityProtocol(SecurityProtocol securityProtocol) {


brandboat · 2024-10-09T02:34:31Z

Found 3 test failures:
FAILED ❌ MetricsDuringTopicCreationDeletionTest > "testMetricsDuringTopicCreateDelete(String).quorum=zk"
FAILED ❌ ShareConsumerTest > testExplicitAcknowledgementCommitAsync()
FAILED ❌ PlaintextAdminIntegrationTest > "testElectPreferredLeaders(String).quorum=kraft"

These 3 tests passed in my local env

chia7712

@brandboat thanks for your patch

chia7712 · 2024-10-09T06:13:43Z

test-common/test-common-api/src/main/java/org/apache/kafka/common/test/api/ClusterConfig.java

    }

-    public Optional<String> listenerName() {
-        return Optional.ofNullable(listenerName);
+    public ListenerName listenerName() {


Could you please align the naming?

chia7712 · 2024-10-09T06:13:45Z

test-common/test-common-api/src/main/java/org/apache/kafka/common/test/api/ClusterConfig.java

@@ -137,11 +140,11 @@ public Map<String, String> saslClientProperties() {
    }

    public SecurityProtocol securityProtocol() {


Could you please align the naming?

chia7712 · 2024-10-09T06:14:28Z

test-common/src/main/java/org/apache/kafka/common/test/TestKitNodes.java

 public class TestKitNodes {

    public static final int CONTROLLER_ID_OFFSET = 3000;
    public static final int BROKER_ID_OFFSET = 0;
+    public static final String DEFAULT_BROKER_LISTENER_NAME = "EXTERNAL";


Could you please also add DEFAULT_BROKER_SECURITY?

Thanks for the comments, already fixed in latest commit.

KAFKA-17256 KRAFT should honor the listener name and security protoco…

3847bed

…l from ClusterConfig

brandboat added 2 commits August 18, 2024 22:25

Add test

77929ca

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

cd1c729

brandboat marked this pull request as ready for review August 18, 2024 15:00

brandboat added 2 commits August 21, 2024 09:43

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

851fc32

fix import order

731e152

chia7712 reviewed Aug 22, 2024

View reviewed changes

brandboat added 4 commits August 29, 2024 21:57

Address comment

cc3c99a

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

661f444

fix checkstyle

eb5b0a6

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

f62d54a

brandboat added 3 commits October 7, 2024 22:12

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

bed09c6

Fix failed tests

7a6f549

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

8afe7db

github-actions bot added streams core Kafka Broker connect tests Test fixes (including flaky tests) labels Oct 7, 2024

Fix checkstyle

e2c8b38

github-actions bot added the tools label Oct 7, 2024

brandboat marked this pull request as draft October 7, 2024 16:08

brandboat marked this pull request as ready for review October 7, 2024 16:10

chia7712 reviewed Oct 8, 2024

View reviewed changes

brandboat added 3 commits October 9, 2024 08:37

Address comments

3400bf7

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

21fc733

Fix indention

46c7f5a

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

b533f51

chia7712 reviewed Oct 14, 2024

View reviewed changes

brandboat added 2 commits October 16, 2024 23:59

Address comments

708d789

Merge remote-tracking branch 'origin/trunk' into KAFKA-17256

23d6556

chia7712 approved these changes Oct 17, 2024

View reviewed changes

chia7712 merged commit 8adfdbb into apache:trunk Oct 17, 2024
6 checks passed

brandboat deleted the KAFKA-17256 branch October 17, 2024 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig #16824

KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig #16824

brandboat commented Aug 7, 2024

chia7712 commented Aug 10, 2024

chia7712 commented Aug 20, 2024

chia7712 left a comment

chia7712 Aug 22, 2024

brandboat Aug 26, 2024 •

edited

Loading

chia7712 Aug 22, 2024

chia7712 Aug 22, 2024

brandboat Aug 27, 2024 •

edited

Loading

chia7712 commented Oct 7, 2024

brandboat commented Oct 8, 2024

chia7712 left a comment

chia7712 Oct 8, 2024

brandboat Oct 8, 2024

chia7712 Oct 8, 2024

brandboat Oct 8, 2024

chia7712 Oct 8, 2024

chia7712 Oct 8, 2024

brandboat commented Oct 9, 2024

chia7712 left a comment

chia7712 Oct 9, 2024

chia7712 Oct 9, 2024

chia7712 Oct 9, 2024

brandboat Oct 16, 2024

		@@ -137,11 +140,11 @@ public Map<String, String> saslClientProperties() {
		}

		public SecurityProtocol securityProtocol() {

KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig #16824

KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig #16824

Conversation

brandboat commented Aug 7, 2024

Committer Checklist (excluded from commit message)

chia7712 commented Aug 10, 2024

chia7712 commented Aug 20, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandboat Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandboat Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

chia7712 commented Oct 7, 2024

brandboat commented Oct 8, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandboat commented Oct 9, 2024

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandboat Aug 26, 2024 •

edited

Loading

brandboat Aug 27, 2024 •

edited

Loading