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

Support Privilege Grants to Roles and Groups in Materialization #626

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

Conversation

soksamnanglim
Copy link
Contributor

@soksamnanglim soksamnanglim commented Oct 4, 2023

resolves #415
docs: dbt-labs/docs.getdbt.com#4193

Problem

Prior to this PR, users were unable to configure grants to roles and groups via grant config on resource.
A valid workaround existed. Users could prefix their permissions with "group" or "role", which introduced concerns:

  1. user error.
  2. get_show_grant did not query for group or role grants on the table. Expected behavior: grants configured via the workaround would be automatically revoked. Actual behavior: workaround grants remain.

Solution

Users may now extend grant configurations to roles and groups like this:

models:
    - name: specific_model
      config:
        grants:
            select: 
                user: ['chidi', 'tahani', 'eleanor']
                role: ['human', 'developer']
                group: ['good']
            insert:
                user: ['michael']
                role: ['architect']
                group: ['bad']

To not break existing dbt projects, dbt users granting privileges to only users may continue configuring grants the following way:

models:
    - name: specific_model
      config:
        grants:
            select: ['user_1']

Upon running, dbt will continue to automatically grant and revoke differences in privileges to users, groups, and roles.

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@cla-bot cla-bot bot added the cla:yes label Oct 4, 2023
@soksamnanglim
Copy link
Contributor Author

Ran all of the integration tests, however the test suite for materialized views fails for me even with no proposed changes. Would love some help on that! Kept getting a TypeError: NoneType is not subscriptable

Also had to refactor many of the integration tests from dbt-core as many methods/macros were overriden to support the new config, would love your input in that area!

@soksamnanglim soksamnanglim marked this pull request as ready for review October 4, 2023 16:40
@soksamnanglim soksamnanglim requested a review from a team as a code owner October 4, 2023 16:40
@colin-rogers-dbt
Copy link
Contributor

@soksamnanglim
Copy link
Contributor Author

Hmmm... integration tests for grants are still failing after I updated the CI workflow.

@colin-rogers-dbt
Copy link
Contributor

Hmmm... integration tests for grants are still failing after I updated the CI workflow.

Failing test:

____________ TestIncrementalGrantsRedshift.test_incremental_grants _____________
[gw2] linux -- Python 3.8.18 /home/runner/work/dbt-redshift/dbt-redshift/.tox/integration-redshift/bin/python

self = <test_grants.TestIncrementalGrantsRedshift object at 0x7f5ec33e4d90>
project = <dbt.tests.fixtures.project.TestProjInfo object at 0x7f5eb8a58e80>
get_test_users = ['dbt_test_user_1', 'dbt_test_user_2', 'dbt_test_user_3']
get_test_groups = [], get_test_roles = []

    def test_incremental_grants(self, project, get_test_users, get_test_groups, get_test_roles):
        # for debugging
        print("incremental test")
    
        # we want the test to fail, not silently skip
        test_users = get_test_users
        test_groups = get_test_groups
        test_roles = get_test_roles
        select_privilege_name = self.privilege_grantee_name_overrides()["select"]
        assert len(test_users) == 3
>       assert len(test_groups) == 3
E       assert 0 == 3
E        +  where 0 = len([])

@soksamnanglim
Copy link
Contributor Author

Failing test:

It's the same error that came up also before updating CI workflow. Perhaps, dbt_test_group_i and dbt_test_role_i need to be created in dbt's test cluster?

"schema.yml": updated_schema,
}

def test_view_table_grants(self, project, get_test_users, get_test_groups, get_test_roles):
Copy link
Contributor

Choose a reason for hiding this comment

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

Running locally I see issues with the expected/actual grants (it looks like the actual grants includes a superset of all users). Kind of hard to debug since there are so many test cases in this test. Can we break this up into separate test cases?

Copy link
Contributor Author

@soksamnanglim soksamnanglim Oct 11, 2023

Choose a reason for hiding this comment

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

Hi Colin, let me take a look.

The tests that are failing successfully pass in my local environment. Is there a way to set up my local environment to mirror dbt's local + remote testing environment? I follow the contributing guide, but some test results conflict between local and remote environment.

Copy link
Contributor Author

@soksamnanglim soksamnanglim Oct 11, 2023

Choose a reason for hiding this comment

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

Remotely, it seems the fixtures are not manifesting even with the fix. https://github.com/dbt-labs/dbt-redshift/actions/runs/6461848915/job/17542365369?pr=626#step:7:275

get_test_users = ['dbt_test_user_1', 'dbt_test_user_2', 'dbt_test_user_3']
get_test_groups = [], get_test_roles = []

Let me try refactoring base_grants to see if that fixes the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@soksamnanglim took another look at the tests and it looks like they're being invoked twice in some places. Simplified by making the "base" classes standard test classes since they shouldn't need to be inherited

Copy link
Contributor

Choose a reason for hiding this comment

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

also confirmed that the roles/groups are setup in our test DB via:
select * from SVV_ROLES; and select * from pg_group;

Copy link
Contributor Author

@soksamnanglim soksamnanglim Oct 24, 2023

Choose a reason for hiding this comment

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

After running revoke SELECT on "database"."schema"."my_model" from dbt_test_user_1, group dbt_test_group_1, role dbt_test_role_1;, test_model_grants successfully passes for me.

It seems that the privileges were not revoked properly the first time, but seems like they are automatically revoking fine now.

I'd like to figure out whether the code for automatic revoking/granting has a bug in it or my testing set up for models is incorrect—Can we try manually revoking those privileges from the dbt testing cluster and running the grant test suite multiple times to ensure its behaving deterministically in the dbt environment?

@soksamnanglim
Copy link
Contributor Author

Any updates here?

@dbeatty10
Copy link
Contributor

Looking at the results from the most recent CI tests, there are 3 tests that are not passing:

FAILED tests/functional/adapter/grants/test_incremental_grants.py::TestIncrementalGrantsRedshift::test_incremental_grants - AssertionError
FAILED tests/functional/adapter/grants/test_model_table_grants.py::TestModelGrantsTableRedshift::test_table_grants - AssertionError
FAILED tests/functional/adapter/grants/test_snapshot_grants.py::TestSnapshotGrantsRedshift::test_snapshot_grants - AssertionError

The failed assertion for each looks identical to me:

TestIncrementalGrantsRedshift

tests/functional/adapter/grants/test_incremental_grants.py::TestIncrementalGrantsRedshift::test_incremental_grants:

actual_grants = {'select': {'group': ['dbt_test_group_1'], 'user': ['dbt_test_user_1']}}
expected_grants = {'select': {'group': ['dbt_test_group_1'], 👉 'role': ['dbt_test_role_1'] 👈, 'user': ['dbt_test_user_1']}}

TestModelGrantsTableRedshift

tests/functional/adapter/grants/test_model_table_grants.py::TestModelGrantsTableRedshift::test_table_grants:

actual_grants = {'select': {'group': ['dbt_test_group_1'], 'user': ['dbt_test_user_1']}}
expected_grants = {'select': {'group': ['dbt_test_group_1'], 👉 'role': ['dbt_test_role_1'] 👈 , 'user': ['dbt_test_user_1']}}

TestSnapshotGrantsRedshift

tests/functional/adapter/grants/test_snapshot_grants.py::TestSnapshotGrantsRedshift::test_snapshot_grants:

actual_grants = {'select': {'group': ['dbt_test_group_1'], 'user': ['dbt_test_user_1']}}
expected_grants = {'select': {'group': ['dbt_test_group_1'], 👉 'role': ['dbt_test_role_1'] 👈, 'user': ['dbt_test_user_1']}}

@dbeatty10
Copy link
Contributor

I checked that the CI environment has values for all the expected environment variables:

22:15:11  DBT_TEST_USER_1: dbt_test_user_1
22:15:11  DBT_TEST_USER_2: dbt_test_user_2
22:15:11  DBT_TEST_USER_3: dbt_test_user_3
22:15:11  DBT_TEST_GROUP_1: dbt_test_group_1
22:15:11  DBT_TEST_GROUP_2: dbt_test_group_2
22:15:11  DBT_TEST_GROUP_3: dbt_test_group_3
22:15:11  DBT_TEST_ROLE_1: dbt_test_role_1
22:15:11  DBT_TEST_ROLE_2: dbt_test_role_2
22:15:11  DBT_TEST_ROLE_3: dbt_test_role_3
  • added roles and groups to env-setup.sh

I'm not seeing any changes to env-setup.sh, so I'm guessing they weren't pushed on accident.

Before trying to make changes to env-setup.sh, I'm going to take a look at the roles listed within select * from svv_roles r as seen by the CI environment.

@dbeatty10
Copy link
Contributor

It looks to me like the logic within tests/functional/adapter/conftest.py that is meant to create groups and roles if they don't exist is not working, because the following query has 0 rows in the CI environment:

select * from svv_roles r

So to unblock the CI tests, I manually executed in the following within Redshift:

CREATE ROLE dbt_test_role_1;
CREATE ROLE dbt_test_role_2;
CREATE ROLE dbt_test_role_3;

'role' as grantee_type,
r.role_name as grantee,
p.privilege_type
from svv_roles r
Copy link
Contributor

Choose a reason for hiding this comment

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

From pairing with @dbeatty10 - in order to query svv_table_info, this requires:

  • that dbt's user has role <somerole>
  • grant access system table to role <somerole>;

References:


{% endmacro %}

{% macro redshift__apply_grants(relation, grant_config, should_revoke=True) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Overriding default__apply_grants.

return grants_dict

@available
def diff_of_two_nested_dicts(self, dict_a, dict_b):
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of diff_of_two_dicts.


# grant-related methods
@available
def standardize_grants_dict(self, grants_table):
Copy link
Contributor

Choose a reason for hiding this comment

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

{% macro redshift__get_show_grant_sql(relation) %}
{# ------- DCL STATEMENT TEMPLATES ------- #}

{%- macro redshift__get_grant_sql(relation, privilege, grantee_dict) -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

{%- endfor -%}
{%- endmacro -%}

{%- macro redshift__get_revoke_sql(relation, privilege, revokee_dict) -%}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +77 to +78
and tp.table_schema=replace(split_part('{{ relation }}', '.', 2), '"', '')
and tp.table_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Relation class attributes (like here and here) rather than splitting on dot separators.

Suggested change
and tp.table_schema=replace(split_part('{{ relation }}', '.', 2), '"', '')
and tp.table_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and tp.table_schema = '{{ relation.schema }}'
and tp.table_name = '{{ relation.identifier }}'

Comment on lines +94 to +95
and rp.namespace_name=replace(split_part('{{ relation }}', '.', 2), '"', '')
and rp.relation_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use Relation class attributes (like here and here) rather than splitting on dot separators.

Suggested change
and rp.namespace_name=replace(split_part('{{ relation }}', '.', 2), '"', '')
and rp.relation_name=replace(split_part('{{ relation }}', '.', 3), '"', '')
and rp.namespace_name = '{{ relation.schema }}'
and rp.relation_name = '{{ relation.identifier }}'

@colin-rogers-dbt colin-rogers-dbt added the community This PR is from a community member label Jul 16, 2024
@pgvillena
Copy link

Any updates here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes community This PR is from a community member ok to test user docs [docs.getdbt.com] Needs better documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ADAP-474] [Feature] Accept users, groups, or roles in grants config on resource
6 participants