-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add an experimental googleclientauth extension #703
Conversation
Codecov Report
@@ Coverage Diff @@
## main #703 +/- ##
==========================================
+ Coverage 69.59% 69.66% +0.07%
==========================================
Files 36 42 +6
Lines 4717 4840 +123
==========================================
+ Hits 3283 3372 +89
- Misses 1286 1317 +31
- Partials 148 151 +3
... and 13 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
cf5f3d7
to
4b43b08
Compare
I think I've thought of a way to test this. We can support custom authenticators on our exporter in client config, and run integration tests using this as a source of auth |
4895b3d
to
ac71b09
Compare
I've decided not to do the integration test in this PR, so i've reverted that commit. I can do it as a follow-up, if it is needed. |
Through the experiment with testing, I have confirmed that the extension can be used with the cloud logging, monitoring, and trace APIs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// Attach system parameters | ||
if ca.config.QuotaProject != "" { | ||
metadata["X-goog-user-project"] = ca.config.QuotaProject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the X
be lowercase as well https://cloud.google.com/apis/docs/system-parameters#:~:text=HTTP%20request%20headers%20with%20keys%20in%20lowercase ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on what the current client does, the X is still capitalized: https://github.com/googleapis/google-api-go-client/blob/113082d14d54f188d1b6c34c652e416592fc51b5/transport/grpc/dial.go#L224
Similar to https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/extension/oauth2clientauthextension, but supports google application default credentials.
This extension:
Things I didn't do, but could have:
X-Goog-Request-Reason
, or other system parameters.Testing: I don't have a way to test this right now, but it seems simple enough that it should work.