Fixes invalid grantor field parsing in grant role propagation #7451

gurkanindibay · 2024-01-24T14:51:42Z

DESCRIPTION: Fixes a bug that breaks distributed GRANT statements with grantor option

In this issue 3 issues are being solved:
1.Correcting the erroneous appending of multiple granted by in the deparser.
2Adding support for grantor (granted by) in grant role propagation.
3. Implementing grantor (granted by) support during the metadata sync grant role propagation phase.

Limitations: Currently, the grantor must be created prior to the metadata sync phase. During metadata sync, both the creation of the grantor and the grants given by that role cannot be performed, as the grantor role is not detected during the dependency resolution phase.

codecov · 2024-01-24T15:03:13Z

Codecov Report

Merging #7451 (6582b44) into main (c665cb8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7451   +/-   ##
=======================================
  Coverage   89.59%   89.59%           
=======================================
  Files         282      282           
  Lines       60314    60318    +4     
  Branches     7512     7513    +1     
=======================================
+ Hits        54037    54042    +5     
+ Misses       4121     4119    -2     
- Partials     2156     2157    +1

onurctirtir · 2024-01-30T13:11:25Z

Do we still plan to work on this PR @gurkanindibay / @halilozanakgul?

gurkanindibay · 2024-02-01T10:45:21Z

Do we still plan to work on this PR @gurkanindibay / @halilozanakgul?

I have plans to work on it. However, I can not get answer for the errors here. There are some intentional tests here in which non-distributed role is being used. After the fix, tests fail since the non-distributed field is being used as grantor

halilozanakgul · 2024-02-02T14:11:38Z

src/test/regress/sql/create_role_propagation.sql

@@ -107,7 +107,7 @@ RESET ROLE;

 SET citus.enable_create_role_propagation TO ON;

-GRANT dist_role_3 TO non_dist_role_3;
+GRANT dist_role_3 TO non_dist_role_3 granted by postgres;


What does this test? Isn't the current user already postgres? And we are not looking at a propagated query or something like that.

Changed to test_admin_role

onurctirtir · 2024-02-05T09:38:43Z

src/backend/distributed/deparser/deparse_role_stmts.c

@@ -486,7 +486,6 @@ AppendGrantRoleStmt(StringInfo buf, GrantRoleStmt *stmt)
 	appendStringInfo(buf, "%s ", stmt->is_grant ? " TO " : " FROM ");
 	AppendRoleList(buf, stmt->grantee_roles);
 	AppendGrantWithAdminOption(buf, stmt);
-	AppendGrantedByInGrantForRoleSpec(buf, stmt->grantor, stmt->is_grant);


Why do we need to remove this?

There are multiple AppendGrantedByInGrantForRoleSpec added there which causes error

onurctirtir · 2024-02-05T09:41:44Z

src/test/regress/expected/create_role_propagation.out

@@ -240,10 +242,11 @@ SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::
      role       |     member      | grantor | admin_option
 ---------------------------------------------------------------------
 dist_role_1     | dist_role_2     | t       | f
- dist_role_3     | non_dist_role_3 | t       | f


Should we change the query into this so that we can verify whether we set grantor properly?

SELECT roleid::regrole::text AS role, member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text LIKE '%dist\_%' ORDER BY 1, 2;

we can not since grantor for pg 14 and pg 15 is different from pg16. I think that's why they put such check

What is the difference?
There is probably some valid reason for this change but it kind of makes the test useless, since the grantor will almost always be postgres in our tests.

Now added test_admin_role into viable grantor list to make grantor t for the list.

onurctirtir · 2024-02-05T09:51:27Z

src/test/regress/expected/create_role_propagation.out

@@ -240,10 +242,11 @@ SELECT roleid::regrole::text AS role, member::regrole::text, (grantor::regrole::
      role       |     member      | grantor | admin_option
 ---------------------------------------------------------------------
 dist_role_1     | dist_role_2     | t       | f
- dist_role_3     | non_dist_role_3 | t       | f
+ dist_role_3     | non_dist_role_3 | f       | f


Can we also add another test to make sure that we properly set grantor on other nodes too, maybe by using:

SELECT result FROM run_command_on_all_nodes( $$ SELECT json_agg(q.* ORDER BY member) FROM ( SELECT member::regrole::text, grantor::regrole::text, admin_option FROM pg_auth_members WHERE roleid::regrole::text = 'dist_role_3' ) q; $$ );

Added run_all_nodes into the query

Added the test to see the grantor for the new statement.

onurctirtir

LGTM except that one last test

onurctirtir · 2024-02-07T10:48:36Z

src/test/regress/expected/create_role_propagation.out

@@ -228,22 +228,41 @@ SET ROLE non_dist_role_1;
 GRANT dist_role_1 TO dist_role_2;
 RESET ROLE;
 SET citus.enable_create_role_propagation TO ON;
-GRANT dist_role_3 TO non_dist_role_3;
+create role test_admin_role;
+grant dist_role_3 to test_admin_role with admin option;


Can we change this into this so that we can actually test whether we propagate grantor. The latter grant stmt doesn't test this because of the none distributed role.

Suggested change

grant dist_role_3 to test_admin_role with admin option;

grant dist_role_3 to test_admin_role granted by ... with admin option;

suggested change is the same as the current code. Do I miss sth?

I added test_admin_role just for granted by. I added a distributed role grant on next step. Is it sufficient?

Regarding this GRANT stmt:

GRANT dist_role_3 TO dist_role_4 granted by test_admin_role;

We get different outputs on different nodes:

"member":"dist_role_4","role":"dist_role_3","grantor":"test_admin_role","admin_option":false} ... "member":"dist_role_4","role":"dist_role_3","grantor":"postgres","admin_option":false} ... "member":"dist_role_4","role":"dist_role_3","grantor":"postgres","admin_option":false}

So I got confused a bit, is this really expected?

This is the bug related to metadata_sync wrong grantor issue. I will fix it in another PR as we have talked

You're right . grantor was being set to NULL. Fixed it.

onurctirtir · 2024-02-07T10:50:01Z

DESCRIPTION: grantor field defined multiple times in role parsing which causes parsing error when using DeparseTreeNode This PR fixes granted by statement parsing for grant role propagation

Description format seems incorrect, could you improve this based on our usual format?

onurctirtir · 2024-02-07T11:55:16Z

DESCRIPTION: This PR fixes granted by statement parsing for grant role propagation

Can we follow our usual description format, as in:

DESCRIPTION: Fixes a bug that breaks distributed GRANT statements with grantor option

…itus into missing_grantor_sync

onurctirtir · 2024-02-08T08:43:48Z

src/backend/distributed/commands/role.c

-	/*
-	 * Postgres don't seem to use the grantor. Even dropping the grantor doesn't
-	 * seem to affect the membership. If this changes, we might need to add grantors
-	 * to the dependency resolution too. For now we just don't propagate it.
-	 */


LGTM now except what this comment states ..

As I mentioned earlier, could you please talk to @halilozanakgul to decide:
a) How hard it is to accomplish what this comment states, or,
b) If we're taking the risk of " role does not exist" errors coming from workers when the grantor is not a distributed role given that we expect such conditions to be rare.

Thanks!

nvm this, but let's fix metadata syncing issue in this PR too

Fixed the grantor propagation other than missing grantor before metadata sync phase. In dependency resolution phase, if grantor has admin rolesi then it can not be propagated. I think it is not a must for our tasks. I can open an issue and continue working on that task with suitable priority

@onurctirtir we discussed the issue in the meeting on 02/12/2024. Now grantor is not being taken from dependency resolution when it has an admin option on it.
I need to investigate further. Right now, since we have blocked issues. We decided to keep this issue as it is and after unblock all issues.
In parallel, I will start investigating the issue with the issue #7488
Thanks.

src/backend/distributed/commands/role.c

Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>

gurkanindibay added 2 commits January 24, 2024 17:51

Fixes grantor on role metadata sync

34b8914

Fixes indentation

fc3d5e7

Fixes indentation

b43d89e

Merge branch 'main' into missing_grantor_sync

9f86fbe

gurkanindibay added 5 commits February 1, 2024 13:56

Fixes non-distributed test error

890429e

Fixes test errors

5b910d5

Rollbacks some changes

201e11b

Rollbacks all changes

b8e915c

Rollbacks grantor changes

21e9381

onurctirtir mentioned this pull request Feb 1, 2024

Adds Grant Role support from non-main db #7404

Merged

gurkanindibay changed the title ~~Fixes grantor on role metadata sync~~ Fixes grantor field propagation in role propagation Feb 1, 2024

gurkanindibay changed the title ~~Fixes grantor field propagation in role propagation~~ Fixes invalid grantor field parsing in grant role propagation Feb 1, 2024

Fixes granted by propagation

244dcf0

gurkanindibay marked this pull request as ready for review February 1, 2024 14:15

gurkanindibay requested review from onurctirtir and halilozanakgul February 1, 2024 14:16

halilozanakgul reviewed Feb 2, 2024

View reviewed changes

Fixes review issues

da236c9

onurctirtir reviewed Feb 5, 2024

View reviewed changes

gurkanindibay added 2 commits February 5, 2024 14:41

Fixes review notes

ec9b87b

Fixes test output

cb59f41

gurkanindibay requested a review from onurctirtir February 5, 2024 12:12

Adds test_role_admin to make the test makes sense

03f67d6

gurkanindibay requested a review from halilozanakgul February 6, 2024 10:37

onurctirtir reviewed Feb 7, 2024

View reviewed changes

Merge branch 'main' into missing_grantor_sync

0c1faf1

gurkanindibay added 6 commits February 7, 2024 15:06

Fixes review notes

e971798

Merge branch 'main' into missing_grantor_sync

2528693

Fixes null grantor issue

752979d

Merge branch 'missing_grantor_sync' of https://github.com/citusdata/c…

2935c57

…itus into missing_grantor_sync

Removes log block

243c40a

Merge branch 'main' into missing_grantor_sync

ce423bf

gurkanindibay requested a review from onurctirtir February 8, 2024 05:34

onurctirtir reviewed Feb 8, 2024

View reviewed changes

onurctirtir mentioned this pull request Feb 8, 2024

Grantor propagation support #7488

Open

onurctirtir linked an issue Feb 8, 2024 that may be closed by this pull request

Grantor propagation support #7488

Open

gurkanindibay added 6 commits February 9, 2024 11:57

Merge branch 'main' into missing_grantor_sync

29fdac0

Adds grantor into metadata_sync

28e5bf7

Fixes metadata syncing issue

0add3c2

Removes non-distributed grantor propagation

2e49474

Fixes indentation

42c840f

Fixes postgres 15 test results

9953721

gurkanindibay requested a review from onurctirtir February 10, 2024 08:51

Merge branch 'main' into missing_grantor_sync

8a0263b

onurctirtir approved these changes Feb 14, 2024

View reviewed changes

src/backend/distributed/commands/role.c Outdated Show resolved Hide resolved

Update src/backend/distributed/commands/role.c

6582b44

Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com>

gurkanindibay enabled auto-merge (squash) February 15, 2024 07:49

gurkanindibay merged commit 59da063 into main Feb 15, 2024
157 checks passed

gurkanindibay deleted the missing_grantor_sync branch February 15, 2024 08:27

gurkanindibay mentioned this pull request Mar 11, 2024

Adds support for granted by propagation for grant statements #7538

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes invalid grantor field parsing in grant role propagation #7451

Fixes invalid grantor field parsing in grant role propagation #7451

gurkanindibay commented Jan 24, 2024 •

edited by onurctirtir

codecov bot commented Jan 24, 2024 •

edited

onurctirtir commented Jan 30, 2024

gurkanindibay commented Feb 1, 2024

halilozanakgul Feb 2, 2024

gurkanindibay Feb 5, 2024

onurctirtir Feb 5, 2024

gurkanindibay Feb 5, 2024

onurctirtir Feb 5, 2024

gurkanindibay Feb 5, 2024

halilozanakgul Feb 5, 2024

gurkanindibay Feb 5, 2024

onurctirtir Feb 5, 2024

gurkanindibay Feb 5, 2024

gurkanindibay Feb 5, 2024

onurctirtir left a comment

onurctirtir Feb 7, 2024 •

edited

gurkanindibay Feb 7, 2024

gurkanindibay Feb 7, 2024

onurctirtir Feb 7, 2024

gurkanindibay Feb 7, 2024 •

edited

gurkanindibay Feb 7, 2024 •

edited

onurctirtir commented Feb 7, 2024

onurctirtir commented Feb 7, 2024

onurctirtir Feb 8, 2024

onurctirtir Feb 8, 2024

gurkanindibay Feb 12, 2024

gurkanindibay Feb 12, 2024

	grant dist_role_3 to test_admin_role with admin option;
	grant dist_role_3 to test_admin_role granted by ... with admin option;

Fixes invalid grantor field parsing in grant role propagation #7451

Fixes invalid grantor field parsing in grant role propagation #7451

Conversation

gurkanindibay commented Jan 24, 2024 • edited by onurctirtir

codecov bot commented Jan 24, 2024 • edited

Codecov Report

onurctirtir commented Jan 30, 2024

gurkanindibay commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurctirtir left a comment

Choose a reason for hiding this comment

onurctirtir Feb 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay Feb 7, 2024 • edited

Choose a reason for hiding this comment

gurkanindibay Feb 7, 2024 • edited

Choose a reason for hiding this comment

onurctirtir commented Feb 7, 2024

onurctirtir commented Feb 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurkanindibay commented Jan 24, 2024 •

edited by onurctirtir

codecov bot commented Jan 24, 2024 •

edited

onurctirtir Feb 7, 2024 •

edited

gurkanindibay Feb 7, 2024 •

edited

gurkanindibay Feb 7, 2024 •

edited