Re: [PATCH 1/1] protocol: limit max protocol version per service

From: Stefan Beller <sbeller@google.com>
Date: Oct 5, 2018, 19:25 UTC
Message-ID: <CAGZ79kbSF5XM72SOQH7XNyjeCLqQ+AuxOqMPTspjOgk-jEgrzw@mail.gmail.com>
In-Reply-To: <20181005001817.GB32105@google.com>

Show 6 quoted lines

> > But given that we transport the version in env variables, we'd
> > need to be extra careful if we just did not see the version
> > in the --remote=. above?
>
> Sorry, I'm not sure I understand this. What about env variables requires
> caution?

By locally transporting the version via env variables means the absence of "version X" lines in the GIT_TRACE output is not a clear indication of version 0, I would think. It is a strong indicator, but now we'd have to dig and see if the env variables were set outside?

Show 13 quoted lines

> >
> > > I suppose if we are strict about serving from a single endpoint, the
> > > version registry makes more sense, and individual operations can declare
> > > acceptable version numbers before calling any network code?
> >
> > Ah yeah, that makes sense!
>
> Thinking out loud here. Please let me know if I say something stupid :)
>
> So we'll have (up to) three pieces of version information we'll care
> about for version negotiation:
>
> 1. (maybe) a client-side protocol.version config entry

(and in case we don't, we have it implicit ly hardcoded, as we have to choose the default for users that don't care themselves about this setting)

> 2. a list of acceptable proto versions for the currently running
>    operation on the client
> 3. a list of acceptable proto versions for the server endpoint that
>    handles the request

Yes that matches my understanding. The issue is between (1) and (2) as (1) is in a generic config, whereas (2) is specific to the command, such that it may differ. And as a user you may want to express things like: "use the highest version", which is done by setting (1) to "version 2" despite (2) not having support of all commands for v2.

Show 8 quoted lines

> According to the doc on protocol.version: "If set, clients will attempt
> to communicate with a server using the specified protocol version. If
> unset, no attempt will be made by the client to communicate using a
> particular protocol version, this results in protocol version 0 being
> used."
>
> So, if protocol.version is not set, or set to 0, the client should not
> attempt any sort of version negotiation.

Yes, as version 0 is unaware of versions, i.e. there are old installations out there where all the versioning code is not there, so in case of an old client the new server *must* speak v0 to be able to communicate (and vice versa).

Show 17 quoted lines

> Otherwise, the client prefers a
> particular version, but we don't guarantee that they will actually use
> that version after the (unspecified) version negotiation procedure.
>
> If protocol.version is set to something other than 0, we construct a
> list of acceptable versions for the given operation. If the
> protocol.version entry is present in that list, we move it to the front
> of the list to note that it is the preferred version. We send all of
> these, in order, in the request.
>
> When the server endpoint begins to handle a request, it first constructs
> a list of acceptable versions. If the client specifies a list of
> versions, we check them one-by-one to see if they are acceptable. If we
> find a match, we use that version. If we don't find any matches or if
> the client did not send a version list, we default to v0.
>
> Seem reasonable?

This sounds super reasonable!

Thanks Stefan