You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "9aman (via GitHub)" <gi...@apache.org> on 2025/06/09 07:57:28 UTC
[PR] Adding changes for supporting RLS [pinot]
9aman opened a new pull request, #16043:
URL: https://github.com/apache/pinot/pull/16043
(no comment)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2159882542
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
Row level security etc. are not considered part of authorization in databases. For example in Snowflake and PG, they are called row access policy and row security policies. These are different from GRANT policies that specify authorization rules in those databases. So the standard setup is to keep it separate in the db core.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2166751512
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pinot.sql.parsers.rewriter;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.logging.log4j.util.Strings;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141920553
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,39 @@
+package org.apache.pinot.sql.parsers.rewriter;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.pinot.common.request.DataSource;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
+
+ private static final String ROW_FILTERS = "rowFilters";
+
+ @Override
+ public PinotQuery rewrite(PinotQuery pinotQuery) {
+ Map<String, String> queryOptions = pinotQuery.getQueryOptions();
+ if (!MapUtils.isEmpty(queryOptions)) {
+ String tableName = pinotQuery.getDataSource().getTableName();
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ String rowFilters = queryOptions.get(rawTableName);
+ Expression expression = CalciteSqlParser.compileToExpression(rowFilters);
+ Expression existingFilterExpression = pinotQuery.getFilterExpression();
+ if (existingFilterExpression != null) {
+ Expression combinedFilterExpression =
+ RequestUtils.getFunctionExpression(SqlKind.AND.toString(), List.of(expression, existingFilterExpression));
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2163422720
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java:
##########
@@ -0,0 +1,330 @@
+/**
+ * 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.pinot.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.client.Connection;
+import org.apache.pinot.client.ConnectionFactory;
+import org.apache.pinot.client.JsonAsyncHttpPinotClientTransportFactory;
+import org.apache.pinot.controller.helix.ControllerRequestClient;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.util.TestUtils;
+import org.junit.Assert;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
+import static org.apache.pinot.integration.tests.ClusterIntegrationTestUtils.getBrokerQueryApiUrl;
+
+
+public class RowLevelSecurityIntegrationTest extends BaseClusterIntegrationTest {
+ private static final String AUTH_TOKEN_USER_2 = "Basic dXNlcjI6bm90U29TZWNyZXQ";
+ public static final Map<String, String> AUTH_HEADER_USER_2 = Map.of("Authorization", AUTH_TOKEN_USER_2);
+ private static final String DEFAULT_TABLE_NAME_2 = "mytable2";
+ private static final String DEFAULT_TABLE_NAME_3 = "mytable3";
+
+ protected List<File> _avroFiles;
+ private static final Logger LOGGER = LoggerFactory.getLogger(RowLevelSecurityIntegrationTest.class);
+
+ @Override
+ protected void overrideControllerConf(Map<String, Object> properties) {
+ properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+ properties.put("controller.admin.access.control.factory.class",
+ "org.apache.pinot.controller.api.access.BasicAuthAccessControlFactory");
+ properties.put("controller.admin.access.control.principals", "admin, user, user2");
+ properties.put("controller.admin.access.control.principals.admin.password", "verysecret");
+ properties.put("controller.admin.access.control.principals.user.password", "secret");
+ properties.put("controller.admin.access.control.principals.user2.password", "notSoSecret");
+ properties.put("controller.admin.access.control.principals.user.tables", "mytable, mytable2, mytable3");
+ properties.put("controller.admin.access.control.principals.user.permissions", "read");
+ properties.put("controller.admin.access.control.principals.user2.tables", "mytable, mytable2, mytable3");
+ properties.put("controller.admin.access.control.principals.user2.permissions", "read");
+ }
+
+ @Override
+ protected void overrideBrokerConf(PinotConfiguration brokerConf) {
+ brokerConf.setProperty("pinot.broker.access.control.class",
+ "org.apache.pinot.broker.broker.BasicAuthAccessControlFactory");
+ brokerConf.setProperty("pinot.broker.access.control.principals", "admin, user, user2");
+ brokerConf.setProperty("pinot.broker.access.control.principals.admin.password", "verysecret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.password", "secret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.password", "notSoSecret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.tables", "mytable, mytable2, mytable3");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.permissions", "read");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.mytable.rls", "AirlineID='19805'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.mytable3.rls",
+ "AirlineID='20409' OR AirTime>'300', DestStateName='Florida'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.tables", "mytable, mytable2, mytable3");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.permissions", "read");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable.rls",
+ "AirlineID='19805', DestStateName='California'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable2.rls",
+ "AirlineID='20409', DestStateName='Florida'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable3.rls",
+ "AirlineID='20409' OR DestStateName='California', DestStateName='Florida'");
+ }
+
+ @Override
+ protected void overrideServerConf(PinotConfiguration serverConf) {
+ serverConf.setProperty("pinot.server.segment.fetcher.auth.token", AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.segment.uploader.auth.token", AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.instance.auth.token", AUTH_TOKEN);
+ }
+
+ @Override
+ public ControllerRequestClient getControllerRequestClient() {
+ if (_controllerRequestClient == null) {
+ _controllerRequestClient =
+ new ControllerRequestClient(_controllerRequestURLBuilder, getHttpClient(), AUTH_HEADER);
+ }
+ return _controllerRequestClient;
+ }
+
+ @Override
+ protected Connection getPinotConnection() {
+ if (_pinotConnection == null) {
+ JsonAsyncHttpPinotClientTransportFactory factory = new JsonAsyncHttpPinotClientTransportFactory();
+ factory.setHeaders(AUTH_HEADER);
+
+ _pinotConnection =
+ ConnectionFactory.fromZookeeper(getZkUrl() + "/" + getHelixClusterName(), factory.buildTransport());
+ }
+ return _pinotConnection;
+ }
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir, _segmentDir, _tarDir);
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+
+ startKafka();
+ _avroFiles = unpackAvroData(_tempDir);
+ pushAvroIntoKafka(_avroFiles);
+
+ // Set up a table for testing different principals.
+ setupTable(DEFAULT_TABLE_NAME);
+ setupTable(DEFAULT_TABLE_NAME_2);
+ setupTable(DEFAULT_TABLE_NAME_3);
+
+ waitForAllDocsLoaded(600_000L);
+ }
+
+ private void setupTable(String tableName)
+ throws Exception {
+
+ Schema schema = createSchema();
+ schema.setSchemaName(tableName);
+ addSchema(schema);
+
+ TableConfig tableConfig = createRealtimeTableConfig(_avroFiles.get(0));
+ tableConfig.setTableName(tableName);
+ tableConfig.getValidationConfig().setRetentionTimeUnit("DAYS");
+ tableConfig.getValidationConfig().setRetentionTimeValue("100000");
+ addTableConfig(tableConfig);
+
+ waitForDocsLoaded(600_000L, true, tableConfig.getTableName());
+ }
+
+ @AfterClass
+ public void tearDown()
+ throws IOException {
+ LOGGER.info("Tearing down...");
+ dropRealtimeTable(getTableName());
+ stopServer();
+ stopBroker();
+ stopController();
+ stopKafka();
+ stopZk();
+ FileUtils.deleteDirectory(_tempDir);
+ }
+
+ @Test
+ public void testRowFiltersForSingleStateQuery()
Review Comment:
```suggestion
public void testRowFiltersForSingleStageQuery()
```
nit: typo (in the other tests as well)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2139200737
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
The request is failed as the user doesn't have the right to access the table.
I feel returning an empty list might be a bit misleading here.
Is there concern here that this function is working both as authorize and fetch row filters ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141774043
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,20 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
Review Comment:
This is in the wrong place. This code block should be moved to `doHandleRequest`
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -135,6 +138,32 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
Review Comment:
This should be moved earlier in the function.?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +304,17 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ for (String tableName : tables) {
+ accessControl.getRowColFilters(requesterIdentity, tableName).getRLSFilters()
+ .ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
+ String key = String.format("%s-%s", CommonConstants.RLS_FILTERS, tableName);
Review Comment:
Is there a better way to store RLS filters for every table instead of mangling string keys ? @yashmayya do you have a better suggestion ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
Another option is to check using `Preconditions`.
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -31,14 +34,17 @@ public class BasicAuthPrincipal {
private final Set<String> _tables;
private final Set<String> _excludeTables;
private final Set<String> _permissions;
+ //key: table name, val: list of RLS filters applicable for that table.
+ private final Map<String, List<String>> _rlsFilters;
public BasicAuthPrincipal(String name, String token, Set<String> tables, Set<String> excludeTables,
- Set<String> permissions) {
+ Set<String> permissions, Map<String, List<String>> rlsFilters) {
Review Comment:
Can you add a constructor that does not have `rlsFilters` as a param ?
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -65,13 +71,27 @@ public boolean hasPermission(String permission) {
return _permissions.isEmpty() || _permissions.contains(permission.toLowerCase());
}
+ /**
+ * Gets the Row-Level Security (RLS) filter configured for the given table.
+ * The RLS filter is applied only if the user has access to the table
+ * (as determined by {@link #hasTable(String)}).
+ *
+ * @param tableName The name of the table.
+ * @return An {@link java.util.Optional} containing the RLS filter string if configured for this principal and table,
+ * otherwise {@link java.util.Optional#empty()}.
+ */
+ public Optional<List<String>> getRLSFilters(String tableName) {
Review Comment:
Nit: Return null instead of `Optional` ?
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -76,8 +79,26 @@ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfigura
Set<String> excludeTables = extractSet(configuration, prefix + "." + name + "." + EXCLUDE_TABLES);
Set<String> permissions = extractSet(configuration, prefix + "." + name + "." + PERMISSIONS);
+ // Extract RLS filters
Review Comment:
Can you add a unit test for this block ?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -76,19 +78,26 @@ private ServerPlanRequestUtils() {
private static final int DEFAULT_LEAF_NODE_LIMIT = Integer.MAX_VALUE;
private static final List<String> QUERY_REWRITERS_CLASS_NAMES =
ImmutableList.of(PredicateComparisonRewriter.class.getName(),
- NonAggregationGroupByToDistinctQueryRewriter.class.getName());
+ NonAggregationGroupByToDistinctQueryRewriter.class.getName(), RlsFiltersRewriter.class.getName());
private static final List<QueryRewriter> QUERY_REWRITERS =
new ArrayList<>(QueryRewriterFactory.getQueryRewriters(QUERY_REWRITERS_CLASS_NAMES));
private static final QueryOptimizer QUERY_OPTIMIZER = new QueryOptimizer();
+ public static OpChain compileLeafStage(OpChainExecutionContext executionContext, StagePlan stagePlan,
+ QueryExecutor leafQueryExecutor, ExecutorService executorService, Map<String, String> rowFilters) {
+ return compileLeafStage(executionContext, stagePlan, leafQueryExecutor, executorService,
+ (planNode, multiStageOperator) -> {
+ }, false, rowFilters);
+ }
+
public static OpChain compileLeafStage(
OpChainExecutionContext executionContext,
StagePlan stagePlan,
QueryExecutor leafQueryExecutor,
ExecutorService executorService) {
return compileLeafStage(executionContext, stagePlan, leafQueryExecutor, executorService,
(planNode, multiStageOperator) -> {
- }, false);
+ }, false, Map.of());
Review Comment:
nit: send null instead of Map.of
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141912186
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,32 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = rlsFilters.getRLSFilters();
+ if (rlsFiltersMaybe.isPresent()) {
+ Map<String, List<String>> rowFilters = rlsFiltersMaybe.get();
+ StringBuilder sb = new StringBuilder();
+ for (String policyId : rowFilters.keySet()) {
+ List<String> filters = rowFilters.get(policyId);
+ for (int i = 0; i < filters.size(); i++) {
+ sb.append(filters.get(i));
+ if (i < filters.size() - 1) {
+ sb.append(" AND ");
+ }
+ }
+ }
Review Comment:
Cleanedup. Now it's a single loop
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2135513783
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
+
+ //precondition: The principal should have the table.
+ Preconditions.checkArgument(principal.hasTable(table),
+ "Principal: " + principal.getName() + " does not have access to table: " + table);
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = principal.getRLSFilters(table);
+ Optional<Map<String, List<String>>> visibleColsMaybe = principal.getVisibleCols(table);
Review Comment:
These are out of scope ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
This should return an empty list ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ } else if (table.equals("upsertPartialMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ }
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ /**
+ * Convenience method to get RLS/CLS filters for a set of tables. By default, we iterate through each table and do
+ * the check using {@link AccessControl#getRowColFilters(RequesterIdentity, String)} and construct the final
+ * response by returning an instance of {@link MultiTableRowColAuthResult}
+ * @param requesterIdentity requester identity
+ * @param tables Set of pinot tables used in the query. Table name can be with or without tableType.
+ * @return {@link MultiTableRowColAuthResult} with the result of the access control check.
+ */
+ default MultiTableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, Set<String> tables) {
Review Comment:
This was an ongoing discussion. Its not clear to me why `MultiTableRowColAuthResult` is required. I suggest you remove it and see if it is still usable by MSE.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
Review Comment:
IMO, the results should be a list. I dont think the concept of policy is required.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -105,9 +108,9 @@ public AuthorizationResult authorize(RequesterIdentity requesterIdentity, Broker
if (!principal.hasTable(brokerRequest.getQuerySource().getTableName())) {
failedTables.add(brokerRequest.getQuerySource().getTableName());
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
Review Comment:
Is this change based on some other yet to be merged PR ?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +306,27 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ MultiTableRowColAuthResult rowColFilters = accessControl.getRowColFilters(requesterIdentity, tables);
Review Comment:
Can you check if TableRowColAuthResult can be used instead ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149080343
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -135,6 +138,32 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
Review Comment:
Is this comment for the class `BaseSingleStageBrokerRequestHandler.java`
as in similar to what is mentioned https://github.com/apache/pinot/pull/16043#discussion_r2141774043
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2142848705
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -31,14 +34,17 @@ public class BasicAuthPrincipal {
private final Set<String> _tables;
private final Set<String> _excludeTables;
private final Set<String> _permissions;
+ //key: table name, val: list of RLS filters applicable for that table.
+ private final Map<String, List<String>> _rlsFilters;
public BasicAuthPrincipal(String name, String token, Set<String> tables, Set<String> excludeTables,
- Set<String> permissions) {
+ Set<String> permissions, Map<String, List<String>> rlsFilters) {
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168127129
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +304,17 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ for (String tableName : tables) {
+ accessControl.getRowColFilters(requesterIdentity, tableName).getRLSFilters()
+ .ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
+ String key = String.format("%s-%s", CommonConstants.RLS_FILTERS, tableName);
Review Comment:
I am keeping it as such. This can help us avoid any conflicts as well with other query options.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149078853
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
Review Comment:
Marking this as resolved. Please reopen if needed.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
+
+ //precondition: The principal should have the table.
+ Preconditions.checkArgument(principal.hasTable(table),
+ "Principal: " + principal.getName() + " does not have access to table: " + table);
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = principal.getRLSFilters(table);
+ Optional<Map<String, List<String>>> visibleColsMaybe = principal.getVisibleCols(table);
Review Comment:
Marking this as resolved. Please reopen if needed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2153918416
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
@vrajat can I mark this as resolved ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2140198489
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,39 @@
+package org.apache.pinot.sql.parsers.rewriter;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.pinot.common.request.DataSource;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
+
+ private static final String ROW_FILTERS = "rowFilters";
+
+ @Override
+ public PinotQuery rewrite(PinotQuery pinotQuery) {
+ Map<String, String> queryOptions = pinotQuery.getQueryOptions();
+ if (!MapUtils.isEmpty(queryOptions)) {
+ String tableName = pinotQuery.getDataSource().getTableName();
+ String rawTableName = TableNameBuilder.extractRawTableName(tableName);
+ String rowFilters = queryOptions.get(rawTableName);
+ Expression expression = CalciteSqlParser.compileToExpression(rowFilters);
+ Expression existingFilterExpression = pinotQuery.getFilterExpression();
+ if (existingFilterExpression != null) {
+ Expression combinedFilterExpression =
+ RequestUtils.getFunctionExpression(SqlKind.AND.toString(), List.of(expression, existingFilterExpression));
Review Comment:
nit: I'd suggest using `FilterKind.AND.name()` here instead even though the string values will be identical (Pinot vs Calcite definition, since this is part of the v1 engine).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "abhishekbafna (via GitHub)" <gi...@apache.org>.
abhishekbafna commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2139252410
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,32 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = rlsFilters.getRLSFilters();
+ if (rlsFiltersMaybe.isPresent()) {
+ Map<String, List<String>> rowFilters = rlsFiltersMaybe.get();
+ StringBuilder sb = new StringBuilder();
+ for (String policyId : rowFilters.keySet()) {
+ List<String> filters = rowFilters.get(policyId);
+ for (int i = 0; i < filters.size(); i++) {
+ sb.append(filters.get(i));
+ if (i < filters.size() - 1) {
+ sb.append(" AND ");
+ }
+ }
+ }
Review Comment:
Is no separator required for out for loop (policy Id) joins?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149358714
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -76,8 +79,26 @@ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfigura
Set<String> excludeTables = extractSet(configuration, prefix + "." + name + "." + EXCLUDE_TABLES);
Set<String> permissions = extractSet(configuration, prefix + "." + name + "." + PERMISSIONS);
+ // Extract RLS filters
Review Comment:
Done.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2160942742
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +436,20 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters =
+ rowFilters.stream().map(filter -> "( " + filter + " )").collect(Collectors.joining(" AND "));
+ queryOptions.put(tableName, combinedFilters);
+ pinotQuery.setQueryOptions(queryOptions);
+ CalciteSqlParser.queryRewrite(pinotQuery);
Review Comment:
Fixed 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2166676328
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
+ /**
+ * Returns the RLS filters associated with a particular table. RLS filters are defined as a list.
+ * @return optional of the RLS filters. Empty optional is there are no RLS filters defined on this table
+ */
+ Optional<List<String>> getRLSFilters();
+
+ TableRowColAuthResult setRLSFilters(List<String> rlsFilters);
Review Comment:
Changed it to void. Thanks for pointing it out.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2162156929
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
I see. So, granting access to the tables in the DB is typically (and intentionally) kept separate from the row/col filters that apply on the data *after* a principal has been granted access to the table. In that case, it makes sense to keep them separate from the authZ interface.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2153640761
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -123,6 +126,12 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
Review Comment:
Remove ? The interface has a default implementation
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -135,6 +138,32 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
Review Comment:
No. I dont remember why I added this comment any more. Maybe because there was some code w.r.t authz ? Anyway please resolve it.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +436,19 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
Review Comment:
Can you add parentheses and a test for complex expressions ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149079272
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +306,27 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ MultiTableRowColAuthResult rowColFilters = accessControl.getRowColFilters(requesterIdentity, tables);
Review Comment:
Marking this as resolved. Please reopen if needed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168128314
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -76,8 +79,26 @@ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfigura
Set<String> excludeTables = extractSet(configuration, prefix + "." + name + "." + EXCLUDE_TABLES);
Set<String> permissions = extractSet(configuration, prefix + "." + name + "." + PERMISSIONS);
+ // Extract RLS filters
Review Comment:
Marking this as resolved
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2163372929
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pinot.sql.parsers.rewriter;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.logging.log4j.util.Strings;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
+
+ private static final String ROW_FILTERS = "rowFilters";
Review Comment:
Unused?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +437,26 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters =
+ rowFilters.stream().map(filter -> "( " + filter + " )").collect(Collectors.joining(" AND "));
+ queryOptions.put(tableName, combinedFilters);
Review Comment:
What if the table name collides with some existing query option name (possible since these aren't reserved words)?
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pinot.sql.parsers.rewriter;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.logging.log4j.util.Strings;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
Review Comment:
Can we add a Javadoc to this class describing its intended usage?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
+ /**
+ * Returns the RLS filters associated with a particular table. RLS filters are defined as a list.
+ * @return optional of the RLS filters. Empty optional is there are no RLS filters defined on this table
Review Comment:
```suggestion
* @return optional of the RLS filters. Empty optional if there are no RLS filters defined on this table
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
+ /**
+ * Returns the RLS filters associated with a particular table. RLS filters are defined as a list.
+ * @return optional of the RLS filters. Empty optional is there are no RLS filters defined on this table
+ */
+ Optional<List<String>> getRLSFilters();
+
+ TableRowColAuthResult setRLSFilters(List<String> rlsFilters);
Review Comment:
Why does this method need to return `TableRowColAuthResult`?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
Review Comment:
We're using these terms RLS / CLS in various classes but I don't see anywhere they're actually being defined? Can we add the full name (presumably row / col level security) to some of these comments?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2166742839
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
+ /**
+ * Returns the RLS filters associated with a particular table. RLS filters are defined as a list.
+ * @return optional of the RLS filters. Empty optional is there are no RLS filters defined on this table
Review Comment:
Done.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2137471818
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -105,9 +108,9 @@ public AuthorizationResult authorize(RequesterIdentity requesterIdentity, Broker
if (!principal.hasTable(brokerRequest.getQuerySource().getTableName())) {
failedTables.add(brokerRequest.getQuerySource().getTableName());
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
Review Comment:
Removing this. Was part of the previous PR raised.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2142707078
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -65,13 +72,27 @@ public boolean hasPermission(String permission) {
return _permissions.isEmpty() || _permissions.contains(permission.toLowerCase());
}
+ /**
+ * Gets the Row-Level Security (RLS) filter configured for the given table.
+ * The RLS filter is applied only if the user has access to the table
+ * (as determined by {@link #hasTable(String)}).
+ *
+ * @param tableName The name of the table.
+ * @return An {@link java.util.Optional} containing the RLS filter string if configured for this principal and table,
+ * otherwise {@link java.util.Optional#empty()}.
+ */
+ public Optional<Map<String, List<String>>> getRLSFilters(String tableName) {
+ return Optional.ofNullable(_rlsFilters.get(tableName));
+ }
+
@Override
public String toString() {
return "BasicAuthPrincipal{"
+ "_name='" + _name + '\''
+ ", _token='" + _token + '\''
+ ", _tables=" + _tables
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2142889370
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -76,19 +78,26 @@ private ServerPlanRequestUtils() {
private static final int DEFAULT_LEAF_NODE_LIMIT = Integer.MAX_VALUE;
private static final List<String> QUERY_REWRITERS_CLASS_NAMES =
ImmutableList.of(PredicateComparisonRewriter.class.getName(),
- NonAggregationGroupByToDistinctQueryRewriter.class.getName());
+ NonAggregationGroupByToDistinctQueryRewriter.class.getName(), RlsFiltersRewriter.class.getName());
private static final List<QueryRewriter> QUERY_REWRITERS =
new ArrayList<>(QueryRewriterFactory.getQueryRewriters(QUERY_REWRITERS_CLASS_NAMES));
private static final QueryOptimizer QUERY_OPTIMIZER = new QueryOptimizer();
+ public static OpChain compileLeafStage(OpChainExecutionContext executionContext, StagePlan stagePlan,
+ QueryExecutor leafQueryExecutor, ExecutorService executorService, Map<String, String> rowFilters) {
+ return compileLeafStage(executionContext, stagePlan, leafQueryExecutor, executorService,
+ (planNode, multiStageOperator) -> {
+ }, false, rowFilters);
+ }
+
public static OpChain compileLeafStage(
OpChainExecutionContext executionContext,
StagePlan stagePlan,
QueryExecutor leafQueryExecutor,
ExecutorService executorService) {
return compileLeafStage(executionContext, stagePlan, leafQueryExecutor, executorService,
(planNode, multiStageOperator) -> {
- }, false);
+ }, false, Map.of());
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "praveenc7 (via GitHub)" <gi...@apache.org>.
praveenc7 commented on PR #16043:
URL: https://github.com/apache/pinot/pull/16043#issuecomment-3353774165
@9aman Looks like a good change, given it rewrites the query. Did we see any performance overhead?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2162929581
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
@9aman This interface should have Access instead of Auth in its name to make the above point clear.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168127566
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +304,17 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ for (String tableName : tables) {
+ accessControl.getRowColFilters(requesterIdentity, tableName).getRLSFilters()
+ .ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
+ String key = String.format("%s-%s", CommonConstants.RLS_FILTERS, tableName);
Review Comment:
Marking this as resolved. If needed @yashmayya @gortiz I can take this up in another PR.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "navina (via GitHub)" <gi...@apache.org>.
navina commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2159472482
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
I don’t think we need a separate API to fetch row col filters as they are typically tied to a specific authZ policy.
You can extend the `AuthorizationResult` interface to include the filter conditions in the authZ response. For example:
```java
public interface AuthorizationResult {
/**
* Indicates whether the access is granted.
*
* @return true if access is granted, false otherwise.
*/
boolean hasAccess();
/**
* Provides the failure message if access is denied.
*
* @return A string containing the failure message if access is denied, otherwise an empty string or null.
*/
String getFailureMessage();
default List<String> rowFilters()
{ return Collections.emptyList(); }
default List<String> colFilters()
{ return Collections.emptyList(); }
}
```
Do you see a problem with such a change? The query handlers can check for these filters and take action if it is non-empty.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168138927
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
Marking this as resolved.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168138307
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
Review Comment:
Renamed it. Marking this as resolved.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya merged PR #16043:
URL: https://github.com/apache/pinot/pull/16043
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149360792
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -76,8 +79,26 @@ public static List<BasicAuthPrincipal> extractBasicAuthPrincipals(PinotConfigura
Set<String> excludeTables = extractSet(configuration, prefix + "." + name + "." + EXCLUDE_TABLES);
Set<String> permissions = extractSet(configuration, prefix + "." + name + "." + PERMISSIONS);
+ // Extract RLS filters
Review Comment:
https://github.com/apache/pinot/pull/16043/files#diff-d9167c8bead99c54777f23550795946dd648e4b251f0959ff293bd0d16b4d48cR79
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2154202186
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/ZkBasicAuthAccessControlFactory.java:
##########
@@ -123,6 +126,12 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
return new TableAuthorizationResult(failedTables);
}
+ @Override
Review Comment:
Done
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +436,19 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2139236758
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
+ throw new NotAuthorizedException("Basic");
+ }
+
+ if (table == null) {
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ TableRowColAuthResult tableRowColAuthResult = new TableRowColAuthResultImpl();
+
+ BasicAuthPrincipal principal = principalOpt.get();
+
+ //precondition: The principal should have the table.
+ Preconditions.checkArgument(principal.hasTable(table),
+ "Principal: " + principal.getName() + " does not have access to table: " + table);
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = principal.getRLSFilters(table);
+ Optional<Map<String, List<String>>> visibleColsMaybe = principal.getVisibleCols(table);
Review Comment:
Removed this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141952074
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,20 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
Review Comment:
Done.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16043:
URL: https://github.com/apache/pinot/pull/16043#issuecomment-2975692841
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/16043?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `56.25000%` with `28 lines` in your changes missing coverage. Please review.
> Project coverage is 56.33%. Comparing base [(`1a476de`)](https://app.codecov.io/gh/apache/pinot/commit/1a476dea72ebf1c15e14ded28ae60c385f71b2c3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`824e9af`)](https://app.codecov.io/gh/apache/pinot/commit/824e9afc6ff4ab716275b14288e3e15e3b908b72?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 262 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/16043?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pinot/sql/parsers/rewriter/RlsFiltersRewriter.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsql%2Fparsers%2Frewriter%2FRlsFiltersRewriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvcGFyc2Vycy9yZXdyaXRlci9SbHNGaWx0ZXJzUmV3cml0ZXIuamF2YQ==) | 44.44% | [9 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/pinot/spi/auth/TableRowColAuthResultImpl.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fauth%2FTableRowColAuthResultImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvYXV0aC9UYWJsZVJvd0NvbEF1dGhSZXN1bHRJbXBsLmphdmE=) | 0.00% | [10 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ava/org/apache/pinot/core/auth/BasicAuthUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fauth%2FBasicAuthUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9hdXRoL0Jhc2ljQXV0aFV0aWxzLmphdmE=) | 78.57% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ry/runtime/plan/server/ServerPlanRequestUtils.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2Fserver%2FServerPlanRequestUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL3NlcnZlci9TZXJ2ZXJQbGFuUmVxdWVzdFV0aWxzLmphdmE=) | 50.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../java/org/apache/pinot/query/QueryEnvironment.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2FQueryEnvironment.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvUXVlcnlFbnZpcm9ubWVudC5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/pinot/query/runtime/QueryRunner.java](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2FQueryRunner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9RdWVyeVJ1bm5lci5qYXZh) | 83.33% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/16043?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1a476de) and HEAD (824e9af). Click for more details.
>
> <details><summary>HEAD has 68 uploads less than BASE</summary>
>
>| Flag | BASE (1a476de) | HEAD (824e9af) |
>|------|------|------|
>|java-21|10|1|
>|unittests|6|1|
>|unittests1|3|1|
>|skip-bytebuffers-true|4|0|
>|temurin|15|1|
>|unittests2|3|0|
>|skip-bytebuffers-false|8|0|
>|java-11|5|0|
>|integration|9|0|
>|integration2|3|0|
>|integration1|3|0|
>|custom-integration1|3|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16043 +/- ##
============================================
- Coverage 62.90% 56.33% -6.58%
+ Complexity 1386 690 -696
============================================
Files 2867 2287 -580
Lines 163354 124995 -38359
Branches 24952 19914 -5038
============================================
- Hits 102755 70413 -32342
+ Misses 52847 48751 -4096
+ Partials 7752 5831 -1921
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <56.25%> (-6.49%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <56.25%> (-6.58%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <56.25%> (-6.57%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.33% <56.25%> (+0.51%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/16043/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/16043?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2142735397
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -129,12 +132,43 @@ public TableAuthorizationResult authorize(RequesterIdentity requesterIdentity, S
failedTables.add(table);
}
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
+// return TableAuthorizationResult.success();
+// }
return new TableAuthorizationResult(failedTables);
}
+ @Override
+ public TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ Optional<BasicAuthPrincipal> principalOpt = getPrincipalOpt(requesterIdentity);
+
+ if (principalOpt.isEmpty()) {
Review Comment:
Have added preconditions on the principal and table.
The function still checks whether the principal has access to the table or not.
I can remove this if needed.
It might be hard to debug if we don't check the access and return an empty list of filters in case the principal does not have access to the table.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "abhishekbafna (via GitHub)" <gi...@apache.org>.
abhishekbafna commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2139251464
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,32 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = rlsFilters.getRLSFilters();
+ if (rlsFiltersMaybe.isPresent()) {
+ Map<String, List<String>> rowFilters = rlsFiltersMaybe.get();
+ StringBuilder sb = new StringBuilder();
+ for (String policyId : rowFilters.keySet()) {
+ List<String> filters = rowFilters.get(policyId);
+ for (int i = 0; i < filters.size(); i++) {
Review Comment:
maybe we can use String.join()
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "abhishekbafna (via GitHub)" <gi...@apache.org>.
abhishekbafna commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2139257739
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthPrincipal.java:
##########
@@ -65,13 +72,27 @@ public boolean hasPermission(String permission) {
return _permissions.isEmpty() || _permissions.contains(permission.toLowerCase());
}
+ /**
+ * Gets the Row-Level Security (RLS) filter configured for the given table.
+ * The RLS filter is applied only if the user has access to the table
+ * (as determined by {@link #hasTable(String)}).
+ *
+ * @param tableName The name of the table.
+ * @return An {@link java.util.Optional} containing the RLS filter string if configured for this principal and table,
+ * otherwise {@link java.util.Optional#empty()}.
+ */
+ public Optional<Map<String, List<String>>> getRLSFilters(String tableName) {
+ return Optional.ofNullable(_rlsFilters.get(tableName));
+ }
+
@Override
public String toString() {
return "BasicAuthPrincipal{"
+ "_name='" + _name + '\''
+ ", _token='" + _token + '\''
+ ", _tables=" + _tables
Review Comment:
Add the separator ('\') for tables too.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2160764540
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -304,6 +304,17 @@ private void checkAuthorization(RequesterIdentity requesterIdentity, RequestCont
if (!tableAuthorizationResult.hasAccess()) {
throwTableAccessError(tableAuthorizationResult);
}
+ AccessControl accessControl = _accessControlFactory.create();
+ for (String tableName : tables) {
+ accessControl.getRowColFilters(requesterIdentity, tableName).getRLSFilters()
+ .ifPresent(rowFilters -> {
+ String combinedFilters = String.join(" AND ", rowFilters);
+ String key = String.format("%s-%s", CommonConstants.RLS_FILTERS, tableName);
Review Comment:
Yeah I'm not a huge fan of this approach either tbh, but I'm not sure if there's a cleaner way to pass on this context from the broker all the way to the leaf stage compilation in the servers without introducing a bunch of additional complexity. @gortiz any idea if there's an equally simple but cleaner way to do this?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +436,20 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters =
+ rowFilters.stream().map(filter -> "( " + filter + " )").collect(Collectors.joining(" AND "));
+ queryOptions.put(tableName, combinedFilters);
+ pinotQuery.setQueryOptions(queryOptions);
+ CalciteSqlParser.queryRewrite(pinotQuery);
Review Comment:
Why are we applying all the query rewriters multiple times here?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149078486
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ } else if (table.equals("upsertPartialMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ }
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ /**
+ * Convenience method to get RLS/CLS filters for a set of tables. By default, we iterate through each table and do
+ * the check using {@link AccessControl#getRowColFilters(RequesterIdentity, String)} and construct the final
+ * response by returning an instance of {@link MultiTableRowColAuthResult}
+ * @param requesterIdentity requester identity
+ * @param tables Set of pinot tables used in the query. Table name can be with or without tableType.
+ * @return {@link MultiTableRowColAuthResult} with the result of the access control check.
+ */
+ default MultiTableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, Set<String> tables) {
Review Comment:
Marking this as resolved. Please reopen if needed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2149078691
##########
pinot-broker/src/main/java/org/apache/pinot/broker/broker/BasicAuthAccessControlFactory.java:
##########
@@ -105,9 +108,9 @@ public AuthorizationResult authorize(RequesterIdentity requesterIdentity, Broker
if (!principal.hasTable(brokerRequest.getQuerySource().getTableName())) {
failedTables.add(brokerRequest.getQuerySource().getTableName());
}
- if (failedTables.isEmpty()) {
- return TableAuthorizationResult.success();
- }
+// if (failedTables.isEmpty()) {
Review Comment:
Marking this as resolved. Please reopen if needed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2167037940
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/RowLevelSecurityIntegrationTest.java:
##########
@@ -0,0 +1,330 @@
+/**
+ * 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.pinot.integration.tests;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import java.io.File;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.pinot.client.Connection;
+import org.apache.pinot.client.ConnectionFactory;
+import org.apache.pinot.client.JsonAsyncHttpPinotClientTransportFactory;
+import org.apache.pinot.controller.helix.ControllerRequestClient;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.data.Schema;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.util.TestUtils;
+import org.junit.Assert;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_HEADER_USER;
+import static org.apache.pinot.integration.tests.BasicAuthTestUtils.AUTH_TOKEN;
+import static org.apache.pinot.integration.tests.ClusterIntegrationTestUtils.getBrokerQueryApiUrl;
+
+
+public class RowLevelSecurityIntegrationTest extends BaseClusterIntegrationTest {
+ private static final String AUTH_TOKEN_USER_2 = "Basic dXNlcjI6bm90U29TZWNyZXQ";
+ public static final Map<String, String> AUTH_HEADER_USER_2 = Map.of("Authorization", AUTH_TOKEN_USER_2);
+ private static final String DEFAULT_TABLE_NAME_2 = "mytable2";
+ private static final String DEFAULT_TABLE_NAME_3 = "mytable3";
+
+ protected List<File> _avroFiles;
+ private static final Logger LOGGER = LoggerFactory.getLogger(RowLevelSecurityIntegrationTest.class);
+
+ @Override
+ protected void overrideControllerConf(Map<String, Object> properties) {
+ properties.put("controller.segment.fetcher.auth.token", AUTH_TOKEN);
+ properties.put("controller.admin.access.control.factory.class",
+ "org.apache.pinot.controller.api.access.BasicAuthAccessControlFactory");
+ properties.put("controller.admin.access.control.principals", "admin, user, user2");
+ properties.put("controller.admin.access.control.principals.admin.password", "verysecret");
+ properties.put("controller.admin.access.control.principals.user.password", "secret");
+ properties.put("controller.admin.access.control.principals.user2.password", "notSoSecret");
+ properties.put("controller.admin.access.control.principals.user.tables", "mytable, mytable2, mytable3");
+ properties.put("controller.admin.access.control.principals.user.permissions", "read");
+ properties.put("controller.admin.access.control.principals.user2.tables", "mytable, mytable2, mytable3");
+ properties.put("controller.admin.access.control.principals.user2.permissions", "read");
+ }
+
+ @Override
+ protected void overrideBrokerConf(PinotConfiguration brokerConf) {
+ brokerConf.setProperty("pinot.broker.access.control.class",
+ "org.apache.pinot.broker.broker.BasicAuthAccessControlFactory");
+ brokerConf.setProperty("pinot.broker.access.control.principals", "admin, user, user2");
+ brokerConf.setProperty("pinot.broker.access.control.principals.admin.password", "verysecret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.password", "secret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.password", "notSoSecret");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.tables", "mytable, mytable2, mytable3");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.permissions", "read");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.mytable.rls", "AirlineID='19805'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user.mytable3.rls",
+ "AirlineID='20409' OR AirTime>'300', DestStateName='Florida'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.tables", "mytable, mytable2, mytable3");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.permissions", "read");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable.rls",
+ "AirlineID='19805', DestStateName='California'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable2.rls",
+ "AirlineID='20409', DestStateName='Florida'");
+ brokerConf.setProperty("pinot.broker.access.control.principals.user2.mytable3.rls",
+ "AirlineID='20409' OR DestStateName='California', DestStateName='Florida'");
+ }
+
+ @Override
+ protected void overrideServerConf(PinotConfiguration serverConf) {
+ serverConf.setProperty("pinot.server.segment.fetcher.auth.token", AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.segment.uploader.auth.token", AUTH_TOKEN);
+ serverConf.setProperty("pinot.server.instance.auth.token", AUTH_TOKEN);
+ }
+
+ @Override
+ public ControllerRequestClient getControllerRequestClient() {
+ if (_controllerRequestClient == null) {
+ _controllerRequestClient =
+ new ControllerRequestClient(_controllerRequestURLBuilder, getHttpClient(), AUTH_HEADER);
+ }
+ return _controllerRequestClient;
+ }
+
+ @Override
+ protected Connection getPinotConnection() {
+ if (_pinotConnection == null) {
+ JsonAsyncHttpPinotClientTransportFactory factory = new JsonAsyncHttpPinotClientTransportFactory();
+ factory.setHeaders(AUTH_HEADER);
+
+ _pinotConnection =
+ ConnectionFactory.fromZookeeper(getZkUrl() + "/" + getHelixClusterName(), factory.buildTransport());
+ }
+ return _pinotConnection;
+ }
+
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir, _segmentDir, _tarDir);
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+
+ startKafka();
+ _avroFiles = unpackAvroData(_tempDir);
+ pushAvroIntoKafka(_avroFiles);
+
+ // Set up a table for testing different principals.
+ setupTable(DEFAULT_TABLE_NAME);
+ setupTable(DEFAULT_TABLE_NAME_2);
+ setupTable(DEFAULT_TABLE_NAME_3);
+
+ waitForAllDocsLoaded(600_000L);
+ }
+
+ private void setupTable(String tableName)
+ throws Exception {
+
+ Schema schema = createSchema();
+ schema.setSchemaName(tableName);
+ addSchema(schema);
+
+ TableConfig tableConfig = createRealtimeTableConfig(_avroFiles.get(0));
+ tableConfig.setTableName(tableName);
+ tableConfig.getValidationConfig().setRetentionTimeUnit("DAYS");
+ tableConfig.getValidationConfig().setRetentionTimeValue("100000");
+ addTableConfig(tableConfig);
+
+ waitForDocsLoaded(600_000L, true, tableConfig.getTableName());
+ }
+
+ @AfterClass
+ public void tearDown()
+ throws IOException {
+ LOGGER.info("Tearing down...");
+ dropRealtimeTable(getTableName());
+ stopServer();
+ stopBroker();
+ stopController();
+ stopKafka();
+ stopZk();
+ FileUtils.deleteDirectory(_tempDir);
+ }
+
+ @Test
+ public void testRowFiltersForSingleStateQuery()
Review Comment:
Fxied.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2167033870
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -434,6 +437,26 @@ protected BrokerResponse doHandleRequest(long requestId, String query, SqlNodeAn
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ rlsFilters.getRLSFilters().ifPresent(rowFilters -> {
+ String combinedFilters =
+ rowFilters.stream().map(filter -> "( " + filter + " )").collect(Collectors.joining(" AND "));
+ queryOptions.put(tableName, combinedFilters);
Review Comment:
Fixed. Have added a prefix and then used the prefixed table name in the query rewriter.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2166744348
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/rewriter/RlsFiltersRewriter.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pinot.sql.parsers.rewriter;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.collections4.MapUtils;
+import org.apache.logging.log4j.util.Strings;
+import org.apache.pinot.common.request.Expression;
+import org.apache.pinot.common.request.PinotQuery;
+import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.apache.pinot.sql.FilterKind;
+import org.apache.pinot.sql.parsers.CalciteSqlParser;
+
+
+public class RlsFiltersRewriter implements QueryRewriter {
+
+ private static final String ROW_FILTERS = "rowFilters";
Review Comment:
Cleaned it up
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2162930542
##########
pinot-spi/src/main/java/org/apache/pinot/spi/auth/TableRowColAuthResult.java:
##########
@@ -0,0 +1,36 @@
+/**
+ * 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.pinot.spi.auth;
+
+import java.util.List;
+import java.util.Optional;
+
+
+/**
+ * This interfaces carries the RLS/CLS filter for a particular table
+ */
+public interface TableRowColAuthResult {
Review Comment:
Can you rename this class to `TableRowColAccessResult` ? This change will make it clear that it determines access and not authorization.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168062824
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
Review Comment:
Have added the java docs in the RLSRewriter and added defined these terms there.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168138627
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
Done.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +122,15 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
Review Comment:
Done. Have renamed 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168139870
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -114,6 +123,11 @@ public static OpChain compileLeafStage(
// 2. Convert PinotQuery into InstanceRequest list (one for each physical table)
PinotQuery pinotQuery = serverContext.getPinotQuery();
pinotQuery.setExplain(explain);
+
+ if (MapUtils.isNotEmpty(rowFilters)) {
+ pinotQuery.setQueryOptions(rowFilters);
+ }
Review Comment:
Keeping this as such for now. I aim to add few more improvements.
I can take this up with those.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2168140452
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -114,6 +123,11 @@ public static OpChain compileLeafStage(
// 2. Convert PinotQuery into InstanceRequest list (one for each physical table)
PinotQuery pinotQuery = serverContext.getPinotQuery();
pinotQuery.setExplain(explain);
+
+ if (MapUtils.isNotEmpty(rowFilters)) {
+ pinotQuery.setQueryOptions(rowFilters);
+ }
Review Comment:
Marking this as resolved in reference to this PR. Will take it up in the next set of improvements.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141911530
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -919,6 +922,32 @@ private CompileResult compileRequest(long requestId, String query, SqlNodeAndOpt
throwAccessDeniedError(requestId, query, requestContext, tableName, authorizationResult);
}
+ //get RLS/CLS filters now
+ TableRowColAuthResult rlsFilters = accessControl.getRowColFilters(requesterIdentity, tableName);
+
+ //rewrite query
+ Map<String, String> queryOptions =
+ pinotQuery.getQueryOptions() == null ? new HashMap<>() : pinotQuery.getQueryOptions();
+
+ Optional<Map<String, List<String>>> rlsFiltersMaybe = rlsFilters.getRLSFilters();
+ if (rlsFiltersMaybe.isPresent()) {
+ Map<String, List<String>> rowFilters = rlsFiltersMaybe.get();
+ StringBuilder sb = new StringBuilder();
+ for (String policyId : rowFilters.keySet()) {
+ List<String> filters = rowFilters.get(policyId);
+ for (int i = 0; i < filters.size(); i++) {
Review Comment:
Resolved this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2140142627
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java:
##########
@@ -114,6 +123,11 @@ public static OpChain compileLeafStage(
// 2. Convert PinotQuery into InstanceRequest list (one for each physical table)
PinotQuery pinotQuery = serverContext.getPinotQuery();
pinotQuery.setExplain(explain);
+
+ if (MapUtils.isNotEmpty(rowFilters)) {
+ pinotQuery.setQueryOptions(rowFilters);
+ }
Review Comment:
Can this be consolidated within the `updateQueryOptions` method that is called when converting the `PinotQuery` into an `InstanceRequest` so that we avoid having multiple different places where the query options are set (eliminates the risk of accidentally overwriting query options)?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141950515
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
Review Comment:
Changed it to a list
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2141951090
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java:
##########
@@ -120,4 +128,39 @@ default TableAuthorizationResult authorize(RequesterIdentity requesterIdentity,
return hasAccess(requesterIdentity, tables) ? TableAuthorizationResult.success()
: new TableAuthorizationResult(tables);
}
+
+
+ /**
+ * Returns RLS/CLS filters for a particular table. By default, there are no RLS/CLS filters on any table.
+ * @param requesterIdentity requested identity
+ * @param table Table used in the query. Table name can be with or without tableType.
+ * @return {@link TableRowColAuthResult} with the result of the access control check
+ */
+ default TableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, String table) {
+ if (table.equals("upsertMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ } else if (table.equals("upsertPartialMeetupRsvp")) {
+ return new TableRowColAuthResultImpl(Map.of("policyID1", List.of("event_id > 60", "event_id < 70")), Map.of(),
+ Map.of());
+ }
+ return TableRowColAuthResultImpl.unrestricted();
+ }
+
+ /**
+ * Convenience method to get RLS/CLS filters for a set of tables. By default, we iterate through each table and do
+ * the check using {@link AccessControl#getRowColFilters(RequesterIdentity, String)} and construct the final
+ * response by returning an instance of {@link MultiTableRowColAuthResult}
+ * @param requesterIdentity requester identity
+ * @param tables Set of pinot tables used in the query. Table name can be with or without tableType.
+ * @return {@link MultiTableRowColAuthResult} with the result of the access control check.
+ */
+ default MultiTableRowColAuthResult getRowColFilters(RequesterIdentity requesterIdentity, Set<String> tables) {
Review Comment:
removed it and relied on TableRowColAuthResult in MSQE as well.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Adding changes for supporting RLS [pinot]
Posted by "9aman (via GitHub)" <gi...@apache.org>.
9aman commented on code in PR #16043:
URL: https://github.com/apache/pinot/pull/16043#discussion_r2142858076
##########
pinot-core/src/main/java/org/apache/pinot/core/auth/BasicAuthUtils.java:
##########
@@ -101,9 +122,10 @@ public static List<ZkBasicAuthPrincipal> extractBasicAuthPrincipals(List<UserCon
.orElseGet(() -> Collections.emptyList())
.stream().map(x -> x.toString())
.collect(Collectors.toSet());
+ //todo: Handle RLS filters properly
return new ZkBasicAuthPrincipal(name,
- org.apache.pinot.common.auth.BasicAuthUtils.toBasicAuthToken(name, password), password,
- component, role, tables, excludeTables, permissions);
+ org.apache.pinot.common.auth.BasicAuthUtils.toBasicAuthToken(name, password), password, component, role,
Review Comment:
TODO: This is still not implemented.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org