diff options
author | Sourabh Sourabh <sourabh.sourabh@est.tech> | 2024-10-08 15:51:01 +0000 |
---|---|---|
committer | Gerrit Code Review <gerrit@onap.org> | 2024-10-08 15:51:01 +0000 |
commit | e2517a8b993ed884edb251b91ce600d0a1a9fefe (patch) | |
tree | e26cc91a338b296275ee6d579a3a3394c4eb0ade | |
parent | c024f967a84719b0ce6d2c546be4c31d496b6e22 (diff) | |
parent | 77e469b27708d2fabe6281082716a8c086f8107d (diff) |
Merge "Policy Executor: handle errors, part 2 (fighting between IntelliJ and Checkstyle best practices)"
21 files changed, 235 insertions, 79 deletions
diff --git a/checkstyle/src/main/resources/cps-java-style.xml b/checkstyle/src/main/resources/cps-java-style.xml index 67b2863695..d10484c31e 100644 --- a/checkstyle/src/main/resources/cps-java-style.xml +++ b/checkstyle/src/main/resources/cps-java-style.xml @@ -32,4 +32,4 @@ </module> <module name="UnusedImports"/> </module> -</module>
\ No newline at end of file +</module> diff --git a/cps-application/src/main/resources/application.yml b/cps-application/src/main/resources/application.yml index b97eabacb8..05b0d09ac5 100644 --- a/cps-application/src/main/resources/application.yml +++ b/cps-application/src/main/resources/application.yml @@ -190,7 +190,7 @@ logging: ncmp: policy-executor: enabled: ${POLICY_SERVICE_ENABLED:false} - defaultDecision: "allow" + defaultDecision: ${POLICY_SERVICE_DEFAULT_DECISION:"allow"} server: address: ${POLICY_SERVICE_URL:http://policy-executor-stub} port: ${POLICY_SERVICE_PORT:8093} diff --git a/cps-dependencies/pom.xml b/cps-dependencies/pom.xml index 2defa709bd..58e87a8031 100644 --- a/cps-dependencies/pom.xml +++ b/cps-dependencies/pom.xml @@ -126,7 +126,12 @@ <dependency> <groupId>com.github.spotbugs</groupId> <artifactId>spotbugs</artifactId> - <version>4.2.3</version> + <version>4.8.6</version> + </dependency> + <dependency> + <groupId>com.github.spotbugs</groupId> + <artifactId>spotbugs-annotations</artifactId> + <version>3.1.3</version> </dependency> <dependency> <groupId>com.google.code.findbugs</groupId> diff --git a/cps-ncmp-rest/src/test/groovy/org/onap/cps/ncmp/rest/controller/NetworkCmProxyRestExceptionHandlerSpec.groovy b/cps-ncmp-rest/src/test/groovy/org/onap/cps/ncmp/rest/controller/NetworkCmProxyRestExceptionHandlerSpec.groovy index 9d36d106c7..e87acacc74 100644 --- a/cps-ncmp-rest/src/test/groovy/org/onap/cps/ncmp/rest/controller/NetworkCmProxyRestExceptionHandlerSpec.groovy +++ b/cps-ncmp-rest/src/test/groovy/org/onap/cps/ncmp/rest/controller/NetworkCmProxyRestExceptionHandlerSpec.groovy @@ -129,19 +129,19 @@ class NetworkCmProxyRestExceptionHandlerSpec extends Specification { then: 'an HTTP response is returned with correct message and details' assertTestResponse(response, expectedErrorCode, expectedErrorMessage, expectedErrorDetails) where: - scenario | exception || expectedErrorCode | expectedErrorMessage | expectedErrorDetails - 'CPS' | new CpsException(sampleErrorMessage, sampleErrorDetails) || INTERNAL_SERVER_ERROR | sampleErrorMessage | sampleErrorDetails - 'NCMP-server' | new ServerNcmpException(sampleErrorMessage, sampleErrorDetails) || INTERNAL_SERVER_ERROR | sampleErrorMessage | null - 'DMI Request' | new DmiRequestException(sampleErrorMessage, sampleErrorDetails) || BAD_REQUEST | sampleErrorMessage | null - 'Invalid Operation' | new InvalidOperationException('some reason') || BAD_REQUEST | 'some reason' | null - 'Unsupported Operation' | new OperationNotSupportedException('not yet') || BAD_REQUEST | 'not yet' | null - 'DataNode Validation' | new DataNodeNotFoundException('myDataspaceName', 'myAnchorName') || NOT_FOUND | 'DataNode not found' | null - 'other' | new IllegalStateException(sampleErrorMessage) || INTERNAL_SERVER_ERROR | sampleErrorMessage | null - 'Data Node Not Found' | new DataNodeNotFoundException('myDataspaceName', 'myAnchorName') || NOT_FOUND | 'DataNode not found' | 'DataNode not found' - 'Existing entry' | new AlreadyDefinedException('name',null) || CONFLICT | 'Already defined exception' | 'name already exists' - 'Existing entries' | AlreadyDefinedException.forDataNodes(['A', 'B'], 'myAnchorName') || CONFLICT | 'Already defined exception' | '2 data node(s) already exist' - 'Operation too large' | new PayloadTooLargeException(sampleErrorMessage) || PAYLOAD_TOO_LARGE | sampleErrorMessage | 'Check logs' - 'Policy Executor' | new PolicyExecutorException(sampleErrorMessage, sampleErrorDetails) || CONFLICT | sampleErrorMessage | sampleErrorDetails + scenario | exception || expectedErrorCode | expectedErrorMessage | expectedErrorDetails + 'CPS' | new CpsException(sampleErrorMessage, sampleErrorDetails) || INTERNAL_SERVER_ERROR | sampleErrorMessage | sampleErrorDetails + 'NCMP-server' | new ServerNcmpException(sampleErrorMessage, sampleErrorDetails) || INTERNAL_SERVER_ERROR | sampleErrorMessage | null + 'DMI Request' | new DmiRequestException(sampleErrorMessage, sampleErrorDetails) || BAD_REQUEST | sampleErrorMessage | null + 'Invalid Operation' | new InvalidOperationException('some reason') || BAD_REQUEST | 'some reason' | null + 'Unsupported Operation' | new OperationNotSupportedException('not yet') || BAD_REQUEST | 'not yet' | null + 'DataNode Validation' | new DataNodeNotFoundException('myDataspaceName', 'myAnchorName') || NOT_FOUND | 'DataNode not found' | null + 'other' | new IllegalStateException(sampleErrorMessage) || INTERNAL_SERVER_ERROR | sampleErrorMessage | null + 'Data Node Not Found' | new DataNodeNotFoundException('myDataspaceName', 'myAnchorName') || NOT_FOUND | 'DataNode not found' | 'DataNode not found' + 'Existing entry' | new AlreadyDefinedException('name',null) || CONFLICT | 'Already defined exception' | 'name already exists' + 'Existing entries' | AlreadyDefinedException.forDataNodes(['A', 'B'], 'myAnchorName') || CONFLICT | 'Already defined exception' | '2 data node(s) already exist' + 'Operation too large' | new PayloadTooLargeException(sampleErrorMessage) || PAYLOAD_TOO_LARGE | sampleErrorMessage | 'Check logs' + 'Policy Executor' | new PolicyExecutorException(sampleErrorMessage, sampleErrorDetails, null) || CONFLICT | sampleErrorMessage | sampleErrorDetails } def 'Post request with exception returns correct HTTP Status.'() { 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<JsonNode> 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<JsonNode> 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) diff --git a/cps-parent/pom.xml b/cps-parent/pom.xml index d583f1d008..256e7a7dee 100644 --- a/cps-parent/pom.xml +++ b/cps-parent/pom.xml @@ -154,12 +154,12 @@ <plugin> <groupId>com.github.spotbugs</groupId> <artifactId>spotbugs-maven-plugin</artifactId> - <version>4.4.2</version> + <version>4.8.6.4</version> <dependencies> <dependency> <groupId>com.github.spotbugs</groupId> <artifactId>spotbugs</artifactId> - <version>4.2.3</version> + <version>4.8.6</version> </dependency> <dependency> <groupId>${project.groupId}</groupId> diff --git a/cps-ri/pom.xml b/cps-ri/pom.xml index c3ad6a2aed..bcf6c802de 100644 --- a/cps-ri/pom.xml +++ b/cps-ri/pom.xml @@ -68,6 +68,11 @@ <artifactId>postgresql</artifactId>
<version>${postgres.version}</version>
</dependency>
+ <!-- Disable SpotBug Rules -->
+ <dependency>
+ <groupId>com.github.spotbugs</groupId>
+ <artifactId>spotbugs-annotations</artifactId>
+ </dependency>
<!-- Add Hibernate support for Postgres datatype JSONB and Postgres arrays -->
<dependency>
<groupId>io.hypersistence</groupId>
diff --git a/cps-ri/src/main/java/org/onap/cps/ri/repository/ModuleReferenceRepositoryImpl.java b/cps-ri/src/main/java/org/onap/cps/ri/repository/ModuleReferenceRepositoryImpl.java index 702b5896c7..4ca02a9c3c 100644 --- a/cps-ri/src/main/java/org/onap/cps/ri/repository/ModuleReferenceRepositoryImpl.java +++ b/cps-ri/src/main/java/org/onap/cps/ri/repository/ModuleReferenceRepositoryImpl.java @@ -20,6 +20,7 @@ package org.onap.cps.ri.repository; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import jakarta.persistence.EntityManager; import jakarta.persistence.PersistenceContext; import jakarta.persistence.Query; @@ -118,6 +119,7 @@ public class ModuleReferenceRepositoryImpl implements ModuleReferenceQuery { query.setParameter(4, dataspaceName); } + @SuppressFBWarnings(value = "VA_FORMAT_STRING_USES_NEWLINE", justification = "no \n in string just in file format") private String buildModuleReferencesSqlQuery(final String parentFragmentClause, final String childFragmentClause) { return """ WITH Fragment AS ( diff --git a/cps-service/pom.xml b/cps-service/pom.xml index 6e53c2d971..68cd206c96 100644 --- a/cps-service/pom.xml +++ b/cps-service/pom.xml @@ -64,6 +64,11 @@ <groupId>com.github.ben-manes.caffeine</groupId> <artifactId>caffeine</artifactId> </dependency> + <!-- Disable SpotBug Rules --> + <dependency> + <groupId>com.github.spotbugs</groupId> + <artifactId>spotbugs-annotations</artifactId> + </dependency> <dependency> <!-- For parsing JSON object --> <groupId>com.google.code.gson</groupId> @@ -139,6 +144,7 @@ <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> </dependency> + <!-- T E S T D E P E N D E N C I E S --> <dependency> <groupId>org.codehaus.groovy</groupId> diff --git a/cps-service/src/main/java/org/onap/cps/utils/YangParserHelper.java b/cps-service/src/main/java/org/onap/cps/utils/YangParserHelper.java index a5865be657..d95aceaf79 100644 --- a/cps-service/src/main/java/org/onap/cps/utils/YangParserHelper.java +++ b/cps-service/src/main/java/org/onap/cps/utils/YangParserHelper.java @@ -22,6 +22,7 @@ package org.onap.cps.utils; import com.google.gson.JsonSyntaxException; import com.google.gson.stream.JsonReader; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.IOException; import java.io.StringReader; import java.net.URISyntaxException; @@ -128,6 +129,7 @@ public class YangParserHelper { return dataContainerNodeBuilder.build(); } + @SuppressFBWarnings(value = "DCN_NULLPOINTER_EXCEPTION", justification = "Problem originates in 3PP code") private ContainerNode parseXmlData(final String xmlData, final SchemaContext schemaContext, final String parentNodeXpath) { diff --git a/docker-compose/config/nginx/nginx.conf b/docker-compose/config/nginx/nginx.conf index 61fed515c3..7d6b997f77 100644 --- a/docker-compose/config/nginx/nginx.conf +++ b/docker-compose/config/nginx/nginx.conf @@ -22,6 +22,7 @@ http { upstream cps-and-ncmp { least_conn; server docker-compose-cps-and-ncmp-1:8080; + ### DEBUG: Disable next line for easier debugging on 1 instance (see also docker-compose.yml) server docker-compose-cps-and-ncmp-2:8080; } diff --git a/docker-compose/docker-compose.yml b/docker-compose/docker-compose.yml index 1e47d47382..fd1df38147 100644 --- a/docker-compose/docker-compose.yml +++ b/docker-compose/docker-compose.yml @@ -23,6 +23,7 @@ services: ### docker-compose --profile dmi-stub --profile tracing up -d -> run CPS with stubbed dmi-plugin (for open telemetry tracing testing make ONAP_TRACING_ENABLED "true" later "http://localhost:16686" can be accessed from browser) ### docker-compose --profile dmi-stub --profile policy-executor-stub up -d -> run CPS with stubbed dmi-plugin and policy executor stub (for policy executor service testing make POLICY_SERVICE_ENABLED "true") ### to disable notifications make notification.enabled to false & comment out kafka/zookeeper services ### + ### DEBUG: Look for '### DEBUG' comments to enable CPS-NCMP debugging dbpostgresql: container_name: dbpostgresql @@ -60,10 +61,14 @@ services: ONAP_OTEL_SAMPLER_JAEGER_REMOTE_ENDPOINT: http://jaeger-service:14250 ONAP_OTEL_EXPORTER_ENDPOINT: http://jaeger-service:4317 POLICY_SERVICE_ENABLED: 'false' + POLICY_SERVICE_DEFAULT_DECISION: 'deny from env' + ### DEBUG: Uncomment next line to enable java debugging + ### DEBUG: JAVA_TOOL_OPTIONS: -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 restart: unless-stopped depends_on: - dbpostgresql deploy: + ### DEBUG: For easier debugging use just 1 instance (also update docker-compose/config/nginx/nginx.conf !) replicas: 2 resources: reservations: @@ -72,6 +77,9 @@ services: limits: cpus: '3' memory: 3G + ### DEBUG: Uncomment next 2 lines to enable java debugging (ensure 'ports' aligns with 'deploy') + ### DEBUG ports: + ### DEBUG - ${CPS_CORE_DEBUG_PORT:-5005}:5005 nginx: container_name: nginx-loadbalancer diff --git a/integration-test/src/test/groovy/org/onap/cps/integration/base/PolicyDispatcher.groovy b/integration-test/src/test/groovy/org/onap/cps/integration/base/PolicyDispatcher.groovy index c93a5274e6..b08d1c1548 100644 --- a/integration-test/src/test/groovy/org/onap/cps/integration/base/PolicyDispatcher.groovy +++ b/integration-test/src/test/groovy/org/onap/cps/integration/base/PolicyDispatcher.groovy @@ -20,15 +20,14 @@ package org.onap.cps.integration.base - import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.RecordedRequest -import org.springframework.beans.factory.annotation.Value import org.springframework.http.HttpHeaders import org.springframework.http.HttpStatus import org.springframework.http.MediaType import org.testcontainers.shaded.com.fasterxml.jackson.databind.ObjectMapper + import java.util.concurrent.TimeUnit /** diff --git a/integration-test/src/test/groovy/org/onap/cps/integration/functional/ncmp/PolicyExecutorIntegrationSpec.groovy b/integration-test/src/test/groovy/org/onap/cps/integration/functional/ncmp/PolicyExecutorIntegrationSpec.groovy index 1d4d19bee0..56d4bfaee4 100644 --- a/integration-test/src/test/groovy/org/onap/cps/integration/functional/ncmp/PolicyExecutorIntegrationSpec.groovy +++ b/integration-test/src/test/groovy/org/onap/cps/integration/functional/ncmp/PolicyExecutorIntegrationSpec.groovy @@ -68,7 +68,7 @@ class PolicyExecutorIntegrationSpec extends CpsIntegrationSpecBase { 'accepted cm handle' | 'ch-1' | 'mock expects "ABC"' || 201 || 'allow' 'un-accepted cm handle' | 'ch-2' | 'mock expects "ABC"' || 409 || 'deny from mock server (dispatcher)' 'timeout' | 'ch-3' | 'mock expects "ABC"' || 409 || 'test default decision' - 'invalid authorization' | 'ch-1' | 'something else' || 500 || '401 Unauthorized from POST http://localhost:8790/policy-executor/api/v1/execute' + 'invalid authorization' | 'ch-1' | 'something else' || 409 || 'test default decision' } } diff --git a/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubController.java b/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubController.java index cdd26c96e9..88073c0a0f 100644 --- a/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubController.java +++ b/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubController.java @@ -41,9 +41,11 @@ import org.springframework.web.bind.annotation.RestController; @Slf4j public class PolicyExecutorStubController implements PolicyExecutorApi { + private final Sleeper sleeper; private final ObjectMapper objectMapper; private static final Pattern ERROR_CODE_PATTERN = Pattern.compile("(\\d{3})"); private int decisionCounter = 0; + private static int slowResponseTimeInSeconds = 40; @Override public ResponseEntity<PolicyExecutionResponse> executePolicyAction( @@ -85,7 +87,14 @@ public class PolicyExecutorStubController implements PolicyExecutorApi { final String decisionId = String.valueOf(++decisionCounter); final String decision; final String message; - + if (targetIdentifier.toLowerCase(Locale.getDefault()).contains("slow")) { + try { + sleeper.haveALittleRest(slowResponseTimeInSeconds); + } catch (final InterruptedException e) { + log.trace("Sleep interrupted, re-interrupting the thread"); + Thread.currentThread().interrupt(); // Re-interrupt the thread + } + } if (targetIdentifier.toLowerCase(Locale.getDefault()).contains("cps-is-great")) { decision = "allow"; message = "All good"; diff --git a/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/Sleeper.java b/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/Sleeper.java new file mode 100644 index 0000000000..8f904cc5f2 --- /dev/null +++ b/policy-executor-stub/src/main/java/org/onap/cps/policyexecutor/stub/controller/Sleeper.java @@ -0,0 +1,35 @@ +/* + * ============LICENSE_START======================================================= + * Copyright (C) 2024 Nordix Foundation + * ================================================================================ + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * ============LICENSE_END========================================================= + */ + +package org.onap.cps.policyexecutor.stub.controller; + +import java.util.concurrent.TimeUnit; +import org.springframework.stereotype.Service; + +/** + * This class is a successfull experiment to extract out sleep functionality so the interrupted exception handling can + * be covered with a test (e.g. using spy ion Sleeper) and help to get too 100% code coverage. + */ +@Service +public class Sleeper { + public void haveALittleRest(final int timeInSeconds) throws InterruptedException { + TimeUnit.SECONDS.sleep(timeInSeconds); + } +} diff --git a/policy-executor-stub/src/test/groovy/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubControllerSpec.groovy b/policy-executor-stub/src/test/groovy/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubControllerSpec.groovy index 064e0234a3..44460daa7e 100644 --- a/policy-executor-stub/src/test/groovy/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubControllerSpec.groovy +++ b/policy-executor-stub/src/test/groovy/org/onap/cps/policyexecutor/stub/controller/PolicyExecutorStubControllerSpec.groovy @@ -25,6 +25,7 @@ import org.onap.cps.policyexecutor.stub.model.NcmpDelete import org.onap.cps.policyexecutor.stub.model.PolicyExecutionRequest import org.onap.cps.policyexecutor.stub.model.PolicyExecutionResponse import org.onap.cps.policyexecutor.stub.model.Request +import org.spockframework.spring.SpringBean import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest import org.springframework.http.HttpStatus @@ -43,8 +44,15 @@ class PolicyExecutorStubControllerSpec extends Specification { @Autowired ObjectMapper objectMapper + @SpringBean + Sleeper sleeper = Spy() + def url = '/policy-executor/api/v1/some-action' + def setup() { + PolicyExecutorStubController.slowResponseTimeInSeconds = 1 + } + def 'Execute policy action.'() { given: 'a policy execution request with target: #targetIdentifier' def requestBody = createRequestBody(targetIdentifier) @@ -66,6 +74,7 @@ class PolicyExecutorStubControllerSpec extends Specification { targetIdentifier || expectedDecsisonId | expectedDecision | expectedMessage 'some fdn' || '1' | 'deny' | "Only FDNs containing 'cps-is-great' are allowed" 'fdn with cps-is-great' || '2' | 'allow' | 'All good' + 'slow' || '3' | 'deny' | "Only FDNs containing 'cps-is-great' are allowed" } def 'Execute policy action with a HTTP error code.'() { @@ -118,6 +127,19 @@ class PolicyExecutorStubControllerSpec extends Specification { assert response.status == HttpStatus.BAD_REQUEST.value() } + def 'Execute policy action with interrupted exception during slow response.'() { + given: 'a policy execution request with target: "slow"' + def requestBody = createRequestBody('slow') + sleeper.haveALittleRest(_) >> { throw new InterruptedException() } + when: 'request is posted' + mockMvc.perform(post(url) + .header('Authorization','some string') + .contentType(MediaType.APPLICATION_JSON) + .content(requestBody)) + then: 'response status is Bad Request' + noExceptionThrown() + } + def 'Execute policy action with missing or invalid attributes.'() { given: 'a policy execution request with decisionType=#decisionType, schema=#schema, targetIdentifier=#targetIdentifier' def requestBody = createRequestBody(decisionType, schema, targetIdentifier) |