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

[CT-581] [Feature] Grants as Node Configs #5189

Closed
3 tasks
nathaniel-may opened this issue Apr 28, 2022 · 4 comments · Fixed by #5230
Closed
3 tasks

[CT-581] [Feature] Grants as Node Configs #5189

nathaniel-may opened this issue Apr 28, 2022 · 4 comments · Fixed by #5230
Assignees
Labels
enhancement New feature or request

Comments

@nathaniel-may
Copy link
Contributor

nathaniel-may commented Apr 28, 2022

Description

This is the first in a two ticket series. #5263 is the second ticket. They will be merged sequentially, but both are required for this feature to be exposed to users.

Today users often configure a post_hook to grant permissions on models, seeds, and snapshots:

{{ config(post_hook = 'grant select on {{ this }} to role reporter') }}

These two tickets aim to make it easier to specify grants allowing it to be configured directly both in dbt_project.yml as well as in source files:

# dbt_project.yml
models:
  export:
    +grants:
      select: ['reporter', 'bi']
-- SQL
{{ config(grants = {'select': ['other_user']}) }}

These grant configs will not necessarily look the same for each and every warehouse. The logic to generate the sql from these configs can be overriden by adapters.

Implementation

  • a "grants" config should be added to model, seed, and snapshot nodes. The value of this config is a dictionary.
  • merge behavior for grants should be to override / clobber not append.
  • test that projects can define grants in 1) the dbt_project.yml, 2) models, seeds, and snapshots, and 3) both the dbt_project.yml and models, seeds, and snapshots with the correct merge behavior.
@nathaniel-may nathaniel-may added the enhancement New feature or request label Apr 28, 2022
@github-actions github-actions bot changed the title [Feature] Grants as Node Configs [CT-581] [Feature] Grants as Node Configs Apr 28, 2022
@jtcohen6
Copy link
Contributor

Looking grand! Two things I picked up on and want to call out:

The default implementation should throw or return an empty string because it is expected to be overridden by adapters.

Our attitude here depends on how much of a standard exists across our relevant databases. It's likely that many many databases support grant <privilege> on <object> to <recipient>. In that case, we should make this the default__ implementation, within the global project, and only require adapters to reimplement if they vary from that standard.

The value of this config is a dictionary.

This is how I have it in the original proposal and in #5090. I'm ~85% sure it's the right data structure. There are two things that give me hesitation:

  1. Some privileges have wonky names. E.g. 'roles/viewer' on BigQuery. This still works (right?), it's just a little uglier:
grants:
  'roles/viewer': ['recipient_one', 'recipient_two']

# but should we instead opt for:

grants:
  - privilege: 'roles/viewer' # or just call this attribute 'name'?
    recipients: ['recipient_one', 'recipient_two']
  1. On most databases, it's possible to grant multiple privileges to multiple people at the same time: grant select, manage, something_else on object this_table to person_a, person_b, group_c. But if we make grants a list, I'm not sure how we can support merging/clobbering. I think it would be better to just require a bit of duplication, in cases where you want to grant multiple privileges to the same group of people.

@nathaniel-may
Copy link
Contributor Author

✅ default implementation - edited.

  1. Since we don't control warehouse naming conventions I figured those would be completely overridable. For an extreme example, dbt_project.yml for FunkyDB could read something like:
grants:
  dance_moves: [1, 15, 79]
  dancers: "engineering_all"
  max_allowed_intensity: 9000

and the adapter override would have the freedom to map that directly to the correct SQL grant statement for FunkyDB.

  1. I don't think I totally understand. Since our merge strategy is to clobber here, I feel like it shouldn't matter if it applies to multiple recipients. I'm fine with requiring duplication if we need to, but I'd like to understand better.

@jtcohen6
Copy link
Contributor

jtcohen6 commented May 2, 2022

Good point re: 2! I had been thinking about a merge strategy of "merge" (updating specific keys, leaving keys without updates in place). If our merge strategy is indeed to clobber, it doesn't matter.

@nathaniel-may
Copy link
Contributor Author

Moved the second half of this description to #5263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants