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. >
