Merge pull request #2679 from murgatroid99/node_explicit_insecure_channel

Make insecure channel/stub construction explicit

src/node/examples/perf_test.js

@@ -41,7 +41,8 @@ var interop_server = require('../interop/interop_server.js');
 function runTest(iterations, callback) {
   var testServer = interop_server.getServer(0, false);
   testServer.server.listen();
-  var client = new testProto.TestService('localhost:' + testServer.port);
+  var client = new testProto.TestService('localhost:' + testServer.port,
+                                         grpc.Credentials.createInsecure());
 
   function runIterations(finish) {
     var start = process.hrtime();

src/node/examples/qps_test.js

@@ -61,7 +61,8 @@ var interop_server = require('../interop/interop_server.js');
 function runTest(concurrent_calls, seconds, callback) {
   var testServer = interop_server.getServer(0, false);
   testServer.server.listen();
-  var client = new testProto.TestService('localhost:' + testServer.port);
+  var client = new testProto.TestService('localhost:' + testServer.port,
+                                         grpc.Credentials.createInsecure());
 
   var warmup_num = 100;
 

src/node/examples/route_guide_client.js

@@ -40,7 +40,8 @@ var path = require('path');
 var _ = require('lodash');
 var grpc = require('..');
 var examples = grpc.load(__dirname + '/route_guide.proto').examples;
-var client = new examples.RouteGuide('localhost:50051');
+var client = new examples.RouteGuide('localhost:50051',
+                                     grpc.Credentials.createInsecure());
 
 var COORD_FACTOR = 1e7;
 

src/node/examples/stock_client.js

@@ -38,7 +38,8 @@ var examples = grpc.load(__dirname + '/stock.proto').examples;
  * This exports a client constructor for the Stock service. The usage looks like
  *
  * var StockClient = require('stock_client.js');
- * var stockClient = new StockClient(server_address);
+ * var stockClient = new StockClient(server_address,
+ *                                   grpc.Credentials.createInsecure());
  * stockClient.getLastTradePrice({symbol: 'GOOG'}, function(error, response) {
  *   console.log(error || response);
  * });

src/node/ext/channel.cc

@@ -98,31 +98,30 @@ NAN_METHOD(Channel::New) {
 
   if (args.IsConstructCall()) {
     if (!args[0]->IsString()) {
-      return NanThrowTypeError("Channel expects a string and an object");
+      return NanThrowTypeError(
+          "Channel expects a string, a credential and an object");
     }
     grpc_channel *wrapped_channel;
     // Owned by the Channel object
     NanUtf8String *host = new NanUtf8String(args[0]);
     NanUtf8String *host_override = NULL;
-    if (args[1]->IsUndefined()) {
+    grpc_credentials *creds;
+    if (!Credentials::HasInstance(args[1])) {
+      return NanThrowTypeError(
+          "Channel's second argument must be a credential");
+    }
+    Credentials *creds_object = ObjectWrap::Unwrap<Credentials>(
+        args[1]->ToObject());
+    creds = creds_object->GetWrappedCredentials();
+    grpc_channel_args *channel_args_ptr;
+    if (args[2]->IsUndefined()) {
+      channel_args_ptr = NULL;
       wrapped_channel = grpc_insecure_channel_create(**host, NULL);
-    } else if (args[1]->IsObject()) {
-      grpc_credentials *creds = NULL;
-      Handle<Object> args_hash(args[1]->ToObject()->Clone());
+    } else if (args[2]->IsObject()) {
+      Handle<Object> args_hash(args[2]->ToObject()->Clone());
       if (args_hash->HasOwnProperty(NanNew(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG))) {
         host_override = new NanUtf8String(args_hash->Get(NanNew(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)));
       }
-      if (args_hash->HasOwnProperty(NanNew("credentials"))) {
-        Handle<Value> creds_value = args_hash->Get(NanNew("credentials"));
-        if (!Credentials::HasInstance(creds_value)) {
-          return NanThrowTypeError(
-              "credentials arg must be a Credentials object");
-        }
-        Credentials *creds_object =
-            ObjectWrap::Unwrap<Credentials>(creds_value->ToObject());
-        creds = creds_object->GetWrappedCredentials();
-        args_hash->Delete(NanNew("credentials"));
-      }
       Handle<Array> keys(args_hash->GetOwnPropertyNames());
       grpc_channel_args channel_args;
       channel_args.num_args = keys->Length();
@@ -149,16 +148,19 @@ NAN_METHOD(Channel::New) {
           return NanThrowTypeError("Arg values must be strings");
         }
       }
-      if (creds == NULL) {
-        wrapped_channel = grpc_insecure_channel_create(**host, &channel_args);
-      } else {
-        wrapped_channel =
-            grpc_secure_channel_create(creds, **host, &channel_args);
-      }
-      free(channel_args.args);
+      channel_args_ptr = &channel_args;
     } else {
       return NanThrowTypeError("Channel expects a string and an object");
     }
+    if (creds == NULL) {
+      wrapped_channel = grpc_insecure_channel_create(**host, channel_args_ptr);
+    } else {
+      wrapped_channel =
+          grpc_secure_channel_create(creds, **host, channel_args_ptr);
+    }
+    if (channel_args_ptr != NULL) {
+      free(channel_args_ptr->args);
+    }
     Channel *channel;
     if (host_override == NULL) {
       channel = new Channel(wrapped_channel, host);
@@ -168,8 +170,8 @@ NAN_METHOD(Channel::New) {
     channel->Wrap(args.This());
     NanReturnValue(args.This());
   } else {
-    const int argc = 2;
-    Local<Value> argv[argc] = {args[0], args[1]};
+    const int argc = 3;
+    Local<Value> argv[argc] = {args[0], args[1], args[2]};
     NanReturnValue(constructor->GetFunction()->NewInstance(argc, argv));
   }
 }

src/node/ext/credentials.cc

@@ -81,6 +81,8 @@ void Credentials::Init(Handle<Object> exports) {
            NanNew<FunctionTemplate>(CreateGce)->GetFunction());
   ctr->Set(NanNew("createIam"),
            NanNew<FunctionTemplate>(CreateIam)->GetFunction());
+  ctr->Set(NanNew("createInsecure"),
+           NanNew<FunctionTemplate>(CreateInsecure)->GetFunction());
   constructor = new NanCallback(ctr);
   exports->Set(NanNew("Credentials"), ctr);
 }
@@ -92,9 +94,6 @@ bool Credentials::HasInstance(Handle<Value> val) {
 
 Handle<Value> Credentials::WrapStruct(grpc_credentials *credentials) {
   NanEscapableScope();
-  if (credentials == NULL) {
-    return NanEscapeScope(NanNull());
-  }
   const int argc = 1;
   Handle<Value> argv[argc] = {
     NanNew<External>(reinterpret_cast<void *>(credentials))};
@@ -128,7 +127,11 @@ NAN_METHOD(Credentials::New) {
 
 NAN_METHOD(Credentials::CreateDefault) {
   NanScope();
-  NanReturnValue(WrapStruct(grpc_google_default_credentials_create()));
+  grpc_credentials *creds = grpc_google_default_credentials_create();
+  if (creds == NULL) {
+    NanReturnNull();
+  }
+  NanReturnValue(WrapStruct(creds));
 }
 
 NAN_METHOD(Credentials::CreateSsl) {
@@ -152,9 +155,12 @@ NAN_METHOD(Credentials::CreateSsl) {
     return NanThrowTypeError(
         "createSSl's third argument must be a Buffer if provided");
   }
-
-  NanReturnValue(WrapStruct(grpc_ssl_credentials_create(
-      root_certs, key_cert_pair.private_key == NULL ? NULL : &key_cert_pair)));
+  grpc_credentials *creds = grpc_ssl_credentials_create(
+      root_certs, key_cert_pair.private_key == NULL ? NULL : &key_cert_pair);
+  if (creds == NULL) {
+    NanReturnNull();
+  }
+  NanReturnValue(WrapStruct(creds));
 }
 
 NAN_METHOD(Credentials::CreateComposite) {
@@ -169,13 +175,21 @@ NAN_METHOD(Credentials::CreateComposite) {
   }
   Credentials *creds1 = ObjectWrap::Unwrap<Credentials>(args[0]->ToObject());
   Credentials *creds2 = ObjectWrap::Unwrap<Credentials>(args[1]->ToObject());
-  NanReturnValue(WrapStruct(grpc_composite_credentials_create(
-      creds1->wrapped_credentials, creds2->wrapped_credentials)));
+  grpc_credentials *creds = grpc_composite_credentials_create(
+      creds1->wrapped_credentials, creds2->wrapped_credentials);
+  if (creds == NULL) {
+    NanReturnNull();
+  }
+  NanReturnValue(WrapStruct(creds));
 }
 
 NAN_METHOD(Credentials::CreateGce) {
   NanScope();
-  NanReturnValue(WrapStruct(grpc_compute_engine_credentials_create()));
+  grpc_credentials *creds = grpc_compute_engine_credentials_create();
+  if (creds == NULL) {
+    NanReturnNull();
+  }
+  NanReturnValue(WrapStruct(creds));
 }
 
 NAN_METHOD(Credentials::CreateIam) {
@@ -188,8 +202,17 @@ NAN_METHOD(Credentials::CreateIam) {
   }
   NanUtf8String auth_token(args[0]);
   NanUtf8String auth_selector(args[1]);
-  NanReturnValue(
-      WrapStruct(grpc_iam_credentials_create(*auth_token, *auth_selector)));
+  grpc_credentials *creds = grpc_iam_credentials_create(*auth_token,
+                                                       *auth_selector);
+  if (creds == NULL) {
+    NanReturnNull();
+  }
+  NanReturnValue(WrapStruct(creds));
+}
+
+NAN_METHOD(Credentials::CreateInsecure) {
+  NanScope();
+  NanReturnValue(WrapStruct(NULL));
 }
 
 }  // namespace node

src/node/ext/credentials.h

@@ -68,6 +68,7 @@ class Credentials : public ::node::ObjectWrap {
   static NAN_METHOD(CreateGce);
   static NAN_METHOD(CreateFake);
   static NAN_METHOD(CreateIam);
+  static NAN_METHOD(CreateInsecure);
   static NanCallback *constructor;
   // Used for typechecking instances of this javascript class
   static v8::Persistent<v8::FunctionTemplate> fun_tpl;

src/node/interop/interop_client.js

@@ -397,6 +397,7 @@ var test_cases = {
 function runTest(address, host_override, test_case, tls, test_ca, done) {
   // TODO(mlumish): enable TLS functionality
   var options = {};
+  var creds;
   if (tls) {
     var ca_path;
     if (test_ca) {
@@ -405,13 +406,14 @@ function runTest(address, host_override, test_case, tls, test_ca, done) {
       ca_path = process.env.SSL_CERT_FILE;
     }
     var ca_data = fs.readFileSync(ca_path);
-    var creds = grpc.Credentials.createSsl(ca_data);
-    options.credentials = creds;
+    creds = grpc.Credentials.createSsl(ca_data);
     if (host_override) {
       options['grpc.ssl_target_name_override'] = host_override;
     }
+  } else {
+    creds = grpc.Credentials.createInsecure();
   }
-  var client = new testProto.TestService(address, options);
+  var client = new testProto.TestService(address, creds, options);
 
   test_cases[test_case](client, done);
 }

src/node/src/client.js

@@ -531,11 +531,13 @@ exports.makeClientConstructor = function(methods, serviceName) {
    * Create a client with the given methods
    * @constructor
    * @param {string} address The address of the server to connect to
+   * @param {grpc.Credentials} credentials Credentials to use to connect
+   *     to the server
    * @param {Object} options Options to pass to the underlying channel
    * @param {function(string, Object, function)=} updateMetadata function to
    *     update the metadata for each request
    */
-  function Client(address, options, updateMetadata) {
+  function Client(address, credentials, options, updateMetadata) {
     if (!updateMetadata) {
       updateMetadata = function(uri, metadata, callback) {
         callback(null, metadata);
@@ -545,7 +547,7 @@ exports.makeClientConstructor = function(methods, serviceName) {
       options = {};
     }
     options['grpc.primary_user_agent'] = 'grpc-node/' + version;
-    this.channel = new grpc.Channel(address, options);
+    this.channel = new grpc.Channel(address, credentials, options);
     this.server_address = address.replace(/\/$/, '');
     this.auth_uri = this.server_address + '/' + serviceName;
     this.updateMetadata = updateMetadata;

src/node/test/call_test.js

@@ -48,6 +48,8 @@ function getDeadline(timeout_secs) {
   return deadline;
 }
 
+var insecureCreds = grpc.Credentials.createInsecure();
+
 describe('call', function() {
   var channel;
   var server;
@@ -55,7 +57,7 @@ describe('call', function() {
     server = new grpc.Server();
     var port = server.addHttp2Port('localhost:0');
     server.start();
-    channel = new grpc.Channel('localhost:' + port);
+    channel = new grpc.Channel('localhost:' + port, insecureCreds);
   });
   after(function() {
     server.shutdown();
@@ -82,7 +84,7 @@ describe('call', function() {
          });
        });
     it('should fail with a closed channel', function() {
-      var local_channel = new grpc.Channel('hostname');
+      var local_channel = new grpc.Channel('hostname', insecureCreds);
       local_channel.close();
       assert.throws(function() {
         new grpc.Call(channel, 'method');

src/node/test/channel_test.js

@@ -36,11 +36,13 @@
 var assert = require('assert');
 var grpc = require('bindings')('grpc.node');
 
+var insecureCreds = grpc.Credentials.createInsecure();
+
 describe('channel', function() {
   describe('constructor', function() {
     it('should require a string for the first argument', function() {
       assert.doesNotThrow(function() {
-        new grpc.Channel('hostname');
+        new grpc.Channel('hostname', insecureCreds);
       });
       assert.throws(function() {
         new grpc.Channel();
@@ -49,38 +51,49 @@ describe('channel', function() {
         new grpc.Channel(5);
       });
     });
-    it('should accept an object for the second parameter', function() {
+    it('should require a credential for the second argument', function() {
       assert.doesNotThrow(function() {
-        new grpc.Channel('hostname', {});
+        new grpc.Channel('hostname', insecureCreds);
       });
       assert.throws(function() {
         new grpc.Channel('hostname', 5);
       });
+      assert.throws(function() {
+        new grpc.Channel('hostname');
+      });
+    });
+    it('should accept an object for the third argument', function() {
+      assert.doesNotThrow(function() {
+        new grpc.Channel('hostname', insecureCreds, {});
+      });
+      assert.throws(function() {
+        new grpc.Channel('hostname', insecureCreds, 'abc');
+      });
     });
     it('should only accept objects with string or int values', function() {
       assert.doesNotThrow(function() {
-        new grpc.Channel('hostname', {'key' : 'value'});
+        new grpc.Channel('hostname', insecureCreds,{'key' : 'value'});
       });
       assert.doesNotThrow(function() {
-        new grpc.Channel('hostname', {'key' : 5});
+        new grpc.Channel('hostname', insecureCreds, {'key' : 5});
       });
       assert.throws(function() {
-        new grpc.Channel('hostname', {'key' : null});
+        new grpc.Channel('hostname', insecureCreds, {'key' : null});
       });
       assert.throws(function() {
-        new grpc.Channel('hostname', {'key' : new Date()});
+        new grpc.Channel('hostname', insecureCreds, {'key' : new Date()});
       });
     });
   });
   describe('close', function() {
     it('should succeed silently', function() {
-      var channel = new grpc.Channel('hostname', {});
+      var channel = new grpc.Channel('hostname', insecureCreds, {});
       assert.doesNotThrow(function() {
         channel.close();
       });
     });
     it('should be idempotent', function() {
-      var channel = new grpc.Channel('hostname', {});
+      var channel = new grpc.Channel('hostname', insecureCreds, {});
       assert.doesNotThrow(function() {
         channel.close();
         channel.close();
@@ -89,7 +102,7 @@ describe('channel', function() {
   });
   describe('getTarget', function() {
     it('should return a string', function() {
-      var channel = new grpc.Channel('localhost', {});
+      var channel = new grpc.Channel('localhost', insecureCreds, {});
       assert.strictEqual(typeof channel.getTarget(), 'string');
     });
   });

src/node/test/end_to_end_test.js

@@ -57,6 +57,8 @@ function multiDone(done, count) {
   };
 }
 
+var insecureCreds = grpc.Credentials.createInsecure();
+
 describe('end-to-end', function() {
   var server;
   var channel;
@@ -64,7 +66,7 @@ describe('end-to-end', function() {
     server = new grpc.Server();
     var port_num = server.addHttp2Port('0.0.0.0:0');
     server.start();
-    channel = new grpc.Channel('localhost:' + port_num);
+    channel = new grpc.Channel('localhost:' + port_num, insecureCreds);
   });
   after(function() {
     server.shutdown();

src/node/test/health_test.js

@@ -56,7 +56,8 @@ describe('Health Checking', function() {
   before(function() {
     var port_num = healthServer.bind('0.0.0.0:0');
     healthServer.start();
-    healthClient = new health.Client('localhost:' + port_num);
+    healthClient = new health.Client('localhost:' + port_num,
+                                     grpc.Credentials.createInsecure());
   });
   after(function() {
     healthServer.shutdown();

src/node/test/math_client_test.js

@@ -53,7 +53,8 @@ describe('Math client', function() {
   before(function(done) {
     var port_num = server.bind('0.0.0.0:0');
     server.start();
-    math_client = new math.Math('localhost:' + port_num);
+    math_client = new math.Math('localhost:' + port_num,
+                                grpc.Credentials.createInsecure());
     done();
   });
   after(function() {

src/node/test/surface_test.js

@@ -124,7 +124,7 @@ describe('Echo service', function() {
     });
     var port = server.bind('localhost:0');
     var Client = surface_client.makeProtobufClientConstructor(echo_service);
-    client = new Client('localhost:' + port);
+    client = new Client('localhost:' + port, grpc.Credentials.createInsecure());
     server.start();
   });
   after(function() {
@@ -169,7 +169,8 @@ describe('Generic client and server', function() {
       var port = server.bind('localhost:0');
       server.start();
       var Client = grpc.makeGenericClientConstructor(string_service_attrs);
-      client = new Client('localhost:' + port);
+      client = new Client('localhost:' + port,
+                          grpc.Credentials.createInsecure());
     });
     after(function() {
       server.shutdown();
@@ -216,7 +217,7 @@ describe('Echo metadata', function() {
     });
     var port = server.bind('localhost:0');
     var Client = surface_client.makeProtobufClientConstructor(test_service);
-    client = new Client('localhost:' + port);
+    client = new Client('localhost:' + port, grpc.Credentials.createInsecure());
     server.start();
   });
   after(function() {
@@ -337,7 +338,7 @@ describe('Other conditions', function() {
     });
     port = server.bind('localhost:0');
     var Client = surface_client.makeProtobufClientConstructor(test_service);
-    client = new Client('localhost:' + port);
+    client = new Client('localhost:' + port, grpc.Credentials.createInsecure());
     server.start();
   });
   after(function() {
@@ -382,7 +383,8 @@ describe('Other conditions', function() {
       };
       var Client = surface_client.makeClientConstructor(test_service_attrs,
                                                         'TestService');
-      misbehavingClient = new Client('localhost:' + port);
+      misbehavingClient = new Client('localhost:' + port,
+                                     grpc.Credentials.createInsecure());
     });
     it('should respond correctly to a unary call', function(done) {
       misbehavingClient.unary(badArg, function(err, data) {
@@ -602,7 +604,7 @@ describe('Cancelling surface client', function() {
     });
     var port = server.bind('localhost:0');
     var Client = surface_client.makeProtobufClientConstructor(mathService);
-    client = new Client('localhost:' + port);
+    client = new Client('localhost:' + port, grpc.Credentials.createInsecure());
     server.start();
   });
   after(function() {