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

[KannarFr]

Search before asking

• I searched in the issues and found nothing similar.

Read release policy

• I understand that unsupported versions don't get bug fixes. I will attempt to reproduce the issue on a supported version of Pulsar client and Pulsar broker.

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?

• I'm willing to submit a PR!

[nodece]

Could you provide your TLS config?

It looks like the tlsCertificateFilePath is missing.

[KannarFr]

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]

@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]

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]

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

[Apurva007]

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

[lhotari]

@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]

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.