-
Notifications
You must be signed in to change notification settings - Fork 648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support authority overrides in the DNS resolver #2776
fix: support authority overrides in the DNS resolver #2776
Conversation
d77e777
to
6278da6
Compare
I have updated per recommendation the |
@murgatroid99 code has been update per your feedback, thank you. I have also tested locally and it does behave as expected. Are there any
|
This feature is fantastic. Thanks so much for implementing it @gkampitakis! 🥳 |
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 |
@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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests that try to resolve localhost
need to be skipped when using the alternate resolver. You can do this by changing the test declaration to use function(done) {...}
and adding this to the test body:
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
this.skip();
}
@@ -460,7 +460,7 @@ describe('Name Resolver', () => { | |||
} | |||
}, | |||
}, | |||
{} | |||
{ 'grpc.dns_min_time_between_resolutions_ms': 2000 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary. This test expects DNS resolution to fail, and this option only takes effect if it succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this test I am getting error without this value. I assume it allow us to "retry" resolution faster but probably it's another issue.
Uncaught AssertionError [ERR_ASSERTION]: resultCount 1 !== 2
+ expected - actual
-1
+2
at Timeout._onTimeout (test/test-resolver.ts:477:16)
at listOnTimeout (node:internal/timers:569:17)
at processTimers (node:internal/timers:512:7)
Also getting timeouts on, should I increase the value?
Should resolve gRPC interop servers:
Error: Timeout of 4000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/george.kampitakis/Dev/grpc-node/packages/grpc-js/build/test/test-resolver.js)
at listOnTimeout (node:internal/timers:569:17)
at processTimers (node:internal/timers:512:7)
Can't tell if the failing tests are related to my changes 🤔 |
@@ -81,6 +81,21 @@ const runTests = checkTask(() => { | |||
); | |||
}); | |||
|
|||
const test = gulp.series(install, copyTestFixtures, runTests); | |||
const testWithAlternativeResolver = checkTask(() => { | |||
process.env.GRPC_NODE_USE_ALTERNATIVE_RESOLVER = 'true'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think this is impacting other test jobs. I think this alternative will work: create a new file called something like alternate-resolver.env
that just says GRPC_NODE_USE_ALTERNATIVE_RESOLVER=true
and add an option in the mocha
command a couple of lines down that says nodeOption: 'env-file=alternate-resolver.env'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are referring to this, I don't see a nodeOption
or something similar from the documentation.
mocha({
reporter: 'mocha-jenkins-reporter',
require: ['ts-node/register'],
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what I'm talking about. The gulp-mocha documentation says
Options are passed directly to the mocha binary, so you can use any its command-line options in a camelCased form.
The mocha command line options documentation includes this line:
-n, --node-option Node or V8 option (no leading "--") [array]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think currently it will be hard to tell whether that actually worked, so it would be a good idea to add something like this in the DNS resolver constructor:
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
trace('Using alternative DNS resolver.');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the node --env-file
was added only on node 20 though right? will it work with the CI running on 14 and 16?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that probably won't work. So, here's another alternative: add another target that just sets process.env.GRPC_NODE_USE_ALTERNATIVE_RESOLVER = 'false';
and add it to the end of the test
series.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I don't think this quite works because at the top level, the test suites are run in parallel. I think changing this line to run in series should fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this seems to be in a good state now. Thank you for contributing this, and for working with me to get the tests working.
@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. |
As per issue #2775, this pr adds support for overriding the authority on DNS resolver.