You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/27 08:18:32 UTC
[GitHub] [pulsar] TakaHiR07 opened a new pull request, #16815: improve revoke permission
TakaHiR07 opened a new pull request, #16815:
URL: https://github.com/apache/pulsar/pull/16815
### Motivation
Steps to reproduce:
1. grant permission for role1 on topic "persistent://public/default/test"
2. revoke permission for role1 on topic "persistent://public/default/test"
3. zk remain empty record, as shown in the picture
### Modifications
If the topic has no roles after revoke permission, remove topicKey from TopicAuthenticationMap
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as *PersistentTopicsTest*.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1229152530
@TakaHiR07 Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#discussion_r961512934
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/api/AuthenticatedProducerConsumerTest.java:
##########
@@ -466,4 +466,45 @@ public void testTlsTransportWithAnyAuth(Supplier<String> url, Authentication aut
@Cleanup
Producer<byte[]> ignored = client.newProducer().topic(topicName).create();
}
+
+ @Test
+ public void testCleanupEmptyTopicAuthenticationMap() throws Exception {
+ Map<String, String> authParams = new HashMap<>();
+ authParams.put("tlsCertFile", TLS_CLIENT_CERT_FILE_PATH);
+ authParams.put("tlsKeyFile", TLS_CLIENT_KEY_FILE_PATH);
+ Authentication authTls = new AuthenticationTls();
+ authTls.configure(authParams);
+ internalSetup(authTls);
+
+ admin.clusters().createCluster("test", ClusterData.builder().build());
Review Comment:
```suggestion
admin.clusters().createCluster("test", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#discussion_r961511797
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -359,7 +359,13 @@ private CompletableFuture<Void> revokePermissionsAsync(String topicUri, String r
}
// Write the new policies to metadata store
return namespaceResources().setPoliciesAsync(namespaceName, p -> {
- p.auth_policies.getTopicAuthentication().get(topicUri).remove(role);
+ p.auth_policies.getTopicAuthentication().computeIfPresent(topicUri, (k, roles) -> {
+ roles.remove(role);
Review Comment:
The `roles` is ImmutableCollections.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] TakaHiR07 commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
TakaHiR07 commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1231265513
> @TakaHiR07 Please rebase the master and resolve the conflict.
@Jason918 done.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#discussion_r961511797
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -359,7 +359,13 @@ private CompletableFuture<Void> revokePermissionsAsync(String topicUri, String r
}
// Write the new policies to metadata store
return namespaceResources().setPoliciesAsync(namespaceName, p -> {
- p.auth_policies.getTopicAuthentication().get(topicUri).remove(role);
+ p.auth_policies.getTopicAuthentication().computeIfPresent(topicUri, (k, roles) -> {
+ roles.remove(role);
Review Comment:
The `roles` is ImmutableCollections.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] TakaHiR07 commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
TakaHiR07 commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1235205936
/pulsarbot run-failure-checks
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1236497219
Link to https://github.com/apache/pulsar/pull/17393.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 merged pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
Jason918 merged PR #16815:
URL: https://github.com/apache/pulsar/pull/16815
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] TakaHiR07 commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
TakaHiR07 commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1208050551
> The change looks good, could you please help add a test for the new change?
Thanks for your review. I have added a test. PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1244825682
Hi @TakaHiR07
It looks like we got many conflicts when cherry-picking it to branch-2.9.
Would you mind pushing a PR to branch-2.9? (To avoid cherry-picking involving bugs)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1229152531
@TakaHiR07 Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #16815: [improve] clean the empty topicAuthenticationMap in zk when revoke permission
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16815:
URL: https://github.com/apache/pulsar/pull/16815#issuecomment-1229152436
@TakaHiR07 Please rebase the master and resolve the conflict.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org