citus
citus copied to clipboard
Adds support for granted by propagation for grant statements
DESCRIPTION:Improving Support for Granted By Propagation in Grant Statements
In previous pull requests (#7451 and #7517), we introduced support for the "granted by" statement. However, a subsequent issue, #7519, emerged regarding the metadata synchronization of "granted by" statements. Consequently, we needed to remove tests related to "granted by" statements in PR #7526.
The primary objective of this pull request is to enhance the overall support for "granted by" statements. Initially aimed at resolving metadata synchronization issues, further testing uncovered additional problems, all of which are addressed in this pull request.
Example SQL Statement:
sql grant granted_role1, non_dist_granted_role1 to grantee_role1, non_dist_grantee_role1 with admin option granted by non_dist_grantor;
Issues Addressed:
-
Exclusion of Non-Distributed Grantor and Granted Statements:
-
Previous behavior: Non-distributed grantor and granted roles were propagated, even if they were non-distributed.
-
Current behavior: This PR filters out non-distributed roles, ensuring that non-distributed grantor and granted roles are not propagated, aligning with the behavior of grantee roles.
Currently, grant statements with non-distributed roles are being filtered and not propagated for granted and grantor roles, similar to grantee roles.
-
-
Elimination of Grant Statement Propagation for Non-Distributed Roles:
-
Previous behavior: Local execution of grant statements with non-distributed roles caused errors in propagation, leading to failures in
citus_add_node
/citus_activate_node
. - Current behavior: All non-distributed roles are filtered before propagation to prevent potential errors, ensuring the success of add/activate node operations.
-
Previous behavior: Local execution of grant statements with non-distributed roles caused errors in propagation, leading to failures in
-
Grant Ordering in Metadata Sync:
- Previous behavior: Errors in issue #7519 were attributed to invalid grant orders, particularly when dealing with 'WITH ADMIN OPTION.'
- Current behavior: This PR addresses the issue by exporting grants with 'WITH ADMIN OPTION' first, followed by other grant statements, establishing a consistent order and resolving the associated errors.
Note: A meeting with @onurctirtir led to the consideration of using a method from PR #7549 for item 3. However, an error was encountered: "Citus cannot handle circular dependencies between distributed objects," creating unintended dependencies related to the grantor.
Codecov Report
Merging #7538 (e0cc004) into main (8afa2d0) will decrease coverage by
0.02%
. The diff coverage is91.48%
.
Additional details and impacted files
@@ Coverage Diff @@
## main #7538 +/- ##
==========================================
- Coverage 89.69% 89.67% -0.02%
==========================================
Files 282 282
Lines 60462 60549 +87
Branches 7530 7542 +12
==========================================
+ Hits 54233 54300 +67
- Misses 4078 4088 +10
- Partials 2151 2161 +10
There is a typo in the pr description. Why is this change needed?
Rather than adjusting the tests to fix the flakyness, we should come up with a better mechanism that would handle the implicit dependency graphs imposed by granted by
clauses.
For example,
If some permissions to role X are granted by Y and some permissions to role Y are granted by Z, then the creation order of those roles should be Z - Y - X
in in ActivateNode()
Rather than adjusting the tests to fix the flakyness, we should come up with a better mechanism that would handle the implicit dependency graphs imposed by
granted by
clauses.For example, If some permissions to role X are granted by Y and some permissions to role Y are granted by Z, then the creation order of those roles should be
Z - Y - X
in inActivateNode()
It is work in progress. Converted it into draft I actually opened it just to see the test status I will evaluate all the ordering here