diff --git a/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionHandler.java b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionHandler.java index 813bb9b0..dc62b6d2 100644 --- a/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionHandler.java +++ b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionHandler.java @@ -1,7 +1,8 @@ package fr.insee.onyxia.api.controller; +import jakarta.servlet.http.HttpServletRequest; +import java.net.URI; import java.util.List; -import java.util.Map; import java.util.stream.Collectors; import org.everit.json.schema.ValidationException; import org.springframework.http.HttpStatus; @@ -9,28 +10,55 @@ import org.springframework.security.access.AccessDeniedException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; -import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; @RestControllerAdvice -public class RestExceptionHandler extends ResponseEntityExceptionHandler { +public class RestExceptionHandler { - @ExceptionHandler(AccessDeniedException.class) - public ProblemDetail handleAccessDeniedException(Exception ignored) { - ProblemDetail problemDetail = ProblemDetail.forStatus(HttpStatus.FORBIDDEN); - problemDetail.setTitle("Access denied"); + // Helper method to create ProblemDetail + private ProblemDetail createProblemDetail( + HttpStatus status, URI type, String title, String detail, String instancePath) { + ProblemDetail problemDetail = ProblemDetail.forStatus(status); + problemDetail.setType(type); + problemDetail.setTitle(title); + problemDetail.setDetail(detail); + problemDetail.setInstance(URI.create(instancePath)); return problemDetail; } + // AccessDeniedException handler + @ExceptionHandler(AccessDeniedException.class) + public ProblemDetail handleAccessDeniedException( + AccessDeniedException exception, HttpServletRequest request) { + return createProblemDetail( + HttpStatus.FORBIDDEN, + RestExceptionTypes.ACCESS_DENIED, + "Access Denied", + "You do not have permission to access this resource.", + request.getRequestURI()); + } + + // ValidationException handler @ExceptionHandler(ValidationException.class) - public ProblemDetail handleValidationException(ValidationException ex) { + public ProblemDetail handleValidationException( + ValidationException ex, HttpServletRequest request) { List errors = - ex.getCausingExceptions().stream() - .map(ValidationException::getMessage) - .collect(Collectors.toList()); + ex.getCausingExceptions() != null && !ex.getCausingExceptions().isEmpty() + ? ex.getCausingExceptions().stream() + .map(ValidationException::getMessage) + .collect(Collectors.toList()) + : List.of(ex.getMessage()); // Fallback to the main exception message if + // causing exceptions are empty. + + ProblemDetail problemDetail = + createProblemDetail( + HttpStatus.BAD_REQUEST, + RestExceptionTypes.VALIDATION_FAILED, + "Validation Failed", + "The request contains invalid data.", + request.getRequestURI()); - ProblemDetail problemDetail = ProblemDetail.forStatus(HttpStatus.BAD_REQUEST); - problemDetail.setProperties(Map.of("errors", errors)); - problemDetail.setTitle("Validation failed"); + // Add 'errors' as a custom property + problemDetail.setProperty("errors", errors); return problemDetail; } } diff --git a/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionTypes.java b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionTypes.java new file mode 100644 index 00000000..341956f3 --- /dev/null +++ b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/RestExceptionTypes.java @@ -0,0 +1,25 @@ +package fr.insee.onyxia.api.controller; + +import java.net.URI; + +// These are mostly examples +public class RestExceptionTypes { + public static final URI ACCESS_DENIED = URI.create("urn:org:onyxia:api:error:access-denied"); + public static final URI VALIDATION_FAILED = + URI.create("urn:org:onyxia:api:error:validation-failed"); + public static final URI INVALID_ARGUMENT = + URI.create("urn:org:onyxia:api:error:invalid-argument"); + public static final URI INSTALLATION_FAILURE = + URI.create("urn:org:onyxia:api:error:installation-failure"); + public static final URI NAMESPACE_NOT_FOUND = + URI.create("urn:org:onyxia:api:error:namespace-not-found"); + public static final URI HELM_LIST_FAILURE = + URI.create("urn:org:onyxia:api:error:helm-list-failure"); + public static final URI HELM_RELEASE_FETCH_FAILURE = + URI.create("urn:org:onyxia:api:error:helm-release-fetch-failure"); + public static final URI GENERIC_ERROR = URI.create("urn:org:onyxia:api:error:unknown-error"); + + private RestExceptionTypes() { + // Prevent instantiation + } +} diff --git a/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/exception/CustomKubernetesException.java b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/exception/CustomKubernetesException.java new file mode 100644 index 00000000..9e2914e1 --- /dev/null +++ b/onyxia-api/src/main/java/fr/insee/onyxia/api/controller/exception/CustomKubernetesException.java @@ -0,0 +1,31 @@ +package fr.insee.onyxia.api.controller.exception; + +import org.springframework.http.ProblemDetail; + +/** + * Custom exception class for propagating Kubernetes-related errors with structured ProblemDetail + * information. + */ +public class CustomKubernetesException extends RuntimeException { + + private final ProblemDetail problemDetail; + + /** + * Constructor to create a CustomKubernetesException. + * + * @param problemDetail The ProblemDetail containing error details + */ + public CustomKubernetesException(ProblemDetail problemDetail) { + super(problemDetail.getDetail()); + this.problemDetail = problemDetail; + } + + /** + * Getter for the ProblemDetail object. + * + * @return ProblemDetail containing structured error details + */ + public ProblemDetail getProblemDetail() { + return problemDetail; + } +} diff --git a/onyxia-api/src/main/java/fr/insee/onyxia/api/services/impl/HelmAppsService.java b/onyxia-api/src/main/java/fr/insee/onyxia/api/services/impl/HelmAppsService.java index 48768b64..cf7e1c3d 100644 --- a/onyxia-api/src/main/java/fr/insee/onyxia/api/services/impl/HelmAppsService.java +++ b/onyxia-api/src/main/java/fr/insee/onyxia/api/services/impl/HelmAppsService.java @@ -7,7 +7,9 @@ import com.fasterxml.jackson.databind.ObjectMapper; import fr.insee.onyxia.api.configuration.kubernetes.HelmClientProvider; import fr.insee.onyxia.api.configuration.kubernetes.KubernetesClientProvider; -import fr.insee.onyxia.api.controller.exception.NamespaceNotFoundException; +import fr.insee.onyxia.api.controller.RestExceptionTypes; +import fr.insee.onyxia.api.controller.exception.*; +import fr.insee.onyxia.api.controller.exception.CustomKubernetesException; import fr.insee.onyxia.api.events.InstallServiceEvent; import fr.insee.onyxia.api.events.OnyxiaEventPublisher; import fr.insee.onyxia.api.events.SuspendResumeServiceEvent; @@ -34,6 +36,7 @@ import io.github.inseefrlab.helmwrapper.service.HelmInstallService.MultipleServiceFound; import java.io.File; import java.io.IOException; +import java.net.URI; import java.text.ParseException; import java.util.*; import java.util.concurrent.CompletableFuture; @@ -44,7 +47,8 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; -import org.springframework.security.access.AccessDeniedException; +import org.springframework.http.HttpStatus; +import org.springframework.http.ProblemDetail; @org.springframework.stereotype.Service @Qualifier("Helm") @@ -93,6 +97,16 @@ private HelmInstallService getHelmInstallService() { return helmClientProvider.defaultHelmInstallService(); } + private ProblemDetail createProblemDetail( + HttpStatus status, URI type, String title, String detail, String instance) { + ProblemDetail problemDetail = ProblemDetail.forStatus(status); + problemDetail.setType(type); + problemDetail.setTitle(title); + problemDetail.setDetail(detail); + problemDetail.setInstance(URI.create(instance)); + return problemDetail; + } + @Override public Collection installApp( Region region, @@ -146,7 +160,30 @@ public Collection installApp( region, namespaceId, requestDTO.getName(), metadata); return List.of(res.getManifest()); } catch (IllegalArgumentException e) { - throw new AccessDeniedException(e.getMessage()); + String instanceUri = + String.format( + "/install-app/%s/%s/%s", namespaceId, catalogId, requestDTO.getName()); + throw new CustomKubernetesException( + createProblemDetail( + HttpStatus.BAD_REQUEST, + RestExceptionTypes.INVALID_ARGUMENT, + "Invalid Argument", + e.getMessage(), + instanceUri)); + } catch (Exception e) { + LOGGER.error("Unexpected error during app installation", e); + + String instanceUri = + String.format( + "/install-app/%s/%s/%s", namespaceId, catalogId, requestDTO.getName()); + + throw new CustomKubernetesException( + createProblemDetail( + HttpStatus.INTERNAL_SERVER_ERROR, + RestExceptionTypes.INSTALLATION_FAILURE, + "Installation Failure", + "An unexpected error occurred while installing the app. Please try again later.", + instanceUri)); } finally { if (!values.delete()) { LOGGER.warn("Failed to delete values file, path {}", values.getAbsolutePath()); @@ -169,9 +206,16 @@ public CompletableFuture getUserServices( return CompletableFuture.completedFuture(new ServicesListing()); } if (StringUtils.isEmpty(project.getNamespace())) { - throw new NamespaceNotFoundException(); + String instanceUri = "/projects/" + project.getId() + "/namespace"; + throw new CustomKubernetesException( + createProblemDetail( + HttpStatus.NOT_FOUND, + RestExceptionTypes.NAMESPACE_NOT_FOUND, + "Namespace Not Found", + "The namespace for the provided project is empty or not defined.", + instanceUri)); } - List installedCharts = null; + List installedCharts; try { installedCharts = Arrays.asList( @@ -180,7 +224,18 @@ public CompletableFuture getUserServices( getHelmConfiguration(region, user), project.getNamespace())); } catch (Exception e) { - return CompletableFuture.completedFuture(new ServicesListing()); + LOGGER.error( + "Failed to list installed Helm charts for namespace {}", + project.getNamespace(), + e); + String instanceUri = "/namespaces/" + project.getNamespace() + "/helm-list"; + throw new CustomKubernetesException( + createProblemDetail( + HttpStatus.INTERNAL_SERVER_ERROR, + RestExceptionTypes.HELM_LIST_FAILURE, + "Helm List Failure", + "Failed to retrieve the list of installed Helm charts. Please try again later.", + instanceUri)); } List services = installedCharts.parallelStream() @@ -284,18 +339,38 @@ public UninstallService destroyService( } private Service getHelmApp(Region region, User user, HelmLs release) { - HelmReleaseInfo helmReleaseInfo = - getHelmInstallService() - .getAll( - getHelmConfiguration(region, user), - release.getName(), - release.getNamespace()); + HelmReleaseInfo helmReleaseInfo; + try { + helmReleaseInfo = + getHelmInstallService() + .getAll( + getHelmConfiguration(region, user), + release.getName(), + release.getNamespace()); + } catch (Exception e) { + LOGGER.error( + "Failed to retrieve Helm release info for release {} in namespace {}", + release.getName(), + release.getNamespace(), + e); + String instanceUri = "/releases/" + release.getName(); + throw new CustomKubernetesException( + createProblemDetail( + HttpStatus.INTERNAL_SERVER_ERROR, + RestExceptionTypes.HELM_RELEASE_FETCH_FAILURE, + "Helm Release Fetch Failure", + "Failed to retrieve Helm release information.", + instanceUri)); + } + Service service = getServiceFromRelease(region, release, helmReleaseInfo.getManifest(), user); + try { service.setStartedAt(helmDateFormat.parse(release.getUpdated()).getTime()); } catch (Exception e) { - service.setStartedAt(0); + LOGGER.warn("Failed to parse release updated date for {}", release.getName(), e); + service.setStartedAt(0); // Fallback to 0 if parsing fails } try { KubernetesClient client = kubernetesClientProvider.getUserClient(region, user); @@ -321,18 +396,22 @@ private Service getHelmApp(Region region, User user, HelmLs release) { } } } catch (Exception e) { - LOGGER.warn("Exception occurred", e); + LOGGER.warn( + "Failed to retrieve or decode Onyxia secret for release {}", + release.getName(), + e); } + service.setId(release.getName()); service.setName(release.getName()); service.setSubtitle(release.getChart()); - service.setName(release.getName()); service.setNamespace(release.getNamespace()); service.setRevision(release.getRevision()); service.setStatus(release.getStatus()); service.setUpdated(release.getUpdated()); service.setChart(release.getChart()); service.setAppVersion(release.getAppVersion()); + try { String values = helmReleaseInfo.getUserSuppliedValues(); JsonNode node = mapperHelm.readTree(values); @@ -346,14 +425,20 @@ private Service getHelmApp(Region region, User user, HelmLs release) { service.setSuspended(Boolean.parseBoolean(service.getEnv().get(SUSPEND_KEY))); } } catch (Exception e) { - LOGGER.warn("Exception occurred", e); + LOGGER.warn( + "Failed to parse user-supplied values for release {}", release.getName(), e); } + try { String notes = helmReleaseInfo.getNotes(); service.setPostInstallInstructions(notes); } catch (Exception e) { - LOGGER.warn("Exception occurred", e); + LOGGER.warn( + "Failed to retrieve post-install instructions for release {}", + release.getName(), + e); } + return service; } @@ -531,9 +616,11 @@ private Service getServiceFromRelease( service.setUrls(urls); } catch (Exception e) { LOGGER.warn( - "Failed to retrieve URLS for release {} namespace {}", + "Failed to retrieve URLs for release {} in namespace {}. Region: {}, User: {}", release.getName(), release.getNamespace(), + region.getName(), + user.getIdep(), e); service.setUrls(List.of()); } @@ -544,12 +631,15 @@ private Service getServiceFromRelease( service.setControllers(controllers); } catch (Exception e) { LOGGER.warn( - "Failed to retrieve controllers for release {} namespace {}", + "Failed to retrieve controllers for release {} in namespace {}. Region: {}, User: {}", release.getName(), release.getNamespace(), + region.getName(), + user.getIdep(), e); service.setControllers(List.of()); } + service.setInstances(1); service.setTasks( diff --git a/onyxia-api/src/test/java/fr/insee/onyxia/api/controller/RestExceptionHandlerTest.java b/onyxia-api/src/test/java/fr/insee/onyxia/api/controller/RestExceptionHandlerTest.java new file mode 100644 index 00000000..d2b0029d --- /dev/null +++ b/onyxia-api/src/test/java/fr/insee/onyxia/api/controller/RestExceptionHandlerTest.java @@ -0,0 +1,94 @@ +package fr.insee.onyxia.api.controller; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.when; + +import jakarta.servlet.http.HttpServletRequest; +import java.net.URI; +import java.util.List; +import org.everit.json.schema.Schema; +import org.everit.json.schema.ValidationException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.http.HttpStatus; +import org.springframework.http.ProblemDetail; +import org.springframework.security.access.AccessDeniedException; + +class RestExceptionHandlerTest { + + private RestExceptionHandler restExceptionHandler; + private HttpServletRequest mockRequest; + + @BeforeEach + void setUp() { + restExceptionHandler = new RestExceptionHandler(); + mockRequest = Mockito.mock(HttpServletRequest.class); + } + + @Test + void testHandleAccessDeniedForNamespaceCreation() { + when(mockRequest.getRequestURI()).thenReturn("/onboarding"); + + AccessDeniedException exception = + new AccessDeniedException("Access Denied for Namespace Creation"); + ProblemDetail result = + restExceptionHandler.handleAccessDeniedException(exception, mockRequest); + + assertEquals(HttpStatus.FORBIDDEN.value(), result.getStatus()); + assertEquals(RestExceptionTypes.ACCESS_DENIED, result.getType()); + assertEquals("Access Denied", result.getTitle()); + assertEquals("You do not have permission to access this resource.", result.getDetail()); + assertEquals(URI.create("/onboarding"), result.getInstance()); + } + + @Test + void testHandleValidationException() { + when(mockRequest.getRequestURI()).thenReturn("/my-lab/app"); + + ValidationException validationException = Mockito.mock(ValidationException.class); + when(validationException.getMessage()).thenReturn("Validation Error Message"); + + ProblemDetail result = + restExceptionHandler.handleValidationException(validationException, mockRequest); + + assertEquals(HttpStatus.BAD_REQUEST.value(), result.getStatus()); + assertEquals(RestExceptionTypes.VALIDATION_FAILED, result.getType()); + assertEquals("Validation Failed", result.getTitle()); + assertEquals("The request contains invalid data.", result.getDetail()); + assertEquals(URI.create("/my-lab/app"), result.getInstance()); + assertThat(result.getProperties()).containsKey("errors"); + } + + @Test + void testHandleValidationExceptionWithErrors() { + when(mockRequest.getRequestURI()).thenReturn("/my-lab/app"); + + Schema mockSchema = Mockito.mock(Schema.class); + + ValidationException ex1 = + new ValidationException(mockSchema, "Field 'name' is required", "name"); + ValidationException ex2 = + new ValidationException(mockSchema, "Field 'version' must be a number", "version"); + + ValidationException validationException = Mockito.mock(ValidationException.class); + when(validationException.getMessage()).thenReturn("Validation Error Message"); + when(validationException.getCausingExceptions()).thenReturn(List.of(ex1, ex2)); + + ProblemDetail result = + restExceptionHandler.handleValidationException(validationException, mockRequest); + + assertEquals(HttpStatus.BAD_REQUEST.value(), result.getStatus()); + assertEquals(RestExceptionTypes.VALIDATION_FAILED, result.getType()); + assertEquals("Validation Failed", result.getTitle()); + assertEquals("The request contains invalid data.", result.getDetail()); + assertEquals(URI.create("/my-lab/app"), result.getInstance()); + + @SuppressWarnings("unchecked") + List errors = (List) result.getProperties().get("errors"); + assertThat(errors) + .containsExactly( + "#: Field 'name' is required", "#: Field 'version' must be a number"); + } +}