[PIP-338] Add default lookup listener and fix inconsistency with listener's usage between different protocols #22039
Conversation
github-actions
bot
added
type/PIP
doc-required
Meet0861
changed the title
|
@codelipenghui @eolivelli @EronWright @wangjialing218 Can you please help here with a review? |
| ## 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. |
There was a problem hiding this comment.
Thinking out aloud here, I haven't come to a conclusion yet.
In PIP-95 there's a solution that I'd prefer instead:
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.
The main problem I have is that there is already a design for solving the problem, but that has never been implemented in Pulsar.
I acknowledge that the benefit of the lookupListenerName solution would be that it could be more practical to implement. In that case, the internalListenerName should be made effective so that it is used in all broker-to-broker connections within the cluster.
The limitation of the lookupListenerName solution is that it assumes that there's only one external listener for the cluster. The PIP-95 solution of "use a unique bind address for each listener." would be better since there wouldn't be any such limits that could cause problems for more advanced use cases.
There was a problem hiding this comment.
I agree that there are inconsistencies in design and implementation. Maybe there were missing things in design of PIP-91 and 95 itself as well.
I do not think a bind address mapping is going to solve this. For instance, the same http scheme/protocol based external facing url leads to multiple different protocol changes in output -
http based lookup call can either result in the client using the pulsar protocol as the output, or an http based redirect url as an output.
http based admin calls always end up with either 200 or http based redirect url.
Both the above cases may have the same entry point in terms of bind address.
Please correct me if my understanding is wrong here in the examples.
There was a problem hiding this comment.
To add to this, I actually would refer to what kafka has been doing for a long while.. our proposal is actually very close to kafka KIP 103 which PIP-95 was actually supposed to be doing..
There was a problem hiding this comment.
@grssam I hope the comment #22039 (comment) explains how this can be solved for the Pulsar Admin API http redirects.
There was a problem hiding this comment.
What about http lookup leading to pulsar protocol being used? The common use case for clients is to use the http load balancer address for initial service discovery, which means that the lookup service used by the client will be the HttpLookupService .. Now in this case, when the actual lookup happens, if its a redirect, the http redirect url is used but in case its a 200 response, the pulsar protocol address is used. Thus - leading to different output listener for the same bind address.
There was a problem hiding this comment.
Now in this case, when the actual lookup happens, if its a redirect, the http redirect url is used but in case its a 200 response, the pulsar protocol address is used.
The code at
findRedirectLookupResultAsync method ignores that listener name that is passed in in the LookupOptions parameter. That looks like a bug and not an implementation gap. :)
There was a problem hiding this comment.
Those are exactly the inconsistencies that we are trying to fix here.
Along with a default lookup listener so that we are not "forced" to just send a custom listener name from the client side without which the system does not work at all. A default shouldn't be something that is this broken for users that without overriding, things don't work at all.
|
|
||
| # Motivation | ||
| ## Issue in existing Code flow | ||
| 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. |
There was a problem hiding this comment.
PIP-91 -> PIP-61. Please also provide links to the PIP definitions.
| ## 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) |
There was a problem hiding this comment.
For topic lookups a Pulsar binary protocol endpoint is always returned. It doesn't make sense to return all endpoints for topic lookups.
There was a problem hiding this comment.
Agreed, its confusing to see the http urls present in lookup response, and we are lacking historic info on why they are present. But if and when they are already present, they should be resolvable based in the ask of the request/default config.
There was a problem hiding this comment.
Yes you are right, this is the bug that would need fixing, mentioned in the previous comment #22039 (comment) .
|
|
||
| **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. |
There was a problem hiding this comment.
This doesn't make sense to me. The internalListenerName is already a default listener. I don't see how this would solve the gap in the solution which is that all Pulsar Admin REST APIs don't have the redirects covered with the selected listener name.
There was a problem hiding this comment.
the users will configure an external, client resolvable listener name as the lookupListnerName (ex - an IP). It will be different from internalListerName in case the internal address (ex - pod fqn) is non-resolvable outside the cluster/k8s layer.
There was a problem hiding this comment.
How would you know what to use, the "internalListenerName" or the "lookupListenerName"? I think that "internalListenerName" is badly named, it's essentially the default listener name. From that perspective, the "lookupListenerName" is conflicting since it's another default.
The problem is that lookups are needed also within the cluster and perhaps for multiple different listeners. The design of PIP-95 is that there isn't a single "external" listener. I think it's better to keep the current PIP-95 design and perhaps extend it to include bindAddresses support also for the web endpoints.
That way it would be possible to have a different default listener name for different bind address/port as it is already possible for the Pulsar binary protocol. That was a scope limitation of PIP-95, mentioned also in this comment.
There was a problem hiding this comment.
How would you know what to use, the "internalListenerName" or the "lookupListenerName"? I think that "internalListenerName" is badly named, it's essentially the default listener name. From that perspective, the "lookupListenerName" is conflicting since it's another default.
Any documentation supporting this idea of internallistenerName being default? Because the way it is used currently, it makes it evident that the address represented by this listener is supposed to be an intenal usage thing.
The core issue arises when your deterministic sticky named address is not resolvable outside the cluster. If we ignore that problem, then this is a non issue. Or in other words, the problem arises when we want to use deterministic sticky addresses to brokers as identifier rather than its IP address. One of the main reason to do this is the fact that this address will be used to identify a broker wrt bundle ownership i.e. even a single pod restart would lead to removal of that broker and creation of a "new" broker in the cluster. Another use case of having a static deterministic broker address is for the regex used in namespace isolation policies.
Generally, these static addresses (stateful pod name) are not resolvable outside the k8s cluster.
Thus, we have to set out advertised address as this static, non-client-resolvable address as we are using isolation groups feature and for the other reason mentioned above as well.
Now as soon as we do that, we have to set the same address in the list of advertised addresses as well, that too, as the internal listener address other wise the check at this line breaks in searchForCandidateBroker which leads to lookup timeouts:
because isBrokerActive does a check of in a list of . So we have to set internalListenerName and ensure that the internal listener is in the same format (deterministic non-IP based host name) as the advertised listener.
So, now, as per implementation itself, at this point, internalListenerName cannot be assumed as the default listener as its something only resolveable inside the pulsar cluster/k8s namespace.
The problem is that lookups are needed also within the cluster and perhaps for multiple different listeners. The design of PIP-95 is that there isn't a single "external" listener. I think it's better to keep the current PIP-95 design and perhaps extend it to include
bindAddressessupport also for the web endpoints.That way it would be possible to have a different default listener name for different bind address/port as it is already possible for the Pulsar binary protocol. That was a scope limitation of PIP-95, mentioned also in this comment.
Could you run us through how that would work for the case when a normal producer client does an http based lookup call only to use the native protocol response from it and to also ensure that redirect in case of 307 response of that lookup call also works correctly?
We have to consider the fact that due to many security/audit reasons, there might only be a single http based load balancer at port 80 possible for the brokers which may only talk to a single broker-port of 8080.
There was a problem hiding this comment.
The core issue arises when your deterministic sticky named address is not resolvable outside the cluster. If we ignore that problem, then this is a non issue. Or in other words, the problem arises when we want to use deterministic sticky addresses to brokers as identifier rather than its IP address.
It would be expected that there would be a way to address that. There are simply bugs and gaps in the current solution as it has been discussed and I think that we are pretty close in getting to a consensus.
Any documentation supporting this idea of internallistenerName being default? Because the way it is used currently, it makes it evident that the address represented by this listener is supposed to be an intenal usage thing.
It's referred to as "internal listener", but in the implementation, it is used as the default. For example
there is "primary channel" in the code comment:
Since the "internalListenerName" is set to the first listener if it is unset, there's always going to be one listener that is the "internal listener". In many ways it could be seen as a default.
Now as soon as we do that, we have to set the same address in the list of advertised addresses as well, that too, as the internal listener address other wise the check at this line breaks in searchForCandidateBroker which leads to lookup timeouts:
The PRs #21894 & #21937 fixed the issue #21897 and there's no coupling from the brokerId to the "advertised listeners" any more. The fixes have been backported all the way to branch-3.0 .
There was a problem hiding this comment.
yes, after your PRs the issue of finding advertised listener in list of internal listeners is fixed. But this regression is introduced in 2.10 . We are upgrading from 2.9 to 2.10.
The only reason it would work post 3.0 is because we will stop setting an internalListenerName all together. Because as soon as we start setting that, we will once again come into the issue of all the inconsistencies mentioned in this PIP and the need to take the listener name from incoming lookup/admin calls.
There was a problem hiding this comment.
But this regression is introduced in 2.10 . We are upgrading from 2.9 to 2.10.
According to the current release policy, there's no active maintenance for 2.10 branch in the Apache Pulsar project, the dev mailing list thread is https://lists.apache.org/thread/n5zfhohvk7olodfx2gksxk9n5f2pt3h8 . The Apache Pulsar community and Apache Pulsar PMC could change this, but this is the current state.
It would be better to target 3.0.x which is the current LTS release and will be actively supported until May 2025.
I would assume that it's possible to upgrade from 2.9 to 3.0.x , but that obviously requires testing in any case.
There was a problem hiding this comment.
I checked this in slack, and we will go from 2.10 to 3.0.x directly, but we were already halfway done with 2.9 to 2.10 upgrade and only in production found this bug.
Currently there is no documentation on upgrade paths at all. All previous conversations with SN or on slack were indicating to do upgrades one version at a time.
There was a problem hiding this comment.
The only reason it would work post 3.0 is because we will stop setting an
internalListenerNameall together. Because as soon as we start setting that, we will once again come into the issue of all the inconsistencies mentioned in this PIP and the need to take the listener name from incoming lookup/admin calls.
This still holds true.
There was a problem hiding this comment.
Currently there is no documentation on upgrade paths at all. All previous conversations with SN or on slack were indicating to do upgrades one version at a time.
That's true, however that's a general rule in thumb. In this case since 3.0.x is the oldest actively supported, it makes sense to target it. If an upgrade path works between 2 versions in tests & experiments, there shouldn't be a reason why it couldn't be used. Obviously it might require some more care in validating if rollbacks are possible.
| 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. |
There was a problem hiding this comment.
This can be covered in the broker code.
One possibility would be to extend this method to handle redirecting to the URL which the incoming request's active listener name points to:
The advertised listeners are available in the BrokerLookupData records which are already used.
There was a problem hiding this comment.
A method could be added to PulsarWebResource for getting the listener name from a parameter or a header. There's no need to add these explicitly for each method in the API as has been done for the Topic lookup.
static final String LISTENERNAME_HEADER = "X-Pulsar-ListenerName";
static final String LISTENERNAME_PARAMETER = "listenerName";
public String listenerName() {
return Optional.ofNullable(httpRequest.getParameter(LISTENERNAME_PARAMETER))
.or(() -> Optional.ofNullable(httpRequest.getHeader(LISTENERNAME_HEADER)))
.orElseGet(() -> pulsar().getConfiguration().getInternalListenerName());
}
There was a problem hiding this comment.
Primary goal here is also to limit the changes to server side only, because there are cases where there is either no scope of adding a listener name as query param or header in the current system. Even in the systems which can add this config, think 100s of different applications needing either a client upgrade or a config change to add this listener name.
And in case we do end up asking all clients to send a specific listener in their API calls or lookup requests, then isn't that a default for us? Thus, the addition of a default value for the incoming listener name.
There was a problem hiding this comment.
because there are cases where there is either no scope of adding a listener name as query param or header in the current system.
In PIP-95 design I think that the idea is that this could be added in a reverse proxy that is in front of Pulsar.
The alternative is to extend the implementation to support a default listener name based on the bind address/port, as mentioned in the previous comment, #22039 (comment) .
I'd prefer that approach since it would make the solution consistent with the other parts of PIP-61 & PIP-95.
There was a problem hiding this comment.
It is becoming more and more clear that at least you are preferring the bind address solution.
The purpose of this PIP was to fix a broken behavior. We have already established that things are broken in the current state, be it a miss/bug or half done implementation.
If this PIP is not the right solve, then we would not like to spend more time on this discussion. We are also not here to pickup and finish someone else's PIP in our current state of urgency with running our production with this bug.
It seems to me that the right approach in our situation is to patch pulsar ourselves and maintain that patch version after version internally.
I wish there was an agreement here but from the start, you have been advocating for a different approach that what our production currently needs.
I will discuss this internally and close this PIP later today. I appreciate your time and effort in the discussion.
There was a problem hiding this comment.
The bind address solution is part of PIP-61 and PIP-95 design. That is the reason to prefer it. The bind address solution could be extended to cover the http server so that a bind address can be configured to have a specific default listener.
Could you please answer this question from the other comment #22039 (comment)
Could you run us through how that would work for the case when a normal producer client does an http based lookup call only to use the native protocol response from it and to also ensure that redirect in case of 307 response of that lookup call also works correctly?
We have to consider the fact that due to many security/audit reasons, there might only be a single http based load balancer at port 80 possible for the brokers which may only talk to a single broker-port of 8080.
There was a problem hiding this comment.
Could you run us through how that would work for the case when a normal producer client does an http based lookup call only to use the native protocol response from it and to also ensure that redirect in case of 307 response of that lookup call also works correctly?
First of all, that would require fixing the bug where the listenerName is ignored, that's mentioned in #22039 (comment) . The HTTP lookup already supports passing the listener name. That would obviously require passing the listener name from clients. The alternative without making changes to Pulsar would be to put a HTTP reverse proxy such as nginx in front of Pulsar REST calls that sets the "X-Pulsar-ListenerName" header. I believe that adding a header is also possible with nginx ingress controller. It's not optimal, but that's how PIP-95 has been scoped.
There was a problem hiding this comment.
That would obviously require passing the listener name from clients. The alternative without making changes to Pulsar would be to put a HTTP reverse proxy such as nginx in front of Pulsar REST calls that sets the "X-Pulsar-ListenerName" header. I believe that adding a header is also possible with nginx ingress controller. It's not optimal, but that's how PIP-95 has been scoped.
My opinion is that a design should not force users to set defaults from outside pulsar systems. Why was it not discussed that this should be done via a config rather than asking either every client to send a value or setup another component or config to add a "required" header at the load balancer layer...
Can we agree that there are cases where things can be miss in old PIP and not all discussed and approved PIPs should be taken as bible :)
There was a problem hiding this comment.
My opinion is that a design should not force users to set defaults from outside pulsar systems. Why was it not discussed that this should be done via a config rather than asking either every client to send a value or setup another component or config to add a "required" header at the load balancer layer...
It's part of PIP-95 design and it is mentioned there. There are also comments about the scope limitation.
I agree that it's not currently convenient when there isn't out-of-the-box support without using a reverse proxy to add the header value. That could be addressed with the bindAddresses solution which sets a default for each binding address/port, in the similar way as it has been addressed for the Pulsar binary protocol.
There was a problem hiding this comment.
just to add to this point that even when listener name is being passed by the clients, as per the current implementation it is not addressed if its corresponding to the http protocol broker's address. The PIP not only focussing on adding a default listener for clients/external communication but also to fix this bug.
Also when we are saying that internalListenerName is a default listener, to be more precise here- it should be default listener for broker to broker communication but not for clients/external communication. That was the reason to add a default listener for external communication as currently we are doing in case of KIP as @grssam mentioned.
| 1. Fix the lookup result formation with all broker addresses rather than only service url | ||
| in NamespaceService.java in following methods: | ||
| ```java | ||
| - CompletableFuture<Optional<LookupResult>> findBrokerServiceUrl(NamespaceBundle bundle, LookupOptions options) |
There was a problem hiding this comment.
a LookupResult is not needed for addressing the Pulsar Admin API's redirects. Please check the previous comment about PulsarWebResource.maybeRedirectToBroker.
There was a problem hiding this comment.
The /lookup/v2/topic API and many other places do not use the above approach (maybeRedirectToBroker) but rather, rely on the lookup result response..
to name some.
The maybeRedirectToBroker method is only used in the API cluster/brokerid/ownedNamespaces and even inside this maybeRedirectToBroker, today, uses the internal listener's http url for redirect. The goal is to have a common method which figures out the correct redirect url in PulsarWebResource.java itself. Maybe it can be this same exact method. We will send a draft PR with changes for early review if needed by tomorrow.
There was a problem hiding this comment.
Yes, there's a need to make things consistent in the implementation as you have suggested.
Motivation
A pip proposal to add a new default lookup listener config and fix bug with listener usage's for http protocol.
Issue in existing Code flow
Currently, the listener mentioned as
internalListenerNamein broker config helps in deciding the listener from the list ofadvertisedListenersto 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-95) corresponding to the http protocol broker’s addresses, it is only used in thepulsarandpulsar+sslprotocols and is not consistent for the other protocols.Need to have lookupListener
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.
Modifications
Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-complete