[GitHub] [pulsar] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353168621
##
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:
I feel this call function `getEncodedLocalName` seems to be incorrect, the
topic name here should have a suffix `-partition-0`. You can try the test in
locally use the command:
```
mvn test
-Dtest=org.apache.pulsar.broker.admin.PersistentTopicsTest#testGrantPartitionedTopic
-pl pulsar-broker
```
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353166132
##
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:
I am not sure whether it is more appropriate to open a new API interface,
because the current authorization and cancellation of permissions for both
partitioned topic and non-partitioned topic are done under the same rest API.
if opening a new rest API to get permissions causes confusion to users, what do
you think? @sijie @jiazhai
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r353164610
##
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:
You are right. But one unit test never passed:
You can test it locally:
```
mvn test -Dtest=org.apache.pulsar.broker.admin.AdminTest#persistentTopics
-pl pulsar-broker
```
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352513441
##
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:
@zhaohaidao We need support rest API of v1 version for the partitioned topic.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352318147
##
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:
`partition` here is a full-path topic, so the splice is wrong.
The following wrong path will be spliced here:
```
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-0
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-1
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-2
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-3
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-4
```
The correct path should look like this:
```
persistent://my-tenant/my-namespace/partitioned-topic-partition-0
persistent://my-tenant/my-namespace/partitioned-topic-partition-1
persistent://my-tenant/my-namespace/partitioned-topic-partition-2
persistent://my-tenant/my-namespace/partitioned-topic-partition-3
persistent://my-tenant/my-namespace/partitioned-topic-partition-4
```
```
public TopicName getPartition(int index) {
if (index == -1 ||
this.toString().contains(PARTITIONED_TOPIC_SUFFIX)) {
return this;
}
String partitionName = this.toString() + PARTITIONED_TOPIC_SUFFIX +
index;
return get(partitionName);
}
@Override
public String toString() {
return completeTopicName;
}
if (isV2()) {
this.completeTopicName = String.format("%s://%s/%s/%s",
domain, tenant,
namespacePortion, localName);
```
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352318147
##
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:
`partition` here is a full-path topic, so the splice is wrong.
The following wrong path will be spliced here:
```
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-0
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-1
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-2
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-3
persistent://my-tenant/my-namespace/persistent://my-tenant/my-namespace/partitioned-topic-partition-4
```
The correct path should look like this:
```
persistent://my-tenant/my-namespace/partitioned-topic-partition-0
persistent://my-tenant/my-namespace/partitioned-topic-partition-1
persistent://my-tenant/my-namespace/partitioned-topic-partition-2
persistent://my-tenant/my-namespace/partitioned-topic-partition-3
persistent://my-tenant/my-namespace/partitioned-topic-partition-4
```
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352317912
##
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:
First of all, this logic has no problem with non-partitioned topics.
Let's look at the creation of the partitioned topic first. When we use the
following command to create a partitioned topic,
```
./bin/pulsar-admin topics create-partitioned-topic test-topic -p 2
```
we will eventually generate the following topic in the zookeeper.
```
persistent://public/default/test-topic-partition-0
persistent://public/default/test-topic-partition-1
```
When we use the `./bin/pulsar-admin topics grant-permissions` command to
authorize for a partitioned topic, we are actually authorizing these topics:
```
persistent://public/default/test-topic-partition-0
persistent://public/default/test-topic-partition-1
```
Therefore, at present, when we get permission, we also need to provide the
following topic name:
```
persistent://public/default/test-topic-partition-0
persistent://public/default/test-topic-partition-1
```
However, in actual use, users prefer to provide the following name to get
all the permissions under the topic.
```
persistent://public/default/test-topic
```
So we hope that if you have time or are interested, you can provide this
support in the `internalGetPermissionsOnTopic` function.
```
if the topic is partitioned topic
Loop to get permissions for each topic
else
Logic unchanged
```
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285215
##
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:
Before that, we seem to add a version of judgment. I'm not sure whether the
partition topic is supported in the v1 version because the v1 version of the
domain path has `cluster` attribute, and the v2 version does not have this
attribute, So calling function `getPartitionedTopicMetadata` directly will
throw an exception. therefore, I think it may need to add a version check here.
```
if (topicName.isV2()) {
getPartitionedTopicMetadata(topicName, true, false);
// Auth to parititioned topic
} else {
// Non-partitioned topic normal authorization
}
```
REST API v1:
```
persistent://tenant/cluster-name/namespace/topic-name
```
REST API v2
```
persistent://tenant/namespace/topic-name
```
@sijie I'd like to hear your opinion. I don't know whether rest API v1
supports partitioned topic or not.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285215
##
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:
Before that, we seem to add a version of judgment. I'm not sure whether the
partition topic is supported in the v1 version because the v1 version of the
domain path has `cluster` attribute, and the v2 version does not have this
attribute, So calling function `getPartitionedTopicMetadata` directly will
throw an exception. therefore, I think it may need to add a version check here.
```
if (topicName.isV2()) {
getPartitionedTopicMetadata(topicName, true, false);
// Auth to parititioned topic
} else {
// Non-partitioned topic normal authorization
}
```
REST API v1:
```
persistent://tenant/cluster-name/namespace/topic-name
```
REST API v2:
```
persistent://tenant/namespace/topic-name
```
@sijie I'd like to hear your opinion. I don't know whether rest API v1
supports partitioned topic or not.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285204
##
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##
@@ -369,6 +373,24 @@ protected void internalRevokePermissionsOnTopic(String
role) {
log.error("[{}] Failed to revoke permissions for topic {}",
clientAppId(), topicUri, e);
throw new RestException(e);
}
+
+}
+
+protected void internalRevokePermissionsOnTopic(String role) {
+// 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:
Same as above.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285631
##
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();
+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> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
+testNamespace, partition.toString());
+Assert.assertEquals(partitionPermissions.get(role), expectActions);
+}
+}
+
+@Test
+public void testRevokeNonPartitionedTopic() {
+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);
+persistentTopics.revokePermissionsOnTopic(testTenant, testNamespace,
topicName, role);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), null);
+}
+
+@Test
+public void testRevokePartitionedTopic() {
+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();
+persistentTopics.createPartitionedTopic(testTenant, testNamespace,
partitionedTopicName, numPartitions);
+
+String role = "role";
+Set expectActions = new HashSet<>();
+expectActions.add(AuthAction.produce);
+persistentTopics.grantPermissionsOnTopic(testTenant, testNamespace,
partitionedTopicName, role, expectActions);
+persistentTopics.revokePermissionsOnTopic(testTenant, testNamespace,
partitionedTopicName, role);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace,
+partitionedTopicName);
+Assert.assertEquals(permissions.get(role), null);
+TopicName topicName=TopicName.get(partitionedTopicName);
+for (int i=0; i
[GitHub] [pulsar] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285560
##
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();
+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> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
+testNamespace, partition.toString());
+Assert.assertEquals(partitionPermissions.get(role), expectActions);
+}
+}
+
+@Test
+public void testRevokeNonPartitionedTopic() {
+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);
+persistentTopics.revokePermissionsOnTopic(testTenant, testNamespace,
topicName, role);
+Map> permissions =
persistentTopics.getPermissionsOnTopic(testTenant, testNamespace, topicName);
+Assert.assertEquals(permissions.get(role), null);
+}
+
+@Test
+public void testRevokePartitionedTopic() {
+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:
The same above.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r35228
##
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:
Mock is not required here.
The following code can be used to check:
```
@Test
public void testGrantPartitionedTopic() throws Exception {
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);
for (int i=0; i < numPartitions; i++) {
Map> partitionPermissions =
persistentTopics.getPermissionsOnTopic(testTenant,
testNamespace, partitionedTopicName + "-partition-" + i);
Assert.assertEquals(partitionPermissions.get(role),
expectActions);
}
}
```
The `getPermissionsOnTopic` function does not seem to support searching
partitioned topic yet. I think you can consider adding a feature to the
`getPermissionsOnTopic` function to support searching partition topic in this
pr or by opening a new pr.
If the `getPermissionsOnTopic` function supports finding partitioned topic,
you can directly call the `getPermissionsOnTopic` function to get the value of
permissions, so this loop is not needed.
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] tuteng commented on a change in pull request #5767: Support batch authorization of partitioned topic
tuteng commented on a change in pull request #5767: Support batch authorization
of partitioned topic
URL: https://github.com/apache/pulsar/pull/5767#discussion_r352285215
##
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:
Before that, we seem to add a version of judgment. I'm not sure whether the
partition topic is supported in the v1 version because the v1 version of the
domain path has `cluster` attribute, and the v2 version does not have this
attribute, So calling function `getPartitionedTopicMetadata` directly will
throw an exception. therefore, I think it may need to add a version check here.
```
if (topicName.isV2()) {
getPartitionedTopicMetadata(topicName, true, false);
// Auth to parititioned topic
} else {
// Non-partitioned topic normal authorization
}
```
REST API v1:
```
persistent://tenant/cluster-name/namespace/topic-name
```
REST API
```
persistent://tenant/namespace/topic-name
```
@sijie I'd like to hear your opinion. I don't know whether rest API v1
supports partitioned topic or not.
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
