Skip to content
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

[#6042] refactor: Delete the privilege of catalog after dropping the catalogs #6045

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+59 −16
Open

[#6042] refactor: Delete the privilege of catalog after dropping the catalogs #6045

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
6f7968b
[#6042] refactor: Use callback to delete the privilege of catalog
jerqi Dec 30, 2024
24aec9f
address comment
jerqi Jan 2, 2025
2eaa909
Update authorizations/authorization-ranger/src/test/java/org/apache/g…
jerqi Jan 2, 2025
eb5b9cc
address comments
jerqi Jan 2, 2025
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
2 changes: 2 additions & 0 deletions ...test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerBaseE2EIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ protected void cleanIT() {
(schema -> {
catalog.asSchemas().dropSchema(schema, false);
}));

// The `dropCatalog` call will invoke the catalog metadata object to remove privileges
Arrays.stream(metalake.listCatalogs())
.forEach((catalogName -> metalake.dropCatalog(catalogName, true)));
client.disableMetalake(metalakeName);
Expand Down
58 changes: 44 additions & 14 deletions core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.apache.gravitino.Entity;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.MetadataObject;
import org.apache.gravitino.MetadataObjects;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.catalog.CatalogManager;
Expand Down Expand Up @@ -174,19 +175,8 @@ public static void callAuthorizationPluginForSecurableObjects(

public static void callAuthorizationPluginForMetadataObject(
String metalake, MetadataObject metadataObject, Consumer<AuthorizationPlugin> consumer) {
CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
NameIdentifier[] catalogs = catalogManager.listCatalogs(Namespace.of(metalake));
// ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we need `BaseCatalog` to
// call authorization plugin method.
for (NameIdentifier catalog : catalogs) {
callAuthorizationPluginImpl(consumer, catalogManager.loadCatalog(catalog));
}
} else if (needApplyAuthorization(metadataObject.type())) {
NameIdentifier catalogIdent =
NameIdentifierUtil.getCatalogIdentifier(
MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
Catalog catalog = catalogManager.loadCatalog(catalogIdent);
List<Catalog> loadedCatalogs = loadMetadataObjectCatalog(metalake, metadataObject);
for (Catalog catalog : loadedCatalogs) {
callAuthorizationPluginImpl(consumer, catalog);
}
}
Expand Down Expand Up @@ -255,15 +245,33 @@ public static void authorizationPluginRemovePrivileges(
if (GravitinoEnv.getInstance().accessControlDispatcher() != null) {
MetadataObject metadataObject = NameIdentifierUtil.toMetadataObject(ident, type);
MetadataObjectChange removeObject = MetadataObjectChange.remove(metadataObject);

String metalake =
type == Entity.EntityType.METALAKE ? ident.name() : ident.namespace().level(0);

callAuthorizationPluginForMetadataObject(
ident.namespace().level(0),
metalake,
metadataObject,
authorizationPlugin -> {
authorizationPlugin.onMetadataUpdated(removeObject);
});
}
}

public static void removeCatalogPrivileges(Catalog catalog) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems no need to extract this logic as a method.
Thoughts?

Copy link
Contributor Author

@jerqi jerqi Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer putting the logic into AuthorizationUtils. callAuthorizationPluginImpl is a private implement method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Em... then why don't we publicize this callAuthorizationPluginImpl?

Copy link
Contributor Author

@jerqi jerqi Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer putting similar logic to one place. Other similar logic are in the class AuthorizationUtils, It's better to maintain the code.
I don't preferring expose more implement methods to other package.

// If we enable authorization, we should remove the privileges about the entity in the
// authorization plugin.
MetadataObject metadataObject =
MetadataObjects.of(null, catalog.name(), MetadataObject.Type.CATALOG);
MetadataObjectChange removeObject = MetadataObjectChange.remove(metadataObject);

callAuthorizationPluginImpl(
authorizationPlugin -> {
authorizationPlugin.onMetadataUpdated(removeObject);
},
catalog);
}

public static void authorizationPluginRenamePrivileges(
NameIdentifier ident, Entity.EntityType type, String newName) {
// If we enable authorization, we should rename the privileges about the entity in the
Expand Down Expand Up @@ -364,4 +372,26 @@ private static void checkCatalogType(
catalogIdent, catalog.type(), privilege);
}
}

private static List<Catalog> loadMetadataObjectCatalog(
String metalake, MetadataObject metadataObject) {
CatalogManager catalogManager = GravitinoEnv.getInstance().catalogManager();
List<Catalog> loadedCatalogs = Lists.newArrayList();
if (needApplyAuthorizationPluginAllCatalogs(metadataObject.type())) {
NameIdentifier[] catalogs = catalogManager.listCatalogs(Namespace.of(metalake));
// ListCatalogsInfo return `CatalogInfo` instead of `BaseCatalog`, we need `BaseCatalog` to
// call authorization plugin method.
for (NameIdentifier catalog : catalogs) {
loadedCatalogs.add(catalogManager.loadCatalog(catalog));
}
} else if (needApplyAuthorization(metadataObject.type())) {
NameIdentifier catalogIdent =
NameIdentifierUtil.getCatalogIdentifier(
MetadataObjectUtil.toEntityIdent(metalake, metadataObject));
Catalog catalog = catalogManager.loadCatalog(catalogIdent);
loadedCatalogs.add(catalog);
}

return loadedCatalogs;
}
}
15 changes: 13 additions & 2 deletions core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,19 @@ public boolean dropCatalog(NameIdentifier ident) {
@Override
public boolean dropCatalog(NameIdentifier ident, boolean force)
throws NonEmptyEntityException, CatalogInUseException {
AuthorizationUtils.authorizationPluginRemovePrivileges(ident, Entity.EntityType.CATALOG);
return dispatcher.dropCatalog(ident, force);
if (!dispatcher.catalogExists(ident)) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return true directly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not exist, we should return false directly. Changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Emm ... think twice about the protocol please.
This function returns a boolean. You can use it for one of two purposes:

  • an indication of whether the dropCatalog operation was successful;
  • an indication of whether the catalog (and related stuff) no longer exists.
    Both are okay.
    I'm more inclined to the second pattern and the reason is that I would expect
    each function to serve a business logic with a business purpose.
    This is a hint I got from line 141, where the function returns dropped as a boolean.
    To be consistent, I'd see a negative return value as catalog not dropped.

This is not a mandatory change request. I was just dumping my brain on the
(functional) interface design. It is still okay to return false in this case,
which means the return value is about whether the operation was successful or not.

Copy link
Contributor Author

@jerqi jerqi Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the catalog doesn't exist, we would return false. Our other code follows this rule, too. Only the catalog exists, we would return true. If we occurs some errors, we would throw exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I wasn't suggesting that this is a right or wrong issue.
It is in a gray area, which means we have to cope with it carefully.
We'd better have a clear mindset / protocol on this.

If we occurs some errors, we would throw exceptions.

This, again, leads us to the situation of multiple exit conditions. We may return true/false AND we may throw exceptions. That is not a good design for primitive functions. There are two options that are simpler and clearer:

  • Ensure that no exception will be thrown from this function, AND we return a boolean to indicate whether the operation was successful. This means we need to catch the underlying exception(s) and stop propagating them in the call stack.
  • Remove the boolean return value. This function either succeeds (silently) or fails with an exception. Hopefully, the exception will be caught very soon before it is causing further misbehaviors.

Copy link
Contributor Author

@jerqi jerqi Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your suggestion. I got your concern. It's a bigger topic. For this pull request, I will the semantic as community discussed before. You can discuss this topic in #5031 if you have concern.


// If we call the authorization plugin after dropping catalog, we can't load the plugin of the
// catalog
Catalog catalog = dispatcher.loadCatalog(ident);
boolean dropped = dispatcher.dropCatalog(ident, force);

if (dropped && catalog != null) {
AuthorizationUtils.removeCatalogPrivileges(catalog);
}
return dropped;
}

@Override
Expand Down
Loading