Parameter ensure on postgresql::server::grant and postgresql::server::database_grant

[georgehansper]

This change updates the defined type postgresql::server::grant to introduce a new parameter:

ensure => present
-- or --
ensure => absent

ensure => present preserves the existing behaviour, using GRANT to add privileges

ensure => absent uses REVOKE to remove privileges

The SQL associated with the unless clause has been updated to work correctly, in general.

[hasegeli]

Neat work! I went through it, but haven't had time for the very long query and tests. I can look at them later, if you like.

[hasegeli]

This is unnecessary with the datatype.

[georgehansper]

I just copied this structure from grant_role.pp

I suspect it pre-dates the introduction of strict typing.

I will remove the redundant code.

[hasegeli]

SELECT 1 WHERE ${unless_function}('${role}', '${_granted_object}', '${unless_privilege}') = ${unless_is}

would be cleaner.

[georgehansper]

Ok, I'll update the code

[hasegeli]

I find this query and the following unnecessarily complicated. First of all, the logic is reverse. It returns a row when there are no matching tables. We can use onlyif instead of unless and just return rows for tables.

We can use NOT EXISTS instead of EXCEPT with list of privileges. It would be cleaner and better optimised.

GROUP BY on the inner most query is unnecessary. count(privilege_type) could be just count(*).

I believe HAVING t.count is an unusual syntax. The subquery could have SELECT count(*) instead. Thank you for showing me this synax though. SQL still surprises me after many years.

Upper case alias is not good style.

In conclusion, I suggest this:

SELECT 1
FROM pg_catalog.pg_tables AS t,
    (VALUES ('SELECT'), ('UPDATE'), ('INSERT'), ('DELETE'), ('TRIGGER'), ('REFERENCES'), ('TRUNCATE')) AS p(privilege_type)
WHERE t.schemaname = '${schema}'
    AND NOT EXISTS (
        SELECT 1
        FROM information_schema.role_table_grants AS g
        WHERE g.grantee = '${role}'
            AND g.table_schema = t.schemaname
            AND g.table_name = t.tablename
            AND g.privilege_type = p.privilege_type
    )

[georgehansper]

I would much rather use 'onlyif' instead of 'unless' for these queries, but for 2 things:

• The existing puppet code was written this way. It uses $unless_function = 'custom' plus this code to determine whether or not to apply the GRANT. Adding the logic for $onlyif_function = 'custom' would make ALL TABLES IN SCHEMA the exception to this approach
• The variable $onlyif_function is currently only used to test for the existence of something - a table, a language. Re-using it for a different purpose adds complexity to this logic.

It's quite feasible to use onlyif, but it's really just simplifying the SQL and making the puppet code more complex.

Let me know your thoughts on this trade-off. I'll change it if you ask.

On the rest of the SQL, the logic around GRANT ALL ON ALL TABLES IN SCHEMA is quite complicated. I want to capture the case where, say one table has one privilege missing. So it is necessary to account for all privileges, not just any one. Hence the count(privilege_type).
This has to be done on a per-table basis, which is why the GROUP BY is there.

count(*) would suffice, I just put privilege_type in there for readability, since that is what we are interested in.

I'm happy to substitute your version.

[hasegeli]

I see the reason to keep the logic reversed. We can encapsulate whole query inside another NOT EXISTS to keep it that way, so:

SELECT 1
WHERE NOT EXISTS (
    SELECT 1
    FROM pg_catalog.pg_tables AS t,
        (VALUES
            ('SELECT'),
            ('UPDATE'),
            ('INSERT'),
            ('DELETE'),
            ('TRIGGER'),
            ('REFERENCES'),
            ('TRUNCATE')
        ) AS p(privilege_type)
    WHERE t.schemaname = '${schema}'
        AND NOT EXISTS (
            SELECT 1
            FROM information_schema.role_table_grants AS g
            WHERE g.grantee = '${role}'
                AND g.table_schema = t.schemaname
                AND g.table_name = t.tablename
                AND g.privilege_type = p.privilege_type
        )
)

[georgehansper]

I've updated the code use SELECT 1 WHERE NOT EXISTS instead of HAVING count(*)=0

I think this makes it a bit more readable.

[georgehansper]

I've updated the code to include the improvements you have suggested.
Is there anything else that needs doing before these changes can be merged in?

[hasegeli]

I don't have rights on the repository. I am just doing reviews to keep it clean. I guess someone from Puppetlabs would pick this up. This is a useful feature in my opinion.

[sammcj]

Thanks, this would be very useful to have George, let us know if there's anything we can do to help get this merged @puppetlabs

[georgehansper]

@hunner thanks for merging #894

Is there anything more I can do to help get this pull reques (#891) be merged in ?
I also have 2 more pull requests: #896 and #897 which are smaller and simpler than #891

Please let me know if there is anything I can do to help things along.

[georgehansper]

Hi @hasegeli

I've made one small change to this pull-request, which is to check that the role exits before revoking DB privileges, thus preventing this operation from failing

This is desirable because a common usage pattern is the revoke all privileges prior to dropping a role. On the next run, after the role is gone, we don't want the ensure=>absent to fail.

The ALL TABLES IN SCHEMA logic does not fail in this way, because it uses a custom SQL query instead of a function like has_database_privilege() which is used by the DATABASE logic.

Thanks for your time.
George

[georgehansper]

I've updates this code to:

• remove a merge conflict that arose due to changes introduced after the PR was created
• fix a failing unit test associated with the same change

All tests are now passing, and there are no merge conflicts.
No functional changes were made to the code (whitespace changes only).

[sammcj]

👍

[sammcj]

FYI - We logged a Puppet Enterprise support ticket to try and get some movement on getting @georgehansper's MRs actioned but they closed the support request as 'SOLVED' stating its a different @puppet team that supports the modules, it's in their backlog and they'll review it next sprint but that there's nothing else Puppet support can do.

[georgehansper]

Issue is MODULES-5634

[sammcj]

Hey @tphoney, thanks for merging #896 - would you mind taking a look this as well?

[georgehansper]

Hi @willmeek

I've fixed the merge-conflict on this branch, so the needs-rebase label can be removed.

[david22swan]

@hunner Could you take a look at this pr please?

[hasegeli]

This has passed 7 months. Any progress?

[tphoney]

+1 from me. @hunner can you have a look, as a final sign off

[david22swan]

@hunner Per TP's request, could you look over this PR?

[david22swan]

@georgehansper This is currently failing the inbuilt Rubocop checks. In order for it to be merged it would first need to be updated to match the set standards. Some example rubocop commands are shown below:

Run command:
bundle exec rubocop

Flags:
-a : run autocorrect
--display-cop-names : add so that each error shows its flag name

Disable Commands:
Disable rubocop over a block off code

# rubocop:disable Lint/UselessAssignment
    {Code Block}
# rubocop:enable Lint/UselessAssignment

Disable rubocop for a single line:

{Code Line} # rubocop:disable RSpec/InstanceVariable

[georgehansper]

@david22swan thanks for the pointers re rubocop.

I've started looking at this, and I expect to have these issues resolved soon.

[georgehansper]

@david22swan @hunner I've resolved the merge conflicts and made rubocop happy again.

[georgehansper]

Thanks all