Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[improve][admin-cli] Add TLS provider support #16700

Merged
merged 1 commit into from
Jul 28, 2022
Merged

[improve][admin-cli] Add TLS provider support #16700

merged 1 commit into from
Jul 28, 2022

Conversation

nodece
Copy link
Member

@nodece nodece commented Jul 20, 2022 •
edited
Loading

Signed-off-by: Zixuan Liu nodeces@gmail.com

Motivation

The pulsar-admin doesn't support setting the TLS provider.

Default to OpenSSL implement, which doesn't support the switch to JDK implement, now we add support for this.

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?

  • 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)

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jul 20, 2022
MarvinCai
MarvinCai reviewed Jul 20, 2022
View reviewed changes
conf/client.conf
# 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=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this config? seems it's just the same as --tls-provider

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, the webserviceTlsProvider equals with --tls-provider flag. I suggest we should add this config.

codelipenghui
codelipenghui reviewed Jul 21, 2022
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit test for the new configuration and CLI option

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 21, 2022
@nodece nodece force-pushed the admin-cli-tls-provider branch from 7608de5 to d713116 Compare July 22, 2022 05:43
@nodece
Copy link
Member Author

nodece commented Jul 22, 2022

Please add unit test for the new configuration and CLI option

@codelipenghui Done, could you review this PR once?

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
mattisonchao
mattisonchao approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Technoboy-
Technoboy- reviewed Jul 26, 2022
View reviewed changes
pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/PulsarAdminTool.java
+ "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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it throw an exception if user providers lowercase like openssl ?
Or we can use Enum instead ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nodece
Copy link
Member Author

nodece commented Jul 26, 2022

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.

@nodece nodece force-pushed the admin-cli-tls-provider branch from d713116 to 0104d0c Compare July 26, 2022 04:24
@nodece
[improve][admin-cli] Add TLS provider support
d01b6fb 
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece force-pushed the admin-cli-tls-provider branch from 0104d0c to d01b6fb Compare July 26, 2022 06:47
@nodece nodece requested a review from Technoboy- July 26, 2022 06:48
@Technoboy- Technoboy- merged commit de42e15 into apache:master Jul 28, 2022
@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Aug 4, 2022
Gleiphir2769 pushed a commit to Gleiphir2769/pulsar that referenced this pull request Aug 4, 2022
@nodece @Gleiphir2769
[improve][admin-cli] Add TLS provider support (apache#16700)
8248ede
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Sep 14, 2022
@nodece @nicoloboschi
[improve][admin-cli] Add TLS provider support (apache#16700)
9517704 
(cherry picked from commit de42e15)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@MarvinCai MarvinCai Awaiting requested review from MarvinCai

Assignees

@nodece nodece

Labels
area/cli doc-complete Your PR changes impact docs and the related docs have been already added.
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nodece @Technoboy- @codelipenghui @MarvinCai @mattisonchao @Anonymitaet