-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(gax): append cred-type header for auth metrics #3186
Conversation
@@ -178,6 +178,7 @@ public static ClientContext create(StubSettings settings) throws IOException { | |||
|
|||
String settingsGdchApiAudience = settings.getGdchApiAudience(); | |||
Credentials credentials = settings.getCredentialsProvider().getCredentials(); | |||
String credentialType = credentials.getCredentialType(); |
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.
What are the possible credential types? cc: @ldetmer as it may affect the API key project.
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.
Current metric design includes 5 types: u - UserCredentials, sa - ServiceAccountCredentials, jwt ServiceAccountCredentials with self signed jwt token flow, mds - ComputeEngineCredentials, imp - ImpersonatedCredentials
And for all other credentials, it defaults to "unknown".
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.
update: currently metrics will only send for above credential types, all other credentials will have type as CredentialTypeForMetrics.DO_NOT_SEND
by default, and will not append credential header.
@@ -322,6 +325,9 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings) | |||
effectiveHeaders.putAll(userHeaders); | |||
effectiveHeaders.putAll(conflictResolution); | |||
|
|||
effectiveHeaders.computeIfPresent( |
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.
I think handwritten libraries and/or customers can override credentials on request level, can we do more investigation? Maybe we can add this header on request level not client level.
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.
Let me look into this.
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.
Update on this: Consider this is a relatively niche case, to unblock this current change, I will look into this separately and create a followup if needed. Updated description to reflect.
context: b/339259830 and [go/send-auth-metrics-java](http://goto.google.com/send-auth-metrics-java) Changes include: - expose `Credentials` type via `getMetricsCredentialType()`. Override this method for UserCredentials, ServiceAccountCredentials, ImpersonatedCredentials, and ComputeEngineCredentials. This is used in both token request and token usage flows. - add metric headers for each of the in-scope token requests. Below are examples of each request flow with added metrics: - User credentials request (at/id): “gl-java/19.0.1 auth/1.24.3 cred-type/u” - SA credentials, VM credentials or Impersonated credentials requests (at/id): “gl-java/19.0.1 auth/1.24.3 auth-request-type/at cred-type/sa” - MDS ping (This is used in ADC during the credential detection): “gl-java/19.0.1 auth/1.24.3 auth-request-type/mds” - What is not tracked: ComputeEngineCredentials getUniverseDomain and getAccount does not send metrics header; TPC flows does not send metrics headers. Related pr: adding for cred_type for token usage requests googleapis/sdk-platform-java#3186
downstream check for java-bigquerystorage is failing because java-bigquerystorage has hardcoded versions for auth libraries. raising googleapis/java-bigquerystorage#2679 to fix. |
gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
Outdated
Show resolved
Hide resolved
@@ -346,6 +353,11 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings) | |||
effectiveHeaders.putAll(userHeaders); | |||
effectiveHeaders.putAll(conflictResolution); | |||
|
|||
if (credentialTypeForMetrics != CredentialTypeForMetrics.DO_NOT_SEND) { | |||
effectiveHeaders.computeIfPresent( | |||
ApiClientHeaderProvider.getDefaultApiClientHeaderKey(), |
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.
Is it decided to append cred-type/
to x-goog-api-client
header? It seems all other header tokens in x-goog-api-client
are static values from the client, not from user input. I wonder why we didn't choose to use a separate header key for this purpose?
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.
Is it decided to append cred-type/ to x-goog-api-client header?
yes, this design is already implemented for other some languages. See details in design go/googleapis-auth-metric-design section 2.3.2. I don't think I saw discussion on using a different header specifically though.
It seems all other header tokens in x-goog-api-client are static values from the client, not from user input.
IIUC, this header is eventually sent per request, at that time, I don't see credentials type much different than other client lib info? Do you have specific concerns about this?
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.
The concern is that x-goog-api-client
was constructed in the generated layer only with static values, the name of the header api-client
also seems to indicate that the header is supposed to contain only info for the client, not for requests.
In terms of the Java implementations, we now also have multiple places modifying the x-goog-api-client
value, in both ClientContext
and ApiClientHeaderProvider
, which make this header harder to maintain in the future and also more error prone.
That being said, it is probably too late to change the overall design. We just need to be ware of the consequences and add more docs if possible.
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.
I see your point.
Let me know if you see better suited places for additional docs.
@@ -346,6 +348,15 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings) | |||
effectiveHeaders.putAll(userHeaders); | |||
effectiveHeaders.putAll(conflictResolution); | |||
|
|||
CredentialTypeForMetrics credentialTypeForMetrics = |
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.
nit: extract this to a private method for better readability.
@@ -346,6 +353,11 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings) | |||
effectiveHeaders.putAll(userHeaders); | |||
effectiveHeaders.putAll(conflictResolution); | |||
|
|||
if (credentialTypeForMetrics != CredentialTypeForMetrics.DO_NOT_SEND) { | |||
effectiveHeaders.computeIfPresent( | |||
ApiClientHeaderProvider.getDefaultApiClientHeaderKey(), |
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.
The concern is that x-goog-api-client
was constructed in the generated layer only with static values, the name of the header api-client
also seems to indicate that the header is supposed to contain only info for the client, not for requests.
In terms of the Java implementations, we now also have multiple places modifying the x-goog-api-client
value, in both ClientContext
and ApiClientHeaderProvider
, which make this header harder to maintain in the future and also more error prone.
That being said, it is probably too late to change the overall design. We just need to be ware of the consequences and add more docs if possible.
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate failed for 'java_showcase_integration_tests'Failed conditions |
🤖 I have created a release *beep* *boop* --- <details><summary>2.47.0</summary> ## [2.47.0](v2.46.1...v2.47.0) (2024-10-04) ### Features * **gax:** add API key authentication to ClientSettings ([#3137](#3137)) ([df08956](df08956)) * **gax:** append cred-type header for auth metrics ([#3186](#3186)) ([ca3ec24](ca3ec24)) ### Bug Fixes * address incorrect universe domain validation when quota project id is set ([#3257](#3257)) ([6e70c37](6e70c37)), closes [#3256](#3256) * Disable automatically retrieving Universe Domain from Metadata Server ([#3272](#3272)) ([f4402bf](f4402bf)) ### Dependencies * update dependency com.fasterxml.jackson:jackson-bom to v2.18.0 ([#3248](#3248)) ([821e83d](821e83d)) * update dependency com.google.errorprone:error_prone_annotations to v2.33.0 ([#3265](#3265)) ([94450a9](94450a9)) * update dependency com.google.errorprone:error_prone_annotations to v2.33.0 ([#3266](#3266)) ([8235463](8235463)) * update dependency com.google.guava:guava to v33.3.1-jre ([#3228](#3228)) ([4e76207](4e76207)) * update dependency net.bytebuddy:byte-buddy to v1.15.3 ([#3246](#3246)) ([2aad71d](2aad71d)) * update google api dependencies ([#3242](#3242)) ([02aae9d](02aae9d)) * update google auth library dependencies to v1.28.0 ([#3267](#3267)) ([6d85864](6d85864)) * update googleapis/java-cloud-bom digest to 0cd97b7 ([#3260](#3260)) ([2d54a5d](2d54a5d)) * update grpc dependencies to v1.67.1 ([#3258](#3258)) ([e08906c](e08906c)) * update grpc dependencies to v1.67.1 in dependencies.properties ([#3279](#3279)) ([5b46e70](5b46e70)) * update junit5 monorepo to v5.11.2 ([#3276](#3276)) ([6b10f94](6b10f94)) * update netty dependencies to v4.1.114.final ([#3263](#3263)) ([8bd83d9](8bd83d9)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This is POC change in gax-java for auth metrics requirements on token usage. See go/googleapis-auth-metric-design for context.
Credentials will expose
getMetricsCredentialType()
method, this change appends it to existingx-goog-api-client
headerNote: Currently implement in gax at client level. There are 2 edge cases not covered and will create followups for: if handwritten library overrides credentials at rpc level; If handwritten library does not build on gax. (ref: b/370039645, b/370038458)
related change in
google-auth-library
googleapis/google-auth-library-java#1503 included in 1.28.0.