[fix][broker] REST Client Producer fails with TLS only

[maanders-tibco]

Fixes #20536

Motivation

When disabling HTTP ports in the Pulsar broker, the REST Client Producer fails to produce messages. For reproduction steps, please reference issue details.

Modifications

Change the following lines:

            LookupResult result = optionalResult.get();
            if (result.getLookupData().getHttpUrl().equals(pulsar().getWebServiceAddress())) {
                // Current broker owns the topic, add to owning topic.

To:

            LookupResult result = optionalResult.get();
            if (result.getLookupData().getHttpUrl().equals(pulsar().getWebServiceAddress())
                    || result.getLookupData().getHttpUrlTls().equals(pulsar().getWebServiceAddressTls())) {
                // Current broker owns the topic, add to owning topic.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage. (outside of the reproduction tests described in the #20536)

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: https://github.com/maanders-tibco/pulsar

[tisonkun]

Thank you!

[tisonkun]

I'm thinking of adding the reproduce to TopicsTest but the framework can be a bit complex so I can try to help here.

EDIT - No. I cannot prepare a new test with TLS correctly set up.

[michaeljmarshall]

Thanks for providing all of the context @maanders-tibco. Just one question about testing the case you discovered.

[michaeljmarshall]

Are you able to add a test to cover this behavior?

[tisonkun]

@michaeljmarshall The issue I encounter when trying to help with adding a test case is that it's not clear how to prepare a correct testenv setup for TLS REST client.

I try to copy org.apache.pulsar.broker.admin.TopicsTest#testProduceToPartitionedTopic and change the init logic as:

    @Override
    protected void doInitConf() throws Exception {
        super.doInitConf();
        this.conf.setBrokerServicePort(Optional.empty());
        this.conf.setWebServicePort(Optional.empty());
    }

But then the test case failed with SSL HANDSHAKE error or so.

Given that @maanders-tibco is a first-time contribution I wonder if you can share some backgrounds here for test cases we can refer to. Digging into the heavily mocked test suite can be quite disappointing.

[michaeljmarshall]

Sure that makes sense. I'll take a look at the code a bit closer to make sure I understand the change and then we can merge it.

[codecov-commenter]

Codecov Report

Merging #20535 (c4936bd) into master (d7186a6) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##             master   #20535   +/-   ##
=========================================
  Coverage     72.93%   72.94%           
+ Complexity    31930    31911   -19     
=========================================
  Files          1867     1867           
  Lines        138555   138555           
  Branches      15218    15219    +1     
=========================================
+ Hits         101059   101066    +7     
+ Misses        29466    29456   -10     
- Partials       8030     8033    +3     

Flag	Coverage Δ
inttests	24.14% <33.33%> (-0.10%)	⬇️
systests	25.00% <33.33%> (+0.10%)	⬆️
unittests	72.21% <66.66%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java	80.64% <ø> (+0.09%)	⬆️
...java/org/apache/pulsar/broker/rest/TopicsBase.java	58.31% <50.00%> (-0.14%)	⬇️
...sar/broker/service/persistent/PersistentTopic.java	79.66% <100.00%> (+0.26%)	⬆️

... and 67 files with indirect coverage changes

[michaeljmarshall]

After reviewing this closer, I feel comfortable with merging this solution without additional test coverage. Thanks for your contribution @maanders-tibco!

[michaeljmarshall]

This is now cherry picked to all active branches.

[michaeljmarshall]

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label