You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/12/04 06:49:42 UTC
[GitHub] [pulsar] nodece opened a new pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
nodece opened a new pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824
Signed-off-by: Zixuan Liu <no...@gmail.com>
Fixes #8338
### Motivation
>In some scenarios, users use their own function-worker to connect to an existing pulsar cluster. Their own function-worker and pulsar cluster have different authentication methods, In the following code, when both function-worker and client have enabled the authentication and authorization services, the authentication and authorization can take effect. A better way is to separate them. function-worker can enable and disable the authentication service, and the broker-client can also enable and disable the authentication service according to the configuration.
### Modifications
Add a configuration called `brokerClientAuthenticationEnabled` in the configuration file, which is disabled by default. It is used to control whether the broker-client of function-worker enable or disable the authentication.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
tuteng commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-739717701
@nodece You are right.
> @nodece @sijie There is a bit of confusion here. If we want to add the prefix of `brokerClient` to distinguish the different operations between client and broker_client, do we also need to add the prefix of `brokerClient` to the following options?
>
> ```
> # Enforce authorization on accessing functions api
> authorizationEnabled: false
> # Set of authentication provider name list, which is a list of class names
> authenticationProviders:
> # Authorization provider fully qualified class-name
> authorizationProvider: org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider
> ```
This is the authentication and authorization plugin for function-worker, `brokerClientAuthenticationPlugin`, `brokerClientAuthenticationParameters` already exist https://github.com/apache/pulsar/blob/master/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L280 to distinguish it from function-worker
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-739762706
@nodece @tuteng thank you for clarifications!
@nodece great contribution!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-739683739
@sijie @wolfstudy These configurations already exist in https://github.com/apache/pulsar/pull/8824/files#diff-d834862abe40feef5d85eec66686cd1b378b78cab5f384570da79890ba85c7b2R64-R70. I'm not sure if is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#discussion_r535900038
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java
##########
@@ -273,6 +273,11 @@
doc = "The frequency of instance liveness check, in milliseconds"
)
private long instanceLivenessCheckFreqMs;
+ @FieldContext(
+ category = CATEGORY_CLIENT_SECURITY,
+ doc = "Whether to enable the broker client authentication used by function workers to talk to brokers"
+ )
+ private boolean brokerClientAuthenticationEnabled = false;
Review comment:
Don't use `boolean`. This would cause a compatibility issue. The worker service is not able to start with an old configuration.
Use boxed `Boolean`.
```
private Boolean brokerClientAuthenticationEnabled = null;
```
Then add an implementation `isBrokerClientAuthenticationEnabled` to check if `brokerClientAuthenticationEnabled` is set or not. If `brokerClientAuthenticationEnabled` is not set, fall back to use `authenticationEnabled`.
```
public boolean isBrokerClientAuthenticationEnabled() {
if (brokerClientAuthenticationEnabled != null) {
return brokerClientAuthenticationEnabled;
} else {
return authenticationEnabled;
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng edited a comment on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
tuteng edited a comment on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-739717701
@nodece You are right.
> @nodece @sijie There is a bit of confusion here. If we want to add the prefix of `brokerClient` to distinguish the different operations between client and broker_client, do we also need to add the prefix of `brokerClient` to the following options?
>
> ```
> # Enforce authorization on accessing functions api
> authorizationEnabled: false
> # Set of authentication provider name list, which is a list of class names
> authenticationProviders:
> # Authorization provider fully qualified class-name
> authorizationProvider: org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider
> ```
This is the authentication and authorization plugin for function-worker, `brokerClientAuthenticationPlugin`, `brokerClientAuthenticationParameters` already exist https://github.com/apache/pulsar/blob/master/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L280 to distinguish it from function-worker @wolfstudy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-791116370
Confirmed with Penghui, the right logic is to allow users to enable/disable authentication separately. This PR fixes the logic and add description in the code snippet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-738713575
@wolfstudy you are right.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tuteng edited a comment on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
tuteng edited a comment on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-739717701
@nodece You are right.
> @nodece @sijie There is a bit of confusion here. If we want to add the prefix of `brokerClient` to distinguish the different operations between client and broker_client, do we also need to add the prefix of `brokerClient` to the following options?
>
> ```
> # Enforce authorization on accessing functions api
> authorizationEnabled: false
> # Set of authentication provider name list, which is a list of class names
> authenticationProviders:
> # Authorization provider fully qualified class-name
> authorizationProvider: org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider
> ```
This is the authentication and authorization plugin configuration for function-worker, `brokerClientAuthenticationPlugin`, `brokerClientAuthenticationParameters` already exist https://github.com/apache/pulsar/blob/master/pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java#L280 to distinguish it from function-worker @wolfstudy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a change in pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#discussion_r535907909
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java
##########
@@ -273,6 +273,11 @@
doc = "The frequency of instance liveness check, in milliseconds"
)
private long instanceLivenessCheckFreqMs;
+ @FieldContext(
+ category = CATEGORY_CLIENT_SECURITY,
+ doc = "Whether to enable the broker client authentication used by function workers to talk to brokers"
+ )
+ private boolean brokerClientAuthenticationEnabled = false;
Review comment:
@sijie Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy commented on pull request #8824: [Issue 8338][Function Worker] Splitting the authentication logic of function worker and client
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8824:
URL: https://github.com/apache/pulsar/pull/8824#issuecomment-738698410
@nodece @sijie There is a bit of confusion here. If we want to add the prefix of `brokerClient` to distinguish the different operations between client and broker_client, do we also need to add the prefix of `brokerClient` to the following options?
```
# Enforce authorization on accessing functions api
authorizationEnabled: false
# Set of authentication provider name list, which is a list of class names
authenticationProviders:
# Authorization provider fully qualified class-name
authorizationProvider: org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org