From 77e469b27708d2fabe6281082716a8c086f8107d Mon Sep 17 00:00:00 2001 From: ToineSiebelink Date: Tue, 1 Oct 2024 18:40:39 +0100 Subject: Policy Executor: handle errors, part 2 (fighting between IntelliJ and Checkstyle best practices) - non-2xx responses are processed using web client exceptions - handle unknown host exception - upgraded spotbugs (checkstyle and related mvn plugin) - fixed some small spotbugs due to upgrade - added commented instructions in docker compose to enable debugging - added some environment variables for policy executor configuration - extract out Sleeper in stub service to achieve 100% coverage - added cause to Policy Executor exceptions where applicable - ignored (new) spotbug rule about catch NPE because of issue in 3pp - ignored (new) spotbug rule about \n in string due to multiline string block Issue-ID: CPS-2412 Change-Id: I6835a73320c436cbeea12cc7a06f15899eec7bf1 Signed-off-by: ToineSiebelink --- .../cps/ncmp/api/exceptions/NcmpException.java | 13 +++- .../api/exceptions/PolicyExecutorException.java | 5 +- .../impl/data/policyexecutor/PolicyExecutor.java | 89 ++++++++++++---------- .../ncmp/impl/inventory/sync/lcm/LcmEventType.java | 4 +- .../data/policyexecutor/PolicyExecutorSpec.groovy | 65 +++++++++++++--- 5 files changed, 119 insertions(+), 57 deletions(-) (limited to 'cps-ncmp-service/src') diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java index 6754965866..3c81d0f536 100644 --- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java +++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/NcmpException.java @@ -39,7 +39,18 @@ public class NcmpException extends RuntimeException { * @param details the error details */ public NcmpException(final String message, final String details) { - super(message); + this(message, details, null); + } + + /** + * Constructor with cause. + * + * @param message the error message + * @param details the error details + * @param cause the cause of the exception + */ + public NcmpException(final String message, final String details, final Throwable cause) { + super(message, cause); this.details = details; } diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java index 333c12271b..bb753b85f1 100644 --- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java +++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/api/exceptions/PolicyExecutorException.java @@ -35,8 +35,9 @@ public class PolicyExecutorException extends NcmpException { * * @param message response message * @param details response details + * @param cause the cause of the exception */ - public PolicyExecutorException(final String message, final String details) { - super(message, details); + public PolicyExecutorException(final String message, final String details, final Throwable cause) { + super(message, details, cause); } } diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java index caed28a648..af4331893d 100644 --- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java +++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutor.java @@ -23,6 +23,7 @@ package org.onap.cps.ncmp.impl.data.policyexecutor; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import java.net.UnknownHostException; import java.time.Duration; import java.time.temporal.ChronoUnit; import java.util.Collections; @@ -45,6 +46,7 @@ import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Service; import org.springframework.web.reactive.function.BodyInserters; import org.springframework.web.reactive.function.client.WebClient; +import org.springframework.web.reactive.function.client.WebClientResponseException; @Slf4j @Service @@ -71,6 +73,8 @@ public class PolicyExecutor { private final ObjectMapper objectMapper; + private static final Throwable NO_ERROR = null; + /** * Use the Policy Executor to check permission for a cm write operation. * Wil throw an exception when the operation is not permitted (work in progress) @@ -88,26 +92,20 @@ public class PolicyExecutor { final String changeRequestAsJson) { log.trace("Policy Executor Enabled: {}", enabled); if (enabled) { - ResponseEntity responseEntity = null; try { - responseEntity = - getPolicyExecutorResponse(yangModelCmHandle, operationType, authorization, resourceIdentifier, - changeRequestAsJson); - } catch (final RuntimeException runtimeException) { - processException(runtimeException); - } - if (responseEntity == null) { - log.warn("No valid response from Policy Executor, ignored"); - return; - } - if (responseEntity.getStatusCode().is2xxSuccessful()) { - if (responseEntity.getBody() == null) { + final ResponseEntity responseEntity = getPolicyExecutorResponse(yangModelCmHandle, + operationType, + authorization, + resourceIdentifier, + changeRequestAsJson); + final JsonNode responseBody = responseEntity.getBody(); + if (responseBody == null) { log.warn("No valid response body from Policy Executor, ignored"); return; } - processSuccessResponse(responseEntity.getBody()); - } else { - processNon2xxResponse(responseEntity.getStatusCode().value()); + processSuccessResponse(responseBody); + } catch (final RuntimeException runtimeException) { + processException(runtimeException); } } } @@ -173,35 +171,17 @@ public class PolicyExecutor { .block(); } - private void processNon2xxResponse(final int httpStatusCode) { - processFallbackResponse("Policy Executor returned HTTP Status code " + httpStatusCode + "."); - } - - private void processException(final RuntimeException runtimeException) { - if (runtimeException.getCause() instanceof TimeoutException) { - processFallbackResponse("Policy Executor request timed out."); - } else { - log.warn("Request to Policy Executor failed with unexpected exception", runtimeException); - throw runtimeException; - } - } - - private void processFallbackResponse(final String message) { - final String decisionId = "N/A"; - final String decision = defaultDecision; - final String warning = message + " Falling back to configured default decision: " + defaultDecision; - log.warn(warning); - processDecision(decisionId, decision, warning); - } - private static void processSuccessResponse(final JsonNode responseBody) { final String decisionId = responseBody.path("decisionId").asText("unknown id"); final String decision = responseBody.path("decision").asText("unknown"); final String messageFromPolicyExecutor = responseBody.path("message").asText(); - processDecision(decisionId, decision, messageFromPolicyExecutor); + processDecision(decisionId, decision, messageFromPolicyExecutor, NO_ERROR); } - private static void processDecision(final String decisionId, final String decision, final String details) { + private static void processDecision(final String decisionId, + final String decision, + final String details, + final Throwable optionalCauseOfError) { log.trace("Policy Executor decision id: {} ", decisionId); if ("allow".equals(decision)) { log.trace("Operation allowed."); @@ -209,8 +189,37 @@ public class PolicyExecutor { log.warn("Policy Executor decision: {}", decision); log.warn("Policy Executor message: {}", details); final String message = "Operation not allowed. Decision id " + decisionId + " : " + decision; - throw new PolicyExecutorException(message, details); + throw new PolicyExecutorException(message, details, optionalCauseOfError); } } + private void processException(final RuntimeException runtimeException) { + if (runtimeException instanceof WebClientResponseException) { + final WebClientResponseException webClientResponseException = (WebClientResponseException) runtimeException; + final int httpStatusCode = webClientResponseException.getStatusCode().value(); + processFallbackResponse("Policy Executor returned HTTP Status code " + httpStatusCode + ".", + webClientResponseException); + } else { + final Throwable cause = runtimeException.getCause(); + if (cause instanceof TimeoutException) { + processFallbackResponse("Policy Executor request timed out.", cause); + } else if (cause instanceof UnknownHostException) { + final String message + = String.format("Cannot connect to Policy Executor (%s:%s).", serverAddress, serverPort); + processFallbackResponse(message, cause); + } else { + log.warn("Request to Policy Executor failed with unexpected exception", runtimeException); + throw runtimeException; + } + } + } + + private void processFallbackResponse(final String message, final Throwable cause) { + final String decisionId = "N/A"; + final String decision = defaultDecision; + final String warning = message + " Falling back to configured default decision: " + defaultDecision; + log.warn(warning); + processDecision(decisionId, decision, warning, cause); + } + } diff --git a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java index 4bc2f10218..1d4e3c8363 100644 --- a/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java +++ b/cps-ncmp-service/src/main/java/org/onap/cps/ncmp/impl/inventory/sync/lcm/LcmEventType.java @@ -26,10 +26,10 @@ public enum LcmEventType { private final String eventName; - private final String eventTypeTemplate = "org.onap.ncmp.cmhandle-lcm-event.%s"; + private static final String EVENT_TYPE_TEMPLATE = "org.onap.ncmp.cmhandle-lcm-event.%s"; LcmEventType(final String eventName) { - this.eventName = String.format(eventTypeTemplate, eventName); + this.eventName = String.format(EVENT_TYPE_TEMPLATE, eventName); } public String getEventType() { diff --git a/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy b/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy index 46c0ddeb93..33dcf5d623 100644 --- a/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy +++ b/cps-ncmp-service/src/test/groovy/org/onap/cps/ncmp/impl/data/policyexecutor/PolicyExecutorSpec.groovy @@ -33,6 +33,7 @@ import org.slf4j.LoggerFactory import org.springframework.http.HttpStatus import org.springframework.http.ResponseEntity import org.springframework.web.reactive.function.client.WebClient +import org.springframework.web.reactive.function.client.WebClientResponseException import reactor.core.publisher.Mono import spock.lang.Specification @@ -59,6 +60,8 @@ class PolicyExecutorSpec extends Specification { def setup() { setupLogger() objectUnderTest.enabled = true + objectUnderTest.serverAddress = 'some host' + objectUnderTest.serverPort = 'some port' mockWebClient.post() >> mockRequestBodyUriSpec mockRequestBodyUriSpec.uri(*_) >> mockRequestBodyUriSpec mockRequestBodyUriSpec.header(*_) >> mockRequestBodyUriSpec @@ -95,8 +98,8 @@ class PolicyExecutorSpec extends Specification { } def 'Permission check with non-2xx response and "allow" default decision.'() { - given: 'other http response' - mockResponse([], HttpStatus.I_AM_A_TEAPOT) + given: 'non-2xx http response' + mockErrorResponse() and: 'the configured default decision is "allow"' objectUnderTest.defaultDecision = 'allow' when: 'permission is checked for an operation' @@ -106,8 +109,8 @@ class PolicyExecutorSpec extends Specification { } def 'Permission check with non-2xx response and "other" default decision.'() { - given: 'other http response' - mockResponse([], HttpStatus.I_AM_A_TEAPOT) + given: 'non-2xx http response' + def webClientException = mockErrorResponse() and: 'the configured default decision is NOT "allow"' objectUnderTest.defaultDecision = 'deny by default' when: 'permission is checked for an operation' @@ -116,25 +119,23 @@ class PolicyExecutorSpec extends Specification { def thrownException = thrown(PolicyExecutorException) assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default' assert thrownException.details == 'Policy Executor returned HTTP Status code 418. Falling back to configured default decision: deny by default' + and: 'the cause is the original web client exception' + assert thrownException.cause == webClientException } def 'Permission check with invalid response from Policy Executor.'() { given: 'invalid response from Policy executor' - mockResponseSpec.toEntity(*_) >> invalidResponse + mockResponseSpec.toEntity(*_) >> Mono.just(new ResponseEntity<>(null, HttpStatus.OK)) when: 'permission is checked for an operation' objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson) then: 'system logs the expected message' - assert getLogEntry(1) == expectedMessage - where: 'following invalid responses are received' - invalidResponse || expectedMessage - Mono.empty() || 'No valid response from Policy Executor, ignored' - Mono.just(new ResponseEntity<>(null, HttpStatus.OK)) || 'No valid response body from Policy Executor, ignored' + assert getLogEntry(1) == 'No valid response body from Policy Executor, ignored' } def 'Permission check with timeout exception.'() { given: 'a timeout during the request' - def cause = new TimeoutException() - mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(cause) } + def timeoutException = new TimeoutException() + mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(timeoutException) } and: 'the configured default decision is NOT "allow"' objectUnderTest.defaultDecision = 'deny by default' when: 'permission is checked for an operation' @@ -143,6 +144,39 @@ class PolicyExecutorSpec extends Specification { def thrownException = thrown(PolicyExecutorException) assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default' assert thrownException.details == 'Policy Executor request timed out. Falling back to configured default decision: deny by default' + and: 'the cause is the original time out exception' + assert thrownException.cause == timeoutException + } + + def 'Permission check with unknown host.'() { + given: 'a unknown host exception during the request' + def unknownHostException = new UnknownHostException() + mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(unknownHostException) } + and: 'the configured default decision is NOT "allow"' + objectUnderTest.defaultDecision = 'deny by default' + when: 'permission is checked for an operation' + objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson) + then: 'Policy Executor exception is thrown' + def thrownException = thrown(PolicyExecutorException) + assert thrownException.message == 'Operation not allowed. Decision id N/A : deny by default' + assert thrownException.details == 'Cannot connect to Policy Executor (some host:some port). Falling back to configured default decision: deny by default' + and: 'the cause is the original unknown host exception' + assert thrownException.cause == unknownHostException + } + + def 'Permission check with #scenario exception and default decision "allow".'() { + given: 'a #scenario exception during the request' + mockResponseSpec.toEntity(*_) >> { throw new RuntimeException(cause)} + and: 'the configured default decision is "allow"' + objectUnderTest.defaultDecision = 'allow' + when: 'permission is checked for an operation' + objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson) + then: 'no exception is thrown' + noExceptionThrown() + where: 'the following exceptions are thrown during the request' + scenario | cause + 'timeout' | new TimeoutException() + 'unknown host' | new UnknownHostException() } def 'Permission check with other runtime exception.'() { @@ -180,6 +214,13 @@ class PolicyExecutorSpec extends Specification { mockResponseSpec.toEntity(*_) >> mono } + def mockErrorResponse() { + def webClientResponseException = Mock(WebClientResponseException) + webClientResponseException.getStatusCode() >> HttpStatus.I_AM_A_TEAPOT + mockResponseSpec.toEntity(*_) >> { throw webClientResponseException } + return webClientResponseException + } + def setupLogger() { def logger = LoggerFactory.getLogger(PolicyExecutor) logger.setLevel(Level.TRACE) -- cgit 1.2.3-korg