You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Manmeet Kaur <me...@gmail.com> on 2024/02/07 18:30:24 UTC
[DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
https://github.com/apache/pulsar/pull/22039
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Girish Sharma <sc...@gmail.com>.
Lari, I think there is a big miss. This issue is not _just_ for admin REST
API calls. It exists for every produce/consume client as well as in most
cases, the lookup service being used is the HTTPLookupService leading to
the same issue as you are mentioning for admin API calls. I've left a
comment for the same here -
https://github.com/apache/pulsar/pull/22039#discussion_r1487332735
On Tue, Feb 13, 2024 at 12:53 PM Lari Hotari <lh...@apache.org> wrote:
> On 2024/02/13 06:46:29 Girish Sharma wrote:
> > Personally, while this may be a much cleaner approach or may be not solve
> > the core issue at all, it is not what we are trying to achieve with our
> > PIP, which is basically only a PIP due to configuration changes involved,
> > but actually is a bug fix for a *bug we are facing in production right
> now.
> > *The same bug is also highlighted by you via the comment you have linked
> in
> > your [3] link
> > There are much more things to consider for the multiple bind address
> > approach and it deserves its own PIP. Specifically the comment on GH that
> > I've made showcasing a use case -
> > https://github.com/apache/pulsar/pull/22039#discussion_r1486400014
>
> The multiple bind address solution is already implemented for the Pulsar
> binary protocol as it is defined in PIP-95. What we are dealing with a gap
> in the solution, mainly about Pulsar Admin API http/https redirects.
> I replied in
> https://github.com/apache/pulsar/pull/22039#discussion_r1487282934 how
> this could possibly be achieved without too many changes.
> That's not a complete design since there would have to be a way to set the
> header value and documentation about how this should be achieved. One
> possibility is that there's a proxy server for the external address which
> sets this header value. (PIP-95 points into that direction)
>
> > Let's treat this PIP as a bug fix only. If needed, we can skip the PIP
> and
> > directly send a bug fix PR if that clears things here.
>
> I'm not sure if that's optimal. A PIP is usually issued when a public
> interface is changed. I agree that this is addressing gaps left in PIP-61 &
> PIP-95 and could be considered as an extension. However it's easier to add
> a new PIP than start extending old PIPs. The benefit of the PIP process is
> that it helps getting consensus about the design before someone puts a lot
> of effort in implementing something that would not be accepted.
> Since we don't have consensus, I think it's better to continue the
> discussion and also meet at the Pulsar community meeting to discuss this
> further. Hopefully others from the community also participate.
>
> > I disagree here. We have clearly identified that there is a bug in the
> > current code. We are trying to do a bug fix here. The goal is not to deep
>
> I'm not sure if it's a bug. The use case isn't covered by PIP-61/PIP-95.
> That is a gap in the design to me, not an implementation bug. However it
> doesn't really matter in the end what we call it. The end-to-end
> functionality isn't usable at the moment for all Pulsar Admin API calls.
>
> > dive into a much bigger design problem as we want to hotfix this ASAP in
> > our system, but also want an alignment with the community so as to not
> > maintain this patch locally, internally, for every version we upgrade to.
>
> Yes, that is a sensible goal. I'm pretty sure that we can achieve a
> solution that addresses the main gap in PIP-61&PIP-95 which is about the
> Pulsar Admin API redirects. It is also notable that there are gaps in the
> documentation of the advertised listeners in Pulsar [1]. The documentation
> also needs some more love. Contributions are more than welcome!
>
> -Lari
>
> 1 -
> https://pulsar.apache.org/docs/next/concepts-multiple-advertised-listeners/#advertised-listeners
>
--
Girish Sharma
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Lari Hotari <lh...@apache.org>.
On 2024/02/13 06:46:29 Girish Sharma wrote:
> Personally, while this may be a much cleaner approach or may be not solve
> the core issue at all, it is not what we are trying to achieve with our
> PIP, which is basically only a PIP due to configuration changes involved,
> but actually is a bug fix for a *bug we are facing in production right now.
> *The same bug is also highlighted by you via the comment you have linked in
> your [3] link
> There are much more things to consider for the multiple bind address
> approach and it deserves its own PIP. Specifically the comment on GH that
> I've made showcasing a use case -
> https://github.com/apache/pulsar/pull/22039#discussion_r1486400014
The multiple bind address solution is already implemented for the Pulsar binary protocol as it is defined in PIP-95. What we are dealing with a gap in the solution, mainly about Pulsar Admin API http/https redirects.
I replied in https://github.com/apache/pulsar/pull/22039#discussion_r1487282934 how this could possibly be achieved without too many changes.
That's not a complete design since there would have to be a way to set the header value and documentation about how this should be achieved. One possibility is that there's a proxy server for the external address which sets this header value. (PIP-95 points into that direction)
> Let's treat this PIP as a bug fix only. If needed, we can skip the PIP and
> directly send a bug fix PR if that clears things here.
I'm not sure if that's optimal. A PIP is usually issued when a public interface is changed. I agree that this is addressing gaps left in PIP-61 & PIP-95 and could be considered as an extension. However it's easier to add a new PIP than start extending old PIPs. The benefit of the PIP process is that it helps getting consensus about the design before someone puts a lot of effort in implementing something that would not be accepted.
Since we don't have consensus, I think it's better to continue the discussion and also meet at the Pulsar community meeting to discuss this further. Hopefully others from the community also participate.
> I disagree here. We have clearly identified that there is a bug in the
> current code. We are trying to do a bug fix here. The goal is not to deep
I'm not sure if it's a bug. The use case isn't covered by PIP-61/PIP-95. That is a gap in the design to me, not an implementation bug. However it doesn't really matter in the end what we call it. The end-to-end functionality isn't usable at the moment for all Pulsar Admin API calls.
> dive into a much bigger design problem as we want to hotfix this ASAP in
> our system, but also want an alignment with the community so as to not
> maintain this patch locally, internally, for every version we upgrade to.
Yes, that is a sensible goal. I'm pretty sure that we can achieve a solution that addresses the main gap in PIP-61&PIP-95 which is about the Pulsar Admin API redirects. It is also notable that there are gaps in the documentation of the advertised listeners in Pulsar [1]. The documentation also needs some more love. Contributions are more than welcome!
-Lari
1 - https://pulsar.apache.org/docs/next/concepts-multiple-advertised-listeners/#advertised-listeners
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Girish Sharma <sc...@gmail.com>.
Hello Lari,
On Tue, Feb 13, 2024 at 12:04 PM Lari Hotari <lh...@apache.org> wrote:
> Thanks for linking to the PIP-95 implementation PR #12056 [1]. I wasn't
> aware that it has been implemented.
>
> That PR is what I thought that was missing. It is the implementation to
> achieve what PIP-61 described as "only return the corresponding service
> URL" and the PIP-95 design of "use a unique bind address for each
> listener". It is possible that there are gaps, but I think that we should
> cover the possible gaps.
>
> > Maybe that's not entirely true. You can configure 100s of listeners for
> all
> > schemes/protocols, but the code only returns the internal or requested or
> > first address for all 4 schemes (pulsar, pulsar+ssl, http, https, and one
> > service url, which i am not sure why it is needed, maybe for backward
> > compatibility). So while, it's not exactly approach 2, it's also not
> purely
>
> I think that this is expected and what PIP-61 and PIP-95 describe.
> The main problem of PIP-61 and PIP-95 is that there isn't documentation of
> how to properly configure the solution. The important piece of
> "bindAddresses" implemented in PIP-95 was missing from PIP-61 in the first
> place and it's likely that PIP-95 isn't fully completed.
>
> The PR 12056 description is perhaps the best documentation of the feature
> and provides an example of how to use the configuration:
> ************
> bindAddresses=external:pulsar://0.0.0.0:6652,external:pulsar+ssl://
> 0.0.0.0:6653
> bindAddress=0.0.0.0
> brokerServicePort=6650
> brokerServicePortTls=6651
>
> advertisedListeners=cluster:pulsar://broker-1.local:6650,cluster:pulsar+ssl://broker-1.local:6651,external:pulsar://
> broker-1.example.dev:6652,external:pulsar+ssl://broker-1.example.dev:6653
> internalListenerName=cluster
> ************
>
>
Personally, while this may be a much cleaner approach or may be not solve
the core issue at all, it is not what we are trying to achieve with our
PIP, which is basically only a PIP due to configuration changes involved,
but actually is a bug fix for a *bug we are facing in production right now.
*The same bug is also highlighted by you via the comment you have linked in
your [3] link
There are much more things to consider for the multiple bind address
approach and it deserves its own PIP. Specifically the comment on GH that
I've made showcasing a use case -
https://github.com/apache/pulsar/pull/22039#discussion_r1486400014
> As it can be seen, the "external" listener is bound to different ports
> than the default "cluster" listener (listener for the brokerServicePort and
> brokerServicePortlTls is specified with internalListenerName).
>
> The clear gaps in the PIP-61 and PIP-95 solution are in the admin API.
> There are multiple APIs where a redirect is sent back to the client. The PR
> 12072 [2] covers the topic lookup over the admin API, but doesn't cover all
> other cases where the redirect happens. This is already pointed out in a
> comment [3] on that PR.
>
>
Let's treat this PIP as a bug fix only. If needed, we can skip the PIP and
directly send a bug fix PR if that clears things here.
> I think that PIP-338 should focus on covering the gaps and providing a
> design that is aligned with the current PIP-61 & PIP-95 implementation.
>
>
I disagree here. We have clearly identified that there is a bug in the
current code. We are trying to do a bug fix here. The goal is not to deep
dive into a much bigger design problem as we want to hotfix this ASAP in
our system, but also want an alignment with the community so as to not
maintain this patch locally, internally, for every version we upgrade to.
> -Lari
>
> 1 - https://github.com/apache/pulsar/pull/12056
> 2 - https://github.com/apache/pulsar/pull/12072
> 3 - https://github.com/apache/pulsar/pull/12072#issuecomment-921663472
>
>
--
Girish Sharma
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Lari Hotari <lh...@apache.org>.
Thanks for linking to the PIP-95 implementation PR #12056 [1]. I wasn't aware that it has been implemented.
That PR is what I thought that was missing. It is the implementation to achieve what PIP-61 described as "only return the corresponding service URL" and the PIP-95 design of "use a unique bind address for each listener". It is possible that there are gaps, but I think that we should cover the possible gaps.
> Maybe that's not entirely true. You can configure 100s of listeners for all
> schemes/protocols, but the code only returns the internal or requested or
> first address for all 4 schemes (pulsar, pulsar+ssl, http, https, and one
> service url, which i am not sure why it is needed, maybe for backward
> compatibility). So while, it's not exactly approach 2, it's also not purely
I think that this is expected and what PIP-61 and PIP-95 describe.
The main problem of PIP-61 and PIP-95 is that there isn't documentation of how to properly configure the solution. The important piece of "bindAddresses" implemented in PIP-95 was missing from PIP-61 in the first place and it's likely that PIP-95 isn't fully completed.
The PR 12056 description is perhaps the best documentation of the feature and provides an example of how to use the configuration:
************
bindAddresses=external:pulsar://0.0.0.0:6652,external:pulsar+ssl://0.0.0.0:6653
bindAddress=0.0.0.0
brokerServicePort=6650
brokerServicePortTls=6651
advertisedListeners=cluster:pulsar://broker-1.local:6650,cluster:pulsar+ssl://broker-1.local:6651,external:pulsar://broker-1.example.dev:6652,external:pulsar+ssl://broker-1.example.dev:6653
internalListenerName=cluster
************
As it can be seen, the "external" listener is bound to different ports than the default "cluster" listener (listener for the brokerServicePort and brokerServicePortlTls is specified with internalListenerName).
The clear gaps in the PIP-61 and PIP-95 solution are in the admin API. There are multiple APIs where a redirect is sent back to the client. The PR 12072 [2] covers the topic lookup over the admin API, but doesn't cover all other cases where the redirect happens. This is already pointed out in a comment [3] on that PR.
I think that PIP-338 should focus on covering the gaps and providing a design that is aligned with the current PIP-61 & PIP-95 implementation.
-Lari
1 - https://github.com/apache/pulsar/pull/12056
2 - https://github.com/apache/pulsar/pull/12072
3 - https://github.com/apache/pulsar/pull/12072#issuecomment-921663472
On 2024/02/12 18:30:01 Girish Sharma wrote:
> Reply inline, and also replied to the GH comment.
>
> On Mon, Feb 12, 2024 at 9:37 PM Lari Hotari <lh...@apache.org> wrote:
>
> > The confusing detail is that in PIP-61 [1], the alternative that has been
> > implemented in the Pulsar code base has been marked as the rejected
> > alternative ("Return all advertised listeners(rejected)"). The preferred
> > and proposed alternative "Only return the corresponding service URL" was
> > never implemented.
> >
> >
> Maybe that's not entirely true. You can configure 100s of listeners for all
> schemes/protocols, but the code only returns the internal or requested or
> first address for all 4 schemes (pulsar, pulsar+ssl, http, https, and one
> service url, which i am not sure why it is needed, maybe for backward
> compatibility). So while, it's not exactly approach 2, it's also not purely
> approach 1. I can speculate and assume that in approach 1, the author meant
> "one of each protocol" and that's actually what's implemented, but its not
> clearly mentioned in the PIP. It would be great if we can get some input
> from folks involved in PIP-61 and PIP-95 . Also, the wiki says PIP-95 [0]
> is something else, while the PRs using PIP-95 [1] in commits refer to
> something else [2] !
>
>
> [0] -
> https://github.com/apache/pulsar/wiki/PIP-95:-Transaction-coordinator-loading-mechanism
> [1] - https://github.com/apache/pulsar/pull/12056
> [2] - https://github.com/apache/pulsar/issues/12040
>
> Regards
>
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Girish Sharma <sc...@gmail.com>.
Reply inline, and also replied to the GH comment.
On Mon, Feb 12, 2024 at 9:37 PM Lari Hotari <lh...@apache.org> wrote:
> The confusing detail is that in PIP-61 [1], the alternative that has been
> implemented in the Pulsar code base has been marked as the rejected
> alternative ("Return all advertised listeners(rejected)"). The preferred
> and proposed alternative "Only return the corresponding service URL" was
> never implemented.
>
>
Maybe that's not entirely true. You can configure 100s of listeners for all
schemes/protocols, but the code only returns the internal or requested or
first address for all 4 schemes (pulsar, pulsar+ssl, http, https, and one
service url, which i am not sure why it is needed, maybe for backward
compatibility). So while, it's not exactly approach 2, it's also not purely
approach 1. I can speculate and assume that in approach 1, the author meant
"one of each protocol" and that's actually what's implemented, but its not
clearly mentioned in the PIP. It would be great if we can get some input
from folks involved in PIP-61 and PIP-95 . Also, the wiki says PIP-95 [0]
is something else, while the PRs using PIP-95 [1] in commits refer to
something else [2] !
[0] -
https://github.com/apache/pulsar/wiki/PIP-95:-Transaction-coordinator-loading-mechanism
[1] - https://github.com/apache/pulsar/pull/12056
[2] - https://github.com/apache/pulsar/issues/12040
Regards
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Lari Hotari <lh...@apache.org>.
The confusing detail is that in PIP-61 [1], the alternative that has been implemented in the Pulsar code base has been marked as the rejected alternative ("Return all advertised listeners(rejected)"). The preferred and proposed alternative "Only return the corresponding service URL" was never implemented.
We need to decide how to address this. PIP-338 is where this decision could be made.
-Lari
1 - https://github.com/apache/pulsar/wiki/PIP-61%3A-Advertised-multiple-addresses
On 2024/02/12 15:59:00 Lari Hotari wrote:
> Thanks for starting the PIP.
> I added some thoughts in this comment:
> https://github.com/apache/pulsar/pull/22039/files#r1486372582
>
> Expanding my comment in the PR here:
>
> It would be interesting to learn why advertised listeners weren't implemented as they had been designed. We wouldn't have this problem if the implementation would match the design.
>
> In PIP-61 [1], the selected approach is "only return the corresponding service URL":
>
> > In the approach, when the client sends a lookup request to the broker, the broker only
> > returns one service URL that with the same listener name with the client uses. The purpose
> > of this approach is keeping client-side simple and not expose extra listeners to the client,
> > this is better for security.
>
> This is also referred in PIP-95 [2] "use a unique bind address for each listener":
>
> > The broker shall be configurable to open a server socket on numerous bind addresses, and
> > to associate each socket with a listener name. When a lookup request arrives on a particular
> > socket, the broker shall use the associated listener by default. Note that an explicit listener
> > name in the request parameters would take precedence.
>
> Why doesn't the implementation in Pulsar match PIP-61 and PIP-95 designs?
> Instead of implementing PIP-338, should we consider implementing the original design for addressing the problem?
>
> -Lari
>
> 1 - https://github.com/apache/pulsar/wiki/PIP-61%3A-Advertised-multiple-addresses
> 2 - https://github.com/apache/pulsar/issues/12040
>
>
> On 2024/02/07 18:42:07 Manmeet Kaur wrote:
> > Hello everyone,
> > Proposing a PIP-338 : https://github.com/apache/pulsar/pull/22039
> > ----------------------------------------------------------------------------
> >
> > # PIP-338: Add default lookup listener and fix inconsistency with
> > listener's usage between different protocols
> >
> > # Motivation
> > ## Issue in existing Code flow
> >
> > Currently, the listener mentioned as `internalListenerName` in broker
> > config helps in deciding the listener from the list of
> > `advertisedListeners` to specify the service URL, is being used for
> > broker-to-broker communication but that is exposed to the client in case of
> > lookup results or redirects as well.
> >
> > Even if the client sends a `listenerName` (PIP-91/PIP-95) corresponding to
> > the http protocol broker’s addresses, it is only used in the `pulsar` and
> > `pulsar+ssl` protocols and is not consistent for the other protocols.
> >
> > See [code](
> > https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L482
> > )
> >
> > In the existing flow, the listenerName is not acknowledged in case of http
> > protocol.
> > Here, it is trying to obtain the broker service url. If the listener has
> > service url/tls (pulsar protocols) then they are used in the response, but
> > the HTTP urls are from the internal listener only (when configured). Later
> > in the call chain, the http urls are used for redirect location - which may
> > be non resolvable for the client.
> >
> > See [code](
> > https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L103
> > )
> >
> > ## Need to have lookupListener
> > The Pulsar broker has the concept of advertised listeners representing
> > broker endpoints that are discoverable by, and accessible to, Pulsar
> > clients. In addition, one listener may be designated as the internal
> > listener, to be used for broker-to-broker communication. Listeners may be
> > understood as named routes to the broker. If a listener name is not
> > specified, the response contains the endpoint for the internal listener, or
> > the first listener for the protocol.
> >
> > But the challenge arises when it considers the same internalListener for
> > lookup requests redirects. This may result in returning an unresolvable
> > broker service address to the clients.
> > It may not be possible to pass the listenerName from the clients, which
> > consequently may lead to cluster downtime.
> >
> > it might not be feasible to have the listerner config at the client side in
> > every tech stack or connector.
> > Also, Currently, there is no option to pass the listener while using the
> > admin APIs. As admin APIs can be called from an external network as well,
> > the use of the internal listener’s broker service URL can lead all admin
> > operations to get affected.
> >
> > Moreover, as per current code provided above, even if the client provides a
> > listenerName/listener header, the redirect urls are taken from the internal
> > listener or the first listener with http protocol which may not be a
> > client-resolvable http address.
> >
> > # Proposed Solution
> > To fix inconsistency with all protocol’s service url, we need to include
> > all broker addresses associated with the listener name while returning the
> > lookupResult, rather than solely the service URL
> >
> > Also, Introduce a broker-level property to determine the appropriate
> > listener for lookup calls if listenerName is missing from the client
> > request. This can help deal with the side effects of not having listerName
> > on the client side when multiple advertisedListeners are being used.
> >
> > Also, this can help to determine the broker service URL that needs to be
> > returned in case of lookup redirects and also for using Pulsar admin API
> > calls from outside the network.
> >
> > This will help in having a more transparent listener identifier for
> > internal and external communications.
> >
> >
> > # Design & Implementation Details
> > ## Approach
> > **1. Fix Code Flow - Inconsistency with listener usage with protocols other
> > than pulsar and pulsar+ssl**
> >
> > Currently, lookup result return the broker service url corresponding to the
> > internal listerner in case of http or https protocol urls. To address the
> > issue in the workflow, it is necessary to include all broker addresses
> > associated with the listener name, rather than solely the service URL.
> > There can be a check if all broker addresses corresponding to the given
> > listener is null, then use the default listener(discussed below)
> >
> > **2. Only return the service URL corresponding to the lookupListener**
> >
> > This approach introduces one new configuration `lookupListenerName` in the
> > broker.conf. The `lookupListenerName` is used to specify which service URL
> > should be returned to the clients from the lookup requests or redirects.
> > `lookupListenerName` should be present in the `advertisedListeners` list.
> > Users can set up the `lookupListenerName`. for example:
> >
> > ```
> > existing configs:
> > advertisedListeners=internal:pulsar://xyz-broker:6660,internal:pulsar+ssl://
> > 192.168.1.11:6651,external:http://192.168.1.11:8080
> > internalListenerName=internal
> >
> > new config required:
> > lookupListenerName=external
> > ```
> >
> > In the approach, when the client sends a lookup request to the broker to
> > get the owner broker without the listnerName, the broker only returns one
> > service URL that is with the same listener name for which the default value
> > of the lookupListenerName is given in the broker configuration. Therefore,
> > we must allow the client to get the corresponding service URL with the same
> > advertised listener name as present in the broker config.
> >
> > This approach's purpose is to keep client-side simple and handle situations
> > when not having listenerName information can lead to breaking scenarios. By
> > having the default value, clients should be able to connect with the broker
> > even if the request lands to the broker who is not the owner and redirects
> > to some other broker.
> >
> >
> > ### Backward Compatibility
> > The above mentioned approach(2) of having a new config as lookupListener is
> > backward compatible as there should not be any impact on the existing users
> > if this configuration would not be set. Users can set this configuration
> > for listener selection for the external communication as per their usecase.
> > The same listener would be picked up for the admin APIs as well if admin
> > calls are coming outside the network.
> >
> > Approach(1), Fixing the code flow for http protocol might be a breaking
> > change for the clients where we are returning the all broker addresses
> > corresponding to the listener rather than just having service url. This can
> > be breaking for the clients who are used to this inconsistency of having
> > internal listener’s service url as lookup request. To deal with this, we
> > can have a flag - `preferHttpClientListenerOverInternalListener` which
> > would indicate whether this change should be enabled for clients or not.
> > By Default, the value of this flag can be set as false for backward
> > compatibility.
> >
> >
> > Regards,
> > Manmeet
> >
> > On Thu, Feb 8, 2024 at 12:00 AM Manmeet Kaur <me...@gmail.com>
> > wrote:
> >
> > > https://github.com/apache/pulsar/pull/22039
> > >
> >
>
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Lari Hotari <lh...@apache.org>.
Thanks for starting the PIP.
I added some thoughts in this comment:
https://github.com/apache/pulsar/pull/22039/files#r1486372582
Expanding my comment in the PR here:
It would be interesting to learn why advertised listeners weren't implemented as they had been designed. We wouldn't have this problem if the implementation would match the design.
In PIP-61 [1], the selected approach is "only return the corresponding service URL":
> In the approach, when the client sends a lookup request to the broker, the broker only
> returns one service URL that with the same listener name with the client uses. The purpose
> of this approach is keeping client-side simple and not expose extra listeners to the client,
> this is better for security.
This is also referred in PIP-95 [2] "use a unique bind address for each listener":
> The broker shall be configurable to open a server socket on numerous bind addresses, and
> to associate each socket with a listener name. When a lookup request arrives on a particular
> socket, the broker shall use the associated listener by default. Note that an explicit listener
> name in the request parameters would take precedence.
Why doesn't the implementation in Pulsar match PIP-61 and PIP-95 designs?
Instead of implementing PIP-338, should we consider implementing the original design for addressing the problem?
-Lari
1 - https://github.com/apache/pulsar/wiki/PIP-61%3A-Advertised-multiple-addresses
2 - https://github.com/apache/pulsar/issues/12040
On 2024/02/07 18:42:07 Manmeet Kaur wrote:
> Hello everyone,
> Proposing a PIP-338 : https://github.com/apache/pulsar/pull/22039
> ----------------------------------------------------------------------------
>
> # PIP-338: Add default lookup listener and fix inconsistency with
> listener's usage between different protocols
>
> # Motivation
> ## Issue in existing Code flow
>
> Currently, the listener mentioned as `internalListenerName` in broker
> config helps in deciding the listener from the list of
> `advertisedListeners` to specify the service URL, is being used for
> broker-to-broker communication but that is exposed to the client in case of
> lookup results or redirects as well.
>
> Even if the client sends a `listenerName` (PIP-91/PIP-95) corresponding to
> the http protocol broker’s addresses, it is only used in the `pulsar` and
> `pulsar+ssl` protocols and is not consistent for the other protocols.
>
> See [code](
> https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L482
> )
>
> In the existing flow, the listenerName is not acknowledged in case of http
> protocol.
> Here, it is trying to obtain the broker service url. If the listener has
> service url/tls (pulsar protocols) then they are used in the response, but
> the HTTP urls are from the internal listener only (when configured). Later
> in the call chain, the http urls are used for redirect location - which may
> be non resolvable for the client.
>
> See [code](
> https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L103
> )
>
> ## Need to have lookupListener
> The Pulsar broker has the concept of advertised listeners representing
> broker endpoints that are discoverable by, and accessible to, Pulsar
> clients. In addition, one listener may be designated as the internal
> listener, to be used for broker-to-broker communication. Listeners may be
> understood as named routes to the broker. If a listener name is not
> specified, the response contains the endpoint for the internal listener, or
> the first listener for the protocol.
>
> But the challenge arises when it considers the same internalListener for
> lookup requests redirects. This may result in returning an unresolvable
> broker service address to the clients.
> It may not be possible to pass the listenerName from the clients, which
> consequently may lead to cluster downtime.
>
> it might not be feasible to have the listerner config at the client side in
> every tech stack or connector.
> Also, Currently, there is no option to pass the listener while using the
> admin APIs. As admin APIs can be called from an external network as well,
> the use of the internal listener’s broker service URL can lead all admin
> operations to get affected.
>
> Moreover, as per current code provided above, even if the client provides a
> listenerName/listener header, the redirect urls are taken from the internal
> listener or the first listener with http protocol which may not be a
> client-resolvable http address.
>
> # Proposed Solution
> To fix inconsistency with all protocol’s service url, we need to include
> all broker addresses associated with the listener name while returning the
> lookupResult, rather than solely the service URL
>
> Also, Introduce a broker-level property to determine the appropriate
> listener for lookup calls if listenerName is missing from the client
> request. This can help deal with the side effects of not having listerName
> on the client side when multiple advertisedListeners are being used.
>
> Also, this can help to determine the broker service URL that needs to be
> returned in case of lookup redirects and also for using Pulsar admin API
> calls from outside the network.
>
> This will help in having a more transparent listener identifier for
> internal and external communications.
>
>
> # Design & Implementation Details
> ## Approach
> **1. Fix Code Flow - Inconsistency with listener usage with protocols other
> than pulsar and pulsar+ssl**
>
> Currently, lookup result return the broker service url corresponding to the
> internal listerner in case of http or https protocol urls. To address the
> issue in the workflow, it is necessary to include all broker addresses
> associated with the listener name, rather than solely the service URL.
> There can be a check if all broker addresses corresponding to the given
> listener is null, then use the default listener(discussed below)
>
> **2. Only return the service URL corresponding to the lookupListener**
>
> This approach introduces one new configuration `lookupListenerName` in the
> broker.conf. The `lookupListenerName` is used to specify which service URL
> should be returned to the clients from the lookup requests or redirects.
> `lookupListenerName` should be present in the `advertisedListeners` list.
> Users can set up the `lookupListenerName`. for example:
>
> ```
> existing configs:
> advertisedListeners=internal:pulsar://xyz-broker:6660,internal:pulsar+ssl://
> 192.168.1.11:6651,external:http://192.168.1.11:8080
> internalListenerName=internal
>
> new config required:
> lookupListenerName=external
> ```
>
> In the approach, when the client sends a lookup request to the broker to
> get the owner broker without the listnerName, the broker only returns one
> service URL that is with the same listener name for which the default value
> of the lookupListenerName is given in the broker configuration. Therefore,
> we must allow the client to get the corresponding service URL with the same
> advertised listener name as present in the broker config.
>
> This approach's purpose is to keep client-side simple and handle situations
> when not having listenerName information can lead to breaking scenarios. By
> having the default value, clients should be able to connect with the broker
> even if the request lands to the broker who is not the owner and redirects
> to some other broker.
>
>
> ### Backward Compatibility
> The above mentioned approach(2) of having a new config as lookupListener is
> backward compatible as there should not be any impact on the existing users
> if this configuration would not be set. Users can set this configuration
> for listener selection for the external communication as per their usecase.
> The same listener would be picked up for the admin APIs as well if admin
> calls are coming outside the network.
>
> Approach(1), Fixing the code flow for http protocol might be a breaking
> change for the clients where we are returning the all broker addresses
> corresponding to the listener rather than just having service url. This can
> be breaking for the clients who are used to this inconsistency of having
> internal listener’s service url as lookup request. To deal with this, we
> can have a flag - `preferHttpClientListenerOverInternalListener` which
> would indicate whether this change should be enabled for clients or not.
> By Default, the value of this flag can be set as false for backward
> compatibility.
>
>
> Regards,
> Manmeet
>
> On Thu, Feb 8, 2024 at 12:00 AM Manmeet Kaur <me...@gmail.com>
> wrote:
>
> > https://github.com/apache/pulsar/pull/22039
> >
>
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Manmeet Kaur <me...@gmail.com>.
Bump
On Thu, Feb 8, 2024 at 12:12 AM Manmeet Kaur <me...@gmail.com>
wrote:
> Hello everyone,
> Proposing a PIP-338 : https://github.com/apache/pulsar/pull/22039
>
> ----------------------------------------------------------------------------
>
> # PIP-338: Add default lookup listener and fix inconsistency with
> listener's usage between different protocols
>
> # Motivation
> ## Issue in existing Code flow
>
> Currently, the listener mentioned as `internalListenerName` in broker
> config helps in deciding the listener from the list of
> `advertisedListeners` to specify the service URL, is being used for
> broker-to-broker communication but that is exposed to the client in case of
> lookup results or redirects as well.
>
> Even if the client sends a `listenerName` (PIP-91/PIP-95) corresponding to
> the http protocol broker’s addresses, it is only used in the `pulsar` and
> `pulsar+ssl` protocols and is not consistent for the other protocols.
>
> See [code](
> https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L482
> )
>
> In the existing flow, the listenerName is not acknowledged in case of http
> protocol.
> Here, it is trying to obtain the broker service url. If the listener has
> service url/tls (pulsar protocols) then they are used in the response, but
> the HTTP urls are from the internal listener only (when configured). Later
> in the call chain, the http urls are used for redirect location - which may
> be non resolvable for the client.
>
> See [code](
> https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L103
> )
>
> ## Need to have lookupListener
> The Pulsar broker has the concept of advertised listeners representing
> broker endpoints that are discoverable by, and accessible to, Pulsar
> clients. In addition, one listener may be designated as the internal
> listener, to be used for broker-to-broker communication. Listeners may be
> understood as named routes to the broker. If a listener name is not
> specified, the response contains the endpoint for the internal listener, or
> the first listener for the protocol.
>
> But the challenge arises when it considers the same internalListener for
> lookup requests redirects. This may result in returning an unresolvable
> broker service address to the clients.
> It may not be possible to pass the listenerName from the clients, which
> consequently may lead to cluster downtime.
>
> it might not be feasible to have the listerner config at the client side
> in every tech stack or connector.
> Also, Currently, there is no option to pass the listener while using the
> admin APIs. As admin APIs can be called from an external network as well,
> the use of the internal listener’s broker service URL can lead all admin
> operations to get affected.
>
> Moreover, as per current code provided above, even if the client provides
> a listenerName/listener header, the redirect urls are taken from the
> internal listener or the first listener with http protocol which may not be
> a client-resolvable http address.
>
> # Proposed Solution
> To fix inconsistency with all protocol’s service url, we need to include
> all broker addresses associated with the listener name while returning the
> lookupResult, rather than solely the service URL
>
> Also, Introduce a broker-level property to determine the appropriate
> listener for lookup calls if listenerName is missing from the client
> request. This can help deal with the side effects of not having listerName
> on the client side when multiple advertisedListeners are being used.
>
> Also, this can help to determine the broker service URL that needs to be
> returned in case of lookup redirects and also for using Pulsar admin API
> calls from outside the network.
>
> This will help in having a more transparent listener identifier for
> internal and external communications.
>
>
> # Design & Implementation Details
> ## Approach
> **1. Fix Code Flow - Inconsistency with listener usage with protocols
> other than pulsar and pulsar+ssl**
>
> Currently, lookup result return the broker service url corresponding to
> the internal listerner in case of http or https protocol urls. To address
> the issue in the workflow, it is necessary to include all broker addresses
> associated with the listener name, rather than solely the service URL.
> There can be a check if all broker addresses corresponding to the given
> listener is null, then use the default listener(discussed below)
>
> **2. Only return the service URL corresponding to the lookupListener**
>
> This approach introduces one new configuration `lookupListenerName` in the
> broker.conf. The `lookupListenerName` is used to specify which service URL
> should be returned to the clients from the lookup requests or redirects.
> `lookupListenerName` should be present in the `advertisedListeners` list.
> Users can set up the `lookupListenerName`. for example:
>
> ```
> existing configs:
>
> advertisedListeners=internal:pulsar://xyz-broker:6660,internal:pulsar+ssl://
> 192.168.1.11:6651,external:http://192.168.1.11:8080
> internalListenerName=internal
>
> new config required:
> lookupListenerName=external
> ```
>
> In the approach, when the client sends a lookup request to the broker to
> get the owner broker without the listnerName, the broker only returns one
> service URL that is with the same listener name for which the default value
> of the lookupListenerName is given in the broker configuration. Therefore,
> we must allow the client to get the corresponding service URL with the same
> advertised listener name as present in the broker config.
>
> This approach's purpose is to keep client-side simple and handle
> situations when not having listenerName information can lead to breaking
> scenarios. By having the default value, clients should be able to connect
> with the broker even if the request lands to the broker who is not the
> owner and redirects to some other broker.
>
>
> ### Backward Compatibility
> The above mentioned approach(2) of having a new config as lookupListener
> is backward compatible as there should not be any impact on the existing
> users if this configuration would not be set. Users can set this
> configuration for listener selection for the external communication as per
> their usecase. The same listener would be picked up for the admin APIs as
> well if admin calls are coming outside the network.
>
> Approach(1), Fixing the code flow for http protocol might be a breaking
> change for the clients where we are returning the all broker addresses
> corresponding to the listener rather than just having service url. This can
> be breaking for the clients who are used to this inconsistency of having
> internal listener’s service url as lookup request. To deal with this, we
> can have a flag - `preferHttpClientListenerOverInternalListener` which
> would indicate whether this change should be enabled for clients or not.
> By Default, the value of this flag can be set as false for backward
> compatibility.
>
>
> Regards,
> Manmeet
>
> On Thu, Feb 8, 2024 at 12:00 AM Manmeet Kaur <me...@gmail.com>
> wrote:
>
>> https://github.com/apache/pulsar/pull/22039
>>
>
Re: [DISCUSS][PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols
Posted by Manmeet Kaur <me...@gmail.com>.
Hello everyone,
Proposing a PIP-338 : https://github.com/apache/pulsar/pull/22039
----------------------------------------------------------------------------
# PIP-338: Add default lookup listener and fix inconsistency with
listener's usage between different protocols
# Motivation
## Issue in existing Code flow
Currently, the listener mentioned as `internalListenerName` in broker
config helps in deciding the listener from the list of
`advertisedListeners` to specify the service URL, is being used for
broker-to-broker communication but that is exposed to the client in case of
lookup results or redirects as well.
Even if the client sends a `listenerName` (PIP-91/PIP-95) corresponding to
the http protocol broker’s addresses, it is only used in the `pulsar` and
`pulsar+ssl` protocols and is not consistent for the other protocols.
See [code](
https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L482
)
In the existing flow, the listenerName is not acknowledged in case of http
protocol.
Here, it is trying to obtain the broker service url. If the listener has
service url/tls (pulsar protocols) then they are used in the response, but
the HTTP urls are from the internal listener only (when configured). Later
in the call chain, the http urls are used for redirect location - which may
be non resolvable for the client.
See [code](
https://github.com/apache/pulsar/blob/a77fb6ed165ac8ad6968558077d80ea3edaf9a7e/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L103
)
## Need to have lookupListener
The Pulsar broker has the concept of advertised listeners representing
broker endpoints that are discoverable by, and accessible to, Pulsar
clients. In addition, one listener may be designated as the internal
listener, to be used for broker-to-broker communication. Listeners may be
understood as named routes to the broker. If a listener name is not
specified, the response contains the endpoint for the internal listener, or
the first listener for the protocol.
But the challenge arises when it considers the same internalListener for
lookup requests redirects. This may result in returning an unresolvable
broker service address to the clients.
It may not be possible to pass the listenerName from the clients, which
consequently may lead to cluster downtime.
it might not be feasible to have the listerner config at the client side in
every tech stack or connector.
Also, Currently, there is no option to pass the listener while using the
admin APIs. As admin APIs can be called from an external network as well,
the use of the internal listener’s broker service URL can lead all admin
operations to get affected.
Moreover, as per current code provided above, even if the client provides a
listenerName/listener header, the redirect urls are taken from the internal
listener or the first listener with http protocol which may not be a
client-resolvable http address.
# Proposed Solution
To fix inconsistency with all protocol’s service url, we need to include
all broker addresses associated with the listener name while returning the
lookupResult, rather than solely the service URL
Also, Introduce a broker-level property to determine the appropriate
listener for lookup calls if listenerName is missing from the client
request. This can help deal with the side effects of not having listerName
on the client side when multiple advertisedListeners are being used.
Also, this can help to determine the broker service URL that needs to be
returned in case of lookup redirects and also for using Pulsar admin API
calls from outside the network.
This will help in having a more transparent listener identifier for
internal and external communications.
# Design & Implementation Details
## Approach
**1. Fix Code Flow - Inconsistency with listener usage with protocols other
than pulsar and pulsar+ssl**
Currently, lookup result return the broker service url corresponding to the
internal listerner in case of http or https protocol urls. To address the
issue in the workflow, it is necessary to include all broker addresses
associated with the listener name, rather than solely the service URL.
There can be a check if all broker addresses corresponding to the given
listener is null, then use the default listener(discussed below)
**2. Only return the service URL corresponding to the lookupListener**
This approach introduces one new configuration `lookupListenerName` in the
broker.conf. The `lookupListenerName` is used to specify which service URL
should be returned to the clients from the lookup requests or redirects.
`lookupListenerName` should be present in the `advertisedListeners` list.
Users can set up the `lookupListenerName`. for example:
```
existing configs:
advertisedListeners=internal:pulsar://xyz-broker:6660,internal:pulsar+ssl://
192.168.1.11:6651,external:http://192.168.1.11:8080
internalListenerName=internal
new config required:
lookupListenerName=external
```
In the approach, when the client sends a lookup request to the broker to
get the owner broker without the listnerName, the broker only returns one
service URL that is with the same listener name for which the default value
of the lookupListenerName is given in the broker configuration. Therefore,
we must allow the client to get the corresponding service URL with the same
advertised listener name as present in the broker config.
This approach's purpose is to keep client-side simple and handle situations
when not having listenerName information can lead to breaking scenarios. By
having the default value, clients should be able to connect with the broker
even if the request lands to the broker who is not the owner and redirects
to some other broker.
### Backward Compatibility
The above mentioned approach(2) of having a new config as lookupListener is
backward compatible as there should not be any impact on the existing users
if this configuration would not be set. Users can set this configuration
for listener selection for the external communication as per their usecase.
The same listener would be picked up for the admin APIs as well if admin
calls are coming outside the network.
Approach(1), Fixing the code flow for http protocol might be a breaking
change for the clients where we are returning the all broker addresses
corresponding to the listener rather than just having service url. This can
be breaking for the clients who are used to this inconsistency of having
internal listener’s service url as lookup request. To deal with this, we
can have a flag - `preferHttpClientListenerOverInternalListener` which
would indicate whether this change should be enabled for clients or not.
By Default, the value of this flag can be set as false for backward
compatibility.
Regards,
Manmeet
On Thu, Feb 8, 2024 at 12:00 AM Manmeet Kaur <me...@gmail.com>
wrote:
> https://github.com/apache/pulsar/pull/22039
>