Skip to content

[#6716] improvement(authz): Delete catalogs if failing to execute post hook actions #6717

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.

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

Merged
merged 7 commits into from
Mar 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,7 @@ protected RangerAuthorizationPlugin(String metalake, Map<String, String> config)
rangerServiceName = config.get(RangerAuthorizationProperties.RANGER_SERVICE_NAME);
rangerClient = new RangerClientExtension(rangerUrl, authType, rangerAdminName, password);

if (Boolean.parseBoolean(
config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))) {
createRangerServiceIfNecessary(config, rangerServiceName);
}
createRangerServiceIfNecessary(config, rangerServiceName);

rangerHelper =
new RangerHelper(
Expand Down Expand Up @@ -786,7 +783,9 @@ private void createRangerServiceIfNecessary(Map<String, String> config, String s
try {
rangerClient.getService(serviceName);
} catch (RangerServiceException rse) {
if (rse.getStatus().equals(ClientResponse.Status.NOT_FOUND)) {
if (Boolean.parseBoolean(
config.get(RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT))
&& ClientResponse.Status.NOT_FOUND.equals(rse.getStatus())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You have changed the original design:
Before: If RANGER_SERVICE_CREATE_IF_ABSENT is false, there will be no exception in any condition.
Now: If RANGER_SERVICE_CREATE_IF_ABSENT is false and the service is unavailable, you will throw

throw new AuthorizationPluginException(
            "Fail to get ranger service name %s, exception: %s", serviceName, rse.getMessage());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the change seems more reasonable for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xunliu, could you please help confirm the original design?

Copy link
Member

Choose a reason for hiding this comment

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

I think it ok

try {
RangerService rangerService = new RangerService();
rangerService.setType(getServiceType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,45 @@ public void createCatalog() {
} catch (Exception e) {
throw new RuntimeException(e);
}

// Test to create a catalog with wrong properties which lacks Ranger service name,
// It will throw RuntimeException with message that Ranger service name is required.
Map<String, String> wrongProperties =
ImmutableMap.of(
HiveConstants.METASTORE_URIS,
HIVE_METASTORE_URIS,
IMPERSONATION_ENABLE,
"true",
AUTHORIZATION_PROVIDER,
"ranger",
RangerAuthorizationProperties.RANGER_SERVICE_TYPE,
"HadoopSQL",
RangerAuthorizationProperties.RANGER_ADMIN_URL,
RangerITEnv.RANGER_ADMIN_URL,
RangerAuthorizationProperties.RANGER_AUTH_TYPE,
RangerContainer.authType,
RangerAuthorizationProperties.RANGER_USERNAME,
RangerContainer.rangerUserName,
RangerAuthorizationProperties.RANGER_PASSWORD,
RangerContainer.rangerPassword,
RangerAuthorizationProperties.RANGER_SERVICE_CREATE_IF_ABSENT,
"true");
Comment on lines +243 to +262
Copy link
Member

Choose a reason for hiding this comment

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

Which is the wrong properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remove the service name. This service name is required.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment in the here. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


int catalogSize = metalake.listCatalogs().length;
Exception exception =
Assertions.assertThrows(
RuntimeException.class,
() ->
metalake.createCatalog(
"wrongTestProperties",
Catalog.Type.RELATIONAL,
provider,
"comment",
wrongProperties));
Assertions.assertTrue(
exception.getMessage().contains("authorization.ranger.service.name is required"));

Assertions.assertEquals(catalogSize, metalake.listCatalogs().length);
}

protected void checkTableAllPrivilegesExceptForCreating() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,16 @@
import org.apache.gravitino.exceptions.NonEmptyEntityException;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.gravitino.utils.PrincipalUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* {@code CatalogHookDispatcher} is a decorator for {@link CatalogDispatcher} that not only
* delegates catalog operations to the underlying catalog dispatcher but also executes some hook
* operations before or after the underlying operations.
*/
public class CatalogHookDispatcher implements CatalogDispatcher {
private static final Logger LOG = LoggerFactory.getLogger(CatalogHookDispatcher.class);
private final CatalogDispatcher dispatcher;

public CatalogHookDispatcher(CatalogDispatcher dispatcher) {
Expand Down Expand Up @@ -82,21 +85,26 @@ public Catalog createCatalog(

Catalog catalog = dispatcher.createCatalog(ident, type, provider, comment, properties);

// Set the creator as the owner of the catalog.
OwnerManager ownerManager = GravitinoEnv.getInstance().ownerManager();
if (ownerManager != null) {
ownerManager.setOwner(
ident.namespace().level(0),
NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.CATALOG),
PrincipalUtils.getCurrentUserName(),
Owner.Type.USER);
}
try {
// Set the creator as the owner of the catalog.
OwnerManager ownerManager = GravitinoEnv.getInstance().ownerManager();
if (ownerManager != null) {
ownerManager.setOwner(
ident.namespace().level(0),
NameIdentifierUtil.toMetadataObject(ident, Entity.EntityType.CATALOG),
PrincipalUtils.getCurrentUserName(),
Owner.Type.USER);
}

// Apply the metalake securable object privileges to authorization plugin
FutureGrantManager futureGrantManager = GravitinoEnv.getInstance().futureGrantManager();
if (futureGrantManager != null && catalog instanceof BaseCatalog) {
futureGrantManager.grantNewlyCreatedCatalog(
ident.namespace().level(0), (BaseCatalog) catalog);
// Apply the metalake securable object privileges to authorization plugin
FutureGrantManager futureGrantManager = GravitinoEnv.getInstance().futureGrantManager();
if (futureGrantManager != null && catalog instanceof BaseCatalog) {
futureGrantManager.grantNewlyCreatedCatalog(
ident.namespace().level(0), (BaseCatalog) catalog);
}
} catch (Exception e) {
LOG.warn("Fail to execute the post hook operations, rollback the catalog " + ident, e);
dispatcher.dropCatalog(ident, true);
}

return catalog;
Expand Down