Feature custom endpoint #2548

rmanyari · 2017-08-21T17:37:57Z

Support for custom endpoints at service instantiation. This first PR covers the following services:

bigquery
bigtable
gce
datastore
dns
language

Custom enpoints can be defined using environment variables: GOOGLE_CLOUD_[SERVICE_NAME]_ENDPOINT or the options.{apiEndpoint,servicePath} options

Issue: #1630

Signed-off-by: Rodrigo Manyari [email protected]

packages/bigquery/src/index.js

  var config = {
-    baseUrl: 'https://www.googleapis.com/bigquery/v2',
+    baseUrl: baseInfo.apiEndpoint + basePath,
+    customEndpoint: baseInfo.customEndpoint,


stephenplusplus · 2017-08-21T19:18:50Z

Thank you very, very much for this!

rmanyari · 2017-08-21T20:54:19Z

I signed it!

stephenplusplus · 2017-08-21T22:25:07Z

googlebot is pretty feature-incomplete. @lukesneeringer can you make sure this is okay with the CLA? (re: #2548 (comment))

lukesneeringer · 2017-08-24T15:46:48Z

Yea, I can check on googlebot while you two get tests working. :-)

lukesneeringer · 2017-08-24T15:57:58Z

Update: @rmanyari, our system still thinks you have not signed the CLA, and in the interest of my not being swarmed by mobs of angry lawyers, this is a prerequisite.

The link is here: https://cla.developers.google.com/

If you are pretty sure you already signed it, the most likely problem is that the email address you used to do so does not match the email address on your commits (git config user.email).

If you need more help, let us know and I will try to get you in touch with someone who can help more.

rmanyari · 2017-08-25T12:53:51Z

(Update) - Nevermind, googlebot clarified things ;)
@lukesneeringer - I looked again at that link, it looks like I'm already covered by a corporate agreement from Two Sigma. I also just signed an individual one using my twosigma.com address. If it's not too much to ask, can you check again if I'm still not covered. Me too I have no interested whatsoever in long email threads with lawyers...

googlebot · 2017-08-25T12:53:57Z

CLAs look good, thanks!

packages/common-grpc/src/service.js

@@ -202,11 +221,14 @@ function GrpcService(config, options) {

  Object.keys(protoServices).forEach(function(name) {
    var protoConfig = protoServices[name];
-    var service = self.loadProtoFile_(protoConfig, config);
+    var service = GrpcService.prototype.loadProtoFile_


stephenplusplus

Overall, it's perfect. I have just two overall thoughts:

I think we should wait until all APIs can adopt the new style before merging
The tests in each API to be sure Service is working isn't necessary as long as the Service tests are sufficient and we know that we properly called it from the API constructor

packages/bigquery/test/index.js

      var baseUrl = 'https://www.googleapis.com/bigquery/v2';
-      assert.strictEqual(calledWith.baseUrl, baseUrl);
+      assert.strictEqual(bq.baseUrl, baseUrl);


packages/bigquery/test/index.js

      assert.deepEqual(calledWith.scopes, [
        'https://www.googleapis.com/auth/bigquery'
      ]);
      assert.deepEqual(calledWith.packageJson, require('../package.json'));
    });
+
+    it('should not be considered custom endpoint if default', function() {


packages/bigquery/test/index.js

+      assert.strictEqual(bq.customEndpoint, false);
+    });
+
+    it('should use options.apiEndpoint if defined', function() {


packages/bigquery/test/index.js

+      assert.strictEqual(bq.customEndpoint, true);
+    });
+
+    it('should use GOOGLE_CLOUD_BIGQUERY_ENDPOINT if defined', function() {


packages/bigtable/src/index.js

    protosDir: path.resolve(__dirname, '../protos'),
    protoServices: {
      Bigtable: {
-        baseUrl: baseUrl,
+        baseUrl: defaultApiEndpoint,


packages/bigtable/test/index.js

+      });
+    });
+
+    it('should trim the protocol in custom endpoint', function() {


packages/common/src/service.js

@@ -52,9 +62,24 @@ function Service(config, options) {
    email: options.email
  });

+  // Some services have not been moved to the


packages/common-grpc/src/service.js

@@ -202,11 +221,14 @@ function GrpcService(config, options) {

  Object.keys(protoServices).forEach(function(name) {
    var protoConfig = protoServices[name];
-    var service = self.loadProtoFile_(protoConfig, config);
+    var service = GrpcService.prototype.loadProtoFile_


packages/common-grpc/src/service.js

@@ -168,6 +176,17 @@ function GrpcService(config, options) {
    return global.GCLOUD_SANDBOX_ENV;
  }

+  if (config.defaultApiEndpoint) {


packages/common/src/util.js

+ * @return {object=} base - Object containg the baseUrl and a boolean
+ *     property indicating whether the baseUrl is a customEndpoint or not.
+ * */
+function determineBaseUrl(options, environmentVariables,


packages/common/src/service.js

@@ -52,9 +57,20 @@ function Service(config, options) {
    email: options.email
  });

+  if (!config.baseUrl) {


rmanyari · 2017-08-30T19:12:33Z

I wrapped up the remaining services and grpcServices. What's left are the services that use gapic:

dlp
language (partially done)
logging-*
monitoring
spanner (partially done)
speech
video-intelligence
vision

To properly support custom endpoint in those services, gax-nodejs needs to have the notion of insecureCrendentials, the same way it's done in commonGrpc. At the moment it forces the calls to go do the auth roundtrip which is inconsistent with how we're doing things everywhere else (why would you want Google auth when going to a custom endpoint?!). I can open a PR in gax-nodejs to get that fixed and then wrap this up. If we really want this done I can override _getCredentials but that's just looking for trouble.

Thoughts?

stephenplusplus · 2017-08-30T19:17:21Z

Thanks for all of your help on this. The issue with gax is actually resolved. It originally popped up in this PR where I am converting Datastore to gax.

Basically, we need to provide options.sslCreds (which is honored here: https://github.com/googleapis/gax-nodejs/blob/e5059c7eb652cbff29cbdc12556946a32a5b7e1a/lib/grpc.js#L83):

optionsForGax.sslCreds = grpc.credentials.createInsecure();

rmanyari · 2017-08-30T19:24:55Z

Fantastic, I'll go ahead and make the changes to the remaining services.

packages/speech/src/index.js

@@ -55,7 +69,7 @@ function speechV1(options) {

  // Create the speech client with the provided options.
  var client = gapic.v1(options).speechClient(options);
-  Object.assign(client.constructor.prototype, helpers());


rmanyari · 2017-09-20T04:17:05Z

Sorry this took forever... Now it has a bunch of conflicts with upstream, lmk if it looks fine and I'll try to fix the conflicts.

Also, for some reason I'm no longer able to build the docs.

HEAD is now at 1aa782a... speech @ 0.9.4 tagged.
/home/rodrigo/dev/google-cloud-node/scripts/docs/builder.js:316
    throw e;
    ^

Error: Unable to generate docs for file: speech/src/index.js. Reason: ENOENT: no such file or directory, open 'packages/speech/src/helpers

Not sure why it's looking at commits hashes, can someone explain what's going on here? packages/speech/src/helpers exists locally but it might (?) not be looking for that

packages/vision/src/index.js

@@ -73,7 +73,7 @@ function visionV1(opts) {
  // Create the image annotator client with the provided options.
  var client = gapic.v1(opts).imageAnnotatorClient(opts);
  if (is.undefined(client.annotateImage)) {
-    Object.assign(client.constructor.prototype, helpers('v1'));


packages/dlp/src/index.js

 var extend = require('extend');
 var gapic = {
  v2beta1: require('./v2beta1')
 };
+var grpc = require('grpc');


packages/common/src/util.js

+ *     these credentials will be used if the api endpoint is considered
+ *     to be a custom endpoint
+ * */
+function resolveGapicOptions(options, environmentVariables,


packages/dlp/src/index.js

 * @param {object=} options - [Configuration object](#/docs).
 * @param {number=} options.port - The port on which to connect to
 *     the remote host.
 * @param {string=} options.servicePath - The domain name of the
 *     API remote host.
 */
 function dlpV2beta1(options) {
+  options = common.util.resolveGapicOptions(


stephenplusplus · 2017-09-21T15:02:51Z

Thanks again for all of the work on this. It's getting a bit hairy when we're mixing in with files that are generated (speech, for example). I don't maintain the generator, so I've asked @landrito to take a look where those edits have been made. If the generator can take it on, we also wouldn't need the new tests-- it would handle that as well.

rmanyari · 2017-10-01T18:07:19Z

Hey guys, any updates in this? Is there anything I can do to help pushing this forward?

rmanyari · 2017-10-18T12:12:58Z

Any luck this gets allocated some time? It would be nice to know if there's still a slim chance that this makes it to master or if I should just forget about it. Thanks!

lukesneeringer · 2017-11-10T15:15:14Z

@stephenplusplus What's the situation on this item?

stephenplusplus · 2017-11-10T15:30:08Z

We got stalled on an issue where edits were required to the generated files: #2548 (comment)

After the repo dissection is complete, we need to go back to the drawing board with this (hopefully not too far), and re-wire this code into each new code base. I'll take on this work, given the above and beyond efforts by @rmanyari already.

stephenplusplus · 2018-01-02T16:15:26Z

I'm going to close the PR and let #1630 track the request for the feature. Sorry that we got caught up in the repo split. I thought we could get these changes in, but other priorities got in the way, which greatly complicated this. Thank you for all of your work and patience.

Rodrigo added 8 commits August 14, 2017 22:21

common: add determineBaseUrl

e505f7a

common: support for grpc endpoints in util.determineBaseUrl

6b83330

bigquery: support for custom api endpoint at instantiation

31a80e4

bigtable: support for custom api endpoint at instantiation

ff7c539

gce: support for custom api endpoint at instantiation

124d90f

datastore: support for custom api endpoint at instantiation

ae0e223

dns: support for custom api endpoint at instantiation

eb91fc6

language: support for custom api endpoint at instantiation

8446f37

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Aug 21, 2017

stephenplusplus reviewed Aug 21, 2017

View reviewed changes

stephenplusplus added the core label Aug 22, 2017

common: support for multiple environment variables

89a563d

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 25, 2017

bringing base url resolution at the service level

0f09c7f

rmanyari commented Aug 26, 2017

View reviewed changes

googleapis deleted a comment from googlebot Aug 28, 2017

stephenplusplus suggested changes Aug 28, 2017

View reviewed changes

refactor grpcService, cleanup tests

98a2f4e

rmanyari commented Aug 29, 2017

View reviewed changes

packages/common/src/service.js Outdated

@@ -52,9 +57,20 @@ function Service(config, options) {

email: options.email

});

if (!config.baseUrl) {

This comment was marked as spam.

Sign in to view

Rodrigo added 3 commits August 29, 2017 19:23

remove unnecessary complexity in having a basePath

0c19807

fix: documentation, customEndpoint flag in common.Service

0d2f432

prediction: support for custom api endpoint at instantiation

fcb916b

Rodrigo added 7 commits August 30, 2017 13:30

pubsub: support for custom api endpoint at instantiation

da50d7f

resource: support for custom api endpoint at instantiation

c2b1fc7

storage: support for custom api endpoint at instantiation

49d3afd

translate: support for custom api endpoint at instantiation

04e7169

spanner: support for custom api endpoint at instantiation

fff6e6c

fix: remove no longer needed check

d81ff37

fix: bumping disk size

b4972c2

gapic: support for custom endpoint in the remaining gapic based services

2565a03

rmanyari requested a review from lukesneeringer as a code owner September 20, 2017 03:13

storage: remove unnecessary comments

f2ce8bb

rmanyari commented Sep 20, 2017

View reviewed changes

vision: bug fix with prototype assignment

23fd6b5

rmanyari commented Sep 20, 2017

View reviewed changes

stephenplusplus reviewed Sep 21, 2017

View reviewed changes

stephenplusplus closed this Jan 2, 2018

rmanyari mentioned this pull request Mar 15, 2018

Interest in supporting custom endpoints googleapis/nodejs-compute#53

Closed

Feature custom endpoint #2548

Feature custom endpoint #2548

Uh oh!

Conversation

rmanyari commented Aug 21, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

stephenplusplus commented Aug 21, 2017

Uh oh!

rmanyari commented Aug 21, 2017

Uh oh!

stephenplusplus commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukesneeringer commented Aug 24, 2017

Uh oh!

lukesneeringer commented Aug 24, 2017

Uh oh!

rmanyari commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Aug 25, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

stephenplusplus left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

stephenplusplus commented Aug 21, 2017 •

edited

Loading

rmanyari commented Aug 25, 2017 •

edited

Loading