Closed shohamyamin closed 1 month ago
Hello, I started looking into this and wanted to share my current though-process on possible approaches.
getColumnMask
Not including here calls from tests and calls from System/CatalogAccessControl
implementations that mostly just delegate (I didn't spend a lot of time on these, please correct me if there is more functionality to consider).
io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitInsert
io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitDelete
io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitUpdate
io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitMerge
io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitTableExecute
io.trino.sql.analyzer.StatementAnalyzer.Visitor#analyzeFiltersAndMasks
All of these iterate over the columns in a table and call getColumMask
on each column.
visitInsert/Delete/Update/Merge/TableExecute
have pretty much the same code block:
for (ColumnMetadata tableColumn : tableMetadata.getColumns()) {
if (accessControl.getColumnMask(session.toSecurityContext(), tableName, tableColumn.getName(), tableColumn.getType()).isPresent()) {
throw semanticException(NOT_SUPPORTED, node, "ALTER TABLE EXECUTE is not supported for table with column masks");
}
}
analyzeFiltersAndMasks
actually uses the returned column masks.
getColumnMask
?As discussed in the slack thread, combining the fetching all of column masks needed for a query into a single call is hard to practically impossible. I think we can still get a good increase in performance by fetching the masks for all the columns in a given table in one pass.
The signature of the bulk function could look like the following (this isn't the best approach but it gets the point across, its pretty easy to add). The default implementation would delegate to getColumnMask
.
List<Optional<ViewExpression>> getColumnMasks(SystemSecurityContext context, CatalogSchemaTableName tableName, List<Pair<String, Type>> columns);
This discussion began in the context of the OPA access control plugin, which does an HTTP call for every getColumnMask
function call.
If the plugin would receive a bulk column masks call through the API there are two main ways of resolving it:
opa.policy.batched-uri
in concept
I don't know which of these is better.
I will look into gathering some data on the performance of the current implementation and one (or both if I have time) of these "solutions".
Would love to hear the thoughts of the community on this!
cc: @vagaerg @dprophet
+1 to the bulk getColumnMask
, even if we cannot add a fully bulk mode in that we'll still get several calls for a given query, moving from one call per column to one call per table is a considerable improvement.
As for OPA, I don't have a strong opinion. However, I feel that ideally OPA would be co-located with Trino deployments and as such we can worry less about network overhead and focus more on ensuring policies remain easy to implement.
Each possible type of request that OPA needs to support adds complexity to the policies. Policy implementers currently need to consider 4 possible type of requests:
If we add a bulk column mask request, policy implementers need to consider whether they want to use the feature, and if so, implement it in a way that is performant.
While the documentation suggests that a policy implementer can use recursion within rego to turn a non-batch policy into a batch one (calling itself and using with
to evaluate itself using different objects), this is far from efficient if the request is large. The cost of adding a new type of request is not negligible so I'd suggest keeping OPA as-is for now, unless we have a clear use case where parallelizing it would not suffice.
This has been a long standing issue with the interface io.trino.spi.security.SystemAccessControl
The Ranger plugin isnt widely used but suffers from the same problem. That said, the Ranger plugin isnt widely used so its has not presented a front-center-state problem.
Now with OPA and the renewed interest in data access/control its time we went back and fundamentally fixed the flaws that cause these access-control storms.
What @vagaerg mentioned is the correct course of action.
I don't think this will work for OPA, but the access control implementation could cache on a per query basis. So when the first request comes in for they table, you just fetch all the column masks in one shot, and if the engine asks again you already have the answer.
As promised, I am back with some findings.
For these tests I added a SystemAccessControl#getBatchColumnMasks
function that gets a list of columns (all from the same tables). See mosiac1/trino
. The changes here are experimental, I will clean then up if we decide to go ahead with this approach.
Edit: Tests were done on https://github.com/mosiac1/trino/commit/ff6e2250f0dc24c97f28e4238bd68568e7f2171c, commits added after are for cleaning up and improving the APIs.
SystemAccessControl
interfaceThe current interface treat each getColumnMask
independently and when it needs to
get multiple column masks it will do it in sequence.
No. cols | Avg. getMask (ms) |
---|---|
100 | 46.1910 |
250 | 86.9650 |
1000 | 248.1755 |
SystemAccessControl
interface, with getBatchColumnMasks
, Using Parallel OPA RequestsThe interface was updated to include a getBatchColumnMasks
function that can fetch the masks of multiple
columns form a table. The trino-opa
plugin was updated to use parallel requests when fetching bulk column
masks, using the default HTTP client configuration for these tests (the thead pool size is most relevant in this case, default settings are 8 for minThreads
and 200 for maxThreads
). The Rego policy is unchanged.
No. cols | Avg. getMask (ms) |
---|---|
100 | 9.9290 |
250 | 34.0555 |
1000 | 72.1950 |
SystemAccessControl
interface, with getBatchColumnMasks
, Using 1 Request to OPAThe conditions listed above still hold. Additionally, an extra configuration key, opa.policy.batch-column-masking-uri
, is added to trino-opa
- this is used to request column masks for a list
of columns. A new rule is added to the Rego policy, batchColumnMasks := []
.
No. cols | Avg. getMask duration |
---|---|
100 | 1.8125 |
250 | 2.2990 |
1000 | 6.1945 |
The pot has the same data as above.
All tests were ran locally on my WSL instance (20GB or ram, 20 vcores). All tests are done using a Trino Development Server. A memory catalog is used, where a single table with N varchar columns is created. Tracing is enabled and all traces go to a local instance of Jaeger (docker run jaegertracing/all-in-one -p "4317:4317" -p "16686:16686"
) - this is used to get the timings. An OPA server is running locally with a simple policy that will allow all Trino requests and return no column masks.
@mosiac1 Thats looks great. Are you plan to make a pull request for that? It is a great improvement from the current state.
I think that making Updates SystemAccessControl interface, with getBatchColumnMasks, Using 1 Request to OPA
optional like it is with the other batch requests and then in the default you will get the improved version but with multiple calls for OPA unless you configure the opa.policy.batch-column-masking-uri
@shohamyamin Yes, I will open a pull request soon
@shohamyamin @dain Created the PR - https://github.com/trinodb/trino/pull/21997
@shohamyamin PR was merged, i think we can close the issue
Explanation: When applying column masking, redundant requests are triggered for every column, regardless of whether they are queried or not. This leads to unnecessary overhead as the system tries to get column masking on all table columns, even those not accessed by the query. Moreover, the current approach sends individual requests for each column, causing some performance slowdowns and unnecessary overhead for the authorization system(in my case OPA).
For example: A query for one column of a table with 250 columns will create 250 requests of getColumnMasking for opa
Proposed Solution:
Enhance Trino to execute getColumnMask only for relevant columns. Optimize Trino to make getColumnMask operation as bulk for all the relevant columns at once rather than send them individually for each column, thereby improving performance.
Relevant slack threads: https://trinodb.slack.com/archives/CGB0QHWSW/p1711991055481149 https://trinodb.slack.com/archives/CGB0QHWSW/p1711991466026649