Re: [PATCH 00/10] Advertise trace2 SID in protocol capabilities

[Josh Steadmon <steadmon@google.com>]

On 2020.10.30 11:54, Jeff Hostetler wrote:
> 
> 
> On 10/29/20 5:32 PM, Josh Steadmon wrote:
> > In order to more easily debug remote operations, it is useful to be able
> > to inspect both client-side and server-side traces. This series allows
> > clients to record the server's trace2 session ID, and vice versa, by
> > advertising the SID in a new "trace2-sid" protocol capability.
> > 
> 
> Very nice!  This should be very helpful when matching up client and
> server commands.
> 
> 
> > Two questions in particular for reviewers:
> > 
> > 1) Is trace2/tr2_sid.h intended to be visible to the rest of the code,
> >    or is the trace2/ directory supposed to be opaque implementation
> >    detail? If the latter, would it be acceptable to move tr2_sid_get()
> >    into trace2.h?
> 
> I put all the trace2-private stuff in that sub-directory and gave
> everything tr2_ prefixes to hide the details as much as I could
> (and as an alternative to the usual tendency of having everything
> be static within a massive .c file).
> 
> So, yeah, my intent was to make all of it opaque.
> So that just trace2.h contains the official API.
> 
> Perhaps in trace2.c you could create:
> 
> const char *trace2_session_id(void)
> {
>     return tr2_sid_get();
> }

Done in V2, thanks.


> > 2) upload-pack generally takes configuration via flags rather than
> >    gitconfig. From offline discussions, it sounds like this is an
> >    intentional choice to limit potential vulnerability from malicious
> >    configs in local repositories accessed via the file:// URL scheme. Is
> >    it reasonable to load the trace2.announceSID option from config files
> >    in upload-pack, or should this be changed to a flag?
> 
> I don't have the history to comment on this.
> 
> One thing to consider is that the SID for a Git process is built up
> from the SID of the parent and the unique data for the current process.
> So for example, if `git fetch` has SID `<sid1>` and it spawns
> `git upload-pack`, the child will have SID `<sid1>/<sid2>` and if that
> spawns `git index-pack`, that child will have `<sid1>/<sid2>/<sid3>`.
> This is very helpful when tracking down parent/child relationships
> and perf hunting.
> 
> This SID inheritance is passed via an environment variable to
> the child, which extends it and passes the longer version to its
> children.
> 
> So the value being passed between client and server over the
> protocol may look like `<sid1>/<sid2>/<sid3>` rather than just a
> single `<sid_x>` term.  For your purposes, do you want or care if
> you get the single term or the full SID ?

I'm not sure we care too much one way or the other. A single component
of the SID should be enough to join client & server logs, but it's
easier to just send the whole thing.


> Also, there's nothing to stop someone from seeding that environment
> variable in their shell with some mischief before launching the
> top-level Git command.  So the above example might see the SID as
> `<mischief>/<sid1>/<sid2>/<sid3>`.  I'm not sure if this could be
> abused when inserted into the V0/V1/V2 protocol or your logging
> facility.
> 
>     $ GIT_TRACE2_EVENT=1 GIT_TRACE2_PARENT_SID=hello git version
>     {"event":"version","sid":"hello/20201030T154143.660608Z-H86606a97-
> P00001d30",...}
>     ...
> 
> So maybe we want to have a public API to return a pointer to just
> the final `<sid_x>` term ?  (Then again, maybe I worry too much.)

Yeah, it's certainly possible to muck with the SID as you describe, but
I'm not sure I see any big problems that could be caused. If someone
points out an issue I've missed, I'll be happy to change this, though.

> Thanks,
> Jeff

Thanks for the review!


> > Josh Steadmon (10):
> >    docs: new capability to advertise trace2 SIDs
> >    docs: new trace2.advertiseSID option
> >    upload-pack: advertise trace2 SID in v0 capabilities
> >    receive-pack: advertise trace2 SID in v0 capabilities
> >    serve: advertise trace2 SID in v2 capabilities
> >    transport: log received server trace2 SID
> >    fetch-pack: advertise trace2 SID in capabilities
> >    upload-pack, serve: log received client trace2 SID
> >    send-pack: advertise trace2 SID in capabilities
> >    receive-pack: log received client trace2 SID
> > 
> >   Documentation/config/trace2.txt               |  4 +
> >   .../technical/protocol-capabilities.txt       | 13 ++-
> >   Documentation/technical/protocol-v2.txt       |  9 +++
> >   builtin/receive-pack.c                        | 16 ++++
> >   fetch-pack.c                                  | 11 +++
> >   send-pack.c                                   |  9 +++
> >   serve.c                                       | 19 +++++
> >   t/t5705-trace2-sid-in-capabilities.sh         | 79 +++++++++++++++++++
> >   transport.c                                   | 10 +++
> >   upload-pack.c                                 | 23 +++++-
> >   10 files changed, 190 insertions(+), 3 deletions(-)
> >   create mode 100755 t/t5705-trace2-sid-in-capabilities.sh
> >