grpc-node icon indicating copy to clipboard operation
grpc-node copied to clipboard

fix: support authority overrides in the DNS resolver

Open gkampitakis opened this issue 1 year ago • 5 comments

As per issue https://github.com/grpc/grpc-node/issues/2775, this pr adds support for overriding the authority on DNS resolver.

gkampitakis avatar Jun 18 '24 17:06 gkampitakis

CLA Signed

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

gkampitakis avatar Jun 27 '24 12:06 gkampitakis

@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

gkampitakis avatar Jul 02 '24 08:07 gkampitakis

This feature is fantastic. Thanks so much for implementing it @gkampitakis! :partying_face:

MartinLoeper avatar Jul 03 '24 11:07 MartinLoeper

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 avatar Jul 03 '24 17:07 murgatroid99

@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?

gkampitakis avatar Jul 05 '24 13:07 gkampitakis

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.

murgatroid99 avatar Jul 08 '24 18:07 murgatroid99

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.

gkampitakis avatar Jul 09 '24 16:07 gkampitakis

Can't tell if the failing tests are related to my changes 🤔

gkampitakis avatar Jul 09 '24 20:07 gkampitakis

@murgatroid99 Thank you for the patience and all the help to bring it in a good state 🙇

gkampitakis avatar Jul 10 '24 17:07 gkampitakis

This is now out in version 1.11.0.

murgatroid99 avatar Jul 15 '24 21:07 murgatroid99