jerryshao commented on code in PR #12793:
URL: https://github.com/apache/gravitino/pull/12793#discussion_r3946197458
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -448,14 +454,38 @@ private static boolean
needApplyAuthorization(MetadataObject.Type type) {
return !SKIP_APPLY_TYPES.contains(type);
}
+ /**
+ * Returns the authorization plugin of the given catalog, or null if the
catalog is not configured
+ * with an authorization provider.
+ *
+ * <p>A catalog that was configured with an authorization provider but no
longer has a plugin may
+ * have been closed. Calling the plugin is then impossible, and silently
skipping the call could
+ * leave stale grants in the external authorization system, so this method
fails loudly instead.
+ *
+ * @param catalog the leased catalog to read the authorization plugin from.
+ * @return the authorization plugin, or null if none is configured.
+ * @throws AuthorizationPluginException if a configured authorization plugin
is unavailable
+ */
+ @Nullable
+ static AuthorizationPlugin getAuthorizationPlugin(BaseCatalog<?> catalog) {
+ AuthorizationPlugin authorizationPlugin = catalog.getAuthorizationPlugin();
+ if (authorizationPlugin == null &&
catalog.isAuthorizationProviderConfigured()) {
+ throw new AuthorizationPluginException(
Review Comment:
**correctness (high)**: This new throw is reached from
`CatalogHookDispatcher.dropCatalog` (unchanged by this diff) via
`AuthorizationUtils.removeCatalogPrivileges`, which runs *before* the actual
`dispatcher.dropCatalog(ident, force)` call. If a catalog's authorization
plugin was already closed by a concurrent cache eviction — the exact scenario
this PR targets — this now aborts the delete entirely, with no way to bypass it
even with `force=true`.
Previously the null plugin was silently skipped and the drop always
proceeded. Now an admin trying to clean up the very catalog whose broken
authorization state triggered this exception has no way to force through the
delete, since `force=true` only reaches `dispatcher.dropCatalog`, which this
exception prevents from ever being called. Worth considering: should
`dropCatalog(..., force=true)` catch/ignore `AuthorizationPluginException` and
proceed, the way `force` already implies for other cleanup failures?
---
**correctness (observability)**: Separately, this exception message
unconditionally attributes the failure to "the catalog may have been closed
while it was in use," but the same null-plugin-while-configured state is also
reachable if a `BaseAuthorization#newPlugin(...)` implementation simply returns
`null` (no non-null contract is enforced on that abstract method) — a static
configuration/plugin bug, not a transient lifecycle race. A misconfigured
plugin factory would produce this exact message on every subsequent
authorization call, misleading operators toward chasing a race condition that
isn't there.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -448,14 +454,38 @@ private static boolean
needApplyAuthorization(MetadataObject.Type type) {
return !SKIP_APPLY_TYPES.contains(type);
}
+ /**
+ * Returns the authorization plugin of the given catalog, or null if the
catalog is not configured
+ * with an authorization provider.
+ *
+ * <p>A catalog that was configured with an authorization provider but no
longer has a plugin may
+ * have been closed. Calling the plugin is then impossible, and silently
skipping the call could
+ * leave stale grants in the external authorization system, so this method
fails loudly instead.
+ *
+ * @param catalog the leased catalog to read the authorization plugin from.
+ * @return the authorization plugin, or null if none is configured.
+ * @throws AuthorizationPluginException if a configured authorization plugin
is unavailable
+ */
+ @Nullable
+ static AuthorizationPlugin getAuthorizationPlugin(BaseCatalog<?> catalog) {
+ AuthorizationPlugin authorizationPlugin = catalog.getAuthorizationPlugin();
+ if (authorizationPlugin == null &&
catalog.isAuthorizationProviderConfigured()) {
+ throw new AuthorizationPluginException(
+ "The authorization plugin of catalog %s is unavailable even though
an authorization "
+ + "provider is configured; the catalog may have been closed
while it was in use",
+ catalog.name());
+ }
+ return authorizationPlugin;
+ }
+
private static void callAuthorizationPluginImpl(
BiConsumer<AuthorizationPlugin, String> consumer,
CatalogManager catalogManager,
NameIdentifier catalogIdent) {
catalogManager.doWithCatalog(
catalogIdent,
catalog -> {
- AuthorizationPlugin authorizationPlugin =
catalog.getAuthorizationPlugin();
+ AuthorizationPlugin authorizationPlugin =
getAuthorizationPlugin(catalog);
Review Comment:
**correctness**: `CatalogHookDispatcher.alterCatalog` (unchanged by this
diff) commits a catalog rename via `dispatcher.alterCatalog(ident, changes)`
*before* calling `AuthorizationUtils.authorizationPluginRenamePrivileges`,
which routes through this line. If the plugin lookup here now throws
`AuthorizationPluginException` (e.g. the catalog was closed mid-rename by a
concurrent eviction), it propagates uncaught with no rollback of the
already-applied rename.
A client renaming a catalog in this window would see the alter request fail
— even though the rename actually succeeded in the entity store — with no
compensating rollback, unlike `createCatalog`'s explicit rollback-on-failure
path a few methods away.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -273,6 +276,21 @@ private boolean isInvokedBy(String methodName) {
.walk(frames -> frames.anyMatch(frame ->
frame.getMethodName().equals(methodName)));
}
+ /**
+ * Returns whether this catalog is configured with an authorization provider.
+ *
+ * <p>The flag is set when {@link
#initAuthorizationPluginInstance(IsolatedClassLoader, long)}
+ * finds an {@code authorization-provider} property, and it stays set for
the whole life of the
+ * catalog. {@link #close()} clears the plugin but not this flag, so a
{@code null} plugin on a
+ * catalog that reports {@code true} here is unavailable, not intentionally
disabled. This can
+ * happen after close or an unsuccessful plugin initialization.
+ *
+ * @return true if an authorization provider was configured for this catalog.
+ */
+ public boolean isAuthorizationProviderConfigured() {
Review Comment:
**conventions**: This new public method
`isAuthorizationProviderConfigured()` is inserted immediately after the private
method `isInvokedBy(String)` (line 274) and is itself followed by more public
methods (`getAuthorizationPlugin()` at 294,
`initAuthorizationPluginInstance(...)` at 311), breaking the visibility
grouping CLAUDE.md requires.
CLAUDE.md's repo-root rules state: "Class Member Ordering: Follow the order:
... 5. Methods (Group by visibility, putting `private` methods at the end)."
This interleaves a private method between public ones instead of grouping all
private methods at the end.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -86,6 +86,9 @@ public abstract class BaseCatalog<T extends BaseCatalog>
// Underlying access control system plugin for this catalog.
private volatile AuthorizationPlugin authorizationPlugin;
+ // Whether an authorization provider is configured for this catalog.
+ private volatile boolean authorizationProviderConfigured;
Review Comment:
**correctness (race, low reachability today)**: `close()` (unchanged by this
diff, line ~347) nulls the `authorizationPlugin` field without acquiring the
same `synchronized (this)` monitor that `initAuthorizationPluginInstance()` and
`getAuthorizationPlugin()` use for their double-checked locking on this same
field. Today `initAuthorizationPluginInstance` is only invoked once,
synchronously, before the catalog is published to the lease cache, so this
isn't reachable through any current call path — but nothing structurally
prevents a future re-init/config-reload feature from re-triggering it on an
already-published catalog, at which point a concurrent `close()` could race it
and leave a "closed" catalog holding a live, never-closed plugin (the same
class of silent-wrong-state bug this PR is meant to eliminate, just inverted).
---
**reuse**: Separately, this new `authorizationProviderConfigured` boolean
caches a fact (whether `AUTHORIZATION_PROVIDER` was set in `conf`) that's
already recomputable on demand from
`catalogPropertiesMetadata().getOrDefault(conf, AUTHORIZATION_PROVIDER)`, since
`conf` is never cleared by `close()`. This creates two sources of truth that
must be kept in sync forever — a future refactor of
`initAuthorizationPluginInstance` (an early return, reordering, or a re-init
path) could change how `conf` is interpreted without remembering to also update
this separately-cached flag, silently desyncing the two and reintroducing a
false negative/positive for exactly the invariant this PR is trying to protect.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -227,7 +229,8 @@ public static void
callAuthorizationPluginForSecurableObjects(
public static void callAuthorizationPluginForMetadataObject(
Review Comment:
**correctness**: `callAuthorizationPluginForMetadataObject` (and the sibling
`callAuthorizationPluginForSecurableObjects`, unchanged by this diff but
sharing the same pattern) fans an operation out across every catalog touched by
a metalake-scoped grant/revoke/rename. The per-catalog loop now aborts entirely
the first time the new checked lookup throws for one catalog, instead of the
old behavior of silently skipping just that catalog and continuing.
If catalog #3 of 10 is mid-close from a concurrent cache eviction during a
metalake-wide role grant or rename, catalogs #1-#2 already received the update,
but the exception thrown for #3 stops the loop before #4-#10 are touched —
leaving the external authorization systems partially and inconsistently updated
with no indication of which catalogs did or didn't receive the change.
`PermissionManager.grantRoleToUser` only catches
`NoSuchEntityException`/`NoSuchRoleException`/`IOException`, so this now fails
the whole call.
##########
core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java:
##########
@@ -273,6 +276,21 @@ private boolean isInvokedBy(String methodName) {
.walk(frames -> frames.anyMatch(frame ->
frame.getMethodName().equals(methodName)));
}
+ /**
+ * Returns whether this catalog is configured with an authorization provider.
+ *
+ * <p>The flag is set when {@link
#initAuthorizationPluginInstance(IsolatedClassLoader, long)}
+ * finds an {@code authorization-provider} property, and it stays set for
the whole life of the
+ * catalog. {@link #close()} clears the plugin but not this flag, so a
{@code null} plugin on a
+ * catalog that reports {@code true} here is unavailable, not intentionally
disabled. This can
+ * happen after close or an unsuccessful plugin initialization.
+ *
+ * @return true if an authorization provider was configured for this catalog.
+ */
+ public boolean isAuthorizationProviderConfigured() {
+ return authorizationProviderConfigured;
+ }
+
public AuthorizationPlugin getAuthorizationPlugin() {
Review Comment:
**altitude**: This unchecked accessor stays `public` and returns the raw,
possibly-null field with no lifecycle validation, while the new safe/checked
lookup (`AuthorizationUtils.getAuthorizationPlugin(BaseCatalog<?>)`) is only
package-private to `org.apache.gravitino.authorization`, so it can't even be
reused by call sites in other packages. Any future call site added elsewhere (a
new dispatcher, a connector module) will naturally reach for this public
accessor and silently reintroduce the exact "closed catalog treated as
authorization disabled" bug this PR fixes — nothing (no `@Deprecated`, no
checkstyle rule) flags the regression.
---
**altitude**: Separately, `ops()` (line 198) and
`catalogCredentialManager()` (line 398) use the identical
lazy-initialize-if-null pattern as `authorizationPlugin`, but `close()` nulls
their backing fields with no equivalent "was configured" guard — a post-close
call silently and transparently re-creates a brand-new
`CatalogOperations`/`CatalogCredentialManager` instance instead of failing
loud, potentially reopening JDBC connections or vending credentials from a
catalog instance that should be considered dead. This PR's stated goal ("a
future regression fails visibly instead of losing updates silently") is only
achieved for the authorization plugin, not for `BaseCatalog`'s other
lifecycle-sensitive resources that share the exact same hazard.
##########
core/src/main/java/org/apache/gravitino/authorization/FutureGrantManager.java:
##########
@@ -57,7 +57,7 @@ public FutureGrantManager(EntityStore entityStore,
OwnerDispatcher ownerDispatch
public void grantNewlyCreatedCatalog(String metalake, BaseCatalog catalog) {
try {
- AuthorizationPlugin authorizationPlugin =
catalog.getAuthorizationPlugin();
+ AuthorizationPlugin authorizationPlugin =
AuthorizationUtils.getAuthorizationPlugin(catalog);
Review Comment:
**correctness (high)**: `grantNewlyCreatedCatalog` only catches
`IOException` (see the catch block at the end of this method), so the new
`AuthorizationPluginException` thrown from
`AuthorizationUtils.getAuthorizationPlugin` propagates straight through. In
`CatalogHookDispatcher.createCatalog` (unchanged by this diff), this is caught
by a generic `catch (Exception postHookException)` that triggers
`dispatcher.dropCatalog(ident, true)` — a full rollback that drops the catalog
that was just successfully created.
This escalates what used to be a silent, graceful "skip the future grant
process to avoid overhead" no-op into destroying a brand-new catalog, for a
failure mode (plugin unexpectedly unavailable) that's arguably transient and
recoverable — future grants could just be retried or applied lazily instead of
triggering catalog destruction.
--
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]
