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

[Issue 5720][authz] - add topics authz granularity #7523

Merged
merged 4 commits into from
May 8, 2021
Merged

[Issue 5720][authz] - add topics authz granularity #7523

merged 4 commits into from
May 8, 2021

Conversation

KannarFr
Copy link
Contributor

Fixes a part of #5720

Motivation

add granularity in topics api authz

@KannarFr KannarFr force-pushed the topics-api branch 3 times, most recently from b1c7751 to ed4a34f Compare July 15, 2020 16:21
@KannarFr KannarFr changed the title [Issue 5720][authz] - WIP add topics authz granularity [Issue 5720][authz] - add topics authz granularity Aug 12, 2020
@KannarFr KannarFr force-pushed the topics-api branch 9 times, most recently from 089b529 to 359a44a Compare August 14, 2020 00:03
frankjkelly
frankjkelly reviewed Aug 29, 2020
View reviewed changes
Copy link
Contributor

@frankjkelly frankjkelly left a comment

Choose a reason for hiding this comment

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

I'm new to Pulsar but had some comments that might be of help I hope? Thanks!

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java Show resolved Hide resolved
sijie
sijie requested changes Sep 4, 2020
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I think you changed the behavior of the existing authorization plugin a lot. That doesn't sound like a good approach. We need to keep backward compatibility.

...roker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java Show resolved Hide resolved
...broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from 0f0ec40 to aa43845 Compare September 14, 2020 14:12
@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from 5ff9d17 to 6cc8f6d Compare October 26, 2020 11:35
@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from a223b0f to d17dab9 Compare January 13, 2021 22:44
@KannarFr
Copy link
Contributor Author

#9179

@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from c8008c6 to 6ea53cd Compare February 9, 2021 13:34
@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from 99f9510 to db618d6 Compare February 10, 2021 02:34
@KannarFr
Copy link
Contributor Author

@sijie @zymap this is ready to review

@KannarFr KannarFr requested a review from sijie March 22, 2021 10:08
@KannarFr
Copy link
Contributor Author

@codelipenghui Can you review too?

zymap
zymap reviewed Apr 3, 2021
View reviewed changes
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

It's a little difficult to review this PR. It's related to the Pulsar permissions so we need to be careful of changing them. And this PR changed too many APIs so it needs to take some time to review it.

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved
@zymap zymap self-requested a review April 4, 2021 09:02
@KannarFr
Copy link
Contributor Author

@zymap @315157973 @hangc0276

@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from 727b7a9 to 4dd0653 Compare April 20, 2021 21:42
@KannarFr
Copy link
Contributor Author

Rebased to support 889b9b8.

@KannarFr KannarFr force-pushed the topics-api branch 2 times, most recently from a395c70 to 3546f72 Compare April 23, 2021 10:30
@KannarFr
Copy link
Contributor Author

@zymap ?

@Geal
Copy link
Contributor

Geal commented May 3, 2021

@sijie @zymap hey, is there anything really blocking this PR, except a review? How can we help you get it merged?

This PR has been going on for over a year now, with @KannarFr spending significant time rebasing it over and over. And we identified the need for Pulsar to get truly multitenant more than 2 years ago ( #5720 ). So this is getting a bit long for a feature that is advertised on Pulsar's website from day one.

The fact is that it has been running fine on our systems for a long time, where we manage very fine rights, so there is much less risks in merging it that you might think.
So, again, how can we help you get this merged?

@zymap
Copy link
Member

zymap commented May 6, 2021

Sorry for the delay. I will take a look soon.

merlimat
merlimat reviewed May 6, 2021
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java Outdated
} catch (Exception e) {
checkConnect(topic);
// unknown error marked as internal server error
log.warn("Unexpected error while authorizing TopicOperation.LOOKUP. topic={}, role={}. Error: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to log here? The exception would be expected if the client is not authorized and since it's already bubbled up, we would end up logging that twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped in 596d85f.

PR rebased due to a recent conflict.

@sijie sijie merged commit 5dc5de8 into apache:master May 8, 2021
@lhotari lhotari mentioned this pull request May 10, 2021
Closed
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
@KannarFr @eolivelli
[Issue 5720][authz] - add topics authz granularity (apache#7523)
f4157cf 
Fixes a part of apache#5720 

### Motivation

add granularity in topics api authz
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankjkelly frankjkelly frankjkelly left review comments

@merlimat merlimat merlimat approved these changes

@sijie sijie sijie approved these changes

@zymap zymap zymap approved these changes

@315157973 315157973 Awaiting requested review from 315157973

@hangc0276 hangc0276 Awaiting requested review from hangc0276

Assignees

@KannarFr KannarFr

Labels
area/security
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@KannarFr @zymap @kyomster @Geal @merlimat @sijie @frankjkelly