Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] [broker] Producer and consumer will all be disconnected while using tenant admin to produce/consume #20066

Closed
2 tasks done
dragonls opened this issue Apr 11, 2023 · 0 comments · Fixed by #20068 or #20142 · May be fixed by #20145
Closed
2 tasks done

[Bug] [broker] Producer and consumer will all be disconnected while using tenant admin to produce/consume #20066

dragonls opened this issue Apr 11, 2023 · 0 comments · Fixed by #20068 or #20142 · May be fixed by #20145
Labels
type/bug The PR fixed a bug or issue reported a bug

Comments

@dragonls
Copy link
Contributor

Search before asking

  • I searched in the issues and found nothing similar.

Version

Pulsar 2.9.3

Minimal reproduce step

  1. Set up a pulsar cluster with topicLevelPoliciesEnabled=true.
  2. Create a namespace t1/n1 and set admin role tenant_admin as the admin of tenant t1.
  3. Create topic t1/n1/t1 and use role tenant_admin to produce/consumer this topic. DO NOT GRANT the permissions of tenant_admin to the topic/namespace.
  4. Update the namespace/topic policy, such as grant another role role1 with produce to topic t1/n1/t1.
  5. We can see the producers/consumers of role tenant_admin will all first disconnect and then reconnect.

What did you expect to see?

All producers and consumers should be stable.

What did you see instead?

The producers/consumers of role tenant_admin will all first disconnect and then reconnect.

Anything else?

According to the logic in org.apache.pulsar.broker.service.ServerCnx, all permission check will go to org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync, which means the tenant admin should be the super user of all namespace/topic under specific tenant.

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@dragonls dragonls added the type/bug The PR fixed a bug or issue reported a bug label Apr 11, 2023
michaeljmarshall added a commit that referenced this issue Apr 20, 2023
…20142)

Fixes: #20066

### Motivation

In #20068 we changed the way that the `AuthorizationService` is implemented. I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method.

Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, this change helps us move in the right direction.

### Modifications

* Update `Producer` and `Consumer` in broker to call the `AuthorizationService#allowTopicOperationAsync` method.

### Verifying this change

This change is trivial.

### Documentation

- [x] `doc-not-needed`

### Matching PR in forked repository

PR in forked repository: Skipping PR as I ran tests locally.
poorbarcode pushed a commit that referenced this issue May 7, 2023
…20142)

Fixes: #20066

### Motivation

In #20068 we changed the way that the `AuthorizationService` is implemented. I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method.

Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, this change helps us move in the right direction.

### Modifications

* Update `Producer` and `Consumer` in broker to call the `AuthorizationService#allowTopicOperationAsync` method.

### Verifying this change

This change is trivial.

### Documentation

- [x] `doc-not-needed`

### Matching PR in forked repository

PR in forked repository: Skipping PR as I ran tests locally.

(cherry picked from commit dc5e497)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment