Merge pull request puppetlabs#517 from cmurphy/fix_datadir

Fix data directory handling

Author: Morgan Haskel

README.md

@@ -335,7 +335,9 @@ Path to your `postgresql.conf` file.
 If false, disables the defaults supplied with the module for `pg\_hba.conf`. This is useful if you disagree with the defaults and wish to override them yourself. Be sure that your changes of course align with the rest of the module, as some access is required to perform basic `psql` operations for example.
 
 ####`datadir`
-This setting can be used to override the default postgresql data directory for the target platform. If not specified, the module will use whatever directory is the default for your OS distro.
+This setting can be used to override the default postgresql data directory for the target platform. If not specified, the module will use whatever directory is the default for your OS distro. Please note that changing the datadir after installation will cause the server to come to a full stop before being able to make the change. For RedHat systems, the data directory must be labeled appropriately for SELinux. On Ubuntu, you need to explicitly set needs\_initdb to true in order to allow Puppet to initialize the database in the new datadir (needs\_initdb defaults to true on other systems).
+
+Warning: If datadir is changed from the default, puppet will not manage purging of the original data directory, which will cause it to fail if the data directory is changed back to the original.
 
 ####`confdir`
 This setting can be used to override the default postgresql configuration directory for the target platform. If not specified, the module will use whatever directory is the default for your OS distro.
@@ -407,7 +409,7 @@ This setting is used to specify the name of the default database to connect with
 This value defaults to `localhost`, meaning the postgres server will only accept connections from localhost. If you'd like to be able to connect to postgres from remote machines, you can override this setting. A value of `*` will tell postgres to accept connections from any remote machine. Alternately, you can specify a comma-separated list of hostnames or IP addresses. (For more info, have a look at the `postgresql.conf` file from your system's postgres package).
 
 ####`port`
-This value defaults to `5432`, meaning the postgres server will listen on TCP port 5432. Note that the same port number is used for all IP addresses the server listens on.
+This value defaults to `5432`, meaning the postgres server will listen on TCP port 5432. Note that the same port number is used for all IP addresses the server listens on. Also note that for RedHat systems and early Debian systems, changing the port will cause the server to come to a full stop before being able to make the change.
 
 ####`ip_mask_deny_postgres_user`
 This value defaults to `0.0.0.0/0`. Sometimes it can be useful to block the superuser account from remote connections if you are allowing other database users to connect remotely. Set this to an IP and mask for which you want to deny connections by the postgres superuser account. So, e.g., the default value of `0.0.0.0/0` will match any remote IP and deny access, so the postgres user won't be able to connect remotely at all. Conversely, a value of `0.0.0.0/32` would not match any remote IP, and thus the deny rule will not be applied and the postgres user will be allowed to connect.

manifests/server/config.pp

@@ -136,4 +136,23 @@
       notify => Class['postgresql::server::reload'],
     }
   }
+
+  if $::osfamily == 'RedHat' {
+    if $::operatingsystemrelease =~ /^7/ or $::operatingsystem == 'Fedora' {
+      file { 'systemd-override':
+        ensure  => present,
+        path    => '/etc/systemd/system/postgresql.service',
+        owner   => root,
+        group   => root,
+        content => template('postgresql/systemd-override.erb'),
+        notify  => [ Exec['restart-systemd'], Class['postgresql::server::service'] ],
+        before  => Class['postgresql::server::reload'],
+      }
+      exec { "restart-systemd":
+        command     => 'systemctl daemon-reload',
+        refreshonly => true,
+        path        => '/bin:/usr/bin:/usr/local/bin'
+      }
+    }
+  }
 }

manifests/server/config_entry.pp

@@ -30,47 +30,38 @@
     }
   }
 
-  # We have to handle ports in a weird and special way.  On early Debian and
-  # Ubuntu we have to ensure we stop the service completely. On Redhat we
-  # either have to create a systemd override for the port or update the
-  # sysconfig file.
+  # We have to handle ports and the data directory in a weird and
+  # special way.  On early Debian and Ubuntu and RHEL we have to ensure
+  # we stop the service completely. On RHEL 7 we either have to create
+  # a systemd override for the port or update the sysconfig file, but this
+  # is managed for us in postgresql::server::config.
   if $::operatingsystem == 'Debian' or $::operatingsystem == 'Ubuntu' {
-    if $::operatingsystemrelease =~ /^6/ or $::operatingsystemrelease =~ /^10\.04/ {
-      if $name == 'port' {
-        exec { 'postgresql_stop':
+    if $name == 'port' and ( $::operatingsystemrelease =~ /^6/ or $::operatingsystemrelease =~ /^10\.04/ ) {
+        exec { "postgresql_stop_${name}":
           command => "service ${::postgresql::server::service_name} stop",
           onlyif  => "service ${::postgresql::server::service_name} status",
           unless  => "grep 'port = ${value}' ${::postgresql::server::postgresql_conf_path}",
           path    => '/usr/sbin:/sbin:/bin:/usr/bin:/usr/local/bin',
           before  => Postgresql_conf[$name],
         }
+    }
+    elsif $name == 'data_directory' {
+      exec { "postgresql_stop_${name}":
+        command => "service ${::postgresql::server::service_name} stop",
+        onlyif  => "service ${::postgresql::server::service_name} status",
+        unless  => "grep \"data_directory = '${value}'\" ${::postgresql::server::postgresql_conf_path}",
+        path    => '/usr/sbin:/sbin:/bin:/usr/bin:/usr/local/bin',
+        before  => Postgresql_conf[$name],
       }
     }
   }
   if $::osfamily == 'RedHat' {
-    if $::operatingsystemrelease =~ /^7/ or $::operatingsystem == 'Fedora' {
-      if $name == 'port' {
-        file { 'systemd-port-override':
-          ensure  => present,
-          path    => '/etc/systemd/system/postgresql.service',
-          owner   => root,
-          group   => root,
-          content => template('postgresql/systemd-port-override.erb'),
-          notify  => [ Exec['restart-systemd'], Class['postgresql::server::service'] ],
-          before  => Class['postgresql::server::reload'],
-        }
-        exec { 'restart-systemd':
-          command     => 'systemctl daemon-reload',
-          refreshonly => true,
-          path        => '/bin:/usr/bin:/usr/local/bin'
-        }
-      }
-    } else {
+    if ! ($::operatingsystemrelease =~ /^7/ or $::operatingsystem == 'Fedora') {
       if $name == 'port' {
         # We need to force postgresql to stop before updating the port
         # because puppet becomes confused and is unable to manage the
         # service appropriately.
-        exec { 'postgresql_stop':
+        exec { "postgresql_stop_${name}":
           command => "service ${::postgresql::server::service_name} stop",
           onlyif  => "service ${::postgresql::server::service_name} status",
           unless  => "grep 'PGPORT=${value}' /etc/sysconfig/pgsql/postgresql",
@@ -86,26 +77,24 @@
           notify  => Class['postgresql::server::service'],
           before  => Class['postgresql::server::reload'],
         }
-      } else {
-        if $name == 'data_directory' {
-          # We need to force postgresql to stop before updating the data directory
-          # otherwise init script breaks
-          exec { "postgresql_${name}":
-            command => "service ${::postgresql::server::service_name} stop",
-            onlyif  => "service ${::postgresql::server::service_name} status",
-            unless  => "grep 'PGDATA=${value}' /etc/sysconfig/pgsql/postgresql",
-            path    => '/sbin:/bin:/usr/bin:/usr/local/bin',
-            require => File['/etc/sysconfig/pgsql/postgresql'],
-          } ->
-          augeas { 'override PGDATA in /etc/sysconfig/pgsql/postgresql':
-            lens    => 'Shellvars.lns',
-            incl    => '/etc/sysconfig/pgsql/*',
-            context => '/files/etc/sysconfig/pgsql/postgresql',
-            changes => "set PGDATA ${value}",
-            require => File['/etc/sysconfig/pgsql/postgresql'],
-            notify  => Class['postgresql::server::service'],
-            before  => Class['postgresql::server::reload'],
-          }
+      } elsif $name == 'data_directory' {
+        # We need to force postgresql to stop before updating the data directory
+        # otherwise init script breaks
+        exec { "postgresql_${name}":
+          command => "service ${::postgresql::server::service_name} stop",
+          onlyif  => "service ${::postgresql::server::service_name} status",
+          unless  => "grep 'PGDATA=${value}' /etc/sysconfig/pgsql/postgresql",
+          path    => '/sbin:/bin:/usr/bin:/usr/local/bin',
+          require => File['/etc/sysconfig/pgsql/postgresql'],
+        } ->
+        augeas { 'override PGDATA in /etc/sysconfig/pgsql/postgresql':
+          lens    => 'Shellvars.lns',
+          incl    => '/etc/sysconfig/pgsql/*',
+          context => '/files/etc/sysconfig/pgsql/postgresql',
+          changes => "set PGDATA ${value}",
+          require => File['/etc/sysconfig/pgsql/postgresql'],
+          notify  => Class['postgresql::server::service'],
+          before  => Class['postgresql::server::reload'],
         }
       }
     }

manifests/server/initdb.pp

@@ -62,5 +62,23 @@
       logoutput => on_failure,
       require   => File[$require_before_initdb],
     }
+    # The package will take care of this for us the first time, but if we
+    # ever need to init a new db we need to make these links explicitly
+    if $::operatingsystem == 'Debian' or $::operatingsystem == 'Ubuntu' {
+      if $::operatingsystemrelease =~ /^6/ or $::operatingsystemrelease =~ /^7/ or $::operatingsystemrelease =~ /^10\.04/ or $::operatingsystemrelease =~ /^12\.04/ {
+        file { 'server.crt':
+          ensure => link,
+          path   => "${datadir}/server.crt",
+          target => '/etc/ssl/certs/ssl-cert-snakeoil.pem',
+          require   => Exec['postgresql_initdb'],
+        }
+        file { 'server.key':
+          ensure => link,
+          path   => "${datadir}/server.key",
+          target => '/etc/ssl/private/ssl-cert-snakeoil.key',
+          require   => Exec['postgresql_initdb'],
+        }
+      }
+    }
   }
 }

spec/acceptance/alternative_port_spec.rb

@@ -5,19 +5,19 @@
 describe 'postgres::server', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
   it 'on an alternative port' do
     pp = <<-EOS
-      class { 'postgresql::server': port => '5433' }
+      class { 'postgresql::server': port => '55433' }
     EOS
 
     apply_manifest(pp, :catch_failures => true)
     apply_manifest(pp, :catch_changes => true)
   end
 
-  describe port(5433) do
+  describe port(55433) do
     it { is_expected.to be_listening }
   end
 
   it 'can connect with psql' do
-    psql('-p 5433 --command="\l" postgres', 'postgres') do |r|
+    psql('-p 55433 --command="\l" postgres', 'postgres') do |r|
       expect(r.stdout).to match(/List of databases/)
     end
   end

spec/acceptance/alternative_pgdata_spec.rb renamed to spec/acceptance/z_alternative_pgdata_spec.rb

@@ -2,18 +2,27 @@
 
 # These tests ensure that postgres can change itself to an alternative pgdata
 # location properly.
+
+# Allow postgresql to use /tmp/* as a datadir
+if fact('osfamily') == 'RedHat'
+  shell("setenforce 0")
+end
+
 describe 'postgres::server', :unless => UNSUPPORTED_PLATFORMS.include?(fact('osfamily')) do
   it 'on an alternative pgdata location' do
     pp = <<-EOS
-      class { 'postgresql::server': datadir => '/var/pgsql' }
+      #file { '/var/lib/pgsql': ensure => directory, } ->
+      # needs_initdb will be true by default for all OS's except Debian
+      # in order to change the datadir we need to tell it explicitly to call initdb
+      class { 'postgresql::server': datadir => '/tmp/data', needs_initdb => true }
     EOS
 
     apply_manifest(pp, :catch_failures => true)
     apply_manifest(pp, :catch_changes => true)
   end
-  
-  describe "Alternate Directory" do
-    File.directory?("/var/pgsql").should be true
+
+  describe file('/tmp/data') do
+    it { should be_directory }
   end
 
   it 'can connect with psql' do

spec/unit/defines/server/config_entry_spec.rb

@@ -44,7 +44,7 @@
       let(:params) {{ :ensure => 'present', :name => 'port_spec', :value => '5432' }}
 
       it 'stops postgresql and changes the port' do
-        is_expected.to contain_exec('postgresql_stop')
+        is_expected.to contain_exec('postgresql_stop_port')
         is_expected.to contain_augeas('override PGPORT in /etc/sysconfig/pgsql/postgresql')
       end
     end
@@ -63,7 +63,7 @@
       let(:params) {{ :ensure => 'present', :name => 'port_spec', :value => '5432' }}
 
       it 'stops postgresql and changes the port' do
-        is_expected.to contain_file('systemd-port-override')
+        is_expected.to contain_file('systemd-override')
         is_expected.to contain_exec('restart-systemd')
       end
     end
@@ -82,7 +82,7 @@
       let(:params) {{ :ensure => 'present', :name => 'port_spec', :value => '5432' }}
 
       it 'stops postgresql and changes the port' do
-        is_expected.to contain_file('systemd-port-override')
+        is_expected.to contain_file('systemd-override')
         is_expected.to contain_exec('restart-systemd')
       end
     end

templates/systemd-override.erb

@@ -0,0 +1,4 @@
+.include /lib/systemd/system/postgresql.service
+[Service]
+Environment=PGPORT=<%= @port %>
+Environment=PGDATA=<%= @datadir %>