Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 3 additions & 4 deletions gcp-auth-extension/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,13 @@ The extension can be configured either by environment variables or system proper

Here is a list of required and optional configuration available for the extension:

#### Required Config
#### Optional Config

- `GOOGLE_CLOUD_PROJECT`: Environment variable that represents the Google Cloud Project ID to which the telemetry needs to be exported.

- Can also be configured using `google.cloud.project` system property.
- This is a required option, the agent configuration will fail if this option is not set.

#### Optional Config
- If neither of these options are set, the extension will attempt to infer the project id from the current credentials as a fallback, however notice that not all credentials implementations will be able to provide a project id, so the inference is only a best-effort attempt.
- **Important Note**: The agent configuration will fail if this option is not set and cannot be inferred.

- `GOOGLE_CLOUD_QUOTA_PROJECT`: Environment variable that represents the Google Cloud Quota Project ID which will be charged for the GCP API usage. To learn more about a *quota project*, see the [Quota project overview](https://cloud.google.com/docs/quotas/quota-project) page. Additional details about configuring the *quota project* can be found on the [Set the quota project](https://cloud.google.com/docs/quotas/set-quota-project) page.

Expand Down
1 change: 1 addition & 0 deletions gcp-auth-extension/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ dependencies {
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine")
testImplementation("org.junit.jupiter:junit-jupiter-api")
testCompileOnly("org.junit.jupiter:junit-jupiter-params")
testImplementation("org.junit-pioneer:junit-pioneer")

testImplementation("io.opentelemetry:opentelemetry-api")
testImplementation("io.opentelemetry:opentelemetry-exporter-otlp")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ public void customize(@Nonnull AutoConfigurationCustomizer autoConfiguration) {
.addMetricExporterCustomizer(
(metricExporter, configProperties) ->
customizeMetricExporter(metricExporter, credentials, configProperties))
.addResourceCustomizer(GcpAuthAutoConfigurationCustomizerProvider::customizeResource);
.addResourceCustomizer(
(resource, configProperties) ->
customizeResource(resource, credentials, configProperties));
}

@Override
Expand Down Expand Up @@ -228,9 +230,19 @@ private static Map<String, String> getRequiredHeaderMap(
}

// Updates the current resource with the attributes required for ingesting OTLP data on GCP.
private static Resource customizeResource(Resource resource, ConfigProperties configProperties) {
String gcpProjectId =
ConfigurableOption.GOOGLE_CLOUD_PROJECT.getConfiguredValue(configProperties);
// Note that credentials can be passed from `customize` function directly
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can remove this comment.

I added this in the suggestion only to clarify my reasoning.

private static Resource customizeResource(
Resource resource, GoogleCredentials credentials, ConfigProperties configProperties) {
String gcpProjectId;
try {
gcpProjectId = ConfigurableOption.GOOGLE_CLOUD_PROJECT.getConfiguredValue(configProperties);
} catch (io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can replace it with an import statement.

gcpProjectId = credentials.getProjectId();
if (gcpProjectId == null) {
// this exception will still contain the accurate message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can remove this comment.

I added this in the suggestion only to clarify my reasoning.

throw e;
}
}
Resource res = Resource.create(Attributes.of(stringKey(GCP_USER_PROJECT_ID_KEY), gcpProjectId));
return resource.merge(res);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junitpioneer.jupiter.ClearSystemProperty;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.Mock;
Expand Down Expand Up @@ -456,6 +457,92 @@ void testQuotaProjectBehavior(QuotaProjectIdTestBehavior testCase) throws IOExce
}
}

@ParameterizedTest
@MethodSource("provideProjectIdBehaviorTestCases")
@ClearSystemProperty(key = "google.cloud.project")
@ClearSystemProperty(key = "google.otel.auth.target.signals")
@SuppressWarnings("CannotMockMethod")
void testProjectIdBehavior(ProjectIdTestBehavior testCase) throws IOException {

// configure environment according to test case
String userSpecifiedProjectId = testCase.getUserSpecifiedProjectId();
if (userSpecifiedProjectId != null && !userSpecifiedProjectId.isEmpty()) {
System.setProperty(
Copy link
Contributor

Choose a reason for hiding this comment

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

The user can pass empty strings as the projectID, as such the test should account for it.

ConfigurableOption.GOOGLE_CLOUD_PROJECT.getSystemProperty(), userSpecifiedProjectId);
}
System.setProperty(
ConfigurableOption.GOOGLE_OTEL_AUTH_TARGET_SIGNALS.getSystemProperty(), SIGNAL_TYPE_TRACES);

// prepare request metadata (may or may not be called depending on test scenario)
AccessToken fakeAccessToken = new AccessToken("fake", Date.from(Instant.now()));
ImmutableMap<String, List<String>> mockedRequestMetadata =
ImmutableMap.of(
"Authorization",
Collections.singletonList("Bearer " + fakeAccessToken.getTokenValue()));
Mockito.lenient()
.when(mockedGoogleCredentials.getRequestMetadata())
.thenReturn(mockedRequestMetadata);

// only mock getProjectId() if it will be called (i.e., user didn't specify project ID)
boolean shouldFallbackToCredentials =
userSpecifiedProjectId == null || userSpecifiedProjectId.isEmpty();
if (shouldFallbackToCredentials) {
Mockito.when(mockedGoogleCredentials.getProjectId())
.thenReturn(testCase.getCredentialsProjectId());
}

// prepare mock exporter
OtlpGrpcSpanExporter mockOtlpGrpcSpanExporter = Mockito.mock(OtlpGrpcSpanExporter.class);
OtlpGrpcSpanExporterBuilder spyOtlpGrpcSpanExporterBuilder =
Mockito.spy(OtlpGrpcSpanExporter.builder());
List<SpanData> exportedSpans = new ArrayList<>();
configureGrpcMockSpanExporter(
mockOtlpGrpcSpanExporter, spyOtlpGrpcSpanExporterBuilder, exportedSpans);

try (MockedStatic<GoogleCredentials> googleCredentialsMockedStatic =
Mockito.mockStatic(GoogleCredentials.class)) {
googleCredentialsMockedStatic
.when(GoogleCredentials::getApplicationDefault)
.thenReturn(mockedGoogleCredentials);

if (testCase.getExpectedToThrow()) {
// expect exception to be thrown when project ID is not available
assertThatThrownBy(
() -> {
OpenTelemetrySdk sdk =
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is expected to throw an exception, on account of improper configuration (User project ID cannot be determined), then it should be thrown only from buildOpenTelemetrySdkWithExporter, right ?

Then there is no need to invoke generateTestSpan or shutdown on the SDK - we should remove these, because this test will pass even if the exception is coming from generateTestSpan or shutdown.

buildOpenTelemetrySdkWithExporter(mockOtlpGrpcSpanExporter);
generateTestSpan(sdk);
sdk.shutdown().join(10, TimeUnit.SECONDS);
})
.isInstanceOf(ConfigurationException.class);
// verify getProjectId() was called to attempt fallback
Mockito.verify(mockedGoogleCredentials, Mockito.times(1)).getProjectId();
} else {
// export telemetry and verify resource attributes contain expected project ID
OpenTelemetrySdk sdk = buildOpenTelemetrySdkWithExporter(mockOtlpGrpcSpanExporter);
generateTestSpan(sdk);
CompletableResultCode code = sdk.shutdown();
CompletableResultCode joinResult = code.join(10, TimeUnit.SECONDS);
assertThat(joinResult.isSuccess()).isTrue();

assertThat(exportedSpans).hasSizeGreaterThan(0);
for (SpanData spanData : exportedSpans) {
assertThat(spanData.getResource().getAttributes().asMap())
.containsEntry(
AttributeKey.stringKey(GCP_USER_PROJECT_ID_KEY),
testCase.getExpectedProjectIdInResource());
}

// verify whether getProjectId() was called based on whether fallback was needed
if (shouldFallbackToCredentials) {
Mockito.verify(mockedGoogleCredentials, Mockito.times(1)).getProjectId();
} else {
Mockito.verify(mockedGoogleCredentials, Mockito.never()).getProjectId();
}
}
}
}

@ParameterizedTest
@MethodSource("provideTargetSignalBehaviorTestCases")
void testTargetSignalsBehavior(TargetSignalBehavior testCase) {
Expand Down Expand Up @@ -680,6 +767,34 @@ private static Stream<Arguments> provideTargetSignalBehaviorTestCases() {
* indicates that the mocked credentials are configured to provide DUMMY_GCP_QUOTA_PROJECT_ID as
* the quota project ID.
*/
private static Stream<Arguments> provideProjectIdBehaviorTestCases() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc string from line 755 to 769 applies only to provideQuotaBehaviorTestCases, please move it so that it sits right above that function.

provideProjectIdBehaviorTestCases would require a similar, but different doc string.

return Stream.of(
// User specified project ID takes precedence
Arguments.of(
ProjectIdTestBehavior.builder()
.setUserSpecifiedProjectId(DUMMY_GCP_RESOURCE_PROJECT_ID)
.setCredentialsProjectId("credentials-project-id")
.setExpectedProjectIdInResource(DUMMY_GCP_RESOURCE_PROJECT_ID)
.setExpectedToThrow(false)
.build()),
// If user doesn't specify project ID, fallback to credentials.getProjectId()
Arguments.of(
ProjectIdTestBehavior.builder()
.setUserSpecifiedProjectId(null)
.setCredentialsProjectId("credentials-project-id")
.setExpectedProjectIdInResource("credentials-project-id")
.setExpectedToThrow(false)
.build()),
// If user doesn't specify and credentials.getProjectId() returns null, throw exception
Arguments.of(
Copy link
Contributor

Choose a reason for hiding this comment

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

Some edge cases should also be tested here - particularly around the extension's behavior on empty string values ("") for setUserSpecifiedProjectId and setCredentialsProjectId.

ProjectIdTestBehavior.builder()
.setUserSpecifiedProjectId(null)
.setCredentialsProjectId(null)
.setExpectedProjectIdInResource(null)
.setExpectedToThrow(true)
.build()));
}

private static Stream<Arguments> provideQuotaBehaviorTestCases() {
return Stream.of(
// If quota project present in metadata, it will be used
Expand Down Expand Up @@ -839,6 +954,42 @@ private static void configureGrpcMockMetricExporter(
.thenReturn(MemoryMode.IMMUTABLE_DATA);
}

@AutoValue
abstract static class ProjectIdTestBehavior {
// A null user specified project ID represents the use case where user omits specifying it
@Nullable
abstract String getUserSpecifiedProjectId();

// The project ID that credentials.getProjectId() returns (can be null)
@Nullable
abstract String getCredentialsProjectId();

// The expected project ID in the resource attributes (null if exception expected)
@Nullable
abstract String getExpectedProjectIdInResource();

// Whether an exception is expected to be thrown
abstract boolean getExpectedToThrow();

static Builder builder() {
return new AutoValue_GcpAuthAutoConfigurationCustomizerProviderTest_ProjectIdTestBehavior
.Builder();
}

@AutoValue.Builder
abstract static class Builder {
abstract Builder setUserSpecifiedProjectId(String projectId);

abstract Builder setCredentialsProjectId(String projectId);

abstract Builder setExpectedProjectIdInResource(String projectId);

abstract Builder setExpectedToThrow(boolean expectedToThrow);

abstract ProjectIdTestBehavior build();
}
}

@AutoValue
abstract static class QuotaProjectIdTestBehavior {
// A null user specified quota represents the use case where user omits specifying quota
Expand Down