You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "brandboat (via GitHub)" <gi...@apache.org> on 2024/08/07 12:15:28 UTC
[PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
brandboat opened a new pull request, #16824:
URL: https://github.com/apache/kafka/pull/16824
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)
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16824:
URL: https://github.com/apache/kafka/pull/16824#issuecomment-2282219378
@brandboat Could you please add tests?
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1732856379
##########
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;
Review Comment:
~~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.
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1792901017
##########
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() {
- return securityProtocol;
+ return brokerSecurityProtocol;
}
- public Optional<String> listenerName() {
- return Optional.ofNullable(listenerName);
+ public ListenerName listenerName() {
Review Comment:
Could you please align the naming?
##########
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() {
Review Comment:
Could you please align the naming?
##########
test-common/src/main/java/org/apache/kafka/common/test/TestKitNodes.java:
##########
@@ -39,10 +40,12 @@
import java.util.stream.IntStream;
import java.util.stream.Stream;
+@SuppressWarnings("NPathComplexity")
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";
Review Comment:
Could you please also add `DEFAULT_BROKER_SECURITY`?
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1731353531
##########
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")
Review Comment:
In the beginning I just want to show an example about how to set listener in annotation. But that's not needed, I will remote in the next commit. Thanks.
##########
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")
Review Comment:
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.
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16824:
URL: https://github.com/apache/kafka/pull/16824#issuecomment-2299680381
@brandboat could you please rebase to trigger CI again?
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #16824:
URL: https://github.com/apache/kafka/pull/16824#issuecomment-2401151571
> 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
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1726136316
##########
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")
Review Comment:
why we need this change?
##########
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;
Review Comment:
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
##########
core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java:
##########
@@ -112,10 +112,13 @@ public static class RaftClusterInstance implements ClusterInstance {
private EmbeddedZookeeper embeddedZookeeper;
private KafkaClusterTestKit clusterTestKit;
private final boolean isCombined;
+ private final ListenerName listenerName;
RaftClusterInstance(ClusterConfig clusterConfig, boolean isCombined) {
this.clusterConfig = clusterConfig;
this.isCombined = isCombined;
+ this.listenerName = clusterConfig.listenerName().map(ListenerName::normalised)
+ .orElse(TestKitNodes.BROKER_LISTENER_NAME);
Review Comment:
Could we align the behavior to zk?
```java
clusterConfig.listenerName().map(ListenerName::normalised)
.orElseGet(() -> ListenerName.forSecurityProtocol(securityProtocol()));
```
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1792079089
##########
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).
Review Comment:
OK, let's add back those default values, will add comments to the setters also. 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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1732856379
##########
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;
Review Comment:
Not only ClusterTest use TestKitNode, there are many places like BrokerMetadataPublisherTest, KRaftClusterTest, ZkMigrationIntegrationTest directly TestKitNodes. That means we still need this default value.
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16824:
URL: https://github.com/apache/kafka/pull/16824#discussion_r1803400912
##########
test-common/src/main/java/org/apache/kafka/common/test/TestKitNodes.java:
##########
@@ -39,10 +40,12 @@
import java.util.stream.IntStream;
import java.util.stream.Stream;
+@SuppressWarnings("NPathComplexity")
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";
Review Comment:
Thanks for the comments, already fixed in latest commit.
--
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
Re: [PR] KAFKA-17256 KRAFT should honor the listener name and security protocol from ClusterConfig [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16824:
URL: https://github.com/apache/kafka/pull/16824
--
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