dajac commented on a change in pull request #8808:
URL: https://github.com/apache/kafka/pull/8808#discussion_r449554332
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -127,25 +131,53 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
testAclCli(adminArgs)
}
- private def createServer(): Unit = {
+ private def createServer(commandConfig: File = null): Unit = {
Review comment:
nit: It would be better to use an `Option` and default to `None` here.
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -127,25 +131,53 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
testAclCli(adminArgs)
}
- private def createServer(): Unit = {
+ private def createServer(commandConfig: File = null): Unit = {
servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
val listenerName =
ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
- adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+
+ var adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+ if (commandConfig != null) {
+ adminArgs ++= Array("--command-config", commandConfig.getAbsolutePath)
+ }
+ this.adminArgs = adminArgs
+ }
+
+ private def callMain(args: Array[String]): (String, String) = {
+ TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
}
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)
+ assertOutputContains("Current ACLs", resources, resourceCmd, addOut)
+
+ Assert.assertEquals("", addErr)
Review comment:
nit: Could you move the empty line above this line to bellow this line?
##########
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:
@tombentley Yes, I meant something along these lines. I am not a huge
fan of having one test that verifies everything so I wish that we could move
towards having smaller and simpler test cases in the future. I do agree that
this is outside of the scope of this PR. The other test case
`testAclCliWithClientId` that you have added is good to cover the fix.
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -161,10 +193,34 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
testProducerConsumerCli(adminArgs)
}
+ @Test
+ def testAclCliWithClientId(): Unit = {
Review comment:
👍
##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -127,25 +131,53 @@ class AclCommandTest extends ZooKeeperTestHarness with
Logging {
testAclCli(adminArgs)
}
- private def createServer(): Unit = {
+ private def createServer(commandConfig: File = null): Unit = {
servers = Seq(TestUtils.createServer(KafkaConfig.fromProps(brokerProps)))
val listenerName =
ListenerName.forSecurityProtocol(SecurityProtocol.PLAINTEXT)
- adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+
+ var adminArgs = Array("--bootstrap-server",
TestUtils.bootstrapServers(servers, listenerName))
+ if (commandConfig != null) {
+ adminArgs ++= Array("--command-config", commandConfig.getAbsolutePath)
+ }
+ this.adminArgs = adminArgs
+ }
+
+ private def callMain(args: Array[String]): (String, String) = {
+ TestUtils.grabConsoleOutputAndError(AclCommand.main(args))
}
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)
+ assertOutputContains("Current ACLs", resources, resourceCmd, addOut)
+
+ Assert.assertEquals("", addErr)
+ for (resource <- resources) {
+ withAuthorizer() { authorizer =>
+ TestUtils.waitAndVerifyAcls(acls, authorizer, resource)
}
+ }
- testRemove(cmdArgs, resources, resourceCmd)
+ val (listOut, listErr) = callMain(cmdArgs :+ "--list")
+ assertOutputContains("Current ACLs", resources, resourceCmd, listOut)
+ Assert.assertEquals("", listErr)
+
+ testRemove(cmdArgs, resources, resourceCmd)
+ }
+ }
+ }
+
+ private def assertOutputContains(prefix: String, resources:
Set[ResourcePattern], resourceCmd: Array[String], output: String) = {
+ resources.foreach { resource =>
+ val resourceType = resource.resourceType.toString
+ (if (resource == ClusterResource) Array("kafka-cluster") else
resourceCmd.filter(!_.startsWith("--"))).foreach { name =>
+ val expected = s"$prefix for resource
`ResourcePattern(resourceType=$resourceType, name=$name, patternType=LITERAL)`:"
+ Assert.assertTrue(s"Substring ${expected} not in --list
output:\n$output",
Review comment:
nit: `--list` can be removed as we use it for other commands as well.
----------------------------------------------------------------
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
