Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1649455825
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -324,12 +348,18 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
}
private def withAuthorizer()(f: Authorizer => Unit): Unit = {
-val kafkaConfig = KafkaConfig.fromProps(brokerProps, doLog = false)
-val authZ = new AclAuthorizer
-try {
- authZ.configure(kafkaConfig.originals)
- f(authZ)
-} finally authZ.close()
+if (isKRaftTest()) {
+ (servers.map(_.authorizer.get) ++
controllerServers.map(_.authorizer.get)).foreach { auth =>
Review Comment:
> That will create zk authorizer even though the tests don't have zk, right?
Yeah, you are right...
Restored the original implementation :ok_hand:
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2171435063 Test seems to be more stable now after recent update. I'm also running it in a loop in IntelliJ, and so far can't reproduce flakyness :crossed_fingers: -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1641784154
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -325,11 +349,15 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
private def withAuthorizer()(f: Authorizer => Unit): Unit = {
val kafkaConfig = KafkaConfig.fromProps(brokerProps, doLog = false)
-val authZ = new AclAuthorizer
+val auth = if (isKRaftTest()) {
+ servers.last.authorizer.get
+} else {
+ new AclAuthorizer
Review Comment:
As I understand, this AclAuthorizer is used for some tests like
`testAclCliWithAuthorizer`, `testProducerConsumerCliWithAuthorizer`,
`testAclsOnPrefixedResourcesWithAuthorizer`, `testPatternTypesWithAuthorizer`.
Those tests don't use AdminClient, so for them there seems to be no broker
created at all, only Zookeeper instance. So there is no broker/controller from
which we can obtain the AclAutorizer, hence creating a new instance of the
autorizer 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1632119543
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -325,11 +349,15 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
private def withAuthorizer()(f: Authorizer => Unit): Unit = {
val kafkaConfig = KafkaConfig.fromProps(brokerProps, doLog = false)
-val authZ = new AclAuthorizer
+val auth = if (isKRaftTest()) {
+ servers.last.authorizer.get
+} else {
+ new AclAuthorizer
Review Comment:
Maybe we can use the authorizer of broker instead of creating new one?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2156108598 > Could you please take a look at failed tests? `AclCommandTest` seems to be fixed, but there are some other failing tests for jdk8 and jdk21 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2143770889 @pasharik Could you please take a look at failed 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1623251171
##
core/src/test/java/kafka/admin/AclCommandIntegrationTest.java:
##
@@ -0,0 +1,453 @@
+package kafka.admin;
+
+import kafka.test.ClusterInstance;
+import kafka.test.annotation.*;
+import kafka.test.junit.ClusterTestExtensions;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.Resource;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.AppInfoParser;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.LogCaptureAppender;
+import org.apache.kafka.common.utils.SecurityUtils;
+import org.apache.kafka.security.authorizer.AclEntry;
+import org.apache.kafka.server.config.ServerConfigs;
+import org.apache.kafka.test.TestUtils;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.extension.ExtendWith;
+import scala.Console$;
+import scala.collection.JavaConverters;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.*;
+import java.util.Map.Entry;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.*;
+import static java.util.stream.Collectors.*;
+import static java.util.stream.Stream.concat;
+import static org.apache.kafka.common.acl.AclOperation.*;
+import static org.apache.kafka.common.acl.AclPermissionType.ALLOW;
+import static org.apache.kafka.common.acl.AclPermissionType.DENY;
+import static org.apache.kafka.common.resource.PatternType.LITERAL;
+import static org.apache.kafka.common.resource.PatternType.PREFIXED;
+import static org.apache.kafka.common.resource.ResourceType.*;
+import static
org.apache.kafka.metadata.authorizer.StandardAuthorizer.SUPER_USERS_CONFIG;
+import static org.apache.kafka.test.TestUtils.tempFile;
+import static org.junit.jupiter.api.Assertions.*;
+import static scala.collection.JavaConverters.setAsJavaSet;
+
+@ExtendWith(value = ClusterTestExtensions.class)
+@ClusterTestDefaults
+@Tag("integration")
+public class AclCommandIntegrationTest {
+private final ClusterInstance cluster;
+
+private static final String AUTHORIZER =
ServerConfigs.AUTHORIZER_CLASS_NAME_CONFIG;
+private static final String ACL_AUTHORIZER =
"kafka.security.authorizer.AclAuthorizer";
+private static final String STANDARD_AUTHORIZER =
"org.apache.kafka.metadata.authorizer.StandardAuthorizer";
+private static final String SUPER_USERS = "super.users";
+private static final String USER_ANONYMOUS = "User:ANONYMOUS";
+
+private static final KafkaPrincipal PRINCIPAL =
SecurityUtils.parseKafkaPrincipal("User:test2");
Review Comment:
Okay, restored it back:
- Moved KRaft tests back into `AclCommandTest.scala`
- Deleted `AclCommandIntegrationTest.java`
- Removed `"Current ACLs"` print output from the AclCommand itself
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1622737514
##
core/src/test/java/kafka/admin/AclCommandIntegrationTest.java:
##
@@ -0,0 +1,453 @@
+package kafka.admin;
+
+import kafka.test.ClusterInstance;
+import kafka.test.annotation.*;
+import kafka.test.junit.ClusterTestExtensions;
+import org.apache.kafka.clients.admin.Admin;
+import org.apache.kafka.common.acl.*;
+import org.apache.kafka.common.resource.PatternType;
+import org.apache.kafka.common.resource.Resource;
+import org.apache.kafka.common.resource.ResourcePattern;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.utils.AppInfoParser;
+import org.apache.kafka.common.utils.Exit;
+import org.apache.kafka.common.utils.LogCaptureAppender;
+import org.apache.kafka.common.utils.SecurityUtils;
+import org.apache.kafka.security.authorizer.AclEntry;
+import org.apache.kafka.server.config.ServerConfigs;
+import org.apache.kafka.test.TestUtils;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.extension.ExtendWith;
+import scala.Console$;
+import scala.collection.JavaConverters;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.*;
+import java.util.Map.Entry;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static java.util.Arrays.asList;
+import static java.util.Collections.*;
+import static java.util.stream.Collectors.*;
+import static java.util.stream.Stream.concat;
+import static org.apache.kafka.common.acl.AclOperation.*;
+import static org.apache.kafka.common.acl.AclPermissionType.ALLOW;
+import static org.apache.kafka.common.acl.AclPermissionType.DENY;
+import static org.apache.kafka.common.resource.PatternType.LITERAL;
+import static org.apache.kafka.common.resource.PatternType.PREFIXED;
+import static org.apache.kafka.common.resource.ResourceType.*;
+import static
org.apache.kafka.metadata.authorizer.StandardAuthorizer.SUPER_USERS_CONFIG;
+import static org.apache.kafka.test.TestUtils.tempFile;
+import static org.junit.jupiter.api.Assertions.*;
+import static scala.collection.JavaConverters.setAsJavaSet;
+
+@ExtendWith(value = ClusterTestExtensions.class)
+@ClusterTestDefaults
+@Tag("integration")
+public class AclCommandIntegrationTest {
+private final ClusterInstance cluster;
+
+private static final String AUTHORIZER =
ServerConfigs.AUTHORIZER_CLASS_NAME_CONFIG;
+private static final String ACL_AUTHORIZER =
"kafka.security.authorizer.AclAuthorizer";
+private static final String STANDARD_AUTHORIZER =
"org.apache.kafka.metadata.authorizer.StandardAuthorizer";
+private static final String SUPER_USERS = "super.users";
+private static final String USER_ANONYMOUS = "User:ANONYMOUS";
+
+private static final KafkaPrincipal PRINCIPAL =
SecurityUtils.parseKafkaPrincipal("User:test2");
Review Comment:
ok, that will have many duplicate code ...
Sorry that could we merge this test into `AclCommandTest`? We don't need to
use new test infra in this PR, and we can do a follow-up for that. Also, using
scala is ok to me for now
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1622734164
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
> Do you think it's worth migrating those tests to java at this stage, if
they are going to be deleted anyway?
we can keep origin test in this PR. the new test file you added is good to me
> we can probably remove console output Current ACLs
agree
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1622734164
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
> Do you think it's worth migrating those tests to java at this stage, if
they are going to be deleted anyway?
we can keep origin test :)
> we can probably remove console output Current ACLs
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
> Do you think it's worth migrating those tests to java at this stage, if
they are going to be deleted anyway?
we can keep origin test :)
> we can probably remove console output Current ACLs
agree
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1621488481
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
- I've moved KRaft tests into a new `AclCommandIntegrationTest.java`
- Left old Zookeeper tests in `AclCommandTest.scala`. As I understand, we
are going to completely delete this test file, once fully moved to KRaft, am I
right? Do you think it's worth migrating those tests to java at this stage, if
they are going to be deleted anyway?
- Race condition described above, is still reproduced on a new
infrastructure :cry: So if there are no objections, we can probably remove
console output `Current ACLs`
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2138307998 @pasharik could you please fix the conflicts? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1617053123
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
> Just wondering, do we need to check this main output at all?
from the testing we can know that output "Current ACLs" could be out-of-date
information. +1 to your previous changes that removing `listAcls` from
`removeAcls` and `addAcls` in order to avoid confusing users.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1612152214
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
Commited [alternative
fix](https://github.com/apache/kafka/pull/15830/commits/d39be8a7c349e868b7c5066e6be89f14a9e6adf3)
for failing tests
For now, I've added a condition, so we don't verify the output from the
`main` for the Kraft tests. For adding ACLs:
```
if (!isKRaftTest()) {
assertOutputContains(...)
```
and for deleting ACLs:
```
if (!isKRaftTest()) {
assertEquals("", out)
```
Just wondering, do we need to check this `main` output at all?
> Maybe we can create `AclCommand.AdminClientService` to test its method
`listAcls` with retry instead of grabbing the output from `main` ?
Yeah, there is already similar logic in the test:
```
for (resource <- resources) {
withAuthorizer() { authorizer =>
TestUtils.waitAndVerifyAcls(...)
```
so eventualy ACLs are propageted in Kraft mode, and tests pass.
The only problem is with capturing the immediate output from the `main`,
which may not be correct sometimes, because of the race condigion
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1610529172
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
Maybe we can create `AclCommand.AdminClientService` to test its method
`listAcls` with retry instead of grabbing the output from `main` ?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1610368294
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
Some tests are failing in Kraft mode because of a race condition, which we
[discussed
above](https://github.com/apache/kafka/pull/15830#discussion_r1585713585)
Currently I'm trying to rewrite the whole test in java and migrate to a new
test infrastructure. Hopefully, it will solve the race condition issue. If it
doesn't help, then probably we'll have to remove some failing checks from the
test or alternatively change the output of the AclCommand tool itself.
I've removed kraft support from those test for now, just to make a jenkins
build pass. Planning to remove this scala test class complately, and replace if
with a new java test class
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1610368294
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
Some tests are failing in Kraft mode because of a race condition, which we
[discussed
above](https://github.com/apache/kafka/pull/15830#discussion_r1585713585)
Currently I'm trying to rewrite the whole test in java and migrate to new
test infrastructure. Hopefully, it will solve the race condition issue. If it
doesn't help, then probably we'll have to remove some failing checks from the
test or alternatively change the output of the AclCommand tool itself.
I've removed kraft support from those test for now, just to make a jenkins
build pass. Planning to remove this scala test complately, and replace if with
a new java test
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1609798224
##
core/src/test/scala/unit/kafka/admin/AclCommandTest.scala:
##
@@ -122,19 +128,27 @@ class AclCommandTest extends QuorumTestHarness with
Logging {
super.tearDown()
}
+ override protected def kraftControllerConfigs(): Seq[Properties] = {
+val controllerConfig = new Properties
+controllerConfig.put(KafkaConfig.AuthorizerClassNameProp,
classOf[StandardAuthorizer].getName)
+controllerConfig.put(StandardAuthorizer.SUPER_USERS_CONFIG,
"User:ANONYMOUS")
+Seq(controllerConfig)
+ }
+
@Test
def testAclCliWithAuthorizer(): Unit = {
testAclCli(zkArgs)
}
- @Test
- def testAclCliWithAdminAPI(): Unit = {
+ @ParameterizedTest
+ @ValueSource(strings = Array("zk"))
Review Comment:
Why this test case can't run with kraft?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2119256124 @pasharik Could you please check the failed test? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1586037686
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
> As I understand, listAcls() can be served by any broker in kraft mode, not
just by controller. So potentially some brokers may not have most up-to-date
copy of metadata right after createAcls() call. Here there is no interval
between createAcls() and listAcls(), so probability of such race condition is
higher.
From [KIP-500](https://cwiki.apache.org/confluence/display/KAFKA/KIP-500)
and the
[diagram](https://cwiki.apache.org/confluence/download/attachments/123898922/b.png)
it seems what brokers have to periodically pull new metadata from the kraft
controller, so metadata is not available on all brokers immediately.
Please correct me if I'm wrong here.
agree to race condition.
> Probably, it can be related to the test infrastructure setup. I'll try to
re-write the test to java and use new test infrastructure, let's see it solves
the issue
agree that we should fix the test infra instead of changing the output of
command tool. I worried about that some users may rely on the output of
`listAcls`, so we should not change the output arbitrarily.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1585713585
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
Restored this print for now.
For me, this print together with checking for the output of this print
inside the test, causes kraft tests to be flaky. E.g.
```
./gradlew core:test --tests
"kafka.admin.AclCommandTest.testAclCliWithAdminAPI" --rerun
```
it always passes for `zk`, but quite often fails for `kraft`.
As I understand, `listAcls()` can be served by any broker in kraft mode, not
just by controller. So potentially some brokers may not have most up-to-date
copy of metadata right after `createAcls()` call. Here there is no interval
between `createAcls()` and `listAcls()`, so probability of such race condition
is higher.
From [KIP-500](https://cwiki.apache.org/confluence/display/KAFKA/KIP-500)
and the
[diagram](https://cwiki.apache.org/confluence/download/attachments/123898922/b.png)
it seems what brokers have to periodically pull new metadata from the kraft
controller, so metadata is not available on all brokers immediately.
Please correct me if I'm wrong here.
I've looked at `TopicCommand`, as another example of the class which manages
metadata. It seems what it doesn't invoke `listTopics()` after creating or
deleting topics. So I thought we can use similar approach here.
I tried to reproduce this flaky behavior with running Kafka controller and
broker locally with `bin/kafka-server-start.sh`, and invoking
`bin/kafka-acls.sh` manually, but wasn't able to reproduce same issue.
Probably, it can be related to the test infrastructure setup. I'll try to
re-write the test to java and use new test infrastructure, let's see it solves
the issue
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1585704666
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
Restored this print for now.
For me, this print together with checking for the output of this print
inside the test, causes kraft tests to be flaky. E.g.
```
./gradlew core:test --tests
"kafka.admin.AclCommandTest.testAclCliWithAdminAPI" --rerun
```
it always passes for `zk`, but quite often fails for `kraft`.
As I understand, `listAcls()` can be served by any broker in kraft mode, not
just by controller. So potentially some brokers may not have most up-to-date
copy of metadata right after `createAcls()` call. Here there is no interval
between `createAcls()` and `listAcls()`, so probability of such race condition
is higher.
From [KIP-500](https://cwiki.apache.org/confluence/display/KAFKA/KIP-500)
and the
[diagram](https://cwiki.apache.org/confluence/download/attachments/123898922/b.png)
it seems what brokers have to periodically pull new metadata from the kraft
controller, so metadata is not available on all brokers immediately.
Please correct me if I'm wrong here.
I've looked at `TopicCommand`, as another example of the class which manages
metadata. It seems what it doesn't invoke `listTopics()` after creating or
deleting topics. So I thought we can use similar approach here.
I tried to reproduce this flaky behavior with running Kafka controller and
broker locally with `bin/kafka-server-start.sh`, and invoking
`bin/kafka-acls.sh` manually, but wasn't able to reproduce same issue.
Probably, it can be related to the test infrastructure setup. I'll try to
re-write the test to java and use new test infrastructure, let's see it solves
the issue
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on PR #15830: URL: https://github.com/apache/kafka/pull/15830#issuecomment-2087745006 > Hi @pasharik. Thanks for the change. > > > In the original implementation, listAcls() method was called directly from addAcls() and removeAcls() methods, which caused a race condition in KRaft mode, so the test become flaky > > Can you share a bit more detail on this? How specifically does `listAcls()` contribute to flakiness? Thanks Replied in the [above thread](https://github.com/apache/kafka/pull/15830#discussion_r1584190323) -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1585704666
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
Restored this print for now.
For me, this print together with checking for the output of this print
inside the test, causes kraft tests to be flaky. E.g.
```
./gradlew core:test --tests
"kafka.admin.AclCommandTest.testAclCliWithAdminAPI" --rerun
```
it always passes for `zk`, but quite often fails for `kraft`.
As I understand, `listAcls()` can be served by any broker in kraft mode, not
just by controller. So potentially some brokers may not have most up-to-date
copy of metadata right after `createAcls()` call. Here there is no interval
between `createAcls()` and `listAcls()`, so probability of such race condition
is higher.
From [KIP-500](https://cwiki.apache.org/confluence/display/KAFKA/KIP-500)
and the
[diagram](https://cwiki.apache.org/confluence/download/attachments/123898922/b.png)
it seems what brokers have to periodically pull new metadata from the kraft
controller, so metadata is not available on all brokers immediately.
Please correct me if I'm wrong here.
I've looked at `TopicCommand`, as another example of the class which manages
metadata. It seems what it doesn't invoke `listTopics()` after creating or
deleting topics. So I thought we can use similar approach here.
I tried to reproduce this flaky behavior with running Kafka controller and
broker locally with `bin/kafka-server-start.sh`, and invoking
`bin/kafka-acls.sh` manually, but wasn't able to reproduce same issue.
Probably, it can be related to the test infrastructure setup. I'll try to
re-write the test to java and use new test infrastructure, let's see it solves
the issue
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
chia7712 commented on code in PR #15830:
URL: https://github.com/apache/kafka/pull/15830#discussion_r1584190323
##
core/src/main/scala/kafka/admin/AclCommand.scala:
##
@@ -115,8 +115,6 @@ object AclCommand extends Logging {
val aclBindings = acls.map(acl => new AclBinding(resource,
acl)).asJavaCollection
adminClient.createAcls(aclBindings).all().get()
}
-
-listAcls(adminClient)
Review Comment:
why removing this print?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] KAFKA-15713: KRaft support in AclCommandTest [kafka]
pasharik opened a new pull request, #15830: URL: https://github.com/apache/kafka/pull/15830 - Updating `AclCommandTest` to support KRaft - Tests which are using `AclAuthoriser` are not updated, as they are expected to be removed after full migration to KRaft - Changed `AclCommand` console output for adding and removing ACLs. In the original implementation, `listAcls()` method was called directly from `addAcls()` and `removeAcls()` methods, which caused a race condition in KRaft mode, so the test become flaky - This is quite a big change for our UI, as I understand. Should it go throuhg KIP process? ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
