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

Add Grant SQL Macros #5369

Merged
merged 37 commits into from
Jul 11, 2022
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
6e88fe7
init push or ct-660 work
McKnight-42 Jun 13, 2022
6d4b938
changes to default versions of get_show_grant_sql and get_grant_sql
McKnight-42 Jun 14, 2022
ea35fd1
completing init default versions of all macros being called for look …
McKnight-42 Jun 14, 2022
7e2b707
minor update to should_revoke
McKnight-42 Jun 14, 2022
898aa8f
post pairing push up (does have log statements to make sure we remove)
McKnight-42 Jun 15, 2022
76050da
minor spacing changes
McKnight-42 Jun 15, 2022
7a3e7c6
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 16, 2022
5fb07c1
minor changes, and removal of logs so people can have clean grab of code
McKnight-42 Jun 16, 2022
4cf705f
minor changes to how get_revoke_sql works
McKnight-42 Jun 17, 2022
2fff84b
init attempt at applying apply_grants to all materialzations
McKnight-42 Jun 17, 2022
6fccef6
name change from recipents -> grantee
McKnight-42 Jun 17, 2022
528dff6
minor changes
McKnight-42 Jun 17, 2022
95f7ae4
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 21, 2022
9079c4a
working on making a context to handle the diff gathering between gran…
McKnight-42 Jun 21, 2022
6bdb4b5
removing logs from most materializations to better track diff of gran…
McKnight-42 Jun 22, 2022
597ab05
starting to build out postgres get_show_grant_sql getting empty query…
McKnight-42 Jun 22, 2022
138f443
6/27 eod update looking into diff_grants variable not getting passed …
McKnight-42 Jun 27, 2022
fc7e24b
changes to loop cases
McKnight-42 Jun 28, 2022
6f53b3d
changes after pairing meeting
McKnight-42 Jun 28, 2022
b3e37cb
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jun 29, 2022
99b1445
adding apply_grants to create_or_replace_view.sql
McKnight-42 Jul 5, 2022
7946a3b
models are building but testing out small issues around revoke statem…
McKnight-42 Jul 5, 2022
0597398
postgrest must fixes from jeremy's feedback
McKnight-42 Jul 6, 2022
1d263b7
postgres minor change to standarize_grants_dict
McKnight-42 Jul 6, 2022
c2e9aeb
updating after pairing with dough and jeremey incorporating the new v…
McKnight-42 Jul 6, 2022
0843f66
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jul 6, 2022
077e4ff
adding ref of diff_of_two_dicts to base keys ref
McKnight-42 Jul 6, 2022
ab6be85
change of method type for standardize_grants_dict
McKnight-42 Jul 6, 2022
da557d9
minor update trying to fix unit test
McKnight-42 Jul 6, 2022
f2c957f
changes based on morning feedback
McKnight-42 Jul 7, 2022
eb935d5
change log message in default_apply_grants macro
McKnight-42 Jul 7, 2022
cefcd4f
CT-808 grant adapter tests (#5447)
gshank Jul 7, 2022
bdc0c71
rename grant[privilege] -> grant_config[privilege]
McKnight-42 Jul 7, 2022
67f5beb
Merge branch 'main' of github.com:dbt-labs/dbt into ct-660-grant-sql
McKnight-42 Jul 11, 2022
72bdae9
postgres macro rename to copy_grants
McKnight-42 Jul 11, 2022
794f4d1
CT-808 more grant adapter tests (#5452)
gshank Jul 11, 2022
068c59b
update to main, create changelog, whitespace fixes
McKnight-42 Jul 11, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changes/unreleased/Under the Hood-20220706-215001.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Under the Hood
body: Add tests for SQL grants
time: 2022-07-06T21:50:01.498562-04:00
custom:
Author: gshank
Issue: "5437"
PR: "5447"
3 changes: 3 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ jobs:
TOXENV: integration
PYTEST_ADDOPTS: "-v --color=yes -n4 --csv integration_results.csv"
DBT_INVOCATION_ENV: github-actions
DBT_TEST_USER_1: dbt_test_user_1
DBT_TEST_USER_2: dbt_test_user_2
DBT_TEST_USER_3: dbt_test_user_3

steps:
- name: Check out the repository
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/structured-logging-schema-check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ jobs:
LOG_DIR: "/home/runner/work/dbt-core/dbt-core/logs"
# tells integration tests to output into json format
DBT_LOG_FORMAT: "json"
# Additional test users
DBT_TEST_USER_1: dbt_test_user_1
DBT_TEST_USER_2: dbt_test_user_2
DBT_TEST_USER_3: dbt_test_user_3

steps:
- name: checkout dev
uses: actions/checkout@v2
Expand Down
28 changes: 28 additions & 0 deletions core/dbt/adapters/base/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ class BaseAdapter(metaclass=AdapterMeta):
- convert_datetime_type
- convert_date_type
- convert_time_type
- standardize_grants_dict

Macros:
- get_catalog
Expand Down Expand Up @@ -538,6 +539,33 @@ def list_relations_without_caching(self, schema_relation: BaseRelation) -> List[
"`list_relations_without_caching` is not implemented for this " "adapter!"
)

###
# Methods about grants
###
@available
def standardize_grants_dict(self, grants_table: agate.Table) -> dict:
"""Translate the result of `show grants` (or equivalent) to match the
grants which a user would configure in their project.

Ideally, the SQL to show grants should also be filtering:
filter OUT any grants TO the current user/role (e.g. OWNERSHIP).
If that's not possible in SQL, it can be done in this method instead.

:param grants_table: An agate table containing the query result of
the SQL returned by get_show_grant_sql
:return: A standardized dictionary matching the `grants` config
:rtype: dict
"""
grants_dict: Dict[str, List[str]] = {}
for row in grants_table:
grantee = row["grantee"]
privilege = row["privilege_type"]
if privilege in grants_dict.keys():
grants_dict[privilege].append(grantee)
else:
grants_dict.update({privilege: [grantee]})
return grants_dict

###
# Provided methods about relations
###
Expand Down
31 changes: 30 additions & 1 deletion core/dbt/context/base.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import json
import os
from typing import Any, Dict, NoReturn, Optional, Mapping, Iterable, Set
from typing import Any, Dict, NoReturn, Optional, Mapping, Iterable, Set, List

from dbt import flags
from dbt import tracking
Expand Down Expand Up @@ -657,6 +657,35 @@ def print(msg: str) -> str:
print(msg)
return ""

@contextmember
@staticmethod
def diff_of_two_dicts(
dict_a: Dict[str, List[str]], dict_b: Dict[str, List[str]]
) -> Dict[str, List[str]]:
"""
Given two dictionaries of type Dict[str, List[str]]:
dict_a = {'key_x': ['value_1', 'VALUE_2'], 'KEY_Y': ['value_3']}
dict_b = {'key_x': ['value_1'], 'key_z': ['value_4']}
Return the same dictionary representation of dict_a MINUS dict_b,
performing a case-insensitive comparison between the strings in each.
All keys returned will be in the original case of dict_a.
returns {'key_x': ['VALUE_2'], 'KEY_Y': ['value_3']}
"""

dict_diff = {}
dict_b_lowered = {k.casefold(): [x.casefold() for x in v] for k, v in dict_b.items()}
for k in dict_a:
if k.casefold() in dict_b_lowered.keys():
diff = []
for v in dict_a[k]:
if v.casefold() not in dict_b_lowered[k.casefold()]:
diff.append(v)
if diff:
dict_diff.update({k: diff})
else:
dict_diff.update({k: dict_a[k]})
return dict_diff


def generate_base_context(cli_vars: Dict[str, Any]) -> Dict[str, Any]:
ctx = BaseContext(cli_vars)
Expand Down
167 changes: 167 additions & 0 deletions core/dbt/include/global_project/macros/adapters/apply_grants.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
{# ------- BOOLEAN MACROS --------- #}

{#
-- COPY GRANTS
-- When a relational object (view or table) is replaced in this database,
-- do previous grants carry over to the new object? This may depend on:
-- whether we use alter-rename-swap versus CREATE OR REPLACE
-- user-supplied configuration (e.g. copy_grants on Snowflake)
-- By default, play it safe, assume TRUE: that grants ARE copied over.
-- This means dbt will first "show" current grants and then calculate diffs.
-- It may require an additional query than is strictly necessary,
-- but better safe than sorry.
#}

{% macro copy_grants() %}
{{ return(adapter.dispatch('copy_grants', 'dbt')()) }}
{% endmacro %}

{% macro default__copy_grants() %}
{{ return(True) }}
{% endmacro %}


{#
-- SUPPORT MULTIPLE GRANTEES PER DCL STATEMENT
-- Does this database support 'grant {privilege} to {grantee_1}, {grantee_2}, ...'
-- Or must these be separate statements:
-- `grant {privilege} to {grantee_1}`;
-- `grant {privilege} to {grantee_2}`;
-- By default, pick the former, because it's what we prefer when available.
#}

{% macro support_multiple_grantees_per_dcl_statement() %}
{{ return(adapter.dispatch('support_multiple_grantees_per_dcl_statement', 'dbt')()) }}
{% endmacro %}

{%- macro default__support_multiple_grantees_per_dcl_statement() -%}
{{ return(True) }}
{%- endmacro -%}


{% macro should_revoke(existing_relation, full_refresh_mode=True) %}

{% if not existing_relation %}
{#-- The table doesn't already exist, so no grants to copy over --#}
{{ return(False) }}
{% elif full_refresh_mode %}
{#-- The object is being REPLACED -- whether grants are copied over depends on the value of user config --#}
{{ return(copy_grants()) }}
{% else %}
{#-- The table is being merged/upserted/inserted -- grants will be carried over --#}
{{ return(True) }}
{% endif %}

{% endmacro %}

{# ------- DCL STATEMENT TEMPLATES --------- #}

{% macro get_show_grant_sql(relation) %}
{{ return(adapter.dispatch("get_show_grant_sql", "dbt")(relation)) }}
{% endmacro %}

{% macro default__get_show_grant_sql(relation) %}
show grants on {{ relation }}
{% endmacro %}


{% macro get_grant_sql(relation, privilege, grantees) %}
{{ return(adapter.dispatch('get_grant_sql', 'dbt')(relation, privilege, grantees)) }}
{% endmacro %}

{%- macro default__get_grant_sql(relation, privilege, grantees) -%}
grant {{ privilege }} on {{ relation }} to {{ grantees | join(', ') }}
{%- endmacro -%}


{% macro get_revoke_sql(relation, privilege, grantees) %}
{{ return(adapter.dispatch('get_revoke_sql', 'dbt')(relation, privilege, grantees)) }}
{% endmacro %}

{%- macro default__get_revoke_sql(relation, privilege, grantees) -%}
revoke {{ privilege }} on {{ relation }} from {{ grantees | join(', ') }}
{%- endmacro -%}


{# ------- RUNTIME APPLICATION --------- #}

{% macro get_dcl_statement_list(relation, grant_config, get_dcl_macro) %}
{{ return(adapter.dispatch('get_dcl_statement_list', 'dbt')(relation, grant_config, get_dcl_macro)) }}
{% endmacro %}

{%- macro default__get_dcl_statement_list(relation, grant_config, get_dcl_macro) -%}
{#
-- Unpack grant_config into specific privileges and the set of users who need them granted/revoked.
-- Depending on whether this database supports multiple grantees per statement, pass in the list of
-- all grantees per privilege, or (if not) template one statement per privilege-grantee pair.
-- `get_dcl_macro` will be either `get_grant_sql` or `get_revoke_sql`
#}
{%- set dcl_statements = [] -%}
{%- for privilege, grantees in grant_config.items() %}
{%- if support_multiple_grantees_per_dcl_statement() and grantees -%}
{%- set dcl = get_dcl_macro(relation, privilege, grantees) -%}
{%- do dcl_statements.append(dcl) -%}
{%- else -%}
{%- for grantee in grantees -%}
{% set dcl = get_dcl_macro(relation, privilege, [grantee]) %}
{%- do dcl_statements.append(dcl) -%}
{% endfor -%}
{%- endif -%}
{%- endfor -%}
{{ return(dcl_statements) }}
{%- endmacro %}


{% macro call_dcl_statements(dcl_statement_list) %}
{{ return(adapter.dispatch("call_dcl_statements", "dbt")(dcl_statement_list)) }}
{% endmacro %}

{% macro default__call_dcl_statements(dcl_statement_list) %}
{#
-- By default, supply all grant + revoke statements in a single semicolon-separated block,
-- so that they're all processed together.

-- Some databases do not support this. Those adapters will need to override this macro
-- to run each statement individually.
#}
{% call statement('grants') %}
{% for dcl_statement in dcl_statement_list %}
{{ dcl_statement }};
{% endfor %}
{% endcall %}
{% endmacro %}


{% macro apply_grants(relation, grant_config, should_revoke) %}
{{ return(adapter.dispatch("apply_grants", "dbt")(relation, grant_config, should_revoke)) }}
{% endmacro %}

{% macro default__apply_grants(relation, grant_config, should_revoke=True) %}
{#-- If grant_config is {} or None, this is a no-op --#}
{% if grant_config %}
{% if should_revoke %}
{#-- We think previous grants may have carried over --#}
{#-- Show current grants and calculate diffs --#}
{% set current_grants_table = run_query(get_show_grant_sql(relation)) %}
{% set current_grants_dict = adapter.standardize_grants_dict(current_grants_table) %}
{% set needs_granting = diff_of_two_dicts(grant_config, current_grants_dict) %}
{% set needs_revoking = diff_of_two_dicts(current_grants_dict, grant_config) %}
{% if not (needs_granting or needs_revoking) %}
{{ log('On ' ~ relation ~': All grants are in place, no revocation or granting needed.')}}
{% endif %}
{% else %}
{#-- We don't think there's any chance of previous grants having carried over. --#}
{#-- Jump straight to granting what the user has configured. --#}
{% set needs_revoking = {} %}
{% set needs_granting = grant_config %}
{% endif %}
{% if needs_granting or needs_revoking %}
{% set revoke_statement_list = get_dcl_statement_list(relation, needs_revoking, get_revoke_sql) %}
{% set grant_statement_list = get_dcl_statement_list(relation, needs_granting, get_grant_sql) %}
{% set dcl_statement_list = revoke_statement_list + grant_statement_list %}
{% if dcl_statement_list %}
{{ call_dcl_statements(dcl_statement_list) }}
{% endif %}
{% endif %}
{% endif %}
{% endmacro %}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{% set grant_config = config.get('grants') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{{ drop_relation_if_exists(preexisting_intermediate_relation) }}
{{ drop_relation_if_exists(preexisting_backup_relation) }}

Expand Down Expand Up @@ -59,6 +61,9 @@
{% do to_drop.append(backup_relation) %}
{% endif %}

{% set should_revoke = should_revoke(existing_relation, full_refresh_mode) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{% if existing_relation is none or existing_relation.is_view or should_full_refresh() %}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
{%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%}
-- as above, the backup_relation should not already exist
{%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
-- grab current tables grants config for comparision later on
{% set grant_config = config.get('grants') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

-- drop the temp relations if they exist already in the database
{{ drop_relation_if_exists(preexisting_intermediate_relation) }}
Expand All @@ -40,6 +42,9 @@

{{ run_hooks(post_hooks, inside_transaction=True) }}

{% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

-- `COMMIT` happens here
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
{%- set identifier = model['alias'] -%}

{%- set old_relation = adapter.get_relation(database=database, schema=schema, identifier=identifier) -%}

{%- set exists_as_view = (old_relation is not none and old_relation.is_view) -%}

{%- set target_relation = api.Relation.create(
identifier=identifier, schema=schema, database=database,
type='view') -%}
{% set grant_config = config.get('grants') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ run_hooks(pre_hooks) }}

Expand All @@ -34,6 +34,9 @@
{{ get_create_view_as_sql(target_relation, sql) }}
{%- endcall %}

{% set should_revoke = should_revoke(exists_as_view, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=True) %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ run_hooks(post_hooks) }}

{{ return({'relations': [target_relation]}) }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
{%- set backup_relation = make_backup_relation(target_relation, backup_relation_type) -%}
-- as above, the backup_relation should not already exist
{%- set preexisting_backup_relation = load_cached_relation(backup_relation) -%}
-- grab current tables grants config for comparision later on
{% set grant_config = config.get('grants') %}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved

{{ run_hooks(pre_hooks, inside_transaction=False) }}

Expand All @@ -47,6 +49,9 @@
{% endif %}
{{ adapter.rename_relation(intermediate_relation, target_relation) }}

{% set should_revoke = should_revoke(existing_relation, full_refresh_mode=True) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{{ run_hooks(post_hooks, inside_transaction=True) }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
{%- set exists_as_table = (old_relation is not none and old_relation.is_table) -%}
{%- set exists_as_view = (old_relation is not none and old_relation.is_view) -%}

{%- set grant_config = config.get('grants') -%}
McKnight-42 marked this conversation as resolved.
Show resolved Hide resolved
{%- set agate_table = load_agate_table() -%}
-- grab current tables grants config for comparision later on

{%- do store_result('agate_table', response='OK', agate_table=agate_table) -%}

{{ run_hooks(pre_hooks, inside_transaction=False) }}
Expand All @@ -35,6 +38,10 @@
{% endcall %}

{% set target_relation = this.incorporate(type='table') %}

{% set should_revoke = should_revoke(old_relation, full_refresh_mode) %}
{% do apply_grants(target_relation, grant_config, should_revoke=should_revoke) %}

{% do persist_docs(target_relation, model) %}

{% if full_refresh_mode or not exists_as_table %}
Expand Down
Loading