bharos commented on code in PR #10704:
URL: https://github.com/apache/gravitino/pull/10704#discussion_r3089285241
##########
docs/iceberg-rest-service.md:
##########
@@ -130,12 +130,16 @@ If you are using multiple JDBC catalog backends, setting
`jdbc-initialize` to tr
Use the REST backend to proxy another Iceberg REST catalog server (IRC2). The
Gravitino Iceberg REST service acts as IRC1 and forwards catalog operations to
IRC2.
-| Configuration item | Description
| Default value | Required | Since Version |
-|------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------|---------------|----------|---------------|
-| `gravitino.iceberg-rest.catalog-backend` | The Catalog backend of the
Gravitino Iceberg REST catalog service. Use the value **`rest`** for the REST
catalog backend. | `memory` | Yes | 0.2.0 |
-| `gravitino.iceberg-rest.uri` | The Iceberg REST catalog URI
(IRC2), such as `http://127.0.0.1:9001/iceberg`.
| (none) | Yes | 0.2.0 |
-| `gravitino.iceberg-rest.warehouse` | The catalog name in the Iceberg
REST spec. Set to a specific catalog name, or leave empty to use the default
catalog on IRC2. | (none) | No | 0.2.0 |
-| `gravitino.iceberg-rest.data-access` | Data access mode exposed to
Iceberg REST clients via `/v1/config`. Supported values: `vended-credentials`,
`remote-signing`. | (none) | No | 1.3.0 |
+By default, when the backend catalog is a REST catalog, IRC1 skips
authorization and behaves as a proxy. IRC2 handles authorization. If you want
IRC1 to keep authorization checks, set
`gravitino.iceberg-rest.skip-authorization-for-rest-backend=false`.
+
+
Review Comment:
nit : remove one blank line
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java:
##########
@@ -131,4 +133,27 @@ protected Optional<AuthorizationHandler>
createAuthorizationHandler(
protected boolean isExceptionPropagate(Exception e) {
return e.getClass().getName().startsWith("org.apache.iceberg.exceptions");
}
+
+ @Override
+ protected boolean shouldSkipAuthorization(Map<EntityType, NameIdentifier>
nameIdentifierMap) {
+ if
(!IcebergRESTServerContext.getInstance().skipAuthorizationForRestBackend()) {
+ return false;
+ }
+
+ NameIdentifier catalogId = nameIdentifierMap.get(EntityType.CATALOG);
+ if (catalogId == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapperManager wrapperManager =
+ IcebergRESTServerContext.getInstance().catalogWrapperManager();
+ if (wrapperManager == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapper catalogWrapper =
wrapperManager.getCatalogWrapper(catalogId.name());
+ // When IRC2 is another Gravitino server, IRC1 acts as a proxy and does
not perform
+ // authorization. IRC2 handles authorization.
+ return catalogWrapper != null && catalogWrapper.getCatalog() instanceof
RESTCatalog;
Review Comment:
can we use
catalogWrapper.isRESTCatalog() here
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java:
##########
@@ -131,4 +133,27 @@ protected Optional<AuthorizationHandler>
createAuthorizationHandler(
protected boolean isExceptionPropagate(Exception e) {
return e.getClass().getName().startsWith("org.apache.iceberg.exceptions");
}
+
+ @Override
+ protected boolean shouldSkipAuthorization(Map<EntityType, NameIdentifier>
nameIdentifierMap) {
+ if
(!IcebergRESTServerContext.getInstance().skipAuthorizationForRestBackend()) {
+ return false;
+ }
+
+ NameIdentifier catalogId = nameIdentifierMap.get(EntityType.CATALOG);
+ if (catalogId == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapperManager wrapperManager =
+ IcebergRESTServerContext.getInstance().catalogWrapperManager();
+ if (wrapperManager == null) {
+ return false;
+ }
+
+ IcebergCatalogWrapper catalogWrapper =
wrapperManager.getCatalogWrapper(catalogId.name());
+ // When IRC2 is another Gravitino server, IRC1 acts as a proxy and does
not perform
+ // authorization. IRC2 handles authorization.
+ return catalogWrapper != null && catalogWrapper.getCatalog() instanceof
RESTCatalog;
Review Comment:
nit : it never throws null here , as
IcebergCatalogWrapperManager::createCatalogWrapper either returns a value or
throws NoSuchCatalogException/ IllegalArgumentException.
So potentially we could skip this null check, but it's ok to keep anyways
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/server/web/filter/IcebergMetadataAuthorizationMethodInterceptor.java:
##########
@@ -26,13 +26,16 @@
import org.apache.gravitino.Entity;
import org.apache.gravitino.Entity.EntityType;
import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.iceberg.common.ops.IcebergCatalogWrapper;
+import org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
import
org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext;
import
org.apache.gravitino.server.authorization.annotations.AuthorizationMetadata;
import
org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata;
import
org.apache.gravitino.server.authorization.annotations.IcebergAuthorizationMetadata.RequestType;
import org.apache.gravitino.utils.NameIdentifierUtil;
import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.rest.RESTCatalog;
Review Comment:
As per below if we use catalogWrapper.isRESTCatalog(), we can remove this
import
--
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]
