-
Notifications
You must be signed in to change notification settings - Fork 611
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
Make granting on ALL TABLES IN SCHEMA idempotent #564
Make granting on ALL TABLES IN SCHEMA idempotent #564
Conversation
Since I am obviously no DBA that can perform powerful magic with SQL the SELECT statement I devised may not be optimal. But it certainly is better than having the GRANT statement executed on every single Puppet run, which is the current situation. |
The failed Travis checks say things like:
I don't think this has anything to do with my changes here. |
default => "SELECT 1 WHERE ${unless_function}('${role}', | ||
false => undef, | ||
'custom' => $custom_unless, | ||
default => "SELECT 1 WHERE ${unless_function}('${role}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this end up being
select 1 where select 1 from ( select …
is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_pe
bit is caused by recent changes in puppetlabs-concat. The fix is at puppetlabs/puppetlabs-concat#270
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igalic, $custom_unless
contains the SQL query that is passed to Postgresql_psql
(via $_unless
). It will be executed as is by Postgresql_psql
, just like the SQL query constructed in the default case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack ack ack, didn't see $custom_privilege
variable name, and thought it was assigned to $_unless
as $unless_function
FWIW, we have this change deployed in our production environments and it works fine. |
@cmurphy I am being forward and pinging you so that this PR possibly gets some attention :) |
Define a proper SELECT statement to feed into Postgresql_psql's `unless` parameter that checks if there are any tables in the specified schema for which the specified role *does not* have the specified privilege. Only then allow the GRANT statement to be executed. For details see comments. Note that this, too, suffers from the problem that there is no feasible way to check if a role has ALL PRIVILEGES on a table in plain SQL. By terrible convention the INSERT privilege represents ALL PRIVILEGES here.
4c9eed8
to
dbbb7aa
Compare
@tphoney pinging you also! |
Make granting on ALL TABLES IN SCHEMA idempotent
Great, thanks!
|
…_all_tables Make granting on ALL TABLES IN SCHEMA idempotent
Define a proper SELECT statement to feed into Postgresql_psql's
unless
parameter that checks if there are any tables in the specifiedschema for which the specified role does not have the specified
privilege. Only then allow the GRANT statement to be executed. For
details see comments.
Note that this, too, suffers from the problem that there is no feasible
way to check if a role has ALL PRIVILEGES on a table in plain SQL. By
terrible convention the INSERT privilege represents ALL PRIVILEGES here.