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 2021/12/23 04:29:50 UTC

[GitHub] [pulsar] massakam opened a new pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

massakam opened a new pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463


   ### Motivation
   
   With #13157, some namespace policies can now be changed by tenant admins as well as superusers. I don't think this change is correct.
   
   For example, I think that the publish rate limiting is to prevent a specific tenant from inconvenience to other tenants by publishing at a very high rate. It is a problem that tenant managers can raise this on their own.
   
   Therefore, the privilege to change the publish rate limit should only be granted to the administrators of the entire Pulsar instance, i.e. the superusers. 
   
   ### Modifications
   
   This reverts commit #13157.
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   ### Documentation
   
   - [ ] `no-need-doc`
   
   


-- 
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] yuruguo commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1001103013


   @massakam Thanks for your meticulous answer đź‘Ť 
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   **Finally**, I am not opposed to revert the PR(#13157), but maybe it's not a good way to revert it directly or completely, the reasons are as follows:
   - there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   - it is possible for tenant administrator not only super user to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery`
   
   Therefore, it may be a better solution to revert purposeful by a special PR, what do @eolivelli @315157973 @michaeljmarshall  think?


-- 
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] codelipenghui edited a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000191428


   We have two things here:
   
   1. The superuser assigns how many credits to a tenant.
   2. The tenant admin assigns how many credits to a namespace
   
   Currently, we don't have any tenant-level limitations, so that the superuser is not able to limit the resources of the tenant.
   https://github.com/apache/pulsar/wiki/PIP-82%3A-Tenant-and-namespace-level-rate-limiting is for this case.
   
   Another point is some policies only applied to a topic, not limit the whole namespace. The tenant admin still can create more partitions to request more resources of the cluster.


-- 
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] yuruguo edited a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000802912


   @massakam Thanks for your answers
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   **Finally**, I am not opposed to revert the PR(#13157), but maybe it's not a good way to revert it directly or completely, the reasons are as follows:
   - there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   - it is possible for tenant administrator instead of super user to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery`
   Therefore, it may be a better solution to revert purposeful, such as creating a special 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] michaeljmarshall commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1006093023


   I lean towards reverting the whole commit and then creating new PRs to add the right functionality. However, I don't have a strong opinion here.
   
   > Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   I think @massakam's point is that #13157 decreased the necessary permission level and that it has problematic consequences for things like tenant rate limits. I agree that we should leave the rate limit permissions at the superuser level for now until we make it possible to set an upper limit for a given tenant.
   
   We should figure this out before 2.10.0 gets released.


-- 
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] yuruguo commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000177091


   @massakam Thanks for your clarification, I have a few questions.
   1. Theoretically, the tenant administrators have the right to manage the namespace under it.
   The `publish-rate` example you cited may reflect the question whether the tenant administrators complete a reasonable operation with the authz. Perhaps `internalSetMaxTopicsPerNamespace` / `internalSetMaxProducersPerTopic` / ` internalSetMaxConsumersPerTopic` also have this problem although the tenant administrators can operate it at present. 
   One solution is to regulate the behavior of tenant administrators. For example, there is an upper limit for the `publish-rate` set by tenant administrators and it can only be operated by the super user once the upper limit is exceeded. 
   2. The role with `lookup topic` authz can set the publish rate to topic, is there a similar problem like the example?
   3. Except for rate-related policies in this PR, can other policies be operated by tenant administrators? 
   Including: `internalSetInactiveTopic`, `internalSetDelayedDelivery`, `internalSetMaxSubscriptionsPerTopic`
   
   @michaeljmarshall Thank you for your reply, I want to try to provide more information
   > I think we are likely missing policies at the tenant level
   
   Currently, we provide `AuthorizationProvider#allowTenantOperationAsync`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java#L283-L290
   But we don’t use `TenantOperation operation` in default implementation-`PulsarAuthorizationProvider`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L511-L517
   Maybe we need to use it to optimize this piece of permission logic
   
   > i.e. what operations should be super user and what should be tenant admin
   
   There is a summary here but there is no update  [PIP 49: Permission levels and inheritance](https://github.com/apache/pulsar/wiki/PIP-49%3A-Permission-levels-and-inheritance)


-- 
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] yuruguo edited a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000802912


   @massakam Thanks for your answers
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   **Finally**, I am not opposed to revert the PR(#13157), but maybe it's not a good way to revert it directly or completely, the reasons are as follows:
   - there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   - it is possible for tenant administrator not only super user to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery`
   
   Therefore, it may be a better solution to revert purposeful, such as creating a special 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] nkurihar merged pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
nkurihar merged pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463


   


-- 
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] yuruguo removed a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000176666


   @massakam Thanks for your clarification, I have a few questions.
   1. Theoretically, the tenant administrators have the right to manage the namespace under it.
   The `publish-rate` example you cited may reflect the question whether the tenant administrators complete a reasonable operation with the authz. Perhaps `internalSetMaxTopicsPerNamespace` / `internalSetMaxProducersPerTopic` / ` internalSetMaxConsumersPerTopic` also have this problem although the tenant administrators can operate it at present. 
   One solution is to regulate the behavior of tenant administrators. For example, there is an upper limit for the `publish-rate` set by tenant administrators and it can only be operated by the super user once the upper limit is exceeded. 
   2. The role with `lookup topic` authz can set the publish rate to topic, is there a similar problem like the example?
   3. Except for rate-related policies in this PR, can other policies be operated by tenant administrators? 
   Including: `internalSetInactiveTopic`, `internalSetDelayedDelivery`, `internalSetMaxSubscriptionsPerTopic`
   
   Thank you for your reply, I want to try to provide more information
   > I think we are likely missing policies at the tenant level
   
   Currently, we provide `AuthorizationProvider#allowTenantOperationAsync`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java#L283-L290
   But we don’t use `TenantOperation operation` in default implementation-`PulsarAuthorizationProvider`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L511-L517
   Maybe we need to use it to optimize this piece of permission logic
   
   > i.e. what operations should be super user and what should be tenant admin
   
   There is a summary here but there is no update  [PIP 49: Permission levels and inheritance](https://github.com/apache/pulsar/wiki/PIP-49%3A-Permission-levels-and-inheritance)


-- 
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] yuruguo removed a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo removed a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000802912


   @massakam Thanks for your answers
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   **Finally**, I am not opposed to revert the PR(#13157), but maybe it's not a good way to revert it directly or completely, the reasons are as follows:
   - there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   - it is possible for tenant administrator not only super user to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery`
   
   Therefore, it may be a better solution to revert purposeful, such as creating a special 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] codelipenghui commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1015502325


   @nkurihar Agree, we can revert the #13157 to avoid introducing break changes in 2.10 and it's better to start a discussion in the dev email thread


-- 
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] massakam commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
massakam commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000252494


   @yuruguo
   > The `publish-rate` example you cited may reflect the question whether the tenant administrators complete a reasonable operation with the authz. Perhaps `internalSetMaxTopicsPerNamespace` / `internalSetMaxProducersPerTopic` / ` internalSetMaxConsumersPerTopic` also have this problem although the tenant administrators can operate it at present.
   
   As far as `internalSetMaxProducersPerTopic` and `internalSetMaxConsumersPerTopic` are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.  We have added these features because a specific tenant in our company created a large number of clients and caused system instability.
   https://github.com/apache/pulsar/pull/1255/files#diff-8696b1e2ba4ee355256e31b33ccc4f1cab77f1c6c79f7b4ab508b73d43d8ad55R1252
   https://github.com/apache/pulsar/pull/1255/files#diff-8696b1e2ba4ee355256e31b33ccc4f1cab77f1c6c79f7b4ab508b73d43d8ad55R1293
   
   > One solution is to regulate the behavior of tenant administrators. For example, there is an `publish-rate` upper limit for  tenant administrators and it can only be operated by the super user once the upper limit is exceeded.
   
   That looks good to me.
   
   > 2. The role with `lookup topic` authz can set the publish rate to topic, is there a similar problem like the example?
   
   I think so. At the very least, the superuser should be able to set an upper limit.
   
   > 3. Except for rate-related policies in this PR, can other policies be operated by tenant administrators? 
   > Including: `internalSetInactiveTopic`, `internalSetDelayedDelivery`, `internalSetMaxSubscriptionsPerTopic`
   
   I think `internalSetMaxSubscriptionsPerTopic` should only be performed by the superuser. Tenants don't have much motivation to limit the number of subscriptions, and unlimited is better. Only the administrators of the Pulsar instance have the motivation to limit it.


-- 
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] nkurihar commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
nkurihar commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1014220700


   My suggestion is
   1. first revert the whole commit (at least before 2.10 release).
   2. then open new PRs and continue to discuss what @yuruguo pointed:
   > * there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   > * it is possible for tenant administrator not only super user to operate internalSetInactiveTopic / internalSetDelayedDelivery
   
   Any thoughts?


-- 
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] hsaputra commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
hsaputra commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1006225027


   FYI @cckellogg , @jerrypeng 


-- 
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] yuruguo commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000176666


   @massakam Thanks for your clarification, I have a few questions.
   1. Theoretically, the tenant administrators have the right to manage the namespace under it.
   The `publish-rate` example you cited may reflect the question whether the tenant administrators complete a reasonable operation with the authz. Perhaps `internalSetMaxTopicsPerNamespace` / `internalSetMaxProducersPerTopic` / ` internalSetMaxConsumersPerTopic` also have this problem although the tenant administrators can operate it at present. 
   One solution is to regulate the behavior of tenant administrators. For example, there is an upper limit for the `publish-rate` set by tenant administrators and it can only be operated by the super user once the upper limit is exceeded. 
   2. The role with `lookup topic` authz can set the publish rate to topic, is there a similar problem like the example?
   3. Except for rate-related policies in this PR, can other policies be operated by tenant administrators? 
   Including: `internalSetInactiveTopic`, `internalSetDelayedDelivery`, `internalSetMaxSubscriptionsPerTopic`
   
   Thank you for your reply, I want to try to provide more information
   > I think we are likely missing policies at the tenant level
   
   Currently, we provide `AuthorizationProvider#allowTenantOperationAsync`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java#L283-L290
   But we don’t use `TenantOperation operation` in default implementation-`PulsarAuthorizationProvider`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L511-L517
   Maybe we need to use it to optimize this piece of permission logic
   
   > i.e. what operations should be super user and what should be tenant admin
   
   There is a summary here but there is no update  [PIP 49: Permission levels and inheritance](https://github.com/apache/pulsar/wiki/PIP-49%3A-Permission-levels-and-inheritance)


-- 
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] yuruguo commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000802912


   @massakam Thanks for your answers
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   Finally, I am not opposed to revert the PR, but it is possible for tenant administrator to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery` / `internalGetBookieAffinityGroup` and perhaps another PR could solve it.


-- 
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] nkurihar commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
nkurihar commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1015964616


   @codelipenghui
   Thank you for your reply.
   
   @yuruguo 
   Sorry for reverting your PRs finally.
   Could you open dev email thread (and new PRs if you can) to discuss what you pointed out?


-- 
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] michaeljmarshall commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1016092659


   I agree with the decision to revert it before 2.10.0 is released and to discuss this on the mailing list. Thanks for merging this, @nkurihar.


-- 
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] yuruguo edited a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000177091


   @massakam Thanks for your clarification, I have a few questions.
   1. Theoretically, the tenant administrators have the right to manage the namespace under it.
   The `publish-rate` example you cited may reflect the question whether the tenant administrators complete a reasonable operation with the authz. Perhaps `internalSetMaxTopicsPerNamespace` / `internalSetMaxProducersPerTopic` / ` internalSetMaxConsumersPerTopic` also have this problem although the tenant administrators can operate it at present. 
   One solution is to regulate the behavior of tenant administrators. For example, there is an `publish-rate` upper limit for  tenant administrators and it can only be operated by the super user once the upper limit is exceeded. 
   2. The role with `lookup topic` authz can set the publish rate to topic, is there a similar problem like the example?
   3. Except for rate-related policies in this PR, can other policies be operated by tenant administrators? 
   Including: `internalSetInactiveTopic`, `internalSetDelayedDelivery`, `internalSetMaxSubscriptionsPerTopic`
   
   @michaeljmarshall Thank you for your reply, I want to try to provide more information
   > I think we are likely missing policies at the tenant level
   
   Currently, we provide `AuthorizationProvider#allowTenantOperationAsync`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java#L283-L290
   But we don’t use `TenantOperation operation` in default implementation-`PulsarAuthorizationProvider`
   https://github.com/apache/pulsar/blob/76f35666deb5a956b7eef9732a3028b246e5294c/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L511-L517
   Maybe we need to use it to optimize this piece of permission logic
   
   > i.e. what operations should be super user and what should be tenant admin
   
   There is a summary here but there is no update  [PIP 49: Permission levels and inheritance](https://github.com/apache/pulsar/wiki/PIP-49%3A-Permission-levels-and-inheritance)


-- 
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] codelipenghui commented on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000191428


   We have two things here


-- 
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] yuruguo edited a comment on pull request #13463: Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

Posted by GitBox <gi...@apache.org>.
yuruguo edited a comment on pull request #13463:
URL: https://github.com/apache/pulsar/pull/13463#issuecomment-1000802912


   @massakam Thanks for your answers
   > As far as internalSetMaxProducersPerTopic and internalSetMaxConsumersPerTopic are concerned, it was my colleague who added these features, and initially only superusers were able to perform these operations.
   
   Maybe we can find the reason why the access permission was changed from super user to tenant administrator based on the submission history.
   
   > I think internalSetMaxSubscriptionsPerTopic should only be performed by the superuser. 
   
   I agree with it.
   
   **Finally**, I am not opposed to revert the PR(#13157), but maybe it's not a good way to revert it directly or completely, the reasons are as follows:
   - there are some corrections to annotations that are not entirely related to the modified authz of namespace policies
   - it is possible for tenant administrator instead of super user to operate `internalSetInactiveTopic` / `internalSetDelayedDelivery`
   
   Therefore, it may be a better solution to revert purposeful, such as creating a special 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org