Added Google Maps API key support

[antoniocarlon]

• Added Google Maps API key support keeping the old behavior.
• Added tests.

Notes for the acceptance:
We need to test the following in both non-org and org users:

1. Set the client_id and secret for Google geocoding (can be done in Superadmin setting the Google Maps query string and Google Maps private key)
   • Test the geocoding functionality
2. Leave the Google Maps query string field empty and set the API key for Google geocoding (can be done in Superadmin setting the Google Maps private key, with
   something like key=AIzaXXXX)
   • Test the geocoding functionality
3. Set the client_id for basemaps and an API key for geocoding
   • Test both the geocoding and the Google basemaps functionality
4. Set an API key for basemaps and and a (different?) API key for geocoding
   • Test both the geocoding and the Google basemaps functionality

Related to https://github.com/CartoDB/cartodb-central/issues/2406

cc @inigomedina

[inigomedina]

Thanks, @antoniocarlon. 👏

@andrewbt, would you be able to help us with the acceptance test?

[andrewbt]

This is great! Happy to help - it's infrequent that I do testing in staging servers, etc so can one of you help me out and slack or email me credentials of an environment that has this PR enabled so I can test?

I'd make these additions/changes to the acceptance criteria below (sight unseen, so maybe these are wrong but I think it covers more valid cases than above). In particular, number 3 is confusing and I'm not sure when it would happen (if a Gmaps user has an API key, they probably will have transitioned off of using their client_id entirely, and Google also says these are either/or authentication options not used together: https://developers.google.com/maps/documentation/javascript/get-api-key#premium-auth). Also, none of these cover the "new normal" case of only one API key needing to be used for both geocoding and basemaps (an API key can be configured to access both or only one of these APIs), with no secret or client id.

1. Set the client_id and secret for Google geocoding (can be done in Superadmin setting the Google Maps query string and Google Maps private key)
   • Test the geocoding and basemaps functionality
2. Leave the Google Maps query string field empty and set the API key for Google geocoding (can be done in Superadmin setting the Google Maps private key, with
   something like key=AIzaXXXX)
   • Test the geocoding and basemaps functionality
3. Set the client_id for basemaps and an API key for geocoding
   • Test both the geocoding and the Google basemaps functionality
   • AT note: I don't think this case will appear in the wild, not sure it makes sense, but could test anyway.
4. Set an API key for basemaps and and a different API key for geocoding
   • Test both the geocoding and the Google basemaps functionality
   • Could also test/verify the different API keys used are configured for the Maps Javascript API and the Geocoding API, and what happens when they are improperly configured (errors returned, etc)
5. Set an API key for basemaps and the same API key for geocoding
   • Test both geocoding and basemaps functionality

[andrewbt]

Oh, also, I'll need access to a GCP console project with Billing enabled to create/delete API keys for the testing. I don't anticipate any actual charges, but Google requires a Billing account set up to create the requisite API keys (https://developers.google.com/maps/documentation/javascript/get-api-key#step-1-get-an-api-key)

(cc @cmpera we might want to think about getting a general Solutions GCP account with Billing anyway to make tasks like this easier as we continue to leverage our GCP relationship for testing/demos/etc. I could use my personal CC and expensify but that doesn't seem the most robust way)

[gonzalosr]

1. API key configured for Geocoding API only
2. API key configured for Maps Javascript API only
3. API key configured for both Geocoding and Maps Javascript APIs

API Keys created and sent to @andrewbt via Slack. Will also get uploaded to Lastpass shared-solutions folder ASAP

you can check them out here

/cc @jvillarf for awareness

[jvillarf]

Let's remove them, once the testing period is over.

[jvillarf]

@andrewbt @cmpera If you need permanent API keys for testing, or demos, etc, please let us know, in order to create you the right ones, in the suitable Google Project, etc.

[andrewbt]

Didn't have time for this last week but I anticipate being able to complete it this week, thanks will post updates here!

…

On Mon, May 20, 2019 at 11:43 AM jvillarf ***@***.***> wrote: @andrewbt <https://github.com/andrewbt> @cmpera <https://github.com/cmpera> If you need permanent API keys for testing, or demos, etc, please let us know, in order to create you the right ones, in the suitable Google Project, etc. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#554?email_source=notifications&email_token=AAPLS4YBTKEFVE3AHWBFAM3PWLBJNA5CNFSM4HLSAURKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVZHYRQ#issuecomment-494042182>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPLS4Y3Q6JPODM7XSZWAELPWLBJNANCNFSM4HLSAURA> .

-- Andrew Thompson, Solutions Engineer *CARTO* | 201 Moore St, Brooklyn, NY, 11206 athompson@carto.com | T +1 508.333.7265 <+15083337265> Web carto.com <http://cartodb.com/> | Twitter @carto <http://twitter.com/carto> Unlock the Potential of Your Location Data Live sessions to learn CARTO <http://carto.com/webinars/>

[andrewbt]

@antoniocarlon Tried to test today but I think the server you had set up before is down/gone, can you spin up again for the pvenkman user and ghostbusters org accounts?

Also @gonzalosr can you confirm that carto-staging.com is added/allowed for those three API keys? Realized that's another prerequisite after reviewing the instructions Antonio sent me.

[antoniocarlon]

@andrewbt done, the server is up and running!

[gonzalosr]

@andrewbt these API keys are only API type limited, so AFAIK they can be used from any ip/host/referrer

[jvillarf]

Hi @andrewbt ,

i've restricted the API keys to the host below. Please, let us know if you need anything else from our side.

[andrewbt]

@antoniocarlon I was trying to import datasets to geocode, ran into this error for both accounts:

You're sure the servers are running now?

[antoniocarlon]

Hi @andrewbt, I have created new accounts: chewie and the millenium org. Let me know if they work for you 👍

[andrewbt]

Began testing today finally. I can confirm that I can enable and toggle the Google Maps basemaps options with an appropriately-permissioned Maps Javascript API key.

However, I haven't gotten geocoding to work yet. It seems like CARTO is checking geocoding quota when Google Maps is enabled as the geocoding provider, and there's something wacky with these users quota settings. Given that Google will bill/monitor users for geocoding quota I don't think CARTO should check for it at all. In other words, on Google-CARTO accounts we should suppress our geocoding quota checking features and just be a straight "pass through" to the Google API.

Additionally, a couple smaller bugs:

Bug #1:

1. Set query string on individual account to maps+geocoding google key; leave private key blank
2. Login
3. Create a map from data that is not geocoded yet (null the_geom)
4. Notice that on the first Builder/Map view, the map area is gray and has not loaded Google. (Bug!)
5. Go into basemap settings, pick any Google maps option. Notice the map refreshes and is now loaded.

Bug #2:
Similar to bug #1 but on an org account instead of individual - first time Builder Map view loads it is set to CARTO Voyager - ideally should be set to Google Maps by default. This one less severe though.

Bug #3:
Here basemap options should not be shown/available when an account has Google configured - they are mutually exclusive providers from a legal perspective.

Will continue testing starting Tuesday next week.

[antoniocarlon]

Thank you for your tests @andrewbt

The issues that you have found seem to be unrelated to this PR, could you open a new issue for them?

Regarding the quotas, I have checked that if the geocoder is set to Google it should not check for quotas (https://github.com/CartoDB/dataservices-api/blob/development/server/lib/python/cartodb_services/cartodb_services/metrics/quota.py#L87-L90) but I'll take a look at it to double check this quota issue 🤔 --> I have added tests for Google quota check and verified that everything is working as expected, how do you know that CARTO is checking geocoding quota when Google Maps is enabled as the geocoding provider?

[andrewbt]

@antoniocarlon , sorry, posted that comment in a rush and meant to include screenshots:

Should I be trying via the SQL API data services functions instead of Builder? (I was going to try them next after I got builder to work) What is the scope of this PR? Are basemaps not included in the scope?

[andrewbt]

And should I create new issues for those other bugs here in dataservices, or some other repo? Builder? Not sure where all the involved functionality is located :)

[antoniocarlon]

Thank you @andrewbt !

I see 3 different issues here:

• Bug 1 and 2
• Bug 3
• The issue related to Builder checking the geocoding quota when the provider is Google. Reagrding this I have checked that Builder does not ask dataservices for the quota info so I think that this comes directly from Redis. In any case, Builder should not block geocodings when the provider is Google no matter the user (or org) geocoding quota.

The three issues are related to Builder and I think that they should be created in the https://github.com/CartoDB/cartodb repo (and I would say that Response Team should take care of them).

[ivanmalagon]

@andrewbt Did you create the issues? Can you give me an update on this project/issue?

[andrewbt]

@ivanmalagon I created two issues, CartoDB/cartodb/issues/14942, CartoDB/cartodb/issues/14943, you can see above too.

I haven't created an issue for @antoniocarlon's point above as I think he/someone else could define the specifics better than me, but this should probably be an issue also:

The issue related to Builder checking the geocoding quota when the provider is Google. Reagrding this I have checked that Builder does not ask dataservices for the quota info so I think that this comes directly from Redis. In any case, Builder should not block geocodings when the provider is Google no matter the user (or org) geocoding quota.

I discussed this with @inigomedina this morning, sorry for the confusion and delay on this one. It's been difficult for me to test it completely in a timely manner and I don't want to be a blocker for the process. I don't think we were ever aligned on the full scope/complexity of the tasks needed to fully support new-style Google API keys across Builder and the CARTO Platform. When Inigo asked me to help with testing this PR, I had been thinking it covered all Google functionality including Builder, but it seems the changes here are maybe only relevant to Data Services functions used via SQL API. That is a step towards complete support but not the whole story. (I'm also not sure the acceptance criteria we defined above is completely correct :-/ ) Definitely take a look at the discussion in CartoDB/cartodb-central/issues/2406 for background.

Let me attempt to define a possible full scope of the initiative here:

1. Google is no longer issuing client id and private key pairs for new Google Maps clients, and is slowly migrating old clients with these credentials to the new GCP API keys. So currently, new GCP customers can't buy/use the CARTO-integrated-with-Google product because they don't have the old-style credentials. A time will likely come when existing CARTO-Google customers will no longer be able to use client id and private key credentials either.
2. Google also has specific terms for their services that require geocode data from Google to be displayed on Google basemaps only. If a CARTO account is configured to use Google LDS, that account should not also have access to CARTO/OSM or Here basemaps (for example) to prevent this. (Obviously, we can't control what CARTO.js developers might do, but we can make reasonable steps in Builder)
3. Therefore, CARTO Superadmin should have a way for accounts to be configured with new-style GCP API keys that have Google Maps Javascript API and Google Geocoding API permissions. (Decision to make on whether we only support one Google API key with both permissions or two separate API keys with each permission. One is what we do now, with this PR, and is simplest)
4. A configured Google-CARTO account should only support Google Basemaps options in Builder, and load those basemaps by default. (That's those two issues I reported above, not the scope of this PR)
5. A configured Google-CARTO account should use the Google geocoder in any UI-based geocoding - Builder Analysis, or the new Analysis Framework to future-proof a bit here. (Not the scope of this PR?)
6. A configured Google-CARTO account should use the Google geocoder in the Data Services API SQL functions. (I think that's the scope of this PR?)
7. Because quota and billing is handled by Google, a configured Google-CARTO account should not "check quota" for any geocoding operations, UI or API.

This PR gets us some of the way to that complete support, but not all the way.

In terms of additional stakeholders or other people to talk to, you and Inigo might want to talk to Jatorre (who is leading our Google partnership nowadays) and our Google engineering contacts like Sean Wohltman.

Hope this detail is helpful.

[jvillarf]

Hey @andrewbt ,

our technical account manager is called Brian Weeks brianweeks@google.com and he will be more than happy to get any question from our side, so please, feel free to write him.

[alrocar]

Anyone in this thread, what's the status of this?

[antoniocarlon]

AFAIK this is not accepted yet.
The changes are small and should not break anything if merged but I don't know if this is still useful 🤷‍♂️

[andrewbt]

@alrocar see my comment on June 4th, as far as I know everything there is still the case, mainly:

I don't think we were ever aligned on the full scope/complexity of the tasks needed to fully support new-style Google API keys across Builder and the CARTO Platform. When Inigo asked me to help with testing this PR, I had been thinking it covered all Google functionality including Builder, but it seems the changes here are maybe only relevant to Data Services functions used via SQL API. That is a step towards complete support but not the whole story.

Agree with @antoniocarlon, it remains not accepted because I wasn't able to get clarity on what the right acceptance criteria were for this PR.

This was probably a mis-communication/mis-scoping, and "full" Google support like we used to have will take more work across more parts of the platform than this PR alone (but this PR could still be part of it - I think?). I think it probably would be worth a conversation with Jatorre to see if the required level of investment is worthwhile, since he is the one managing the Google strategic relationship nowadays.

[alrocar]

I see, thanks both for the update.

So for me this could be a product initiative to be considered for priorization.

@andrewbt Would you be able to write a product issue with a description of the problem and a reference to this PR.

[andrewbt]

@alrocar done, thanks :) https://github.com/CartoDB/product/issues/470

[alrocar]

@andrewbt AWESOME!! Thanks!!