[refactor][fn] Use AuthorizationServer more in Function Worker API #19975

michaeljmarshall · 2023-03-31T02:38:09Z

Motivation

The current function worker interfaces introduced by #8560 are hard to extend. It is better to pass an object that we can add fields to as needed.

Modifications

Introduce a new AuthenticationParameters class to wrap all of the relevant authentication "data". This class is somewhat unfortunate in that we already have many classes that hold authentication information. However, my goal with this class is to wrap all of the relevant auth info. This class is only currently used in the Function Worker API, but I plan to add it to the rest of the Admin HTTP endpoints.
Add proxyRoles setting to function worker.

Verifying this change

There are tests to cover the changes.

Documentation

doc

This change has Javadoc updates to document the changes.

Matching PR in forked repository

PR in forked repository: michaeljmarshall#37

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/Authentication.java

...r-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/WorkerImpl.java

...functions/worker/src/main/java/org/apache/pulsar/functions/worker/service/api/Component.java

This reverts commit e6b451c.

michaeljmarshall · 2023-04-07T05:11:28Z

@lhotari - thanks for your review. I've refactored things a bit more. Would you take another look? Thanks!

…19975) The current function worker interfaces introduced by #8560 are hard to extend. It is better to pass an object that we can add fields to as needed. * Introduce a new `Authentication` class to wrap all of the relevant authentication "data". This class is somewhat unfortunate in that we already have many classes that hold authentication information. However, my goal with this class is to wrap all of the relevant auth info. This class is only currently used in the Function Worker API, but I plan to add it to the rest of the Admin HTTP endpoints. * Deprecate all of the methods that are no longer needed. Add a default implementation to call the new methods that supersede those methods. * Add `proxyRoles` setting to function worker. There are tests to cover the changes. This PR changes several interfaces in completely backwards compatible ways. It's possible we'll want to improve the abstraction for the `Authentication` class by making it an interface. - [x] `doc` This change has Javadoc updates to document the changes. PR in forked repository: michaeljmarshall#37 (cherry picked from commit 55acbe6)

…pache#19975) The current function worker interfaces introduced by apache#8560 are hard to extend. It is better to pass an object that we can add fields to as needed. * Introduce a new `Authentication` class to wrap all of the relevant authentication "data". This class is somewhat unfortunate in that we already have many classes that hold authentication information. However, my goal with this class is to wrap all of the relevant auth info. This class is only currently used in the Function Worker API, but I plan to add it to the rest of the Admin HTTP endpoints. * Deprecate all of the methods that are no longer needed. Add a default implementation to call the new methods that supersede those methods. * Add `proxyRoles` setting to function worker. There are tests to cover the changes. This PR changes several interfaces in completely backwards compatible ways. It's possible we'll want to improve the abstraction for the `Authentication` class by making it an interface. - [x] `doc` This change has Javadoc updates to document the changes. PR in forked repository: #37 (cherry picked from commit 55acbe6) (cherry picked from commit 54c97ad)

…pache#19975) The current function worker interfaces introduced by apache#8560 are hard to extend. It is better to pass an object that we can add fields to as needed. * Introduce a new `Authentication` class to wrap all of the relevant authentication "data". This class is somewhat unfortunate in that we already have many classes that hold authentication information. However, my goal with this class is to wrap all of the relevant auth info. This class is only currently used in the Function Worker API, but I plan to add it to the rest of the Admin HTTP endpoints. * Deprecate all of the methods that are no longer needed. Add a default implementation to call the new methods that supersede those methods. * Add `proxyRoles` setting to function worker. There are tests to cover the changes. This PR changes several interfaces in completely backwards compatible ways. It's possible we'll want to improve the abstraction for the `Authentication` class by making it an interface. - [x] `doc` This change has Javadoc updates to document the changes. PR in forked repository: #37 (cherry picked from commit 55acbe6) (cherry picked from commit 54c97ad) (cherry picked from commit cf40926)

…0046) ### Motivation In #19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. ### Modifications * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. ### Verifying this change This change is covered by the `TopicsAuthTest`. ### Documentation - [x] `doc-not-needed` This is an internal change that does not need to be documented. ### Matching PR in forked repository PR in forked repository: skipping PR since the relevant tests pass locally

…0046) In #19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. This change is covered by the `TopicsAuthTest`. - [x] `doc-not-needed` This is an internal change that does not need to be documented. PR in forked repository: skipping PR since the relevant tests pass locally (cherry picked from commit 7990948)

…0046) In #19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. This change is covered by the `TopicsAuthTest`. - [x] `doc-not-needed` This is an internal change that does not need to be documented. PR in forked repository: skipping PR since the relevant tests pass locally (cherry picked from commit 7990948) (cherry picked from commit 02b27e8)

…ache#20046) ### Motivation In apache#19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. ### Modifications * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. ### Verifying this change This change is covered by the `TopicsAuthTest`. ### Documentation - [x] `doc-not-needed` This is an internal change that does not need to be documented. ### Matching PR in forked repository PR in forked repository: skipping PR since the relevant tests pass locally

…pache#19975) The current function worker interfaces introduced by apache#8560 are hard to extend. It is better to pass an object that we can add fields to as needed. * Introduce a new `Authentication` class to wrap all of the relevant authentication "data". This class is somewhat unfortunate in that we already have many classes that hold authentication information. However, my goal with this class is to wrap all of the relevant auth info. This class is only currently used in the Function Worker API, but I plan to add it to the rest of the Admin HTTP endpoints. * Deprecate all of the methods that are no longer needed. Add a default implementation to call the new methods that supersede those methods. * Add `proxyRoles` setting to function worker. There are tests to cover the changes. This PR changes several interfaces in completely backwards compatible ways. It's possible we'll want to improve the abstraction for the `Authentication` class by making it an interface. - [x] `doc` This change has Javadoc updates to document the changes. PR in forked repository: michaeljmarshall#37 (cherry picked from commit 55acbe6)

…ache#20046) In apache#19975, we introduced a wrapper for all authentication parameters. This PR adds that wrapper to the Rest Producer. * Use `AuthenticationParameters` to simplify parameter management in Rest Producer. * Add method to the `AuthorizationService` that takes the `AuthenticationParameters`. * Update annotations on Rest Producer to indicate that a 401 is an expected response. This change is covered by the `TopicsAuthTest`. - [x] `doc-not-needed` This is an internal change that does not need to be documented. PR in forked repository: skipping PR since the relevant tests pass locally (cherry picked from commit 7990948) (cherry picked from commit 02b27e8)

freeznet · 2023-04-13T05:19:06Z

@michaeljmarshall I noticed that this PR changed the interface of the functions worker's API, such changes should be avoided to cherry-pick into the historical branches.
As for the APIs changes, it requires a PIP first, then adopting the implementation to the codebase, see https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md#when-is-a-pip-required. It should be better to revert this PR first, send the PIP to the community, and then get the implementation to the codebase, WDYT?

eolivelli · 2023-04-13T06:30:51Z

This is not strictly a public API change, it is more like fixing a security issue.
Nobody is expected to implement these interfaces.

codelipenghui · 2023-04-13T07:44:42Z

@freeznet I have forward the email under the private mailing list to your apache email, so that you can get all the context about the changes.

michaeljmarshall · 2023-04-21T19:19:26Z

This actually made it into 2.10.4.

michaeljmarshall · 2023-06-27T21:56:01Z

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

[refactor][fn] Use AuthorizationServer more in Function Worker API

76eaafe

michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/function area/admin labels Mar 31, 2023

michaeljmarshall added this to the 3.0.0 milestone Mar 31, 2023

michaeljmarshall requested review from codelipenghui, eolivelli, lhotari and nicoloboschi March 31, 2023 02:38

michaeljmarshall self-assigned this Mar 31, 2023

lhotari requested changes Apr 6, 2023

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/Authentication.java Show resolved Hide resolved

lhotari reviewed Apr 6, 2023

View reviewed changes

...r-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/WorkerImpl.java Outdated Show resolved Hide resolved

lhotari reviewed Apr 6, 2023

View reviewed changes

...functions/worker/src/main/java/org/apache/pulsar/functions/worker/service/api/Component.java Show resolved Hide resolved

michaeljmarshall added 15 commits April 6, 2023 14:12

Rename Authentication to AuthenticationParameters

178147e

Remove deprecated methods

eee95fe

Fix compilation and checkstyle errors

4ec82fc

Restore parameter order for updateSink

82403c3

Fix throwIfNotSuperUser

821aa2c

Pass client auth data source to be consistent

c809da6

Undo unnecessary import cleanup

a20b74a

Merge remote-tracking branch 'upstream/master' into refactor-fn-auth

7489711

Clean up methods

63af656

Add test for WorkerImpl and fix throwIfNotSuperUser

2b148c4

Remove null check from isValidOriginalPrincipal

72e840f

Fix pom.xml

e6b451c

Revert "Fix pom.xml"

8ccc09a

This reverts commit e6b451c.

Undo unnecessary whitespace change

a5b4ce5

Remove unnecessarily deprected methods

dd65b16

Remove unused imports

360be0f

michaeljmarshall merged commit 55acbe6 into apache:master Apr 7, 2023

michaeljmarshall deleted the refactor-fn-auth branch April 7, 2023 17:21

michaeljmarshall mentioned this pull request Apr 8, 2023

[refactor][broker] Use AuthenticationParameters for rest producer #20046

Merged

1 task

michaeljmarshall added cherry-picked/branch-2.11 release/2.11.1 release/2.10.5 labels Apr 8, 2023

michaeljmarshall mentioned this pull request Apr 8, 2023

Branch-2.10 cherry pick tests michaeljmarshall/pulsar#40

Merged

michaeljmarshall mentioned this pull request Apr 8, 2023

Branch-2.9 cherry pick tests michaeljmarshall/pulsar#41

Merged

michaeljmarshall added cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 release/2.9.6 labels Apr 9, 2023

michaeljmarshall added release/2.10.4 and removed release/2.10.5 labels Apr 21, 2023

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

michaeljmarshall mentioned this pull request Jul 25, 2023

Add proxyRoles config to function worker configmap datastax/kaap#106

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor][fn] Use AuthorizationServer more in Function Worker API #19975

[refactor][fn] Use AuthorizationServer more in Function Worker API #19975

Uh oh!

michaeljmarshall commented Mar 31, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaeljmarshall commented Apr 7, 2023

Uh oh!

freeznet commented Apr 13, 2023

Uh oh!

eolivelli commented Apr 13, 2023

Uh oh!

codelipenghui commented Apr 13, 2023

Uh oh!

michaeljmarshall commented Apr 21, 2023

Uh oh!

michaeljmarshall commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[refactor][fn] Use AuthorizationServer more in Function Worker API #19975

[refactor][fn] Use AuthorizationServer more in Function Worker API #19975

Uh oh!

Conversation

michaeljmarshall commented Mar 31, 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!

Uh oh!

Uh oh!

Uh oh!

michaeljmarshall commented Apr 7, 2023

Uh oh!

freeznet commented Apr 13, 2023

Uh oh!

eolivelli commented Apr 13, 2023

Uh oh!

codelipenghui commented Apr 13, 2023

Uh oh!

michaeljmarshall commented Apr 21, 2023

Uh oh!

michaeljmarshall commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaeljmarshall commented Mar 31, 2023 •

edited

Loading