yuqi1129 commented on code in PR #12793:
URL: https://github.com/apache/gravitino/pull/12793#discussion_r3948117594
##########
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:
I removed authorizationProviderConfigured entirely. BaseCatalog now derives
the configured state on demand from
catalogPropertiesMetadata().getOrDefault(conf, AUTHORIZATION_PROVIDER), so
there is only one source of truth and metadata defaults remain honored. I did
not add close/init synchronization because there is no current re-init path
after publication; synchronization alone would also not define what a
sequential re-init after close should do. A future reload feature should
introduce an explicit lifecycle state and specify that contract as a separate
change.
##########
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:
Fixed. The extra public configuration accessor was removed, and the
pre-existing private isInvokedBy helper was moved into the private-method
section at the end of BaseCatalog.
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -227,7 +229,8 @@ public static void
callAuthorizationPluginForSecurableObjects(
public static void callAuthorizationPluginForMetadataObject(
Review Comment:
Agreed that this fan-out is not transactional, but that behavior already
exists for every authorization-plugin callback exception. Continuing after one
catalog fails would hide the failed external update and report overall success,
so I kept the existing fail-fast policy. Each per-catalog callback now holds
its own lease from #12404, which prevents eviction from producing the
unavailable-plugin state during the callback. Cross-catalog aggregation or
compensation would require a separate design because the external plugins do
not expose rollback.
##########
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:
Thanks for tracing this path. I kept the propagation behavior here. The
authorization rename callback was already post-commit, and any exception from
AuthorizationPlugin.onMetadataUpdated() has the same non-atomic outcome; adding
a compensating catalog rename would be a larger transactional change with its
own collision and rollback-failure cases. After #12404, the callback executes
under doWithCatalog's lease, so concurrent cache eviction cannot make the
plugin unavailable in this window. The checked lookup is now centralized in the
public BaseCatalog accessor and represents an invariant failure rather than a
normal eviction outcome.
##########
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:
I addressed the accessor concern by moving the checked invariant directly
into the public BaseCatalog.getAuthorizationPlugin() method. Callers in every
package now get the same validation; null is returned only when no provider is
configured, and the method is annotated Nullable. I am keeping ops() and
catalogCredentialManager() out of this patch: #12404 makes their active use
lease-scoped as well, while defining a terminal closed state for every lazy
BaseCatalog resource is a broader lifecycle change. Expanding this
authorization fix to those resources would materially increase risk.
##########
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:
Thanks. I addressed the observability point by moving the invariant check
into BaseCatalog.getAuthorizationPlugin() and making the message cause-neutral:
it now reports that the plugin is unavailable while a provider is configured,
without attributing the state to a close race. I kept drop fail-closed
intentionally. The force flag controls catalog metadata cleanup; existing
authorization callback failures already abort the drop before metadata removal.
Swallowing only this failure would reintroduce the stale-grant security issue.
With the lease changes from #12404, cache eviction cannot close the catalog
during this callback, so reaching this branch indicates an invariant or
configuration failure that should not be silently bypassed.
##########
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:
I kept rollback-on-post-hook-failure consistent with the existing create
path. Future grants are part of catalog creation completion, and failures from
the authorization plugin already cause the same rollback; treating an
unavailable configured plugin as a no-op would leave the new catalog without
required future grants. The call is lease-scoped after #12404, so eviction
cannot cause this state while grantNewlyCreatedCatalog runs. The validation now
lives in the public BaseCatalog accessor rather than a
FutureGrantManager-specific helper.
--
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]
