Support target_role in default_privileges

[fish-face]

ALTER DEFAULT PRIVILEGES supports the FOR ROLE <target_role> argument,
without which the statement applies only to objects created by the
current role, which may not be most useful.

Support specifying the target role.

Includes a unit test (but no integration test) and updates the inline docs - it looks like you only update REFERENCE.md for release?

[CLAassistant]

All committers have signed the CLA.

[puppet-community-rangefinder]

postgresql::server::default_privileges is a type

that may have no external impact to Forge modules.

This module is declared in 70 of 578 indexed public Puppetfiles.

---

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

[david22swan]

@fish-face Everything looks good at a first glance, but I would prefer an acceptance test be added for this as my PostgreSQL knowledge is not the best and thus would like the actual functionality to be checked.
In regards the the REFERENCE.md it is automatically created on release from the documentation within the manifests themselves.

[fish-face]

@david22swan I'd be happy to add an acceptance test, but was unable to run the suite. The only instructions in the README result in everything failing - here's an example:

RuntimeError:
       apply manifest failed
       `LC_ALL=en_US.UTF-8  puppet apply /tmp/manifest_20210922_2275083_1f6kzlq.pp --trace --modulepath /home/clesueur/repos/puppetlabs-postgresql/spec/fixtures/modules --detailed-exitcodes`
       with exit code 1 (expected: [0, 2])
       ====== Start output of failed Puppet apply ======
       Error: Parameter user failed on Exec[postgresql_initdb]: Only root can execute commands as other users (file: /home/clesueur/repos/puppetlabs-postgresql/spec/fixtures/modules/postgresql/manifests/server/initdb.pp, line: 140)

I guess I can try as root, but I am a bit wary that this might be trying to run commands that should be executing in a VM on the my actual machine. Is there anything beyond involving vagrant and virtualbox that needs to be done to set things up?

[fish-face]

One other question about your and the other maintainers' workload at the moment - do you know how soon you are able to look at this and the other two PRs I created (or how often, if they need some iteration). I ask because we need to make some decisions about sequencing things which might be different depending on whether we anticipate these changes being in the official module or only on my fork at the start of next month. I understand if this isn't really predictable but thought I'd ask in case it was :)

[david22swan]

@fish-face Apologies, the test instructions are out of date and need to be updated. To properly run the test use the following set of commands:

# Setup the environment
bundle install --path .bundle/
bundle exec rake spec_prep

# Provision a machine
bundle exec rake 'litmus:provision[docker, litmusimage/ubuntu:20.04]'

# Setup the machine
bundle exec rake 'litmus:install_agent[puppet7-nightly]'
bundle exec rake litmus:install_module

# Run the tests
bundle exec rake litmus:acceptance:parallel

In regards to your question, we have a community day where we go through all current PRs every monday, outside of that it can be more spotty as we have internal work to do but we will try to look in and respond when we can.

[fish-face]

Hi @david22swan thanks for the help there. This and the other two PRs about default privileges now have acceptance tests. There'll be some conflicts between this and the other two (but the third is based on the second so will be fine). It should be easy to resolve though!

BTW I had to fiddle a little to get the instructions to work: it runs out that the ubuntu image doesn't work on my Fedora machine, I think due to cgroups stuff (that image mounts some cgroups file from /sys/). I can't remember how I solved the Fedora cgroups issue that arose last year on this machine so it could even also depend on local configuration. Anyway it was fine with the litmus centos-based image - just mentioning in case it ever comes up again and could help someone else. The symptom was that the provisioning failed with an error message like Container <sha> is not running and the container logs complained of a readonly filesystem.

[david22swan]

@fish-face Hy, sorry but we've got a small error with your change. It's not a big thing but you are failing a rubocop syntex check:
spec/acceptance/server/default_privileges_spec.rb:177:1: C: [Correctable] Layout/TrailingWhitespace: Trailing whitespace detected.
To run this check yourself just do: bundle exec rake rubocop
Sorry for not mentioning it with the other test instructions.
Other than that everything look's fine so far. A good few of the acceptance blocks have passed and the remaining ones are looking good.

[fish-face]

I was just trying to get to the test logs from the email so thanks for that! I should be able to fix it up quickly.

…

On Mon, 4 Oct 2021, 09:46 david22swan, ***@***.***> wrote: @fish-face <https://github.com/fish-face> Hy, sorry but we've got a small error with your change. It's not a big thing but you are failing a rubocop syntex check: spec/acceptance/server/default_privileges_spec.rb:177:1: C: [Correctable] Layout/TrailingWhitespace: Trailing whitespace detected. To run this check yourself just do: bundle exec rake rubocop Sorry for not mentioning it with the other test instructions. Other than that everything look's fine so far. A good few of the acceptance blocks have passed and the remaining ones are looking good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1297 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7SILR3C6ZAOB36VJSKTJ3UFFSUXANCNFSM5ECQ2W3Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[fish-face]

Should be fixed now (also ran rubocop on the other PR branches)

[david22swan]

@fish-face Seeing some failures on Debian OS's, they seem to be working fine on all other platforms. If you could get these fixed then I would be happy to merge.
The failures are consistent and I've linked them below:

Failures:

  1) postgresql::server::default_privileges: grants default privileges to a user on a specific target role
     On host `localhost:2222'
     Failure/Error: LitmusHelper.instance.run_shell("cd /tmp; su #{shellescape(user)} -c #{shellescape(psql)}", acceptable_exit_codes: exit_codes, &block)
     RuntimeError:
       shell failed
       `cd /tmp; su psql_grant_role_tester -c psql\ --command\=\"SET\ client_min_messages\ \=\ \'error\'\;\ SELECT\ 1\ FROM\ pg_default_acl\ a\ LEFT\ JOIN\ pg_namespace\ AS\ b\ ON\ a.defaclnamespace\ \=\ b.oid\ WHERE\ \'psql_grant_role_tester\=arwdDxt/target_role_user\'\ \=\ ANY\ \(defaclacl\)\ AND\ nspname\ \=\ \'public\'\ AND\ defaclobjtype\ \=\ \'r\'\;\"\ --db\=grant_role_test`
       ======
       [{"target"=>"localhost:2222", "action"=>"command", "object"=>"cd /tmp; su psql_grant_role_tester -c psql\\ --command\\=\\\"SET\\ client_min_messages\\ \\=\\ \\'error\\'\\;\\ SELECT\\ 1\\ FROM\\ pg_default_acl\\ a\\ LEFT\\ JOIN\\ pg_namespace\\ AS\\ b\\ ON\\ a.defaclnamespace\\ \\=\\ b.oid\\ WHERE\\ \\'psql_grant_role_tester\\=arwdDxt/target_role_user\\'\\ \\=\\ ANY\\ \\(defaclacl\\)\\ AND\\ nspname\\ \\=\\ \\'public\\'\\ AND\\ defaclobjtype\\ \\=\\ \\'r\\'\\;\\\"\\ --db\\=grant_role_test", "status"=>"failure", "value"=>{"stdout"=>"", "stderr"=>"su: user psql_grant_role_tester does not exist\n", "merged_output"=>"su: user psql_grant_role_tester does not exist\n", "exit_code"=>1, "_error"=>{"kind"=>"puppetlabs.tasks/command-error", "issue_code"=>"COMMAND_ERROR", "msg"=>"The command failed with exit code 1", "details"=>{"exit_code"=>1}}}}]
       
     # ./vendor/bundle/ruby/2.7.0/gems/puppet_litmus-0.30.0/lib/puppet_litmus/puppet_helpers.rb:237:in `block in run_shell'
     # ./vendor/bundle/ruby/2.7.0/gems/honeycomb-beeline-2.7.0/lib/honeycomb/client.rb:62:in `start_span'
     # ./vendor/bundle/ruby/2.7.0/gems/puppet_litmus-0.30.0/lib/puppet_litmus/puppet_helpers.rb:221:in `run_shell'
     # ./spec/spec_helper_acceptance_local.rb:75:in `psql'
     # ./spec/acceptance/server/default_privileges_spec.rb:178:in `block (2 levels) in <top (required)>'

  2) postgresql::server::default_privileges: revokes default privileges from a user on a specific target role
     On host `localhost:2222'
     Failure/Error: LitmusHelper.instance.run_shell("cd /tmp; su #{shellescape(user)} -c #{shellescape(psql)}", acceptable_exit_codes: exit_codes, &block)
     RuntimeError:
       shell failed
       `cd /tmp; su psql_grant_role_tester -c psql\ --command\=\"SET\ client_min_messages\ \=\ \'error\'\;\ SELECT\ 1\ FROM\ pg_default_acl\ a\ LEFT\ JOIN\ pg_namespace\ AS\ b\ ON\ a.defaclnamespace\ \=\ b.oid\ WHERE\ \'psql_grant_role_tester\=arwdDxt/target_role_user\'\ \=\ ANY\ \(defaclacl\)\ AND\ nspname\ \=\ \'public\'\ AND\ defaclobjtype\ \=\ \'r\'\;\"\ --db\=grant_role_test`
       ======
       [{"target"=>"localhost:2222", "action"=>"command", "object"=>"cd /tmp; su psql_grant_role_tester -c psql\\ --command\\=\\\"SET\\ client_min_messages\\ \\=\\ \\'error\\'\\;\\ SELECT\\ 1\\ FROM\\ pg_default_acl\\ a\\ LEFT\\ JOIN\\ pg_namespace\\ AS\\ b\\ ON\\ a.defaclnamespace\\ \\=\\ b.oid\\ WHERE\\ \\'psql_grant_role_tester\\=arwdDxt/target_role_user\\'\\ \\=\\ ANY\\ \\(defaclacl\\)\\ AND\\ nspname\\ \\=\\ \\'public\\'\\ AND\\ defaclobjtype\\ \\=\\ \\'r\\'\\;\\\"\\ --db\\=grant_role_test", "status"=>"failure", "value"=>{"stdout"=>"", "stderr"=>"su: user psql_grant_role_tester does not exist\n", "merged_output"=>"su: user psql_grant_role_tester does not exist\n", "exit_code"=>1, "_error"=>{"kind"=>"puppetlabs.tasks/command-error", "issue_code"=>"COMMAND_ERROR", "msg"=>"The command failed with exit code 1", "details"=>{"exit_code"=>1}}}}]
       
     # ./vendor/bundle/ruby/2.7.0/gems/puppet_litmus-0.30.0/lib/puppet_litmus/puppet_helpers.rb:237:in `block in run_shell'
     # ./vendor/bundle/ruby/2.7.0/gems/honeycomb-beeline-2.7.0/lib/honeycomb/client.rb:62:in `start_span'
     # ./vendor/bundle/ruby/2.7.0/gems/puppet_litmus-0.30.0/lib/puppet_litmus/puppet_helpers.rb:221:in `run_shell'
     # ./spec/spec_helper_acceptance_local.rb:75:in `psql'
     # ./spec/acceptance/server/default_privileges_spec.rb:190:in `block (2 levels) in <top (required)>'

Finished in 11 minutes 54 seconds (files took 1.23 seconds to load)
53 examples, 2 failures, 10 pending

Failed examples:

rspec ./spec/acceptance/server/default_privileges_spec.rb:174 # postgresql::server::default_privileges: grants default privileges to a user on a specific target role
rspec ./spec/acceptance/server/default_privileges_spec.rb:185 # postgresql::server::default_privileges: revokes default privileges from a user on a specific target role

[fish-face]

Damn, looks like this was masked by another test creating the user and not removing it again. I have pushed the following change to the acceptance test:

@@ -84,6 +84,10 @@ describe 'postgresql::server::default_privileges:' do
       $target_user = #{target_user}
       $target_password = #{target_password}
 
+      user {$user:
+        ensure => present,
+      }
+
       class { 'postgresql::server': }
 
       postgresql::server::role { $user:
@@ -120,6 +124,10 @@ describe 'postgresql::server::default_privileges:' do
       $target_user = #{target_user}
       $target_password = #{target_password}
 
+      user {$user:
+        ensure => present,
+      }
+
       class { 'postgresql::server': }
 
       postgresql::server::role { $user:

which fixes the broken test, although it propagates the issue of leaving a user around. I noticed there's another test isolation issue: if I run this spec file on its own on a freshly provisioned target, psql isn't installed, but by the time we get to this test it normally is (this was already an issue so I don't propose to fix it :P)

[fish-face]

And that didn't work but hopefully this will (the issue is compounded because if I try to run just the one acceptance test on a freshly provisioned container, they fail because psql isn't installed. I can't see anything different in the other tests which use it, but I was able to first run db_spec and then default_privileges_spec without an error on the latest iteration.)

[david22swan]

@fish-face Thank's for getting those fixed, the current failures are unrelated to your changes so I feel good about going ahead and merging them.
Thank's for your work and looking forward to getting your other PRs across the line.

[david22swan]

LGTM