You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Michael Marshall <mm...@apache.org> on 2023/02/24 04:16:11 UTC
[DISCUSS] PIP-250: Add proxyVersion to CommandConnect
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.
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Xiangying Meng <xi...@apache.org>.
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 <mm...@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 <xi...@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 <ma...@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 <mm...@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 <eo...@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 <zi...@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 <
> mmarshall@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.
>
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Michael Marshall <mm...@apache.org>.
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 <xi...@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 <ma...@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 <mm...@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 <eo...@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 <zi...@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 <mm...@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.
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Xiangying Meng <xi...@apache.org>.
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 <ma...@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 <mm...@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 <eo...@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 <zi...@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 <
> mmarshall@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.
>
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by ma...@gmail.com.
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 <mm...@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 <eo...@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 <zi...@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 <mm...@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.
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Michael Marshall <mm...@apache.org>.
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 <eo...@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 <zi...@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 <mm...@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.
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Enrico Olivelli <eo...@gmail.com>.
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 <zi...@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 <mm...@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.
Re: [DISCUSS] PIP-250: Add proxyVersion to CommandConnect
Posted by Zike Yang <zi...@apache.org>.
Hi, Michael
Thanks for initiating this PIP.
+1
BR,
Zike Yang
Zike Yang
On Fri, Feb 24, 2023 at 12:16 PM Michael Marshall <mm...@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.