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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Oct 4, 2024
Merged

feat(gax): append cred-type header for auth metrics #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2dbd2d8
feat: expose credential type as part of x-goog-api-client header.
zhumin8 Sep 3, 2024
ee2624c
test: add test correctly appends cred-type to header.
zhumin8 Sep 10, 2024
fe3ed54
use enum for credential types.
zhumin8 Sep 13, 2024
0cd0ad8
update type.
zhumin8 Oct 1, 2024
b1dd5b0
fix naming and comment.
zhumin8 Oct 2, 2024
781f161
Merge branch 'main' into auth-metrics
zhumin8 Oct 2, 2024
01adbde
Merge branch 'main' into auth-metrics
zhumin8 Oct 3, 2024
32f8934
fix credential is null case.
zhumin8 Oct 3, 2024
51bed23
add test case.
zhumin8 Oct 3, 2024
0424fd2
update test, extract test setup steps.
zhumin8 Oct 3, 2024
0e69f72
Merge branch 'main' into auth-metrics
zhumin8 Oct 4, 2024
b50896a
address feedback: encapsulate credentials metrics logic inside getHea…
zhumin8 Oct 4, 2024
6e60633
address feedback.
zhumin8 Oct 4, 2024
33e24d9
update comment.
zhumin8 Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.google.api.gax.tracing.ApiTracerFactory;
import com.google.api.gax.tracing.BaseApiTracerFactory;
import com.google.auth.ApiKeyCredentials;
import com.google.auth.CredentialTypeForMetrics;
import com.google.auth.Credentials;
import com.google.auth.oauth2.GdchCredentials;
import com.google.auto.value.AutoValue;
Expand Down Expand Up @@ -210,7 +211,8 @@ public static ClientContext create(StubSettings settings) throws IOException {
if (transportChannelProvider.needsExecutor() && settings.getExecutorProvider() != null) {
transportChannelProvider = transportChannelProvider.withExecutor(backgroundExecutor);
}
Map<String, String> headers = getHeadersFromSettings(settings);

Map<String, String> headers = getHeaders(settings, credentials);
if (transportChannelProvider.needsHeaders()) {
transportChannelProvider = transportChannelProvider.withHeaders(headers);
}
Expand Down Expand Up @@ -318,8 +320,11 @@ static GdchCredentials getGdchCredentials(
/**
* Getting a header map from HeaderProvider and InternalHeaderProvider from settings with Quota
* Project Id.
*
* <p>Then if credentials is present and its type for metrics is not {@code
* CredentialTypeForMetrics.DO_NOT_SEND}, append this type info to x-goog-api-client header.
*/
private static Map<String, String> getHeadersFromSettings(StubSettings settings) {
private static Map<String, String> getHeaders(StubSettings settings, Credentials credentials) {
// Resolve conflicts when merging headers from multiple sources
Map<String, String> userHeaders = settings.getHeaderProvider().getHeaders();
Map<String, String> internalHeaders = settings.getInternalHeaderProvider().getHeaders();
Expand All @@ -346,6 +351,20 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings)
effectiveHeaders.putAll(userHeaders);
effectiveHeaders.putAll(conflictResolution);

return appendCredentialTypeToHeaderIfPresent(effectiveHeaders, credentials);
}

private static Map<String, String> appendCredentialTypeToHeaderIfPresent(
Map<String, String> effectiveHeaders, Credentials credentials) {
CredentialTypeForMetrics credentialTypeForMetrics =
Copy link
Collaborator

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.

credentials == null
? CredentialTypeForMetrics.DO_NOT_SEND
: credentials.getMetricsCredentialType();
if (credentialTypeForMetrics != CredentialTypeForMetrics.DO_NOT_SEND) {
effectiveHeaders.computeIfPresent(
ApiClientHeaderProvider.getDefaultApiClientHeaderKey(),
Copy link
Collaborator

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?

Copy link
Contributor Author

@zhumin8 zhumin8 Oct 4, 2024 •
edited
Loading

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

(key, value) -> value + " cred-type/" + credentialTypeForMetrics.getLabel());
}
return ImmutableMap.copyOf(effectiveHeaders);
}

Expand Down
82 changes: 82 additions & 0 deletions gax-java/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.api.core.ApiClock;
import com.google.api.gax.core.BackgroundResource;
Expand All @@ -53,6 +54,7 @@
import com.google.api.gax.rpc.testing.FakeStubSettings;
import com.google.api.gax.rpc.testing.FakeTransportChannel;
import com.google.auth.ApiKeyCredentials;
import com.google.auth.CredentialTypeForMetrics;
import com.google.auth.Credentials;
import com.google.auth.oauth2.ComputeEngineCredentials;
import com.google.auth.oauth2.GdchCredentials;
Expand Down Expand Up @@ -677,6 +679,86 @@ void testUserAgentConcat() throws Exception {
.containsEntry("user-agent", "user-supplied-agent internal-agent");
}

@Test
void testApiClientHeaderAppendsCredType() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.USER_CREDENTIALS);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsEntry("x-goog-api-client", "internal-agent cred-type/u");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenNoApiClientHeader() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.USER_CREDENTIALS);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("some-other-header", "internal-agent"));

assertThat(headers).doesNotContainKey("x-goog-api-client");
assertThat(headers).containsEntry("some-other-header", "internal-agent");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenNullCredentials() throws IOException {
Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
NoCredentialsProvider.create(),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsKey("x-goog-api-client");
assertThat(headers.get("x-goog-api-client")).doesNotContain("cred-type/");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenCredTypeDoNotSend() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.DO_NOT_SEND);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsKey("x-goog-api-client");
assertThat(headers.get("x-goog-api-client")).doesNotContain("cred-type/");
}

private Map<String, String> setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
CredentialsProvider credentialsProvider, HeaderProvider headerProvider) throws IOException {
TransportChannelProvider transportChannelProvider =
new FakeTransportProvider(
FakeTransportChannel.create(new FakeChannel()),
null,
true,
null,
null,
DEFAULT_ENDPOINT);

ClientSettings.Builder builder =
new FakeClientSettings.Builder()
.setExecutorProvider(
FixedExecutorProvider.create(Mockito.mock(ScheduledExecutorService.class)))
.setTransportChannelProvider(transportChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setInternalHeaderProvider(headerProvider);

ClientContext clientContext = ClientContext.create(builder.build());
FakeTransportChannel transportChannel =
(FakeTransportChannel) clientContext.getTransportChannel();
return transportChannel.getHeaders();
}

private static String endpoint = "https://foo.googleapis.com";
private static String mtlsEndpoint = "https://foo.mtls.googleapis.com";

Expand Down
Loading