chia7712 commented on a change in pull request #9938:
URL: https://github.com/apache/kafka/pull/9938#discussion_r561156309
##########
File path:
core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
import scala.jdk.CollectionConverters._
+object DescribeAuthorizedOperationsTest {
+ val Group1 = "group1"
Review comment:
Is ```GROUP_1" more better? it is global variable in this test.
##########
File path:
core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -86,54 +122,36 @@ class DescribeAuthorizedOperationsTest extends
IntegrationTestHarness with SaslS
closeSasl()
}
- val group1Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP,
group1, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL,
AclPermissionType.ALLOW))
-
- val group2Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP,
group2, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DESCRIBE,
AclPermissionType.ALLOW))
-
- val group3Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP,
group3, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DELETE,
AclPermissionType.ALLOW))
-
- val clusterAllAcl = new AclBinding(new ResourcePattern(ResourceType.CLUSTER,
Resource.CLUSTER_NAME, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL,
AclPermissionType.ALLOW))
-
- val topic1Acl = new AclBinding(new ResourcePattern(ResourceType.TOPIC,
topic1, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL,
AclPermissionType.ALLOW))
-
- val topic2All = new AclBinding(new ResourcePattern(ResourceType.TOPIC,
topic2, PatternType.LITERAL),
- new AccessControlEntry("User:" +
JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DELETE,
AclPermissionType.ALLOW))
-
- def createConfig(): Properties = {
+ private def createConfig(): Properties = {
val adminClientConfig = new Properties()
adminClientConfig.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG,
brokerList)
adminClientConfig.put(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG, "20000")
val securityProps: util.Map[Object, Object] =
TestUtils.adminClientSecurityConfigs(securityProtocol, trustStoreFile,
clientSaslProperties)
- securityProps.forEach { (key, value) =>
adminClientConfig.put(key.asInstanceOf[String], value) }
+ securityProps.forEach((key, value) =>
adminClientConfig.put(key.asInstanceOf[String], value))
Review comment:
How about ```adminClientConfig.putAll(securityProps)```?
##########
File path:
core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
import scala.jdk.CollectionConverters._
+object DescribeAuthorizedOperationsTest {
+ val Group1 = "group1"
+ val Group2 = "group2"
+ val Group3 = "group3"
+ val Topic1 = "topic1"
+ val Topic2 = "topic2"
+
+ val Group1Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Group2Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DESCRIBE, ALLOW))
+
+ val Group3Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DELETE, ALLOW))
+
+ val ClusterAllAcl = new AclBinding(
+ new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME,
PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Topic1Acl = new AclBinding(
+ new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Topic2All = new AclBinding(
+ new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DELETE, ALLOW))
+
+ private def accessControlEntry(
+ userName: String,
+ operation: AclOperation,
+ permissionType: AclPermissionType
+ ): AccessControlEntry = {
+ new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE,
userName).toString,
+ AclEntry.WildcardHost, operation, permissionType)
+ }
+}
+
class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with
SaslSetup {
+ import DescribeAuthorizedOperationsTest._
+
override val brokerCount = 1
this.serverConfig.setProperty(KafkaConfig.ZkEnableSecureAclsProp, "true")
this.serverConfig.setProperty(KafkaConfig.AuthorizerClassNameProp,
classOf[AclAuthorizer].getName)
var client: Admin = _
Review comment:
all test cases need client so we should create it in construction or
```BeforeEach``` block.
##########
File path:
core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
import scala.jdk.CollectionConverters._
+object DescribeAuthorizedOperationsTest {
+ val Group1 = "group1"
+ val Group2 = "group2"
+ val Group3 = "group3"
+ val Topic1 = "topic1"
+ val Topic2 = "topic2"
+
+ val Group1Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Group2Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DESCRIBE, ALLOW))
+
+ val Group3Acl = new AclBinding(
+ new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DELETE, ALLOW))
+
+ val ClusterAllAcl = new AclBinding(
+ new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME,
PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Topic1Acl = new AclBinding(
+ new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
ALL, ALLOW))
+
+ val Topic2All = new AclBinding(
+ new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+ accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2,
DELETE, ALLOW))
+
+ private def accessControlEntry(
+ userName: String,
+ operation: AclOperation,
+ permissionType: AclPermissionType
Review comment:
It seems this filed is always ```ALLOW```
##########
File path:
core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends
IntegrationTestHarness with SaslS
TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
}
- private def accessControlEntry(userName: String, permissionType:
AclPermissionType, operation: AclOperation): AccessControlEntry = {
- new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE,
userName).toString,
- AclEntry.WildcardHost, operation, permissionType)
- }
-
@AfterEach
override def tearDown(): Unit = {
if (client != null)
Review comment:
unnecessary check
----------------------------------------------------------------
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:
[email protected]
