From: sourabh_sourabh Date: Fri, 20 Jun 2025 14:31:27 +0000 (+0100) Subject: Refactor: Improve PolicyExecutor WebClient error logging X-Git-Url: https://gerrit.onap.org/r/gitweb?a=commitdiff_plain;h=refs%2Fchanges%2F62%2F141362%2F5;p=cps.git Refactor: Improve PolicyExecutor WebClient error logging - Improved logging and ensured consistency with processFallbackResponse(). Issue-ID: CPS-2859 Change-Id: Ia74226e0aab79bc0542eff6869ccbcbd3995276d Signed-off-by: sourabh_sourabh --- 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 38105329d1..6e69a5c473 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 @@ -46,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.WebClientRequestException; import org.springframework.web.reactive.function.client.WebClientResponseException; @Slf4j @@ -155,6 +156,10 @@ public class PolicyExecutor { final Object bodyAsObject = createBodyAsObject(operationAsMap); + log.debug("Sending permission check to Policy Executor for CMHandle: {} with operation: {}", + yangModelCmHandle.getId(), operationType); + log.trace("Policy Executor request body: {}", bodyAsObject); + final UrlTemplateParameters urlTemplateParameters = RestServiceUrlTemplateBuilder.newInstance() .fixedPathSegment(REQUEST_PATH) .createUrlTemplateParameters(String.format("%s:%s", serverAddress, serverPort), PERMISSION_BASE_PATH); @@ -191,26 +196,41 @@ public class PolicyExecutor { } } + private String createErrorDetailsMessage(final Throwable throwable) { + if (throwable instanceof WebClientResponseException webClientResponseException) { + return "Policy Executor returned HTTP Status code " + + webClientResponseException.getStatusCode().value() + "."; + } + if (throwable instanceof WebClientRequestException) { + return "Network or I/O error while attempting to contact Policy Executor."; + } + if (throwable instanceof TimeoutException || throwable.getCause() instanceof TimeoutException) { + return "Policy Executor request timed out."; + } + if (throwable.getCause() instanceof UnknownHostException) { + return String.format("Cannot connect to Policy Executor (%s:%s).", serverAddress, serverPort); + } + return "Unexpected error during Policy Executor call."; + } + private void processException(final RuntimeException runtimeException) { - if (runtimeException instanceof WebClientResponseException) { - final WebClientResponseException webClientResponseException = (WebClientResponseException) runtimeException; - log.warn("HTTP Error Message: {}", webClientResponseException.getMessage()); - 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; - } + final String errorDetailsMessage = createErrorDetailsMessage(runtimeException); + + log.warn("Exception during Policy Execution check. Class: {}, Message: {}, Details: {}", + runtimeException.getClass().getSimpleName(), runtimeException.getMessage(), errorDetailsMessage); + + if (runtimeException instanceof WebClientResponseException + || runtimeException instanceof WebClientRequestException) { + processFallbackResponse(errorDetailsMessage, runtimeException); + return; + } + final Throwable nestedThrowable = runtimeException.getCause(); + if (nestedThrowable instanceof TimeoutException || nestedThrowable instanceof UnknownHostException) { + final String nestedErrorDetailsMessage = createErrorDetailsMessage(nestedThrowable); + processFallbackResponse(nestedErrorDetailsMessage, nestedThrowable); + return; } + throw runtimeException; } private void processFallbackResponse(final String message, final Throwable cause) { @@ -220,5 +240,4 @@ public class PolicyExecutor { log.warn(warning); processDecision(decisionId, decision, warning, cause); } - } 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 9423246134..96330ab8ec 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 @@ -1,6 +1,6 @@ /* * ============LICENSE_START======================================================= - * Copyright (C) 2024 Nordix Foundation + * Copyright (C) 2024-2025 OpenInfra Foundation Europe. All rights reserved. * ================================================================================ * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -33,10 +33,10 @@ 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.WebClientRequestException import org.springframework.web.reactive.function.client.WebClientResponseException import reactor.core.publisher.Mono import spock.lang.Specification - import java.util.concurrent.TimeoutException import static org.onap.cps.ncmp.api.data.models.OperationType.CREATE @@ -79,7 +79,7 @@ class PolicyExecutorSpec extends Specification { when: 'permission is checked for an operation' objectUnderTest.checkPermission(new YangModelCmHandle(), operationType, 'my credentials','my resource',someValidJson) then: 'system logs the operation is allowed' - assert getLogEntry(2) == 'Operation allowed.' + assert getLogEntry(4) == 'Operation allowed.' and: 'no exception occurs' noExceptionThrown() where: 'all write operations are tested' @@ -129,7 +129,7 @@ class PolicyExecutorSpec extends Specification { 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) == 'No valid response body from Policy Executor, ignored' + assert getLogEntry(3) == 'No valid response body from Policy Executor, ignored' } def 'Permission check with timeout exception.'() { @@ -159,7 +159,7 @@ class PolicyExecutorSpec extends Specification { 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' + assert thrownException.details == 'Unexpected error during Policy Executor call. Falling back to configured default decision: deny by default' and: 'the cause is the original unknown host exception' assert thrownException.cause == unknownHostException } @@ -208,6 +208,21 @@ class PolicyExecutorSpec extends Specification { assert getLogEntry(0) == 'Policy Executor Enabled: false' } + def 'Permission check with web client request exception.'() { + given: 'a WebClientRequestException is thrown during the Policy Executor call' + def webClientRequestException = Mock(WebClientRequestException) + webClientRequestException.getMessage() >> "some error message" + mockResponseSpec.toEntity(*_) >> { throw webClientRequestException } + objectUnderTest.defaultDecision = 'deny' + when: 'permission is checked for a write operation' + objectUnderTest.checkPermission(new YangModelCmHandle(), CREATE, 'my credentials', 'my resource', someValidJson) + then: 'a PolicyExecutorException is thrown with the expected fallback message' + def thrownException = thrown(PolicyExecutorException) + thrownException.message == 'Operation not allowed. Decision id N/A : deny' + thrownException.details == 'Network or I/O error while attempting to contact Policy Executor. Falling back to configured default decision: deny' + thrownException.cause == webClientRequestException + } + def mockResponse(mockResponseAsMap, httpStatus) { JsonNode jsonNode = spiedObjectMapper.readTree(spiedObjectMapper.writeValueAsString(mockResponseAsMap)) def mono = Mono.just(new ResponseEntity<>(jsonNode, httpStatus))