feat: add x-goog-user-project header from quota_project_id field by tmatsuo · Pull Request #21324 · grpc/grpc

tmatsuo · 2019-11-27T23:33:31Z

fixes #21225

linux-foundation-easycla · 2019-11-27T23:33:34Z

The committers are authorized under a signed CLA.

✅ Takashi Matsuo (2895232, 6acdfdb)

jiangtaoli2016 · 2019-11-27T23:55:42Z

Thanks @tmatsuo for the contribution. Ping me when this is ready for review.

broady · 2019-12-02T20:02:14Z

Sorry, we changed the field to quota_project_id for more consistency.

LGTM though

tmatsuo · 2019-12-03T22:43:21Z

@jiangtaoli2016

Added some test cases. I think it's ready for review. Can you take a look?

Also, I can not add the labels. Can you add some labels for mergeable check?

Thanks!

tmatsuo · 2019-12-04T21:48:16Z

ASAN build caught the memory leak. I'll fix that with the next commit

tmatsuo · 2019-12-04T21:53:52Z

@jiangtaoli2016 I believe the memory leak has been fixed.

jiangtaoli2016

Thanks @tmatsuo for implementation. Looks good over all. A couple of minor comments.

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

src/core/lib/security/credentials/oauth2/oauth2_credentials.h

tmatsuo

Thanks for the review. I think I addressed the comments. PTAL

src/core/lib/security/credentials/oauth2/oauth2_credentials.h

jiangtaoli2016 · 2019-12-05T23:47:27Z

Please wait for @apolcyn review.
Also please squash before submit.

tmatsuo · 2019-12-06T18:13:45Z

@jiangtaoli2016 Thanks.

I have a question w.r.t. squashing. Do you use "Squash and merge" feature on this repo, or should I locally squash commits and force push the new change?

tmatsuo · 2019-12-11T00:45:38Z

@apolcyn friendly ping

apolcyn · 2019-12-11T00:16:11Z

test/core/security/credentials_test.cc

+    "{ \"client_id\": \"32555999999.apps.googleusercontent.com\","
+    "  \"client_secret\": \"EmssLNjJy1332hD4KFsecret\","
+    "  \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\","
+    "  \"quota_project_id\": \"my-quota-project-id\","


question: quota_project _id could also be a number, right?

It is always a string.

"project ID" is always a string
"project number" is always a number

When I read the spec, it was fluid. However, is it really a problem if we treat numeric ID as a string?

go/adc-spec is a (very) draft spec for the JSON file, and we've specified it as a string.

If in the future we need to support project numbers (and not treat the number as a string), it'll be a separate field, "quota_project_number".

apolcyn · 2019-12-11T00:17:41Z

test/core/security/credentials_test.cc

+  grpc_call_credentials* creds = grpc_google_refresh_token_credentials_create(
+      test_refresh_token_with_quota_project_id_str, nullptr);
+
+  /* First request: http put should be called. */


nit: this comment is a little confusing, is it accurate? I don't think PUT requests are ever performed, in any case, if I'm correct?

Good catch, changed to post.

apolcyn · 2019-12-11T00:27:07Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

  result.type = GRPC_AUTH_JSON_TYPE_AUTHORIZED_USER;

+  // quota_project_id is optional, so we don't check the result of the copy.
+  grpc_copy_json_string_property(json, "quota_project_id",


Related to below comment in test.

I've seen some docs that say that this can be a string or a numberic project ID. Does the format used for the refresh token credentials only allow the human readable project name format? If number format is also a possibility, then parsing may need adjusting.

@broady Can you confirm?

However, do you think it is problematic if we treat a numeric ID as a string?

Answered below. It's always a string. Project number will not be in this field.

apolcyn · 2019-12-11T00:27:58Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

+void grpc_google_refresh_token_credentials::add_additional_metadata(
+    grpc_credentials_mdelem_array* md_array) {
+  if (refresh_token_.quota_project_id != nullptr) {
+    grpc_mdelem quota_project_md = GRPC_MDNULL;


nit: unnecessary line, it can be set to grpc_mdelem_from_slices

Thanks, done

tmatsuo

@apolcyn Thanks for the review! I addressed some comments. PTAL.

tmatsuo · 2019-12-11T01:11:19Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

+void grpc_google_refresh_token_credentials::add_additional_metadata(
+    grpc_credentials_mdelem_array* md_array) {
+  if (refresh_token_.quota_project_id != nullptr) {
+    grpc_mdelem quota_project_md = GRPC_MDNULL;


Thanks, done

tmatsuo · 2019-12-11T01:11:42Z

test/core/security/credentials_test.cc

+  grpc_call_credentials* creds = grpc_google_refresh_token_credentials_create(
+      test_refresh_token_with_quota_project_id_str, nullptr);
+
+  /* First request: http put should be called. */


Good catch, changed to post.

tmatsuo · 2019-12-11T01:13:25Z

test/core/security/credentials_test.cc

+    "{ \"client_id\": \"32555999999.apps.googleusercontent.com\","
+    "  \"client_secret\": \"EmssLNjJy1332hD4KFsecret\","
+    "  \"refresh_token\": \"1/Blahblasj424jladJDSGNf-u4Sua3HDA2ngjd42\","
+    "  \"quota_project_id\": \"my-quota-project-id\","


When I read the spec, it was fluid. However, is it really a problem if we treat numeric ID as a string?

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

apolcyn

minor comments.

A couple question on the feature:

I believe the way to use "x-goog-user-project" so far has been to just set it manually; is there possibility that when the quota_project field is added to json auth files for the first time, that it can conflict with such usage?
Is the quota_project_id value that is added to auth json files just specified by user?

apolcyn · 2019-12-11T08:30:19Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

    grpc_polling_entity* pollent, grpc_auth_metadata_context /*context*/,
    grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata,
    grpc_error** /*error*/) {
+  add_additional_metadata(md_array);


naming nit: can we prefix this with maybe, i.e. change to maybe_add_additional_metadata

apolcyn · 2019-12-11T08:38:29Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.h

 // auth_refresh_token parsing.
-typedef struct {
+struct grpc_auth_refresh_token {
+  grpc_auth_refresh_token() : quota_project_id(nullptr) {}


nit: can remove this line and set quota_project_id = nullptr below

tmatsuo

I believe the way to use "x-goog-user-project" so far has been to just set it manually; is there possibility that when the quota_project field is added to json auth files for the first time, that it can conflict with such usage?

I don't think it's a problem.

Is the quota_project_id value that is added to auth json files just specified by user?

Mainly this is auto selected by gcloud sdk depending on the user configuration (config->project).

Adding @broady to confirm.

tmatsuo · 2019-12-11T16:02:52Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

    grpc_polling_entity* pollent, grpc_auth_metadata_context /*context*/,
    grpc_credentials_mdelem_array* md_array, grpc_closure* on_request_metadata,
    grpc_error** /*error*/) {
+  add_additional_metadata(md_array);


tmatsuo · 2019-12-11T16:03:43Z

src/core/lib/security/credentials/oauth2/oauth2_credentials.h

 // auth_refresh_token parsing.
-typedef struct {
+struct grpc_auth_refresh_token {
+  grpc_auth_refresh_token() : quota_project_id(nullptr) {}


tmatsuo · 2019-12-11T23:54:29Z

@apolcyn

After doing some experiment, it turns out that, if you specify x-goog-user-project manually like:

context.AddMetadata("x-goog-user-project", "some-other-project");

and if the auth json file contains quota_project_id, it is actually a problem.

In this case, it seems like those values are combined with a comma and I'm getting an error message as follows

Project 'some-other-project,my-quota-project' not found or deleted.

The initial thing came to mind was to check contents of the md_array and only add x-goog-user-project when the key doesn't exist in grpc_google_refresh_token_credentials::maybe_add_additional_metadata function. However, it doesn't work because md_array is always empty there.

So I'm not sure how to handle this case. Is there a nice way to disallow duplicated keys in metadata?

apolcyn · 2019-12-12T00:03:44Z

So I'm not sure how to handle this case. Is there a nice way to disallow duplicated keys in metadata?

Unfortunately, I'm not sure that there is, at least from within the the oauth2 credentials implementation. It may be possible to do some sort of header interception for this e.g. within the client_auth_filter, but that would probably need some larger changes.

But I'm also wondering if handling that is really necessary, this is why I wonder "how is the quota_project_id" being set within the auth json files.

If "quota_project_id" being filled into auth json files is an opt-in thing, then maybe we don't need to handle this, since onus is on the user to not do something that will conflict with their code.

OTOH if it's going to be filled in transparently, then it could perhaps cause problems for existing code.

What do you think?

tmatsuo · 2019-12-12T04:35:18Z

@apolcyn Thanks. I don't think the change will be an opt-in.

However, currently x-goog-user-project metadata header is not documented anywhere. Also the auth json file with the type authorized_user is only for development purpose. So I think the user impact will be negligible.

I'm on the fence. I'll take a look at the client_auth_filter code and then think about this again.

broady · 2019-12-12T04:36:19Z

I agree. It's very unlikely to cause an issue in practice.

tmatsuo · 2019-12-12T18:15:35Z

@apolcyn
@broady

Thanks,

I took a look at client_auth_filter, I don't think it is a trivial change to handle duplicate metadata key. Also it's probably better to separate PRs, even if we decide to handle this case.

So, now I think we should proceed with the current code.

WDYT?

tmatsuo · 2020-01-10T18:41:15Z

@apolcyn @jiangtaoli2016
PTAL
The last commit add the metadata only when the same key doesn't exist.

jiangtaoli2016 · 2020-01-13T23:11:03Z

Adding one-off check on generic client_auth_filter seems hacky.
@apolcyn do you know if we can put the check on oauth2_credentials.cc?

tmatsuo · 2020-01-14T19:49:52Z

@jiangtaoli2016
I understand. How about to have a new function like grpc_metadata_batch_add_tail_when_key_not_exist or something in metadata_batch.cc? It will make the code little bit cleaner and more generic I think.

tmatsuo · 2020-01-14T21:42:39Z

@jiangtaoli2016 The last commit added grpc_metadata_batch_add_tail_when_key_not_exist function.

apolcyn · 2020-01-15T19:21:58Z

@jiangtaoli2016

Adding one-off check on generic client_auth_filter seems hacky.
@apolcyn do you know if we can put the check on oauth2_credentials.cc?

I don't think it's possible to put the check in oauth2_credentials.cc, at least not without some sort of change to the get_request_metadata API, since oauth2_credentials doesn't currently have a way to access the initial_metadata of the grpc_transport_stream_op_batch that the client auth filter is working with.

tmatsuo · 2020-01-17T22:52:17Z

@jiangtaoli2016

PTAL with the last commit. The code is now little bit more generic and cleaner.

jiangtaoli2016

Sorry for delay. Thanks @tmatsuo for revision.

tmatsuo · 2020-01-28T18:41:14Z

@apolcyn I have just squashed the commits. Do you mind taking a look and merge it if it looks ok?

apolcyn

Sorry for delay. LGTM but I'll actually ask @yashykt to take a second look at the metadata_batch.{h.cc} change

tmatsuo · 2020-02-05T18:13:16Z

@yashykt friendly ping

tmatsuo · 2020-02-07T01:08:12Z

@jiangtaoli2016 It's approved. Can you go ahead and merge this?

jiangtaoli2016 · 2020-02-20T22:57:38Z

test failures unrelated.

feat: add x-goog-user-project header from quota_project_id field

tmatsuo marked this pull request as ready for review December 3, 2019 22:41

jiangtaoli2016 requested review from apolcyn and jiangtaoli2016 December 5, 2019 19:54

jiangtaoli2016 added area/security kokoro:force-run release notes: yes Indicates if PR needs to be in release notes labels Dec 5, 2019

grpc-kokoro removed the kokoro:force-run label Dec 5, 2019

tmatsuo changed the title ~~feat: add x-goog-user-project header from quota_project field~~ feat: add x-goog-user-project header from quota_project_id field Dec 5, 2019

jiangtaoli2016 reviewed Dec 5, 2019

View reviewed changes

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc Show resolved Hide resolved

src/core/lib/security/credentials/oauth2/oauth2_credentials.h Outdated Show resolved Hide resolved

tmatsuo commented Dec 5, 2019

View reviewed changes

jiangtaoli2016 approved these changes Dec 5, 2019

View reviewed changes

src/core/lib/security/credentials/oauth2/oauth2_credentials.h Outdated Show resolved Hide resolved

apolcyn reviewed Dec 11, 2019

View reviewed changes

tmatsuo commented Dec 11, 2019

View reviewed changes

apolcyn reviewed Dec 11, 2019

View reviewed changes

src/core/lib/security/credentials/oauth2/oauth2_credentials.cc Outdated Show resolved Hide resolved

apolcyn reviewed Dec 11, 2019

View reviewed changes

tmatsuo commented Dec 11, 2019

View reviewed changes

jiangtaoli2016 approved these changes Jan 23, 2020

View reviewed changes

jiangtaoli2016 added the kokoro:force-run label Jan 23, 2020

grpc-kokoro removed the kokoro:force-run label Jan 23, 2020

tmatsuo force-pushed the x-goog-user-project branch from 556ddb3 to 0c66117 Compare January 28, 2020 18:39

feat: add x-goog-user-project header from quota_project field

9bed17b

tmatsuo force-pushed the x-goog-user-project branch from 0c66117 to 9bed17b Compare January 28, 2020 21:36

apolcyn approved these changes Jan 29, 2020

View reviewed changes

apolcyn requested a review from yashykt January 29, 2020 19:48

yashykt approved these changes Feb 7, 2020

View reviewed changes

jiangtaoli2016 self-assigned this Feb 20, 2020

jiangtaoli2016 added the kokoro:force-run label Feb 20, 2020

grpc-kokoro removed the kokoro:force-run label Feb 20, 2020

jiangtaoli2016 added lang/core release notes: yes Indicates if PR needs to be in release notes and removed release notes: yes Indicates if PR needs to be in release notes labels Feb 20, 2020

jiangtaoli2016 merged commit ddd0107 into grpc:master Feb 20, 2020

codeblooded pushed a commit to codeblooded/grpc that referenced this pull request Feb 21, 2020

Merge pull request grpc#21324 from tmatsuo/x-goog-user-project

1a303dc

feat: add x-goog-user-project header from quota_project_id field

vjpai mentioned this pull request Feb 24, 2020

Revert "feat: add x-goog-user-project header from quota_project_id field" #22118

Merged

jiangtaoli2016 mentioned this pull request Apr 14, 2020

add x-goog-user-project header from quota_project_id field #22666

Closed

lock bot locked as resolved and limited conversation to collaborators May 20, 2020

Conversation

tmatsuo commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiangtaoli2016 commented Nov 27, 2019

Uh oh!

broady commented Dec 2, 2019

Uh oh!

tmatsuo commented Dec 3, 2019

Uh oh!

tmatsuo commented Dec 4, 2019

Uh oh!

tmatsuo commented Dec 4, 2019

Uh oh!

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jiangtaoli2016 commented Dec 5, 2019

Uh oh!

tmatsuo commented Dec 6, 2019

Uh oh!

tmatsuo commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apolcyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo commented Nov 27, 2019 •

edited

Loading

linux-foundation-easycla bot commented Nov 27, 2019 •

edited

Loading

tmatsuo commented Dec 12, 2019 •

edited

Loading

broady commented Dec 12, 2019 via email •

edited

Loading

tmatsuo commented Jan 14, 2020 •

edited

Loading

apolcyn left a comment •

edited

Loading