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

Custom Endpoint Support #1630

Closed
1 of 14 tasks
callmehiphop opened this issue Sep 26, 2016 · 17 comments
Closed
1 of 14 tasks

Custom Endpoint Support #1630

callmehiphop opened this issue Sep 26, 2016 · 17 comments
Assignees
Labels
core type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects

Comments

@callmehiphop
Copy link
Contributor

callmehiphop commented Sep 26, 2016

Let's add support for any custom endpoint across the board. We already have support for Datastore, PubSub and (soon) Bigtable. For the most part supporting them has just been some minor configuration changes, so we should be able to support it everywhere.

We should have a setting, e.g. options.apiEndpoint = 'http://localhost:8888 as well as environment variables.

  • GOOGLE_CLOUD_BIGQUERY_ENDPOINT
  • GOOGLE_CLOUD_BIGTABLE_ENDPOINT
  • GOOGLE_CLOUD_COMPUTE_ENGINE_ENDPOINT // GCE_ENDPOINT?
  • GOOGLE_CLOUD_DATASTORE_ENDPOINT
  • GOOGLE_CLOUD_DNS_ENDPOINT
  • GOOGLE_CLOUD_LANGUAGE_ENDPOINT
  • GOOGLE_CLOUD_LOGGING_ENDPOINT
  • GOOGLE_CLOUD_PREDICTION_ENDPOINT
  • GOOGLE_CLOUD_PUBSUB_ENDPOINT
  • GOOGLE_CLOUD_RESOURCE_MANAGER_ENDPOINT
  • GOOGLE_CLOUD_SPEECH_ENDPOINT
  • GOOGLE_CLOUD_STORAGE_ENDPOINT
  • GOOGLE_CLOUD_TRANSLATE_ENDPOINT - translate: support GOOGLE_CLOUD_TRANSLATE_ENDPOINT env var #1674
  • GOOGLE_CLOUD_VISION_ENDPOINT
@stephenplusplus stephenplusplus changed the title Emulator Support Custom Endpoint Support Sep 27, 2016
@stephenplusplus
Copy link
Contributor

Small title change to make sure it's clear we want to support "http://any-uri" as opposed to only official service emulators.

@jmuk jmuk added priority: p2 Moderately-important priority. Fix may not be included in next release. Status: Acknowledged labels Mar 7, 2017
@jmuk
Copy link
Contributor

jmuk commented Mar 7, 2017

@danoscarmike might also be interested in the idea like this.

@rmanyari
Copy link

I'm would be super excited to see this feature merged in the next release (custom endpoints are very very helpful for people who operate behind forward proxies).

I can open a PR in the next days that puts a function very similar to {Datastore|PubSub}.prototype.determineBaseUrl_ in common.utils and then use it across the different services.

Rodrigo

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Aug 14, 2017

Are these individual environment variables? That seems lousy. I would much rather have this be a parameter that can be passed to clients.

Individual environment variables means we are always chasing the full list of APIs; a consistent parameters is a one-and-done.

@rmanyari
Copy link

PubSub does expose a parameter (vs. other services that use environment variables):

https://github.com/GoogleCloudPlatform/google-cloud-node/blob/a64ad81517045196cf5a3f468ea15aad1e2c25da/packages/pubsub/src/index.js#L83

I prefer the parameter approach as well. @lukesneeringer any other comments on the suggested changes? I can start writing some code for GCE

@lukesneeringer
Copy link
Contributor

The other primary comment I have is whether or not the client objects wrap a common object (I think they do). If they do, then the best answer is to place the apiEndpoint parameter there. If not, then it would need to be applied to each object.

It looks like it needs to be explicitly passed -- see here in Spanner, for instance. That value just needs to change to options.apiEndpoint || 'spanner.googleapis.com'. Similar in every gRPC-based API.

@stephenplusplus
Copy link
Contributor

I believe this was originally a @jgeewax request to have env vars for each. I think the nice part of env vars is you don't have to modify your code depending on where your app is running.

Either way, we should expose at least an override via options.apiEndpoint for all APIs. Thank you for volunteering @rmanyari!

@lukesneeringer
Copy link
Contributor

lukesneeringer commented Aug 14, 2017

I believe this was originally a @jgeewax request to have env vars for each. I think the nice part of env vars is you don't have to modify your code depending on where your app is running.

You can always have your code set a value based on your own environment variable. Not so much the converse. If you are in a situation where environment variables are inconvenient, there is not much you can do.

@rmanyari
Copy link

rmanyari commented Aug 15, 2017

Silly question. I'm extracting determineBaseUrl in ./packages/common and ./packages/common-grpc. Before calling common.utils.determineBaseUrl in ./packages/compute/src/index.js I have to do:

> npm link ../packages/common 

to pickup my local changes. I'm not 100% sure how the publishing process works for this project. Should I submit a PR for the changes in common and common-grpc first and then bump the version in the other service packages before submitting the "actual" changes or is there a clever way of building everything that I'm missing out?

@stephenplusplus
Copy link
Contributor

It's fine to put it in one PR. We don't have automated releases, so I'll be manually testing / publishing anyway :)

Just a tip, we have an npm run script that you can run from the root directory of the project:

$ npm run link-common

@rmanyari
Copy link

rmanyari commented Aug 21, 2017

Just opened a PR. A few things:

I only added support for bigquery, bigtable, compute, datastore, dns and language since the change set is already getting big. If everything looks OK, I can continue with the 2nd half.

The build is failing because there are changes under common that are not picked up by the build. Should the build do npm run link-common before trying to run the tests?

It has the cla: no label since I don't think I was added (yet) to the list of authorized contributors at the company I work for. I already got cleared internally, this should go away soon

@stephenplusplus
Copy link
Contributor

@alexander-fenster do we still want environment variable support for endpoints, or is an option, i.e. config.apiEndpoint = 'http://my-api.com' good enough?

@stephenplusplus
Copy link
Contributor

We have 3 different API types:

Not supported

  • REST (nodejs-common)

Already supported!

  • GRPC (nodejs-common-grpc):
    • new Firestore({ baseUrl: 'http://my-custom-endpoint' })
  • GAPIC:
    • new Datastore({ servicePath: 'http://my-custom-endpoint' })

  • options.apiEndpoint is what our REST APIs have used before, and I'm not sure if any still do.
  • options.baseUrl is what we used in common-grpc. I believe Firestore is the only API still using that module for API requests.
  • options.servicePath is a term right from gRPC, and thus GAPIC. We transparently accept it, as all user options are passed right through to GAPIC.

Does anyone have thoughts on how we should support this concept of a custom API endpoint?

@alexander-fenster
Copy link
Contributor

I might be a little bit out of context, but having a possibility to override the endpoint via the environment variable might be valuable (e.g. for testing purposes). I don't think it's a high priority request though.

@JustinBeckwith JustinBeckwith added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: enhancement labels Jun 13, 2018
@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Jun 13, 2018
@JustinBeckwith JustinBeckwith removed the 🚨 This issue needs some love. label Jun 25, 2018
@JustinBeckwith
Copy link
Contributor

So... good news! This is now a high priority thing 🙃 Let's track it over in #2928

@stephenplusplus
Copy link
Contributor

This issue also covered env vars to change the API endpoint, which was a Googler-requested feature at one point. I can see the value for the user, but it would also be the same as a small modification the user can make themselves with the same net effect:

$ echo $GOOGLE_CLOUD_BIGQUERY_ENDPOINT
https://my-custom-endpoint
new Storage({
  apiEndpoint: process.env.GOOGLE_CLOUD_BIGQUERY_ENDPOINT
})

@JustinBeckwith
Copy link
Contributor

That's a really good question :) I've never heard this request. Is this primarily a testing thing? (cc @broady)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
No open projects
State
Not Started
Development

No branches or pull requests

7 participants