This is an automated email from the ASF dual-hosted git repository. michaelsmith pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 4255926b126039fad81c3f1107f2b94c3846c9d2 Author: Fang-Yu Rao <[email protected]> AuthorDate: Wed Nov 29 13:38:16 2023 -0800 IMPALA-12554: Create one Ranger policy for multi-column GRANT This patch makes Impala create only one Ranger policy for the GRANT statement when there are multiple columns specified to reduce the number of policies created on the Ranger server. Note that this patch relies on RANGER-4585 and RANGER-4638. Testing: - Manually verified that Impala's catalog daemon only sends one GrantRevokeRequest to the Ranger plug-in and that the value of the key 'column' is a comma-separated list of column names involved in the GRANT statement. - Added an end-to-end test to verify only one Ranger policy will be created in a multi-column GRANT statement. Change-Id: I2b0ebba256c7135b4b0d2160856202292d720c6d Reviewed-on: http://gerrit.cloudera.org:8080/21940 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- .../ranger/RangerCatalogdAuthorizationManager.java | 59 +++++++++++++++++- .../ranger/RangerImpalaResourceBuilder.java | 2 + tests/authorization/test_ranger.py | 70 +++++++++++++++++++++- 3 files changed, 128 insertions(+), 3 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java index 809f2928b..3c9a02d59 100644 --- a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java +++ b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java @@ -28,6 +28,7 @@ import org.apache.impala.catalog.AuthzCacheInvalidation; import org.apache.impala.catalog.CatalogServiceCatalog; import org.apache.impala.common.ImpalaException; import org.apache.impala.common.InternalException; +import org.apache.impala.common.Pair; import org.apache.impala.common.UnsupportedFeatureException; import org.apache.impala.thrift.TCatalogServiceRequestHeader; import org.apache.impala.thrift.TCreateDropRoleParams; @@ -49,7 +50,9 @@ import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; @@ -308,6 +311,7 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager @VisibleForTesting public void grantPrivilege(List<GrantRevokeRequest> requests, String sqlStmt, String clientIp) throws ImpalaException { + long startTime = System.currentTimeMillis(); try { for (GrantRevokeRequest request : requests) { try (AutoFlush auditHandler = RangerBufferAuditHandler.autoFlush(sqlStmt, @@ -319,12 +323,16 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager LOG.error("Error granting a privilege in Ranger: ", e); throw new InternalException("Error granting a privilege in Ranger. " + "Ranger error message: " + e.getMessage()); + } finally { + LOG.debug("Handling granting privilege(s) took {} ms", + (System.currentTimeMillis() - startTime)); } } @VisibleForTesting public void revokePrivilege(List<GrantRevokeRequest> requests, String sqlStmt, String clientIp) throws ImpalaException { + long startTime = System.currentTimeMillis(); try { for (GrantRevokeRequest request : requests) { try (AutoFlush auditHandler = RangerBufferAuditHandler.autoFlush(sqlStmt, @@ -336,6 +344,9 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager LOG.error("Error revoking a privilege in Ranger: ", e); throw new InternalException("Error revoking a privilege in Ranger. " + "Ranger error message: " + e.getMessage()); + } finally { + LOG.debug("Handling revoking privilege(s) took {} ms", + (System.currentTimeMillis() - startTime)); } } @@ -419,7 +430,7 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager } } - return requests; + return consolidateGrantRevokeRequests(requests); } private static GrantRevokeRequest createGrantRevokeRequest(String grantor, String user, @@ -480,6 +491,52 @@ public class RangerCatalogdAuthorizationManager implements AuthorizationManager return request; } + /** + * This method combines GrantRevokeRequest's for specified columns in the same table + * into one GrantRevokeRequest to reduce the number of Ranger policies created on the + * Ranger server. This method implicitly assumes that for those GrantRevokeRequest's + * with specified columns on the given List, the fields other than database, table, and + * column names are the same. + */ + private static List<GrantRevokeRequest> consolidateGrantRevokeRequests( + List<GrantRevokeRequest> requests) { + List<GrantRevokeRequest> combinedRequests = new LinkedList<>(); + Map<Pair<String, String>, GrantRevokeRequest> consolidatedColumnRequests = + new HashMap<>(); + List<GrantRevokeRequest> unconsolidatedRequests = new LinkedList<>(); + + for (GrantRevokeRequest request : requests) { + Map<String, String> resource = request.getResource(); + String column = resource.get(RangerImpalaResourceBuilder.COLUMN); + if (column != null && + !column.equals(RangerImpalaResourceBuilder.WILDCARD_ASTERISK)) { + // If 'column' is the name of a specified column, the database name and the table + // name must have been specified too. + String database = resource.get(RangerImpalaResourceBuilder.DATABASE); + String table = resource.get(RangerImpalaResourceBuilder.TABLE); + Pair<String, String> key = new Pair<>(database, table); + if (!consolidatedColumnRequests.containsKey(key)) { + consolidatedColumnRequests.put(key, request); + } else { + // Combine 'column' into the GrantRevokeRequest for other specified columns in + // the same table. + Map<String, String> consolidatedResource = + consolidatedColumnRequests.get(key).getResource(); + String consolidatedColumns = consolidatedResource + .get(RangerImpalaResourceBuilder.COLUMN); + consolidatedResource.put(RangerImpalaResourceBuilder.COLUMN, + consolidatedColumns + RangerImpalaResourceBuilder.COLUMN_SEP + column); + } + } else { + unconsolidatedRequests.add(request); + } + } + + combinedRequests.addAll(consolidatedColumnRequests.values()); + combinedRequests.addAll(unconsolidatedRequests); + return combinedRequests; + } + /** * The caller of this method calls Ranger's REST API to grant/revoke roles * corresponding to 'targetRoleNames' to/from groups associated with 'groupNames'. diff --git a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaResourceBuilder.java b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaResourceBuilder.java index e743c0c5f..ce98b65e9 100644 --- a/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaResourceBuilder.java +++ b/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpalaResourceBuilder.java @@ -27,10 +27,12 @@ public class RangerImpalaResourceBuilder { public static final String DATABASE = "database"; public static final String TABLE = "table"; public static final String COLUMN = "column"; + public static final String COLUMN_SEP = ","; public static final String UDF = "udf"; public static final String URL = "url"; public static final String STORAGE_TYPE = "storage-type"; public static final String STORAGE_URL = "storage-url"; + public static final String WILDCARD_ASTERISK = "*"; private final RangerAccessResourceImpl rangerAccessResource = new RangerAccessResourceImpl(); diff --git a/tests/authorization/test_ranger.py b/tests/authorization/test_ranger.py index c2f06cb44..51f78fa64 100644 --- a/tests/authorization/test_ranger.py +++ b/tests/authorization/test_ranger.py @@ -1051,7 +1051,7 @@ class TestRanger(CustomClusterTestSuite): @staticmethod def _get_ranger_privileges_db(user, db): - policies = TestRanger._get_ranger_privileges(user) + policies = TestRanger._get_ranger_privileges() result = [] for policy in policies: @@ -1065,7 +1065,7 @@ class TestRanger(CustomClusterTestSuite): return result @staticmethod - def _get_ranger_privileges(user): + def _get_ranger_privileges(): r = requests.get("{0}/service/plugins/policies" .format(RANGER_HOST), auth=RANGER_AUTH, headers=REST_HEADERS) @@ -1271,6 +1271,72 @@ class TestRanger(CustomClusterTestSuite): impala_client, query, user=username, query_options={'sync_ddl': 1}) return self.execute_query_expect_failure(impala_client, query, user=username) + @pytest.mark.execute_serially + @CustomClusterTestSuite.with_args( + impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS, reset_ranger=True) + def test_grant_multiple_columns(self): + admin_client = self.create_impala_client() + access_type = "select" + db = "functional" + tbl = "alltypes" + cols = ["id", "bool_col", "tinyint_col", "smallint_col", "int_col", "bigint_col", + "float_col", "double_col", "date_string_col", "string_col", "timestamp_col", + "year", "month"] + cols_str = "" + for col in cols: + if not cols_str: + cols_str = col + else: + cols_str = cols_str + ", " + col + test_data = [("user", "non_owner", "users"), ("group", "non_owner", "groups"), + ("role", "test_role", "roles")] + + for data in test_data: + kw = data[0] + principal_name = data[1] + principal_key = data[2] + try: + policy_ids = set() + if kw == "role": + admin_client.execute("create role {0}".format(principal_name), user=ADMIN) + admin_client.execute("grant {0}({1}) on table {2}.{3} to {4} {5}" + .format(access_type, cols_str, db, tbl, kw, principal_name), user=ADMIN) + policies = TestRanger._get_ranger_privileges() + for col in cols: + policy_ids = policy_ids \ + .union(TestRanger._get_ranger_policy_ids(policies, principal_name, + principal_key, db, tbl, col, access_type)) + # After the GRANT statement above, there should be only one single Ranger policy + # that grants the privilege of 'access_type' on the column 'db'.'tbl'.'col' to + # the principal 'principal_name' for each column in 'cols'. + assert len(policy_ids) == 1 + finally: + admin_client.execute("revoke {0}({1}) on table {2}.{3} from {4} {5}" + .format(access_type, cols_str, db, tbl, kw, principal_name), user=ADMIN) + if kw == "role": + admin_client.execute("drop role {0}".format(principal_name), user=ADMIN) + + @staticmethod + def _get_ranger_policy_ids(policies, principal_name, principal_key, db, tbl, col, + access_type): + """Returns the set of Ranger policy id's that grant the privilege of 'access_type' on + the column 'db'.'tbl'.'col'. to the principal 'principal_name'.""" + result = set() + + for policy in policies: + id = policy["id"] + resources = policy["resources"] + if "database" in resources and db in resources["database"]["values"] \ + and "table" in resources and tbl in resources["table"]["values"] \ + and "column" in resources and col in resources["column"]["values"]: + for policy_items in policy["policyItems"]: + if principal_name in policy_items[principal_key]: + for access in policy_items["accesses"]: + if access_type in access["type"] and access["isAllowed"] is True: + result.add(id) + break + return result + @pytest.mark.execute_serially @CustomClusterTestSuite.with_args( impalad_args=IMPALAD_ARGS, catalogd_args=CATALOGD_ARGS)
