Merge pull request #1375 from murgatroid99/grpc-js_proxy_structured_uri_fixes

grpc-js: Fix interactions between proxy code and new URI parsing

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",

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,21 @@ 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 (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 {
-      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(

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);
+  let proxyUrl: URL;
-  if (proxyUrl === null) {
+  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) {
+  if (proxyUrl.username) {
-    log(LogVerbosity.INFO, 'userinfo found in proxy URI');
+    if (proxyUrl.password) {
-    userCred = splitPath[0];
+      log(LogVerbosity.INFO, 'userinfo found in proxy URI');
-    host = splitPath[1];
+      userCred = `${proxyUrl.username}:${proxyUrl.password}`;
-  } else {
+    } else {
-    host = proxyUrl.path;
+      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,
   };
 }

packages/grpc-js/src/resolver-dns.ts

@@ -18,7 +18,7 @@ import {
   Resolver,
   ResolverListener,
   registerResolver,
-  registerDefaultResolver,
+  registerDefaultScheme,
 } from './resolver';
 import * as dns from 'dns';
 import * as util from 'util';
@@ -281,7 +281,7 @@ class DnsResolver implements Resolver {
  */
 export function setup(): void {
   registerResolver('dns', DnsResolver);
-  registerDefaultResolver(DnsResolver);
+  registerDefaultScheme('dns');
 }
 
 export interface DnsUrl {

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

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) {
+export function registerDefaultScheme(scheme: string) {
-  defaultResolver = resolverClass;
+  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) {
+    throw new Error(
-      /* If the scheme does not correspond to a registered scheme, we assume
+      `No resolver could be created for target ${uriToString(target)}`
-       * 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)}`
-  );
 }
 
 /**
@@ -135,12 +127,23 @@ export function getDefaultAuthority(target: GrpcUri): string {
   if (target.scheme !== undefined && target.scheme in registeredResolvers) {
     return registeredResolvers[target.scheme].getDefaultAuthority(target);
   } else {
-    if (defaultResolver !== null) {
+    throw new Error(`Invalid target ${uriToString(target)}`);
-      // See comment in createResolver for why we handle the target like this
+  }
-      return defaultResolver.getDefaultAuthority({ path: uriToString(target) });
+}
+
+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 {
+      return null;
     }
   }
-  throw new Error(`Invalid target ${uriToString(target)}`);
+  return target;
 }
 
 export function registerAll() {

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);
+    const initialPortUri = parseUri(port);
-    if (portUri === null) {
+    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) {

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 target1 = resolverManager.mapUriDefaultScheme(parseUri('grpc-test.sandbox.googleapis.com')!)!;
-      const target2 = parseUri('grpc-test4.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,9 +400,9 @@ describe('Name Resolver', () => {
     }
 
     it('Should return the correct authority if a different resolver has been registered', () => {
-      const target = parseUri('other:name')!;
-      console.log(target);
       resolverManager.registerResolver('other', OtherResolver);
+      const target = resolverManager.mapUriDefaultScheme(parseUri('other:name')!)!;
+      console.log(target);
 
       const authority = resolverManager.getDefaultAuthority(target);
       assert.equal(authority, 'other');

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}[] = [