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 2021/05/26 06:39:37 UTC
[GitHub] [pulsar] linlinnn opened a new pull request #10710: Geo rp conf
linlinnn opened a new pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710
Fixes #10693
### Motivation
use unify configuration from remote cluster for geo-replicator, and remove the former logic that using the local cluster's configuration.
--
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 #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710
--
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] gaoran10 commented on a change in pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#discussion_r642744964
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java
##########
@@ -83,11 +83,67 @@ void run() throws PulsarAdminException {
@Parameter(names = "--proxy-protocol", description = "protocol to decide type of proxy routing eg: SNI", required = false)
private ProxyProtocol proxyProtocol;
+ @Parameter(names = "--tls-enable", description = "Enable tls connection", required = false)
+ private boolean brokerClientTlsEnabled = false;
+
+ @Parameter(names = "--tls-allow-insecure", description = "Allow insecure tls connection", required = false)
+ private boolean tlsAllowInsecureConnection = false;
+
+ @Parameter(names = "--tls-enable-keystore", description = "Whether use KeyStore type to authenticate", required = false)
+ private boolean brokerClientTlsEnabledWithKeyStore = false;
+
+ @Parameter(names = "--tls-trust-store-type", description = "TLS TrustStore type configuration for internal client eg: JKS", required = false)
+ private String brokerClientTlsTrustStoreType = "JKS";
+
+ @Parameter(names = "--tls-trust-store", description = "TLS TrustStore path for internal client", required = false)
+ private String brokerClientTlsTrustStore;
+
+ @Parameter(names = "--tls-trust-store-pwd", description = "TLS TrustStore password for internal client", required = false)
+ private String brokerClientTlsTrustStorePassword;
+
+ @Parameter(names = "--tls-trust-certs-filepath", description = "path for the trusted TLS certificate file", required = false)
+ private String brokerClientTrustCertsFilePath;
+
Review comment:
Maybe we could add a new base class for Create and Update command, there are many same configurations.
--
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] linlinnn commented on pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#issuecomment-851960753
@wangjialing218 @codelipenghui @gaoran10 I address your comments, could you please help review again?
--
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] gaoran10 commented on a change in pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#discussion_r642744964
##########
File path: pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdClusters.java
##########
@@ -83,11 +83,67 @@ void run() throws PulsarAdminException {
@Parameter(names = "--proxy-protocol", description = "protocol to decide type of proxy routing eg: SNI", required = false)
private ProxyProtocol proxyProtocol;
+ @Parameter(names = "--tls-enable", description = "Enable tls connection", required = false)
+ private boolean brokerClientTlsEnabled = false;
+
+ @Parameter(names = "--tls-allow-insecure", description = "Allow insecure tls connection", required = false)
+ private boolean tlsAllowInsecureConnection = false;
+
+ @Parameter(names = "--tls-enable-keystore", description = "Whether use KeyStore type to authenticate", required = false)
+ private boolean brokerClientTlsEnabledWithKeyStore = false;
+
+ @Parameter(names = "--tls-trust-store-type", description = "TLS TrustStore type configuration for internal client eg: JKS", required = false)
+ private String brokerClientTlsTrustStoreType = "JKS";
+
+ @Parameter(names = "--tls-trust-store", description = "TLS TrustStore path for internal client", required = false)
+ private String brokerClientTlsTrustStore;
+
+ @Parameter(names = "--tls-trust-store-pwd", description = "TLS TrustStore password for internal client", required = false)
+ private String brokerClientTlsTrustStorePassword;
+
+ @Parameter(names = "--tls-trust-certs-filepath", description = "path for the trusted TLS certificate file", required = false)
+ private String brokerClientTrustCertsFilePath;
+
Review comment:
Maybe we could add a new base class for the `Create` and `Update` command, there are many same configurations.
--
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 commented on a change in pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#discussion_r639785017
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
##########
@@ -1029,21 +1029,19 @@ public PulsarClient getReplicationClient(String cluster) {
clientBuilder.authentication(pulsar.getConfiguration().getBrokerClientAuthenticationPlugin(),
pulsar.getConfiguration().getBrokerClientAuthenticationParameters());
}
- if (pulsar.getConfiguration().isBrokerClientTlsEnabled()) {
+ if (data.isBrokerClientTlsEnabled()) {
Review comment:
This will be a breaking change if the existing clusters enabled the TLS with the same TLS certificate. If the configuration does not present in the `ClusterData`, we should use the broker configuration file.
--
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] linlinnn commented on pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
linlinnn commented on pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#issuecomment-851965221
@wangjialing218
Thanks for your suggestions.
I use `--cluster-config-file` to solve your first suggestion.
For the second one, I don't see it in former logic.
--
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 commented on pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#issuecomment-851440626
@linlinnn Could you please check the last comment of @wangjialing218 ?
--
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] wangjialing218 commented on pull request #10710: [issue 10693] use unify configuration from remote cluster for geo-replicator
Posted by GitBox <gi...@apache.org>.
wangjialing218 commented on pull request #10710:
URL: https://github.com/apache/pulsar/pull/10710#issuecomment-849499080
I have two suggestions:
1. There are too many paramaters for tls setting in `CmdClusters`. Could we combine all remote cluster auth setting to one paramater as a JSON string, just like "--external-pulsars" in `CmdFunctions`
https://github.com/apache/pulsar/blob/5a7ba52193c069bc40705bcdc74287aae1066b17/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdFunctions.java#L325
2. Compared with `AuthenticationConfig` class, `tlsHostnameVerificationEnable` is missed. Shoud this paramter also be config in geo-replicator?
https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-common/src/main/java/org/apache/pulsar/common/functions/AuthenticationConfig.java#L35
--
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