Revert "[Authorization] Converge authz of ns policies from super-user to tenant-administrator (#13157)"

[massakam]

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

[michaeljmarshall]

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.

This is a good point. My mistake was in assuming that a tenant admin should have permission to update all namespace policies.

I think we are likely missing policies at the tenant level. It seems like a tenant admin should be able to operate on all namespace policies. That being said, I agree that we should revert this commit now.

@yuruguo - the work doesn't need to be lost completely. We could update the default PulsarAuthorizationProvider implementation to align with correct permissions (i.e. what operations should be super user and what should be tenant admin). It might be worth discussing the "right" permission level on the mailing list.

[yuruguo]

@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

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

Lines 283 to 290 in 76f3566

	default CompletableFuture<Boolean> allowTenantOperationAsync(String tenantName, String role,
	TenantOperation operation,
	AuthenticationDataSource authData) {
	return FutureUtil.failedFuture(new IllegalStateException(
	String.format("allowTenantOperation(%s) on tenant %s is not supported by the Authorization" +
	" provider you are using.",
	operation.toString(), tenantName)));
	}

But we don’t use TenantOperation operation in default implementation-PulsarAuthorizationProvider

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

Lines 511 to 517 in 76f3566

	@Override
	public CompletableFuture<Boolean> allowTenantOperationAsync(String tenantName,
	String role,
	TenantOperation operation,
	AuthenticationDataSource authData) {
	return validateTenantAdminAccess(tenantName, role, authData);
	}

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

[codelipenghui]

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.

[massakam]

@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.

[yuruguo]

@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?

[michaeljmarshall]

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.

[hsaputra]

FYI @cckellogg , @jerrypeng

[nkurihar]

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?

[codelipenghui]

@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

[nkurihar]

@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?

[michaeljmarshall]

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.