Failure to clone git repository - server sent https2 GOAWAY

[miketonks]

Source Controller seems to use http2 keep alive when cloning git repository.

Error message:

unable to clone 'https://example.com/test/repo, error: http2: server sent GOAWAY and closed the connection; LastStreamID=1999, ErrCode=NO_ERROR, debug=""

For short poll intervals (I tried 1m and 1m40s) we get a small but repeatable number of failures, exactly every 1999 requests. This is due to nginx server config, which will reset connection after maximum is reached. However client can request keepalive or not, and it would be better to not use it in this case.

Default go http client enables keepalive with a time of 90s

Using keepalive for polling seems incorrect. Can we disable keepalive please?

[miketonks]

Digging a bit deeper, it looks like go-git defaults to using the http.DefaultClient here:

https://github.com/go-git/go-git/blob/bf3471db54b0255ab5b159005069f37528a151b7/plumbing/transport/client/client.go#L19

but it is possible to override the transport as per example here:

source-controller/controllers/gitrepository_controller_test.go

Line 306 in 2dc7132

client.InstallProtocol("https", transport)

So it should be quite simple to change defaults, for go-git at least.

Any thoughts about this issue? Would a PR be likely to be considered to change default client behavior to disable keep-alives, or to allow a configuration option to set this?

[squaremo]

I'm not sure exactly how git uses HTTP/2, but my expectation is that it will make multiple requests in sequence, i.e., not pipelined. Keeping the connection open will be useful for that. But since the latency of establishing a connection is going to be much less than the polling interval, it is not worthwhile keeping the connection open across polls. My estimation is that it would be better to make keep-alive a low number of seconds (lower than a reasonable polling interval), rather than switching it off altogether.

[miketonks]

Yes totally agree. The git fetch probably makes multiple requests where connection reuse would be useful.

Just needs to be configurable so we can set it below the poll interval.