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.

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

Listener TLS customization #8082

Merged
merged 8 commits into from
Jan 31, 2022
Merged

Listener TLS customization #8082

merged 8 commits into from
Jan 31, 2022

Conversation

apucher
Copy link
Contributor

@apucher apucher commented Jan 28, 2022

Description

Some enterprise scenarios require multiple TLS certs to be served by pinot components. The TLS specification (in Java) limits us to a single certificte per entry point, thus necessitating the creation of dedicated listeners per certificate.

This PR adds listener-specific TLS configuration options. It leverages the existing TLS settings as defaults and enables listener specs to override individual properties, including keystore, truststore, etc. The PR also contains a number of smaller enhancements and bug fixes to TLS support in pinot.

The PR further adds a dedicated integration test for TLS scenarios that test the correctness of accepting and rejecting secure connection attempts.

TlsIntegrationTest.java serves as documentation for the use of the added features

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Listener specs can now use arbitrary names if they specify a protocol:

controller.access.protocols=internal,https
controller.access.protocols.internal.protocol=http

Listener specs can now override TLS settings on a per-property basis:

controller.tls.keystore.path=./mykeystore.p12
controller.tls.truststore.path=./mytruststore.p12
controller.access.protocols.external.tls.truststore.path=./mycustomtruststore.p12

keystore.path and truststore.path can now point to URLs instead of local files only

Documentation

@apucher apucher added the release-notes Referenced by PRs that need attention when compiling the next release notes label Jan 28, 2022
@apucher apucher requested a review from xiangfu0 January 28, 2022 04:34
@apucher apucher force-pushed the listener-tls-customization branch from 3633704 to 88f040a Compare January 28, 2022 23:20
@apucher apucher marked this pull request as ready for review January 28, 2022 23:22
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2022

Codecov Report

Merging #8082 (29e4860) into master (dd73ee7) will increase coverage by 0.19%.
The diff coverage is 72.36%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8082      +/-   ##
============================================
+ Coverage     71.17%   71.37%   +0.19%     
+ Complexity     4261     4197      -64     
============================================
  Files          1607     1607              
  Lines         83419    83464      +45     
  Branches      12460    12461       +1     
============================================
+ Hits          59377    59570     +193     
+ Misses        19995    19838     -157     
- Partials       4047     4056       +9     
Flag Coverage Δ
integration1 29.18% <43.42%> (+0.22%) ⬆️
integration2 27.65% <72.36%> (+<0.01%) ⬆️
unittests1 67.81% <6.57%> (-0.05%) ⬇️
unittests2 14.21% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
...a/org/apache/pinot/spi/env/PinotConfiguration.java 92.85% <0.00%> (-2.27%) ⬇️
...org/apache/pinot/core/util/ListenerConfigUtil.java 77.00% <48.27%> (-10.81%) ⬇️
...main/java/org/apache/pinot/core/util/TlsUtils.java 75.29% <86.66%> (-1.74%) ⬇️
...ava/org/apache/pinot/core/transport/TlsConfig.java 97.50% <100.00%> (+1.34%) ⬆️
...e/impl/forward/FixedByteMVMutableForwardIndex.java 90.90% <0.00%> (-3.04%) ⬇️
...controller/helix/core/minion/PinotTaskManager.java 65.64% <0.00%> (-1.91%) ⬇️
...e/pinot/segment/local/io/util/PinotDataBitSet.java 95.62% <0.00%> (-1.46%) ⬇️
.../helix/core/realtime/SegmentCompletionManager.java 72.00% <0.00%> (-0.82%) ⬇️
...lix/core/realtime/PinotRealtimeSegmentManager.java 81.67% <0.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd73ee7...29e4860. Read the comment docs.

@apucher apucher requested a review from Jackie-Jiang January 29, 2022 11:37
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -167,24 +172,38 @@ private ListenerConfigUtil() {
return listeners;
}

private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String protocol,
private static ListenerConfig buildListenerConfig(PinotConfiguration config, String namespace, String name,
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) why changing the parameter from protocol to name? protocol seems more clear to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the listener config originally required the listener name to be the protocol. now, the protocol can be configured separately from the listener name, i.e. it's possible to have two HTTPS listeners - one internally and the other externally.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea is that you can run multiple listeners for the same protocol.

For example, running 2 https listeners serving different certificates with different keystore configurations. In that case, protocol is duplicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I win.

@apucher apucher merged commit 0009877 into master Jan 31, 2022
@apucher apucher deleted the listener-tls-customization branch January 31, 2022 21:50
apucher added a commit that referenced this pull request Feb 2, 2022
…#8106)

This is a fix of a config validation error when using the new TLS listerner specs intoduced in #8082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants