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 2020/06/11 23:49:22 UTC
[GitHub] [pulsar] rdhabalia opened a new pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
rdhabalia opened a new pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251
### Motivation
Right now, Tenant admin is not able to get tenant-info to validate certain tenant info such as: `allowedClusters` for the tenant.
### Modification
Allow tenant-admin to see tenant information.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752835204
> Hi @tuteng, any thoughts on this? Thanks
There seems to be no way to do automatic generation
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752312727
@tuteng does this should be automatically generated from code like https://github.com/apache/pulsar/issues/7275?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-784172494
Hi, @rdhabalia. It looks like this PR is opened for a long time, do we still need this? If we don't need this I will close this PR and remove the `release/2.7.1` label.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet removed a comment on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet removed a comment on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752859127
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752835244
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752885782
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r551895241
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
Hi @rdhabalia
Since this code change affects both REST API and pulsar-admin docs, could you please help update the docs in this PR as below? (P.S. I've confirmed this w/ @tuteng)
Many thanks~
- REST API
Go to [TenantsBase.java, Line 69](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java#L69), add `This operation requires admin privilege` after `Get the admin configuration for a given tenant.`
- pulsar-admin
Go to [CmdTenants.java, Line 43](https://github.com/apache/pulsar/blob/master/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTenants.java#L43), add `This operation requires admin privilege` after `Gets the configuration of a tenant.`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet edited a comment on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet edited a comment on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-785686582
Hi @rdhabalia docs will be added if this PR is reopened and merged. And you can take [my comments](https://github.com/apache/pulsar/pull/7251#discussion_r551895241) as references when you want to add docs.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r551895241
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
Hi @rdhabalia
Since this code change affects both REST API and pulsar-admin docs, could you please update the docs in this PR as below? Many thanks~
- REST API
Go to [TenantsBase.java, Line 69](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java#L69), add `This operation requires admin privilege` after `Get the admin configuration for a given tenant.`
- pulsar-admin
Go to [CmdTenants.java, Line 43](https://github.com/apache/pulsar/blob/master/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTenants.java#L43), add `This operation requires admin privilege` after `Gets the configuration of a tenant.`
(P.S. I've confirmed this w/ @tuteng)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-753786477
@tuteng any thoughts on how we update this doc (admin cli doc) currently?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-644474667
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-718388706
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] frankjkelly commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
frankjkelly commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r479665515
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
This change is pretty smart - is there any documentation that needs to change as a result (from personal experience setting up AuthN/AuthZ is pretty hard outside of a standalone case). Thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-718401824
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng removed a comment on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
tuteng removed a comment on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752835204
> Hi @tuteng, any thoughts on this? Thanks
There seems to be no way to do automatic generation
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-718367908
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Huanli-Meng commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Huanli-Meng commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r522529303
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
ok, will let you know once the doc is updated. Thanks
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r551895241
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
Hi @rdhabalia
Since this code change affects both REST API and pulsar-admin docs, could you please help update the docs in this PR as below? Many thanks~
- REST API
Go to [TenantsBase.java, Line 69](https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java#L69), add `This operation requires admin privilege` after `Get the admin configuration for a given tenant.`
- pulsar-admin
Go to [CmdTenants.java, Line 43](https://github.com/apache/pulsar/blob/master/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTenants.java#L43), add `This operation requires admin privilege` after `Gets the configuration of a tenant.`
(P.S. I've confirmed this w/ @tuteng)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752859127
@tuteng thanks for your feedback, then how we update this doc (admin cli doc) currently?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-650475514
ping @sijie @jiazhai please help take a look at this PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r522480303
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
Yes. We should document that. /cc @Huanli-Meng @Jennifer88huang
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752818544
Hi @tuteng, any thoughts on this? Thanks
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-785686582
Hi @rdhabalia docs will be added if this PR is reopened and merged. And you can take my comments as references when you want to add docs.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on a change in pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on a change in pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#discussion_r554721513
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java
##########
@@ -76,7 +76,7 @@
public TenantInfo getTenantAdmin(
@ApiParam(value = "The tenant name")
@PathParam("tenant") String tenant) {
- validateSuperUserAccess();
+ validateAdminAccessForTenant(tenant);
Review comment:
Hi @rdhabalia, any thoughts on the docs?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] rdhabalia commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-718948805
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-785512425
Because this PR is opened for a long time. I will close this PR. Feel free to reopen it if you 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:
users@infra.apache.org
[GitHub] [pulsar] zymap closed pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
zymap closed pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet removed a comment on pull request #7251: [pulsar-admin] Fix tenant admin should be able to get tenant-info
Posted by GitBox <gi...@apache.org>.
Anonymitaet removed a comment on pull request #7251:
URL: https://github.com/apache/pulsar/pull/7251#issuecomment-752312727
----------------------------------------------------------------
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:
users@infra.apache.org