diff --git a/CHANGELOG.md b/CHANGELOG.md index bef920fa73..3bba12e819 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti - Python CLI `setup` now preserves `endpoint_internal` and `sts_endpoint` during apply and export for S3 configuration - Fixed a false-negative in the JDBC optimized location-overlap check (`OPTIMIZED_SIBLING_CHECK`). Ancestor locations stored in `location_without_scheme` without a trailing slash were not matched by the generated ancestor equality terms, allowing nested table/namespace locations to be created under existing prefixes. The query now emits both slash-terminated and non-slash-terminated prefix terms and uses a slash-terminated `LIKE` pattern for descendant matching. - Python CLI `setup` now preserves the Azure `hierarchical` storage flag during apply and export +- A metastore failure while resolving a principal's roles during authentication now returns HTTP 503, as it already did when looking up the principal entity; previously it propagated unwrapped and was reported as HTTP 500. All three lookups now report `PolarisServiceUnavailableException` as the error `type`, where the principal entity lookup previously reported `ServiceUnavailableException` with the same status. All three also send `Retry-After: 0`; the Iceberg REST spec has a client retry a non-idempotent request only when that header is present. ### Commits diff --git a/polaris-core/src/main/java/org/apache/polaris/core/exceptions/PolarisServiceUnavailableException.java b/polaris-core/src/main/java/org/apache/polaris/core/exceptions/PolarisServiceUnavailableException.java index fa596bf287..e734f4ece4 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/exceptions/PolarisServiceUnavailableException.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/exceptions/PolarisServiceUnavailableException.java @@ -24,6 +24,9 @@ /** * Signals a transient failure that the client may resolve by retrying. Mapped to HTTP 503 (Service * Unavailable) with a {@code Retry-After} header whose value is {@link #getRetryAfterSeconds()}. + * + *

{@code Retry-After} admits only a non-negative delay, so a negative {@code retryAfterSeconds} + * is stored as {@code 0}. */ public class PolarisServiceUnavailableException extends PolarisException { @@ -32,7 +35,7 @@ public class PolarisServiceUnavailableException extends PolarisException { @FormatMethod public PolarisServiceUnavailableException(int retryAfterSeconds, String message, Object... args) { super(String.format(message, args)); - this.retryAfterSeconds = retryAfterSeconds; + this.retryAfterSeconds = Math.max(0, retryAfterSeconds); } public int getRetryAfterSeconds() { diff --git a/runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java b/runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java index 65e227b943..c5a8a06b2b 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java @@ -25,7 +25,6 @@ import io.smallrye.common.annotation.Identifier; import jakarta.enterprise.context.RequestScoped; import jakarta.inject.Inject; -import jakarta.ws.rs.ServiceUnavailableException; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -40,6 +39,7 @@ import org.apache.polaris.core.entity.PolarisGrantRecord; import org.apache.polaris.core.entity.PrincipalEntity; import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.exceptions.PolarisServiceUnavailableException; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.dao.entity.LoadGrantsResult; import org.eclipse.microprofile.jwt.JsonWebToken; @@ -144,12 +144,12 @@ protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials) .orElse(null); } } catch (Exception e) { - LOGGER - .atError() - .addKeyValue(StructuredLogKeys.ERR_MSG, e.getMessage()) - .addKeyValue(StructuredLogKeys.STACK_TRACE, Throwables.getStackTraceAsString(e)) - .log("Unable to resolve principal entity from credentials"); - throw new ServiceUnavailableException("Unable to fetch principal entity"); + throw metaStoreUnavailable( + e, + "Unable to fetch principal entity", + "Unable to resolve principal entity from credentials, principalName={} principalId={}", + credentials.getPrincipalName(), + credentials.getPrincipalId()); } if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) { @@ -196,7 +196,7 @@ protected PrincipalRoleSelection resolvePrincipalRoles( Set activeRoles = loadGrantsResult.getGrantRecords().stream() - .map(gr -> loadSecurableEntity(gr, entitiesById)) + .map(gr -> loadSecurableEntity(gr, entitiesById, principal)) .filter(Objects::nonNull) .filter(entity -> entity.getType() == PolarisEntityType.PRINCIPAL_ROLE) .map(PrincipalRoleEntity::of) @@ -258,8 +258,17 @@ protected PrincipalRoleSelection extractRequestedRoles(PolarisCredential credent */ protected LoadGrantsResult loadPrincipalGrants(PrincipalEntity principal) { PolarisCallContext polarisContext = callContext.getPolarisCallContext(); - LoadGrantsResult principalGrantResults = - metaStoreManager.loadGrantsToGrantee(polarisContext, principal); + LoadGrantsResult principalGrantResults; + try { + principalGrantResults = metaStoreManager.loadGrantsToGrantee(polarisContext, principal); + } catch (Exception e) { + throw metaStoreUnavailable( + e, + "Unable to fetch principal grants", + "Unable to load grants, principalName={} principalId={}", + principal.getName(), + principal.getId()); + } diagnostics.check( principalGrantResults.isSuccess(), "Failed to resolve principal roles for principal name={} id={}", @@ -278,20 +287,55 @@ protected LoadGrantsResult loadPrincipalGrants(PrincipalEntity principal) { /** * Resolves the securable entity for a grant record, using preloaded entities when available and * falling back to {@link PolarisMetaStoreManager#loadEntity} only when the metastore did not - * populate {@link LoadGrantsResult#getEntities()}. + * populate {@link LoadGrantsResult#getEntities()}. The principal identifies the failing request + * if that fallback hits a metastore failure. */ private @Nullable PolarisBaseEntity loadSecurableEntity( - PolarisGrantRecord grant, @Nullable Map entitiesById) { + PolarisGrantRecord grant, + @Nullable Map entitiesById, + PrincipalEntity principal) { if (entitiesById != null) { return entitiesById.get(grant.getSecurableId()); } - return metaStoreManager - .loadEntity( - callContext.getPolarisCallContext(), - grant.getSecurableCatalogId(), - grant.getSecurableId(), - PolarisEntityType.PRINCIPAL_ROLE) - .getEntity(); + PolarisCallContext polarisContext = callContext.getPolarisCallContext(); + try { + return metaStoreManager + .loadEntity( + polarisContext, + grant.getSecurableCatalogId(), + grant.getSecurableId(), + PolarisEntityType.PRINCIPAL_ROLE) + .getEntity(); + } catch (Exception e) { + throw metaStoreUnavailable( + e, + "Unable to fetch securable entity", + "Unable to load securable entity for grant, principalName={} principalId={} " + + "securableCatalogId={} securableId={}", + principal.getName(), + principal.getId(), + grant.getSecurableCatalogId(), + grant.getSecurableId()); + } + } + + /** + * Logs a metastore failure raised during authentication and returns the exception to throw, so + * that a failing backend is reported as a transient condition instead of an internal error. + * + * @param cause the metastore failure + * @param responseMessage the message returned to the client + * @param logMessage the log message, with SLF4J placeholders for {@code logArgs} + * @param logArgs the values for the placeholders in {@code logMessage} + */ + private static PolarisServiceUnavailableException metaStoreUnavailable( + Exception cause, String responseMessage, String logMessage, Object... logArgs) { + LOGGER + .atError() + .addKeyValue(StructuredLogKeys.ERR_MSG, cause.getMessage()) + .addKeyValue(StructuredLogKeys.STACK_TRACE, Throwables.getStackTraceAsString(cause)) + .log(logMessage, logArgs); + return new PolarisServiceUnavailableException(0, "%s", responseMessage); } protected record PrincipalRoleSelection(Set roles, boolean allRolesRequested) {} diff --git a/runtime/service/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java b/runtime/service/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java index b3ed74bf29..8b2b55cff5 100644 --- a/runtime/service/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java +++ b/runtime/service/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java @@ -56,8 +56,7 @@ public Response toResponse(PolarisException exception) { .build(); Response.ResponseBuilder builder = Response.status(statusCode).entity(errorResponse).type(MediaType.APPLICATION_JSON_TYPE); - if (exception instanceof PolarisServiceUnavailableException e - && e.getRetryAfterSeconds() != 0) { + if (exception instanceof PolarisServiceUnavailableException e) { builder.header(HttpHeaders.RETRY_AFTER, e.getRetryAfterSeconds()); } return builder.build(); diff --git a/runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java b/runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java index d502b73ac0..14cb2a5f12 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java @@ -34,7 +34,6 @@ import io.quarkus.test.junit.QuarkusTest; import io.smallrye.common.annotation.Identifier; import jakarta.inject.Inject; -import jakarta.ws.rs.ServiceUnavailableException; import java.util.Map; import java.util.Set; import org.apache.polaris.core.PolarisDiagnostics; @@ -46,6 +45,7 @@ import org.apache.polaris.core.entity.PolarisEntityType; import org.apache.polaris.core.entity.PrincipalEntity; import org.apache.polaris.core.entity.PrincipalRoleEntity; +import org.apache.polaris.core.exceptions.PolarisServiceUnavailableException; import org.apache.polaris.core.identity.provider.ServiceIdentityProvider; import org.apache.polaris.core.persistence.PolarisMetaStoreManager; import org.apache.polaris.core.persistence.dao.entity.LoadGrantsResult; @@ -151,7 +151,60 @@ public void testFetchPrincipalThrowsServiceExceptionOnMetastoreException() { DefaultAuthenticator standaloneAuthenticator = newStandaloneAuthenticator(metaStoreManagerSpy); assertThatThrownBy(() -> standaloneAuthenticator.authenticate(identityFor(credentials))) - .isInstanceOf(ServiceUnavailableException.class); + .isInstanceOfSatisfying( + PolarisServiceUnavailableException.class, + e -> assertThat(e.getRetryAfterSeconds()).isZero()); + } + + @Test + void testLoadGrantsThrowsServiceExceptionOnMetastoreException() { + // Given: a metastore that fails while loading the principal's grants + PolarisCredential credentials = + PolarisCredential.of( + principalEntity.getId(), null, Set.of(DefaultAuthenticator.PRINCIPAL_ROLE_ALL)); + + PolarisMetaStoreManager metaStoreManagerSpy = Mockito.spy(metaStoreManager); + Mockito.doThrow(new RuntimeException("Metastore exception")) + .when(metaStoreManagerSpy) + .loadGrantsToGrantee(any(), any()); + + DefaultAuthenticator standaloneAuthenticator = newStandaloneAuthenticator(metaStoreManagerSpy); + + // When/Then: the metastore failure should surface as PolarisServiceUnavailableException + assertThatThrownBy(() -> standaloneAuthenticator.authenticate(identityFor(credentials))) + .isInstanceOfSatisfying( + PolarisServiceUnavailableException.class, + e -> assertThat(e.getRetryAfterSeconds()).isZero()); + } + + @Test + void testLoadSecurableEntityThrowsServiceExceptionOnMetastoreException() { + // Given: grants without preloaded entities, so role resolution falls back to loadEntity, + // and a metastore that fails on that fallback + LoadGrantsResult grants = + metaStoreManager.loadGrantsToGrantee(callContext.getPolarisCallContext(), principalEntity); + LoadGrantsResult grantsWithoutEntities = + new LoadGrantsResult(grants.getGrantsVersion(), grants.getGrantRecords(), null); + + PolarisMetaStoreManager metaStoreManagerSpy = Mockito.spy(metaStoreManager); + Mockito.doReturn(grantsWithoutEntities) + .when(metaStoreManagerSpy) + .loadGrantsToGrantee( + any(), Mockito.argThat(p -> p != null && p.getId() == principalEntity.getId())); + Mockito.doThrow(new RuntimeException("Metastore exception")) + .when(metaStoreManagerSpy) + .loadEntity(any(), anyLong(), anyLong(), Mockito.eq(PolarisEntityType.PRINCIPAL_ROLE)); + + PolarisCredential credentials = + PolarisCredential.of(null, PRINCIPAL_NAME, Set.of(DefaultAuthenticator.PRINCIPAL_ROLE_ALL)); + + DefaultAuthenticator standaloneAuthenticator = newStandaloneAuthenticator(metaStoreManagerSpy); + + // When/Then: the metastore failure should surface as PolarisServiceUnavailableException + assertThatThrownBy(() -> standaloneAuthenticator.authenticate(identityFor(credentials))) + .isInstanceOfSatisfying( + PolarisServiceUnavailableException.class, + e -> assertThat(e.getRetryAfterSeconds()).isZero()); } @Test diff --git a/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java b/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java index 7ab4284a28..daada9184d 100644 --- a/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java +++ b/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java @@ -54,6 +54,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Mockito; import org.slf4j.Logger; @@ -112,13 +113,15 @@ public void testNamespaceException() { assertThat(response.getStatus()).isEqualTo(409); } - @Test - public void testServiceUnavailableWithRetryAfter() { + @ParameterizedTest + @CsvSource({"3, 3", "0, 0", "-1, 0"}) + public void testServiceUnavailableRetryAfter(int retryAfterSeconds, String expectedHeader) { PolarisExceptionMapper mapper = new PolarisExceptionMapper(); Response response = - mapper.toResponse(new PolarisServiceUnavailableException(3, "transient conflict")); + mapper.toResponse( + new PolarisServiceUnavailableException(retryAfterSeconds, "transient conflict")); assertThat(response.getStatus()).isEqualTo(503); - assertThat(response.getHeaderString(HttpHeaders.RETRY_AFTER)).isEqualTo("3"); + assertThat(response.getHeaderString(HttpHeaders.RETRY_AFTER)).isEqualTo(expectedHeader); } @ParameterizedTest