citus icon indicating copy to clipboard operation
citus copied to clipboard

Adds support for granted by propagation for grant statements

Open gurkanindibay opened this issue 1 year ago • 4 comments

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:

  1. 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.

  2. 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.
  3. 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.

gurkanindibay avatar Feb 26 '24 16:02 gurkanindibay

Codecov Report

Merging #7538 (e0cc004) into main (8afa2d0) will decrease coverage by 0.02%. The diff coverage is 91.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     

codecov[bot] avatar Feb 26 '24 16:02 codecov[bot]

There is a typo in the pr description. Why is this change needed?

eaydingol avatar Feb 27 '24 06:02 eaydingol

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()

onurctirtir avatar Feb 27 '24 08:02 onurctirtir

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()

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

gurkanindibay avatar Feb 27 '24 11:02 gurkanindibay