jerryshao commented on code in PR #13699:
URL: https://github.com/apache/gravitino/pull/13699#discussion_r4230347293
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizationLookups.java:
##########
@@ -131,11 +97,85 @@ public Optional<OwnerInfo> resolveOwnerId(
id -> ownerRelCache.get(id, k -> loadOwner(k, metadataType)));
}
+ // Preserve normalization failures as distinct from missing objects for
DENY-policy consumers.
+ MetadataIdResolution resolveMetadataIdResult(
+ MetadataObject metadataObject, String metalake,
AuthorizationRequestContext requestContext) {
+ MetadataObject cacheObject;
+ try {
+ // Use the same capability rules as ID resolution so hooks and peer
change-log replay
+ // evict every equivalent spelling from both cache tiers.
+ cacheObject =
+ requestContext.computeNormalizedMetadataObjectIfAbsent(
+ JcasbinAuthorizationCacheKeys.metadataIdCacheKey(metalake,
metadataObject),
+ ignored ->
MetadataIdConverter.normalizeMetadataObject(metadataObject, metalake));
+ } catch (NotFoundException e) {
+ return new MetadataIdResolution(Optional.empty(), false);
+ } catch (RuntimeException e) {
+ // Never fall back to the raw key: it can retain an ID after
canonical-name invalidation.
+ // Catch only normalization failures; entity-store and cache-loader
failures still propagate.
+ // Failures are not memoized, so a transient one can succeed on retry in
the same request.
+ // The cost: while a catalog cannot initialize, every object of that
catalog looked up in a
+ // request reloads it (connector initialization included) and logs this
warning again. A
+ // per-request, per-catalog failure memo or a once-per-request warning
would bound that.
+ LOG.warn(
+ "Cannot normalize metadata object {}:{} in metalake {};
authorization lookup is unresolved",
+ metadataObject.type(),
+ metadataObject.fullName(),
+ metalake,
+ e);
+ return new MetadataIdResolution(Optional.empty(), true);
Review Comment:
[Important] A failed capability lookup is memoized nowhere, so one broken
catalog now costs many connector-initialization attempts per request instead of
one.
Two layers both decline to remember the failure.
`computeNormalizedMetadataObjectIfAbsent` uses `computeIfAbsent`
(`AuthorizationRequestContext.java:225-230`), which records no mapping when the
loader throws, and `CatalogManager.loadCatalogAndWrapInternal` goes through
`catalogCache.get(ident, this::loadCatalogInternal)`
(`CatalogManager.java:1642-1654`), which Caffeine does not populate when its
loader throws. So every lookup that reaches here re-runs `createCatalogWrapper`
and the connector's `initialize()`.
Count for one request on such a catalog under `ANY(DENY_SELECT_TABLE,
METALAKE, CATALOG, SCHEMA, TABLE)`
(`AuthorizationExpressionConverter.java:256`): `SCHEMA` and `TABLE` are both
capability-scoped and the allow and deny authorizers each resolve both, so 4
attempts for the checked object alone; plus one per distinct securable object
of that catalog while loading each role (`JcasbinAuthorizer.java:1873`); plus a
repeat of all of it on each `evaluateWithLoadedRolePolicies` reload, up to
`MAX_ROLE_POLICY_RELOADS` = 3. On `origin/main` the first failure aborted the
request after exactly one attempt. If `initialize()` blocks on a connect
timeout (Hive metastore client, JDBC pool), the request holds its thread for a
multiple of that timeout and logs this WARN with a stack trace each time;
enough concurrent requests and the amplification reaches the request thread
pool, working against this change's "other catalogs stay usable" goal.
The comment here already names the fix. A per-request set of `(metalake,
catalog)` pairs whose capability resolution failed, consulted before
`normalizeMetadataObject` and returning the same unresolved result, bounds this
to one attempt and one WARN per catalog per request. It weakens nothing: the
outcome for every object of that catalog in that request is already identical,
and a per-request memo still lets the next request observe recovery.
Verified by: reading `computeNormalizedMetadataObjectIfAbsent`,
`normalizeMetadataObject` into `CapabilityHelpers.getCapability` (`:55-64`)
into
`CatalogManager.doWithCatalog`/`loadCatalogAndWrapInternal`/`loadCatalogInternal`,
both `resolveMetadataIdResult` call sites (`JcasbinAuthorizer.java:973`,
`:1873`), and the DENY lists in `AuthorizationExpressionConverter.java:241-374`
in this checkout.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/jcasbin/JcasbinAuthorizer.java:
##########
@@ -1861,37 +1869,91 @@ private ResolvedRolePolicies resolveRolePolicies(
List<String> unresolvedObjects = new ArrayList<>();
for (SecurableObject securableObject : securableObjects) {
- Optional<Long> metadataId =
- lookups.resolveMetadataId(securableObject, metalake, requestContext);
+ JcasbinAuthorizationLookups.MetadataIdResolution resolution =
+ lookups.resolveMetadataIdResult(securableObject, metalake,
requestContext);
+ Optional<Long> metadataId = resolution.metadataId();
if (!metadataId.isPresent()) {
unresolvedObjects.add(securableObject.type().name() + ":" +
securableObject.fullName());
+ if (resolution.normalizationFailed()) {
+ addUnresolvedDenyPolicies(
+ securableObject, metalake, roleIdStr, requestContext, allowRows,
denyRows);
+ }
continue;
}
- String metadataIdStr = String.valueOf(metadataId.get());
- for (Privilege privilege : securableObject.privileges()) {
- Privilege.Condition condition = privilege.condition();
- String action =
- AuthorizationUtils.replaceLegacyPrivilegeName(privilege.name())
- .name()
- .toUpperCase(Locale.ROOT);
- if (AuthConstants.DENY.equalsIgnoreCase(condition.name())) {
- denyRows.add(
- new String[] {
- roleIdStr, securableObject.type().name(), metadataIdStr,
action, AuthConstants.ALLOW
- });
- }
+ addPolicyRows(
+ roleIdStr,
+ securableObject.type(),
+ metadataId.get(),
+ securableObject.privileges(),
+ allowRows,
+ denyRows);
+ }
+ return new ResolvedRolePolicies(allowRows, denyRows, unresolvedObjects);
+ }
- allowRows.add(
+ private void addUnresolvedDenyPolicies(
+ SecurableObject object,
+ String metalake,
+ String roleId,
+ AuthorizationRequestContext requestContext,
+ List<String[]> allowRows,
+ List<String[]> denyRows) {
+ List<Privilege> denies =
+ object.privileges().stream()
+ .filter(privilege -> privilege.condition() ==
Privilege.Condition.DENY)
+ .collect(Collectors.toList());
+ if (denies.isEmpty()) {
+ return;
+ }
+ // An unresolved child DENY must not disappear while an ancestor or
another role grants
+ // ALLOW. Conservatively deny the same privileges throughout its catalog
until the existing
+ // partial-role retry resolves the precise object again. Catalog IDs need
no capability lookup,
+ // and other catalogs remain usable. A missing entity alone does not
broaden existing policies.
+ MetadataObject catalog =
+ NameIdentifierUtil.toMetadataObject(
+ NameIdentifierUtil.getCatalogIdentifier(
+ MetadataObjectUtil.toEntityIdent(metalake, object)),
+ Entity.EntityType.CATALOG);
+ Optional<Long> catalogId = lookups.resolveMetadataId(catalog, metalake,
requestContext);
+ if (!catalogId.isPresent()) {
+ // Without a catalog ID even a catalog-scoped guard cannot be installed
safely. A missing
+ // catalog makes normalization report a missing object instead, so this
is only reachable
+ // when the catalog is dropped between the two lookups. Skipping the
DENY could drop it for
+ // a catalog recreated under the same name, so fail this load; the next
request retries.
+ throw new IllegalStateException(
+ "Cannot resolve catalog for unresolved deny policy on " +
object.fullName());
Review Comment:
[Question] On the initial role load this `IllegalStateException` still
escapes as a 500, while the identical failure one call site later fails closed.
Is the asymmetry deliberate?
`resolveRolePolicies` runs inside `loadStaleRolePolicies`, which
`loadRolePrivilege` calls with no guard (`JcasbinAuthorizer.java:1325-1327`),
so this throw propagates out through `computeIfAbsent` in
`AuthorizationRequestContext` and surfaces as a server error. The other caller
wraps the same method in `catch (RuntimeException)` and returns
`Optional.empty()`, which the authorizers turn into `unstablePolicyResult`,
i.e. a 403 (`JcasbinAuthorizer.java:1765-1779`).
I agree the window is narrow, and the comment's reasoning holds up:
`loadCatalogInternal` converts a missing catalog entity into
`NoSuchCatalogException` (`CatalogManager.java:1746-1757`), which reaches
`resolveMetadataIdResult` as a `NotFoundException` and is reported as a missing
object, not a normalization failure - so this needs the catalog to disappear
between the capability call and the catalog-ID lookup in the same request.
`testUnresolvedDenyWithoutCatalogIdFailsBeforePoliciesAreApplied` pins the
behaviour and shows no partial policies are published, which is the important
half.
Still, it is the one remaining path where an unresolvable catalog turns an
authorization request into a 500 - the failure mode the rest of this commit
removes. Adding the role to the `unreadableRoleIds` set that
`loadStaleRolePolicies` already builds and returns
(`JcasbinAuthorizer.java:1492`, `:1523`, `:1564`) would fail closed through the
existing channel, publish nothing, and still retry on the next request, without
the DENY-dropping risk the comment rightly rules out. If you prefer to keep the
throw, a line in the PR description naming this residual 500 would settle it.
Verified by: reading both `versionCheckAndLoadRoles` call sites and
`evaluateWithLoadedRolePolicies` in this checkout, `loadCatalogInternal`'s
exception mapping, and the `NotFoundException` branch at
`JcasbinAuthorizationLookups.java:111-112`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
