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-3173] [Bug] Postgres + Grants + Username-With-Dashes does not work #55

Closed
1 of 2 tasks
tomans-spirent opened this issue Sep 29, 2023 · 7 comments
Closed
1 of 2 tasks
Labels
bug Something isn't working grants Issues related to dbt's grants functionality quoting Issues related to dbt's quoting behavior refinement Needs refining prior to being ready

Comments

@tomans-spirent
Copy link

tomans-spirent commented Sep 29, 2023

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

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 tomans-spirent added bug Something isn't working triage labels Sep 29, 2023
@github-actions github-actions bot changed the title [Bug] Postgres + Grants + Username-With-Dashes does not work [CT-3173] [Bug] Postgres + Grants + Username-With-Dashes does not work Sep 29, 2023
@tomans-spirent
Copy link
Author

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
Copy link
Contributor

Thanks for reporting this @tomans-spirent !

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

@dbeatty10 dbeatty10 removed their assignment Oct 4, 2023
@tomans-spirent
Copy link
Author

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
Copy link
Contributor

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

dbt-labs/dbt-redshift#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
Copy link
Author

tomans-spirent commented Oct 4, 2023

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
Copy link
Contributor

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 dbt-labs/dbt-core#2986.

@dbeatty10 dbeatty10 added refinement Needs refining prior to being ready and removed triage labels Oct 4, 2023
@dbeatty10 dbeatty10 transferred this issue from dbt-labs/dbt-core Apr 10, 2024
@Fleid Fleid added triage and removed triage labels Apr 11, 2024
@Fleid
Copy link

Fleid commented Apr 18, 2024

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!

@dbeatty10 dbeatty10 added quoting Issues related to dbt's quoting behavior grants Issues related to dbt's grants functionality labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working grants Issues related to dbt's grants functionality quoting Issues related to dbt's quoting behavior refinement Needs refining prior to being ready
Projects
None yet
4 participants