[GitHub] [pulsar] michaeljmarshall commented on pull request #19540: [fix][proxy] Fix using wrong client version in pulsar proxy

["michaeljmarshall (via GitHub)" <gi...@apache.org>]

michaeljmarshall commented on PR #19540:
URL: https://github.com/apache/pulsar/pull/19540#issuecomment-1439536226

   > Hi, @michaeljmarshall . Thanks for your comment.
   > 
   > > The client version is still filtered out for the Java client for the same reason that the Pulsar proxy client version is filtered out.
   > 
   > Do you have reproducible steps? It seems to work fine for me. The java client should always set the client_version here:
   > 
   > https://github.com/apache/pulsar/blob/954f406fab03ce8858883335ce6d5a676bb45c92/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java#L295-L297
   > 
   > It will not use `Pulsar Client` as the value here:
   > 
   > https://github.com/apache/pulsar/blob/954f406fab03ce8858883335ce6d5a676bb45c92/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/Commands.java#L198
   
   Sorry, I read the code too quickly, I see that the Java Client's version string works as you described.
   
   > > Further, it would be helpful to know when a connection is being proxied, which brings into scope this PR. Instead of using just one clientVersion, I think we should add a new protobuf field specifically for the proxyVersion. I would also make the restriction that a client cannot set this field unless it authenticates as a proxy (assuming authentication/authorization is enabled).
   > 
   > I'm +1 for this. But from my understanding, this is out of the scope of this PR discussion. This is adding a new feature to show the version of the proxy that the client connects.
   
   I think that depends on whether the behavior before this PR was a bug. In my opinion, changing the `clientVersion` that the proxy sends is a larger change that increases the scope of this PR.
    
   > > Further, I think it is a stretch to call this a bug. I view it as just a missing feature.
   > 
   > We could only get the client version from the broker if the client connects to the broker directly instead of through the proxy. It results in different behavior when using the proxy. I think it's a bug because the proxy should be transparent to users for the `client_version`.
   
   To say "the proxy should be transparent to users for the `client_version`" seems more like finding a potentially inconsistent design decision than finding a bug. Ultimately, the question is how should the `client_version` field be used? Is it for users or administrators? I can see the argument for end users not needing to know the proxy is in the middle, but I can also see an argument for operators wanting to know that the client connected through the proxy and to know which version of the proxy is connected. It is very relevant auditing information.
   
   > Also, I have created a discussion here: https://lists.apache.org/thread/n59k537fhthjnzkfxtc2p4zk4l0cv3mp to discuss about the client version conflict issue. Welcome to join this thread.
   
   Thanks, I replied. I think the scope of the discussion could be increased, though.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org