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

[fix][security] Add timeout of sync methods and avoid call sync method for AuthoriationService #15694

Merged
merged 8 commits into from
Jun 9, 2022
Merged

[fix][security] Add timeout of sync methods and avoid call sync method for AuthoriationService #15694

merged 8 commits into from
Jun 9, 2022

Conversation

codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented May 21, 2022 •
edited by github-actions bot
Loading

Motivation

Add timeout of sync methods for the AuthoriationService to avoid infinite web thread blocking.

image

image

image

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • no-need-doc
    (Please explain why)
  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 21, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone May 21, 2022
nodece
nodece requested changes May 21, 2022
View reviewed changes
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

I don't think this PR can fix thread blocking, we should call the async method, not the sync method to authorization.

@codelipenghui
Copy link
Contributor Author

codelipenghui commented May 21, 2022 •
edited
Loading

@nodece For the pulsar codebase, we don't have a place to call allowTenantOperation, but since they are public methods, which might be called in the plugins? This is my understanding of why the method have have any reference in Pulsar but did not remove yet.

@codelipenghui
Copy link
Contributor Author

@RobertIndie @mattisonchao Please help review again, I have changed the allowTopicOperation callers to use the async method allowTopicOperationAsync so that we don't have async calls for AuthoriationService anymore in the master branch.

@codelipenghui codelipenghui changed the title [fix][security] Add timeout of sync methods for AuthoriationService [fix][security] Add timeout of sync methods and avoid call sync method for AuthoriationService May 21, 2022
@Technoboy-
Copy link
Contributor

Better to rebase

@mattisonchao mattisonchao mentioned this pull request May 21, 2022
Closed
2 tasks
eolivelli
eolivelli approved these changes May 22, 2022
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.

+1

@codelipenghui
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui force-pushed the penghui/timeout-authoriation-service branch from 2ff2b86 to e46fdf5 Compare June 3, 2022 06:44
@codelipenghui codelipenghui force-pushed the penghui/timeout-authoriation-service branch from e46fdf5 to a4ff8ce Compare June 8, 2022 14:26
@codelipenghui codelipenghui merged commit 6af365e into apache:master Jun 9, 2022
@codelipenghui codelipenghui deleted the penghui/timeout-authoriation-service branch June 9, 2022 01:06
@mattisonchao mattisonchao mentioned this pull request Jun 12, 2022
Closed
@codelipenghui codelipenghui mentioned this pull request Jun 12, 2022
Merged
1 task
codelipenghui added a commit that referenced this pull request Jun 12, 2022
@codelipenghui
[fix][security] Add timeout of sync methods and avoid call sync metho…
7c88bd1 
…d for AuthoriationService (#15694)

(cherry picked from commit 6af365e)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 13, 2022
@codelipenghui @nicoloboschi
[fix][security] Add timeout of sync methods and avoid call sync metho…
6a7d024 
…d for AuthoriationService (apache#15694)

(cherry picked from commit 6af365e)
(cherry picked from commit 7c88bd1)
@mattisonchao mattisonchao mentioned this pull request Jun 16, 2022
Merged
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jun 17, 2022
BewareMyPower pushed a commit that referenced this pull request Jul 28, 2022
@codelipenghui @BewareMyPower
[branch-2.8][fix][security] Add timeout of sync methods and avoid cal…
3a04eb5 
…l sync method for AuthoriationService (#15694)

(cherry picked from commit 6af365e)

Besides resolving the basic conflicts, this PR
- migrate `validateAdminAccessForTenantAsync` from #14149
- migrate `TenantResources#getTenantAsync` from #11693
BewareMyPower pushed a commit that referenced this pull request Jul 28, 2022
@codelipenghui @BewareMyPower
[branch-2.8][fix][security] Add timeout of sync methods and avoid cal…
a4ece07 
…l sync method for AuthoriationService (#15694)

(cherry picked from commit 6af365e)

Besides resolving the basic conflicts, this PR
- migrate `validateAdminAccessForTenantAsync` from #14149
- migrate `TenantResources#getTenantAsync` from #11693
BewareMyPower pushed a commit that referenced this pull request Jul 28, 2022
@codelipenghui @BewareMyPower
[branch-2.8][fix][security] Add timeout of sync methods and avoid cal…
5bb38a8 
…l sync method for AuthoriationService (#15694)

(cherry picked from commit 6af365e)

Besides resolving the basic conflicts, this PR
- migrate `validateAdminAccessForTenantAsync` from #14149
- migrate `TenantResources#getTenantAsync` from #11693
BewareMyPower pushed a commit that referenced this pull request Jul 29, 2022
@codelipenghui @BewareMyPower
[branch-2.8][fix][security] Add timeout of sync methods and avoid cal…
553b6d7 
…l sync method for AuthoriationService (#15694)

(cherry picked from commit 6af365e)

Besides resolving the basic conflicts, this PR
- migrate `validateAdminAccessForTenantAsync` from #14149
- migrate `TenantResources#getTenantAsync` from #11693
@BewareMyPower
Copy link
Contributor

Move the release/2.8.4 label to #16831

BewareMyPower added a commit that referenced this pull request Aug 2, 2022
@BewareMyPower @codelipenghui
[branch-2.8][fix][security] Add timeout of sync methods and avoid cal…
d81568e 
…l sync method for AuthoriationService (#15694) (#16831)

(cherry picked from commit 6af365e)

Besides resolving the basic conflicts, this PR
- migrate `validateAdminAccessForTenantAsync` from #14149
- migrate `TenantResources#getTenantAsync` from #11693

Co-authored-by: lipenghui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@eolivelli eolivelli eolivelli approved these changes

@nodece nodece nodece approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

Assignees

@codelipenghui codelipenghui

Labels
area/security cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
8 participants
@codelipenghui @Technoboy- @BewareMyPower @eolivelli @nodece @RobertIndie @Demogorgon314 @mattisonchao