DNS Ping Proposal

[bretambrose]

aws/aws-iot-device-sdk-python-v2#399

A set of changes designed to allow us to safely disable the DNS "pinging" functionality on a per-connection basis, while keeping the changes uncoupled to the particulars of the conditionally-undesired behavior and leaving default behavior unchanged.

• Moves host resolution time from system clock to high res clock
• Removes DNS host listener functionality completely. It is no longer used as of Multiple Bucket Support aws-c-s3#136
• Un-entangle max-ttl from the DNS refresh frequency.
• • Add support for customizing resolve frequency.
• • Rework inner resolver thread exit condition to reflect these changes.
• Host resolution configuration can now be overridden at connection kick off time
• Make DNS TTL test more reliable by using a mock clock

Follow-up PRs in aws-c-http and aws-c-mqtt add host resolution override support to their public connection establishment options:

• aws-c-http - Support for host resolution override per connection aws-c-http#434
• aws-c-mqtt - Resolution override aws-c-mqtt#269

Aws-c-mqtt PR also uses the host resolution override to, by default, disable all unsolicited DNS queries when using MQTT, either directly or over websockets.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov-commenter]

Codecov Report

Patch coverage: 94.44% and project coverage change: -0.24 ⚠️

Comparison is base (8c24d7e) 79.33% compared to head (82a444e) 79.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
- Coverage   79.33%   79.09%   -0.24%     
==========================================
  Files          25       25              
  Lines        5646     5478     -168     
==========================================
- Hits         4479     4333     -146     
+ Misses       1167     1145      -22     

Impacted Files	Coverage Δ
source/channel_bootstrap.c	72.74% <75.00%> (-0.08%)	⬇️
source/host_resolver.c	90.23% <100.00%> (+0.83%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TwistedTwigleg]

Looks good to me. 👍

[DmitriyMusatkin]

Removes DNS host listener functionality completely. It is no longer used since S3 endpoint DNS now returns large groups of records

Is this assertion true? from testing it seems that MVA DNS for s3 now returns a max of 8 ips and for 100 gbps nics min amount of ips we want is around 10. Will this cause perf degradation since we are not acquiring enough ips?

[DmitriyMusatkin]

i know this is not a new constant introduced by this code, but why 30? a lot of aws services ive seen use at least 60. are we refreshing dns too much with default config?

[bretambrose]

I don't recall what, if any, reasons were behind the original choice.

[bretambrose]

Removes DNS host listener functionality completely. It is no longer used since S3 endpoint DNS now returns large groups of records

Is this assertion true? from testing it seems that MVA DNS for s3 now returns a max of 8 ips and for 100 gbps nics min amount of ips we want is around 10. Will this cause perf degradation since we are not acquiring enough ips?

Assertion was incorrect about root reason. host listener functionality was removed far earlier here: awslabs/aws-c-s3#136

Will update PR commentary, but s3 client will be unaffected

[JonathanHenson]

discussed in my office hours, We can move the host resolver config init function to return an instance via value since it's trivially copyable and constructable.

The cvar wakeup can go outside the mutex lock/unlock. Otherwise looks good to me. Fix and ship.