Ct 716 add grants to materializations #178
Conversation
|
7/5/22 EOD REPORT: main report can be found on SQL GRANTS. did have a question on if the |
There was a problem hiding this comment.
Nice work!! This is running for me locally.
SHOULD fix (inefficient but correct):
- Let's figure out the right approach to
should_revokeon Snowflake. I left a big comment talking through this. It will have implications for the implementation we ultimately land on indbt-core.
dbt/adapters/snowflake/impl.py
Outdated
| for row in grants_table: | ||
| grantee = row['grantee_name'] | ||
| privilege = row['privilege'] | ||
| if privilege != 'OWNERSHIP': |
There was a problem hiding this comment.
The OWNERSHIP filter makes sense to me, and is one valid way of proceeding here.
My inclination would be to generalize this beyond just OWNERSHIP: all grants given BY the current user (=role), and NOT TO the current user (=role):
for row in grants_table:
# three relevant columns
grantee = row['grantee_name']
privilege = row['privilege']
granted_by = row['granted_by']
# super gross, but it works
# (this is the value from profiles.yml)
current_role = self.connections.profile.credentials.role
if (
# granted BY the current role
granted_by.lower() == current_role.lower() and
# NOT granted TO the current role
grantee.lower() != current_role.lower()
):
...|
|
||
| {% set target_relation = this.incorporate(type='view') %} | ||
|
|
||
| {% do apply_grants(target_relation, grant_config, should_revoke=False) %} |
There was a problem hiding this comment.
We don't need this here if apply_grants is already happening within create_or_replace_view() (here)
| @@ -25,6 +27,8 @@ | |||
|
|
|||
| {{ run_hooks(post_hooks) }} | |||
|
|
|||
| {% do apply_grants(target_relation, grant_config, should_revoke=True) %} | |||
There was a problem hiding this comment.
Okay, let's talk about should_revoke!
View + table:
should_revoke = False, because grants are NOT carried over- UNLESS the user has explicitly opted into the
copy_grantsconfig, in which case they are - What should our behavior be? I think it should be conditional based on
copy_grants, and this could simply beshould_revoke = copy_grants. Either both are False or both are True.
Incremental, seed, snapshot: These have the added questions of: Are we building this table for the first time? Are we fully replacing (full-refreshing) it? If we ARE fully replacing it, we should use the exact same conditional logic (based on copy_grants) to determine
If we are going to have conditional logic based on copy_grants, it requires us to have different logic for different models, rather than a single True/False setting for the entire adapter. That's a vote in favor of putting this logic into the materialization, rather than an adapter method, as discussed in dbt-labs/dbt-core#5369 (comment)
|
@McKnight-42 @dbeatty10 Follow-ups from pairing work: ct-716-add-materialization...jerco/grants-over-finish-line More context in dbt-labs/dbt-core#5369 (comment). Implements most of the SHOULD + COULD comments from above. |
…jeremey and doug earlier today.
…-716-add-materialization
|
@McKnight-42 Functional/integration tests are failing because this branch depends on the changes in dbt-labs/dbt-core#5369. Could you update these lines: dbt-snowflake/dev-requirements.txt Lines 3 to 4 in 0203a16 To point to your dbt-core branch (ct-660-grant-sql:
This is exactly how @dbeatty10 and I were doing the lift-and-shift of cross-database macros. Once the |
…/dbt-snowflake into ct-716-add-materialization
dataders
changed the title
resolves: CT-716
related to: CT-660
Before merge
dev-requirements.txtDescription
implementing
apply_grants, will add tests, for snowflake instances.Checklist
CHANGELOG.mdand added information about my change to the "dbt-snowflake next" section.