Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for granted by propagation for grant statements #7538

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gurkanindibay
Copy link
Contributor

@gurkanindibay gurkanindibay commented Feb 26, 2024

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 Flaky WITH ADMIN OPTION test #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.

Copy link

codecov bot commented Feb 26, 2024

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     

@eaydingol
Copy link
Contributor

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

@onurctirtir
Copy link
Member

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

@gurkanindibay gurkanindibay marked this pull request as draft February 27, 2024 11:00
@gurkanindibay
Copy link
Contributor Author

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 gurkanindibay changed the title Moves test_atmin_role after node removal Moves test_admin_role after node removal Feb 27, 2024
@gurkanindibay gurkanindibay changed the title Moves test_admin_role after node removal Adds support for granted by propagation for grant statements Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants