Merge pull request voxpupuli#1330 from alexjfisher/ssl_on_fix

Replace `add_listen_directive` with `nginx_version`

README.md

@@ -144,6 +144,13 @@ To create only a HTTPS server, set `ssl => true` and also set `listen_port` to t
 same value as `ssl_port`. Setting these to the same value disables the HTTP server.
 The resulting server will be listening on `ssl_port`.
 
+### Idempotency with nginx 1.15.0 and later
+
+By default, this module might configure the deprecated `ssl on` directive.  When
+you next run puppet, this will be removed since the `nginx_version` fact will now
+be available. To avoid this idempotency issue, you can manually set the base
+class's `nginx_version` parameter.
+
 ### Locations
 
 Locations require specific settings depending on whether they should be included

manifests/init.pp

@@ -24,6 +24,14 @@
 # node default {
 #   include nginx
 # }
+#
+# @param nginx_version
+#   The version of nginx installed (or being installed).
+#   Unfortunately, different versions of nginx may need configuring
+#   differently.  The default is derived from the version of nginx
+#   already installed.  If the fact is unavailable, it defaults to '1.6.0'.
+#   You may need to set this manually to get a working and idempotent
+#   configuration.
 class nginx (
   ### START Nginx Configuration ###
   Variant[Stdlib::Absolutepath, Boolean] $client_body_temp_path = $nginx::params::client_body_temp_path,
@@ -185,7 +193,7 @@
   Hash $nginx_upstreams                                   = {},
   Nginx::UpstreamDefaults $nginx_upstreams_defaults       = {},
   Boolean $purge_passenger_repo                           = true,
-  Boolean $add_listen_directive                           = $nginx::params::add_listen_directive,
+  String[1] $nginx_version                                = pick(fact('nginx_version'), '1.6.0'),
 
   ### END Hiera Lookups ###
 ) inherits nginx::params {

manifests/params.pp

@@ -227,11 +227,5 @@
   $sites_available_group = $_module_parameters['root_group']
   $sites_available_mode  = '0644'
   $super_user            = true
-  if fact('nginx_version') {
-    # enable only for releases that are older than 1.15.0
-    $add_listen_directive = versioncmp(fact('nginx_version'), '1.15.0') < 0
-  } else {
-    $add_listen_directive = true
-  }
   ### END Referenced Variables
 }

manifests/resource/server.pp

@@ -131,7 +131,6 @@
 #   [*error_pages*]                - Hash: setup errors pages, hash key is the http code and hash value the page
 #   [*locations*]                  - Hash of servers resources used by this server
 #   [*locations_defaults*]         - Hash of location default settings
-#   [*add_listen_directive*]       - Boolean to determine if we should add 'ssl on;' to the vhost or not. defaults to true for nginx 1.14 and older, otherwise false
 # Actions:
 #
 # Requires:
@@ -269,7 +268,6 @@
   $error_pages                                                                   = undef,
   Hash $locations                                                                = {},
   Hash $locations_defaults                                                       = {},
-  Boolean $add_listen_directive                                                  = $nginx::add_listen_directive,
 ) {
 
   if ! defined(Class['nginx']) {

spec/acceptance/nginx_mail_spec.rb

@@ -35,4 +35,33 @@ class { 'nginx':
   describe port(465) do
     it { is_expected.to be_listening }
   end
+
+  context 'when configured for nginx 1.14' do
+    it 'runs successfully' do
+      pp = "
+    class { 'nginx':
+      mail          => true,
+      nginx_version => '1.14.0',
+    }
+    nginx::resource::mailhost { 'domain1.example':
+      ensure      => present,
+      auth_http   => 'localhost/cgi-bin/auth',
+      protocol    => 'smtp',
+      listen_port => 587,
+      ssl         => true,
+      ssl_port    => 465,
+      ssl_cert    => '/etc/pki/tls/certs/blah.cert',
+      ssl_key     => '/etc/pki/tls/private/blah.key',
+      xclient     => 'off',
+    }
+      "
+
+      apply_manifest(pp, catch_failures: true)
+    end
+    describe file('/etc/nginx/conf.mail.d/domain1.example.conf') do
+      it 'does\'t contain `ssl` on `listen` line' do
+        is_expected.to contain 'listen                *:465;'
+      end
+    end
+  end
 end

spec/acceptance/nginx_server_spec.rb

@@ -69,7 +69,8 @@ class { 'nginx': }
 
     describe file('/etc/nginx/sites-available/www.puppetlabs.com.conf') do
       it { is_expected.to be_file }
-      it { is_expected.to contain 'ssl on;' }
+      it { is_expected.not_to contain 'ssl on;' } # As of nginx 1.15 (1.16 stable), this will not be set.
+      it { is_expected.to contain 'listen       *:443 ssl;' }
     end
 
     describe file('/etc/nginx/sites-enabled/www.puppetlabs.com.conf') do

spec/defines/resource_mailhost_spec.rb

@@ -485,7 +485,7 @@
               title: 'should set the IPv4 SSL listen port',
               attr: 'ssl_port',
               value: 45,
-              match: '  listen                *:45 ssl;'
+              match: '  listen                *:45;'
             },
             {
               title: 'should enable IPv6',
@@ -600,6 +600,41 @@
               end
             end
           end
+          context 'on nginx 1.16' do
+            let(:params) do
+              {
+                listen_port: 25,
+                ssl_port: 587,
+                ipv6_enable: true,
+                ssl: true,
+                ssl_protocols: 'default-protocols',
+                ssl_ciphers: 'default-ciphers',
+                ssl_cert: 'dummy.crt',
+                ssl_key: 'dummy.key'
+              }
+            end
+
+            context 'when version comes from fact' do
+              let(:facts) do
+                facts.merge(nginx_version: '1.16.0')
+              end
+
+              let(:pre_condition) { ['include ::nginx'] }
+
+              it 'has `ssl` at end of listen directive' do
+                content = catalogue.resource('concat::fragment', "#{title}-ssl").send(:parameters)[:content]
+                expect(content).to include('listen                *:587 ssl;')
+              end
+            end
+            context 'when version comes from parameter' do
+              let(:pre_condition) { ['class { "nginx": nginx_version => "1.16.0"}'] }
+
+              it 'also has `ssl` at end of listen directive' do
+                content = catalogue.resource('concat::fragment', "#{title}-ssl").send(:parameters)[:content]
+                expect(content).to include('listen                *:587 ssl;')
+              end
+            end
+          end
         end
 
         context 'attribute resources' do

templates/mailhost/mailhost.erb

@@ -38,7 +38,7 @@ server {
 <%- end -%>
 <%= scope.function_template(["nginx/mailhost/mailhost_common.erb"]) -%>
 
-<% if @add_listen_directive -%>
+<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%>
   ssl                   off;
 <% end -%>
   starttls              <%= @starttls %>;

templates/mailhost/mailhost_ssl.erb

@@ -20,10 +20,10 @@ server {
 <% end -%>
 <%- if @listen_ip.is_a?(Array) then -%>
   <%- @listen_ip.each do |ip| -%>
-  listen       <%= ip %>:<%= @ssl_port %><% unless @add_listen_directive -%> ssl<% end -%>;
+  listen       <%= ip %>:<%= @ssl_port %><% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) >= 0 -%> ssl<% end -%>;
   <%- end -%>
 <%- else -%>
-  listen                <%= @listen_ip %>:<%= @ssl_port %><% unless @add_listen_directive -%> ssl<% end -%>;
+  listen                <%= @listen_ip %>:<%= @ssl_port %><% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) >= 0 -%> ssl<% end -%>;
 <%- end -%>
 <%# check to see if ipv6 support exists in the kernel before applying -%>
 <%# FIXME this logic is duplicated all over the place -%>
@@ -38,7 +38,7 @@ server {
 <%- end -%>
 <%= scope.function_template(["nginx/mailhost/mailhost_common.erb"]) -%>
 
-<% if @add_listen_directive -%>
+<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%>
   ssl                   on;
 <% end -%>
   starttls              off;

templates/server/server_ssl_settings.erb

@@ -1,4 +1,4 @@
-<% if @add_listen_directive -%>
+<% if scope.call_function('versioncmp', [scope['nginx::nginx_version'], '1.15.0']) < 0 -%>
   ssl on;
 <% end -%>
 <% if @ssl_cert -%>