Skip to content

Support google_default channel credentials#6366

Merged
htuch merged 1 commit intoenvoyproxy:masterfrom
qfel:master
Mar 27, 2019
Merged

Support google_default channel credentials#6366
htuch merged 1 commit intoenvoyproxy:masterfrom
qfel:master

Conversation

@qfel
Copy link
Copy Markdown
Contributor

@qfel qfel commented Mar 23, 2019

Support google_default in channel credentials configuration. The documentation mentions this option and yet it's ignored. Confusing.

Risk Level: Low, the option was seemingly useless/unused. If anybody relies on it doing nothing, they can just unset it.

Testing: Tried running my own envoy, seemed to pick up the credentials pointed to be GOOGLE_APPLICATION_CREDENTIALS environment variable.

@dio
Copy link
Copy Markdown
Member

dio commented Mar 23, 2019

@qfel I think you need to fix you DCO 🙂

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @qfel. Can you add a test to /test/common/grpc/google_grpc_creds_test.cc to cover this new behavior? It can be super simple, see existing examples in GetChannelCredentials.

/wait

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, just for context, this is how you do OAuth2 legacy with gRPC? From https://grpc.io/docs/guides/auth.html I gather the metadata callback approach is the new way. Can you comment on how this is likely to be used and the tradeoff in this thread?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, I have little direct daily contact with gRPC/Envoy/C++. The reason for this PR is I spent some time puzzled/blaming my test env trying to figure out why Envoy isn't able to talk to the Google API I'm pointing it to. Turned out that the google_default option just doesn't seem to be implemented even though it's in the docs..

I'm not sure what you mean by callback, the only references to it seem to be under PHP section? This is the exact example for Authenticate with Google from that page, I don't see it being marked as legacy or any alternative option for the matter.

Right now if you want it to connect to a Google endpoint and present some meaningful credentials (eg. service account), you have to:

  • set channel_credentials to ssl_credentials (which is kinda silly, it can point at empty file)
  • set call_credentials to google_compute_engine, which will use the service account associated with the VM your code is running on

With this change you should be able to just set channel_credentials to google_default and get consistent behavior with other tools, ie.

  1. If GOOGLE_APPLICATION_CREDENTIALS is defined, use that
  2. If there are application default credentials generated by gcloud command line, use that
  3. If running on Google Cloud, use VM service account

Or something roughly like that. That way you can also be flexible about what credentials you use on the VM. Take what I say with a grain of salt, I've been trying to get it to work in a messy test environment that's not quite stable yet, there may be something I missed/misinterpreted/misremembered. But even if you look at the sample on grpc.io and compare to the code here, it seems to be the same (ie. should work as expected).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: you can also use JWT token and maybe others, but then you're stuck inlining the credentials in Envoy config. google_default/google_compute_engine make it easier to configure them in a more familiar way, without having to understand Envoy config (ie. you can ship with google_default and the user can configure Envoy credentials the same way they configure them for other tools).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sure, I think you are right actually. I know that gRPC has metadata callback stuff that is used elsewhere in C++, and noticed the JS/PHP metadata callback examples, so assumed that this was now a thing as well.

Thanks for the explanation. This PR looks good, can you address my nit comment above and we can ship?

The documentation mentions this option and yet it's ignored. Confusing.

Signed-off-by: qfel <qfel.pl@gmail.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@htuch htuch merged commit 5655900 into envoyproxy:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants