shared client: Fix global timeout not being respected

[gandro]

This fixes a bug where the shared client did not respect c.Client.Timeout, resulting in DNS requests timing out too soon.

A per Go docs, Client.Timeout is supposed to also apply to the write timeout. This is implemented in the regular client by using Client.getTimeoutForRequest to determine the actual timeout. This commit adapts the shared client code to do the same.

Ref: cilium/cilium#31999

[gandro]

Not sure who owns this repo, so I've just added the few people who have contributed in the past

[marseel]

Shouldn't that be something like this?
https://github.com/cilium/cilium/blob/94f6553f5b79383b561e8630bdf40bd824769ede/vendor/github.com/cilium/dns/client.go#L250-L251

[gandro]

Shouldn't that be something like this? https://github.com/cilium/cilium/blob/94f6553f5b79383b561e8630bdf40bd824769ede/vendor/github.com/cilium/dns/client.go#L250-L251

Not sure what you are suggesting. Using context.WithDeadline instead of WithTimeout? I don't think it makes a difference? Internally, they are identical.

[marseel]

Ah, I was referring to the fact that we are adding two timeouts there - one for write and the other for read, so in theory we should do the same here I think, no?

[marseel]

ok, nvm. This only applies to writing requests so it's fine.

[gandro]

Ah, I was referring to the fact that we are adding two timeouts there - one for write and the other for read, so in theory we should do the same here I think, no?

I'm honestly not sure how to implement the read timeout with the shared client. The logic is very convoluted.

The way I understand the code, the timeout created here:

dns/shared_client.go

Line 294 in ee3124a

timeout := c.getTimeoutForRequest(c.Client.writeTimeout())

Is passed via req.ctx to SendContext here:

dns/shared_client.go

Line 241 in ee3124a

err := client.SendContext(req.ctx, req.msg, conn, start)

Which then actually splits the passed in context into write and read timeouts. But I guess given the way it's implemented, if the write timeout is lower than the read timeout, then it will use our timeout created here as the read timeout too.

[gandro]

I did do some local testing and at least with this patch, the library now does respect the 10 second timeout Cilium configures, rather than falling back on the default dnsTimeout of two seconds which it did before. There might still be corner cases where the timeouts are messed up because of all the sharing, but since this already is an improvement over the status quo I'll merge as is.

[marseel]

There might still be corner cases where the timeouts are messed up because of all the sharing, but since this already is an improvement over the status quo I'll merge as is.

yup, makes sense, that is my understanding as well.