Skip to content
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

Merged
merged 12 commits into from
Jul 10, 2024
5 changes: 5 additions & 0 deletions doc/environment_variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,8 @@ can be set.
- INFO - log INFO and ERROR message
- ERROR - log only errors (default)
- NONE - won't log any

* GRPC_NODE_USE_ALTERNATIVE_RESOLVER
Allows changing dns resolve behavior and parse DNS server authority as described in https://github.com/grpc/grpc/blob/master/doc/naming.md
- true - use alternative resolver
- false - use default resolver (default)
2 changes: 1 addition & 1 deletion gulpfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const nativeTestOnly = gulp.parallel(healthCheck.test);

const nativeTest = gulp.series(build, nativeTestOnly);

const testOnly = gulp.parallel(jsCore.test, nativeTestOnly, protobuf.test, jsXds.test, reflection.test);
const testOnly = gulp.series(jsCore.test, nativeTestOnly, protobuf.test, jsXds.test, reflection.test);

const test = gulp.series(build, testOnly, internalTest.test);

Expand Down
23 changes: 22 additions & 1 deletion packages/grpc-js/gulpfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,27 @@ const runTests = checkTask(() => {
);
});

const test = gulp.series(install, copyTestFixtures, runTests);
const testWithAlternativeResolver = checkTask(() => {
process.env.GRPC_NODE_USE_ALTERNATIVE_RESOLVER = 'true';
Copy link
Member

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'.

Copy link
Contributor Author

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'],
  })

Copy link
Member

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]

Copy link
Member

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.');
}

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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.

return gulp.src(`${outDir}/test/test-resolver.js`).pipe(
mocha({
reporter: 'mocha-jenkins-reporter',
require: ['ts-node/register'],
})
);
});

const resetEnv = () => {
process.env.GRPC_NODE_USE_ALTERNATIVE_RESOLVER = 'false';
return Promise.resolve();
};

const test = gulp.series(
install,
copyTestFixtures,
runTests,
testWithAlternativeResolver,
resetEnv
);

export { install, lint, clean, cleanAll, compile, test };
19 changes: 19 additions & 0 deletions packages/grpc-js/src/environment.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright 2024 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

export const GRPC_NODE_USE_ALTERNATIVE_RESOLVER =
(process.env.GRPC_NODE_USE_ALTERNATIVE_RESOLVER ?? 'false') === 'true';
73 changes: 54 additions & 19 deletions packages/grpc-js/src/resolver-dns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import {
registerResolver,
registerDefaultScheme,
} from './resolver';
import * as dns from 'dns';
import * as util from 'util';
import { promises as dns } from 'node:dns';
import { extractAndSelectServiceConfig, ServiceConfig } from './service-config';
import { Status } from './constants';
import { StatusObject } from './call-interface';
Expand All @@ -33,6 +32,7 @@ import { GrpcUri, uriToString, splitHostPort } from './uri-parser';
import { isIPv6, isIPv4 } from 'net';
import { ChannelOptions } from './channel-options';
import { BackoffOptions, BackoffTimeout } from './backoff-timeout';
import { GRPC_NODE_USE_ALTERNATIVE_RESOLVER } from './environment';

const TRACER_NAME = 'dns_resolver';

Expand All @@ -47,9 +47,6 @@ export const DEFAULT_PORT = 443;

const DEFAULT_MIN_TIME_BETWEEN_RESOLUTIONS_MS = 30_000;

const resolveTxtPromise = util.promisify(dns.resolveTxt);
const dnsLookupPromise = util.promisify(dns.lookup);

/**
* Resolver implementation that handles DNS names and IP addresses.
*/
Expand All @@ -63,7 +60,7 @@ class DnsResolver implements Resolver {
* Failures are handled by the backoff timer.
*/
private readonly minTimeBetweenResolutionsMs: number;
private pendingLookupPromise: Promise<dns.LookupAddress[]> | null = null;
private pendingLookupPromise: Promise<TcpSubchannelAddress[]> | null = null;
private pendingTxtPromise: Promise<string[][]> | null = null;
private latestLookupResult: Endpoint[] | null = null;
private latestServiceConfig: ServiceConfig | null = null;
Expand All @@ -76,12 +73,17 @@ class DnsResolver implements Resolver {
private isNextResolutionTimerRunning = false;
private isServiceConfigEnabled = true;
private returnedIpResult = false;
private alternativeResolver = new dns.Resolver();

constructor(
private target: GrpcUri,
private listener: ResolverListener,
channelOptions: ChannelOptions
) {
trace('Resolver constructed for target ' + uriToString(target));
if (target.authority) {
this.alternativeResolver.setServers([target.authority]);
}
const hostPort = splitHostPort(target.path);
if (hostPort === null) {
this.ipResult = null;
Expand Down Expand Up @@ -185,11 +187,7 @@ class DnsResolver implements Resolver {
* revert to an effectively blank one. */
this.latestLookupResult = null;
const hostname: string = this.dnsHostname;
/* We lookup both address families here and then split them up later
* because when looking up a single family, dns.lookup outputs an error
* if the name exists but there are no records for that family, and that
* error is indistinguishable from other kinds of errors */
this.pendingLookupPromise = dnsLookupPromise(hostname, { all: true });
this.pendingLookupPromise = this.lookup(hostname);
this.pendingLookupPromise.then(
addressList => {
if (this.pendingLookupPromise === null) {
Expand All @@ -198,17 +196,12 @@ class DnsResolver implements Resolver {
this.pendingLookupPromise = null;
this.backoff.reset();
this.backoff.stop();
const subchannelAddresses: TcpSubchannelAddress[] = addressList.map(
addr => ({ host: addr.address, port: +this.port! })
);
this.latestLookupResult = subchannelAddresses.map(address => ({
this.latestLookupResult = addressList.map(address => ({
addresses: [address],
}));
const allAddressesString: string =
'[' +
subchannelAddresses
.map(addr => addr.host + ':' + addr.port)
.join(',') +
addressList.map(addr => addr.host + ':' + addr.port).join(',') +
']';
trace(
'Resolved addresses for target ' +
Expand Down Expand Up @@ -253,7 +246,7 @@ class DnsResolver implements Resolver {
/* We handle the TXT query promise differently than the others because
* the name resolution attempt as a whole is a success even if the TXT
* lookup fails */
this.pendingTxtPromise = resolveTxtPromise(hostname);
this.pendingTxtPromise = this.resolveTxt(hostname);
this.pendingTxtPromise.then(
txtRecord => {
if (this.pendingTxtPromise === null) {
Expand Down Expand Up @@ -302,6 +295,48 @@ class DnsResolver implements Resolver {
}
}

private async lookup(hostname: string): Promise<TcpSubchannelAddress[]> {
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
trace('Using alternative DNS resolver.');

const records = await Promise.allSettled([
gkampitakis marked this conversation as resolved.
Show resolved Hide resolved
this.alternativeResolver.resolve4(hostname),
this.alternativeResolver.resolve6(hostname),
]);

if (records.every(result => result.status === 'rejected')) {
throw new Error((records[0] as PromiseRejectedResult).reason);
}

return records
.reduce<string[]>((acc, result) => {
return result.status === 'fulfilled'
? [...acc, ...result.value]
: acc;
}, [])
.map(addr => ({
host: addr,
port: +this.port!,
}));
}

/* We lookup both address families here and then split them up later
* because when looking up a single family, dns.lookup outputs an error
* if the name exists but there are no records for that family, and that
* error is indistinguishable from other kinds of errors */
const addressList = await dns.lookup(hostname, { all: true });
return addressList.map(addr => ({ host: addr.address, port: +this.port! }));
}

private async resolveTxt(hostname: string): Promise<string[][]> {
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
trace('Using alternative DNS resolver.');
return this.alternativeResolver.resolveTxt(hostname);
}

return dns.resolveTxt(hostname);
}

private startNextResolutionTimer() {
clearTimeout(this.nextResolutionTimer);
this.nextResolutionTimer = setTimeout(() => {
Expand Down
16 changes: 13 additions & 3 deletions packages/grpc-js/test/test-resolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
subchannelAddressEqual,
} from '../src/subchannel-address';
import { parseUri, GrpcUri } from '../src/uri-parser';
import { GRPC_NODE_USE_ALTERNATIVE_RESOLVER } from '../src/environment';

function hasMatchingAddress(
endpointList: Endpoint[],
Expand All @@ -55,7 +56,10 @@ describe('Name Resolver', () => {
describe('DNS Names', function () {
// For some reason DNS queries sometimes take a long time on Windows
this.timeout(4000);
it('Should resolve localhost properly', done => {
it('Should resolve localhost properly', function (done) {
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
this.skip();
}
const target = resolverManager.mapUriDefaultScheme(
parseUri('localhost:50051')!
)!;
Expand All @@ -82,7 +86,10 @@ describe('Name Resolver', () => {
const resolver = resolverManager.createResolver(target, listener, {});
resolver.updateResolution();
});
it('Should default to port 443', done => {
it('Should default to port 443', function (done) {
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
this.skip();
}
const target = resolverManager.mapUriDefaultScheme(
parseUri('localhost')!
)!;
Expand Down Expand Up @@ -402,7 +409,10 @@ describe('Name Resolver', () => {
const resolver2 = resolverManager.createResolver(target2, listener, {});
resolver2.updateResolution();
});
it('should not keep repeating successful resolutions', done => {
it('should not keep repeating successful resolutions', function (done) {
if (GRPC_NODE_USE_ALTERNATIVE_RESOLVER) {
this.skip();
}
const target = resolverManager.mapUriDefaultScheme(
parseUri('localhost')!
)!;
Expand Down