search

apache / pulsar

Apache Pulsar - distributed pub-sub messaging system
https://pulsar.apache.org/
Apache License 2.0
14.28k stars 3.59k forks source link

[Bug] SSL initialization errors when having TLS ports without TLS configs #23392

Open KannarFr opened 1 month ago

KannarFr commented 1 month ago

Search before asking

Read release policy

Version

4.0.0-preview.1

Minimal reproduce step

Configure a broker with TLS deactivated but TLS port defined.

What did you expect to see?

Nothing wrong (as in the previous version).

What did you see instead?

https://gist.githubusercontent.com/KannarFr/fa521bf61e92d8801a08d7505e9c9b2c/raw/0690469c7c05d93a91da92f3f4d7e3620c600157/gistfile1.txt

Anything else?

Set empty in TLS ports fixes the issue.

Probably related to https://github.com/apache/pulsar/blob/master/pip/pip-337.md.

Are you willing to submit a PR?

nodece commented 1 month ago

Could you provide your TLS config?

It looks like the tlsCertificateFilePath is missing.

KannarFr commented 1 month ago

Could you provide your TLS config?

It looks like the tlsCertificateFilePath is missing.

Hi,

It is and it was missing, it just breaks current configurations with port activated but not other configuration keys.

Apurva007 commented 1 month ago

@KannarFr Pulsar considers SSL enabled if the TLS ports are specified in the configuration. Prior to PIP-337, it would not load the SSL Context till the first incoming connection on the TLS port. This could lead to runtime errors and would mask the misconfiguration. Hence, the change was made to initialize the SSL Context at startup itself to fail fast in case of SSL mis-configuration issues.

nodece commented 1 month ago

PIP-337 improves the TLS feature, which can help you find the TLS config's error.

It is and it was missing, it just breaks current configurations with port activated but not other configuration keys.

tlsCertificateFilePath is required in the broker server.

KannarFr commented 1 month ago

@Apurva007 yup, this is my point, a breaking change. I have no problem with it, but it should be highlighted.

Apurva007 commented 1 month ago

@KannarFr Makes sense. @lhotari / @nodece is there a way we could highlight this via documentation or release notes?

lhotari commented 1 month ago

@KannarFr Makes sense. @lhotari / @nodece is there a way we could highlight this via documentation or release notes?

@Apurva007 We don't currently have a proper process for release notes. In the meantime, you can add the content as a comment to this issue or reference for example a GitHub Gist url with the content. During the release of 4.0.0, I'll be looking closely to all issues labeled as release/blocker and ensure that I handle them. As an improvement to the release notes, I've made a proposal a long time ago which would address this. While creating PRs, there should be a release notes staging directory where each PR that contributes to release notes would add a markdown file. During the release, these release notes files could be easily merged into the release notes. After 4.0.0, I'll make a PIP for this that we finally fix the problem we have with release notes and editorial content for it. The current PR naming helps with the changelog, but there's no way to handle the content management of written content for the release notes.

nodece commented 1 month ago

This is not a breaking change. In the old version, your broker starts fine, but the TLS port doesn't work.

In the new version, your broker starts failed, because you don't setup tlsCertificateFilePath in the broker.conf.