jerryshao commented on code in PR #13778: URL: https://github.com/apache/gravitino/pull/13778#discussion_r4374962676
########## iceberg/iceberg-rest-server-experiment/src/main/java/org/apache/gravitino/iceberg/service/restriction/ExperimentalIcebergReadRestrictionLoader.java: ########## @@ -0,0 +1,306 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.iceberg.service.restriction; + +import java.security.Principal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.apache.gravitino.Entity; +import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.MetadataObject; +import org.apache.gravitino.NameIdentifier; +import org.apache.gravitino.UserGroup; +import org.apache.gravitino.UserPrincipal; +import org.apache.gravitino.authorization.AccessControlDispatcher; +import org.apache.gravitino.exceptions.NoSuchGroupException; +import org.apache.gravitino.iceberg.common.utils.IcebergIdentifierUtils; +import org.apache.gravitino.iceberg.service.authorization.IcebergRESTServerContext; +import org.apache.gravitino.listener.api.event.IcebergRequestContext; +import org.apache.gravitino.meta.PolicyEntity; +import org.apache.gravitino.policy.ColumnMaskContent; +import org.apache.gravitino.policy.Policy; +import org.apache.gravitino.policy.PolicyDispatcher; +import org.apache.gravitino.policy.RowFilterContent; +import org.apache.gravitino.policy.rego.RestrictedRegoExpressionParserFacade; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RowFilter; +import org.apache.gravitino.utils.HierarchicalSchemaUtil; +import org.apache.gravitino.utils.NameIdentifierUtil; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.iceberg.Schema; +import org.apache.iceberg.TableMetadata; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.rest.responses.LoadTableResponse; +import org.apache.iceberg.rest.restrictions.ReadRestrictions; +import org.apache.iceberg.rest.restrictions.ReadRestrictionsParser; +import org.apache.iceberg.types.Types; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Loads tag-selected Gravitino policies into experimental Iceberg read restrictions. */ +public final class ExperimentalIcebergReadRestrictionLoader + implements IcebergReadRestrictionLoader { + private static final Logger LOG = + LoggerFactory.getLogger(ExperimentalIcebergReadRestrictionLoader.class); + + /** Creates the service-loaded read-restriction loader. */ + public ExperimentalIcebergReadRestrictionLoader() {} + + @Override + public boolean enabled() { + return GravitinoEnv.getInstance().internalPolicyDispatcher() != null; + } + + @Override + public Result load( + IcebergRequestContext context, TableIdentifier tableIdentifier, LoadTableResponse response) { + GravitinoEnv environment = GravitinoEnv.getInstance(); + PolicyDispatcher policyDispatcher = environment.internalPolicyDispatcher(); + if (policyDispatcher == null) { + return Result.of(response, Optional.empty()); + } + + String metalake = IcebergRESTServerContext.getInstance().metalakeName(); + NameIdentifier tableName = + IcebergIdentifierUtils.toGravitinoTableIdentifier( + metalake, + context.catalogName(), + tableIdentifier, + HierarchicalSchemaUtil.schemaSeparator()); + NameIdentifierUtil.checkMetadataObjectName( + NameIdentifierUtil.ofMetalake(metalake), Entity.EntityType.METALAKE); + NameIdentifierUtil.checkMetadataObjectName( + NameIdentifierUtil.ofCatalog(metalake, context.catalogName()), Entity.EntityType.CATALOG); + NameIdentifierUtil.checkMetadataObjectName( + NameIdentifierUtil.ofSchema( + metalake, context.catalogName(), tableName.namespace().level(2)), + Entity.EntityType.SCHEMA); + Schema schema = response.tableMetadata().schema(); + PolicyEntity[] rowPolicies = + policyDispatcher.listPolicyInfosForMetadataObject( + metalake, NameIdentifierUtil.toMetadataObject(tableName, Entity.EntityType.TABLE)); + Map<Integer, PolicyEntity[]> maskPolicies = new LinkedHashMap<>(); + for (Types.NestedField field : schema.columns()) { + NameIdentifier columnName = + NameIdentifierUtil.ofColumn( + metalake, + context.catalogName(), + tableName.namespace().level(2), + tableName.name(), + field.name()); + MetadataObject column = + NameIdentifierUtil.toMetadataObject(columnName, Entity.EntityType.COLUMN); + maskPolicies.put( + field.fieldId(), policyDispatcher.listPolicyInfosForMetadataObject(metalake, column)); + } + + IdentityBindingContext bindingContext = + new IdentityBindingContext( + context.userName(), metalake, environment.internalAccessControlDispatcher()); Review Comment: [Important] Group-based restrictions can never resolve on the default server configuration, so load-table returns HTTP 500 for every reader of a governed table. `enabled()` and the early return key only on `internalPolicyDispatcher()` (lines 72-75 and 80-84), which `GravitinoEnv` creates unconditionally (`core/src/main/java/org/apache/gravitino/GravitinoEnv.java:937`). The `internalAccessControlDispatcher()` passed into `IdentityBindingContext` on this line is null whenever `gravitino.authorization.enable` is false (`GravitinoEnv.java:1030-1038`), and that flag defaults to **false** (`core/src/main/java/org/apache/gravitino/Configs.java:390-395`). With it null, `resolveGroupMembership` returns `Optional.empty()` (line 274), `ResolutionContext.groupMembership` converts an empty result into `IllegalArgumentException("Cannot resolve group membership for the applicable read restriction")` (`IcebergReadRestrictionConverter.java:753-764`), and `resolveForLoad` wraps that into `IllegalStateException` (lines 160-167), which `IcebergExceptionMapper.getErrorCode` does not map - a bare HTTP 500. So on a default-config deployment, any row filter or column mask containing `is_group_member(...)` makes every load of that table fail for every reader, including the two examples a reader will copy straight out of the docs (`docs/policies.md:95` and `docs/policies.md:102`). Failing closed is right when membership is genuinely unknowable, but here it is a deployment prerequisite that nothing states, nothing checks and no test covers. Suggestion: detect the missing dispatcher explicitly rather than letting it look like an unresolvable group. Either fail with a message that names the required configuration, or require it in `enabled()`/`load()` the way the policy dispatcher is required - and either way add the prerequisite to the new limitations paragraph in `docs/policies.md`, next to the dotted-name note. Verified by: reading the null path at line 274 through `ResolutionContext.groupMembership` and `resolveForLoad` in this checkout; `internalAccessControlDispatcher` is assigned null in the else branch of `initInternalAuthorizationComponents`, while `internalPolicyDispatcher` is set outside that branch; `grep` over the experiment module's tests shows `IdentityBindingContext` is only ever built with a Mockito mock dispatcher. I did not run a server to reproduce it. -- 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]
