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 database role support #1207

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

Conversation

seediang
Copy link

resolves #1206
docs dbt-labs/docs.getdbt.com/#

Problem

dbt-snowflake only supports granting to Account-level database roles. Snowflake supports granting to other objects including Database Roles and Shares. However, dbt-snowflake does not.

Solution

The solution allow dbt-snowflake to manage database roles as well as existing account level roles. The configuration is extended to include a nested object_type level but also supports the original style. The original style is mapped into the new style under a "role" key.

models:
    - name: MODEL_NAME_1
      config:
        grants:
            # New syntax option
            select:
                role: [ROLE_NAME_1, ROLE_NAME_2, ...]
                database_role: [DATABASE_ROLE_NAME_1, DATABASE_ROLE_NAME_2, ...]
            insert:
                role: [ROLE_NAME_1, ROLE_NAME_2, ...]
                database_role: [DATABASE_ROLE_NAME_1, DATABASE_ROLE_NAME_2, ...]
    - name: MODEL_NAME_2
      config:
        grants:
            # Also preserve existing syntax for full backwards compatibility
            select: [ROLE_NAME_1, ROLE_NAME_2, ...]

The solution has added a number of python functions to the adapter. Originally these were implemented as macros but the logic was also required in python for the automated tests. Rather than duplicate the logic it was moved into python and made available in the jinja context.

The new functions are below. I believe in the long term these would make sense migrating into the dbt-adapter project.

  • standardize_grant_config
  • diff_of_grants

The solution also introduces three new macros, which are similar to existing ones but take an additional object_type parameter ie role or database_role:

  • get_grant_sql_by_type
  • get_revoke_sql_by_type
  • get_dcl_statement_list_by_type

Areas that need advice

  1. The existing test_grants.py has been patched to make use of the new functions and the tests are passing. This is not the cleanest and any suggestion for improvement would be appreciated.
  2. The best way to extended the current tests to support roles in the new style.
  3. The best way to include database roles in the functional tests.

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 12, 2024

class BaseGrantsSnowflake(BaseGrantsSnowflakePatch, OrigBaseGrants):
@pytest.fixture(scope="session", autouse=True)
def ensure_database_roles(project):
Copy link
Author

Choose a reason for hiding this comment

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

To allow tests that use database_roles, this fixture has been added at session level to create 3 database roles in a similar style to the account-level roles that exist in the testing framework.

@seediang seediang marked this pull request as ready for review October 20, 2024 17:47
@seediang seediang requested a review from a team as a code owner October 20, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Support for database roles
1 participant