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.

Sign up for GitHub

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

Jump to bottom

[broker] fix GetTopicsOfNamespace with binary lookup service not check auth #11172

Merged
merged 4 commits into from
Aug 6, 2021
Merged

[broker] fix GetTopicsOfNamespace with binary lookup service not check auth #11172

merged 4 commits into from
Aug 6, 2021

Conversation

freeznet
Copy link
Contributor

@freeznet freeznet commented Jun 30, 2021 •
edited
Loading

Motivation

apache/pulsar-client-go#554 reported an issue and we found out an inconsistency between http lookup service and binary lookup service. It appears that http lookup service requires auth check for GetTopicsOfNamespace but not with binary lookup service.

This PR adds

  • auth check to GetTopicsOfNamespace with binary lookup service.
  • lookup throttling to GetTopicsOfNamespace with binary lookup service.

Verifying this change

  • Make sure that the change passes the CI checks.

@wolfstudy wolfstudy requested review from cckellogg, merlimat, codelipenghui, sijie and jiazhai and removed request for cckellogg June 30, 2021 13:14
@wolfstudy wolfstudy added the type/feature The PR added a new feature or issue requested a new feature label Jun 30, 2021
@wolfstudy wolfstudy added this to the 2.9.0 milestone Jun 30, 2021
@freeznet
Copy link
Contributor Author

freeznet commented Jul 1, 2021

/pulsarbot run-failure-checks

@Anonymitaet
Copy link
Member

@freeznet does this affect docs?

@freeznet
Copy link
Contributor Author

freeznet commented Jul 1, 2021

@Anonymitaet thanks for asking, and I dont think this fix relates to any doc yet.

@Anonymitaet Anonymitaet added the doc-not-needed Your PR changes do not impact docs label Jul 1, 2021
eolivelli
eolivelli requested changes Jul 2, 2021
View reviewed changes
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.

very good.

can you add a test case ?

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Show resolved Hide resolved
@freeznet freeznet force-pushed the freeznet/check-auth-for-get-topics-of-namespace branch from 31a8e16 to b1f268d Compare July 5, 2021 13:09
@freeznet freeznet force-pushed the freeznet/check-auth-for-get-topics-of-namespace branch from b1f268d to 5edbfef Compare July 6, 2021 03:49
@freeznet
Copy link
Contributor Author

freeznet commented Jul 6, 2021

/pulsarbot run-failure-checks

@freeznet freeznet requested a review from eolivelli July 6, 2021 05:25
@freeznet
Copy link
Contributor Author

freeznet commented Jul 6, 2021

@eolivelli changes have been made according to your comment, PTAL, thanks.

@sijie
Copy link
Member

sijie commented Jul 14, 2021

@eolivelli please review this PR again.

@sijie
Copy link
Member

sijie commented Jul 20, 2021

@eolivelli Can you review this again?

michaeljmarshall
michaeljmarshall approved these changes Jul 30, 2021
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM. Left a couple of low priority comments about reducing code duplication.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
@@ -1722,30 +1723,97 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
});
}

private CompletableFuture<Boolean> isNamespaceOperationAllowed(NamespaceName namespaceName,
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this method and the isTopicOperationAllowed method share a lot of core logic with just a couple places that would branch for looking up allowNamespaceOperationAsync vs allowNamespaceOperationAsync. It'd be good to consolidate this logic, if possible.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

return null;
});
final Semaphore lookupSemaphore = service.getLookupRequestSemaphore();
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing some code duplication in this file where we call service.getLookupRequestSemaphore(). It could be good to come back later and consolidate the code.

@hangc0276
Copy link
Contributor

@eolivelli Would you please help review again? thanks.

eolivelli
eolivelli approved these changes Aug 6, 2021
View reviewed changes
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.

Lgrm

@eolivelli eolivelli merged commit 64b44a5 into apache:master Aug 6, 2021
LeBW pushed a commit to LeBW/pulsar that referenced this pull request Aug 9, 2021
@freeznet @LeBW
[broker] fix GetTopicsOfNamespace with binary lookup service not ch…
04c082f 
…eck auth (apache#11172)
hangc0276 pushed a commit that referenced this pull request Aug 12, 2021
@freeznet @hangc0276
[broker] fix GetTopicsOfNamespace with binary lookup service not ch…
705aae3 
…eck auth (#11172)

(cherry picked from commit 64b44a5)
@hangc0276 hangc0276 added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Aug 12, 2021
@michaeljmarshall
Copy link
Member

@hangc0276 - I believe this commit should be cherry picked to branch-2.7 as well.

@hangc0276
Copy link
Contributor

@hangc0276 - I believe this commit should be cherry picked to branch-2.7 as well.

@michaeljmarshall Thanks for your remind, i have add release-2.7.4 label, this PR will be cherry-picked to branch-2.7.

@freeznet freeznet mentioned this pull request Sep 28, 2021
Merged
wolfstudy pushed a commit to apache/pulsar-client-go that referenced this pull request Sep 28, 2021
@freeznet
fix TestNamespaceTopicsNamespaceDoesNotExit (#625)
b0f328e 
fix TestNamespaceTopicsNamespaceDoesNotExit error after pulsar release 2.8.1
ref: apache/pulsar#11172
@MarvinCai MarvinCai mentioned this pull request Oct 27, 2021
Closed
michaeljmarshall pushed a commit that referenced this pull request Dec 10, 2021
@freeznet @michaeljmarshall
[broker] fix GetTopicsOfNamespace with binary lookup service not ch…
deb9b0b 
…eck auth (#11172)

(cherry picked from commit 64b44a5)
@michaeljmarshall michaeljmarshall added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Dec 10, 2021
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
@freeznet @ciaocloud
[broker] fix GetTopicsOfNamespace with binary lookup service not ch…
0fd8e4f 
…eck auth (apache#11172)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sijie sijie sijie approved these changes

@hangc0276 hangc0276 hangc0276 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@cckellogg cckellogg Awaiting requested review from cckellogg

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@jiazhai jiazhai Awaiting requested review from jiazhai

Assignees

@freeznet freeznet

Labels
area/security cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life doc-not-needed Your PR changes do not impact docs release/2.7.4 release/2.8.1 type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@freeznet @Anonymitaet @sijie @hangc0276 @michaeljmarshall @eolivelli @codelipenghui @wolfstudy