[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353499759
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,76 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+persistentTopics.createPartitionedTopic(testTenant, testNamespace,
partitionedTopicName, numPartitions);
+
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
partitionedTopicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace,
+partitionedTopicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+TopicName topicName = TopicName.get(TopicDomain.persistent.value(),
testTenant, testNamespace,
+partitionedTopicName);
+for (int i = 0; i < numPartitions; i++) {
+TopicName partition = topicName.getPartition(i);
+Map> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
+testNamespace, partition.getEncodedLocalName());
Review comment:
Thanks for reminding. PersistentTopicsTest passed locally.
I use getEncodedLocalName because the parameter for the rest API
getPermissionsOnTopic is named encodedTopic as followed.
```java
public Map> getPermissionsOnTopic(
@ApiParam(value = "Specify the tenant", required = true)
@PathParam("tenant") String tenant,
@ApiParam(value = "Specify the namespace", required = true)
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic) {
validateTopicName(tenant, namespace, encodedTopic);
return internalGetPermissionsOnTopic();
}
```
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353499759
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,76 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+persistentTopics.createPartitionedTopic(testTenant, testNamespace,
partitionedTopicName, numPartitions);
+
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
partitionedTopicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace,
+partitionedTopicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+TopicName topicName = TopicName.get(TopicDomain.persistent.value(),
testTenant, testNamespace,
+partitionedTopicName);
+for (int i = 0; i < numPartitions; i++) {
+TopicName partition = topicName.getPartition(i);
+Map> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
+testNamespace, partition.getEncodedLocalName());
Review comment:
Thanks for reminding. PersistentTopicsTest passed locally.
I use getEncodedLocalName because the parameter for rest API is named
encodedTopic as follwed.
```java
public Map> getPermissionsOnTopic(
@ApiParam(value = "Specify the tenant", required = true)
@PathParam("tenant") String tenant,
@ApiParam(value = "Specify the namespace", required = true)
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic) {
validateTopicName(tenant, namespace, encodedTopic);
return internalGetPermissionsOnTopic();
}
```
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353498429
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##
@@ -301,13 +295,29 @@ protected void internalGrantPermissionsOnTopic(String
role, Set acti
log.warn("[{}] Failed to grant permissions on topic {}: concurrent
modification", clientAppId(),
topicUri);
throw new RestException(Status.CONFLICT, "Concurrent
modification");
-}
-catch (Exception e) {
+} catch (Exception e) {
log.error("[{}] Failed to grant permissions for topic {}",
clientAppId(), topicUri, e);
throw new RestException(e);
}
}
+protected void internalGrantPermissionsOnTopic(String role,
Set actions) {
+// This operation should be reading from zookeeper and it should be
allowed without having admin privileges
+validateAdminAccessForTenant(namespaceName.getTenant());
+validatePoliciesReadOnlyAccess();
+
+PartitionedTopicMetadata meta = getPartitionedTopicMetadata(topicName,
true, false);
Review comment:
Thanks for reminding. I have fixed this failed case.
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352942140
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
> > you can provide this support in the internalGetPermissionsOnTopic
function.
>
> Is it better to create a new api called getPermissionsOnPartitionedTopic?
> I understand if I support searching partitioned topic in
internalGetPermissionsOnTopic. The output structure of getPermissionsOnTopic
will be changed to
> Map which will break API compatibility?
@tuteng Can you give me some advice about this?
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352938788
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##
@@ -301,13 +295,29 @@ protected void internalGrantPermissionsOnTopic(String
role, Set acti
log.warn("[{}] Failed to grant permissions on topic {}: concurrent
modification", clientAppId(),
topicUri);
throw new RestException(Status.CONFLICT, "Concurrent
modification");
-}
-catch (Exception e) {
+} catch (Exception e) {
log.error("[{}] Failed to grant permissions for topic {}",
clientAppId(), topicUri, e);
throw new RestException(e);
}
}
+protected void internalGrantPermissionsOnTopic(String role,
Set actions) {
+// This operation should be reading from zookeeper and it should be
allowed without having admin privileges
+validateAdminAccessForTenant(namespaceName.getTenant());
+validatePoliciesReadOnlyAccess();
+
+PartitionedTopicMetadata meta = getPartitionedTopicMetadata(topicName,
true, false);
Review comment:
@tuteng As far as I understand, rest API v1 already supports partitioned
topic. Can you help me check if my understanding is right.
org.apache.pulsar.broker.admin.v1.PersistentTopics has implement API about
partitioned topic, like createPartitionedTopic and getPartitionedMetadata as
followed
```java
@PUT
@Path("/{property}/{cluster}/{namespace}/{topic}/partitions")
@ApiOperation(hidden = true, value = "Create a partitioned topic.",
notes = "It needs to be called before creating a producer on a partitioned
topic.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have
admin permission"),
@ApiResponse(code = 409, message = "Partitioned topic already
exist") })
public void createPartitionedTopic(@PathParam("property") String
property, @PathParam("cluster") String cluster,
@PathParam("namespace") String namespace, @PathParam("topic")
@Encoded String encodedTopic,
int numPartitions) {
validateTopicName(property, cluster, namespace, encodedTopic);
internalCreatePartitionedTopic(numPartitions);
}
...
@GET
@Path("/{property}/{cluster}/{namespace}/{topic}/partitions")
@ApiOperation(hidden = true, value = "Get partitioned topic metadata.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have
admin permission") })
public PartitionedTopicMetadata
getPartitionedMetadata(@PathParam("property") String property,
@PathParam("cluster") String cluster, @PathParam("namespace")
String namespace,
@PathParam("topic") @Encoded String encodedTopic,
@QueryParam("authoritative") @DefaultValue("false") boolean
authoritative,
@QueryParam("checkAllowAutoCreation") @DefaultValue("false")
boolean checkAllowAutoCreation) {
validateTopicName(property, cluster, namespace, encodedTopic);
return internalGetPartitionedMetadata(authoritative,
checkAllowAutoCreation);
}
```
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352337822
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
> you can provide this support in the internalGetPermissionsOnTopic function.
Is it better to create a new api called getPermissionsOnPartitionedTopic?
I understand if I support searching partitioned topic in
internalGetPermissionsOnTopic. The output structure of getPermissionsOnTopic
will be changed to
Map> which will break API compatibility?
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352337822
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
you can provide this support in the internalGetPermissionsOnTopic function.
===
Is it better to create a new api called getPermissionsOnPartitionedTopic?
I understand if I support searching partitioned topic in
internalGetPermissionsOnTopic. The output structure of getPermissionsOnTopic
will be changed to
Map> which will break API compatibility?
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352337039
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,74 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+persistentTopics.createPartitionedTopic(testTenant, testNamespace,
partitionedTopicName, numPartitions);
+
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
partitionedTopicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace,
+partitionedTopicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+TopicName topicName = TopicName.get(partitionedTopicName);
+for (int i = 0; i < numPartitions; i++) {
+TopicName partition = topicName.getPartition(i);
+Map> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
Review comment:
Thanks for reminding. I have fixed failed cases
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352336995
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
Thanks for the explanation, I understand.
I will open a new pr to support searching partitioned in
**getPermissionsOnTopic**
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352316028
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
Hi, I went through the grant and get logic for permissions and It seems
current logic support get permissions for partitions of a topic. Pls help me
check if my understanding is right.
The permissions for partitions will be stored in a map named
destination_auth, the same as the parent topic of partitions.
```java
private void grantPermissions(String topicUri, String role,
Set actions) {
try {
...
Policies policies = jsonMapper().readValue(content,
Policies.class);
if
(!policies.auth_policies.destination_auth.containsKey(topicUri)) {
policies.auth_policies.destination_auth.put(topicUri, new
TreeMap>());
}
policies.auth_policies.destination_auth.get(topicUri).put(role,
actions);
// Write the new policies to zookeeper
globalZk().setData(path(POLICIES, namespaceName.toString()),
jsonMapper().writeValueAsBytes(policies),
nodeStat.getVersion());
...
}
```
Then get permissions logic for a partition: try to get permissions from
auth_policies.destination_auth directly by topicUri.
```java
protected Map> internalGetPermissionsOnTopic() {
// This operation should be reading from zookeeper and it should be
allowed without having admin privileges
validateAdminAccessForTenant(namespaceName.getTenant());
String topicUri = topicName.toString();
try {
...
// Then add topic level permissions
if (auth.destination_auth.containsKey(topicUri)) {
for (Map.Entry> entry :
auth.destination_auth.get(topicUri).entrySet()) {
String role = entry.getKey();
Set topicPermissions = entry.getValue();
if (!permissions.containsKey(role)) {
permissions.put(role, topicPermissions);
} else {
// Do the union between namespace and topic level
Set union =
Sets.union(permissions.get(role), topicPermissions);
permissions.put(role, union);
}
}
}
return permissions;
} catch (Exception e) {
...
}
}
```
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
With regards,
Apache Git Services
[GitHub] [pulsar] zhaohaidao commented on a change in pull request #5767: Support batch authorization of partitioned topic
zhaohaidao commented on a change in pull request #5767: Support batch
authorization of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352316028
##
File path:
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
##
@@ -312,4 +316,82 @@ public void testGetPartitionedTopicsList() throws
KeeperException, InterruptedEx
Assert.assertEquals(nonPersistentPartitionedTopics.size(), 1);
Assert.assertEquals(TopicName.get(nonPersistentPartitionedTopics.get(0)).getDomain().value(),
TopicDomain.non_persistent.value());
}
+
+@Test
+public void testGrantNonPartitionedTopic() {
+final String topicName = "non-partitioned-topic";
+persistentTopics.createNonPartitionedTopic(testTenant, testNamespace,
topicName, true);
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
topicName, role, expectActions);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), expectActions);
+}
+
+@Test
+public void testGrantPartitionedTopic() {
+final String partitionedTopicName = "partitioned-topic";
+final int numPartitions = 5;
+LocalZooKeeperCacheService mockLocalZooKeeperCacheService =
mock(LocalZooKeeperCacheService.class);
+ZooKeeperChildrenCache mockZooKeeperChildrenCache =
mock(ZooKeeperChildrenCache.class);
+
doReturn(mockLocalZooKeeperCacheService).when(pulsar).getLocalZkCacheService();
+
doReturn(mockZooKeeperChildrenCache).when(mockLocalZooKeeperCacheService).managedLedgerListCache();
Review comment:
Hi, I went through the grant and get logic for permissions and It seems
current logic support get permissions for partitions of a topic. Pls help me
check if my understanding is right.
The permissions will be stored in a map named destination_auth as followed.
```java
private void grantPermissions(String topicUri, String role,
Set actions) {
try {
...
Policies policies = jsonMapper().readValue(content,
Policies.class);
if
(!policies.auth_policies.destination_auth.containsKey(topicUri)) {
policies.auth_policies.destination_auth.put(topicUri, new
TreeMap>());
}
policies.auth_policies.destination_auth.get(topicUri).put(role,
actions);
// Write the new policies to zookeeper
globalZk().setData(path(POLICIES, namespaceName.toString()),
jsonMapper().writeValueAsBytes(policies),
nodeStat.getVersion());
...
}
```
Then get permissions logic for a partition: try to get permissions from
auth_policies.destination_auth directly by topic_name.
```java
protected Map> internalGetPermissionsOnTopic() {
// This operation should be reading from zookeeper and it should be
allowed without having admin privileges
validateAdminAccessForTenant(namespaceName.getTenant());
String topicUri = topicName.toString();
try {
...
// Then add topic level permissions
if (auth.destination_auth.containsKey(topicUri)) {
for (Map.Entry> entry :
auth.destination_auth.get(topicUri).entrySet()) {
String role = entry.getKey();
Set topicPermissions = entry.getValue();
if (!permissions.containsKey(role)) {
permissions.put(role, topicPermissions);
} else {
// Do the union between namespace and topic level
Set union =
Sets.union(permissions.get(role), topicPermissions);
permissions.put(role, union);
}
}
}
return permissions;
} catch (Exception e) {
...
}
}
```
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
With regards,
Apache Git Services
