Work around golang/go#59690

[patrobinson]

Description

Work around a Go bug in HTTP/2 on Linux.

Context

Various reports of connection failures.

Steps to reproduce:

1. Launch the agent on a linux machine
2. Obtain the source ports being used using netstat -anp
3. Block traffic destined for the agent source ports iptables -I INPUT -p tcp --dport $PORT -j DROP

Expected behaviour:
After the initial connection times out a new connection is established. The old connections no longer show as ESTABLISHED and communications to agent.buildkite.com are restored

Observed behaviour:

The agent hangs for a long period of time (>3 minutes) and continuosly prints errors communicating with the agent backend:

2024-09-23 12:44:54 ERROR  HTTP Timing Trace uri=https://agent.buildkite.com/v3/ping method=GET hostPort=agent.buildkite.com:443 getConn=38.959µs gotConn=40.126µs reused=true idle=false idleTime=0s localAddr=198.19.249.69:42018 wroteHeaders=127.044µs wroteRequest=128.753µs
2024-09-23 12:44:54 WARN   ubuntu Failed to ping: Get "https://agent.buildkite.com/v3/ping": net/http: request canceled (Client.Timeout exceeded while awaiting headers) (Last successful was 13m0.810827775s ago)

Changes

• Use http2.ConfigureTransports to gain access to ReadIdleTimeout, and set it to a nice low 30s, as documented in x/net/http2: raciness in stream close prevents closing of failed connections, blocking subsequent requests golang/go#59690 (comment).
• Make some related changes to authenticatedTransport. Getting rid of authenticatedTransport is too much hassle right now, but we can take inspiration from https://github.com/golang/oauth2/blob/master/transport.go#L20.

Testing

• Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
• Code is formatted (with go fmt ./...)

[patrobinson]

I can't approve this but it LGTM

[patrobinson]

Is this incase err != nil ? Should we just make this an else to the previous conditional?

[DrJosh9000]

I think that writing it this way emphasises that each return arg is its own value, whereas if err != nil { ... } else { ... } makes an assumption about when the returns are nil or not. In this case either would probably be fine.

[DrJosh9000]

✅