dbt-labs / dbt-postgres

Apache License 2.0
31 stars 12 forks source link

[CT-3173] [Bug] Postgres + Grants + Username-With-Dashes does not work #55

Closed tomans-spirent closed 3 months ago

tomans-spirent commented 1 year ago

Is this a new bug in dbt-core?

Current Behavior

When I configure grants in dbt for a postgres database and a db role like 'foo-bar' I get postgres syntax errors because the name is not quoted.

If I try to quote it like "\"foo-bar\"" it works for the grants part but breaks in the incremental grants part. Looks like some places do not strip and some places do strip those extra quotes I added to my db role name.

Expected Behavior

I should not have to put in extra quotes - dbt should be able to create the appropriate database syntax for supported role names.

Steps To Reproduce

Create a postgres role with dashes in the name.

Try to have dbt assign grants to that user - it will break. Add manual quotes - it will work.

Try to make a incremental model with those same grants - it will break.

Relevant log output

No response

Environment

- OS: macOS ventura 13.6
- Python: 3.10.12
- dbt: 1.6.2

Which database adapter are you using with dbt?

postgres

Additional Context

No response

tomans-spirent commented 1 year ago

As an update and workaround to this issue - I have been able to leverage the post-hook feature of models to apply the grants myself - where I just make sure I properly quote my db role name.

dbeatty10 commented 1 year ago

Thanks for reporting this @tomans-spirent !

Could you try this workaround and see if it also works for you?

tomans-spirent commented 1 year ago

Seems like a similar issue - but no such luck if I add that revoke macro to my project.

Some extra context from my dbt run log output:

Database Error in model my_model (models/my_group/my_model.sql)
  syntax error at or near "-"
  LINE 4: ...db"."schema"."my_model" to grantee-with-dash;

With the following in my dbt_project.yml

models:
  my_group:
    +grants:
      select: ["grantee-with-dash"]
dbeatty10 commented 1 year ago

Ah, sorry I only gave you the revoke-side of the story @tomans-spirent ! 😅

Related issues

It looks to me like there are two different (but very related) things going on:

  1. dbt-postgres/redshift doesn't quote username when granting (#8751)
  2. dbt-postgres/redshift doesn't quote username when revoking (#6444)

See below for two different approaches you can try.

Option 1: Add more quotes

Does it work if you add double quotes within your configuration?

For example, like this for models YAML configuration within dbt_project.yml (note both single and double quotes):

models:
  my_group:
    +grants:
      select: ['"grantee-with-dash"']

If this does work for you, I think you'll still need to that macro you just added to handle the revoke side of things.

Option 2: Add a grant-related macro

https://github.com/dbt-labs/dbt-redshift/pull/282 has a proposed solution for both of those issues listed above (which is converted from being redshift-specific to default below):

{%- macro default__get_grant_sql(relation, privilege, grantees) -%}
    grant {{ privilege }} on {{ relation }} to
    {% for grantee in grantees %}
    {{ adapter.quote(grantee) }}
    {% if not loop.last %},{% endif %}
    {% endfor %}
{%- endmacro -%}

{%- macro default__get_revoke_sql(relation, privilege, grantees) -%}
    revoke {{ privilege }} on {{ relation }} from
    {% for grantee in grantees %}
    {{ adapter.quote(grantee) }}
    {% if not loop.last %},{% endif %}
    {% endfor %}
{%- endmacro -%}

Trade-offs

I didn't try these out myself, but I'm assuming it requires all grants to be case-sensitive when they are written in YAML and config for dbt models. So that trade-off would probably work for you, but might break others if we were to just roll these macros out everywhere as-is.

Wanna give one or both of these ideas a try and let us know how it goes and what you think?

tomans-spirent commented 1 year ago

First option: works EXCEPT for incremental table grants. (same error as above)

Second option: Works for views, materialized, and incremental tables!


I have added a file: macros/github-issue-8751.sql

With the contents from your comment and it patches the issue for me locally


Interesting point re: the case sensitivity and existing backwards compatibility. I tested against postgres and indeed it was case sensitive.

So: I can verify the second workaround works with the caveats you expected.

dbeatty10 commented 1 year ago

Thanks for trying out both of those options with a host of types (view, table, & incremental) @tomans-spirent ! 🏆

I'm going to label this as Refinement for us to decide whether to roll-out the second option as the default or not. One thing for us to consider is if we can/should add a quote_grants config for grants (or something else similar to quote_columns). If we add some kind of quoting-related config, hopefully it would fit together with https://github.com/dbt-labs/dbt-core/issues/2986.

Fleid commented 6 months ago

Hi there! The priority for this issue is too low for us to take a stab at it in the short term. Sorry about that. We do want to leave the issue open for now though. We will bump it up if others also encounter the same problem. Thanks for your contribution!