Unterminated quoted string when creating read user

[waipeng]

puppetdb in default config will create a read-only user, however there is a bug[1] with the syntax to set the default read grant.

Fix it with help from comments[2].

[1] #330 (comment)
[2] #339 (comment)

[CLAassistant]

All committers have signed the CLA.

[waipeng]

There are no tests, but I can confirm this works.

I am checking out the situation with the tests.

[smortex]

It looks like the format depend on the actual role names: puppetdb-read is quoted but puppetdb is not:

puppetdb=# SELECT pg_default_acl.defaclacl::text FROM pg_default_acl;
            defaclacl             
----------------------------------
 {"\"puppetdb-read\"=U/puppetdb"}
 {"\"puppetdb-read\"=X/puppetdb"}
 {"\"puppetdb-read\"=r/puppetdb"}
(3 lignes)

I think the - in puppetdb-read trigger the quoting. I suggested some changes to make the matching more robust in any circumstances.

[smortex]

Out of curiosity, I do not alter the default roles and have not hit this issue. Can you share the configuration you use that trigger the issue?

[waipeng]

Out of curiosity, I do not alter the default roles and have not hit this issue. Can you share the configuration you use that trigger the issue?

We are seeing this on Ubuntu 22.04, Puppet Agent 7.26.0, Puppet Server 7.13.0.

Node hiera is pretty simple

puppetdb::listen_address: '0.0.0.0'
puppetdb::postgres_version: '12'
postgresql::server::contrib::package_name: 'postgresql-contrib'
puppetdb::java_args:
  '-Xmx': '2g'

A puppet run with -d shows the following:

Debug: Executing with uid=postgres gid=postgres: 'psql -d puppetdb -t -X -c "SELECT COUNT(*) FROM (SELECT                                                                                                                                                                                                                                                                                                                                   
                  ns.nspname,                                                                                                                                                                                         
                  acl.defaclobjtype,                                                                                                                                                                                                                                                                                                                                                                                                        
                  acl.defaclacl                                                                                                                                                                                       
                FROM pg_default_acl acl                                                                                                                                                                               
                JOIN pg_namespace ns ON acl.defaclnamespace=ns.oid                                                                                                                                                    
                WHERE acl.defaclacl::text ~ '.*\\\"puppetdb-read\\\"=r/puppetdb\\".*'                                                                                                                                 
                AND nspname = 'public') count"'                                                                                                                                                                       
Error: /Stage[main]/Puppetdb::Database::Postgresql/Puppetdb::Database::Read_only_user[puppetdb-read]/Puppetdb::Database::Default_read_grant[puppetdb grant read permission on new objects from puppetdb to puppetdb-read]/Postgresql_psql[grant default select permission for puppetdb-read]: Could not evaluate: Error evaluating 'unless' clause, returned pid 11837 exit 2: 'sh: 8: Syntax error: Unterminated quoted string             
'    

Please let me know if you need more information on replicating this. Thanks for your help!

[smortex]

lol, I was forgetting that the unbalanced quote was the reason for this PR 🤦. In our infra, we have manage_dbserver => false, that's why we never hit this issue…

The file was introduced with the mismatch in #330. The lack of unit tests seems to allow a broken default config for 5 minor releases in a row… Let's fix the code at least, can you check the suggested regexp changes I proposed and if they work as expected adjust the PR?

[waipeng]

I have replied on the suggestions you gave, and added a commit with your change. Thanks for looking at this.

I have taken a look at the tests and #344 was done some time ago and the logs are lost. If you can resurrect it, maybe we can all chip in to get tests working again.

[smortex]

Let's give this another try. Can you try these suggestions?

[smortex]

lol

So I should adjust my comment #371 (comment)

The file was introduced with the mismatch in #330. The lack of unit tests seems to allow a broken default config for 5 minor releases in a row… Let's fix the code at least, can you check the suggested regexp changes I proposed and if they work as expected adjust the PR?

s/5 minor releases in a row/6 minor releases in a row/ 🙃

Cc @austb. I feel like you are a bit alone to try to do something with this module, I am working on fixing CI, I'll at-you to be sure you not miss it.

[waipeng]

@smortex your suggestions are good. I've added them to the PR.

[smortex]

Hey @waipeng! I see you replaced <SPACE>+<SPACE> with @ in all fragments but one. Was it on purpose of just missing? I think it's better to use the same construct each time for consistency.

[waipeng]

My fault due to inexperience with Github PR.

Do you have preference on keeping the commits you suggested, or is it OK if I squash them all into one commit and put you as co-author? I am having a hard time tracing through the commits in the current form.

[smortex]

Feel free to squash everything ;-)

[smortex]

LGTM 💯 !

[waipeng]

Hi, can this be merged and a new version be cut please?
Right now it is broken in default config which is not very healthy.

[alaunay]

Hello, no news on this ? permissions are checked every time without this patch. Thanks !

[h0tw1r3]

@waipeng can you rebase on main please?

[smortex]

Somebody fixed the test suite 😱. We need to update the tests according to the code changes 😁

@waipeng the "Files changed" tab shows the offending tests (beware, the view lacks some ... symbols so the diff is not really readable around the failures context), this should help adjusting the test suite to make sure we have the expected code.

Thanks!

[h0tw1r3]

@waipeng tests are failing due to unrelated CI changes. Rebase on main again should get past that.