Add Grant SQL Macros #5369

McKnight-42 · 2022-06-13T20:20:41Z

Resolves #5263
Resolves #5329

Description:
Allow users to define grants as a reasonable default in dbt_project.yml and within each model sql or yml file as a config.

Checklist

I have read the contributing guide and understand what's expected of me
I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
I have run changie new to create a changelog entry

github-actions · 2022-06-13T20:20:57Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

jtcohen6

Great start on this @McKnight-42! Thanks for opening the draft PR so early, so that we can keep pushing forward on this.

In my comments, I tried to provide relevant information that will help us decide if we're implementing the right standard, by initializing this default behavior in dbt-core. Much of the database-specific nuance will come out in the per-adapter implementations, but we always need to first make sure that we're putting the right core constructs in place.

core/dbt/include/global_project/macros/adapters/apply_grants.sql

…over and collaboration

jtcohen6

nice progress!

core/dbt/include/global_project/macros/adapters/apply_grants.sql

core/dbt/include/global_project/macros/materializations/models/table/table.sql

core/dbt/include/global_project/macros/adapters/apply_grants.sql

…t_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log

…ts generation logs

McKnight-42 · 2022-06-22T17:43:14Z

NOTE: create postgres version of get_show_grant sql https://linuxhint.com/check-postgres-user-privileges/

… errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching

…into get_revoke_sql

jtcohen6 · 2022-07-07T16:49:49Z

plugins/postgres/dbt/include/postgres/macros/adapters.sql

+{%- macro postgres__get_show_grant_sql(relation) -%}
+  select grantee, privilege_type
+  from information_schema.role_table_grants
+      where grantor = current_role


Not a TODO, just an open question: Should we / do we need to limit ourselves to just the grants provided by the current_role?

To copy-paste what I just said in dbt-snowflake:

The original motivation for this was: If someone else has granted a privilege on this model, and then dbt tries to revoke it, the revocation may fail. But if dbt's user/role owns the table, it should be able to revoke any access on it... right?

By removing this logic, we'd be making a strong claim, that ALL the grants applied on this object, MUST be applied by dbt, in the grants config. I think that might be a strong opinion we want to hold!

No action needed right now. Just wanted to leave the comment for future reference.

core/dbt/include/global_project/macros/adapters/apply_grants.sql

jtcohen6 · 2022-07-07T17:54:51Z

@McKnight-42 I've gone back through the threads above, and I think all the blocking TODOs have been resolved! Nice work!

The tests are still failing, and the fixes for those tests are in #5447. Let's either:

pull the fixes into this branch, so we can merge this PR into main
wait to merge CT-808 grant adapter tests #5447 (into this branch), before merging anything into main

* Create initial test for grants Co-authored-by: Jeremy Cohen <[email protected]>

core/dbt/include/global_project/macros/adapters/apply_grants.sql

dataders · 2022-07-08T19:21:00Z

core/dbt/context/base.py

+        dict_diff = {}
+        dict_a = {k.lower(): v for k, v in dict_a.items()}
+        dict_b = {k.lower(): v for k, v in dict_b.items()}
+        for k in dict_a:
+            if k in dict_b:
+                a_lowered = map(lambda x: x.lower(), dict_a[k])
+                b_lowered = map(lambda x: x.lower(), dict_b[k])
+                diff = list(set(a_lowered) - set(b_lowered))
+                if diff:
+                    dict_diff.update({k: diff})
+            else:
+                dict_diff.update({k: dict_a[k]})


have we tested the performance on this when the grants dict config is huge (and very different from current grants)? might not suck once, but might if they have this same complicated diff on hundreds of models?

This code can + should absolutely be refactored for more Pythonic comprehension + better performance / lower memory footprint. I'm not the right person to do that :)

hundreds of models?

FWIW this code will be executed in parallel --threads, assuming the user is making use of them

plugins/postgres/dbt/include/postgres/macros/adapters.sql

ChenyuLInx · 2022-07-08T23:45:25Z

core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql

@@ -20,6 +20,8 @@
  -- BEGIN, in a separate transaction
  {%- set preexisting_intermediate_relation = load_cached_relation(intermediate_relation)-%}
  {%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
+   -- grab current tables grants config for comparision later on


Not something needed to do before merging, but we added the same change pretty much for all materialization methods. Should we consider refactoring the code so we can only do it in one place?

* Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <[email protected]> Co-authored-by: Emily Rockman <[email protected]>

dbeatty10

👍
Looks good to me. Only thing we should do first is apply the white space fixes.

My opinion is to ship this now so that we can unblock merging for everything else downstream.

Separately, we can break out any optimizations or refactoring separately to incorporate @nathaniel-may and @ChenyuLInx's ideas.

core/dbt/include/global_project/macros/materializations/models/incremental/incremental.sql

core/dbt/include/global_project/macros/materializations/models/table/table.sql

core/dbt/include/global_project/macros/materializations/models/view/create_or_replace_view.sql

core/dbt/include/global_project/macros/materializations/models/view/view.sql

core/dbt/include/global_project/macros/materializations/seeds/seed.sql

core/dbt/include/global_project/macros/materializations/snapshots/snapshot.sql

jtcohen6 · 2022-07-11T16:10:52Z

What Doug said! The recommended changes are good ones, and we can pick them up in subsequent refactors. Let's unblock the adapter PRs in the meantime.

* init push or ct-660 work * changes to default versions of get_show_grant_sql and get_grant_sql * completing init default versions of all macros being called for look over and collaboration * minor update to should_revoke * post pairing push up (does have log statements to make sure we remove) * minor spacing changes * minor changes, and removal of logs so people can have clean grab of code * minor changes to how get_revoke_sql works * init attempt at applying apply_grants to all materialzations * name change from recipents -> grantee * minor changes * working on making a context to handle the diff gathering between grant_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log * removing logs from most materializations to better track diff of grants generation logs * starting to build out postgres get_show_grant_sql getting empty query errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching * 6/27 eod update looking into diff_grants variable not getting passed into get_revoke_sql * changes to loop cases * changes after pairing meeting * adding apply_grants to create_or_replace_view.sql * models are building but testing out small issues around revoke statement never building * postgrest must fixes from jeremy's feedback * postgres minor change to standarize_grants_dict * updating after pairing with dough and jeremey incorporating the new version of should revoke logic. * adding ref of diff_of_two_dicts to base keys ref * change of method type for standardize_grants_dict * minor update trying to fix unit test * changes based on morning feedback * change log message in default_apply_grants macro * CT-808 grant adapter tests (dbt-labs#5447) * Create initial test for grants Co-authored-by: Jeremy Cohen <[email protected]> * rename grant[privilege] -> grant_config[privilege] * postgres macro rename to copy_grants * CT-808 more grant adapter tests (dbt-labs#5452) * Add tests for invalid user and invalid privilege * Add more grant tests * Macro touch ups * Many more tests * Allow easily replacing privilege names * Keep adding tests * Refactor macros to return lists, fix test * Code checks * Keep tweaking tests * Revert cool grantees join bc Snowflake isnt happy * Use Postgres/BQ as standard for standardize_grants_dict * Code checks * add missing replace * small replace tweak, add additional dict diffs * All tests passing on BQ * Add type cast to test_snapshot_grants * Refactor for DRYer apply_grants macros Co-authored-by: Jeremy Cohen <[email protected]> Co-authored-by: Emily Rockman <[email protected]> * update to main, create changelog, whitespace fixes Co-authored-by: Gerda Shank <[email protected]> Co-authored-by: Jeremy Cohen <[email protected]> Co-authored-by: Emily Rockman <[email protected]>

init push or ct-660 work

6e88fe7

cla-bot bot added the cla:yes label Jun 13, 2022

McKnight-42 changed the title ~~init push or ct-660 work~~ Add Grant SQL Macros Jun 13, 2022

jtcohen6 reviewed Jun 14, 2022

View reviewed changes

core/dbt/include/global_project/macros/adapters/apply_grants.sql Outdated Show resolved Hide resolved

McKnight-42 self-assigned this Jun 14, 2022

McKnight-42 added 2 commits June 14, 2022 10:49

changes to default versions of get_show_grant_sql and get_grant_sql

6d4b938

completing init default versions of all macros being called for look …

ea35fd1

…over and collaboration

McKnight-42 requested a review from jtcohen6 June 14, 2022 21:19

McKnight-42 added 3 commits June 14, 2022 16:20

minor update to should_revoke

7e2b707

post pairing push up (does have log statements to make sure we remove)

898aa8f

minor spacing changes

76050da

jtcohen6 reviewed Jun 16, 2022

View reviewed changes

McKnight-42 added 2 commits June 16, 2022 14:09

Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql

7a3e7c6

minor changes, and removal of logs so people can have clean grab of code

5fb07c1

jtcohen6 reviewed Jun 16, 2022

View reviewed changes

core/dbt/include/global_project/macros/adapters/apply_grants.sql Outdated Show resolved Hide resolved

McKnight-42 added 3 commits June 17, 2022 12:47

minor changes to how get_revoke_sql works

4cf705f

init attempt at applying apply_grants to all materialzations

2fff84b

name change from recipents -> grantee

6fccef6

McKnight-42 commented Jun 17, 2022

View reviewed changes

core/dbt/include/global_project/macros/adapters/apply_grants.sql Outdated Show resolved Hide resolved

McKnight-42 added 4 commits June 17, 2022 16:53

minor changes

528dff6

Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql

95f7ae4

working on making a context to handle the diff gathering between gran…

9079c4a

…t_config and curreent_grants to see what needs to be revoked, I know if we assign a role, and a model becomes dependent on it we can't drop the role now still not seeing the diff appear in log

removing logs from most materializations to better track diff of gran…

6bdb4b5

…ts generation logs

McKnight-42 added 2 commits June 22, 2022 17:01

starting to build out postgres get_show_grant_sql getting empty query…

597ab05

… errors hopefully will clear up as we add the other postgres versions of macros and isn't a psycopg2 issue as indicated by searching

6/27 eod update looking into diff_grants variable not getting passed …

138f443

…into get_revoke_sql

McKnight-42 requested a review from jtcohen6 June 27, 2022 22:10

changes to loop cases

fc7e24b

jtcohen6 mentioned this pull request Jul 7, 2022

CT-808 grant adapter tests #5447

Merged

6 tasks

dbeatty10 mentioned this pull request Jul 7, 2022

[CT-660] [Feature] Add Grant SQL to Global Project #5263

Closed

9 tasks

changes based on morning feedback

f2c957f

jtcohen6 reviewed Jul 7, 2022

View reviewed changes

core/dbt/include/global_project/macros/adapters/apply_grants.sql Outdated Show resolved Hide resolved

McKnight-42 and others added 2 commits July 7, 2022 14:57

change log message in default_apply_grants macro

eb935d5

CT-808 grant adapter tests (#5447)

cefcd4f

* Create initial test for grants Co-authored-by: Jeremy Cohen <[email protected]>

gshank requested a review from leahwicz as a code owner July 7, 2022 20:02

McKnight-42 mentioned this pull request Jul 7, 2022

Add Grants to BigQuery Materializations dbt-labs/dbt-bigquery#212

Merged

6 tasks

McKnight-42 requested a review from jtcohen6 July 7, 2022 21:32

rename grant[privilege] -> grant_config[privilege]

bdc0c71

jtcohen6 reviewed Jul 8, 2022

View reviewed changes

core/dbt/include/global_project/macros/adapters/apply_grants.sql Outdated Show resolved Hide resolved

dataders reviewed Jul 8, 2022

View reviewed changes

plugins/postgres/dbt/include/postgres/macros/adapters.sql Outdated Show resolved Hide resolved

ChenyuLInx reviewed Jul 8, 2022

View reviewed changes

McKnight-42 and others added 3 commits July 11, 2022 09:02

Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql

67f5beb

postgres macro rename to copy_grants

72bdae9

McKnight-42 requested review from jtcohen6, dataders and ChenyuLInx July 11, 2022 15:36

dbeatty10 approved these changes Jul 11, 2022

View reviewed changes

jtcohen6 approved these changes Jul 11, 2022

View reviewed changes

update to main, create changelog, whitespace fixes

068c59b

McKnight-42 merged commit c25260e into main Jul 11, 2022

McKnight-42 deleted the ct-660-grant-sql branch July 11, 2022 16:58

jtcohen6 mentioned this pull request Nov 1, 2022

[CT-1457] Move dbt-postgres into its own repository #6189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Grant SQL Macros #5369

Add Grant SQL Macros #5369

McKnight-42 commented Jun 13, 2022 •

edited by dbeatty10

Loading

github-actions bot commented Jun 13, 2022

jtcohen6 left a comment

jtcohen6 left a comment

McKnight-42 commented Jun 22, 2022 •

edited

Loading

jtcohen6 Jul 7, 2022 •

edited

Loading

jtcohen6 commented Jul 7, 2022

dataders Jul 8, 2022

jtcohen6 Jul 11, 2022

ChenyuLInx Jul 8, 2022

dbeatty10 left a comment

jtcohen6 commented Jul 11, 2022 •

edited

Loading

Add Grant SQL Macros #5369

Add Grant SQL Macros #5369

Conversation

McKnight-42 commented Jun 13, 2022 • edited by dbeatty10 Loading

Checklist

github-actions bot commented Jun 13, 2022

jtcohen6 left a comment

Choose a reason for hiding this comment

jtcohen6 left a comment

Choose a reason for hiding this comment

McKnight-42 commented Jun 22, 2022 • edited Loading

jtcohen6 Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

jtcohen6 commented Jul 7, 2022

dataders Jul 8, 2022

Choose a reason for hiding this comment

jtcohen6 Jul 11, 2022

Choose a reason for hiding this comment

ChenyuLInx Jul 8, 2022

Choose a reason for hiding this comment

dbeatty10 left a comment

Choose a reason for hiding this comment

jtcohen6 commented Jul 11, 2022 • edited Loading

McKnight-42 commented Jun 13, 2022 •

edited by dbeatty10

Loading

McKnight-42 commented Jun 22, 2022 •

edited

Loading

jtcohen6 Jul 7, 2022 •

edited

Loading

jtcohen6 commented Jul 11, 2022 •

edited

Loading