feat: add timeouts when default client is in use

[hbagdi]

Go's default client has no timeouts and relies on the kernel timeouts,
which goes against the recommended practice of setting timeouts at
application-level whenever possible.

This change ensures that if the default client is used, it is used with
proper timeouts for TCP handshake, TLS handshake and HTTP
request-response lifecycle.

There is no emperical data for the value of 10 seconds.
It is a good starting point and a sane default. Users are encouraged to
create their own http.Client and many users already do so to control
TLSConfig.

[rainest]

We probably want instrumentation around this before setting a default, at least for the application-level timeout. Anecdotally, the DB-less config post can be fairly slow to respond with larger config blobs, and I'm fairly certain that 10s would be short enough to break it for a non-insignificant number of users.

That said, the controller does already instantiate its own HTTP client for use with the Kong client, with no timeouts, so that wouldn't actually be an issue for it.

Would you be open to a separate transport+TLS and HTTP timeout, with the latter being much less aggressive to start, maybe 120s? I'm not aware of issues with the current long (infinite) HTTP timeout, so while I agree that we should set one to follow general good practice, it seems reasonable that we may want to err on the longer side absent data on typical response times.

[hbagdi]

That said, the controller does already instantiate its own HTTP client for use with the Kong client, with no timeouts, so that wouldn't actually be an issue for it.

That's the reason for keeping the timeout so low.

Would you be open to a separate transport+TLS and HTTP timeout, with the latter being much less aggressive to start, maybe 120s? I'm not aware of issues with the current long (infinite) HTTP timeout, so while I agree that we should set one to follow general good practice, it seems reasonable that we may want to err on the longer side absent data on typical response times.

I think a single endpoint behaving uniquely shouldn't be a reason to change the default timeout. For majority of use-cases of this library, Kong should be able to respond within few seconds, barring network/infra hiccups.

I'd like to start with a lower timeout as it would help point out problems in the user's stack but I do see how the potential for (unwelcomed) surprises.
How about 60s?
@mflendrich @shaneutt Anything to add here?

[rainest]

I think a single endpoint behaving uniquely shouldn't be a reason to change the default timeout.

Ideally yes, but as you can't override it per request (searching suggests you can use contexts and NewRequestWithContext to work around lack of dedicated request-level timeouts, but you'd have to then set the timeout on all requests individually), we're kinda stuck with something that's long enough for the slowest endpoint.

60s is better, though I'd still want the controller version to be configurable before releasing it.

[hbagdi]

60s is better, though I'd still want the controller version to be configurable before releasing it.

To be fair here, the timeout we are adding here is only a default.
Users (like decK and KIC) can supply their own *http.Client and set the default to whatever they consider is acceptable in their environment. Is that good enough to keep the default timeout at 10s?

[mflendrich]

On finite vs infinite timeout: There is a class of problems (such as network failure or reconnection) that will cause clients of go-kong to wait forever, which is not a good failure mode. Therefore my preference is for a finite timeout.

On whether the default timeout should be 10, 60, 120s or anything else: There are at least 2 components to be considered: network RTT, and remote processing duration.
Network RTT can easily take 10 seconds in corner cases such as a droopy internet connection (think: tethered phone when in movement on a highway, roaming between cells) or network incidents such as packet loss or routing changes.
Also, as @rainest pointed out, writing DB-less /config can trip the 10s timeout.

Therefore my educated guess is that 60s might be a good choice here, as a compromise between:

• not breaking automations suffering from an intermittent lag longer than 10s (which seems likely for a variety of rasons),
• providing a decent failure mode in case of a filtered connection, a permanent network failure or other similar issues.

Therefore my suggestion is a 60s default timeout, + the attached code comment.

[mflendrich]

I second Travis's point about making it possible to adjust the timeout as the client, and recommend the solution below.

Expecting the user to adjust the timeout in a returned client isn't perfect either because it's at odds with the open-closed principle.

What I'd recommend would be to provide an "overloaded" NewClient:

func NewClient(...) (*Client, error) {
  return NewClientWithTimeout(defaultTimeout, ...)
}

func NewClientWithTimeout(timeout time.Duration, ...) (*Client, error) {
  // actual implementation goes here
}

[hbagdi]

This seems reasonable at first glance but on a closer inspection, it seems an unnecessary addition to the API surface.
An *http.Client has numerous knobs for tweaking network behavior and addition a method for each to this API seems unnecessary burden.
The reason we accept an *http.Client in the constructor is so that the user of the API can define all client-specific behavior as they see fit. This is how this client is used in all downstream repositories I know.
We may add WithTimeout function today but then that opens the door for WithXs for future and adding those here doesn't really make things better for us or the end user in my view.

For reference, here is another API that does this the same way: https://pkg.go.dev/github.com/google/go-github/v35/github#NewClient

(go-kong is inspired heavily from that API)

[hbagdi]

Based on the above comments, I updated the default timeout to 60s.

[mflendrich]

Consider exporting the default timeout and adding a godoc comment, for sake of self-documenting code.

This will make a difference for readers of autogenerated docs.

[hbagdi]

Done