From 726e7453f5ffc2bfeeb834a76d5c73db35a470c4 Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 09:18:35 -0700 Subject: [PATCH 1/6] grpc-js: Fix proxy + URI parsing bugs --- packages/grpc-js/src/channel.ts | 14 +++++----- packages/grpc-js/src/http_proxy.ts | 33 ++++++++++++++---------- packages/grpc-js/src/resolver-dns.ts | 2 -- packages/grpc-js/src/resolver.ts | 38 +++++++++++++++------------- 4 files changed, 45 insertions(+), 42 deletions(-) diff --git a/packages/grpc-js/src/channel.ts b/packages/grpc-js/src/channel.ts index c067a6929..85df3cf36 100644 --- a/packages/grpc-js/src/channel.ts +++ b/packages/grpc-js/src/channel.ts @@ -33,7 +33,7 @@ import { FilterStackFactory } from './filter-stack'; import { CallCredentialsFilterFactory } from './call-credentials-filter'; import { DeadlineFilterFactory } from './deadline-filter'; import { CompressionFilterFactory } from './compression-filter'; -import { getDefaultAuthority } from './resolver'; +import { getDefaultAuthority, mapUriDefaultScheme } from './resolver'; import { ServiceConfig, validateServiceConfig } from './service-config'; import { trace, log } from './logging'; import { SubchannelAddress } from './subchannel'; @@ -170,20 +170,18 @@ export class ChannelImplementation implements Channel { if (originalTargetUri === null) { throw new Error(`Could not parse target name "${target}"`); } + /* This ensures that the target has a scheme that is registered with the + * resolver */ + const defaultSchemeMapResult = mapUriDefaultScheme(originalTargetUri); if (this.options['grpc.default_authority']) { this.defaultAuthority = this.options['grpc.default_authority'] as string; } else { - this.defaultAuthority = getDefaultAuthority(originalTargetUri); + this.defaultAuthority = getDefaultAuthority(defaultSchemeMapResult); } - const proxyMapResult = mapProxyName(originalTargetUri, options); + const proxyMapResult = mapProxyName(defaultSchemeMapResult, options); this.target = proxyMapResult.target; this.options = Object.assign({}, this.options, proxyMapResult.extraOptions); - const targetUri = parseUri(target); - if (targetUri === null) { - throw new Error(`Could not parse target name "${target}"`); - } - this.target = targetUri; /* The global boolean parameter to getSubchannelPool has the inverse meaning to what * the grpc.use_local_subchannel_pool channel option means. */ this.subchannelPool = getSubchannelPool( diff --git a/packages/grpc-js/src/http_proxy.ts b/packages/grpc-js/src/http_proxy.ts index 2721055fc..18c0e115b 100644 --- a/packages/grpc-js/src/http_proxy.ts +++ b/packages/grpc-js/src/http_proxy.ts @@ -29,6 +29,7 @@ import { } from './subchannel'; import { ChannelOptions } from './channel-options'; import { GrpcUri, parseUri, splitHostPort, uriToString } from './uri-parser'; +import { URL } from 'url'; const TRACER_NAME = 'proxy'; @@ -60,30 +61,31 @@ function getProxyInfo(): ProxyInfo { } else { return {}; } - const proxyUrl = parseUri(proxyEnv); - if (proxyUrl === null) { + let proxyUrl: URL; + try { + proxyUrl = new URL(proxyEnv); + } catch (e) { log(LogVerbosity.ERROR, `cannot parse value of "${envVar}" env var`); return {}; } - if (proxyUrl.scheme !== 'http') { + if (proxyUrl.protocol !== 'http:') { log( LogVerbosity.ERROR, - `"${proxyUrl.scheme}" scheme not supported in proxy URI` + `"${proxyUrl.protocol}" scheme not supported in proxy URI` ); return {}; } - const splitPath = proxyUrl.path.split('@'); - let host: string; let userCred: string | null = null; - if (splitPath.length === 2) { - log(LogVerbosity.INFO, 'userinfo found in proxy URI'); - userCred = splitPath[0]; - host = splitPath[1]; - } else { - host = proxyUrl.path; + if (proxyUrl.username) { + if (proxyUrl.password) { + log(LogVerbosity.INFO, 'userinfo found in proxy URI'); + userCred = `${proxyUrl.username}:${proxyUrl.password}`; + } else { + userCred = proxyUrl.username; + } } const result: ProxyInfo = { - address: host, + address: proxyUrl.host, }; if (userCred) { result.creds = userCred; @@ -145,7 +147,10 @@ export function mapProxyName( extraOptions['grpc.http_connect_creds'] = proxyInfo.creds; } return { - target: { path: proxyInfo.address }, + target: { + scheme: 'dns', + path: proxyInfo.address + }, extraOptions: extraOptions, }; } diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 58cc6dc84..565e2daf4 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -18,7 +18,6 @@ import { Resolver, ResolverListener, registerResolver, - registerDefaultResolver, } from './resolver'; import * as dns from 'dns'; import * as util from 'util'; @@ -281,7 +280,6 @@ class DnsResolver implements Resolver { */ export function setup(): void { registerResolver('dns', DnsResolver); - registerDefaultResolver(DnsResolver); } export interface DnsUrl { diff --git a/packages/grpc-js/src/resolver.ts b/packages/grpc-js/src/resolver.ts index e75f178ae..6af996a2e 100644 --- a/packages/grpc-js/src/resolver.ts +++ b/packages/grpc-js/src/resolver.ts @@ -74,7 +74,7 @@ export interface ResolverConstructor { } const registeredResolvers: { [scheme: string]: ResolverConstructor } = {}; -let defaultResolver: ResolverConstructor | null = null; +let defaultScheme: string | null = null; /** * Register a resolver class to handle target names prefixed with the `prefix` @@ -95,8 +95,8 @@ export function registerResolver( * any registered prefix. * @param resolverClass */ -export function registerDefaultResolver(resolverClass: ResolverConstructor) { - defaultResolver = resolverClass; +export function registerDefaultScheme(scheme: string) { + defaultScheme = scheme; } /** @@ -112,18 +112,10 @@ export function createResolver( if (target.scheme !== undefined && target.scheme in registeredResolvers) { return new registeredResolvers[target.scheme](target, listener); } else { - if (defaultResolver !== null) { - /* If the scheme does not correspond to a registered scheme, we assume - * that the whole thing is the path, and the scheme was pulled out - * incorrectly. For example, it is valid to parse "localhost:80" as - * having a scheme of "localhost" and a path of 80, but that is not - * how the resolver should see it */ - return new defaultResolver({ path: uriToString(target) }, listener); - } + throw new Error( + `No resolver could be created for target ${uriToString(target)}` + ); } - throw new Error( - `No resolver could be created for target ${uriToString(target)}` - ); } /** @@ -135,12 +127,22 @@ export function getDefaultAuthority(target: GrpcUri): string { if (target.scheme !== undefined && target.scheme in registeredResolvers) { return registeredResolvers[target.scheme].getDefaultAuthority(target); } else { - if (defaultResolver !== null) { - // See comment in createResolver for why we handle the target like this - return defaultResolver.getDefaultAuthority({ path: uriToString(target) }); + throw new Error(`Invalid target ${uriToString(target)}`); + } +} + +export function mapUriDefaultScheme(target: GrpcUri): GrpcUri { + if (target.scheme === undefined || !(target.scheme in registeredResolvers)) { + if (defaultScheme !== null) { + return { + scheme: defaultScheme, + path: uriToString(target) + }; + } else { + throw new Error(`Invalid target ${uriToString(target)}`); } } - throw new Error(`Invalid target ${uriToString(target)}`); + return target; } export function registerAll() { From ec4eb785fe0f9f38cdc21f3e0b9c453540adff9f Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 09:26:51 -0700 Subject: [PATCH 2/6] Actually register 'dns' as the default scheme --- packages/grpc-js/src/resolver-dns.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/grpc-js/src/resolver-dns.ts b/packages/grpc-js/src/resolver-dns.ts index 565e2daf4..1e84fe9bd 100644 --- a/packages/grpc-js/src/resolver-dns.ts +++ b/packages/grpc-js/src/resolver-dns.ts @@ -18,6 +18,7 @@ import { Resolver, ResolverListener, registerResolver, + registerDefaultScheme, } from './resolver'; import * as dns from 'dns'; import * as util from 'util'; @@ -280,6 +281,7 @@ class DnsResolver implements Resolver { */ export function setup(): void { registerResolver('dns', DnsResolver); + registerDefaultScheme('dns'); } export interface DnsUrl { From 23e2353ea05fd3cd738ac5a160d4957c2d547bdf Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 09:58:34 -0700 Subject: [PATCH 3/6] Update tests and add new ones --- packages/grpc-js/src/channel.ts | 3 +++ packages/grpc-js/src/resolver-uds.ts | 1 - packages/grpc-js/src/resolver.ts | 5 ++-- packages/grpc-js/test/test-resolver.ts | 30 ++++++++++++------------ packages/grpc-js/test/test-uri-parser.ts | 18 ++++++++++++++ 5 files changed, 39 insertions(+), 18 deletions(-) diff --git a/packages/grpc-js/src/channel.ts b/packages/grpc-js/src/channel.ts index 85df3cf36..764c7a699 100644 --- a/packages/grpc-js/src/channel.ts +++ b/packages/grpc-js/src/channel.ts @@ -173,6 +173,9 @@ export class ChannelImplementation implements Channel { /* This ensures that the target has a scheme that is registered with the * resolver */ const defaultSchemeMapResult = mapUriDefaultScheme(originalTargetUri); + if (defaultSchemeMapResult === null) { + throw new Error(`Could not find a default scheme for target name "${target}"`); + } if (this.options['grpc.default_authority']) { this.defaultAuthority = this.options['grpc.default_authority'] as string; } else { diff --git a/packages/grpc-js/src/resolver-uds.ts b/packages/grpc-js/src/resolver-uds.ts index c147f6370..759cb233e 100644 --- a/packages/grpc-js/src/resolver-uds.ts +++ b/packages/grpc-js/src/resolver-uds.ts @@ -18,7 +18,6 @@ import { Resolver, ResolverListener, registerResolver, - registerDefaultResolver, } from './resolver'; import { SubchannelAddress } from './subchannel'; import { GrpcUri } from './uri-parser'; diff --git a/packages/grpc-js/src/resolver.ts b/packages/grpc-js/src/resolver.ts index 6af996a2e..7d6d1ad5a 100644 --- a/packages/grpc-js/src/resolver.ts +++ b/packages/grpc-js/src/resolver.ts @@ -131,15 +131,16 @@ export function getDefaultAuthority(target: GrpcUri): string { } } -export function mapUriDefaultScheme(target: GrpcUri): GrpcUri { +export function mapUriDefaultScheme(target: GrpcUri): GrpcUri | null { if (target.scheme === undefined || !(target.scheme in registeredResolvers)) { if (defaultScheme !== null) { return { scheme: defaultScheme, + authority: undefined, path: uriToString(target) }; } else { - throw new Error(`Invalid target ${uriToString(target)}`); + return null; } } return target; diff --git a/packages/grpc-js/test/test-resolver.ts b/packages/grpc-js/test/test-resolver.ts index 0ff40ac19..d525d97d2 100644 --- a/packages/grpc-js/test/test-resolver.ts +++ b/packages/grpc-js/test/test-resolver.ts @@ -32,7 +32,7 @@ describe('Name Resolver', () => { resolverManager.registerAll(); }); it('Should resolve localhost properly', done => { - const target = parseUri('localhost:50051')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('localhost:50051')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -67,7 +67,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should default to port 443', done => { - const target = parseUri('localhost')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('localhost')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -102,7 +102,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should correctly represent an ipv4 address', done => { - const target = parseUri('1.2.3.4')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('1.2.3.4')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -129,7 +129,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should correctly represent an ipv6 address', done => { - const target = parseUri('::1')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('::1')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -156,7 +156,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should correctly represent a bracketed ipv6 address', done => { - const target = parseUri('[::1]:50051')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('[::1]:50051')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -183,7 +183,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should resolve a public address', done => { - const target = parseUri('example.com')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('example.com')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -203,7 +203,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should resolve a name with multiple dots', done => { - const target = parseUri('loopback4.unittest.grpc.io')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('loopback4.unittest.grpc.io')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -232,7 +232,7 @@ describe('Name Resolver', () => { /* TODO(murgatroid99): re-enable this test, once we can get the IPv6 result * consistently */ it.skip('Should resolve a DNS name to an IPv6 address', done => { - const target = parseUri('loopback6.unittest.grpc.io')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('loopback6.unittest.grpc.io')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -259,7 +259,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should resolve a DNS name to IPv4 and IPv6 addresses', done => { - const target = parseUri('loopback46.unittest.grpc.io')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('loopback46.unittest.grpc.io')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -290,7 +290,7 @@ describe('Name Resolver', () => { it('Should resolve a name with a hyphen', done => { /* TODO(murgatroid99): Find or create a better domain name to test this with. * This is just the first one I found with a hyphen. */ - const target = parseUri('network-tools.com')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('network-tools.com')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -311,8 +311,8 @@ describe('Name Resolver', () => { }); it('Should resolve gRPC interop servers', done => { let completeCount = 0; - const target1 = parseUri('grpc-test.sandbox.googleapis.com')!; - const target2 = parseUri('grpc-test4.sandbox.googleapis.com')!; + const target1 = resolverManager.mapUriDefaultScheme(parseUri('grpc-test.sandbox.googleapis.com')!)!; + const target2 = resolverManager.mapUriDefaultScheme(parseUri('grpc-test4.sandbox.googleapis.com')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -339,7 +339,7 @@ describe('Name Resolver', () => { }); describe('UDS Names', () => { it('Should handle a relative Unix Domain Socket name', done => { - const target = parseUri('unix:socket')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('unix:socket')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -363,7 +363,7 @@ describe('Name Resolver', () => { resolver.updateResolution(); }); it('Should handle an absolute Unix Domain Socket name', done => { - const target = parseUri('unix:///tmp/socket')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('unix:///tmp/socket')!)!; const listener: resolverManager.ResolverListener = { onSuccessfulResolution: ( addressList: SubchannelAddress[], @@ -400,7 +400,7 @@ describe('Name Resolver', () => { } it('Should return the correct authority if a different resolver has been registered', () => { - const target = parseUri('other:name')!; + const target = resolverManager.mapUriDefaultScheme(parseUri('other:name')!)!; console.log(target); resolverManager.registerResolver('other', OtherResolver); diff --git a/packages/grpc-js/test/test-uri-parser.ts b/packages/grpc-js/test/test-uri-parser.ts index 75aa82bfe..d04cae539 100644 --- a/packages/grpc-js/test/test-uri-parser.ts +++ b/packages/grpc-js/test/test-uri-parser.ts @@ -17,6 +17,7 @@ import * as assert from 'assert'; import * as uriParser from '../src/uri-parser'; +import * as resolver from '../src/resolver'; describe('URI Parser', function(){ describe('parseUri', function() { @@ -37,6 +38,23 @@ describe('URI Parser', function(){ }); } }); + + describe('parseUri + mapUriDefaultScheme', function() { + const expectationList: {target: string, result: uriParser.GrpcUri | null}[] = [ + {target: 'localhost', result: {scheme: 'dns', authority: undefined, path: 'localhost'}}, + {target: 'localhost:80', result: {scheme: 'dns', authority: undefined, path: 'localhost:80'}}, + {target: 'dns:localhost', result: {scheme: 'dns', authority: undefined, path: 'localhost'}}, + {target: 'dns:///localhost', result: {scheme: 'dns', authority: '', path: 'localhost'}}, + {target: 'dns://authority/localhost', result: {scheme: 'dns', authority: 'authority', path: 'localhost'}}, + {target: 'unix:socket', result: {scheme: 'unix', authority: undefined, path: 'socket'}}, + {target: 'bad:path', result: {scheme: 'dns', authority: undefined, path: 'bad:path'}} + ]; + for (const {target, result} of expectationList) { + it(target, function() { + assert.deepStrictEqual(resolver.mapUriDefaultScheme(uriParser.parseUri(target) ?? {path: 'null'}), result); + }) + } + }); describe('splitHostPort', function() { const expectationList: {path: string, result: uriParser.HostPort | null}[] = [ From 0b522b2289e8cde8263d6e60f61b8e4de6ee0ae5 Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 10:04:40 -0700 Subject: [PATCH 4/6] Bump grpc-js to 1.0.1 --- packages/grpc-js/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json index 4551d1de8..6a3556359 100644 --- a/packages/grpc-js/package.json +++ b/packages/grpc-js/package.json @@ -1,6 +1,6 @@ { "name": "@grpc/grpc-js", - "version": "1.0.0", + "version": "1.0.1", "description": "gRPC Library for Node - pure JS implementation", "homepage": "https://grpc.io/", "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js", From b6846f0709509e77523fa34b539c7840b5d5e83f Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 10:32:58 -0700 Subject: [PATCH 5/6] Update server to handle default schemes --- packages/grpc-js/src/server.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 95aa68a24..c8b8f2b22 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -45,7 +45,7 @@ import { } from './server-call'; import { ServerCredentials } from './server-credentials'; import { ChannelOptions } from './channel-options'; -import { createResolver, ResolverListener } from './resolver'; +import { createResolver, ResolverListener, mapUriDefaultScheme } from './resolver'; import { log } from './logging'; import { SubchannelAddress, @@ -226,10 +226,14 @@ export class Server { throw new TypeError('callback must be a function'); } - const portUri = parseUri(port); - if (portUri === null) { + const initialPortUri = parseUri(port); + if (initialPortUri === null) { throw new Error(`Could not parse port "${port}"`); } + const portUri = mapUriDefaultScheme(initialPortUri); + if (portUri === null) { + throw new Error(`Could not get a default scheme for port "${port}"`); + } const serverOptions: http2.ServerOptions = {}; if ('grpc.max_concurrent_streams' in this.options) { From e0533363ec4882b33d64e7932d75be392e9320d5 Mon Sep 17 00:00:00 2001 From: Michael Lumish <mlumish@google.com> Date: Tue, 21 Apr 2020 10:55:06 -0700 Subject: [PATCH 6/6] Fix "other" resolver test --- packages/grpc-js/test/test-resolver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js/test/test-resolver.ts b/packages/grpc-js/test/test-resolver.ts index d525d97d2..d2a85fc47 100644 --- a/packages/grpc-js/test/test-resolver.ts +++ b/packages/grpc-js/test/test-resolver.ts @@ -400,9 +400,9 @@ describe('Name Resolver', () => { } it('Should return the correct authority if a different resolver has been registered', () => { + resolverManager.registerResolver('other', OtherResolver); const target = resolverManager.mapUriDefaultScheme(parseUri('other:name')!)!; console.log(target); - resolverManager.registerResolver('other', OtherResolver); const authority = resolverManager.getDefaultAuthority(target); assert.equal(authority, 'other');