Skip to content

Return 503 instead of 500 when the metastore fails during authentication #5247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shyundev wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Return 503 instead of 500 when the metastore fails during authentication #5247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d70ac7
Return 503 instead of 500 when the metastore fails during authentication
shyundev Aug 6, 2026
704b18c
Pass the cause to the log event
shyundev Aug 7, 2026
eb6c4a7
Throw PolarisServiceUnavailableException from metaStoreUnavailable
shyundev Aug 14, 2026
4a3fba7
Treat zero as a valid Retry-After value
shyundev Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
56 changes: 40 additions & 16 deletions runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -144,12 +144,10 @@ 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 resolve principal entity from credentials",
"Unable to fetch principal entity");
}

if (principal == null || principal.getType() != PolarisEntityType.PRINCIPAL) {
Expand Down Expand Up @@ -258,8 +256,13 @@ 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 load grants for principal", "Unable to fetch principal grants");

@dimas-b dimas-b Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're touching this code... I believe it is preferable not to disclose any error details to the client before it has been properly authorized (to reduce the risk of disclosing valuable information about the server to malicious clients).

In this case I believe we ought to log these messages (Unable to fetch principal grants, etc.) with a UUID and pass the UUID back to the client with a generic "service unavailable" message.

The admin user will be able to correlate the client-side error UUID to specific failures in Polaris logs if it comes to debugging.

@dimas-b dimas-b Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cf. #5011

@dimas-b dimas-b Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree, we probably need to rebase this PR on top of #5119 and redo those new messages too.

@shyundev shyundev Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. On the UUID, #5011 points at something already in place: the response carries the request id as X-Request-ID and the default log format prints the same id, so there is nothing new to mint. #4406 took that route on the 403 path, and I checked a 503 raised during authentication comes back with the header set.

That leaves the message, and it should cover the existing Unable to fetch principal entity too, otherwise two of the three lookups go generic and one keeps its current text. Doing that here rewrites a message that has already shipped, so I'd rather put all three in one follow-up together with the exception type from the other thread, since both come down to what metaStoreUnavailable throws. I'll open it as soon as this is in.

On the rebase: in DefaultAuthenticator the only lines both PRs change are the resolvePrincipalEntity catch block, so nothing here has to wait. If #5119 goes in first I'll rebase onto it.

@dimas-b dimas-b Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up SGTM 👍

@shyundev shyundev Aug 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Narrowing that follow-up to the three messages, since the exception type is now in this PR.

}
diagnostics.check(
principalGrantResults.isSuccess(),
"Failed to resolve principal roles for principal name={} id={}",
Expand All @@ -285,13 +288,34 @@ protected LoadGrantsResult loadPrincipalGrants(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 load securable entity for grant", "Unable to fetch securable entity");
}
}

/**
* 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.
*/
private static PolarisServiceUnavailableException metaStoreUnavailable(
Exception cause, String logMessage, String responseMessage) {
LOGGER
.atError()
.setCause(cause)
.addKeyValue(StructuredLogKeys.ERR_MSG, cause.getMessage())
.addKeyValue(StructuredLogKeys.STACK_TRACE, Throwables.getStackTraceAsString(cause))

@dimas-b dimas-b Aug 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not also pass cause to .setCause()? That will ensure logging the stack trace in plain test loggers too. We can certainly keep the STACK_TRACE attribute for backward compatibility.

@shyundev shyundev Aug 13, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: .setCause(cause) added in metaStoreUnavailable, with the STACK_TRACE attribute kept as you suggested. One thing that turned up while checking it: slf4j-jboss-logmanager's Slf4jLogger is not LoggingEventAware, so slf4j merges key-values into the message text instead of passing them through, and with the default %s%e format the trace now appears twice, once inside the message and once from the cause. I kept the attribute anyway, since that merged text is what anything reading stackTrace= today already sees.

.log(logMessage);
return new PolarisServiceUnavailableException(0, "%s", responseMessage);
}

protected record PrincipalRoleSelection(Set<String> roles, boolean allRolesRequested) {}
Expand Down
2 changes: 1 addition & 1 deletion ...me/service/src/main/java/org/apache/polaris/service/exception/PolarisExceptionMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public Response toResponse(PolarisException exception) {
Response.ResponseBuilder builder =
Response.status(statusCode).entity(errorResponse).type(MediaType.APPLICATION_JSON_TYPE);
if (exception instanceof PolarisServiceUnavailableException e
&& e.getRetryAfterSeconds() != 0) {
&& e.getRetryAfterSeconds() >= 0) {
builder.header(HttpHeaders.RETRY_AFTER, e.getRetryAfterSeconds());
}
return builder.build();
Expand Down
57 changes: 55 additions & 2 deletions runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
13 changes: 9 additions & 4 deletions runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -112,13 +113,17 @@ public void testNamespaceException() {
assertThat(response.getStatus()).isEqualTo(409);
}

@Test
public void testServiceUnavailableWithRetryAfter() {
@ParameterizedTest
@CsvSource(
value = {"3, 3", "0, 0", "-1, null"},
nullValues = "null")
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
Expand Down