From d5d68f6d0a196ad8e445d464f1e9737b49535ddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Skaar=2C=20Bj=C3=B8rn-Andre?= Date: Tue, 25 Apr 2023 10:06:54 +0200 Subject: [PATCH 1/4] Implement retry mechanism for SocketExceptions --- .../java/no/ssb/guardian/RetryListener.java | 22 ++++++ .../MaskinportenAccessTokenController.java | 15 ++++ ...nportenAccessTokenControllerRetryTest.java | 76 +++++++++++++++++++ ...MaskinportenAccessTokenControllerTest.java | 15 ++-- 4 files changed, 121 insertions(+), 7 deletions(-) create mode 100644 src/main/java/no/ssb/guardian/RetryListener.java create mode 100644 src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java diff --git a/src/main/java/no/ssb/guardian/RetryListener.java b/src/main/java/no/ssb/guardian/RetryListener.java new file mode 100644 index 0000000..17e496c --- /dev/null +++ b/src/main/java/no/ssb/guardian/RetryListener.java @@ -0,0 +1,22 @@ +package no.ssb.guardian; + +import io.micronaut.retry.event.RetryEvent; +import io.micronaut.retry.event.RetryEventListener; +import jakarta.inject.Singleton; +import lombok.extern.slf4j.Slf4j; + +@Slf4j +@Singleton +public class RetryListener implements RetryEventListener { + @Override + public void onApplicationEvent(RetryEvent event) { + log.error("Request failed {} time(s) for {} with error:", event.getRetryState().currentAttempt(), + event.getSource().getExecutableMethod().getName(), + event.getThrowable()); + } + + @Override + public boolean supports(RetryEvent event) { + return true; + } +} diff --git a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java index dd71cfb..df380fb 100644 --- a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java +++ b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java @@ -9,6 +9,8 @@ import io.micronaut.http.annotation.Post; import io.micronaut.http.hateoas.JsonError; import io.micronaut.http.hateoas.Link; +import io.micronaut.retry.annotation.RetryPredicate; +import io.micronaut.retry.annotation.Retryable; import io.micronaut.security.annotation.Secured; import io.micronaut.security.rules.SecurityRule; import lombok.AllArgsConstructor; @@ -23,6 +25,7 @@ import no.ssb.guardian.maskinporten.config.MaskinportenClientConfig; import no.ssb.guardian.maskinporten.config.MaskinportenConfig; +import java.net.SocketException; import java.security.Principal; import java.util.Set; @@ -37,6 +40,7 @@ public class MaskinportenAccessTokenController { private final MaskinportenConfig maskinportenConfig; @Post("/maskinporten/access-token") + @Retryable(predicate = WrappedSocketExceptionRetryPredicate.class) public HttpResponse fetchMaskinportenAccessToken(Principal principal, FetchMaskinportenAccessTokenRequest request) { log.info("Request: {}", request); log.info("AUDIT {}", PrincipalUtil.auditInfoOf(principal)); @@ -130,6 +134,17 @@ public ClientRequestException(HttpStatus httpStatus, String errorTag, String mes } + /** + * The {@link io.micronaut.retry.annotation.DefaultRetryPredicate} does not support wrapped exceptions, so, this + * prediate checks the exception {@code cause} for a {@link SocketException}. + */ + public static class WrappedSocketExceptionRetryPredicate implements RetryPredicate { + @Override + public boolean test(Throwable throwable) { + return throwable.getCause() != null && throwable.getCause().getClass().equals(SocketException.class); + } + } + @Data @NoArgsConstructor @AllArgsConstructor @Builder diff --git a/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java new file mode 100644 index 0000000..ee35283 --- /dev/null +++ b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java @@ -0,0 +1,76 @@ +package no.ssb.guardian.maskinporten; + +import io.micronaut.http.HttpStatus; +import io.micronaut.runtime.server.EmbeddedServer; +import io.micronaut.test.annotation.MockBean; +import io.micronaut.test.extensions.junit5.annotation.MicronautTest; +import io.restassured.RestAssured; +import io.restassured.http.ContentType; +import jakarta.inject.Inject; +import no.ssb.guardian.maskinporten.MaskinportenAccessTokenController.FetchMaskinportenAccessTokenRequest; +import no.ssb.guardian.maskinporten.client.MaskinportenClient; +import no.ssb.guardian.maskinporten.client.MaskinportenClientRegistry; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.net.SocketException; +import java.util.Set; + +import static io.restassured.RestAssured.*; +import static no.ssb.guardian.testsupport.KeycloakDevTokenIssuer.*; +import static org.hamcrest.Matchers.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; + +@MicronautTest +public class MaskinportenAccessTokenControllerRetryTest { + + private final static String MASKINPORTEN_ACCESS_TOKEN_ENDPOINT = "/maskinporten/access-token"; + private final static String MASKINPORTEN_DUMMY_ACCESS_TOKEN = "maskinporten-dummy-token"; + private final static String MASKINPORTEN_CLIENT_ID_1 = "7ea43b76-6b7d-49e8-af2b-4114ebb66c80"; + private final static Set REQUESTED_SCOPES = Set.of("some:scope1"); + + @Inject + private EmbeddedServer embeddedServer; + + @Inject + MaskinportenClientRegistry maskinportenClientRegistry; + + @MockBean(MaskinportenClientRegistry.class) + MaskinportenClientRegistry maskinportenklientRegistry() { + return mock(MaskinportenClientRegistry.class); + } + + private MaskinportenClient maskinportenClientMock = mock(MaskinportenClient.class); + @BeforeEach + void setUp() { + RestAssured.port = embeddedServer.getPort(); + when(maskinportenClientRegistry.get(any())).thenReturn(maskinportenClientMock); + } + + @Test + void validRequest_shouldRetryOnConnectionError() { + when(maskinportenClientMock.getAccessToken(anyCollection())) + .thenThrow(new RuntimeException(new SocketException(("Connection reset")))) + .thenReturn(MASKINPORTEN_DUMMY_ACCESS_TOKEN); + + given() + .auth().oauth2(personalAccessToken("Kjell", "Fjell")) + .contentType(ContentType.JSON) + .when() + .body(FetchMaskinportenAccessTokenRequest.builder() + .maskinportenClientId(MASKINPORTEN_CLIENT_ID_1) + .scopes(REQUESTED_SCOPES) + .build() + ) + .post(MASKINPORTEN_ACCESS_TOKEN_ENDPOINT) + .then() + .statusCode(HttpStatus.OK.getCode()) + .body( + "accessToken", equalTo(MASKINPORTEN_DUMMY_ACCESS_TOKEN) + ); + verify(maskinportenClientMock, times(2)).getAccessToken(anyCollection()); + } +} diff --git a/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerTest.java b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerTest.java index 4115d36..261229f 100644 --- a/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerTest.java +++ b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerTest.java @@ -23,8 +23,7 @@ import static org.hamcrest.Matchers.startsWith; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyCollection; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @MicronautTest public class MaskinportenAccessTokenControllerTest { @@ -33,7 +32,6 @@ public class MaskinportenAccessTokenControllerTest { private final static String MASKINPORTEN_DUMMY_ACCESS_TOKEN = "maskinporten-dummy-token"; private final static String MASKINPORTEN_CLIENT_ID_1 = "7ea43b76-6b7d-49e8-af2b-4114ebb66c80"; private final static String MASKINPORTEN_CLIENT_ID_2 = "675c0111-2035-4d15-9cce-037f55439e80"; - private final static Set DEFAULT_SCOPES = Set.of("some:scope1", "some:scope2"); private final static Set REQUESTED_SCOPES = Set.of("some:scope1"); @Inject @@ -41,6 +39,8 @@ public class MaskinportenAccessTokenControllerTest { @Inject MaskinportenClientRegistry maskinportenClientRegistry; + private MaskinportenClient maskinportenClientMock = mock(MaskinportenClient.class); + @MockBean(MaskinportenClientRegistry.class) MaskinportenClientRegistry maskinportenklientRegistry() { @@ -50,10 +50,8 @@ MaskinportenClientRegistry maskinportenklientRegistry() { @BeforeEach void setUp() { RestAssured.port = embeddedServer.getPort(); - - MaskinportenClient maskinportenClient = mock(MaskinportenClient.class); - when(maskinportenClient.getAccessToken(anyCollection())).thenReturn(MASKINPORTEN_DUMMY_ACCESS_TOKEN); - when(maskinportenClientRegistry.get(any())).thenReturn(maskinportenClient); + when(maskinportenClientMock.getAccessToken(anyCollection())).thenReturn(MASKINPORTEN_DUMMY_ACCESS_TOKEN); + when(maskinportenClientRegistry.get(any())).thenReturn(maskinportenClientMock); } private String serviceAccountKeycloakToken() { @@ -76,6 +74,7 @@ void validServiceAccount_getAccessToken_shouldReturnToken() { .body( "accessToken", equalTo(MASKINPORTEN_DUMMY_ACCESS_TOKEN) ); + verify(maskinportenClientMock, times(1)).getAccessToken(anyCollection()); } @Test @@ -91,6 +90,7 @@ void serviceAccount_getAccessTokenWithEmptyRequestBody_shouldReturnToken() { .body( "accessToken", equalTo(MASKINPORTEN_DUMMY_ACCESS_TOKEN) ); + verify(maskinportenClientMock, times(1)).getAccessToken(anyCollection()); } @Test @@ -109,6 +109,7 @@ void serviceAccount_getAccessTokenWithExplicitlySpecifiedMaskinportenClientId_sh .body( "message", equalTo("maskinportenClientId cannot be explicitly specified for service account users") ); + verifyNoInteractions(maskinportenClientMock); } @Test From f6de92c729e3efb2d17de327499a3cc0960584e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Skaar=2C=20Bj=C3=B8rn-Andre?= Date: Tue, 25 Apr 2023 10:22:22 +0200 Subject: [PATCH 2/4] Merge branch 'master' into feature/STAT-478 # Conflicts: # src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerTest.java --- .../MaskinportenAccessTokenControllerRetryTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java index ee35283..33d7303 100644 --- a/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java +++ b/src/test/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenControllerRetryTest.java @@ -52,7 +52,7 @@ void setUp() { @Test void validRequest_shouldRetryOnConnectionError() { - when(maskinportenClientMock.getAccessToken(anyCollection())) + when(maskinportenClientMock.getAccessToken(anySet())) .thenThrow(new RuntimeException(new SocketException(("Connection reset")))) .thenReturn(MASKINPORTEN_DUMMY_ACCESS_TOKEN); @@ -71,6 +71,6 @@ void validRequest_shouldRetryOnConnectionError() { .body( "accessToken", equalTo(MASKINPORTEN_DUMMY_ACCESS_TOKEN) ); - verify(maskinportenClientMock, times(2)).getAccessToken(anyCollection()); + verify(maskinportenClientMock, times(2)).getAccessToken(anySet()); } } From 074f69a9b521f7db5e67270e2ec6b7d51b277c8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Skaar=2C=20Bj=C3=B8rn-Andre?= Date: Tue, 25 Apr 2023 10:53:06 +0200 Subject: [PATCH 3/4] Added metric for runtime exceptions and additional error logs --- src/main/java/no/ssb/guardian/RetryListener.java | 6 ++++-- .../maskinporten/MaskinportenAccessTokenController.java | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/no/ssb/guardian/RetryListener.java b/src/main/java/no/ssb/guardian/RetryListener.java index 17e496c..31332fe 100644 --- a/src/main/java/no/ssb/guardian/RetryListener.java +++ b/src/main/java/no/ssb/guardian/RetryListener.java @@ -1,5 +1,6 @@ package no.ssb.guardian; +import io.micronaut.core.type.MutableArgumentValue; import io.micronaut.retry.event.RetryEvent; import io.micronaut.retry.event.RetryEventListener; import jakarta.inject.Singleton; @@ -10,8 +11,9 @@ public class RetryListener implements RetryEventListener { @Override public void onApplicationEvent(RetryEvent event) { - log.error("Request failed {} time(s) for {} with error:", event.getRetryState().currentAttempt(), - event.getSource().getExecutableMethod().getName(), + final MutableArgumentValue request = event.getSource().getParameters().get("request"); + log.error("Request failed {} time(s) for {}:", event.getRetryState().currentAttempt(), + request != null ? request.getValue(): "", event.getThrowable()); } diff --git a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java index df380fb..af75676 100644 --- a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java +++ b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java @@ -183,6 +183,11 @@ public HttpResponse clientRequestError(HttpRequest request, Maskinpor return error(request, e, e.getHttpStatus(), e.getMessage()); } + @Error + public HttpResponse runtimeError(HttpRequest request, RuntimeException e) { + metrics.incrementClientError("runtime-exception"); + return error(request, e, HttpStatus.INTERNAL_SERVER_ERROR, e.getMessage()); + } private static HttpResponse error(HttpRequest request, Exception e, HttpStatus httpStatus, String httpStatusReason) { JsonError error = new JsonError(e.getMessage()) .link(Link.SELF, Link.of(request.getUri())); From e886ad613d82074b1df995a6163871e23d2baf7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Skaar=2C=20Bj=C3=B8rn-Andre?= Date: Tue, 25 Apr 2023 11:27:20 +0200 Subject: [PATCH 4/4] Changed to server error --- .../maskinporten/MaskinportenAccessTokenController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java index af75676..7a67ed4 100644 --- a/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java +++ b/src/main/java/no/ssb/guardian/maskinporten/MaskinportenAccessTokenController.java @@ -185,7 +185,7 @@ public HttpResponse clientRequestError(HttpRequest request, Maskinpor @Error public HttpResponse runtimeError(HttpRequest request, RuntimeException e) { - metrics.incrementClientError("runtime-exception"); + metrics.incrementServerError("runtime-exception"); return error(request, e, HttpStatus.INTERNAL_SERVER_ERROR, e.getMessage()); } private static HttpResponse error(HttpRequest request, Exception e, HttpStatus httpStatus, String httpStatusReason) {