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

Alert user when keyfile does not include projectId #171

Closed
tmatsuo opened this issue Sep 28, 2016 · 9 comments
Closed

Alert user when keyfile does not include projectId #171

tmatsuo opened this issue Sep 28, 2016 · 9 comments
Assignees
Labels
auth priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@tmatsuo
Copy link
Contributor

tmatsuo commented Sep 28, 2016

cc: @bshaffer

Basically I do like this:

$datastore = new DatastoreClient();
$task = $datastore->entity('Task');
$task['category'] = 'Personal';
$task['done'] = false;
$task['priority'] = 4;
$task['description'] = 'Learn Cloud Datastore';
$datastore->insert($task);

with a hope that the client library correctly guesses the projectId from the credentials (the credentials are given via the GOOGLE_APPLICATION_CREDENTIALS envvar).

The test is failing with a wrong projectId guess:
https://travis-ci.org/GoogleCloudPlatform/php-docs-samples/jobs/163269293#L3696

I think the test is failing because this guess is based on the project where the GCE instance is running in (travis is running GCE).

Can this library make a right guess from the credentials file? The credentials file contains projectId in it, so it's technically possible.

@jdpedrie jdpedrie self-assigned this Sep 28, 2016
@jdpedrie
Copy link
Contributor

I'm looking into this now.

@jdpedrie
Copy link
Contributor

@tmatsuo I'm having trouble duplicating this. php-tools is copying the base64-encoded keyfile data from travis' config into credentials.json and making it available in the environment variable.

It seems that you're authenticating fine because you wouldn't get so far as a datastore not enabled error if it was a keyFile issue...

What should the project ID be? The project ID that the test is using is travis-ci-prod-3.

When we guess about a project ID, the first thing we check is the keyFile, if it's provided. In this case I think it is, via the environment variable, so it should never reach the GCE project ID check.

It's odd that you'd authenticate fine but then run into an incorrect project ID.

@jdpedrie
Copy link
Contributor

I tried forking the php-docs-samples repo and running the datastore-veneer branch against travis. The tests are passing for me. I am using my normal Project ID that I use for everything, and I am seeing that it is using the correct Project ID.

Is it possible that the keyFile in GOOGLE_CREDENTIALS_BASE64 is incorrect?

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 28, 2016

I had a chat on hangout with @dwsupplee and @jdpedrie and we figured out what is happening here.

The current code for guessing projectId is here:
https://github.com/GoogleCloudPlatform/google-cloud-php/blob/master/src/ClientTrait.php#L124

It looks up the project_id key in the json file. This works perfectly for newly created json credentials file. Unfortunately some old json credentials file doesn't have this field. So in this case, the library falls back to use the GCE metadata. Anecdotally, our travis tests are running on a GCE instance and it fetches the metadata for the project; then guessed it is 'travis-ci-prod-3'.

To unblock our test, we will re-generate the json key file, I'm pretty certain we'll be fine.

It might be a good idea to give a warning when a user is using old format json key file, something like: "The credentials file you are using has an old format. We recommend that you re-generate the credentials file".

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 28, 2016

JFYI, I confirmed that the tests pass with a new json key file just generated.

@jdpedrie
Copy link
Contributor

I'll go ahead and close this out then. :)

@dwsupplee
Copy link
Contributor

@jdpedrie Would you mind if we kept this open to track adding the warning for users?

@jdpedrie jdpedrie reopened this Sep 28, 2016
@jdpedrie jdpedrie changed the title Wrong projectId guess with the Datastore client Alert user when keyfile does not include projectId Oct 3, 2016
@jdpedrie jdpedrie added api: datastore Issues related to the Datastore API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed api: datastore Issues related to the Datastore API. labels Oct 3, 2016
@michaelbausor michaelbausor added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Jul 11, 2017
@danoscarmike
Copy link
Contributor

@dwsupplee what's the status of this one? Are we actively pursuing a solution or is it 'nice-to-have' for now?

@dwsupplee
Copy link
Contributor

@danoscarmike, This is currently a nice to have.

@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Jun 8, 2018
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

7 participants