fix: support authority overrides in the DNS resolver
As per issue https://github.com/grpc/grpc-node/issues/2775, this pr adds support for overriding the authority on DNS resolver.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: gkampitakis / name: Georgios Kampitakis (3aaf0c6e52e14149269131bb58b6c5b3eb2a6848, 201595c665df212aa6557e2a4b97af7849dc80e1, 717725caf13c8bf62d5e887d4f453dc85b59b7e9, a9aea4557012c2a05b175148648b264fef0a92c0, 7f27b322450d369e471542eaf8b6d7e99a6a26f6, 6278da6aec2fde629459e3f256dcb21575da108c, 2a9f8f4c40c35069020982f7d601be709bb224dc, c9fc8b2e0ce85a1e006d37ae5c0065d9a601ca46, cb58fbc5b021f5bdae9d6def8baabacb0c8ae457, c3d073b0cc62ca825d89f3e9a0dc2b431240dcd8, e2a93d188a279407b5bd8a63953209117184ebab, f0afe6946f8f7b874d1632d96475390131322a40)
I have updated per recommendation the lookup method to resolve explicitly both ipv4 and ipv6. I see there are some unit tests but are skipped, what would be your suggestion for that matter? @murgatroid99
@murgatroid99 code has been update per your feedback, thank you. I have also tested locally and it does behave as expected.
Are there any
- tests that need to be added/updated
- docs added/updated
This feature is fantastic. Thanks so much for implementing it @gkampitakis! :partying_face:
For documentation, this file should be updated with the new environment variable.
In terms of testing, ideally there would be at least some automated testing for this code path. I think running the entire test suite is impractical, because I believe the alternate resolver can't resolve the address localhost, which many of the tests use. This file has the most relevant tests, but it also has a few that won't work because of the localhost issue. So, I think it would make sense to test with just that file, with those localhost tests skipped if the environment variable is set.
@murgatroid99 I don't see an example of how to test this code path (using an env variable).
To make this testable with no weird monkey patching the import I would have to rework the code and pass this env variable as a parameter but again I don't see any good pattern in the code base? Any suggestions for tackling the testing here?
OK, for testing I was thinking that a gulpfile target could be added that runs just that one test file with the environment variable set. Then the actual test target would run both tests in sequence.
OK, for testing I was thinking that a gulpfile target could be added that runs just that one test file with the environment variable set. Then the actual test target would run both tests in sequence.
That makes sense. I am not very familiar with this setup. Thanks for the advice. I have added the new test but on my machine the timeouts are quite flaky, but didn't want to "touch" them without understanding what's happening there.
Can't tell if the failing tests are related to my changes 🤔
@murgatroid99 Thank you for the patience and all the help to bring it in a good state 🙇
This is now out in version 1.11.0.