[fix][broker] Producer/Consumer should call allowTopicOperationAsync #20142

michaeljmarshall · 2023-04-19T16:12:14Z

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

doc-not-needed

Matching PR in forked repository

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

…)" This reverts commit fc17c1d.

nodece · 2023-04-19T17:20:03Z

#20068 is the correct implementation, I don't suggest we revert that.

I suggest we just update the Producer.java and Consumer.java to call the AuthorizationService#allowTopicOperationAsync method, and we also can add the @Deprecated to AuthorizationService#canProduceAsync, AuthorizationService#canConsumeAsync.

BTW, the canLookupAsync should also be deprecated.

michaeljmarshall · 2023-04-19T17:22:37Z

#20068 is the correct implementation, I don't suggest we revert that.

What is the purpose of changing the implementation of an abstraction that we are deprecating?

nodece · 2023-04-19T17:24:44Z

#20068 is the correct implementation, I don't suggest we revert that.

What is the purpose of changing the implementation of an abstraction that we are deprecating?

Make sure this logic/behavior is correct.

Maybe this affects the commit history, but I still suggest we keep #20068.

michaeljmarshall · 2023-04-19T17:31:49Z

#20068 is the correct implementation, I don't suggest we revert that.

What is the purpose of changing the implementation of an abstraction that we are deprecating?

Make sure this logic is correct.

The logic has been there for a long time. It's likely extensions built on top of those methods already account for the short coming, which is why I said that solution could have unintended consequences.

The problem described by #20066 is that the Producer and Consumer use the wrong method in the AuthorizationService. In my opinion, the right fix is to fix which method those classes use.

Maybe this affects the commit history, but I still suggest we keep #20068.

That's a fair point. I will break this PR into two PRs to make the discussion clearer.

…ache#20068)"" This reverts commit bc3346a.

nodece

LGTM

michaeljmarshall · 2023-04-19T17:39:25Z

@nodece - I'll open a PR to deprecate those methods.

nodece

I rethought this PR, if the custom authorization provider didn't handle the TopicOperation, which introduces unintended consequences.

michaeljmarshall · 2023-04-19T18:43:21Z

I rethought this PR, if the custom authorization provider didn't handle the TopicOperation, which introduces unintended consequences.

@nodece - by that same logic, #20068 is invalid. The default for the provider is to fail, so it is a safe change:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

Lines 344 to 351 in efb251c

    
           default CompletableFuture<Boolean> allowTopicOperationAsync(TopicName topic, 
        
                                                                       String role, 
        
                                                                       TopicOperation operation, 
        
                                                                       AuthenticationDataSource authData) { 
        
               return FutureUtil.failedFuture( 
        
                   new IllegalStateException("TopicOperation [" + operation.name() + "] is not supported by the Authorization" 
        
                           + "provider you are using.")); 
        
           }

Further, the allowTopicOperationAsync has been used by ServerCnx for a while.

nodece · 2023-04-19T18:59:44Z

I rethought this PR, if the custom authorization provider didn't handle the TopicOperation, which introduces unintended consequences.

@nodece - by that same logic, #20068 is invalid. The default for the provider is to fail, so it is a safe change:

pulsar/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationProvider.java

Lines 344 to 351 in efb251c

default CompletableFuture<Boolean> allowTopicOperationAsync(TopicName topic,

String role,

TopicOperation operation,

AuthenticationDataSource authData) {

return FutureUtil.failedFuture(

new IllegalStateException("TopicOperation [" + operation.name() + "] is not supported by the Authorization"

+ "provider you are using."));

}

Further, the allowTopicOperationAsync has been used by ServerCnx for a while.

I know we've been using allowTopicOperationAsync for a long time, but I'm still concerned about compatibility issues.

If we allow for this change, I think #20068 is also safe, is it right?

michaeljmarshall · 2023-04-19T19:11:58Z

If we allow for this change, I think #20068 is also safe, is it right?

My concern for #20068 is not with the AuthorizationProvider implementation but rather with pulsar extensions running in the broker that call the AuthorizationService#canProduceAsync and AuthorizationService#canConsumeAsync. My primary argument is that there is no compelling reason to change the AuthorizationService implementation because there is already a "correct" alternative to use. As such, I want to leave those methods unchanged and to update the broker code to use the newer method to get the correct behavior.

nodece

Thank you for your explanation, we focus on different points, so different ideas emerge.
In my opinion, both #20142 and #20068 PRs are able to solve #20066, and in order to solve the issue completely I submitted #20145.

#20068 can also be reverted.

codecov-commenter · 2023-04-20T04:57:40Z

Codecov Report

Merging #20142 (f757419) into master (9b72302) will increase coverage by 39.78%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master   #20142       +/-   ##
=============================================
+ Coverage     33.17%   72.96%   +39.78%     
- Complexity    12236    31927    +19691     
=============================================
  Files          1499     1868      +369     
  Lines        114413   138402    +23989     
  Branches      12431    15236     +2805     
=============================================
+ Hits          37962   100986    +63024     
+ Misses        71499    29397    -42102     
- Partials       4952     8019     +3067

Flag	Coverage Δ
inttests	`24.25% <50.00%> (?)`
systests	`24.84% <62.50%> (?)`
unittests	`72.26% <100.00%> (+39.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...sar/broker/authorization/AuthorizationService.java	`58.92% <100.00%> (+48.92%)`	⬆️
...ava/org/apache/pulsar/broker/service/Consumer.java	`86.64% <100.00%> (+26.85%)`	⬆️
...ava/org/apache/pulsar/broker/service/Producer.java	`82.46% <100.00%> (+25.20%)`	⬆️

... and 1538 files with indirect coverage changes

…#20143) This reverts commit fc17c1d. ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think this approach could have unintended consequences. Instead, I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. I propose an update to the `Consumer` and `Producer` logic here #20142. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, I think we should not change their implementations. ### Modifications * Revert #20068 ### Verifying this change This change is trivial. It removes certain test changes that were only made to make the previous PR work. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally.

poorbarcode · 2023-05-07T23:03:42Z

@michaeljmarshall

To avoid conflicts, I cherry-picked all three patches into branch 3.0.

…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)

…#20143) This reverts commit fc17c1d. ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think this approach could have unintended consequences. Instead, I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. I propose an update to the `Consumer` and `Producer` logic here #20142. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, I think we should not change their implementations. ### Modifications * Revert #20068 ### Verifying this change This change is trivial. It removes certain test changes that were only made to make the previous PR work. ### 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 00dc7a0)

Similar to: #20142 ### Motivation In #20142 we changed the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. Our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, so this change helps us move in the right direction. This PR follows the same logic and updates the WebSocket proxy to remove all calls to the `can*` methods in the `AuthorizationService` ### Modifications * Update `ProducerHandler`, `ConsumerHandler`, and `ReaderHander` in the WebSocket Proxy 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.

michaeljmarshall added 2 commits April 19, 2023 10:44

Revert "[fix][broker] Fix tenant admin authorization bug. (apache#20068…

bc3346a

…)" This reverts commit fc17c1d.

[fix][broker] Producer/Consumer should call allowTopicOperationAsync

11bea29

michaeljmarshall added type/bug The PR fixed a bug or issue reported a bug doc-not-needed Your PR changes do not impact docs ready-to-test area/authz labels Apr 19, 2023

michaeljmarshall added this to the 3.1.0 milestone Apr 19, 2023

michaeljmarshall requested review from congbobo184, lhotari and nodece April 19, 2023 16:12

michaeljmarshall self-assigned this Apr 19, 2023

michaeljmarshall mentioned this pull request Apr 19, 2023

[fix][broker] Fix tenant admin authorization bug. #20068

Merged

15 tasks

Revert "Revert "[fix][broker] Fix tenant admin authorization bug. (ap…

f757419

…ache#20068)"" This reverts commit bc3346a.

nodece approved these changes Apr 19, 2023

View reviewed changes

michaeljmarshall mentioned this pull request Apr 19, 2023

[revert] "[fix][broker] Fix tenant admin authorization bug. (#20068)" #20143

Merged

1 task

michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use and removed type/bug The PR fixed a bug or issue reported a bug labels Apr 19, 2023

nodece requested changes Apr 19, 2023

View reviewed changes

nodece approved these changes Apr 19, 2023

View reviewed changes

michaeljmarshall merged commit dc5e497 into apache:master Apr 20, 2023

michaeljmarshall deleted the fix-authz branch April 20, 2023 05:17

poorbarcode added release/3.0.1 cherry-picked/branch-3.0 labels May 7, 2023

michaeljmarshall mentioned this pull request May 11, 2023

[improve][ws] Use allowTopicOperationAsync for authz checks #20299

Merged

1 task

RobertIndie added the area/broker label Aug 7, 2023

[fix][broker] Producer/Consumer should call allowTopicOperationAsync #20142

[fix][broker] Producer/Consumer should call allowTopicOperationAsync #20142

Uh oh!

Conversation

michaeljmarshall commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

Uh oh!

nodece commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

nodece commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

nodece left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

nodece commented Apr 19, 2023

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 20, 2023

Codecov Report

Uh oh!

poorbarcode commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michaeljmarshall commented Apr 19, 2023 •

edited

Loading

nodece commented Apr 19, 2023 •

edited

Loading

nodece commented Apr 19, 2023 •

edited

Loading

nodece left a comment •

edited

Loading

poorbarcode commented May 7, 2023 •

edited

Loading