Add in getHttpAuthenticationService to GrpcAuthenticationProvider #1529

dapowers87 · 2022-06-21T17:33:01Z

Signed-off-by: David Powers ddpowers@amazon.com

Description

Create an optional plugin to get an HTTP Auth service for the GRPC Auth Provider

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable · 2022-06-21T17:52:46Z

...ce-source/src/main/java/com/amazon/dataprepper/plugins/source/oteltrace/OTelTraceSource.java

@@ -102,7 +104,10 @@ public void start(Buffer<Record<Object>> buffer) {

            final ServerBuilder sb = Server.builder();
            sb.disableServerHeader();
-            sb.service(grpcServiceBuilder.build());
+
+            final Function<? super HttpService, ? extends HttpService> httpAuthenticationService = authenticationProvider.getHttpAuthenticationService().get();


This will throw an exception when the Optional is empty. This code should be refactored to check that the Optional is present. And only if it is add it to the service builder.

dlvenable · 2022-06-21T17:52:58Z

...ce-source/src/main/java/com/amazon/dataprepper/plugins/source/oteltrace/OTelTraceSource.java

@@ -102,7 +104,10 @@ public void start(Buffer<Record<Object>> buffer) {

            final ServerBuilder sb = Server.builder();
            sb.disableServerHeader();
-            sb.service(grpcServiceBuilder.build());
+
+            final Function<? super HttpService, ? extends HttpService> httpAuthenticationService = authenticationProvider.getHttpAuthenticationService().get();


Can we add tests for this code?

dlvenable · 2022-06-21T17:53:12Z

.../src/main/java/com/amazon/dataprepper/armeria/authentication/GrpcAuthenticationProvider.java

+     * Returns an optional service for HTTP authentication
+     * @since 1.5
+     */
+    default Optional<Function<? super HttpService, ? extends HttpService>> getHttpAuthenticationService() {


Please add a simple unit test to verify the default behavior.

Please be sure to include the unit test here. You can use a mock which calls actual methods to verify the behavior.

dlvenable · 2022-06-21T17:55:22Z

.../src/main/java/com/amazon/dataprepper/armeria/authentication/GrpcAuthenticationProvider.java

@@ -27,4 +31,12 @@ public interface GrpcAuthenticationProvider {
     * @since 1.2
     */
    ServerInterceptor getAuthenticationInterceptor();
+
+    /**
+     * Returns an optional service for HTTP authentication


We may wish to clarify this. This service is to intercept the HTTP request as part of performing gRPC authentication. Perhaps:

Allows implementors to provide an {@link HttpService} to either intercept the HTTP request prior to validation, or to perform validation on the HTTP request. This may be optional, in which case it is not used.

dlvenable · 2022-06-21T17:55:41Z

...ce-source/src/main/java/com/amazon/dataprepper/plugins/source/oteltrace/OTelTraceSource.java

@@ -102,7 +104,10 @@ public void start(Buffer<Record<Object>> buffer) {

            final ServerBuilder sb = Server.builder();
            sb.disableServerHeader();
-            sb.service(grpcServiceBuilder.build());
+
+            final Function<? super HttpService, ? extends HttpService> httpAuthenticationService = authenticationProvider.getHttpAuthenticationService().get();


Please make a similar change in the otel_metrics_source.

Signed-off-by: David Powers <ddpowers@amazon.com>

codecov-commenter · 2022-06-21T19:56:43Z

Codecov Report

Merging #1529 (bc6219d) into main (96ab0f7) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1529   +/-   ##
=========================================
  Coverage     94.18%   94.18%           
  Complexity     1181     1181           
=========================================
  Files           165      165           
  Lines          3386     3386           
  Branches        277      277           
=========================================
  Hits           3189     3189           
  Misses          141      141           
  Partials         56       56

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96ab0f7...bc6219d. Read the comment docs.

dlvenable · 2022-06-21T20:45:25Z

...ource/src/test/java/com/amazon/dataprepper/plugins/source/oteltrace/OTelTraceSourceTest.java

@@ -453,6 +455,19 @@ void start_without_Health_configured_does_not_include_HealthCheck_service() thro
        verify(grpcServiceBuilder, never()).addService(isA(HealthGrpcService.class));
    }

+    @Test
+    public void testOptionalHttpAuthServiceNotInPlace() {
+        try (MockedStatic<Server> armeriaServerMock = Mockito.mockStatic(Server.class)) {


This try-catch block only needs to be around the SOURCE.start(buffer) call. It will make it more understand if you tighten it to only be there.

when(server.stop()).thenReturn(completableFuture); try(MockedStatic<Server> armeriaServerMock = ...) { armeriaServerMock.when(Server::builder).thenReturn(serverBuilder); SOURCE.start(buffer); } verify(serverBuilder, never()).service(isA(GrpcService.class), isA(Function.class)); verify(serverBuilder).service(isA(GrpcService.class));

dlvenable · 2022-06-21T20:46:29Z

...ource/src/test/java/com/amazon/dataprepper/plugins/source/oteltrace/OTelTraceSourceTest.java

@@ -453,6 +455,19 @@ void start_without_Health_configured_does_not_include_HealthCheck_service() thro
        verify(grpcServiceBuilder, never()).addService(isA(HealthGrpcService.class));
    }

+    @Test


This is a good test. Please also add one that verifies the opposite when the Optional is not empty.

verify(serverBuilder).service(isA(GrpcService.class), isA(Function.class));
verify(serverBuilder, never()).service(isA(GrpcService.class));

dlvenable · 2022-06-21T20:47:15Z

.../src/main/java/com/amazon/dataprepper/armeria/authentication/GrpcAuthenticationProvider.java

+     * Returns an optional service for HTTP authentication
+     * @since 1.5
+     */
+    default Optional<Function<? super HttpService, ? extends HttpService>> getHttpAuthenticationService() {


Please be sure to include the unit test here. You can use a mock which calls actual methods to verify the behavior.

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable · 2022-06-22T15:34:23Z

...eria-common/src/test/java/com/amazon/dataprepper/plugins/GrpcAuthenticationProviderTest.java

+    public void confirmDefaultBehavior() {
+        when(provider.getHttpAuthenticationService()).thenCallRealMethod();
+
+        Assertions.assertNotNull(provider.getHttpAuthenticationService());


We are generally preferring Hamcrest matchers. But, I'm fine with this.

e.g.

assertThat(provider.getHttpAuthenticationService(), notNullValue());

Noted going forward ✅

dlvenable · 2022-06-22T15:35:46Z

...eria-common/src/test/java/com/amazon/dataprepper/plugins/GrpcAuthenticationProviderTest.java

+    public void confirmDefaultBehavior() {
+        when(provider.getHttpAuthenticationService()).thenCallRealMethod();
+
+        Assertions.assertNotNull(provider.getHttpAuthenticationService());


This should be extracted into a variable and re-used. Otherwise you are "testing" twice.

Optional<Function<? super HttpService, ? extends HttpService>> actualHttpService = provider.getHttpAuthenticationService());

Personally I'm also fine using var here. But, I know that can be contentious.

var actualHttpService = provider.getHttpAuthenticationService());

I'm all for var!

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable · 2022-06-22T16:57:49Z

Thanks for making these improvements to the authentication support and the code!

…ensearch-project#1529) * Add in getHttpAuthenticationService to GrpcAuthenticationProvider Signed-off-by: David Powers <ddpowers@amazon.com> Signed-off-by: Finn Roblin <finnrobl@amazon.com>

Add in getHttpAuthenticationService to GrpcAuthenticationProvider

5623503

Signed-off-by: David Powers <ddpowers@amazon.com>

dapowers87 requested a review from a team as a code owner June 21, 2022 17:33

Add javadoc

fa16593

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable requested changes Jun 21, 2022

View reviewed changes

David Powers added 2 commits June 21, 2022 14:04

Add unit tests, enhance javadoc

a9e8514

Signed-off-by: David Powers <ddpowers@amazon.com>

remove unused imports

3d8e425

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable requested changes Jun 21, 2022

View reviewed changes

David Powers added 2 commits June 21, 2022 16:17

additional tests, refactor the way the decorator was being applied

a62bbeb

Signed-off-by: David Powers <ddpowers@amazon.com>

add GrpcAuthenticationProviderTest

06bda59

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable reviewed Jun 22, 2022

View reviewed changes

dlvenable requested changes Jun 22, 2022

View reviewed changes

extract variable

bc6219d

Signed-off-by: David Powers <ddpowers@amazon.com>

dlvenable approved these changes Jun 22, 2022

View reviewed changes

dinujoh approved these changes Jun 22, 2022

View reviewed changes

dapowers87 merged commit ba6efac into opensearch-project:main Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add in getHttpAuthenticationService to GrpcAuthenticationProvider #1529

Add in getHttpAuthenticationService to GrpcAuthenticationProvider #1529

dapowers87 commented Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

codecov-commenter commented Jun 21, 2022 •

edited

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 21, 2022

dlvenable Jun 22, 2022

dapowers87 Jun 22, 2022

dlvenable Jun 22, 2022

dapowers87 Jun 22, 2022

dlvenable commented Jun 22, 2022

Add in getHttpAuthenticationService to GrpcAuthenticationProvider #1529

Add in getHttpAuthenticationService to GrpcAuthenticationProvider #1529

Conversation

dapowers87 commented Jun 21, 2022

Description

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 21, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable commented Jun 22, 2022

codecov-commenter commented Jun 21, 2022 •

edited