Hi Michael, I appreciate your detailed explanation. I agree with your proposal. It seems that sharing version information between the proxy and broker would indeed be beneficial for debugging purposes without creating an unnecessary tight coupling between the two components.
I appreciate your willingness to discuss this feature further r and for taking the time to address the concerns raised. Best regards, Xiangying On Tue, Apr 11, 2023 at 12:17 PM Michael Marshall <mmarsh...@apache.org> wrote: > Thanks for your feedback Mattison and Xiangying. I'll note that the > PIP vote did close already and I have the implementation just about > done, but I'm happy to discuss this feature a bit more here. > > > we should avoid coupling the concepts in the proxy and the broker > > Sharing version information does not tightly couple the proxy and the > broker. It makes it easier to collect the relevant debugging > information that can help solve problems faster. The client already > tells the broker its version. Are you able to provide more insight > here? > > > The proxy should be a separate component. Instead of continuing to > couple the relevant proxy concepts into the broker, everyone should be a > client to the broker. > > I don't think this analogy holds true. The pulsar proxy is not a layer > 4 proxy. It sends its own pulsar protocol messages. I want to quickly > know the proxy version when debugging issues observed in the broker, > and this is the only way I see for the broker to get this information. > > > I think the intention behind this is excellent, but directly modifying > the protocol might be a bit too heavy-handed. > > Do you disagree with sharing this information with the broker? I think > we generally view the protocol as lightweight. > > > Wouldn't it be better to directly expose the proxyVersion and > clientVersion information via Prometheus metrics > > Which server is producing these metrics? My goal is for the broker to > get this information so it can be part of the logs. The only way to > transport these metrics is via the protocol. > > If there is a serious objection, I can add an option for the proxy to > withhold this data from the broker. However, I think we should enable > it by default. > > Thanks, > Michael > > On Sat, Apr 8, 2023 at 2:19 AM Xiangying Meng <xiangy...@apache.org> > wrote: > > > > Dear Michael, > > > > I think the intention behind this is excellent, but directly modifying > the protocol might be a bit too heavy-handed. > > This approach may lead to data redundancy. > > In large-scale clusters, every client connection would need to transmit > the extra proxy version information, > > which might increase network overhead. > > Therefore, we should consider a more lightweight solution. > > > > If the primary purpose of the proxy version information is for > diagnostics or auditing, > > we could explore alternative methods for collecting this information > without modifying the protocol level. > > Wouldn't it be better to directly expose the proxyVersion and > clientVersion information via Prometheus metrics, > > as mentioned in the "Future work(Anything else)`" section of the > proposal? > > > > Please let me know what you think about this suggestion. > > > > Best regards, > > Xiangying > > > > On Thu, Apr 6, 2023 at 3:46 PM <mattisonc...@gmail.com> wrote: > >> > >> Sorry for the late response. > >> > >> Why do we need to make the broker aware of the proxy when, by normal > software design, we should avoid coupling the concepts in the proxy and the > broker? The previous authentication was for historical reasons, but we > should not continue to introduce this coupling. > >> > >> The proxy should be a separate component. Instead of continuing to > couple the relevant proxy concepts into the broker, everyone should be a > client to the broker. > >> > >> Best, > >> Mattison > >> On Feb 25, 2023, 01:12 +0800, Michael Marshall <mmarsh...@apache.org>, > wrote: > >> > Great suggestions, Enrico. > >> > > >> > > It would be interesting to reject connections on the Proxy if the > >> > > client tries to set that field. > >> > > >> > I support making the proxy reject invalid input. We could also have > >> > the client reject connections where the client connect command > >> > includes `original_principal`, `original_auth_data`, or > >> > `original_auth_method`, since those are also only meant to be sent by > >> > the proxy. > >> > > >> > > On the broker there is no way to distinguish a proxy from a client, > that's fair. > >> > > >> > We can reject these connections when authentication and authorization > >> > are enabled. My draft PR includes such logic. > >> > > >> > Thanks, > >> > Michael > >> > > >> > On Fri, Feb 24, 2023 at 7:29 AM Enrico Olivelli <eolive...@gmail.com> > wrote: > >> > > > >> > > Makes sense. > >> > > > >> > > It would be interesting to reject connections on the Proxy if the > >> > > client tries to set that field. > >> > > On the broker there is no way to distinguish a proxy from a client, > that's fair. > >> > > But on the proxy it is not expected to see a connection from > another proxy. > >> > > > >> > > +1 > >> > > > >> > > Enrico > >> > > > >> > > Il giorno ven 24 feb 2023 alle ore 10:00 Zike Yang <z...@apache.org> > ha scritto: > >> > > > > > >> > > > > Hi, Michael > >> > > > > > >> > > > > Thanks for initiating this PIP. > >> > > > > > >> > > > > +1 > >> > > > > > >> > > > > BR, > >> > > > > Zike Yang > >> > > > > > >> > > > > > >> > > > > Zike Yang > >> > > > > > >> > > > > On Fri, Feb 24, 2023 at 12:16 PM Michael Marshall < > mmarsh...@apache.org> wrote: > >> > > > > > > > >> > > > > > > Hi Pulsar Community, > >> > > > > > > > >> > > > > > > In talking with Zike Yang on > >> > > > > > > https://github.com/apache/pulsar/pull/19540, we identified > that it > >> > > > > > > would be helpful for the proxy to forward its own version > when > >> > > > > > > connecting to the broker. Here is a related PIP to improve > the > >> > > > > > > connection information available to operators. > >> > > > > > > > >> > > > > > > Issue: https://github.com/apache/pulsar/issues/19623 > >> > > > > > > Implementation: https://github.com/apache/pulsar/pull/19618 > >> > > > > > > > >> > > > > > > I look forward to your feedback! > >> > > > > > > > >> > > > > > > Thanks, > >> > > > > > > Michael > >> > > > > > > > >> > > > > > > Text of PIP copied below: > >> > > > > > > > >> > > > > > > ### Motivation > >> > > > > > > > >> > > > > > > When clients connect through the proxy, it is valuable to > know which > >> > > > > > > version of the proxy connected to the broker. That > information isn't > >> > > > > > > currently logged or reported in any easily identifiable > way. The only > >> > > > > > > way to get information about the connection is to infer > which proxy > >> > > > > > > forwarded a connection based on matching up the IP address > in the > >> > > > > > > logs. > >> > > > > > > > >> > > > > > > An additional change proposed in the implementation is to > log this new > >> > > > > > > information along with the `clientVersion`, > `clientProtocolVersion`, > >> > > > > > > and relevant authentication role information. This > information will > >> > > > > > > improve debug-ability and could also serve as a form of > audit logging. > >> > > > > > > > >> > > > > > > ### Goal > >> > > > > > > > >> > > > > > > Improve the value of the broker's logs and metrics about > connections > >> > > > > > > to simplify debugging and to make it easier for Pulsar > operators to > >> > > > > > > understand how clients are connecting to their clusters. > >> > > > > > > > >> > > > > > > ### API Changes > >> > > > > > > > >> > > > > > > Add the following: > >> > > > > > > > >> > > > > > > ```proto > >> > > > > > > message CommandConnect { > >> > > > > > > // Other fields omitted > >> > > > > > > optional string proxy_version = 11; // Version of the proxy. > >> > > > > > > Should only be forwarded by a proxy. > >> > > > > > > } > >> > > > > > > ``` > >> > > > > > > > >> > > > > > > ### Implementation > >> > > > > > > > >> > > > > > > Initial implementation: > https://github.com/apache/pulsar/pull/19618 > >> > > > > > > > >> > > > > > > ### Alternatives > >> > > > > > > > >> > > > > > > The `CommandAuthResponse` has a `client_version` field. > It's possible > >> > > > > > > that someone would want this `proxy_version` field on that > message. > >> > > > > > > However, I think we should not continue down the path of > duplicating > >> > > > > > > fields in the connection handshake protocol. > >> > > > > > > > >> > > > > > > ### Anything else? > >> > > > > > > > >> > > > > > > Future work could include exposing this `proxyVersion` and > >> > > > > > > `clientVersion` information via Prometheus metrics. >
