Revert #45073 c-ares: make qcache_max_ttl configurable - #46577
Revert #45073 c-ares: make qcache_max_ttl configurable#46577yanjunxiang-google wants to merge 3 commits into
Conversation
|
CC @envoyproxy/runtime-guard-changes: FYI only for changes made to |
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
9bbff04 to
a202c4a
Compare
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
441ffe0 to
a44fa3c
Compare
|
/assign @andy-fong @yanavlasov |
|
@andy-fong cannot be assigned to this issue. |
|
@yanjunxiang-google Would you accept a PR that change the reloadable feature default to false (assuming this is related to #42836 I mentioned). As a complete revert is a breaking API change and we do rely on that feature and not affected by 42836. |
ah... I saw you comment about the race. Yes, I didn't consider the dns_filter, I was only thinking about the main thread dns lookup when talking about the race. I was also thinking, when there is no custom c-ares config, envoy default to shared the resolver as well. So, I thought that was safe. |
|
ok. It's been a while, the "default sharing" I remember was actually at the dns cluster level, so that doesn't impact dns_filter like the changes I introduced. |
|
My original goal was to try to mitigate the DNS "DDoS" from envoy in a large env with many services on the same Host but different ports. Given this problem and the others I have brought up in the original PR, I might need to go back to the drawing board. So, I am ok with the revert but what's the best approach for the 1.39 branch? |
|
@andy-fong thanks for your quickly response on this PR. Where disable the runtime flag can also avoid the issue, however, I guess you might need some time to re-work the PR for the issues you mentioned above. Let's revert to keep it simple. Once you are ready, please feel free to raise a new PR. We need to revert in 1.39 branch as well. |
@andy-fong could you please elaborate why "a complete revert is a breaking API change"? |
…t-45073-qcache-ttl Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Because there was a new field in api/envoy/extensions/network/dns_resolver/cares/v3/cares_dns_resolver.proto, if someone is using it (from 1.39), envoy now will reject it. Also, "12" should be reserved now to avoid future issue when a future new field is added, it will re-use 12 which could be a potential problem if someone from 1.39.0 using it and upgrade directly to this new version that re-use field 12. Chance is very slim but the risk is there. |
|
@andy-fong emm, the concern is that if you disable the runtime guard, then you will have to adding a notes there to mention it should not be enabled due to the race issue. BTW, are you going to be able to fix the thread issue quickly? If you are, then it is fine to me to just disable the runtime guard. |
|
For this race, I have an experimental fix that create a new createSharedDnsResolver() function that used by DNS Cluster (still gated by the same runtime flag). Everything else will continue to use createDnsResolver() to get the old behavior back. I also just learnt that (through AI), envoy has some tests support having multiple main_threads, so will still add a mutex and add the dispatcher as part of the key to isolate the resolver. If that sounds acceptable, I can create a PR for that and also create 2 PR's (main and 1.39) to turn the feature off in the mean time. |
|
@yanjunxiang-google This PR turns off the feature in 1.39 branch for now: https://github.com/envoyproxy/envoy/pull/46585/changes I can create one for main as well if that's what we want to do. |
|
@yanjunxiang-google I ended up taking a different approach and I think it's much cleaner than adding mutex. Please take a look at #46657 |
3 participants
Revert #45073 c-ares: make qcache_max_ttl configurable
Please check: #45073 (comment) for details.