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:
us...@infra.apache.org


Reply via email to