dns: reinitialize c-ares channel on ARES_ETIMEOUT

[Najji]

Commit Message: dns: reinitialize c-ares channel on ARES_ETIMEOUT

Additional Description::
Add ARES_ETIMEOUT to the list of error conditions that trigger c-ares channel reinitialization. When DNS queries timeout, the c-ares channel can enter a broken state where UDP sockets become unusable and subsequent queries continue to fail. This is similar to ARES_ECONNREFUSED and other connection errors that already trigger reinitialization.

Risk Level: Low

Testing: Updated existing DnsImplZeroTimeoutTest::Timeout test to verify channel reinitialization after timeout.

Docs Changes: N/A

Release Notes: Added to changelogs/current.yaml

Platform Specific Features: N/A

[repokitteh-read-only]

Hi @Najji, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #41718 was opened by Najji.

see: more, trace.

[agrawroh]

AFAIK DNS timeouts might have nothing to do with channel state. DNS could timeout due to intermittent network and reinitializing the channel on every timeout could cause constant churning of the UDP sockets.

Could you use max_udp_channel_duration to periodically reinit the channel?

[Najji]

AFAIK DNS timeouts might have nothing to do with channel state. DNS could timeout due to intermittent network and reinitializing the channel on every timeout could cause constant churning of the UDP sockets.

Could you use max_udp_channel_duration to periodically reinit the channel?

Hey @agrawroh, thanks for the review! We've seen this pattern in production where c-ares DNS timeouts are followed by requests to stale IPs even hours later. tcpdump showed DNS resolution from the pod was working fine (returning correct IPs), but Envoy never updated. Only a restart fixed it.

Could you help me understand the rationale for not including ARES_ETIMEOUT, considering it is thrown after retries. It also looks semantically similar to the other errors in the list like ARES_ESERVFAIL and ARES_ECONNREFUSED

max_udp_channel_duration may help auto recover but it will still be reactive/ after serving stale ip addresses for some time.

[agrawroh]

AFAIK DNS timeouts might have nothing to do with channel state. DNS could timeout due to intermittent network and reinitializing the channel on every timeout could cause constant churning of the UDP sockets.
Could you use max_udp_channel_duration to periodically reinit the channel?

Hey @agrawroh, thanks for the review! We've seen this pattern in production where c-ares DNS timeouts are followed by requests to stale IPs even hours later. tcpdump showed DNS resolution from the pod was working fine (returning correct IPs), but Envoy never updated. Only a restart fixed it.

Could you help me understand the rationale for not including ARES_ETIMEOUT, considering it is thrown after retries. It also looks semantically similar to the other errors in the list like ARES_ESERVFAIL and ARES_ECONNREFUSED

max_udp_channel_duration may help auto recover but it will still be reactive/ after serving stale ip addresses for some time.

Wasn't I clear enough when I said that DNS could timeout due to intermittent network issues as well? How would you differentiate the timeouts happening due to sporadic network issues vs. the ones due to broken channel?

[Najji]

AFAIK DNS timeouts might have nothing to do with channel state. DNS could timeout due to intermittent network and reinitializing the channel on every timeout could cause constant churning of the UDP sockets.
Could you use max_udp_channel_duration to periodically reinit the channel?

Hey @agrawroh, thanks for the review! We've seen this pattern in production where c-ares DNS timeouts are followed by requests to stale IPs even hours later. tcpdump showed DNS resolution from the pod was working fine (returning correct IPs), but Envoy never updated. Only a restart fixed it.
Could you help me understand the rationale for not including ARES_ETIMEOUT, considering it is thrown after retries. It also looks semantically similar to the other errors in the list like ARES_ESERVFAIL and ARES_ECONNREFUSED
max_udp_channel_duration may help auto recover but it will still be reactive/ after serving stale ip addresses for some time.

Wasn't I clear enough when I said that DNS could timeout due to intermittent network issues as well? How would you differentiate the timeouts happening due to sporadic network issues vs. the ones due to broken channel?

Thanks for your response. Agreed it's not differentiating but the existing errors can also be due to intermittent network issues, yet we reinit on them. Is there an inconsistency here, or is there differentiation logic I'm missing?

The cost trade-off I'm seeing is reinitializing on timeouts (after retries) vs. the production impact we observed (hours of stale IPs despite DNS working).

[yanavlasov]

If there is a concern that timeouts can cause channel churn, what do you think about adding a configuration option to re-init the channel on timeouts? In this way operators that face this problem in production can enable it without impacting other deployments.

/wait-any

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #41718 was synchronize by Najji.

see: more, trace.

[Najji]

/retest

[Najji]

If there is a concern that timeouts can cause channel churn, what do you think about adding a configuration option to re-init the channel on timeouts? In this way operators that face this problem in production can enable it without impacting other deployments.

Thanks @yanavlasov for the suggestion! I've implemented it as a configurable option (opt-in, defaults to false for backward compatibility). Let me know what you think.

[abeyad]

If the UDP sockets timeout, wouldn't c-ares try to use a new UDP socket next time it needs to send a DNS query? If you could explain a bit more in detail about the exact flow and socket state that leads to needing to re-initialize the channel, that would be helpful, thanks!

[Najji]

Hi @abeyad,

This PR builds on work from #9899 to address #4543. We're observing a situation where after DNS timeouts occur, Envoy enters a bad state and continues using stale IP addresses (despite packet capture showing correct IPs are being returned). This is only fixed after we restart Envoy.

ares_reinit() will help achieve the same effect without needing a full restart. The other error codes (ARES_ECONNREFUSED, ARES_ESERVFAIL, etc.) already trigger reinit under the same assumption that the channel state may be broken. The c-ares project has acknowledged similar channel state issues: c-ares/c-ares#301.

Let me know what you think!

[abeyad]

Hi @Najji , I'm not opposed to the option, as it seems we do similar restarting of the c-ares channel for ARES_ECONNREFUSED, as long as it is false by default.

It seems like the issue is in the connection managed by c-ares though, so I'm guessing there is a bug in c-ares that doesn't deal with timed-out connections properly. But given we don't control c-ares, I think it's fine to add this option to get around the issue.

Thanks!

[abeyad]

We've seen this pattern in production where c-ares DNS timeouts are followed by requests to stale IPs even hours later.

@Najji do you mean the DNS query times out, then a subsequent query to the DNS server sends the query to a stale IP address?

[Najji]

We've seen this pattern in production where c-ares DNS timeouts are followed by requests to stale IPs even hours later.

@Najji do you mean the DNS query times out, then a subsequent query to the DNS server sends the query to a stale IP address?

@abeyad No, the DNS server responses are correct. However, after timeouts occur Envoy stops updating IP lists based on the responses as per our observation.

[yanavlasov]

@Najji thanks for making this change. I will wait for approvals from @agrawroh and @abeyad and will submit.

/wait-any

[abeyad]

/lgtm api

thanks @Najji !

[agrawroh]

LGTM as well! Thanks for putting it behind a flag.

[agrawroh]

Merging it as it's already approved by a Senior Maintainer.

[Najji]

Thanks for the review everyone!