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/07/20 10:30:42 UTC
[GitHub] [pulsar] nodece opened a new pull request, #16700: [improve][admin-cli] Add TLS provider support
nodece opened a new pull request, #16700:
URL: https://github.com/apache/pulsar/pull/16700
Signed-off-by: Zixuan Liu <no...@gmail.com>
### Motivation
The `pulsar-admin` doesn't support setting the TLS provider.
### Modifications
- Add `webserviceTlsProvider` to `client.conf` file
- Add setting the TLS provider in `PulsarAdminTool.java`
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-required`
Add setting the `webserviceTlsProvider` to `reference-configuration#client` section.
- [ ] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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] MarvinCai commented on a diff in pull request #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on code in PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#discussion_r925519666
##########
conf/client.conf:
##########
@@ -67,3 +67,8 @@ tlsTrustStorePath=
# TLS TrustStore password
tlsTrustStorePassword=
+
+# Set up TLS provider for web service
+# When TLS authentication with CACert is used, the valid value is either OPENSSL or JDK.
+# When TLS authentication with KeyStore is used, available options can be SunJSSE, Conscrypt and so on.
+webserviceTlsProvider=
Review Comment:
do we really need this config? seems it's just the same as `--tls-provider`
--
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 #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#discussion_r929493156
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java:
##########
@@ -79,6 +80,12 @@ public static class RootParams {
description = "Enable TLS common name verification")
Boolean tlsEnableHostnameVerification;
+ @Parameter(names = {"--tls-provider"}, description = "Set up TLS provider. "
+ + "When TLS authentication with CACert is used, the valid value is either OPENSSL or JDK. "
+ + "When TLS authentication with KeyStore is used, available options can be SunJSSE, Conscrypt "
+ + "and so on.")
+ String tlsProvider;
Review Comment:
> Will it throw an exception if user providers lowercase like `openssl`?
Yes. It will throw no Enum.
> Or we can use Enum instead ?
Here are multiple values: OPENSSL, JDK, SunJSSE and so on.
--
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 pull request #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#issuecomment-1192209721
> Please add unit test for the new configuration and CLI option
@codelipenghui Done, could you review this PR once?
--
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] Technoboy- merged pull request #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #16700:
URL: https://github.com/apache/pulsar/pull/16700
--
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 #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#discussion_r926194856
##########
conf/client.conf:
##########
@@ -67,3 +67,8 @@ tlsTrustStorePath=
# TLS TrustStore password
tlsTrustStorePassword=
+
+# Set up TLS provider for web service
+# When TLS authentication with CACert is used, the valid value is either OPENSSL or JDK.
+# When TLS authentication with KeyStore is used, available options can be SunJSSE, Conscrypt and so on.
+webserviceTlsProvider=
Review Comment:
You are right, the `webserviceTlsProvider` equals with `--tls-provider` flag. I suggest we should add this config.
--
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] Technoboy- commented on a diff in pull request #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#discussion_r929488245
##########
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java:
##########
@@ -79,6 +80,12 @@ public static class RootParams {
description = "Enable TLS common name verification")
Boolean tlsEnableHostnameVerification;
+ @Parameter(names = {"--tls-provider"}, description = "Set up TLS provider. "
+ + "When TLS authentication with CACert is used, the valid value is either OPENSSL or JDK. "
+ + "When TLS authentication with KeyStore is used, available options can be SunJSSE, Conscrypt "
+ + "and so on.")
+ String tlsProvider;
Review Comment:
Will it throw an exception if user providers lowercase like `openssl` ?
Or we can use Enum instead ?
--
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 pull request #16700: [improve][admin-cli] Add TLS provider support
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #16700:
URL: https://github.com/apache/pulsar/pull/16700#issuecomment-1194965049
> LGTM, but I'm still concerned if we need to add the test to verify this configuration can work correctly. not just test the configuration has this property.
Good catch. Currently, we are just verifying whether we load the tlsProvider config correctly, add an integration test is the next work.
--
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