From 927a45593530ef1a33bfe5b03e82784d917889b9 Mon Sep 17 00:00:00 2001 From: Emre Hasegeli Date: Wed, 7 Mar 2018 17:10:37 +0100 Subject: [PATCH 1/4] Improve style of the role resource --- manifests/server/role.pp | 56 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/manifests/server/role.pp b/manifests/server/role.pp index c8db8b334c..2b6b10e72a 100644 --- a/manifests/server/role.pp +++ b/manifests/server/role.pp @@ -40,14 +40,13 @@ } Postgresql_psql { - db => $db, - port => $port_override, - psql_user => $psql_user, - psql_group => $psql_group, - psql_path => $psql_path, + db => $db, + port => $port_override, + psql_user => $psql_user, + psql_group => $psql_group, + psql_path => $psql_path, connect_settings => $connect_settings, - cwd => $module_workdir, - require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], + cwd => $module_workdir, } if $ensure == 'present' { @@ -69,43 +68,50 @@ command => "CREATE ROLE \"${username}\" ${password_sql} ${login_sql} ${createrole_sql} ${createdb_sql} ${superuser_sql} ${replication_sql} CONNECTION LIMIT ${connection_limit}", unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}'", environment => $environment, - require => undef, } - postgresql_psql {"ALTER ROLE \"${username}\" ${superuser_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolsuper = ${superuser}", + postgresql_psql { "ALTER ROLE \"${username}\" ${superuser_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolsuper = ${superuser}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } - postgresql_psql {"ALTER ROLE \"${username}\" ${createdb_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcreatedb = ${createdb}", + postgresql_psql { "ALTER ROLE \"${username}\" ${createdb_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcreatedb = ${createdb}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } - postgresql_psql {"ALTER ROLE \"${username}\" ${createrole_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcreaterole = ${createrole}", + postgresql_psql { "ALTER ROLE \"${username}\" ${createrole_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcreaterole = ${createrole}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } - postgresql_psql {"ALTER ROLE \"${username}\" ${login_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcanlogin = ${login}", + postgresql_psql { "ALTER ROLE \"${username}\" ${login_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolcanlogin = ${login}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } - postgresql_psql {"ALTER ROLE \"${username}\" ${inherit_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolinherit = ${inherit}", + postgresql_psql { "ALTER ROLE \"${username}\" ${inherit_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolinherit = ${inherit}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } if(versioncmp($version, '9.1') >= 0) { if $replication_sql == '' { - postgresql_psql {"ALTER ROLE \"${username}\" NOREPLICATION": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolreplication = ${replication}", + postgresql_psql { "ALTER ROLE \"${username}\" NOREPLICATION": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolreplication = ${replication}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } } else { - postgresql_psql {"ALTER ROLE \"${username}\" ${replication_sql}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolreplication = ${replication}", + postgresql_psql { "ALTER ROLE \"${username}\" ${replication_sql}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolreplication = ${replication}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } } } - postgresql_psql {"ALTER ROLE \"${username}\" CONNECTION LIMIT ${connection_limit}": - unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolconnlimit = ${connection_limit}", + postgresql_psql { "ALTER ROLE \"${username}\" CONNECTION LIMIT ${connection_limit}": + unless => "SELECT 1 FROM pg_roles WHERE rolname = '${username}' AND rolconnlimit = ${connection_limit}", + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } if $password_hash and $update_password { @@ -119,13 +125,13 @@ command => "ALTER ROLE \"${username}\" ${password_sql}", unless => "SELECT 1 FROM pg_shadow WHERE usename = '${username}' AND passwd = '${pwd_hash_sql}'", environment => $environment, + require => Postgresql_psql["CREATE ROLE ${username} ENCRYPTED PASSWORD ****"], } } } else { # ensure == absent postgresql_psql { "DROP ROLE \"${username}\"": onlyif => "SELECT 1 FROM pg_roles WHERE rolname = '${username}'", - require => undef, } } } From c24687b32c2e516a6d7cb56d2d590bf5b539da06 Mon Sep 17 00:00:00 2001 From: Emre Hasegeli Date: Wed, 7 Mar 2018 17:35:30 +0100 Subject: [PATCH 2/4] Remove db argument from the role resource This option doesn't make sense, because roles are not specific to databases on PostgreSQL. It was not documented on the README, so I don't see value in making this backwards compatible. --- manifests/server/role.pp | 4 ++-- spec/acceptance/server/grant_role_spec.rb | 4 ---- spec/acceptance/server/grant_spec.rb | 2 -- spec/acceptance/server/reassign_owned_by_spec.rb | 2 -- 4 files changed, 2 insertions(+), 10 deletions(-) diff --git a/manifests/server/role.pp b/manifests/server/role.pp index 2b6b10e72a..d5fec85ccf 100644 --- a/manifests/server/role.pp +++ b/manifests/server/role.pp @@ -4,7 +4,6 @@ $password_hash = false, $createdb = false, $createrole = false, - $db = $postgresql::server::default_database, $port = undef, $login = true, $inherit = true, @@ -15,6 +14,7 @@ $connect_settings = $postgresql::server::default_connect_settings, Enum['present', 'absent'] $ensure = 'present', ) { + $psql_db = $postgresql::server::default_database $psql_user = $postgresql::server::user $psql_group = $postgresql::server::group $psql_path = $postgresql::server::psql_path @@ -40,7 +40,7 @@ } Postgresql_psql { - db => $db, + db => $psql_db, port => $port_override, psql_user => $psql_user, psql_group => $psql_group, diff --git a/spec/acceptance/server/grant_role_spec.rb b/spec/acceptance/server/grant_role_spec.rb index b8da0ad8f0..ea8c97452a 100644 --- a/spec/acceptance/server/grant_role_spec.rb +++ b/spec/acceptance/server/grant_role_spec.rb @@ -52,7 +52,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: - db => $db, login => false, require => Postgresql::Server::Database[$db], } @@ -106,7 +105,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: - db => $db, login => false, require => Postgresql::Server::Database[$db], } @@ -160,7 +158,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: - db => $db, login => false, require => Postgresql::Server::Database[$db], } @@ -197,7 +194,6 @@ class { 'postgresql::server': } # Create a role to grant to the nonexistent user postgresql::server::role { $group: - db => $db, login => false, require => Postgresql::Server::Database[$db], } diff --git a/spec/acceptance/server/grant_spec.rb b/spec/acceptance/server/grant_spec.rb index c53d5ff685..11b14d03c8 100644 --- a/spec/acceptance/server/grant_spec.rb +++ b/spec/acceptance/server/grant_spec.rb @@ -31,8 +31,6 @@ class { 'postgresql::server': } # Create a user to grant privileges to postgresql::server::role { $user: - db => $db, - require => Postgresql::Server::Database[$db], } # Make a local user for ident auth diff --git a/spec/acceptance/server/reassign_owned_by_spec.rb b/spec/acceptance/server/reassign_owned_by_spec.rb index 44eea11bc7..224a738e60 100644 --- a/spec/acceptance/server/reassign_owned_by_spec.rb +++ b/spec/acceptance/server/reassign_owned_by_spec.rb @@ -27,8 +27,6 @@ class { 'postgresql::server': } # Create a user to reassign ownership to postgresql::server::role { $new_owner: - db => $db, - require => Postgresql::Server::Database[$db], } # Make a local user for ident auth From a51fefe2ed3f9b57c2e2fb17d5816dce5e5a252a Mon Sep 17 00:00:00 2001 From: Emre Hasegeli Date: Thu, 15 Mar 2018 11:21:11 +0100 Subject: [PATCH 3/4] Improve style of the database resource --- manifests/server/database.pp | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/manifests/server/database.pp b/manifests/server/database.pp index 057bc46be6..76fc3617c1 100644 --- a/manifests/server/database.pp +++ b/manifests/server/database.pp @@ -81,13 +81,14 @@ # This will prevent users from connecting to the database unless they've been # granted privileges. - ~> postgresql_psql { "REVOKE ${public_revoke_privilege} ON DATABASE \"${dbname}\" FROM public": + postgresql_psql { "REVOKE ${public_revoke_privilege} ON DATABASE \"${dbname}\" FROM public": refreshonly => true, + subscribe => Postgresql_psql["CREATE DATABASE \"${dbname}\""], } - Postgresql_psql["CREATE DATABASE \"${dbname}\""] - -> postgresql_psql { "UPDATE pg_database SET datistemplate = ${istemplate} WHERE datname = '${dbname}'": - unless => "SELECT 1 FROM pg_database WHERE datname = '${dbname}' AND datistemplate = ${istemplate}", + postgresql_psql { "UPDATE pg_database SET datistemplate = ${istemplate} WHERE datname = '${dbname}'": + unless => "SELECT 1 FROM pg_database WHERE datname = '${dbname}' AND datistemplate = ${istemplate}", + require => Postgresql_psql["CREATE DATABASE \"${dbname}\""], } if $comment { @@ -96,10 +97,11 @@ '8.1' => 'obj_description', default => 'shobj_description', } - Postgresql_psql["CREATE DATABASE \"${dbname}\""] - -> postgresql_psql { "COMMENT ON DATABASE \"${dbname}\" IS '${comment}'": - unless => "SELECT 1 FROM pg_catalog.pg_database d WHERE datname = '${dbname}' AND pg_catalog.${comment_information_function}(d.oid, 'pg_database') = '${comment}'", - db => $dbname, + + postgresql_psql { "COMMENT ON DATABASE \"${dbname}\" IS '${comment}'": + unless => "SELECT 1 FROM pg_catalog.pg_database d WHERE datname = '${dbname}' AND pg_catalog.${comment_information_function}(d.oid, 'pg_database') = '${comment}'", + db => $dbname, + require => Postgresql_psql["CREATE DATABASE \"${dbname}\""], } } @@ -109,9 +111,7 @@ require => Postgresql_psql["CREATE DATABASE \"${dbname}\""], } - if defined(Postgresql::Server::Role[$owner]) { - Postgresql::Server::Role[$owner]->Postgresql_psql["ALTER DATABASE \"${dbname}\" OWNER TO \"${owner}\""] - } + Postgresql::Server::Role <| username == $owner |> -> Postgresql_psql["ALTER DATABASE \"${dbname}\" OWNER TO \"${owner}\""] } if $tablespace { @@ -120,9 +120,6 @@ require => Postgresql_psql["CREATE DATABASE \"${dbname}\""], } - if defined(Postgresql::Server::Tablespace[$tablespace]) { - # The tablespace must be there, before we create the database. - Postgresql::Server::Tablespace[$tablespace]->Postgresql_psql["CREATE DATABASE \"${dbname}\""] - } + Postgresql::Server::Tablespace <| spcname == $tablespace |> -> Postgresql_psql["CREATE DATABASE \"${dbname}\""] } } From 9e7c6a04b70e8b4770f84c6bdd24050a3ddb49d7 Mon Sep 17 00:00:00 2001 From: Emre Hasegeli Date: Thu, 1 Mar 2018 18:22:01 +0100 Subject: [PATCH 4/4] Resolve dependencies to the databases automatically --- manifests/server/database.pp | 7 +++++++ manifests/server/extension.pp | 7 ------- manifests/server/grant.pp | 4 ---- manifests/server/reassign_owned_by.pp | 4 ---- spec/acceptance/server/grant_role_spec.rb | 4 ---- spec/acceptance/server/grant_spec.rb | 5 ----- spec/acceptance/server/reassign_owned_by_spec.rb | 3 +-- spec/acceptance/server/schema_spec.rb | 1 - spec/unit/defines/server/db_spec.rb | 2 +- spec/unit/defines/server/extension_spec.rb | 12 ++++++------ 10 files changed, 15 insertions(+), 34 deletions(-) diff --git a/manifests/server/database.pp b/manifests/server/database.pp index 76fc3617c1..cf4aaaeb42 100644 --- a/manifests/server/database.pp +++ b/manifests/server/database.pp @@ -79,6 +79,13 @@ require => Class['postgresql::server::service'] } + # Automatically require the database for the statements that requires it + # + # XXX: We should probably do everything in this class before. Postgresql_psql resource could + # specify this better, but currently it is impossible to use "autorequire" against custom + # resources. We can improve this when a better way is available. + Postgresql_psql["CREATE DATABASE \"${dbname}\""] -> Postgresql_psql <| db == $dbname |> + # This will prevent users from connecting to the database unless they've been # granted privileges. postgresql_psql { "REVOKE ${public_revoke_privilege} ON DATABASE \"${dbname}\" FROM public": diff --git a/manifests/server/extension.pp b/manifests/server/extension.pp index 9fb100c246..b466fc25be 100644 --- a/manifests/server/extension.pp +++ b/manifests/server/extension.pp @@ -33,13 +33,6 @@ } } - if( $database != 'postgres' ) { - # The database postgres cannot managed by this module, so it is exempt from this dependency - Postgresql_psql { - require => Postgresql::Server::Database[$database], - } - } - postgresql_psql { "${database}: ${command}": psql_user => $user, diff --git a/manifests/server/grant.pp b/manifests/server/grant.pp index 22e1040da7..7a717a766d 100644 --- a/manifests/server/grant.pp +++ b/manifests/server/grant.pp @@ -418,8 +418,4 @@ if($role != undef and defined(Postgresql::Server::Role[$role])) { Postgresql::Server::Role[$role]->Postgresql_psql["grant:${name}"] } - - if($db != undef and defined(Postgresql::Server::Database[$db])) { - Postgresql::Server::Database[$db]->Postgresql_psql["grant:${name}"] - } } diff --git a/manifests/server/reassign_owned_by.pp b/manifests/server/reassign_owned_by.pp index d4d6f5b088..def12be65d 100644 --- a/manifests/server/reassign_owned_by.pp +++ b/manifests/server/reassign_owned_by.pp @@ -53,8 +53,4 @@ if($new_role != undef and defined(Postgresql::Server::Role[$new_role])) { Postgresql::Server::Role[$new_role]->Postgresql_psql["reassign_owned_by:${db}:${sql_command}"] } - - if($db != undef and defined(Postgresql::Server::Database[$db])) { - Postgresql::Server::Database[$db]->Postgresql_psql["reassign_owned_by:${db}:${sql_command}"] - } } diff --git a/spec/acceptance/server/grant_role_spec.rb b/spec/acceptance/server/grant_role_spec.rb index ea8c97452a..e3ec538335 100644 --- a/spec/acceptance/server/grant_role_spec.rb +++ b/spec/acceptance/server/grant_role_spec.rb @@ -53,7 +53,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: login => false, - require => Postgresql::Server::Database[$db], } # Grant the role to the user @@ -106,7 +105,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: login => false, - require => Postgresql::Server::Database[$db], } # Grant the role to the user @@ -159,7 +157,6 @@ class { 'postgresql::server': } # Create a role to grant to the user postgresql::server::role { $group: login => false, - require => Postgresql::Server::Database[$db], } # Grant the role to the user @@ -195,7 +192,6 @@ class { 'postgresql::server': } # Create a role to grant to the nonexistent user postgresql::server::role { $group: login => false, - require => Postgresql::Server::Database[$db], } # Grant the role to the nonexistent user diff --git a/spec/acceptance/server/grant_spec.rb b/spec/acceptance/server/grant_spec.rb index 11b14d03c8..b3209b3900 100644 --- a/spec/acceptance/server/grant_spec.rb +++ b/spec/acceptance/server/grant_spec.rb @@ -58,7 +58,6 @@ class { 'postgresql::server': } db => $db, psql_user => '#{superuser}', unless => "SELECT 1 from pg_language where lanname = 'plpgsql'", - require => Postgresql::Server::Database[$db], } postgresql::server::grant { 'grant usage on plpgsql': @@ -138,7 +137,6 @@ class { 'postgresql::server': } db => $db, psql_user => $owner, unless => "SELECT 1 FROM information_schema.sequences WHERE sequence_name = 'test_seq'", - require => Postgresql::Server::Database[$db], } postgresql::server::grant { 'grant usage on test_seq': @@ -159,7 +157,6 @@ class { 'postgresql::server': } db => $db, psql_user => $owner, unless => "SELECT 1 FROM information_schema.sequences WHERE sequence_name = 'test_seq'", - require => Postgresql::Server::Database[$db], } postgresql::server::grant { 'grant update on test_seq': @@ -224,7 +221,6 @@ class { 'postgresql::server': } db => $db, psql_user => $owner, unless => "SELECT 1 FROM information_schema.sequences WHERE sequence_name = 'test_seq2'", - require => Postgresql::Server::Database[$db], } postgresql::server::grant { 'grant usage on all sequences': @@ -246,7 +242,6 @@ class { 'postgresql::server': } db => $db, psql_user => $owner, unless => "SELECT 1 FROM information_schema.sequences WHERE sequence_name = 'test_seq2'", - require => Postgresql::Server::Database[$db], } postgresql::server::grant { 'grant usage on all sequences': diff --git a/spec/acceptance/server/reassign_owned_by_spec.rb b/spec/acceptance/server/reassign_owned_by_spec.rb index 224a738e60..1a2fe918b9 100644 --- a/spec/acceptance/server/reassign_owned_by_spec.rb +++ b/spec/acceptance/server/reassign_owned_by_spec.rb @@ -78,14 +78,13 @@ class { 'postgresql::server': } db => '#{db}', psql_user => '#{old_owner}', unless => "SELECT tablename FROM pg_catalog.pg_tables WHERE tablename = 'test_tbl'", - require => Postgresql::Server::Database['#{db}'], } postgresql_psql { 'create test sequence': command => 'CREATE SEQUENCE test_seq', db => '#{db}', psql_user => '#{old_owner}', unless => "SELECT relname FROM pg_catalog.pg_class WHERE relkind='S' AND relname = 'test_seq'", - require => [ Postgresql_psql['create test table'], Postgresql::Server::Database['#{db}'] ], + require => Postgresql_psql['create test table'], } MANIFEST end diff --git a/spec/acceptance/server/schema_spec.rb b/spec/acceptance/server/schema_spec.rb index 41fcf0572f..15020297f4 100644 --- a/spec/acceptance/server/schema_spec.rb +++ b/spec/acceptance/server/schema_spec.rb @@ -48,7 +48,6 @@ class { 'postgresql::server': } postgresql::server::schema { $user: db => $db, owner => $user, - require => Postgresql::Server::Database[$db], } MANIFEST end diff --git a/spec/unit/defines/server/db_spec.rb b/spec/unit/defines/server/db_spec.rb index 9dd9ad4eb9..b10466fac9 100644 --- a/spec/unit/defines/server/db_spec.rb +++ b/spec/unit/defines/server/db_spec.rb @@ -32,7 +32,7 @@ it { is_expected.to contain_postgresql__server__db('test') } it { is_expected.to contain_postgresql__server__database('test').with_owner('tester') } - it { is_expected.to contain_postgresql__server__role('test').that_comes_before('Postgresql::Server::Database[test]') } + it { is_expected.to contain_postgresql__server__role('test') } it { is_expected.to contain_postgresql__server__database_grant('GRANT test - ALL - test') } end diff --git a/spec/unit/defines/server/extension_spec.rb b/spec/unit/defines/server/extension_spec.rb index 84bd53de1d..b9b6c9a472 100644 --- a/spec/unit/defines/server/extension_spec.rb +++ b/spec/unit/defines/server/extension_spec.rb @@ -30,7 +30,7 @@ context 'with mandatory arguments only' do it { is_expected.to contain_postgresql_psql('template_postgis: CREATE EXTENSION "postgis"') - .with(db: 'template_postgis', command: 'CREATE EXTENSION "postgis"').that_requires('Postgresql::Server::Database[template_postgis]') + .with(db: 'template_postgis', command: 'CREATE EXTENSION "postgis"') } end @@ -63,7 +63,7 @@ it { is_expected.to contain_postgresql_psql('template_postgis: DROP EXTENSION "postgis"') - .with(db: 'template_postgis', command: 'DROP EXTENSION "postgis"').that_requires('Postgresql::Server::Database[template_postgis]') + .with(db: 'template_postgis', command: 'DROP EXTENSION "postgis"') } it { @@ -78,7 +78,7 @@ it { is_expected.to contain_postgresql_psql('template_postgis: DROP EXTENSION "postgis"') - .with(db: 'template_postgis', command: 'DROP EXTENSION "postgis"').that_requires('Postgresql::Server::Database[template_postgis]') + .with(db: 'template_postgis', command: 'DROP EXTENSION "postgis"') } it { @@ -97,7 +97,7 @@ it { is_expected.to contain_postgresql_psql('template_postgis: ALTER EXTENSION "postgis" UPDATE TO \'99.99.99\'') - .with(db: 'template_postgis', unless: "SELECT 1 FROM pg_extension WHERE extname='postgis' AND extversion='99.99.99'").that_requires('Postgresql::Server::Database[template_postgis]') + .with(db: 'template_postgis', unless: "SELECT 1 FROM pg_extension WHERE extname='postgis' AND extversion='99.99.99'") } end @@ -111,7 +111,7 @@ it { is_expected.to contain_postgresql_psql('template_postgis: ALTER EXTENSION "postgis" UPDATE') .with(db: 'template_postgis', - unless: "SELECT 1 FROM pg_available_extensions WHERE name = 'postgis' AND default_version = installed_version").that_requires('Postgresql::Server::Database[template_postgis]') + unless: "SELECT 1 FROM pg_available_extensions WHERE name = 'postgis' AND default_version = installed_version") } end end @@ -147,7 +147,7 @@ context 'with mandatory arguments only' do it { is_expected.to contain_postgresql_psql('template_postgis2: CREATE EXTENSION "postgis"') - .with(db: 'template_postgis2', command: 'CREATE EXTENSION "postgis"').that_requires('Postgresql::Server::Database[template_postgis2]') + .with(db: 'template_postgis2', command: 'CREATE EXTENSION "postgis"') } end end