abhishekmauryaKsolves commented on PR #70647:
URL: https://github.com/apache/airflow/pull/70647#issuecomment-5121832114
Thanks for the pointer to _is_batch_authorized — I've refactored
batch_is_authorized_connection, batch_is_authorized_dag,
batch_is_authorized_pool, batch_is_authorized_variable,
filter_authorized_connections, filter_authorized_pools, and
filter_authorized_variables to use it. This reduces each of these to a single
Keycloak call instead of N parallel calls.
A few things worth flagging explicitly since they change behavior:
1. filter_authorized_* is now all-or-nothing. Since resource_id isn't used
for policy enforcement (per your earlier comment), the batch permission check
happens at the resource-type level, not per-item. That means
filter_authorized_connections, filter_authorized_pools, and
filter_authorized_variables now return either the full input set or an empty
set — they can no longer do true per-item filtering (e.g. "user has access to
conn_0 and conn_2 but not conn_1" is no longer expressible). I've updated the
unit tests to reflect this, but wanted to confirm this is the
intended/acceptable tradeoff before this merges, since it's a real functional
change vs. the previous per-item behavior.
2. dag_entity attribute dropped from batch_is_authorized_dag. The old
is_authorized_dag-based implementation passed attributes={"dag_entity": ...}
per-request. The new batch call doesn't currently support per-item attributes,
so this is dropped. I've marked the corresponding test as xfail for now — let
me know if this attribute needs to be preserved (would mean extending
_get_batch_payload/_is_batch_authorized to accept per-item attributes) or if
it's safe to drop, same as resource_id.
3. Caching removed. The single_flight cache-based dedup that existed in the
old filter_authorized_connections/pools/variables is gone in the batch
versions, since there's now one call per invocation instead of N. I dropped the
3 cache-hit tests that assumed this caching existed — happy to add it back
around the batch call if it's still wanted.
Let me know your thoughts on 1 and 2 in particular — I can adjust the
implementation depending on which tradeoffs are acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
