dajac commented on a change in pull request #8808:
URL: https://github.com/apache/kafka/pull/8808#discussion_r446893874
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -130,22 +132,61 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
private def createServer(): Unit = {
servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
val listenerName =
ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
- adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+
+ val tmp = File.createTempFile(classOf[AclCommandTest].getName,
"createServer")
+ tmp.deleteOnExit()
+ val pw = new PrintWriter(tmp)
+ pw.println("client.id=my-client")
+ pw.close()
+
+ adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName),
+ "--command-config", tmp.getAbsolutePath)
+ }
+
+ private def callMain(args: Array[String]): (String, String) = {
+ val appender = LogCaptureAppender.createAndRegister()
+ val previousLevel =
LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], Level.WARN)
+ val outErr = TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
+ LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser],
previousLevel)
+ LogCaptureAppender.unregister(appender)
+ Assert.assertEquals("Command should execute without logging errors or
warnings",
+ "",
+ appender.getMessages.map{event => s"${event.getLevel}
${event.getMessage}" }.mkString("\n"))
+ outErr
}
private def testAclCli(cmdArgs: Array[String]): Unit = {
+
for ((resources, resourceCmd) <- ResourceToCommand) {
for (permissionType <- Set(ALLOW, DENY)) {
val operationToCmd = ResourceToOperations(resources)
val (acls, cmd) = getAclToCommand(permissionType, operationToCmd._1)
- AclCommand.main(cmdArgs ++ cmd ++ resourceCmd ++ operationToCmd._2
:+ "--add")
- for (resource <- resources) {
- withAuthorizer() { authorizer =>
- TestUtils.waitAndVerifyAcls(acls, authorizer, resource)
- }
+ val (addOut, addErr) = callMain(cmdArgs ++ cmd ++ resourceCmd ++
operationToCmd._2 :+ "--add")
+ assertOutputContains("Adding ACLs", resources, resourceCmd, addOut)
Review comment:
If not mistaken, `--add` also lists all the ACLs. Should we verify both
here?
Instead of adding everything here (e.g. list in the middle), it may be
easier to add a simpler test case which verifies the output when say 2 ACLs are
added. We could do the same for `--list` and `--remove`. I am not sure if it is
really better or not. It is just a suggestion.
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -130,22 +132,61 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
private def createServer(): Unit = {
servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
val listenerName =
ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
- adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+
+ val tmp = File.createTempFile(classOf[AclCommandTest].getName,
"createServer")
+ tmp.deleteOnExit()
+ val pw = new PrintWriter(tmp)
+ pw.println("client.id=my-client")
+ pw.close()
+
+ adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName),
+ "--command-config", tmp.getAbsolutePath)
+ }
+
+ private def callMain(args: Array[String]): (String, String) = {
+ val appender = LogCaptureAppender.createAndRegister()
+ val previousLevel =
LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], Level.WARN)
+ val outErr = TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
+ LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser],
previousLevel)
+ LogCaptureAppender.unregister(appender)
+ Assert.assertEquals("Command should execute without logging errors or
warnings",
+ "",
+ appender.getMessages.map{event => s"${event.getLevel}
${event.getMessage}" }.mkString("\n"))
+ outErr
}
private def testAclCli(cmdArgs: Array[String]): Unit = {
+
Review comment:
nit: empty line can be removed.
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -130,22 +132,61 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
private def createServer(): Unit = {
servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
val listenerName =
ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
- adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+
+ val tmp = File.createTempFile(classOf[AclCommandTest].getName,
"createServer")
+ tmp.deleteOnExit()
+ val pw = new PrintWriter(tmp)
+ pw.println("client.id=my-client")
+ pw.close()
+
+ adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName),
+ "--command-config", tmp.getAbsolutePath)
+ }
+
+ private def callMain(args: Array[String]): (String, String) = {
+ val appender = LogCaptureAppender.createAndRegister()
+ val previousLevel =
LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], Level.WARN)
+ val outErr = TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
+ LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser],
previousLevel)
+ LogCaptureAppender.unregister(appender)
+ Assert.assertEquals("Command should execute without logging errors or
warnings",
+ "",
+ appender.getMessages.map{event => s"${event.getLevel}
${event.getMessage}" }.mkString("\n"))
+ outErr
Review comment:
I would personally prefer to not add all the logic here but to define an
explicit test case which reproduce the issue that you have identified. I think
that it would be better to 1) ensure that we don't re-introduce it in the
future; and 2) ensure that someone without context understands it. It is not
clear why we suddenly defines `client.id` in `createServer`.
Moreover, instead of asserting that there is no errors or warnings, I would
assert that the exception that you have identified does not show up similarly
to what we do here:
https://github.com/apache/kafka/blob/7f90a58b69cd0eb63ba122b41e6ef6195b0a5d98/core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala#L631
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -248,7 +289,9 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
private def testRemove(cmdArgs: Array[String], resources:
Set[ResourcePattern], resourceCmd: Array[String]): Unit = {
for (resource <- resources) {
- AclCommand.main(cmdArgs ++ resourceCmd :+ "--remove" :+ "--force")
+ val (out, err) = callMain(cmdArgs ++ resourceCmd :+ "--remove" :+
"--force")
+ Assert.assertEquals("", out)
+ Assert.assertEquals("", err)
Review comment:
I wonder if it is wise to have the assertions here because `--remove`
lists the ACLs. At the moment, it seems that we always use this helper to
remove all ACLs so it seems to work OK. Is it always the case?
btw, it seems that we could potentially put the call to `--remove` outside
of the loop, no?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
