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/07/02 02:49:21 UTC
[GitHub] [pulsar] wolfstudy opened a new pull request #7424: [Issue: 7379] Improve security setting of Pulsar Functions
wolfstudy opened a new pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424
Signed-off-by: xiaolong.ran <rx...@apache.org>
Fixes #7379
### Motivation
Rename some settings to make those broker-client TLS settings clearer.
### Modifications
- replace `clientAuthenticationParameters` with `brokerClientAuthenticationParameters`
- replace `clientAuthenticationPlugin` with `brokerClientAuthenticationPlugin`
- replace `tlsHostnameVerificationEnable` with `tlsEnableHostnameVerification`
----------------------------------------------------------------
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] jerrypeng commented on pull request #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424#issuecomment-652785706
@wolfstudy I have a BC consideration for this. Upgrading an existing cluster to this might cause configurations not to become set. While we can move to the new names for these configs when should still honor the old config names as well.
----------------------------------------------------------------
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 #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424#discussion_r448751767
##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/functions/worker/PulsarFunctionE2ESecurityTest.java
##########
@@ -177,10 +177,10 @@ void setup(Method method) throws Exception {
ClientBuilder clientBuilder = PulsarClient.builder().serviceUrl(this.workerConfig.getPulsarServiceUrl())
.operationTimeout(1000, TimeUnit.MILLISECONDS);
- if (isNotBlank(workerConfig.getClientAuthenticationPlugin())
- && isNotBlank(workerConfig.getClientAuthenticationParameters())) {
- clientBuilder.authentication(workerConfig.getClientAuthenticationPlugin(),
- workerConfig.getClientAuthenticationParameters());
+ if (isNotBlank(workerConfig.getBrokerClientAuthenticationPlugin())
Review comment:
You need to consider backward compatibility. You can add `getBrokerClientAuthenticationPlugin()` method in WorkerConfig.
```
String getBrokerClientAuthenticationPlugin() {
if (null == brokerClientAuthenticationParameters) {
return clientAuthenticationParameters;
} else {
return brokerClientAuthenticationParameters;
}
}
```
We should do the same thing for `getBrokerClientAuthenticationPlugin`.
##########
File path: conf/functions_worker.yml
##########
@@ -48,13 +48,25 @@ downloadDirectory: download/pulsar_functions
# Configure the pulsar client used by function metadata management
#
-# points
+# points
+# Whether to enable TLS when clients connect to broker
+useTls: false
+# For TLS:
+# brokerServiceUrl=pulsar+ssl://localhost:6651/
pulsarServiceUrl: pulsar://localhost:6650
+# For TLS:
+# webServiceUrl=https://localhost:8443/
pulsarWebServiceUrl: http://localhost:8080
+
+############################################
+# security settings for pulsar broker client
+############################################
+# The path to trusted certificates used by the Pulsar client to authenticate with Pulsar brokers
+# brokerClientTrustCertsFilePath:
# the authentication plugin to be used by the pulsar client used in worker service
-# clientAuthenticationPlugin:
Review comment:
Can you move this to end of the file and mark them as `@Deprecated`?
##########
File path: pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java
##########
@@ -258,12 +258,12 @@
category = CATEGORY_CLIENT_SECURITY,
doc = "The authentication plugin used by function workers to talk to brokers"
)
- private String clientAuthenticationPlugin;
+ private String brokerClientAuthenticationPlugin;
Review comment:
Please keep the original settings and mark them as deprecated.
----------------------------------------------------------------
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 #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424#issuecomment-652802200
> @wolfstudy I have a BC consideration for this. Upgrading an existing cluster to this might cause configurations not to become set. While we can move to the new names for these configs when should still honor the old config names as well.
Thanks to @sijie @jerrypeng for reminding me, I ignored backward compatibility, will fix it.
----------------------------------------------------------------
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] codelipenghui merged pull request #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424
----------------------------------------------------------------
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 #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424#issuecomment-653072078
/pulsarbot run-failure-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] wolfstudy commented on pull request #7424: [Issue: 7379] Improve security setting of Pulsar Functions
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7424:
URL: https://github.com/apache/pulsar/pull/7424#issuecomment-652861771
@sijie @jerrypeng PTAL again thanks
----------------------------------------------------------------
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