-
Notifications
You must be signed in to change notification settings - Fork 172
[gcp-auth-extension]: Try resolving GCP_PROJECT from Google credentials if not provided #2109
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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 | ||
| 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Then there is no need to invoke |
||
| 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) { | ||
|
|
@@ -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() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The javadoc string from line 755 to 769 applies only to
|
||
| 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( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
| 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 | ||
|
|
@@ -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 | ||
|
|
||
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: Can remove this comment.
I added this in the suggestion only to clarify my reasoning.