From 32b65b874af29595af03c40e16d34104cd841dbf Mon Sep 17 00:00:00 2001 From: Chris Price Date: Sat, 27 Oct 2012 18:04:15 -0700 Subject: [PATCH 1/3] Fix tests so that they can be run multiple times One of the spec tests was creating a table to test some user privileges. However, it wasn't dropping the table afterwards, meaning that the same test would fail on the next run because that table already existed. This commit adds a command to drop the table so that the tests can be run several times in sequence. --- spec/postgresql_spec.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/spec/postgresql_spec.rb b/spec/postgresql_spec.rb index aaf0d99..404154b 100755 --- a/spec/postgresql_spec.rb +++ b/spec/postgresql_spec.rb @@ -34,6 +34,7 @@ describe "postgresql" do @logger.debug("Running command: '#{args[0]}'") @env.primary_vm.channel.sudo(args[0]) do |ch, data| @logger.debug(data) + data end end @@ -144,8 +145,10 @@ describe "postgresql" do # Run again to check for idempotence sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") - # Check that the user can select from the table in + # Check that the user can create a table in the database sudo_and_log('sudo -u psql_grant_tester psql --command="create table foo (foo int)" postgres') + + sudo_and_log('sudo -u psql_grant_tester psql --command="drop table foo" postgres') end end end From 8529c1fa4661b8a1a16b40b903aa5de613d3ccb4 Mon Sep 17 00:00:00 2001 From: Chris Price Date: Sat, 27 Oct 2012 18:41:47 -0700 Subject: [PATCH 2/3] Add deprecation warning for postgres::psql This commit adds a deprecation warning for the Exec-based postgres::psql type, and a test to verify that the deprecation warning is being generated. --- manifests/psql.pp | 14 ++++++++++---- spec/postgresql_spec.rb | 19 +++++++++++++++---- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/manifests/psql.pp b/manifests/psql.pp index dbe58bf..77e2642 100644 --- a/manifests/psql.pp +++ b/manifests/psql.pp @@ -26,9 +26,9 @@ define postgresql::psql( require postgresql::params - # TODO: FIXME: shellquote does not work, and this regex works for trivial things but not nested escaping. - # Need a lexer, preferably a ruby SQL parser to catch errors at catalog time - # Possibly https://github.com/omghax/sql ? + # TODO: FIXME: shellquote does not work, and this regex works for trivial + # things but not nested escaping. Need a lexer, preferably a ruby SQL parser + # to catch errors at catalog time. Possibly https://github.com/omghax/sql ? if ($::postgres_default_version != "8.1") { $no_password_option = "--no-password" @@ -38,7 +38,13 @@ define postgresql::psql( $quoted_command = regsubst($command, '"', '\\"') $quoted_unless = regsubst($unless, '"', '\\"') - exec {"/bin/echo \"$quoted_command\" | $psql |egrep -v -q '^$'": + $final_cmd = "/bin/echo \"$quoted_command\" | $psql |egrep -v -q '^$'" + + notify { "deprecation warning: $final_cmd": + message => "postgresql::psql is deprecated ; please use postgresql_psql instead.", + } -> + + exec { $final_cmd: cwd => '/tmp', user => $user, returns => 1, diff --git a/spec/postgresql_spec.rb b/spec/postgresql_spec.rb index 404154b..ceee109 100755 --- a/spec/postgresql_spec.rb +++ b/spec/postgresql_spec.rb @@ -32,10 +32,12 @@ describe "postgresql" do def sudo_and_log(*args) @logger.debug("Running command: '#{args[0]}'") - @env.primary_vm.channel.sudo(args[0]) do |ch, data| - @logger.debug(data) - data - end + result = "" + @env.primary_vm.channel.sudo(args[0]) do |ch, data| + result << data + @logger.debug(data) + end + result end before(:all) do @@ -118,6 +120,15 @@ describe "postgresql" do # Check for exit code 0 sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") end + + it 'should emit a deprecation warning' do + test_class = 'class {"postgresql_tests::test_psql": command => "SELECT * FROM pg_datbase limit 1", unless => "SELECT 1 WHERE 1=1" }' + + data = sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'; [ $? == 2 ]") + + data.should match /postgresql::psql is deprecated/ + + end end describe 'postgresql::user' do From 2a922f104a52050a8ecbdadf5df20a64159b3393 Mon Sep 17 00:00:00 2001 From: Chris Price Date: Sun, 4 Nov 2012 21:47:49 -0800 Subject: [PATCH 3/3] Initial working implementation of ruby psql type/provider This commit provides a working implementation of a ruby type/provider (`postgresql_psql` for handling the PSQL commands. This is a little more flexible than doing it via Exec resources, which is what the `postgresql::psql` type was doing. The old type is still present but now includes a deprecation notification, and all of the other types that were using the `::psql` type have been ported over to use the `postgresql_psql` type instead. --- README.md | 2 +- lib/puppet/provider/postgresql_psql/ruby.rb | 58 ++++++++++++++++ lib/puppet/type/postgresql_psql.rb | 74 +++++++++++++++++++++ manifests/database.pp | 14 ++-- manifests/database_grant.pp | 8 +-- manifests/role.pp | 8 +-- spec/manifests/test_ruby_psql.pp | 30 +++++++++ spec/postgresql_spec.rb | 65 ++++++++++-------- 8 files changed, 215 insertions(+), 44 deletions(-) create mode 100644 lib/puppet/provider/postgresql_psql/ruby.rb create mode 100644 lib/puppet/type/postgresql_psql.rb create mode 100644 spec/manifests/test_ruby_psql.pp diff --git a/README.md b/README.md index 81e225d..dcd5ace 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ This module provides the following defined resource types for managing postgres: And the fallback, analogous to exec resources, only for SQL statements: - * `postgresql::psql` + * `postgresql_psql` Basic usage ----------- diff --git a/lib/puppet/provider/postgresql_psql/ruby.rb b/lib/puppet/provider/postgresql_psql/ruby.rb new file mode 100644 index 0000000..a875d6d --- /dev/null +++ b/lib/puppet/provider/postgresql_psql/ruby.rb @@ -0,0 +1,58 @@ +Puppet::Type.type(:postgresql_psql).provide(:ruby) do + + def command() + if ((! resource[:unless]) or (resource[:unless].empty?)) + if (resource[:refreshonly]) + # So, if there's no 'unless', and we're in "refreshonly" mode, + # we need to return the target command here. If we don't, + # then Puppet will generate an event indicating that this + # property has changed. + return resource[:command] + end + + # if we're not in refreshonly mode, then we return nil, + # which will cause Puppet to sync this property. This + # is what we want if there is no 'unless' value specified. + return nil + end + + output, status = run_unless_sql_command(resource[:unless]) + + if status != 0 + self.fail("Error evaluating 'unless' clause: '#{output}'") + end + result_count = output.strip.to_i + if result_count > 0 + # If the 'unless' query returned rows, then we don't want to execute + # the 'command'. Returning the target 'command' here will cause + # Puppet to treat this property as already being 'insync?', so it + # won't call the setter to run the 'command' later. + return resource[:command] + end + + # Returning 'nil' here will cause Puppet to see this property + # as out-of-sync, so it will call the setter later. + nil + end + + def command=(val) + output, status = run_sql_command(val) + + if status != 0 + self.fail("Error executing SQL; psql returned #{status}: '#{output}'") + end + end + + + def run_unless_sql_command(sql) + # for the 'unless' queries, we wrap the user's query in a 'SELECT COUNT', + # which makes it easier to parse and process the output. + run_sql_command('SELECT COUNT(*) FROM (' << sql << ') count') + end + + def run_sql_command(sql) + Puppet::Util::SUIDManager. + run_and_capture('psql -t -c "' << sql.gsub('"', '\"') << '"', resource[:psql_user], resource[:psql_group]) + end + +end \ No newline at end of file diff --git a/lib/puppet/type/postgresql_psql.rb b/lib/puppet/type/postgresql_psql.rb new file mode 100644 index 0000000..fd7826b --- /dev/null +++ b/lib/puppet/type/postgresql_psql.rb @@ -0,0 +1,74 @@ +Puppet::Type.newtype(:postgresql_psql) do + + newparam(:name) do + desc "An arbitrary tag for your own reference; the name of the message." + isnamevar + end + + newproperty(:command) do + desc 'The SQL command to execute via psql.' + + defaultto { @resource[:name] } + + def sync(refreshing = false) + # We're overriding 'sync' here in order to do some magic + # in support of providing a 'refreshonly' parameter. This + # is kind of hacky because the logic for 'refreshonly' is + # spread between the type and the provider, but this is + # the least horrible way that I could determine to accomplish + # it. + # + # Note that our overridden version of 'sync' takes a parameter, + # 'refreshing', which the parent version doesn't take. This + # allows us to call the sync method directly from the 'refresh' + # method, and then inside of the body of 'sync' we can tell + # whether or not we're refreshing. + + if ((@resource[:refreshonly] == :false) || refreshing) + # If we're not in 'refreshonly' mode, or we're not currently + # refreshing, then we just call the parent method. + super() + else + # If we get here, it means we're in 'refreshonly' mode and + # we're not being called by the 'refresh' method, so we + # just no-op. We'll be called again by the 'refresh' + # method momentarily. + nil + end + end + end + + newparam(:unless) do + desc "An optional SQL command to execute prior to the main :command; " + + "this is generally intended to be used for idempotency, to check " + + "for the existence of an object in the database to determine whether " + + "or not the main SQL command needs to be executed at all." + end + + newparam(:db) do + desc "The name of the database to execute the SQL command against." + end + + newparam(:psql_user) do + desc "The system user account under which the psql command should be executed." + defaultto("postgres") + end + + newparam(:psql_group) do + desc "The system user group account under which the psql command should be executed." + defaultto("postgres") + end + + newparam(:refreshonly) do + desc "If 'true', then the SQL will only be executed via a notify/subscribe event." + + defaultto(:false) + end + + def refresh() + # All of the magic for this type is attached to the ':command' property, so + # we just need to sync it to accomplish a 'refresh'. + self.property(:command).sync(true) + end + +end \ No newline at end of file diff --git a/manifests/database.pp b/manifests/database.pp index e254736..4873516 100644 --- a/manifests/database.pp +++ b/manifests/database.pp @@ -31,19 +31,21 @@ define postgresql::database( $createdb_command = "${postgresql::params::createdb_path} --template=template0 --encoding '$charset' $locale_option '$dbname'" + postgresql_psql { "Check for existence of db '$dbname'": + command => "SELECT 1", + unless => "SELECT datname FROM pg_database WHERE datname='$dbname'", + } ~> + exec { $createdb_command : - unless => "${postgresql::params::psql_path} --command=\"SELECT datname FROM pg_database WHERE datname=\'$dbname\' \" --pset=tuples_only | grep -q $dbname", + refreshonly => true, user => 'postgres', - } + } ~> # This will prevent users from connecting to the database unless they've been # granted privileges. - postgresql::psql {"REVOKE CONNECT ON DATABASE $dbname FROM public": + postgresql_psql {"REVOKE CONNECT ON DATABASE $dbname FROM public": db => 'postgres', - user => 'postgres', - unless => 'SELECT 1 where 1 = 0', refreshonly => true, - subscribe => Exec[$createdb_command], } } diff --git a/manifests/database_grant.pp b/manifests/database_grant.pp index 9dbd02b..6bfe925 100644 --- a/manifests/database_grant.pp +++ b/manifests/database_grant.pp @@ -49,10 +49,10 @@ define postgresql::database_grant( default => $privilege, } - postgresql::psql {"GRANT $privilege ON database $db TO $role": - db => $psql_db, - user => $psql_user, - unless => "SELECT 1 WHERE has_database_privilege('$role', '$db', '$unless_privilege')", + postgresql_psql {"GRANT $privilege ON database $db TO $role": + db => $psql_db, + psql_user => $psql_user, + unless => "SELECT 1 WHERE has_database_privilege('$role', '$db', '$unless_privilege')", } } diff --git a/manifests/role.pp b/manifests/role.pp index 7b840b0..d38033a 100644 --- a/manifests/role.pp +++ b/manifests/role.pp @@ -32,9 +32,9 @@ define postgresql::role( $superuser_sql = $superuser ? { true => 'SUPERUSER' , default => 'NOSUPERUSER' } # TODO: FIXME: Will not correct the superuser / createdb / createrole / login status of a role that already exists - postgresql::psql {"CREATE ROLE ${username} ENCRYPTED PASSWORD '${password_hash}' $login_sql $createrole_sql $createdb_sql $superuser_sql": - db => $db, - user => 'postgres', - unless => "SELECT rolname FROM pg_roles WHERE rolname='$username'", + postgresql_psql {"CREATE ROLE ${username} ENCRYPTED PASSWORD '${password_hash}' $login_sql $createrole_sql $createdb_sql $superuser_sql": + db => $db, + psql_user => 'postgres', + unless => "SELECT rolname FROM pg_roles WHERE rolname='$username'", } } diff --git a/spec/manifests/test_ruby_psql.pp b/spec/manifests/test_ruby_psql.pp new file mode 100644 index 0000000..8262d9e --- /dev/null +++ b/spec/manifests/test_ruby_psql.pp @@ -0,0 +1,30 @@ +# puppet-postgresql +# For all details and documentation: +# http://github.com/inkling/puppet-postgresql +# +# Copyright 2012- Inkling Systems, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +class postgresql_tests::test_ruby_psql($command = $title, $unless) { + + include postgresql::server + + postgresql_psql { $title: + db => 'postgres', + psql_user => 'postgres', + command => $command, + unless => $unless, + require => Class['postgresql::server'], + } +} diff --git a/spec/postgresql_spec.rb b/spec/postgresql_spec.rb index ceee109..f0fabcb 100755 --- a/spec/postgresql_spec.rb +++ b/spec/postgresql_spec.rb @@ -50,7 +50,7 @@ describe "postgresql" do # Sahara ignores :cwd so we have to chdir for now, see https://github.com/jedi4ever/sahara/issues/9 Dir.chdir(vagrant_dir) - # @env.cli("destroy") # Takes too long + @env.cli("destroy --force") # Takes too long @env.cli("up") # We are not testing the "package" resource type, so pull stuff in in advance @@ -88,39 +88,23 @@ describe "postgresql" do # A bare-minimum class to add a DB to postgres, which will be running due to ubuntu test_class = 'class {"postgresql_tests::test_db": db => "postgresql_test_db" }' - - # Run once to check for crashes - sudo_and_log("puppet apply -e '#{test_class}'") - - # Run again to check for idempotence - sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") - # Check that the database name is present - sudo_and_log('sudo -u postgres psql postgresql_test_db --command="select datname from pg_database limit 1"') + begin + # Run once to check for crashes + sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'; [ $? == 2 ]") + + # Run again to check for idempotence + sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") + + # Check that the database name is present + sudo_and_log('sudo -u postgres psql postgresql_test_db --command="select datname from pg_database limit 1"') + ensure + sudo_and_log('sudo -u postgres psql --command="drop database postgresql_test_db" postgres') + end end end describe 'postgresql::psql' do - it 'should run some SQL when the unless query returns no rows' do - test_class = 'class {"postgresql_tests::test_psql": command => "SELECT \'foo\'", unless => "SELECT 1 WHERE 1=2" }' - - # Run once to get all packages set up - sudo_and_log("puppet apply -e '#{test_class}'") - - # Check for exit code 2 - sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}' ; [ $? == 2 ]") - end - - it 'should not run SQL when the unless query returns rows' do - test_class = 'class {"postgresql_tests::test_psql": command => "SELECT * FROM pg_datbase limit 1", unless => "SELECT 1 WHERE 1=1" }' - - # Run once to get all packages set up - sudo_and_log("puppet apply -e '#{test_class}'") - - # Check for exit code 0 - sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") - end - it 'should emit a deprecation warning' do test_class = 'class {"postgresql_tests::test_psql": command => "SELECT * FROM pg_datbase limit 1", unless => "SELECT 1 WHERE 1=1" }' @@ -131,6 +115,29 @@ describe "postgresql" do end end + describe 'postgresql_psql' do + it 'should run some SQL when the unless query returns no rows' do + test_class = 'class {"postgresql_tests::test_ruby_psql": command => "SELECT 1", unless => "SELECT 1 WHERE 1=2" }' + + # Run once to get all packages set up + sudo_and_log("puppet apply -e '#{test_class}'") + + # Check for exit code 2 + sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}' ; [ $? == 2 ]") + end + + it 'should not run SQL when the unless query returns rows' do + test_class = 'class {"postgresql_tests::test_ruby_psql": command => "SELECT * FROM pg_datbase limit 1", unless => "SELECT 1 WHERE 1=1" }' + + # Run once to get all packages set up + sudo_and_log("puppet apply -e '#{test_class}'") + + # Check for exit code 0 + sudo_and_log("puppet apply --detailed-exitcodes -e '#{test_class}'") + end + + end + describe 'postgresql::user' do it 'should idempotently create a user who can log in' do test_class = 'class {"postgresql_tests::test_user": user => "postgresql_test_user", password => "postgresql_test_password" }'