Skip to content

Commit

Permalink
Handle more cloud errors (#864)
Browse files Browse the repository at this point in the history 
* Forward cloud exception status codes

* Update mappings

* Add 504 => 500 test

* 3xx => 422, 408 => 408, 504 => 504, 5xx => 502, cleanup tests
  • Loading branch information
@andrew4699
andrew4699 authored Jan 28, 2025
1 parent 22201d0 commit 5c38780
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 44 deletions.
1 change: 0 additions & 1 deletion build-logic/src/main/kotlin/polaris-quarkus.gradle.kts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/

import gradle.kotlin.dsl.accessors._fa00c0b20184971a79f32516372275b9.testing
import org.gradle.api.attributes.TestSuiteType
import org.gradle.api.plugins.jvm.JvmTestSuite
import org.gradle.kotlin.dsl.register
Expand Down
62 changes: 53 additions & 9 deletions .../service/src/test/java/org/apache/polaris/service/quarkus/IcebergExceptionMapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.polaris.service.dropwizard;
package org.apache.polaris.service.quarkus;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import com.azure.core.exception.AzureException;
import com.azure.core.exception.HttpResponseException;
import com.azure.core.http.HttpResponse;
import com.google.cloud.storage.StorageException;
import jakarta.ws.rs.core.Response;
import java.util.Map;
import java.util.stream.Stream;
import org.apache.polaris.service.exception.IcebergExceptionMapper;
import org.junit.jupiter.params.ParameterizedTest;
Expand All @@ -33,14 +38,40 @@
class IcebergExceptionMapperTest {

static Stream<Arguments> fileIOExceptionMapping() {
return Stream.of(
Arguments.of(new AzureException("Unknown"), 500),
Arguments.of(new AzureException("Forbidden"), 403),
Arguments.of(new AzureException("FORBIDDEN"), 403),
Arguments.of(new AzureException("Not Authorized"), 403),
Arguments.of(new AzureException("Access Denied"), 403),
Arguments.of(S3Exception.builder().message("Access denied").build(), 403),
Arguments.of(new StorageException(1, "access denied"), 403));
Map<Integer, Integer> cloudCodeMappings =
Map.of(
// Map of HTTP code returned from a cloud provider to the HTTP code Polaris is expected
// to return
302, 422,
400, 400,
401, 403,
403, 403,
404, 400,
408, 408,
429, 429,
503, 502,
504, 504);

return Stream.concat(
Stream.of(
Arguments.of(new AzureException("Unknown"), 500),
Arguments.of(new AzureException("Forbidden"), 403),
Arguments.of(new AzureException("FORBIDDEN"), 403),
Arguments.of(new AzureException("Not Authorized"), 403),
Arguments.of(new AzureException("Access Denied"), 403),
Arguments.of(S3Exception.builder().message("Access denied").build(), 403),
Arguments.of(new StorageException(1, "access denied"), 403)),
cloudCodeMappings.entrySet().stream()
.flatMap(
entry ->
Stream.of(
Arguments.of(
new HttpResponseException("", mockAzureResponse(entry.getKey()), ""),
entry.getValue()),
Arguments.of(
S3Exception.builder().message("").statusCode(entry.getKey()).build(),
entry.getValue()),
Arguments.of(new StorageException(entry.getKey(), ""), entry.getValue()))));
}

@ParameterizedTest
Expand All @@ -52,4 +83,17 @@ void fileIOExceptionMapping(RuntimeException ex, int statusCode) {
assertThat(response.getEntity()).extracting("message").isEqualTo(ex.getMessage());
}
}

/**
* Creates a mock of the Azure-specific HttpResponse object, as it's quite difficult to construct
* a "real" one.
*
* @param statusCode
* @return
*/
private static HttpResponse mockAzureResponse(int statusCode) {
HttpResponse res = mock(HttpResponse.class);
when(res.getStatusCode()).thenReturn(statusCode);
return res;
}
}
2 changes: 2 additions & 0 deletions service/common/build.gradle.kts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ dependencies {

implementation(platform(libs.azuresdk.bom))
implementation("com.azure:azure-core")
implementation("com.azure:azure-storage-blob")
implementation("com.azure:azure-storage-file-datalake")

testImplementation(platform(libs.junit.bom))
testImplementation("org.junit.jupiter:junit-jupiter")
Expand Down
117 changes: 83 additions & 34 deletions ...ice/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
package org.apache.polaris.service.exception;

import com.azure.core.exception.AzureException;
import com.azure.core.exception.HttpResponseException;
import com.google.cloud.storage.StorageException;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableSet;
import jakarta.ws.rs.WebApplicationException;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.core.Response.Status;
import jakarta.ws.rs.ext.ExceptionMapper;
import jakarta.ws.rs.ext.Provider;
import java.util.Arrays;
Expand Down Expand Up @@ -73,40 +75,7 @@ public IcebergExceptionMapper() {}
@Override
public Response toResponse(RuntimeException runtimeException) {
LOGGER.info("Handling runtimeException {}", runtimeException.getMessage());
int responseCode =
switch (runtimeException) {
case NoSuchNamespaceException e -> Response.Status.NOT_FOUND.getStatusCode();
case NoSuchIcebergTableException e -> Response.Status.NOT_FOUND.getStatusCode();
case NoSuchTableException e -> Response.Status.NOT_FOUND.getStatusCode();
case NoSuchViewException e -> Response.Status.NOT_FOUND.getStatusCode();
case NotFoundException e -> Response.Status.NOT_FOUND.getStatusCode();
case AlreadyExistsException e -> Response.Status.CONFLICT.getStatusCode();
case CommitFailedException e -> Response.Status.CONFLICT.getStatusCode();
case UnprocessableEntityException e -> 422;
case CherrypickAncestorCommitException e -> Response.Status.BAD_REQUEST.getStatusCode();
case CommitStateUnknownException e -> Response.Status.BAD_REQUEST.getStatusCode();
case DuplicateWAPCommitException e -> Response.Status.BAD_REQUEST.getStatusCode();
case ForbiddenException e -> Response.Status.FORBIDDEN.getStatusCode();
case jakarta.ws.rs.ForbiddenException e -> Response.Status.FORBIDDEN.getStatusCode();
case NotAuthorizedException e -> Response.Status.UNAUTHORIZED.getStatusCode();
case NamespaceNotEmptyException e -> Response.Status.BAD_REQUEST.getStatusCode();
case ValidationException e -> Response.Status.BAD_REQUEST.getStatusCode();
case ServiceUnavailableException e -> Response.Status.SERVICE_UNAVAILABLE.getStatusCode();
case RuntimeIOException e -> Response.Status.SERVICE_UNAVAILABLE.getStatusCode();
case ServiceFailureException e -> Response.Status.SERVICE_UNAVAILABLE.getStatusCode();
case CleanableFailure e -> Response.Status.BAD_REQUEST.getStatusCode();
case RESTException e -> Response.Status.SERVICE_UNAVAILABLE.getStatusCode();
case IllegalArgumentException e -> Response.Status.BAD_REQUEST.getStatusCode();
case UnsupportedOperationException e -> Response.Status.NOT_ACCEPTABLE.getStatusCode();
case S3Exception e when doesAnyThrowableContainAccessDeniedHint(e) ->
Response.Status.FORBIDDEN.getStatusCode();
case AzureException e when doesAnyThrowableContainAccessDeniedHint(e) ->
Response.Status.FORBIDDEN.getStatusCode();
case StorageException e when doesAnyThrowableContainAccessDeniedHint(e) ->
Response.Status.FORBIDDEN.getStatusCode();
case WebApplicationException e -> e.getResponse().getStatus();
default -> Response.Status.INTERNAL_SERVER_ERROR.getStatusCode();
};
int responseCode = mapExceptionToResponseCode(runtimeException);
if (responseCode == Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()) {
LOGGER.error("Unhandled exception returning INTERNAL_SERVER_ERROR", runtimeException);
}
Expand Down Expand Up @@ -144,4 +113,84 @@ public static boolean containsAnyAccessDeniedHint(String message) {
public static Collection<String> getAccessDeniedHints() {
return ImmutableSet.copyOf(ACCESS_DENIED_HINTS);
}

static int mapExceptionToResponseCode(RuntimeException rex) {
// Cloud exceptions
if (rex instanceof S3Exception
|| rex instanceof AzureException
|| rex instanceof StorageException) {
return mapCloudExceptionToResponseCode(rex);
}

// Non-cloud exceptions
return switch (rex) {
case NoSuchNamespaceException e -> Status.NOT_FOUND.getStatusCode();
case NoSuchIcebergTableException e -> Status.NOT_FOUND.getStatusCode();
case NoSuchTableException e -> Status.NOT_FOUND.getStatusCode();
case NoSuchViewException e -> Status.NOT_FOUND.getStatusCode();
case NotFoundException e -> Status.NOT_FOUND.getStatusCode();
case AlreadyExistsException e -> Status.CONFLICT.getStatusCode();
case CommitFailedException e -> Status.CONFLICT.getStatusCode();
case UnprocessableEntityException e -> 422;
case CherrypickAncestorCommitException e -> Status.BAD_REQUEST.getStatusCode();
case CommitStateUnknownException e -> Status.BAD_REQUEST.getStatusCode();
case DuplicateWAPCommitException e -> Status.BAD_REQUEST.getStatusCode();
case ForbiddenException e -> Status.FORBIDDEN.getStatusCode();
case jakarta.ws.rs.ForbiddenException e -> Status.FORBIDDEN.getStatusCode();
case NotAuthorizedException e -> Status.UNAUTHORIZED.getStatusCode();
case NamespaceNotEmptyException e -> Status.BAD_REQUEST.getStatusCode();
case ValidationException e -> Status.BAD_REQUEST.getStatusCode();
case ServiceUnavailableException e -> Status.SERVICE_UNAVAILABLE.getStatusCode();
case RuntimeIOException e -> Status.SERVICE_UNAVAILABLE.getStatusCode();
case ServiceFailureException e -> Status.SERVICE_UNAVAILABLE.getStatusCode();
case CleanableFailure e -> Status.BAD_REQUEST.getStatusCode();
case RESTException e -> Status.SERVICE_UNAVAILABLE.getStatusCode();
case IllegalArgumentException e -> Status.BAD_REQUEST.getStatusCode();
case UnsupportedOperationException e -> Status.NOT_ACCEPTABLE.getStatusCode();
case WebApplicationException e -> e.getResponse().getStatus();
default -> Status.INTERNAL_SERVER_ERROR.getStatusCode();
};
}

static int mapCloudExceptionToResponseCode(RuntimeException rex) {
if (doesAnyThrowableContainAccessDeniedHint(rex)) {
return Status.FORBIDDEN.getStatusCode();
}

int httpCode =
switch (rex) {
case S3Exception s3e -> s3e.statusCode();
case HttpResponseException hre -> hre.getResponse().getStatusCode();
case StorageException se -> se.getCode();
default -> -1;
};
Status httpStatus = Status.fromStatusCode(httpCode);
Status.Family httpFamily = Status.Family.familyOf(httpCode);

if (httpStatus == Status.NOT_FOUND) {
return Status.BAD_REQUEST.getStatusCode();
}
if (httpStatus == Status.UNAUTHORIZED) {
return Status.FORBIDDEN.getStatusCode();
}
if (httpStatus == Status.BAD_REQUEST
|| httpStatus == Status.FORBIDDEN
|| httpStatus == Status.REQUEST_TIMEOUT
|| httpStatus == Status.TOO_MANY_REQUESTS
|| httpStatus == Status.GATEWAY_TIMEOUT) {
return httpCode;
}
if (httpFamily == Status.Family.REDIRECTION) {
// Currently Polaris doesn't know how to follow redirects from cloud providers, thus clients
// shouldn't expect it to.
// This is a 4xx error to indicate that the client may be able to resolve this by changing
// some data, such as their catalog's region.
return 422;
}
if (httpFamily == Status.Family.SERVER_ERROR) {
return Status.BAD_GATEWAY.getStatusCode();
}

return Status.INTERNAL_SERVER_ERROR.getStatusCode();
}
}

0 comments on commit 5c38780

Please sign in to comment.