[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation #23110

Apurva007 · 2024-07-31T22:54:23Z

PIP: #22016

Motivation

Apache Pulsar's TLS encryption configuration is not pluggable. It only supports file-based certificates. This makes
Pulsar difficult to adopt for organizations that require loading TLS certificates by other mechanisms.

Modifications

Creating a new PulsarSslFactory plugin that has a default implementation which matches current Pulsar behavior of reading file based certificates. This factory is then introduced into each of the classes that create Netty, Jetty or Async Http objects.

It also deletes various current classes that perform the above operations based on the underlying connection framework.

Verifying this change

Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: Apurva007#6

pulsar-common/src/main/java/org/apache/pulsar/common/util/NetworkMode.java

lhotari

Impressive work @Apurva007! This WIP PR is almost finished. I added a comment about documenting the PulsarSslFactory which I believe you already had on your todo list.

pulsar-common/src/main/java/org/apache/pulsar/common/util/PulsarSslFactory.java

lhotari · 2024-08-10T09:35:05Z

please rebase (or merge latest master) to resolve the merge conflicts with current master branch.

lhotari

A few comments, mainly about the ScheduledExecutorService lifecycle handling.

pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/ConnectionPoolTest.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConnectionPool.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarChannelInitializer.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/AdminProxyHandler.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ServiceChannelInitializer.java

pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java

lhotari

Recent changes look good to me. I added a comment about disabling refreshing.

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarChannelInitializer.java

lhotari

LGTM

lhotari · 2024-08-19T19:54:45Z

Great work @Apurva007 !

Apurva007 · 2024-08-20T04:42:37Z

Thanks @lhotari !

codecov-commenter · 2024-08-20T07:45:36Z

Codecov Report

Attention: Patch coverage is 85.21971% with 111 lines in your changes missing coverage. Please review.

Project coverage is 74.55%. Comparing base (bbc6224) to head (dde4770).
Report is 534 commits behind head on master.

Files	Patch %	Lines
...he/pulsar/common/util/DefaultPulsarSslFactory.java	81.98%	21 Missing and 8 partials ⚠️
.../apache/pulsar/proxy/server/AdminProxyHandler.java	79.62%	10 Missing and 1 partial ⚠️
...rg/apache/pulsar/broker/service/BrokerService.java	61.11%	6 Missing and 1 partial ⚠️
...java/org/apache/pulsar/client/impl/HttpClient.java	83.72%	5 Missing and 2 partials ⚠️
...ulsar/broker/service/PulsarChannelInitializer.java	81.81%	5 Missing and 1 partial ⚠️
.../java/org/apache/pulsar/broker/web/WebService.java	85.00%	5 Missing and 1 partial ⚠️
...che/pulsar/functions/worker/rest/WorkerServer.java	84.61%	5 Missing and 1 partial ⚠️
...pulsar/proxy/server/ServiceChannelInitializer.java	83.78%	5 Missing and 1 partial ⚠️
...java/org/apache/pulsar/proxy/server/WebServer.java	85.71%	5 Missing and 1 partial ⚠️
...g/apache/pulsar/websocket/service/ProxyServer.java	84.61%	5 Missing and 1 partial ⚠️
... and 8 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23110       /-   ##
============================================
  Coverage     73.57%   74.55%    0.98%     
- Complexity    32624    33706     1082     
============================================
  Files          1877     1918       41     
  Lines        139502   144717     5215     
  Branches      15299    15819      520     
============================================
  Hits         102638   107894     5256     
  Misses        28908    28581     -327     
- Partials       7956     8242      286

Flag	Coverage Δ
inttests	`27.69% <52.86%> ( 3.11%)`	⬆️
systests	`24.66% <4.52%> ( 0.33%)`	⬆️
unittests	`73.91% <85.21%> ( 1.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	`99.03% <100.00%> (-0.36%)`	⬇️
...pache/pulsar/jetty/tls/JettySslContextFactory.java	`90.00% <100.00%> (-1.31%)`	⬇️
...n/java/org/apache/pulsar/broker/PulsarService.java	`84.45% <100.00%> ( 2.08%)`	⬆️
...va/org/apache/pulsar/compaction/CompactorTool.java	`69.73% <100.00%> ( 0.81%)`	⬆️
...pache/pulsar/common/policies/data/ClusterData.java	`100.00% <ø> (ø)`
...a/org/apache/pulsar/admin/cli/PulsarAdminTool.java	`90.47% <100.00%> ( 3.70%)`	⬆️
...org/apache/pulsar/client/cli/PulsarClientTool.java	`79.43% <100.00%> ( 10.98%)`	⬆️
...g/apache/pulsar/client/impl/ClientBuilderImpl.java	`86.82% <100.00%> ( 0.83%)`	⬆️
.../org/apache/pulsar/client/impl/ConnectionPool.java	`76.44% <100.00%> ( 1.92%)`	⬆️
...rg/apache/pulsar/client/impl/PulsarClientImpl.java	`75.55% <100.00%> ( 1.24%)`	⬆️
... and 24 more

... and 486 files with indirect coverage changes

…ext and SSL Engine generation (apache#23110) Co-authored-by: Apurva Telang <[email protected]>

github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jul 31, 2024

lhotari added the ready-to-test label Aug 1, 2024

lhotari reviewed Aug 1, 2024

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/util/NetworkMode.java Outdated Show resolved Hide resolved

lhotari reviewed Aug 1, 2024

View reviewed changes

pulsar-common/src/main/java/org/apache/pulsar/common/util/PulsarSslFactory.java Outdated Show resolved Hide resolved

Apurva007 force-pushed the pip-337-impl branch 7 times, most recently from 8c05ae0 to 5c1da7e Compare August 14, 2024 00:36

Apurva007 changed the title ~~[improve] [Work In Progress] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation~~ [improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation Aug 14, 2024

github-actions bot added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-required Your PR changes impact docs and you will update later. labels Aug 14, 2024

Apurva007 force-pushed the pip-337-impl branch from 5c1da7e to ebd9bd8 Compare August 14, 2024 04:10

Apurva007 requested a review from lhotari August 14, 2024 05:51

lhotari reviewed Aug 14, 2024

View reviewed changes

Apurva007 force-pushed the pip-337-impl branch 2 times, most recently from e6b169e to 5824f6e Compare August 14, 2024 21:53

Apurva007 requested a review from lhotari August 15, 2024 06:13

lhotari reviewed Aug 15, 2024

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarChannelInitializer.java Show resolved Hide resolved

pip-337-impl

dde4770

Apurva007 force-pushed the pip-337-impl branch from 5824f6e to dde4770 Compare August 19, 2024 08:14

Apurva007 requested a review from lhotari August 19, 2024 16:21

lhotari approved these changes Aug 19, 2024

View reviewed changes

lhotari requested review from nodece, michaeljmarshall and Technoboy- August 19, 2024 19:55

lhotari merged commit 2d46bfa into apache:master Aug 20, 2024
51 checks passed

gaoran10 mentioned this pull request Aug 23, 2024

Fix TLS initialization streamnative/mop#1426

Merged

3 tasks

grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024

[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Cont…

848075e

…ext and SSL Engine generation (apache#23110) Co-authored-by: Apurva Telang <[email protected]>

lhotari added this to the 4.0.0 milestone Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation #23110

[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation #23110

Apurva007 commented Jul 31, 2024 •

edited

Loading

lhotari left a comment

lhotari commented Aug 10, 2024

lhotari left a comment

lhotari left a comment •

edited

Loading

lhotari left a comment

lhotari commented Aug 19, 2024

Apurva007 commented Aug 20, 2024

codecov-commenter commented Aug 20, 2024 •

edited

Loading

[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation #23110

[improve] PIP-337: Implement SSL Factory Plugin to customize SSL Context and SSL Engine generation #23110

Conversation

Apurva007 commented Jul 31, 2024 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

lhotari left a comment

Choose a reason for hiding this comment

lhotari commented Aug 10, 2024

lhotari left a comment

Choose a reason for hiding this comment

lhotari left a comment • edited Loading

Choose a reason for hiding this comment

lhotari left a comment

Choose a reason for hiding this comment

lhotari commented Aug 19, 2024

Apurva007 commented Aug 20, 2024

codecov-commenter commented Aug 20, 2024 • edited Loading

Codecov Report

Apurva007 commented Jul 31, 2024 •

edited

Loading

lhotari left a comment •

edited

Loading

codecov-commenter commented Aug 20, 2024 •

edited

Loading