[PIP 95][Issue 12040][web] Topic lookup with listener header #12072

EronWright · 2021-09-16T23:14:04Z

Master Issue: #12040

Motivation

This PR introduces an optional HTTP header X-Pulsar-ListenerName to the TopicLookup operation of the Pulsar REST API. The header supplies the listener name to use when the broker has multiple advertised listeners. Today the TopicLookup operation accepts a query parameter listenerName for that same purpose.

The motivation for adding a header-based alternative is to improve support smart listener selection via HTTP gateways or proxies that are capable of rewriting headers (see: Istio VirtualService).

See #12040 for more background.

Modifications

Modify TopicLookup operation (V1 + V2) to use a header (query string takes precedence).

Verifying this change

Make sure that the change passes the CI checks.

The TopicLookup operation doesn't seem to have any unit tests.

Tested using ad-hoc queries, for example against a cluster that has a gateway listener.

$ curl http://octopus.home:8080/lookup/v2/topic/persistent/public/functions/coordinate -H 'X-Pulsar-ListenerName: gateway'
{"brokerUrl":"pulsar://gateway.home:6650","brokerUrlTls":"pulsar+ssl://gateway.home:6651","httpUrl":"http://octopus.home:8080","nativeUrl":"pulsar://gateway.home:6650","brokerUrlSsl":""}⏎

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

The public API: yes - the REST API has a new optional header parameter.

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc

(javadocs)

wangjialing218 · 2021-09-17T09:48:24Z

I think it's not enough to only add the listener head to lookupTopicAsync.
For web request, there is some admin operation which may return a HTTP REDIRECT response with the http url address of the broker own the target topic. For example send a create subscription request to a broker which is not the owner of topic.
In this case, REDIRECT response using the internal webSeviceUrl and the client which send the HTTP requst may not able to connect to the REDIRECT address from outside.
Current there is only brokerSeviceUrl in advertised address, To support advertised listener for HTTP, we need add webSeviceUrl to advertised address first.

EronWright · 2021-09-17T15:52:03Z

@wangjialing218 I agree, the advertised listener configuration does not apply to web service endpoints, it applies only to the Pulsar protocol endpoints. This PR is limited in scope. Let's discuss on the dev mailing list about whether #12040 should include this aspect.

codelipenghui

LGTM, could you please also help add a test for the new change?

hangc0276

Look good to me. Would you please add test for listenerName is empty and listenerNameHeader not null ?

EronWright · 2021-09-20T18:30:28Z

@codelipenghui since this is a part of PIP-95 feature work, should probably target 2.9 not 2.8.2. Some of the other PRs are just bug fixes that could be in 2.8.2 in my opinion, but not this one.

- add tests

eolivelli

LGTM

eolivelli · 2021-10-08T08:28:41Z

the failure is about a test related to this PR

INFO]
Error: Failures:
Error: org.apache.pulsar.broker.lookup.http.v2.TopicLookupTest.testListenerName(org.apache.pulsar.broker.lookup.http.v2.TopicLookupTest)
[INFO] Run 1: PASS
Error: Run 2: TopicLookupTest.testListenerName:53->JerseyTest.target:593->JerseyTest.target:579 » NullPointer
[INFO]
[INFO]

- ensure that setup/teardown runs when `-Dgroup=broker`

- add logging statements

- use setup-per-class not setup-per-method

EronWright · 2021-10-12T20:46:54Z

This is ready to merge. The test failure seems unrelated:

Error:  org.apache.pulsar.broker.admin.TopicPoliciesTest.testDisableSubscribeRate(org.apache.pulsar.broker.admin.TopicPoliciesTest)

addisonj · 2021-10-12T22:09:05Z

/pulsarbot run-failure-checks

…listener

EronWright · 2021-10-13T22:30:31Z

/pulsarbot run-failure-checks

* up/master: (37 commits) re-enabling integration tests for Sinks (apache#12307) [PIP 95][Issue 12040][web] Topic lookup with listener header (apache#12072) Fix the master CI broken with update dispatch rate block issue (apache#12360) Fix message being ignored when the non-persistent topic reader reconnect. (apache#12348) Fix log format. (apache#12346) [website][upgrade]feat: docs migration - version-2.7.2 Concepts and Architecture (apache#12354) [website][upgrade] feat: full docs migration for version 2.8.0 (apache#12359) [website][upgrade]feat: dynamic replace version info before build (apache#12337) Fix flaky tests: ElasticSearchClientTests (apache#12347) Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete (apache#12113) fix-npe-ZkBookieRackAffinityMapping (apache#11947) [pulsar-admin] Allow setting --forward-source-message-property to false when updating a pulsar function (apache#12128) [website][upgrade]feat: docs migration - Development (apache#12320) Update delete inactive topic configuration documentation (apache#12350) [PIP 95][Issue 12040][broker] Multiple bind addresses for Pulsar protocol (apache#12056) Added Debezium Source for MS SQL Server (apache#12256) Fix: flaky oracle tests (apache#12306) [C++] Use URL encoded content type for OAuth 2.0 authentication (apache#12341) [C++] Handle OAuth 2.0 exceptional cases gracefully (apache#12335) feat(cli): add restart command to pulsar-daemon (apache#12279) ... # Conflicts: # site2/website-next/docusaurus.config.js # site2/website-next/versioned_sidebars/version-2.7.2-sidebars.json # site2/website-next/versions.json

…12072) Master Issue: apache#12040 ### Motivation This PR introduces an optional HTTP header `X-Pulsar-ListenerName` to the `TopicLookup` operation of the Pulsar REST API. The header supplies the listener name to use when the broker has multiple advertised listeners. Today the `TopicLookup` operation accepts a query parameter `listenerName` for that same purpose. The motivation for adding a header-based alternative is to improve support smart listener selection via HTTP gateways or proxies that are capable of rewriting headers (see: [Istio VirtualService](https://istio.io/latest/docs/reference/config/networking/virtual-service/#Headers)). See apache#12040 for more background. ### Modifications - Modify `TopicLookup` operation (V1 + V2) to use a header (query string takes precedence).

EronWright added 2 commits September 16, 2021 15:16

[Issue 12040][web] Topic Lookup - Listener Header

5458ec0

[Issue 12040][web] Topic Lookup - Listener Header

5cefc53

codelipenghui reviewed Sep 18, 2021

View reviewed changes

codelipenghui requested review from 315157973, hangc0276, merlimat and sijie September 18, 2021 13:58

codelipenghui assigned EronWright Sep 18, 2021

codelipenghui added this to the 2.9.0 milestone Sep 18, 2021

codelipenghui added the release/2.8.2 label Sep 18, 2021

hangc0276 reviewed Sep 20, 2021

View reviewed changes

eolivelli changed the title ~~[Issue 12040][web] Topic lookup with listener header~~ [PIP 95][Issue 12040][web] Topic lookup with listener header Sep 20, 2021

[Issue 12040][web] Topic Lookup - Listener Header

1af5dd7

- add tests

eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021

codelipenghui removed the release/2.8.2 label Oct 8, 2021

eolivelli approved these changes Oct 8, 2021

View reviewed changes

EronWright added 4 commits October 11, 2021 19:28

[Issue 12040][web] Topic Lookup - Listener Header

43b4fe3

- ensure that setup/teardown runs when `-Dgroup=broker`

[Issue 12040][web] Topic Lookup - Listener Header

5d4d447

- ensure that setup/teardown runs when `-Dgroup=broker`

[Issue 12040][web] Topic Lookup - Listener Header

debcf71

- add logging statements

[Issue 12040][web] Topic Lookup - Listener Header

93a84b3

- use setup-per-class not setup-per-method

Merge remote-tracking branch 'origin/master' into ewright/pip95-admin…

c8e0ed9

…listener

codelipenghui removed this from the 2.10.0 milestone Oct 14, 2021

codelipenghui added this to the 2.9.0 milestone Oct 14, 2021

codelipenghui approved these changes Oct 14, 2021

View reviewed changes

codelipenghui merged commit c792ef8 into apache:master Oct 14, 2021

codelipenghui deleted the ewright/pip95-adminlistener branch October 14, 2021 23:29

lhotari mentioned this pull request Feb 13, 2024

[PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols #22039

Open

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PIP 95][Issue 12040][web] Topic lookup with listener header #12072

[PIP 95][Issue 12040][web] Topic lookup with listener header #12072

EronWright commented Sep 16, 2021 •

edited

Loading

wangjialing218 commented Sep 17, 2021

EronWright commented Sep 17, 2021

codelipenghui left a comment

hangc0276 left a comment

EronWright commented Sep 20, 2021 •

edited

Loading

eolivelli left a comment

eolivelli commented Oct 8, 2021

EronWright commented Oct 12, 2021

addisonj commented Oct 12, 2021

EronWright commented Oct 13, 2021

[PIP 95][Issue 12040][web] Topic lookup with listener header #12072

[PIP 95][Issue 12040][web] Topic lookup with listener header #12072

Conversation

EronWright commented Sep 16, 2021 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

wangjialing218 commented Sep 17, 2021

EronWright commented Sep 17, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

hangc0276 left a comment

Choose a reason for hiding this comment

EronWright commented Sep 20, 2021 • edited Loading

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Oct 8, 2021

EronWright commented Oct 12, 2021

addisonj commented Oct 12, 2021

EronWright commented Oct 13, 2021

EronWright commented Sep 16, 2021 •

edited

Loading

EronWright commented Sep 20, 2021 •

edited

Loading