Skip to content

Commit 090127c

Browse files
author
Tomas Longo
committed
[WIP] Fix issue with http service being enabled, while grpc service accepts unframed requests
Signed-off-by: Tomas Longo <tomas.longo@sap.com>
1 parent f3644ad commit 090127c

5 files changed

Lines changed: 31 additions & 21 deletions

File tree

‎data-prepper-plugins/otel-metrics-source/src/test/java/org/opensearch/dataprepper/plugins/source/otelmetrics/OTelMetricsSourceTest.java‎

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,6 @@ void testHttpFullJsonWithCustomPathAndUnframedRequests() throws InvalidProtocolB
350350
.join();
351351
}
352352

353-
354353
@Test
355354
void testHttpFullJsonWithCustomPathAndAuthHeader_with_successful_response() throws InvalidProtocolBufferException {
356355
when(httpBasicAuthenticationConfig.getUsername()).thenReturn(USERNAME);
@@ -420,7 +419,7 @@ void testHttpRequestWithInvalidCredentials_with_unsuccessful_response() throws I
420419
when(httpBasicAuthenticationConfig.getUsername()).thenReturn(USERNAME);
421420
when(httpBasicAuthenticationConfig.getPassword()).thenReturn(PASSWORD);
422421
final GrpcAuthenticationProvider grpcAuthenticationProvider = new GrpcBasicAuthenticationProvider(httpBasicAuthenticationConfig);
423-
422+
424423
when(pluginFactory.loadPlugin(eq(GrpcAuthenticationProvider.class), any(PluginSetting.class)))
425424
.thenReturn(grpcAuthenticationProvider);
426425
when(oTelMetricsSourceConfig.getAuthentication()).thenReturn(new PluginModel("http_basic",
@@ -430,17 +429,17 @@ void testHttpRequestWithInvalidCredentials_with_unsuccessful_response() throws I
430429
)));
431430
when(oTelMetricsSourceConfig.enableUnframedRequests()).thenReturn(true);
432431
when(oTelMetricsSourceConfig.getPath()).thenReturn(TEST_PATH);
433-
432+
434433
configureObjectUnderTest();
435434
SOURCE.start(buffer);
436-
435+
437436
final String invalidUsername = "wrong_user";
438437
final String invalidPassword = "wrong_password";
439438
final String invalidCredentials = Base64.getEncoder()
440439
.encodeToString(String.format("%s:%s", invalidUsername, invalidPassword).getBytes(StandardCharsets.UTF_8));
441-
440+
442441
final String transformedPath = "/" + TEST_PIPELINE_NAME + "/v1/metrics";
443-
442+
444443
WebClient.of().prepare()
445444
.post("http://127.0.0.1:21891" + transformedPath)
446445
.content(MediaType.JSON_UTF_8, JsonFormat.printer().print(createExportMetricsRequest()).getBytes())
@@ -450,7 +449,7 @@ void testHttpRequestWithInvalidCredentials_with_unsuccessful_response() throws I
450449
.whenComplete((response, throwable) -> assertSecureResponseWithStatusCode(response, HttpStatus.UNAUTHORIZED, throwable))
451450
.join();
452451
}
453-
452+
454453
@Test
455454
void testGrpcRequestWithInvalidCredentials_with_unsuccessful_response() throws Exception {
456455
when(httpBasicAuthenticationConfig.getUsername()).thenReturn(USERNAME);
@@ -489,10 +488,10 @@ void testHttpWithoutSslFailsWhenSslIsEnabled() throws InvalidProtocolBufferExcep
489488
when(oTelMetricsSourceConfig.getSslKeyFile()).thenReturn("data/certificate/test_decrypted_key.key");
490489
configureObjectUnderTest();
491490
SOURCE.start(buffer);
492-
491+
493492
WebClient client = WebClient.builder("http://127.0.0.1:21891")
494493
.build();
495-
494+
496495
CompletionException exception = assertThrows(CompletionException.class, () -> client.execute(RequestHeaders.builder()
497496
.scheme(SessionProtocol.HTTP)
498497
.authority("127.0.0.1:21891")
@@ -503,28 +502,28 @@ void testHttpWithoutSslFailsWhenSslIsEnabled() throws InvalidProtocolBufferExcep
503502
HttpData.copyOf(JsonFormat.printer().print(createExportMetricsRequest()).getBytes()))
504503
.aggregate()
505504
.join());
506-
505+
507506
assertThat(exception.getCause(), instanceOf(ClosedSessionException.class));
508507
}
509-
508+
510509
@Test
511510
void testGrpcFailsIfSslIsEnabledAndNoTls() {
512511
when(oTelMetricsSourceConfig.isSsl()).thenReturn(true);
513512
when(oTelMetricsSourceConfig.getSslKeyCertChainFile()).thenReturn("data/certificate/test_cert.crt");
514513
when(oTelMetricsSourceConfig.getSslKeyFile()).thenReturn("data/certificate/test_decrypted_key.key");
515514
configureObjectUnderTest();
516515
SOURCE.start(buffer);
517-
516+
518517
MetricsServiceGrpc.MetricsServiceBlockingStub client = Clients.builder(GRPC_ENDPOINT)
519518
.build(MetricsServiceGrpc.MetricsServiceBlockingStub.class);
520-
519+
521520
StatusRuntimeException actualException = assertThrows(StatusRuntimeException.class, () -> client.export(createExportMetricsRequest()));
522-
521+
523522
assertThat(actualException.getStatus(), notNullValue());
524523
assertThat(actualException.getStatus().getCode(), equalTo(Status.Code.UNKNOWN));
525524
}
526-
527-
525+
526+
528527
@Test
529528
void testServerStartCertFileSuccess() throws IOException {
530529
try (MockedStatic<Server> armeriaServerMock = Mockito.mockStatic(Server.class)) {

‎data-prepper-plugins/otel-trace-source/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/OTelTraceSource.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,10 @@ public void start(Buffer<Record<Object>> buffer) {
8888
configureTaskExecutor(serverBuilder);
8989

9090
configureGrpcService(serverBuilder, buffer);
91-
configureHttpService(serverBuilder, buffer);
91+
// todo tlongo needed until clarified if unframedRequests should survive
92+
if (!oTelTraceSourceConfig.enableUnframedRequests()) {
93+
configureHttpService(serverBuilder, buffer);
94+
}
9295

9396
server = serverBuilder.build();
9497

‎data-prepper-plugins/otel-trace-source/src/main/java/org/opensearch/dataprepper/plugins/source/oteltrace/http/HttpService.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ private ArmeriaHttpAuthenticationProvider createAuthenticationProvider(final Plu
7676
// - it becomes testable
7777
// cons:
7878
// - currently tied to one impl by using 'new'.
79+
// todo tlongo clarify if simplifying config handling is something worth considering in this PR
7980
return new HttpBasicArmeriaHttpAuthenticationProvider(new HttpBasicAuthenticationConfig(pluginSettings.get("username").toString(), pluginSettings.get("password").toString()));
8081
}
8182
}

‎data-prepper-plugins/otel-trace-source/src/test/java/org/opensearch/dataprepper/plugins/source/oteltrace/OTelTraceSourceTest.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import org.apache.commons.io.IOUtils;
4141
import org.junit.jupiter.api.AfterEach;
4242
import org.junit.jupiter.api.BeforeEach;
43+
import org.junit.jupiter.api.Disabled;
4344
import org.junit.jupiter.api.Test;
4445
import org.junit.jupiter.api.extension.ExtendWith;
4546
import org.mockito.ArgumentCaptor;
@@ -600,10 +601,15 @@ void testOptionalHttpAuthServiceNotInPlace() {
600601
}
601602

602603
@Test
603-
void testOptionalHttpAuthServiceInPlace() {
604+
@Disabled
605+
void testOptionalHttpAuthServiOceInPlace() {
604606
final Optional<Function<? super HttpService, ? extends HttpService>> function = Optional.of(httpService -> httpService);
605607

606608
final Map<String, Object> settingsMap = new HashMap<>();
609+
// todo tlongo: Providing this pipeline config to data prepper let's it refuse to boot right away. Why is this test green, then?
610+
// Because the it's the plugin factory that stumbles over the missing plugin settings. But, the plugin factory is
611+
// mocked in this whole test class, always returning a GrpcBasicAuthenticationProvider. IMO this test does not
612+
// service its purpose
607613
settingsMap.put("authentication", new PluginModel("test", null));
608614
settingsMap.put("unauthenticated_health_check", true);
609615

@@ -627,6 +633,7 @@ void testOptionalHttpAuthServiceInPlace() {
627633
}
628634

629635
@Test
636+
@Disabled // todo tlongo see testOptionalHttpAuthServiOceInPlace on why I disabled this test
630637
void testOptionalHttpAuthServiceInPlaceWithUnauthenticatedDisabled() {
631638
final Optional<Function<? super HttpService, ? extends HttpService>> function = Optional.of(httpService -> httpService);
632639

‎data-prepper-plugins/otel-trace-source/src/test/java/org/opensearch/dataprepper/plugins/source/oteltrace/OTelTraceSource_GrpcRequestTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ void gRPC_request_with_custom_path_throws_when_written_to_default_path() {
270270

271271
final StatusRuntimeException actualException = assertThrows(StatusRuntimeException.class, () -> client.export(createExportTraceRequest()));
272272
assertThat(actualException.getStatus(), notNullValue());
273-
assertThat(actualException.getStatus().getCode(), equalTo(Status.UNIMPLEMENTED.getCode()));
273+
assertThat(actualException.getMessage(), actualException.getStatus().getCode(), equalTo(Status.UNIMPLEMENTED.getCode()));
274274
}
275275

276276
@ParameterizedTest
@@ -291,7 +291,7 @@ void gRPC_request_returns_expected_status_for_exceptions_from_buffer(
291291
final StatusRuntimeException actualException = assertThrows(StatusRuntimeException.class, () -> client.export(exportTraceRequest));
292292

293293
assertThat(actualException.getStatus(), notNullValue());
294-
assertThat(actualException.getStatus().getCode(), equalTo(expectedStatusCode));
294+
assertThat(actualException.getMessage(), actualException.getStatus().getCode(), equalTo(expectedStatusCode));
295295
}
296296

297297
@Test
@@ -306,7 +306,7 @@ void gRPC_request_throws_InvalidArgument_for_malformed_trace_data() {
306306
final StatusRuntimeException actualException = assertThrows(StatusRuntimeException.class, () -> client.export(exportTraceRequest));
307307

308308
assertThat(actualException.getStatus(), notNullValue());
309-
assertThat(actualException.getStatus().getCode(), equalTo(Status.Code.INVALID_ARGUMENT));
309+
assertThat(actualException.getMessage(), actualException.getStatus().getCode(), equalTo(Status.Code.INVALID_ARGUMENT));
310310

311311
verifyNoInteractions(buffer);
312312
}

0 commit comments

Comments
 (0)