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

Fix topic ownership is not checked #9767

Merged
merged 4 commits into from
Mar 3, 2021

Conversation

315157973
Copy link
Contributor

Motivation

  1. Currently, the API of topic policies does not check the ownership of topic. In the case of multiple brokers, a cache not init error will appear.

  2. The parameter validation of the read and write API should not be the same, and the read API should not verify validatePoliciesReadOnlyAccess(). If ReadOnly is set, the read API will also be abnormal.

  3. General API should not use validateAdminAccessForTenant().

@eolivelli
Copy link
Contributor

@315157973 can you please add tests that cover your fix ?

You are removing the assertions from PersistentTopicBase.
Are we losing those assertions in the v1 API ?

@315157973
Copy link
Contributor Author

315157973 commented Mar 1, 2021

@315157973 can you please add tests that cover your fix ?

You are removing the assertions from PersistentTopicBase.
Are we losing those assertions in the v1 API ?

I will add unit tests later. Topic policies currently only exist on V2,so are those assertions

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @315157973 , Could you please add a multiple brokers test case since the problem can't reproduce on a single broker cluster. We need to ensure no matter the admin to call which broker of the cluster, it can get the correct policy data. So my suggestion is to check the returned policy multiple times for different brokers.

@codelipenghui codelipenghui added this to the 2.8.0 milestone Mar 1, 2021
@codelipenghui codelipenghui added release/2.7.1 component/topic-policy type/bug The PR fixed a bug or issue reported a bug labels Mar 1, 2021
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@315157973 thanks for your clarification

@codelipenghui codelipenghui merged commit 38a1f1d into apache:master Mar 3, 2021
codelipenghui pushed a commit that referenced this pull request Mar 5, 2021
…ch-2.7) (#9781)

### Motivation
1. Currently, the API of topic policies does not check the ownership of topic. In the case of multiple brokers, a `cache not init error` will appear.

2. The parameter validation of the read and write API should not be the same, and the read API should not verify `validatePoliciesReadOnlyAccess()`. If `ReadOnly` is set, the read API will also be abnormal.

3. General API should not use `validateAdminAccessForTenant()`.

The original PR is #9767
zymap added a commit to streamnative/pulsar-archived that referenced this pull request Mar 5, 2021
…ch-2.7) (apache#9781)

### Motivation
1. Currently, the API of topic policies does not check the ownership of topic. In the case of multiple brokers, a `cache not init error` will appear.

2. The parameter validation of the read and write API should not be the same, and the read API should not verify `validatePoliciesReadOnlyAccess()`. If `ReadOnly` is set, the read API will also be abnormal.

3. General API should not use `validateAdminAccessForTenant()`.

The original PR is apache#9767
@zymap zymap added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Mar 5, 2021
mlyahmed pushed a commit to mlyahmed/pulsar that referenced this pull request Mar 5, 2021
### Motivation
1. Currently, the API of topic policies does not check the ownership of topic. In the case of multiple brokers, a `cache not init error` will appear.
@315157973 315157973 deleted the validate-owner branch March 12, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants