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 2022/06/11 03:41:05 UTC
[GitHub] [pulsar] nodece opened a new pull request, #16014: [fix][broker] Fix create client with TLS config
nodece opened a new pull request, #16014:
URL: https://github.com/apache/pulsar/pull/16014
Signed-off-by: Zixuan Liu <no...@gmail.com>
### Motivation
In PulsarService, create a client with an incorrect config.
### Modifications
- Fix check TLS enable
- Setup ciphers and protocols
- Remove duplicate setTlsTrustCertsFilePath
### Documentation
- [x] `doc-not-needed`
It is a fix.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r896310678
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
> @nodece - is your point that since `tlsEnabled` is broken, we don't need to support it?
Yes, I want to using `this.getConfiguration().isBrokerClientTlsEnabled()` for keep same with create admin client.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895192277
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
> Looks like the reason why we can't use the `isTlsEnabled` here is that we can have tls port and non-tls port for a broker? It's better to add a description here to clarify it.
The `isTlsEnabled` has been deprecated and used for the broker, not the client. The `brokerClient` prefix is used for client/admin in config file.
Currently, the `getAdminClient()` has the same check, link to https://github.com/apache/pulsar/pull/16014/files#diff-8db8be87611ed21e4a80eb15b8749729d23a49a835cb0401c8c72e5225241b21R1461, we should keep config.
> And, please also add the compatibility explanation for the change. Or do we need to consider the compatibility issue? Since users can upgrade to a new version broker but with the old version config file.
OK, we should update this description to be cleaner.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895184770
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
Looks like the reason why we can't use the `isTlsEnabled` here is that we can have tls port and non-tls port for a broker? It's better to add a description here to clarify it.
And, please also add the compatibility explanation for the change. Or do we need to consider the compatibility issue? Since users can upgrade to a new version broker but with the old version config file.
The document of `brokerClientTlsEnabled` also needs to update since we also use broker clients for system topics.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r896163915
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
Is it worth making it like this:
```suggestion
boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled() || this.getConfiguration().isTlsEnabled();
```
That would ensure backwards compatibility.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895626778
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
@nodece Could you please provide more details of `No breaking changes.`? Without this change, `this.getConfiguration().isTlsEnabled()` will be used to create the broker client. But after this change, users must configure `brokerClientTlsEnabled`, what will happen if `brokerClientTlsEnabled` absents?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16014:
URL: https://github.com/apache/pulsar/pull/16014
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895192277
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
> Looks like the reason why we can't use the `isTlsEnabled` here is that we can have tls port and non-tls port for a broker? It's better to add a description here to clarify it.
The `isTlsEnabled` has been deprecated and used for the broker, not the client. The `brokerClient` prefix is used for client/admin in config file.
Currently, the `getAdminClient()` has the same check, link to https://github.com/apache/pulsar/pull/16014/files#diff-8db8be87611ed21e4a80eb15b8749729d23a49a835cb0401c8c72e5225241b21R1461, we should keep config.
> And, please also add the compatibility explanation for the change. Or do we need to consider the compatibility issue? Since users can upgrade to a new version broker but with the old version config file.
No breaking changes.
> The document of `brokerClientTlsEnabled` also needs to update since we also use broker clients for system topics.
OK, we should update this description to be cleaner.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r896310822
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
Thanks @michaeljmarshall.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nodece commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r895789888
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
Currently, there has a bug:
```
conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
```
The `getTlsCertificateFilePath()` is not ca cert, which shoule be `this.getConfiguration().getTlsCertificateFilePath()` or `this.getConfiguration().getBrokerClientTrustCertsFilePath()`.
When `this.getConfiguration().isTlsEnabled()` is `true`, users will meet `Failed reason: General OpenSslEngine problem`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #16014: [fix][broker] Fix create client with TLS config
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #16014:
URL: https://github.com/apache/pulsar/pull/16014#discussion_r896165819
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -1418,12 +1418,13 @@ public synchronized PulsarClient getClient() throws PulsarServerException {
ClientConfigurationData conf =
ConfigurationDataUtils.loadData(overrides, initialConf, ClientConfigurationData.class);
- conf.setServiceUrl(this.getConfiguration().isTlsEnabled()
- ? this.brokerServiceUrlTls : this.brokerServiceUrl);
- conf.setTlsAllowInsecureConnection(this.getConfiguration().isTlsAllowInsecureConnection());
- conf.setTlsTrustCertsFilePath(this.getConfiguration().getTlsCertificateFilePath());
+ boolean tlsEnabled = this.getConfiguration().isBrokerClientTlsEnabled();
Review Comment:
@nodece - is your point that since `tlsEnabled` is broken, we don't need to support 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org